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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 15/06/2023 09:05, Michal Orzel wrote:
> Hi Ayan,
Hi Michal,
> On 02/06/2023 14:07, Ayan Kumar Halder wrote:
> > Restructure the code so that one can use pa_range_info[] table for both
> > ARM_32 as well as ARM_64.
> I grepped for ARM_{32,64} in our code base and could not find any use in source 
> files except for things
> introduced by this commit. While I'm ok with it in a commit message I think for 
> consistency we should be
> using arm32/arm64 in the code.
AFAIU, arm32/arm64 refers to the Architecture. ARM_32/ARM_64 refers to 
the configuration.
If you see the original code (xen/arch/arm/include/asm/p2m.h, 
xen/arch/arm/p2m.c)
ARM_32/ARM_64 has been used.

Thus, I used ARM_32/ARM_64 in this commit. Let me know if it makes sense.
The rest of your comments look sane to me.

- Ayan

> > Also, removed the hardcoding for P2M_ROOT_ORDER and P2M_ROOT_LEVEL as
> > p2m_root_order can be obtained from the pa_range_info[].root_order and
> > p2m_root_level can be obtained from pa_range_info[].sl0.
> >
> > Refer ARM DDI 0406C.d ID040418, B3-1345,
> > "Use of concatenated first-level translation tables
> >
> > ...However, a 40-bit input address range with a translation granularity of 4KB
> > requires a total of 28 bits of address resolution. Therefore, a stage 2
> > translation that supports a 40-bit input address range requires two concatenated
> > first-level translation tables,..."
> >
> > Thus, root-order is 1 for 40-bit IPA on ARM_32.
> >
> > Refer ARM DDI 0406C.d ID040418, B3-1348,
> >
> > "Determining the required first lookup level for stage 2 translations
> >
> > For a stage 2 translation, the output address range from the stage 1
> > translations determines the required input address range for the stage 2
> > translation. The permitted values of VTCR.SL0 are:
> >
> > 0b00 Stage 2 translation lookup must start at the second level.
> > 0b01 Stage 2 translation lookup must start at the first level.
> >
> > VTCR.T0SZ must indicate the required input address range. The size of the input
> > address region is 2^(32-T0SZ) bytes."
> >
> > Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = -8 when the size of input
> > address region is 2^40 bytes.
> >
> > Thus, pa_range_info[].t0sz = 1 (VTCR.S) | 8 (VTCR.T0SZ) ie 11000b which is 24.
> >
> > VTCR.T0SZ, is bits [5:0] for ARM_64.
> > VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
> >
> > For this, we have used struct bitfields to convert pa_range_info[].t0sz to its
> > ARM_32 variant.
> >
> > pa_range_info[] is indexed by ID_AA64MMFR0_EL1.PARange which is present in Arm64
> > only. This is the reason we do not specify the indices for ARM_32. Also, we
> > duplicated the entry "{ 40,      24/*24*/,  1,          1 }" between ARM_64 and
> > ARM_32. This is done to avoid introducing extra #if-defs.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > Changes from -
> >
> > v3 - 1. New patch introduced in v4.
> > 2. Restructure the code such that pa_range_info[] is used both by ARM_32 as
> > well as ARM_64.
> >
> > v4 - 1. Removed the hardcoded definitions of P2M_ROOT_ORDER and P2M_ROOT_LEVEL.
> > The reason being root_order will not be always 1 (See the next patch).
> > 2. Updated the commit message to explain t0sz, sl0 and root_order values for
> > 32-bit IPA on Arm32.
> > 3. Some sanity fixes.
> >
> > v5 - pa_range_info is indexed by system_cpuinfo.mm64.pa_range. ie
> > when PARange is 0, the PA size is 32, 1 -> 36 and so on. So pa_range_info[] has
> > been updated accordingly.
> > For ARM_32 pa_range_info[0] = 0 and pa_range_info[1] = 0 as we do not support
> > 32-bit, 36-bit physical address range yet.
> >
> > v6 - 1. Added pa_range_info[] entries for ARM_32 without indices. Some entry
> > may be duplicated between ARM_64 and ARM_32.
> > 2. Recalculate p2m_ipa_bits for ARM_32 from T0SZ (similar to ARM_64).
> > 3. Introduced an union to reinterpret T0SZ bits between ARM_32 and ARM_64.
> >
> > v7 - 1. Used struct bifield instead of union to reinterpret T0SZ bits between
> > ARM_32 and ARM_64.
> > 2. Removed the invalid entry for ARM_32.
> >
> >   xen/arch/arm/include/asm/p2m.h |  6 ----
> >   xen/arch/arm/p2m.c             | 50 +++++++++++++++++++++++++---------
> >   2 files changed, 37 insertions(+), 19 deletions(-)
> >
> > diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> > index f67e9ddc72..940495d42b 100644
> > --- a/xen/arch/arm/include/asm/p2m.h
> > +++ b/xen/arch/arm/include/asm/p2m.h
> > @@ -14,16 +14,10 @@
> >   /* Holds the bit size of IPAs in p2m tables.  */
> >   extern unsigned int p2m_ipa_bits;
> >
> > -#ifdef CONFIG_ARM_64
> >   extern unsigned int p2m_root_order;
> >   extern unsigned int p2m_root_level;
> >   #define P2M_ROOT_ORDER    p2m_root_order
> >   #define P2M_ROOT_LEVEL p2m_root_level
> > -#else
> > -/* First level P2M is always 2 consecutive pages */
> > -#define P2M_ROOT_ORDER    1
> > -#define P2M_ROOT_LEVEL 1
> > -#endif
> >
> >   struct domain;
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 418997843d..76388ba54b 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -19,9 +19,9 @@
> >
> >   #define INVALID_VMID 0 /* VMID 0 is reserved */
> >
> > -#ifdef CONFIG_ARM_64
> >   unsigned int __read_mostly p2m_root_order;
> >   unsigned int __read_mostly p2m_root_level;
> > +#ifdef CONFIG_ARM_64
> >   static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT;
> >   /* VMID is by default 8 bit width on AArch64 */
> >   #define MAX_VMID       max_vmid
> > @@ -2247,16 +2247,6 @@ void __init setup_virt_paging(void)
> >       /* Setup Stage 2 address translation */
> >       register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
> >
> > -#ifdef CONFIG_ARM_32
> > -    if ( p2m_ipa_bits < 40 )
> > -        panic("P2M: Not able to support %u-bit IPA at the moment\n",
> > -              p2m_ipa_bits);
> > -
> > -    printk("P2M: 40-bit IPA\n");
> > -    p2m_ipa_bits = 40;
> > -    val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
> > -    val |= VTCR_SL0(0x1); /* P2M starts at first level */
> > -#else /* CONFIG_ARM_64 */
> >       static const struct {
> >           unsigned int pabits; /* Physical Address Size */
> >           unsigned int t0sz;   /* Desired T0SZ, minimum in comment */
> > @@ -2265,6 +2255,7 @@ void __init setup_virt_paging(void)
> >       } pa_range_info[] __initconst = {
> >           /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
> >           /*      PA size, t0sz(min), root-order, sl0(max) */
> > +#ifdef CONFIG_ARM_64
> >           [0] = { 32,      32/*32*/,  0,          1 },
> >           [1] = { 36,      28/*28*/,  0,          1 },
> >           [2] = { 40,      24/*24*/,  1,          1 },
> > @@ -2273,11 +2264,28 @@ void __init setup_virt_paging(void)
> >           [5] = { 48,      16/*16*/,  0,          2 },
> >           [6] = { 52,      12/*12*/,  4,          2 },
> >           [7] = { 0 }  /* Invalid */
> > +#else
> > +        { 40,      24/*24*/,  1,          1 }
> > +#endif
> >       };
> >
> >       unsigned int i;
> >       unsigned int pa_range = 0x10; /* Larger than any possible value */
> >
> > +#ifdef CONFIG_ARM_32
> > +    /*
> > +     * Typecast pa_range_info[].t0sz into ARM_32 bit variant.
> > +     *
> > +     * VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
> > +     * Thus, pa_range_info[].t0sz is translated to its ARM_32 variant using
> > +     * struct bitfields.
> > +     */
> > +    struct
> > +    {
> > +        signed int val:5;
> > +    } t0sz_32;
> > +#else
> > +
> no need for this empty line
>
> >       /*
> >        * 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).
>
> > +    }
> >
> > +#ifdef CONFIG_ARM_64
> >       val |= VTCR_PS(pa_range);
> >       val |= VTCR_TG0_4K;
> >
> >       /* Set the VS bit only if 16 bit VMID is supported. */
> >       if ( MAX_VMID == MAX_VMID_16_BIT )
> >           val |= VTCR_VS;
> > +#endif
> > +
> >       val |= VTCR_SL0(pa_range_info[pa_range].sl0);
> >       val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
> >
> >       p2m_root_order = pa_range_info[pa_range].root_order;
> >       p2m_root_level = 2 - pa_range_info[pa_range].sl0;
> > +
> > +#ifdef CONFIG_ARM_64
> >       p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
> > +#else
> > +    t0sz_32.val = pa_range_info[pa_range].t0sz;
> > +    p2m_ipa_bits = 32 - t0sz_32.val;
> > +#endif
> >
> >       printk("P2M: %d-bit IPA with %d-bit PA and %d-bit VMID\n",
> >              p2m_ipa_bits,
> >              pa_range_info[pa_range].pabits,
> >              ( MAX_VMID == MAX_VMID_16_BIT ) ? 16 : 8);
> > -#endif
> > +
> >       printk("P2M: %d levels with order-%d root, VTCR 0x%"PRIregister"\n",
> >              4 - P2M_ROOT_LEVEL, P2M_ROOT_ORDER, val);
> >
> > --
> > 2.17.1
> ~Michal