On Fri, Aug 25, 2023 at 04:12:59PM +0100, Bruce Richardson wrote: > 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.
omitting the volatile qualifier is okay. often the reason why parameters are qualified is as a convenience so that you may pass a volatile qualified argument (and not force removal of the qualification). > > > > 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. yes, this is canonical way to remove the qualification. > > #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
