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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 8 Mar 2021, Julien Grall 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.
> 
> > 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")
> 
> 
> The rest of the code looks good to me:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Aside from these minor issues:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>