<div dir="auto">For humor<div dir="auto">#define RTE_CASTAWAY(x) ((void *)(uinptr_t)(x))</div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 28, 2023, 12:29 PM Bruce Richardson <<a href="mailto:bruce.richardson@intel.com">bruce.richardson@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Aug 28, 2023 at 11:29:05AM +0200, David Marchand wrote: > On Fri, Aug 25, 2023 at 5:29 PM Bruce Richardson > <<a href="mailto:bruce.richardson@intel.com" target="_blank" rel="noreferrer">bruce.richardson@intel.com</a>> 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: <a href="mailto:roretzla@linux.microsoft.com" target="_blank" rel="noreferrer">roretzla@linux.microsoft.com</a> > > > > Signed-off-by: Bruce Richardson <<a href="mailto:bruce.richardson@intel.com" target="_blank" rel="noreferrer">bruce.richardson@intel.com</a>> > > --- > > 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 <<a href="mailto:david.marchand@redhat.com" target="_blank" rel="noreferrer">david.marchand@redhat.com</a>> > > 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 </blockquote></div>