From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B9A1841B29; Mon, 28 Aug 2023 12:42:50 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 90B834026D; Mon, 28 Aug 2023 12:42:50 +0200 (CEST) Received: from mail-vk1-f181.google.com (mail-vk1-f181.google.com [209.85.221.181]) by mails.dpdk.org (Postfix) with ESMTP id 815784021E for ; Mon, 28 Aug 2023 12:42:49 +0200 (CEST) Received: by mail-vk1-f181.google.com with SMTP id 71dfb90a1353d-4871e5dbe0cso2509860e0c.1 for ; Mon, 28 Aug 2023 03:42:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20221208.gappssmtp.com; s=20221208; t=1693219369; x=1693824169; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=3v5XYYNkyvHo3ew3xEGYOiP2jebYzVbn8ZshnNE6FRs=; b=Q2uxnEkfOg0yJU2rYI5kW63bThMQIXbiM6otz80cLYXteG1Sl87q3SpveZH6A2SGk1 /WqrkmAW51z0VgGJRGbmIDwZ+s7e0+g1dY56TAHiF8azenG/H8tVDiLaTwQ6V5GNnDqr nAiB3n5seYeepk2Jwf/0I9MKq19HD7dxmM8JEJ0KwaFZurU42dCNSEDgUBbkL6EZTZV5 kYQ4zEVBsiJNBKugkgMxIiQ1YAti/lwSru52np+2vUxmZDTHNSvPPd1EzSsj+viUGaDX l/Tr2ge9N1n4HR5O8aIVhgmUjsb5KtTwG5LRqSCgqQ68LaIm7us6CHPzBAfdCpeDFqV6 BAzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1693219369; x=1693824169; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=3v5XYYNkyvHo3ew3xEGYOiP2jebYzVbn8ZshnNE6FRs=; b=QoFng/e/E+67jTjdJz1mphPZO4hFWUUWp4yiO0Xo8e8fy2qLrK/mgm0KACskzo5pCE +yCNceR6mjr08bAPZhKiVNXJ0dzRwfK7OwSsl0DUTx9HnFGTbzzdZU9+7uH9op+A7wfv eyX5fvo+aelREgWi/5XBd312MKOJKD5qSP94LiWVqsfBwRwwXzXz6A2JR0uTndKW68xW dh513QfdNrZIrBupJ3S5lv+Mak6HEZ7CTPd4Wd6M3kRr7MhbellLEWFw7wI+tzRZdRM7 tyaI44x6EyuHcEXFDJvt40JR+KhJv1O9T/367eycNV9vWzXyJ+IptIgwB69JP0VpiRcb 5ysA== X-Gm-Message-State: AOJu0YwwVYxNqLt7Hz/K8gvzBRTRgMNl7T4YqVviGmxSYdaPL3W4Qslu zuPY/mBNJE54x0jLBICgwo/Qaf+73/+M6CBPFJHwGg== X-Google-Smtp-Source: AGHT+IH8APYYtyTafaMEAVcJL7i2AJ6QVLuJCv15zkuKU+tBvDkPBiGiYqHW1YcyrjeU7YEB6c2ut2iqhJi/Cxl1JmY= X-Received: by 2002:a1f:e7c1:0:b0:48f:c0f5:518d with SMTP id e184-20020a1fe7c1000000b0048fc0f5518dmr6321665vkh.2.1693219368887; Mon, 28 Aug 2023 03:42:48 -0700 (PDT) MIME-Version: 1.0 References: <20230825152850.1107690-1-bruce.richardson@intel.com> In-Reply-To: From: Stephen Hemminger Date: Mon, 28 Aug 2023 12:42:38 +0200 Message-ID: Subject: Re: [PATCH] eal/x86: fix build on systems with WAITPKG support To: Bruce Richardson Cc: David Marchand , dev , =?UTF-8?Q?Morten_Br=C3=B8rup?= , Tyler Retzlaff Content-Type: multipart/alternative; boundary="000000000000027cd70603f95b66" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000027cd70603f95b66 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable For humor #define RTE_CASTAWAY(x) ((void *)(uinptr_t)(x)) On Mon, Aug 28, 2023, 12:29 PM Bruce Richardson wrote: > On Mon, Aug 28, 2023 at 11:29:05AM +0200, David Marchand wrote: > > On Fri, Aug 25, 2023 at 5:29=E2=80=AFPM Bruce Richardson > > 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 t= he > > > 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=3Ddiscarded-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: roretzla@linux.microsoft.com > > > > > > Signed-off-by: Bruce Richardson > > > --- > > > 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 =3D 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 > > > > 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 > --000000000000027cd70603f95b66 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
For humor
#define RTE_CASTAWAY(x) ((void= *)(uinptr_t)(x))

On Mon, Aug 28, 2023, 12:29 PM Bruce Richardson <= bruce.richardson@intel.com> wrote:
On Mon, Aug 28, 2023 a= t 11:29:05AM +0200, David Marchand wrote:
> On Fri, Aug 25, 2023 at 5:29=E2=80=AFPM 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" intri= nsic, 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 argu= ment 1
> > of '_umonitor' discards 'volatile' qualifier from= pointer target type
> > [-Werror=3Ddiscarded-qualifiers]
> >=C2=A0 =C2=A0113 |=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0_umonitor(pmc-= >addr);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0~~~^~~~~~
> >
> > We can avoid this issue by using RTE_PTR_ADD(..., 0) to cast the = pointer
> > through "uintptr_t" and thereby remove the volatile wit= hout 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 mana= gement")
> > Cc: roretzla@linux.microsoft.com
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@inte= l.com>
> > ---
> >=C2=A0 lib/eal/x86/rte_power_intrinsics.c | 12 ++++++------
> >=C2=A0 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_mo= nitor_cond *pmc,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rte_spinlock_lock(&s->loc= k);
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->monitor_addr =3D pmc->a= ddr;
> >
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 * we're using raw byte codes for= now as only the newest compiler
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 * versions support this instruction = natively.
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> > -
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* set address for UMONITOR */ > >=C2=A0 #if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0_umonitor(pmc->addr);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* use RTE_PTR_ADD to cast away "= ;volatile" when using the intrinsic */
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0_umonitor(RTE_PTR_ADD(pmc->addr, 0= ));
> >=C2=A0 #else
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 * we're using raw byte codes for= compiler versions which
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 * don't support this instruction= natively.
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0asm volatile(".byte 0xf3, 0= x0f, 0xae, 0xf7;"
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0: "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? := -)
>
Not sure. I would hope so, though.

/Bruce
--000000000000027cd70603f95b66--