Currently, 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 32-bit unsinged
integer will do unsigned extend while doing some operations
with 64-bit unsigned integer. This can lead to unexpected
result in some use cases.
For example, in gicv3_send_sgi_list of GICv3 driver:
uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK;
When MPIDR_AFF0_MASK is 0xFFU, compiler output:
f7c: 92785c16 and x22, x0, #0xffffff00
When MPIDR_AFF0_MASK is 0xFFUL, compiler output:
f88: 9278dc75 and x21, x3, #0xffffffffffffff00
If cpu_logical_map(cpu) = 0x100000000UL and MPIDR_AFF0_MASK is
0xFFU, the cluster_id returns 0. But the expected value should
be 0x100000000.
So, in this patch, we force aarch64 to use unsigned long
as MPIDR mask to avoid such unexpected results.
Signed-off-by: Wei Chen <wei.chen@arm.com>
---
v1 -> v2:
1. Remove inaccurate descriptions
2. Update example description
---
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
Hi Wei, How about the following title: "xen/arm: Don't ignore the affinity level 3 in the MPIDR" On 08/01/2021 06:29, Wei Chen wrote: > Currently, 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 32-bit unsinged s/unsinged/unsigned/ > integer will do unsigned extend while doing some operations > with 64-bit unsigned integer. This can lead to unexpected > result in some use cases. > > For example, in gicv3_send_sgi_list of GICv3 driver: > uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; > > When MPIDR_AFF0_MASK is 0xFFU, compiler output: > f7c: 92785c16 and x22, x0, #0xffffff00 > When MPIDR_AFF0_MASK is 0xFFUL, compiler output: > f88: 9278dc75 and x21, x3, #0xffffffffffffff00 > > If cpu_logical_map(cpu) = 0x100000000UL and MPIDR_AFF0_MASK is > 0xFFU, the cluster_id returns 0. But the expected value should > be 0x100000000. > > So, in this patch, we force aarch64 to use unsigned long > as MPIDR mask to avoid such unexpected results. How about the following commit message: "Currently, Xen is considering that all the affinity bits are defined below 32-bit. However, Arm64 define a 3rd level affinity in bits 32-39. The function gicv3_send_sgi_list in the GICv3 driver will compute the cluser using the following code: uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; Because MPIDR_AFF0_MASK is defined as a 32-bit value, we will miss out the 3rd level affinity. As a consequence, the IPI would not be sent to the correct vCPU. This particular error can be solved by switching MPIDR_AFF0_MASK to use unsigned long. However, take the opportunity to switch all the MPIDR_* define to use unsigned long to avoid anymore issue. " I can update the commit message while committing if you are happy with it. Cheers, > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > v1 -> v2: > 1. Remove inaccurate descriptions > 2. Update example description > > --- > 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 > -- Julien Grall
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年1月8日 19:46 > 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 v2] xen/arm: Using unsigned long for arm64 MPIDR mask > > Hi Wei, > > How about the following title: > > "xen/arm: Don't ignore the affinity level 3 in the MPIDR" > This title makes more sense. > On 08/01/2021 06:29, Wei Chen wrote: > > Currently, 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 32-bit unsinged > > s/unsinged/unsigned/ > thanks. > > integer will do unsigned extend while doing some operations > > with 64-bit unsigned integer. This can lead to unexpected > > result in some use cases. > > > > For example, in gicv3_send_sgi_list of GICv3 driver: > > uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; > > > > When MPIDR_AFF0_MASK is 0xFFU, compiler output: > > f7c: 92785c16 and x22, x0, #0xffffff00 > > When MPIDR_AFF0_MASK is 0xFFUL, compiler output: > > f88: 9278dc75 and x21, x3, #0xffffffffffffff00 > > > > If cpu_logical_map(cpu) = 0x100000000UL and MPIDR_AFF0_MASK is > > 0xFFU, the cluster_id returns 0. But the expected value should > > be 0x100000000. > > > > So, in this patch, we force aarch64 to use unsigned long > > as MPIDR mask to avoid such unexpected results. > > How about the following commit message: > > "Currently, Xen is considering that all the affinity bits are defined > below 32-bit. However, Arm64 define a 3rd level affinity in bits 32-39. > > The function gicv3_send_sgi_list in the GICv3 driver will compute the > cluser using the following code: > > uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; > > Because MPIDR_AFF0_MASK is defined as a 32-bit value, we will miss out > the 3rd level affinity. As a consequence, the IPI would not be sent to > the correct vCPU. > > This particular error can be solved by switching MPIDR_AFF0_MASK to use > unsigned long. However, take the opportunity to switch all the MPIDR_* > define to use unsigned long to avoid anymore issue. > " > > I can update the commit message while committing if you are happy with it. > Yes, that would be good, thank you very much : ) > Cheers, > > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > > > --- > > v1 -> v2: > > 1. Remove inaccurate descriptions > > 2. Update example description > > > > --- > > 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 > > > > -- > Julien Grall
On 08/01/2021 11:50, Wei Chen wrote: > Hi Julien Hi Wei, Sorry for the late answer. While cleaning my inbox today, I noticied that I didn't reply to this thread :(. >>> integer will do unsigned extend while doing some operations >>> with 64-bit unsigned integer. This can lead to unexpected >>> result in some use cases. >>> >>> For example, in gicv3_send_sgi_list of GICv3 driver: >>> uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; >>> >>> When MPIDR_AFF0_MASK is 0xFFU, compiler output: >>> f7c: 92785c16 and x22, x0, #0xffffff00 >>> When MPIDR_AFF0_MASK is 0xFFUL, compiler output: >>> f88: 9278dc75 and x21, x3, #0xffffffffffffff00 >>> >>> If cpu_logical_map(cpu) = 0x100000000UL and MPIDR_AFF0_MASK is >>> 0xFFU, the cluster_id returns 0. But the expected value should >>> be 0x100000000. >>> >>> So, in this patch, we force aarch64 to use unsigned long >>> as MPIDR mask to avoid such unexpected results. >> >> How about the following commit message: >> >> "Currently, Xen is considering that all the affinity bits are defined >> below 32-bit. However, Arm64 define a 3rd level affinity in bits 32-39. >> >> The function gicv3_send_sgi_list in the GICv3 driver will compute the >> cluser using the following code: >> >> uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; >> >> Because MPIDR_AFF0_MASK is defined as a 32-bit value, we will miss out >> the 3rd level affinity. As a consequence, the IPI would not be sent to >> the correct vCPU. >> >> This particular error can be solved by switching MPIDR_AFF0_MASK to use >> unsigned long. However, take the opportunity to switch all the MPIDR_* >> define to use unsigned long to avoid anymore issue. >> " >> >> I can update the commit message while committing if you are happy with it. >> > > Yes, that would be good, thank you very much : ) Reviewed-by: Julien Grall <jgrall@amazon.com> And committed. Cheers, -- Julien Grall
Hi, > On 20 Jan 2021, at 17:58, Julien Grall <julien@xen.org> wrote: > > On 08/01/2021 11:50, Wei Chen wrote: >> Hi Julien > > Hi Wei, > > Sorry for the late answer. While cleaning my inbox today, I noticied that I didn't reply to this thread :(. > >>>> integer will do unsigned extend while doing some operations >>>> with 64-bit unsigned integer. This can lead to unexpected >>>> result in some use cases. >>>> >>>> For example, in gicv3_send_sgi_list of GICv3 driver: >>>> uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; >>>> >>>> When MPIDR_AFF0_MASK is 0xFFU, compiler output: >>>> f7c: 92785c16 and x22, x0, #0xffffff00 >>>> When MPIDR_AFF0_MASK is 0xFFUL, compiler output: >>>> f88: 9278dc75 and x21, x3, #0xffffffffffffff00 >>>> >>>> If cpu_logical_map(cpu) = 0x100000000UL and MPIDR_AFF0_MASK is >>>> 0xFFU, the cluster_id returns 0. But the expected value should >>>> be 0x100000000. >>>> >>>> So, in this patch, we force aarch64 to use unsigned long >>>> as MPIDR mask to avoid such unexpected results. >>> >>> How about the following commit message: >>> >>> "Currently, Xen is considering that all the affinity bits are defined >>> below 32-bit. However, Arm64 define a 3rd level affinity in bits 32-39. >>> >>> The function gicv3_send_sgi_list in the GICv3 driver will compute the >>> cluser using the following code: >>> >>> uint64_t cluster_id = cpu_logical_map(cpu) & ~MPIDR_AFF0_MASK; >>> >>> Because MPIDR_AFF0_MASK is defined as a 32-bit value, we will miss out >>> the 3rd level affinity. As a consequence, the IPI would not be sent to >>> the correct vCPU. >>> >>> This particular error can be solved by switching MPIDR_AFF0_MASK to use >>> unsigned long. However, take the opportunity to switch all the MPIDR_* >>> define to use unsigned long to avoid anymore issue. >>> " >>> >>> I can update the commit message while committing if you are happy with it. >>> >> Yes, that would be good, thank you very much : ) > > Reviewed-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Also tested on a platform where Xen was not booting without this patch and i can confirm it fixed the issue :-) Cheers Bertrand > > And committed. > > Cheers, > > -- > Julien Grall
© 2016 - 2024 Red Hat, Inc.