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

[David Marchand <david.marchand@redhat.com>]

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.


> 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? :-)


-- 
David Marchand