diff mbox series

[v11,05/16] eal: use umonitor umwait and tpause intrinsics

Message ID 1691781658-32520-6-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series msvc integration changes |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Tyler Retzlaff Aug. 11, 2023, 7:20 p.m. UTC 
  Inline assembly is not supported for MSVC x64 instead use _umonitor,
_umwait and _tpause intrinsics.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
---
 lib/eal/x86/rte_power_intrinsics.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Bruce Richardson Aug. 25, 2023, 2:12 p.m. UTC | #1 
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 <roretzla@linux.microsoft.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Konstantin Ananyev <konstantin.v.ananyev@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?

/Bruce
Morten Brørup Aug. 25, 2023, 2:56 p.m. UTC | #2 
> From: Bruce Richardson [mailto:bruce.richardson@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 <roretzla@linux.microsoft.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Konstantin Ananyev <konstantin.v.ananyev@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.

But I don't object to the quick fix. :-)

-Morten
Bruce Richardson Aug. 25, 2023, 3:12 p.m. UTC | #3 
On Fri, Aug 25, 2023 at 04:56:51PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@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 <roretzla@linux.microsoft.com>
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > Acked-by: Konstantin Ananyev <konstantin.v.ananyev@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
Tyler Retzlaff Aug. 25, 2023, 3:44 p.m. UTC | #4 
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.richardson@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 <roretzla@linux.microsoft.com>
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > Acked-by: Konstantin Ananyev <konstantin.v.ananyev@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
diff mbox series

Patch

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
 	asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
 			:
 			: "D"(pmc->addr));
+#endif
 
 	/* now that we've put this address into monitor, we can unlock */
 	rte_spinlock_unlock(&s->lock);
@@ -123,10 +127,14 @@ 
 		goto end;
 
 	/* execute UMWAIT */
+#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
+	_umwait(tsc_l, tsc_h);
+#else
 	asm volatile(".byte 0xf2, 0x0f, 0xae, 0xf7;"
 			: /* ignore rflags */
 			: "D"(0), /* enter C0.2 */
 			  "a"(tsc_l), "d"(tsc_h));
+#endif
 
 end:
 	/* erase sleep address */
@@ -153,10 +161,14 @@ 
 		return -ENOTSUP;
 
 	/* execute TPAUSE */
+#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
+	_tpause(tsc_l, tsc_h);
+#else
 	asm volatile(".byte 0x66, 0x0f, 0xae, 0xf7;"
 			: /* ignore rflags */
 			: "D"(0), /* enter C0.2 */
 			"a"(tsc_l), "d"(tsc_h));
+#endif
 
 	return 0;
 }