Hi, > On 20 Jan 2021, at 17:58, Julien Grall <jul...@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 <jgr...@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marq...@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
