* [PATCH] eal/x86: add vendor ID checks for specific implementation @ 2023-11-09 5:28 Sivaprasad Tummala 2023-11-22 11:25 ` Ferruh Yigit 2023-11-23 7:27 ` [PATCH v2] eal/x86: add AMD vendor check to choose TSC calibration Sivaprasad Tummala 0 siblings, 2 replies; 6+ messages in thread From: Sivaprasad Tummala @ 2023-11-09 5:28 UTC (permalink / raw) To: bruce.richardson, konstantin.v.ananyev; +Cc: dev, ferruh.yigit Current get_tsc_freq_arch() implementation is specific for Intel processors. Added vendor checks to gracefully return on AMD EPYC processors. Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> --- lib/eal/x86/rte_cycles.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/eal/x86/rte_cycles.c b/lib/eal/x86/rte_cycles.c index 69ed59b4f0..f147a5231d 100644 --- a/lib/eal/x86/rte_cycles.c +++ b/lib/eal/x86/rte_cycles.c @@ -10,6 +10,10 @@ #include <cpuid.h> #endif +#define x86_vendor_amd(t1, t2, t3) \ + ((t1 == 0x68747541) && /* htuA */ \ + (t2 == 0x444d4163) && /* DMAc */ \ + (t3 == 0x69746e65)) /* itne */ #include "eal_private.h" @@ -110,6 +114,18 @@ get_tsc_freq_arch(void) uint8_t mult, model; int32_t ret; +#ifdef RTE_TOOLCHAIN_MSVC + __cpuid(cpuinfo, 0); + a = cpuinfo[0]; + b = cpuinfo[1]; + c = cpuinfo[2]; + d = cpuinfo[3]; +#else + __cpuid(0, a, b, c, d); +#endif + if (x86_vendor_amd(b, c, d)) + return 0; + /* * Time Stamp Counter and Nominal Core Crystal Clock * Information Leaf -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] eal/x86: add vendor ID checks for specific implementation 2023-11-09 5:28 [PATCH] eal/x86: add vendor ID checks for specific implementation Sivaprasad Tummala @ 2023-11-22 11:25 ` Ferruh Yigit 2023-11-22 17:45 ` Tummala, Sivaprasad 2023-11-23 7:27 ` [PATCH v2] eal/x86: add AMD vendor check to choose TSC calibration Sivaprasad Tummala 1 sibling, 1 reply; 6+ messages in thread From: Ferruh Yigit @ 2023-11-22 11:25 UTC (permalink / raw) To: Sivaprasad Tummala, bruce.richardson, konstantin.v.ananyev Cc: dev, David Marchand On 11/9/2023 5:28 AM, Sivaprasad Tummala wrote: > Current get_tsc_freq_arch() implementation is specific for > Intel processors. > > Added vendor checks to gracefully return on AMD EPYC processors. > Hi Siva, Is this fixing an issue in AMD platform, if so can you please describe the impact of the issue and add fixes tag? > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> > --- > lib/eal/x86/rte_cycles.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/lib/eal/x86/rte_cycles.c b/lib/eal/x86/rte_cycles.c > index 69ed59b4f0..f147a5231d 100644 > --- a/lib/eal/x86/rte_cycles.c > +++ b/lib/eal/x86/rte_cycles.c > @@ -10,6 +10,10 @@ > #include <cpuid.h> > #endif > > +#define x86_vendor_amd(t1, t2, t3) \ > + ((t1 == 0x68747541) && /* htuA */ \ > + (t2 == 0x444d4163) && /* DMAc */ \ > + (t3 == 0x69746e65)) /* itne */ > > #include "eal_private.h" > > @@ -110,6 +114,18 @@ get_tsc_freq_arch(void) > uint8_t mult, model; > int32_t ret; > > +#ifdef RTE_TOOLCHAIN_MSVC > + __cpuid(cpuinfo, 0); > We already discussed in the past to abstract the cpuid(), even David sent a patch for it [1]. If this is customer facing issue, OK to get it as it is for the release, but in long term I think better idea to switch to abstract. [1] https://patchwork.dpdk.org/project/dpdk/list/?series=29605&state=* > + a = cpuinfo[0]; > + b = cpuinfo[1]; > + c = cpuinfo[2]; > + d = cpuinfo[3]; > +#else > + __cpuid(0, a, b, c, d); > +#endif > + if (x86_vendor_amd(b, c, d)) > + return 0; > + > /* > * Time Stamp Counter and Nominal Core Crystal Clock > * Information Leaf ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] eal/x86: add vendor ID checks for specific implementation 2023-11-22 11:25 ` Ferruh Yigit @ 2023-11-22 17:45 ` Tummala, Sivaprasad 0 siblings, 0 replies; 6+ messages in thread From: Tummala, Sivaprasad @ 2023-11-22 17:45 UTC (permalink / raw) To: Yigit, Ferruh, bruce.richardson, konstantin.v.ananyev; +Cc: dev, David Marchand [AMD Official Use Only - General] Hi Ferruh, > -----Original Message----- > From: Yigit, Ferruh <Ferruh.Yigit@amd.com> > Sent: Wednesday, November 22, 2023 4:56 PM > To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>; > bruce.richardson@intel.com; konstantin.v.ananyev@yandex.ru > Cc: dev@dpdk.org; David Marchand <david.marchand@redhat.com> > Subject: Re: [PATCH] eal/x86: add vendor ID checks for specific implementation > > On 11/9/2023 5:28 AM, Sivaprasad Tummala wrote: > > Current get_tsc_freq_arch() implementation is specific for Intel > > processors. > > > > Added vendor checks to gracefully return on AMD EPYC processors. > > > > Hi Siva, > > Is this fixing an issue in AMD platform, if so can you please describe the impact of > the issue and add fixes tag? Yes, this patch fixes incorrect TSC issue on AMD platforms. Sure, will add the fixes tag in next version. > > > > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> > > --- > > lib/eal/x86/rte_cycles.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/lib/eal/x86/rte_cycles.c b/lib/eal/x86/rte_cycles.c index > > 69ed59b4f0..f147a5231d 100644 > > --- a/lib/eal/x86/rte_cycles.c > > +++ b/lib/eal/x86/rte_cycles.c > > @@ -10,6 +10,10 @@ > > #include <cpuid.h> > > #endif > > > > +#define x86_vendor_amd(t1, t2, t3) \ > > + ((t1 == 0x68747541) && /* htuA */ \ > > + (t2 == 0x444d4163) && /* DMAc */ \ > > + (t3 == 0x69746e65)) /* itne */ > > > > #include "eal_private.h" > > > > @@ -110,6 +114,18 @@ get_tsc_freq_arch(void) > > uint8_t mult, model; > > int32_t ret; > > > > +#ifdef RTE_TOOLCHAIN_MSVC > > + __cpuid(cpuinfo, 0); > > > > We already discussed in the past to abstract the cpuid(), even David sent a patch > for it [1]. > > If this is customer facing issue, OK to get it as it is for the release, but in long term > I think better idea to switch to abstract. Yes, this fixes a customer issue. Sure, will look into this in the long term as the below patch series in "Deferred" > > > [1] > https://patchwork.dpdk.org/project/dpdk/list/?series=29605&state=* > > > > + a = cpuinfo[0]; > > + b = cpuinfo[1]; > > + c = cpuinfo[2]; > > + d = cpuinfo[3]; > > +#else > > + __cpuid(0, a, b, c, d); > > +#endif > > + if (x86_vendor_amd(b, c, d)) > > + return 0; > > + > > /* > > * Time Stamp Counter and Nominal Core Crystal Clock > > * Information Leaf ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] eal/x86: add AMD vendor check to choose TSC calibration 2023-11-09 5:28 [PATCH] eal/x86: add vendor ID checks for specific implementation Sivaprasad Tummala 2023-11-22 11:25 ` Ferruh Yigit @ 2023-11-23 7:27 ` Sivaprasad Tummala 2023-11-23 10:29 ` Ferruh Yigit 1 sibling, 1 reply; 6+ messages in thread From: Sivaprasad Tummala @ 2023-11-23 7:27 UTC (permalink / raw) To: bruce.richardson, konstantin.v.ananyev Cc: dev, david.marchand, ferruh.yigit, jerin.jacob, stable AMD Epyc processors doesn't support get_tsc_freq_arch(). The patch allows graceful return to allow fallback to alternate TSC calibration. Fixes: 3dbc565e81a0 ("timer: honor arch-specific TSC frequency query") Cc: jerin.jacob@caviumnetworks.com Cc: stable@dpdk.org Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> --- lib/eal/x86/rte_cycles.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/eal/x86/rte_cycles.c b/lib/eal/x86/rte_cycles.c index 69ed59b4f0..f147a5231d 100644 --- a/lib/eal/x86/rte_cycles.c +++ b/lib/eal/x86/rte_cycles.c @@ -10,6 +10,10 @@ #include <cpuid.h> #endif +#define x86_vendor_amd(t1, t2, t3) \ + ((t1 == 0x68747541) && /* htuA */ \ + (t2 == 0x444d4163) && /* DMAc */ \ + (t3 == 0x69746e65)) /* itne */ #include "eal_private.h" @@ -110,6 +114,18 @@ get_tsc_freq_arch(void) uint8_t mult, model; int32_t ret; +#ifdef RTE_TOOLCHAIN_MSVC + __cpuid(cpuinfo, 0); + a = cpuinfo[0]; + b = cpuinfo[1]; + c = cpuinfo[2]; + d = cpuinfo[3]; +#else + __cpuid(0, a, b, c, d); +#endif + if (x86_vendor_amd(b, c, d)) + return 0; + /* * Time Stamp Counter and Nominal Core Crystal Clock * Information Leaf -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] eal/x86: add AMD vendor check to choose TSC calibration 2023-11-23 7:27 ` [PATCH v2] eal/x86: add AMD vendor check to choose TSC calibration Sivaprasad Tummala @ 2023-11-23 10:29 ` Ferruh Yigit 2024-02-13 15:14 ` Thomas Monjalon 0 siblings, 1 reply; 6+ messages in thread From: Ferruh Yigit @ 2023-11-23 10:29 UTC (permalink / raw) To: Sivaprasad Tummala, bruce.richardson, konstantin.v.ananyev Cc: dev, david.marchand, jerin.jacob, stable On 11/23/2023 7:27 AM, Sivaprasad Tummala wrote: > AMD Epyc processors doesn't support get_tsc_freq_arch(). > The patch allows graceful return to allow fallback to > alternate TSC calibration. > > Fixes: 3dbc565e81a0 ("timer: honor arch-specific TSC frequency query") > Cc: jerin.jacob@caviumnetworks.com > Cc: stable@dpdk.org > > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> > Acked-by: Ferruh Yigit <ferruh.yigit@amd.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] eal/x86: add AMD vendor check to choose TSC calibration 2023-11-23 10:29 ` Ferruh Yigit @ 2024-02-13 15:14 ` Thomas Monjalon 0 siblings, 0 replies; 6+ messages in thread From: Thomas Monjalon @ 2024-02-13 15:14 UTC (permalink / raw) To: Sivaprasad Tummala Cc: bruce.richardson, konstantin.v.ananyev, dev, david.marchand, jerin.jacob, stable, Ferruh Yigit, Morten Brørup, Tyler Retzlaff 23/11/2023 11:29, Ferruh Yigit: > On 11/23/2023 7:27 AM, Sivaprasad Tummala wrote: > > AMD Epyc processors doesn't support get_tsc_freq_arch(). > > The patch allows graceful return to allow fallback to > > alternate TSC calibration. > > > > Fixes: 3dbc565e81a0 ("timer: honor arch-specific TSC frequency query") > > Cc: jerin.jacob@caviumnetworks.com > > Cc: stable@dpdk.org > > > > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com> > > Acked-by: Ferruh Yigit <ferruh.yigit@amd.com> Applied I don't want to block longer this fix, but I am not satisfied with the implementation. David, Ferruh, Bruce and Morten were proposing some abstractions for CPU features identification. I think we should start a new enum in rte_cpuflags.h defining some common CPU features, example is timer calibration here. The function get_tsc_freq_arch() was already doing some __cpuid calls, so it's one more, but please could you work on abstracting it in EAL? Thank you ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-13 15:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-09 5:28 [PATCH] eal/x86: add vendor ID checks for specific implementation Sivaprasad Tummala 2023-11-22 11:25 ` Ferruh Yigit 2023-11-22 17:45 ` Tummala, Sivaprasad 2023-11-23 7:27 ` [PATCH v2] eal/x86: add AMD vendor check to choose TSC calibration Sivaprasad Tummala 2023-11-23 10:29 ` Ferruh Yigit 2024-02-13 15:14 ` Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).