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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 15/05/2023 10:25, Julien Grall wrote:
> Hi Ayan,
Hi Julien,
> 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).
By the restriction from SMMU subsystem, I think you mean 
p2m_restrict_ipa_bits().
As I can see, p2m_restrict_ipa_bits() gets invoked much later than 
setup_virt_paging().
So p2m_ipa_bits will take into account SMMU restrictions. Thus, this 
approach should be correct.
Am I missing something ?

- Ayan

> > 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,