Re: [XEN v8 1/5] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Julien Grall <julien@xxxxxxx>]

Hi,

On 15/06/2023 09:05, Michal Orzel wrote:
> >       /*
> >        * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured
> >        * with IPA bits == PA bits, compare against "pabits".
> > @@ -2291,6 +2299,7 @@ void __init setup_virt_paging(void)
> >        */
> >       if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
> >           max_vmid = MAX_VMID_16_BIT;
> > +#endif
> >
> >       /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
> >       for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> > @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void)
> >
> >       /* pa_range is 4 bits but we don't support all modes */
> this comment makes sense really only on arm64 as it refers to PARange field of 
> ID_AA64MMFR0_EL1.
>
> >       if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
> > !pa_range_info[pa_range].pabits )
> > -        panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", pa_range);
> > +    {
> > +        /*
> > +         * In case of ARM_64, we do not support this encoding of
> > +         * ID_AA64MMFR0_EL1.PARange
> > +         */
> > +        panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits);
> NIT: Putting variable names in messages visible by users is not a great idea 
> IMO.
> "Unsupported IPA size" would read better. Furthermore, I do not think printing 
> IPA size in hex
> is beneficial. I would use "%u bits" (i.e. 32 bits reads better than 0x20 bits).
I went with the following:

-    /* pa_range is 4 bits but we don't support all modes */
+    /* Check if we found the associated entry in the array */
     if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
!pa_range_info[pa_range].pabits )
-    {
-        /*
-         * In case of ARM_64, we do not support this encoding of
-         * ID_AA64MMFR0_EL1.PARange
-         */
-        panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits);
-    }
+        panic("%-bit P2M is not supported\n", p2m_ipa_bits);

This should be generic enough.

--
Julien Grall