On Fri, Sep 22, 2023 at 11:43:23AM +0200, David Marchand wrote: > On Fri, Sep 15, 2023 at 4:35 PM Ferruh Yigit <ferruh.yi...@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.yi...@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: sta...@dpdk.org > > >>> > > >>> Signed-off-by: Selwin Sebastian <selwin.sebast...@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 ;-)). > For this particular change, it seem the only issue with the non-x86 builds is the calls to __cpuid. Would the following simple tweak work, without getting overly complex into adding EAL functions for generic cpu identification. Replace the simple #include <cpuid.h> with:
#ifdef RTE_ARCH_X86 #include <cpuid.h> #else #define __cpuid(n, a, b, c, d) do { a = b = c = d =n; } while(0) #endif [Obviously, the contents of the do-while can be modified depending on what the compiler looks for in terms of modifying vars.] Regards, /Bruce