* [PATCH] xen/arm: Using unsigned long for arm64 MPIDR mask
@ 2021-01-05 10:17 Wei Chen
2021-01-05 11:01 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Wei Chen @ 2021-01-05 10:17 UTC (permalink / raw)
To: wei.chen, xen-devel, sstabellini, julien
Cc: Bertrand.Marquis, Penny.Zheng, Jiamei.Xie, nd
Curretly, Xen is using UINT32 for MPIDR mask to retrieve
affinity[0,1,2,3] values for MPIDR_EL1 register. The value
of MPIDR_EL1 is 64-bit unsigned long. The operation of 64-bit
and 32-bit integers are compiler related. This means the value
is unpredictable.
For example, when we are using MPIDR_AFF0_MASK to get
cluster_id from a 64-bit integer in gic-v3 driver:
uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK;
When MPIDR_AFF0_MASK is UINT32, compiler output:
f7c: 92785c16 and x22, x0, #0xffffff00
When MPIDR_AFF0_MASK is unsigned long, compiler output:
f88: 9278dc75 and x21, x3, #0xffffffffffffff00
If we have a cpu_logical_map(cpu)= 0x1,00000000. We except
to get a cluster_id 1, but with UINT32 MPIDR_AFF0_MASK, we
will get 0.
So, in this patch, we force aarch64 to use unsigned long
as MPIDR mask to avoid such unpredictable operations.
Signed-off-by: Wei Chen <wei.chen@arm.com>
---
xen/include/asm-arm/processor.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 87c8136022..5c1768cdec 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -75,11 +75,11 @@
/* MPIDR Multiprocessor Affinity Register */
#define _MPIDR_UP (30)
-#define MPIDR_UP (_AC(1,U) << _MPIDR_UP)
+#define MPIDR_UP (_AC(1,UL) << _MPIDR_UP)
#define _MPIDR_SMP (31)
-#define MPIDR_SMP (_AC(1,U) << _MPIDR_SMP)
+#define MPIDR_SMP (_AC(1,UL) << _MPIDR_SMP)
#define MPIDR_AFF0_SHIFT (0)
-#define MPIDR_AFF0_MASK (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
+#define MPIDR_AFF0_MASK (_AC(0xff,UL) << MPIDR_AFF0_SHIFT)
#ifdef CONFIG_ARM_64
#define MPIDR_HWID_MASK _AC(0xff00ffffff,UL)
#else
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xen/arm: Using unsigned long for arm64 MPIDR mask
2021-01-05 10:17 [PATCH] xen/arm: Using unsigned long for arm64 MPIDR mask Wei Chen
@ 2021-01-05 11:01 ` Julien Grall
2021-01-05 13:43 ` Wei Chen
0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2021-01-05 11:01 UTC (permalink / raw)
To: Wei Chen, xen-devel, sstabellini
Cc: Bertrand.Marquis, Penny.Zheng, Jiamei.Xie, nd
Hi Wei,
On 05/01/2021 10:17, Wei Chen wrote:
> Curretly, Xen is using UINT32 for MPIDR mask to retrieve
s/Curretly/Currently/
> affinity[0,1,2,3] values for MPIDR_EL1 register. The value
> of MPIDR_EL1 is 64-bit unsigned long. The operation of 64-bit
> and 32-bit integers are compiler related. This means the value
> is unpredictable.
So I agree that ~MPIDR_AFF0_MASK will do the negation in 32-bit rather
than 64-bit. However, I disagree that this is unpredicable or compiler
specific.
>
> For example, when we are using MPIDR_AFF0_MASK to get
> cluster_id from a 64-bit integer in gic-v3 driver:
> uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK;
>
> When MPIDR_AFF0_MASK is UINT32, compiler output:
> f7c: 92785c16 and x22, x0, #0xffffff00
> When MPIDR_AFF0_MASK is unsigned long, compiler output:
> f88: 9278dc75 and x21, x3, #0xffffffffffffff00
>
> If we have a cpu_logical_map(cpu)= 0x1,00000000. We except
> to get a cluster_id 1, but with UINT32 MPIDR_AFF0_MASK, we
> will get 0.
Something doesn't match here. If the cluster_id were 1, then it should
surely be 1 as well even with the 32-bit mask because there is no shift.
So did you intend to say 0x1,00000000?
>
> So, in this patch, we force aarch64 to use unsigned long
> as MPIDR mask to avoid such unpredictable operations.
Per above, I don't think this is unpredictable.
>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> ---
> xen/include/asm-arm/processor.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 87c8136022..5c1768cdec 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -75,11 +75,11 @@
>
> /* MPIDR Multiprocessor Affinity Register */
> #define _MPIDR_UP (30)
> -#define MPIDR_UP (_AC(1,U) << _MPIDR_UP)
> +#define MPIDR_UP (_AC(1,UL) << _MPIDR_UP)
> #define _MPIDR_SMP (31)
> -#define MPIDR_SMP (_AC(1,U) << _MPIDR_SMP)
> +#define MPIDR_SMP (_AC(1,UL) << _MPIDR_SMP)
> #define MPIDR_AFF0_SHIFT (0)
> -#define MPIDR_AFF0_MASK (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
> +#define MPIDR_AFF0_MASK (_AC(0xff,UL) << MPIDR_AFF0_SHIFT)
> #ifdef CONFIG_ARM_64
> #define MPIDR_HWID_MASK _AC(0xff00ffffff,UL)
> #else
>
--
Julien Grall
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] xen/arm: Using unsigned long for arm64 MPIDR mask
2021-01-05 11:01 ` Julien Grall
@ 2021-01-05 13:43 ` Wei Chen
0 siblings, 0 replies; 3+ messages in thread
From: Wei Chen @ 2021-01-05 13:43 UTC (permalink / raw)
To: Julien Grall, xen-devel, sstabellini
Cc: Bertrand Marquis, Penny Zheng, Jiamei Xie, nd
Hi Julien,
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2021年1月5日 19:01
> To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Penny Zheng
> <Penny.Zheng@arm.com>; Jiamei Xie <Jiamei.Xie@arm.com>; nd
> <nd@arm.com>
> Subject: Re: [PATCH] xen/arm: Using unsigned long for arm64 MPIDR mask
>
> Hi Wei,
>
> On 05/01/2021 10:17, Wei Chen wrote:
> > Curretly, Xen is using UINT32 for MPIDR mask to retrieve
>
> s/Curretly/Currently/
>
got it.
> > affinity[0,1,2,3] values for MPIDR_EL1 register. The value
> > of MPIDR_EL1 is 64-bit unsigned long. The operation of 64-bit
> > and 32-bit integers are compiler related. This means the value
> > is unpredictable.
>
> So I agree that ~MPIDR_AFF0_MASK will do the negation in 32-bit rather
> than 64-bit. However, I disagree that this is unpredicable or compiler
> specific.
>
Yeah, I agree with you, my commit message here is not accurate. This
problem hadn't been found so far, so I had thought maybe someone
can work well with aff3. From the code, Xen had used 'U' for constant in
MPIDR_AFF0_MASK, so it would always be a unsigned extend. The aff3
will be dropped. I will update my commit message in next version.
> >
> > For example, when we are using MPIDR_AFF0_MASK to get
> > cluster_id from a 64-bit integer in gic-v3 driver:
> > uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK;
> >
> > When MPIDR_AFF0_MASK is UINT32, compiler output:
> > f7c: 92785c16 and x22, x0, #0xffffff00
> > When MPIDR_AFF0_MASK is unsigned long, compiler output:
> > f88: 9278dc75 and x21, x3, #0xffffffffffffff00
> >
> > If we have a cpu_logical_map(cpu)= 0x1,00000000. We except
> > to get a cluster_id 1, but with UINT32 MPIDR_AFF0_MASK, we
> > will get 0.
>
> Something doesn't match here. If the cluster_id were 1, then it should
> surely be 1 as well even with the 32-bit mask because there is no shift.
>
> So did you intend to say 0x1,00000000?
>
Sorry, I copied the value from device tree node name. I intend to say the
cpu_logical_map(cpu)= 0x100000000UL, the aff3 (bit32 ~ 39) is 1.
I am not sure we are in the same page of cluster_id. The cluster_id I mentioned
above is a variable in gicv3_send_sgi_list function. The cluster_id you had
mentioned is aff2?
> >
> > So, in this patch, we force aarch64 to use unsigned long
> > as MPIDR mask to avoid such unpredictable operations.
>
> Per above, I don't think this is unpredictable.
I will remove the last part of above sentence.
>
> >
> > Signed-off-by: Wei Chen <wei.chen@arm.com>
> > ---
> > xen/include/asm-arm/processor.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-
> arm/processor.h
> > index 87c8136022..5c1768cdec 100644
> > --- a/xen/include/asm-arm/processor.h
> > +++ b/xen/include/asm-arm/processor.h
> > @@ -75,11 +75,11 @@
> >
> > /* MPIDR Multiprocessor Affinity Register */
> > #define _MPIDR_UP (30)
> > -#define MPIDR_UP (_AC(1,U) << _MPIDR_UP)
> > +#define MPIDR_UP (_AC(1,UL) << _MPIDR_UP)
> > #define _MPIDR_SMP (31)
> > -#define MPIDR_SMP (_AC(1,U) << _MPIDR_SMP)
> > +#define MPIDR_SMP (_AC(1,UL) << _MPIDR_SMP)
> > #define MPIDR_AFF0_SHIFT (0)
> > -#define MPIDR_AFF0_MASK (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
> > +#define MPIDR_AFF0_MASK (_AC(0xff,UL) << MPIDR_AFF0_SHIFT)
> > #ifdef CONFIG_ARM_64
> > #define MPIDR_HWID_MASK _AC(0xff00ffffff,UL)
> > #else
> >
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-05 13:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 10:17 [PATCH] xen/arm: Using unsigned long for arm64 MPIDR mask Wei Chen
2021-01-05 11:01 ` Julien Grall
2021-01-05 13:43 ` Wei Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.