On Fri, Aug 25, 2023 at 04:56:51PM +0200, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richard...@intel.com] > > Sent: Friday, 25 August 2023 16.13 > > > > On Fri, Aug 11, 2023 at 12:20:47PM -0700, Tyler Retzlaff wrote: > > > Inline assembly is not supported for MSVC x64 instead use _umonitor, > > > _umwait and _tpause intrinsics. > > > > > > Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com> > > > Acked-by: Morten Brørup <m...@smartsharesystems.com> > > > Acked-by: Konstantin Ananyev <konstantin.v.anan...@yandex.ru> > > > --- > > > lib/eal/x86/rte_power_intrinsics.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/lib/eal/x86/rte_power_intrinsics.c > > b/lib/eal/x86/rte_power_intrinsics.c > > > index f749da9..4066d13 100644 > > > --- a/lib/eal/x86/rte_power_intrinsics.c > > > +++ b/lib/eal/x86/rte_power_intrinsics.c > > > @@ -109,9 +109,13 @@ > > > */ > > > > > > /* set address for UMONITOR */ > > > +#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__) > > > + _umonitor(pmc->addr); > > > +#else > > > > This change is unfortunately giving build errors on system with WAITPKG, > > since the intrinsics do not take volatile parameters, unlike the inline > > ASM > > which works fine with both volatile and non-volatile variables. This is > > the > > error I see: > > > > ../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); > > | ~~~^~~~~~ > > > > The easy fix for now seems to be just dropping the "|| > > defined(__WAITPKG__)" part of the #ifdef, and leave the intrinsic for > > MSVC > > only. > > Any objections? > > I wonder if omitting the "volatile" qualifier is correct for this parameter. > Although the authors of _umonitor() apparently think so, I have seen > built-ins with questionable qualifiers before, so I wouldn't trust it to be > correct. > > Replacing inline assembly with built-ins is generally preferable. So I would > prefer if you typecast it away, or temporarily disable that warning. > Simple typecasting doesn't work to remove the warning. However, if we typecast via "uintptr_t" it does seem to work.
#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__) - _umonitor(pmc->addr); + uintptr_t addr_val = (uintptr_t)pmc->addr; + _umonitor((void *)addr_val); #else Seems bit clunky to me. If we want a one-line workaround, we can probably use RTE_PTR_ADD to do the typecasting via uintptr_t for us. /Bruce
