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

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

[Selwin Sebastian]

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>
+
 static int eth_axgbe_dev_init(struct rte_eth_dev *eth_dev);
 static int  axgbe_dev_configure(struct rte_eth_dev *dev);
 static int  axgbe_dev_start(struct rte_eth_dev *dev);
@@ -172,9 +174,14 @@ static const struct axgbe_xstats axgbe_xstats_strings[] = {
 
 /* The set of PCI devices this driver supports */
 #define AMD_PCI_VENDOR_ID       0x1022
-#define AMD_PCI_RV_ROOT_COMPLEX_ID	0x15d0
-#define AMD_PCI_YC_ROOT_COMPLEX_ID	0x14b5
-#define AMD_PCI_SNOWY_ROOT_COMPLEX_ID	0x1450
+
+#define	Fam17h	0x17
+#define	Fam19h	0x19
+
+#define	CPUID_VENDOR_AuthenticAMD_ebx	0x68747541
+#define	CPUID_VENDOR_AuthenticAMD_ecx	0x444d4163
+#define	CPUID_VENDOR_AuthenticAMD_edx	0x69746e65
+
 #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458
 #define AMD_PCI_AXGBE_DEVICE_V2B 0x1459
 
@@ -2111,29 +2118,6 @@ static void axgbe_default_config(struct axgbe_port *pdata)
 	pdata->power_down = 0;
 }
 
-/*
- * Return PCI root complex device id on success else 0
- */
-static uint16_t
-get_pci_rc_devid(void)
-{
-	char pci_sysfs[PATH_MAX];
-	const struct rte_pci_addr pci_rc_addr = {0, 0, 0, 0};
-	unsigned long device_id;
-
-	snprintf(pci_sysfs, sizeof(pci_sysfs), "%s/" PCI_PRI_FMT "/device",
-		 rte_pci_get_sysfs_path(), pci_rc_addr.domain,
-		 pci_rc_addr.bus, pci_rc_addr.devid, pci_rc_addr.function);
-
-	/* get device id */
-	if (eal_parse_sysfs_value(pci_sysfs, &device_id) < 0) {
-		PMD_INIT_LOG(ERR, "Error in reading PCI sysfs\n");
-		return 0;
-	}
-
-	return (uint16_t)device_id;
-}
-
 /* Used in dev_start by primary process and then
  * in dev_init by secondary process when attaching to an existing ethdev.
  */
@@ -2186,6 +2170,9 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
 	uint32_t len;
 	int ret;
 
+	unsigned int eax = 0, ebx = 0, ecx = 0, edx = 0;
+	unsigned char cpu_family = 0, cpu_model = 0;
+
 	eth_dev->dev_ops = &axgbe_eth_dev_ops;
 
 	eth_dev->rx_descriptor_status = axgbe_dev_rx_descriptor_status;
@@ -2230,26 +2217,55 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
 		pdata->vdata = &axgbe_v2b;
 
 	/*
-	 * Use PCI root complex device ID to identify the CPU
+	 * Use CPUID to get Family and model ID to identify the CPU
 	 */
-	switch (get_pci_rc_devid()) {
-	case AMD_PCI_RV_ROOT_COMPLEX_ID:
-		pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
-		pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
-		break;
-	case AMD_PCI_YC_ROOT_COMPLEX_ID:
-		pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF;
-		pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT;
-		/* Yellow Carp devices do not need cdr workaround */
-		pdata->vdata->an_cdr_workaround = 0;
+	__cpuid(0x0, eax, ebx, ecx, edx);
+
+	if (ebx == CPUID_VENDOR_AuthenticAMD_ebx &&
+		edx == CPUID_VENDOR_AuthenticAMD_edx &&
+		ecx == CPUID_VENDOR_AuthenticAMD_ecx) {
+		int unknown_cpu = 0;
+		eax = 0, ebx = 0, ecx = 0, edx = 0;
+
+		__cpuid(0x1, eax, ebx, ecx, edx);
+
+		cpu_family = ((GET_BITS(eax, 8, 4)) + (GET_BITS(eax, 20, 8)));
+		cpu_model = ((GET_BITS(eax, 4, 4)) | (((GET_BITS(eax, 16, 4)) << 4) & 0xF0));
+
+		switch (cpu_family) {
+		case Fam17h:
+		/* V1000/R1000 */
+		if (cpu_model >= 0x10 && cpu_model <= 0x1F) {
+			pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
+			pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
+		/* EPYC 3000 */
+		} else if (cpu_model >= 0x01 && cpu_model <= 0x0F) {
+			pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
+			pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
+		} else {
+			unknown_cpu = 1;
+		}
 		break;
-	case AMD_PCI_SNOWY_ROOT_COMPLEX_ID:
-		pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
-		pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
+		case Fam19h:
+		/* V3000 (Yellow Carp) */
+		if (cpu_model >= 0x44 && cpu_model <= 0x47) {
+			pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF;
+			pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT;
+
+			/* Yellow Carp devices do not need cdr workaround */
+			pdata->vdata->an_cdr_workaround = 0;
+		} else {
+			unknown_cpu = 1;
+		}
 		break;
-	default:
-		PMD_DRV_LOG(ERR, "No supported devices found\n");
-		return -ENODEV;
+		default:
+			unknown_cpu = 1;
+			break;
+		}
+		if (unknown_cpu) {
+			PMD_DRV_LOG(ERR, "Unknown CPU family, no supported axgbe device found\n");
+			return -ENODEV;
+		}
 	}
 
 	/* Configure the PCS indirect addressing support */
-- 
2.34.1

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

[Ferruh Yigit]

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?


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?

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

[David Marchand]

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?



-- 
David Marchand

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

[Ferruh Yigit]

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.

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

[David Marchand]

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

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

[Bruce Richardson]

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.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 ;-)).
> 
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