Hi Julien > -----Original Message----- > From: Julien Grall <jul...@xen.org> > Sent: 2021年1月8日 19:46 > To: Wei Chen <wei.c...@arm.com>; xen-devel@lists.xenproject.org; > sstabell...@kernel.org > Cc: Bertrand Marquis <bertrand.marq...@arm.com>; Penny Zheng > <penny.zh...@arm.com>; Jiamei Xie <jiamei....@arm.com>; nd > <n...@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.c...@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
