Re: [PATCH] eal/x86: fix build on systems with WAITPKG support

[Stephen Hemminger <stephen@networkplumber.org>]

[-- Attachment #1: Type: text/plain, Size: 3279 bytes --]

For humor
#define RTE_CASTAWAY(x) ((void *)(uinptr_t)(x))

On Mon, Aug 28, 2023, 12:29 PM Bruce Richardson <bruce.richardson@intel.com>
wrote:

> On Mon, Aug 28, 2023 at 11:29:05AM +0200, David Marchand wrote:
> > On Fri, Aug 25, 2023 at 5:29 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > When doing a build for a system with WAITPKG support and a modern
> > > compiler, we get build errors for the "_umonitor" intrinsic, due to the
> > > casting away of the "volatile" on the parameter.
> > >
> > > ../lib/eal/x86/rte_power_intrinsics.c: In function 'rte_power_monitor':
> > > ../lib/eal/x86/rte_power_intrinsics.c:113:22: error: passing argument 1
> > > of '_umonitor' discards 'volatile' qualifier from pointer target type
> > > [-Werror=discarded-qualifiers]
> > >   113 |         _umonitor(pmc->addr);
> > >         |                   ~~~^~~~~~
> > >
> > > We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the
> pointer
> > > through "uintptr_t" and thereby remove the volatile without warning.
> >
> > As Morten, I prefer an explicit cast (keeping your comments) as it
> > seems we are exploiting an implementation detail of RTE_PTR_ADD.
> >
>
> Ok, I'll do a respin with explicit cast.
>
> >
> > > We also ensure comments are correct for each leg of the
> > > ifdef..else..endif block.
> >
> > Thanks.. I had fixed other places but I have missed this one.
> >
> >
> > >
> > > Fixes: 60943c04f3bc ("eal/x86: use intrinsics for power management")
> > > Cc: roretzla@linux.microsoft.com
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  lib/eal/x86/rte_power_intrinsics.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/lib/eal/x86/rte_power_intrinsics.c
> b/lib/eal/x86/rte_power_intrinsics.c
> > > index 4066d1392e..4f0404bfb8 100644
> > > --- a/lib/eal/x86/rte_power_intrinsics.c
> > > +++ b/lib/eal/x86/rte_power_intrinsics.c
> > > @@ -103,15 +103,15 @@ rte_power_monitor(const struct
> rte_power_monitor_cond *pmc,
> > >         rte_spinlock_lock(&s->lock);
> > >         s->monitor_addr = pmc->addr;
> > >
> > > -       /*
> > > -        * we're using raw byte codes for now as only the newest
> compiler
> > > -        * versions support this instruction natively.
> > > -        */
> > > -
> > >         /* set address for UMONITOR */
> > >  #if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
> > > -       _umonitor(pmc->addr);
> > > +       /* use RTE_PTR_ADD to cast away "volatile" when using the
> intrinsic */
> > > +       _umonitor(RTE_PTR_ADD(pmc->addr, 0));
> > >  #else
> > > +       /*
> > > +        * we're using raw byte codes for compiler versions which
> > > +        * don't support this instruction natively.
> > > +        */
> > >         asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
> > >                         :
> > >                         : "D"(pmc->addr));
> >
> > Tested-by: David Marchand <david.marchand@redhat.com>
> >
> > An additional question, would Intel CI catch such issue?
> > Or was it caught only because you are blessed with bleeding edge hw? :-)
> >
> Not sure. I would hope so, though.
>
> /Bruce
>

[-- Attachment #2: Type: text/html, Size: 4657 bytes --]