Re: [XEN v6 11/12] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Julien Grall <julien@xxxxxxx>]

Hi Ayan,

On 11/05/2023 12:45, Ayan Kumar Halder wrote:
> On 03/05/2023 13:20, Julien Grall wrote:
> > Hi,
> Hi Julien,
>
> I have some clarification.
>
> > On 28/04/2023 18:55, 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.
> > >
> > > 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.
> > > 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.
> > >
> > >   xen/arch/arm/include/asm/p2m.h |  8 +-------
> > >   xen/arch/arm/p2m.c             | 32 ++++++++++++++++++--------------
> > >   2 files changed, 19 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/include/asm/p2m.h 
> > > b/xen/arch/arm/include/asm/p2m.h
> > > index f67e9ddc72..4ddd4643d7 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_ORDER p2m_root_order
> > This looks like a spurious change.
> >
> > >   #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..1fe3cccf46 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,19 +2255,26 @@ 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) */
> > > +        [2] = { 40,      24/*24*/,  1,          1 },
> > I don't like the fact that the index are not ordered anymore and...
> >
> > > +#ifdef CONFIG_ARM_64
> > >           [0] = { 32,      32/*32*/,  0,          1 },
> > >           [1] = { 36,      28/*28*/,  0,          1 },
> > > -        [2] = { 40,      24/*24*/,  1,          1 },
> > >           [3] = { 42,      22/*22*/,  3,          1 },
> > >           [4] = { 44,      20/*20*/,  0,          2 },
> > >           [5] = { 48,      16/*16*/,  0,          2 },
> > >           [6] = { 52,      12/*12*/,  4,          2 },
> > >           [7] = { 0 }  /* Invalid */
> > > +#else
> > > +        [0] = { 0 },  /* Invalid */
> > > +        [1] = { 0 },  /* Invalid */
> > > +        [3] = { 0 }  /* Invalid */
> > > +#endif
> > ... it is not clear to me why we are adding 3 extra entries. I think 
> > it would be better if we do:
> > #ifdef CONFIG_ARM_64
> >    [0] ...
> >    [1] ...
> > #endif
> >    [2] ...
> > #ifdef CONFIG_ARM_64
> >    [3] ...
> >    [4] ...
> >    ...
> > #endif
> >
> > >       };
> > >         unsigned int i;
> > >       unsigned int pa_range = 0x10; /* Larger than any possible value */
> > >   +#ifdef CONFIG_ARM_64
> > >       /*
> > >        * Restrict "p2m_ipa_bits" if needed. As P2M table is always 
> > > configured
> > >        * with IPA bits == PA bits, compare against "pabits".
> > > @@ -2291,6 +2288,9 @@ void __init setup_virt_paging(void)
> > >        */
> > >       if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
> > >           max_vmid = MAX_VMID_16_BIT;
> > > +#else
> > > +    p2m_ipa_bits = PADDR_BITS;
> > > +#endif
> > Why do we need to reset p2m_ipa_bits for Arm?
> Ah, this is a mistake. I will remove this.
>
> > >         /* Choose suitable "pa_range" according to the resulted 
> > > "p2m_ipa_bits". */
> > >       for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> > > @@ -2306,24 +2306,28 @@ void __init setup_virt_paging(void)
> > >       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);
> > >   +#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;
> > > +
> > > +    p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
> > > +#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;
> > > -    p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
> > I think this line should stay for 32-bit as well because we 
> > p2m_ipa_bits should be based on the PA range we selected (see the loop 
> > 'Choose suitable "pa_range"...').
> This isn't true for ARM_32.
You are correct that the line is not correct for Arm32. But my point was 
more for that fact you don't update p2m_ipa_bits based on the PA range 
selected.
> ReferARM DDI 0406C.d ID040418, B3-1348, "Determining the required first 
> lookup level for stage 2 translations"
> "...The size of this input address region is 2(32-TxSZ) bytes, ..."
>
> So for
>
> #ifdef CONFIG_ARM_32
>
> p2m_ipa_bits = 32 - pa_range_info[pa_range].t0sz;
>
> #endif
>
> This will be a problem for 40-bit PA as T0SZ = 24.
>
> So p2m_ipa_bits = 32 - 24 = 8 (which is incorrect).
>
>
> To get around this issue, there are two possible solutions :-
>
> 1. For ARM_32,  do not modify p2m_ipa_bits. Thus p2m_ipa_bits will be 
> using its initialized value (ie PADDR_BITS).
AFAICT, this approach would be incorrect because we wouldn't take into 
account any restriction from the SMMU susbystem (it may support less 
than what the processor support).
> 2. T0SZ should be signed int for ARM_32 (so that it can hold -8) and 
> unsigned int for ARM_64.
> ie
>
>      static const struct {
>          unsigned int pabits; /* Physical Address Size */
> #ifdef CONFIG_ARM_64
>          unsigned int t0sz:5;   /* Desired T0SZ, minimum in comment */
> #else
>          signed int t0sz:5;   /* Desired T0SZ, minimum in comment */
> #endif
>          unsigned int root_order; /* Page order of the root of the p2m */
>          unsigned int sl0;    /* Desired SL0, maximum in comment */
>      } pa_range_info[] __initconst = {
> ....
>
>
> I will prefer option 1 for the sake of less if..defs.
I don't think the two options are equivalent. So my first priority is 
correctness, then if we have still multiple possibility, we can discuss 
which one look the nicest to read.
To avoid the #ifdef in the struct, we could possibly use a series of 
cast. It might be slightly better because a reader will be less confused 
on the type change.
Cheers,

--
Julien Grall