Re: [PATCH] xen/arm: Use register_t type in cpuinfo entries

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 8 Mar 2021, at 20:48, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 08/03/2021 17:18, Bertrand Marquis wrote:
>> All cpu identification registers that we store in the cpuinfo structure
>> are 64bit on arm64 and 32bit on arm32 so storing the values in 32bit on
>> arm64 is removing the higher bits which might contain information in the
>> future.
>> This patch is changing the types in cpuinfo to register_t (which is
>> 32bit on arm32 and 64bit on arm64) and adding the necessary paddings
>> inside the unions.
> 
> I read this as we would replace uint32_t with register_t. However, there are 
> a few instances where you, validly, replace uint64_t with register_t. I would 
> suggest to clarify it in the commit message.

How about adding the following sentence: “For coherency uint64_t entries are 
also changed to register_t on 64bit systems."

> 
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index cae2179126..ea0dd3451e 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -321,7 +321,8 @@ void start_secondary(void)
>>      if ( !opt_hmp_unsafe &&
>>           current_cpu_data.midr.bits != boot_cpu_data.midr.bits )
>>      {
>> -        printk(XENLOG_ERR "CPU%u MIDR (0x%x) does not match boot CPU MIDR 
>> (0x%x),\n"
>> +        printk(XENLOG_ERR "CPU%u MIDR (0x%"PRIregister") does not match 
>> boot "
>> +               "CPU MIDR (0x%"PRIregister"),\n"
> 
> For printk messages, we don't tend to split it like that (even for more than 
> 80 characters one). Instead, the preferred approach is:
> 
> printk(XENLOG_ERR
>       "line 1\n"
>       "line 2\n")

Ok.

Do you want me to send a v2 or can you fix this during the commit ?

> 
> 
> The rest of the code looks good to me:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks :-)

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall