[PATCH v2 1/3] eal: add x86 cpuid support for monitorx

[Tummala, Sivaprasad]

[AMD Official Use Only - General]



> -----Original Message-----
> From: David Marchand <david.marchand at redhat.com>
> Sent: Friday, April 14, 2023 12:36 PM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala at amd.com>
> Cc: david.hunt at intel.com; dev at dpdk.org; Thomas Monjalon
> <thomas at monjalon.net>; Burakov, Anatoly <anatoly.burakov at intel.com>
> Subject: Re: [PATCH v2 1/3] eal: add x86 cpuid support for monitorx
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> On Thu, Apr 13, 2023 at 7:51 PM Tummala, Sivaprasad
> <Sivaprasad.Tummala at amd.com> wrote:
> >
> > [AMD Official Use Only - General]
> >
> > Hi David,
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand at redhat.com>
> > > Sent: Thursday, April 13, 2023 5:30 PM
> > > To: Tummala, Sivaprasad <Sivaprasad.Tummala at amd.com>
> > > Cc: david.hunt at intel.com; dev at dpdk.org; Thomas Monjalon
> > > <thomas at monjalon.net>; Burakov, Anatoly <anatoly.burakov at intel.com>
> > > Subject: Re: [PATCH v2 1/3] eal: add x86 cpuid support for monitorx
> > >
> > > Caution: This message originated from an External Source. Use proper
> > > caution when opening attachments, clicking links, or responding.
> > >
> > >
> > > On Thu, Apr 13, 2023 at 1:54 PM Sivaprasad Tummala
> > > <sivaprasad.tummala at amd.com> wrote:
> > > >
> > > > Add a new CPUID flag to indicate support for monitorx instruction
> > > > on AMD Epyc processors.
> > > >
> > > > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala at amd.com>
> > > > ---
> > > >  lib/eal/include/generic/rte_cpuflags.h | 2 ++
> > > >  lib/eal/x86/include/rte_cpuflags.h     | 1 +
> > > >  lib/eal/x86/rte_cpuflags.c             | 3 +++
> > > >  3 files changed, 6 insertions(+)
> > > >
> > > > diff --git a/lib/eal/include/generic/rte_cpuflags.h
> > > > b/lib/eal/include/generic/rte_cpuflags.h
> > > > index d35551e931..db653a8dd7 100644
> > > > --- a/lib/eal/include/generic/rte_cpuflags.h
> > > > +++ b/lib/eal/include/generic/rte_cpuflags.h
> > > > @@ -26,6 +26,8 @@ struct rte_cpu_intrinsics {
> > > >         /**< indicates support for rte_power_pause function */
> > > >         uint32_t power_monitor_multi : 1;
> > > >         /**< indicates support for rte_power_monitor_multi
> > > > function */
> > > > +       uint32_t amd_power_monitorx : 1;
> > > > +       /**< indicates amd support for rte_power_monitor function
> > > > + */
> > >
> > > I did not look at the patch detail, I just stopped at this part.
> > > What makes the AMD monitorx stuff special that it needs to be
> > > exposed in the generic API?
> >
> > Monitorx is different ISA /opcode (0F 01 FA) as compared to UMonitor
> > (0F 01 C8). This need to be distinguished on specific x86 platforms.
> > Hence in the current power intrinsics, for x86 we require a new flag to distinguish
> MonitorX and UMonitor and invoke the appropriate x86 ISA in the datapath.
> 
> Requiring a new x86 cpuflag to identify this ISA presence is ok.
> 
> 
> But here, I am talking about the generic power instrinsic API.
> Let me phrase my comment differently...
> 
> As described in the API:
>         uint32_t power_monitor : 1;
>         /**< indicates support for rte_power_monitor function */
> 
> Does AMD thing behave completely different from the x86?
> Looking at patch 3, I understand this is not the case.
> 
> So we don't need a "amd" flag in the generic flags.
> The indirection for calling the right ISA should be hidden in
> rte_power_* helpers implemented for x86.
> 

Thanks for your feedback. I will fix this and send an updated patch.