Re: [PATCH v2] xen/arm: Using unsigned long for arm64 MPIDR mask

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 20 Jan 2021, at 17:58, Julien Grall <julien@xxxxxxx> 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@xxxxxxxxxx>

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

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