Re: [PATCH v1] net/axgbe: use CPUID to identify cpu

[David Marchand <david.marchand@redhat.com>]

On Fri, Sep 15, 2023 at 4:35 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 9/15/2023 2:02 PM, David Marchand wrote:
> > On Fri, Sep 15, 2023 at 12:54 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >> On 8/31/2023 1:31 PM, Selwin Sebastian wrote:
> >>> Using root complex to identify cpu will not work for vm passthrough.
> >>> CPUID is used to get family and modelid to identify cpu
> >>>
> >>> Fixes: b0db927b5eba ("net/axgbe: use PCI root complex device to distinguish device")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
> >>> ---
> >>>  drivers/net/axgbe/axgbe_ethdev.c | 102 ++++++++++++++++++-------------
> >>>  1 file changed, 59 insertions(+), 43 deletions(-)
> >>>
> >>> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> >>> index 48714eebe6..59f5d713d0 100644
> >>> --- a/drivers/net/axgbe/axgbe_ethdev.c
> >>> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> >>> @@ -12,6 +12,8 @@
> >>>
> >>>  #include "eal_filesystem.h"
> >>>
> >>> +#include <cpuid.h>
> >>> +
> >>>
> >>
> >> This patch cause build errors for some non x86 architecture, because of
> >> 'cpuid.h'.
> >>
> >> There is already a 'rte_cpuid.h' file that includes 'cpuid.h' and it is
> >> x86 only file.
> >>
> >> @Selwin, does it makes sense to implement the feature you are trying to
> >> get in eal/x86 level and use that API in the driver?
> >
> > This driver was expected to compile on all arches.
> > The meson.build seems to show an intention of compiling/working on non
> > x86 arch...
> >
> > On the other hand (and if I understand correctly the runtime check),
> > it was never expected to work with anything but a AMD PCI root
> > complex.
> >
> >
> >>
> >>
> >> For those eal/x86 APIs, they will be missing in other architectures,
> >>
> >> @David which one is better, to implement APIs for other architectures
> >> but those just return error, or restrict driver build to x86?
> >
> > We gain compilation coverage, but if the vendor itself refuses runtime
> > on anything but its platform... I don't think we should bother with
> > this.
> > Did I miss something?
> >
>
> It is embedded device, so it will only run on x86.
>
> Current meson config doesn't restrict it to x86 (existing x86 check is
> only for vectorization code), but we can restrict if we want to.
>
>
> One option is to restrict driver to x86 arch, and have the local
> '__cpuid()',
>
> other option is move __cpuid() support to eal x86 specific area, so that
> other components also can benefit from it as well as the driver, similar
> to the getting CPU flags code, cpuid() is to get some information from
> CPU, so it is a generic thing, not specific to driver.
>
>
> I think second option is better, but that requires managing this for
> other archs, my question was related to it.

Ok, this patch wants to make sure the CPU vendor is AMD and adjust its
internal configuration based on this AMD processor family.
And as you mentionned, there is a (ugly asm) piece of code too in mlx5
that required identifying a Intel processor family.

I am unclear whether defining a cross arch API for identifying cpu
model details is doable but I suppose it won't be a quick thing to
define.
I came up with a rather simple API, posted it and Cc'd other arch maintainers.
https://patchwork.dpdk.org/project/dpdk/list/?series=29605&state=%2A&archive=both

Comments welcome (but not too many, please ;-)).


-- 
David Marchand