Re: [XEN v2 11/11] xen/arm: p2m: Enable support for 32bit IPA

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 10/02/2023 16:19, Julien Grall wrote:
> Hi,
Hi Julien,

I need some inputs.

> On 10/02/2023 15:39, Ayan Kumar Halder wrote:
> > On 09/02/2023 11:45, Julien Grall wrote:
> > > Hi,
> > Hi Julien,
> > > On 07/02/2023 15:34, Ayan Kumar Halder wrote:
> > > > On 20/01/2023 11:06, Julien Grall wrote:
> > > > > Hi Ayan,
> > > > Hi Julien,
> > > > > On 17/01/2023 17:43, Ayan Kumar Halder wrote:
> > > > > > VTCR.T0SZ should be set as 0x20 to support 32bit IPA.
> > > > > > Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR,
> > > > > > "Virtualization Translation Control Register" for the bit 
> > > > > > descriptions.
> > > > > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > > > > > ---
> > > > > > Changes from -
> > > > > >
> > > > > > v1 - New patch.
> > > > > >
> > > > > >   xen/arch/arm/p2m.c | 10 +++++++---
> > > > > >   1 file changed, 7 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > > > index 948f199d84..cfdea55e71 100644
> > > > > > --- a/xen/arch/arm/p2m.c
> > > > > > +++ b/xen/arch/arm/p2m.c
> > > > > > @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void)
> > > > > >       register_t val = 
> > > > > > VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
> > > > > >     #ifdef CONFIG_ARM_32
> > > > > > -    if ( p2m_ipa_bits < 40 )
> > > > > > +    if ( p2m_ipa_bits < PADDR_BITS )
> > > > > >           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;
> > > > > > +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
> > > > > > +    p2m_ipa_bits = PADDR_BITS;
> > > > > > +#ifdef CONFIG_ARM_PA_32
> > > > > > +    val |= VTCR_T0SZ(0x20); /* 32 bit IPA */
> > > > > > +#else
> > > > > >       val |= VTCR_T0SZ(0x18); /* 40 bit IPA */
> > > > > > +#endif
> > > > > I am wondering whether this is right time to switch to an array 
> > > > > like the arm64 code? This would allow to use 32-bit IPA also when 
> > > > > Xen support 64-bit physical address.
> > > > In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the 
> > > > physical address range supported at runtime. This is then used as 
> > > > an index into pa_range_info[] to determine t0sz, root_order, etc.
> > > It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to 
> > > decide the size.
> > Ack.
> > > > However, for AArch32 I do not see an equivalent register (similar 
> > > > to ID_AA64MMFR0_EL1) or any register to determine the physical 
> > > > address range. Thus, I will prefer to keep the code as it is unless 
> > > > you suggest any alternative.
> > > I looked at the Arm Arm and indeed it doesn't look like there are 
> > > equivalent for ID_AA64MMFR0_EL1.PARange.
> > > However, my point was less about reading the system register but 
> > > more about the fact we could have the code a bit more generic and 
> > > avoid the assumption that PADDR_BITS is only modified when 
> > > CONFIG_ARM_PA_32 is set.
> > I had a rework at the patch. Please let me know if the following 
> > looks better.
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 948f199d84..bc3bdf5f3e 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void)
> >       register_t val = 
> > VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA;
> >   #ifdef CONFIG_ARM_32
> > -    if ( p2m_ipa_bits < 40 )
> > +    static const struct {
> > +        unsigned int pabits; /* Physical Address Size */
> > +        unsigned int t0sz;   /* Desired T0SZ */
> > +        unsigned int sl0;    /* Desired SL0 */
> Hmmm... Why don't you include the root order? In fact...
>
> > +    } pa_range_info[] __initconst = {
> > +        [0] = { 32,     32,     1 },
> > +        [1] = { 40,     24,     1 },
> ... looking at the ARMv7 specification (see B3-1345 in ARM DDI 
> 0406C.d), the root page-table is only concatenated for 40-bits.
Sorry, I could not follow how you inferred this. Can you paste the 
relevant snippet ?
> > +    };
> > +    int i = 0;
> > +
> > +    if ( p2m_ipa_bits < PADDR_BITS )
> > +        panic("P2M: Not able to support %u-bit IPA at the moment\n",
> > +              p2m_ipa_bits);
> This check seems unnecessary now.
We still need this check as arm_smmu_device_cfg_probe() invokes 
p2m_restrict_ipa_bits(size).
And referARM IHI 0062D.cID070116 (SMMU arch version 2.0 Specification), 
the IPA address size can be
0.

   0b0000 32-bit.

1.

   0b0001 36-bit.

10.

   0b0010 40-bit.

11.

   0b0011 42-bit.

100.

   0b0100 44-bit.

101.

   0b0101 48-bit.

So if p2m_ipa_bits = 36 bits and PADDR_BITS = 40, then we want to panic.

- Ayan

> > +
> > +    printk("P2M: %u-bit IPA\n",PADDR_BITS);
> > +    p2m_ipa_bits = PADDR_BITS;
> > +
> > +    for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> > +        if ( p2m_ipa_bits == pa_range_info[i].pabits )
> > +            break;
> > +
> > +    if ( i == ARRAY_SIZE(pa_range_info) )
> >           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 */
> > +    val |= VTCR_T0SZ(pa_range_info[i].t0sz);
> > +    val |= VTCR_SL0(pa_range_info[i].sl0);
> > +
> I think you can share a lot of code between arm64 and arm32. A diff 
> below (not tested):
> diff --git a/xen/arch/arm/include/asm/p2m.h 
> b/xen/arch/arm/include/asm/p2m.h
> index 91df922e1c9f..05f26780a29d 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 948f199d848f..6fda0b92230a 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -2264,17 +2264,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 */
> @@ -2286,29 +2275,26 @@ void __init setup_virt_paging(void)
>          [0] = { 32,      32/*32*/,  0,          1 },
>          [1] = { 36,      28/*28*/,  0,          1 },
>          [2] = { 40,      24/*24*/,  1,          1 },
> +#ifdef CONFIG_ARM_64
>          [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 */
> +#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".
>       */
>      if ( pa_range_info[system_cpuinfo.mm64.pa_range].pabits < 
> p2m_ipa_bits )
>          p2m_ipa_bits = 
> pa_range_info[system_cpuinfo.mm64.pa_range].pabits;
> -
> -    /*
> -     * cpu info sanitization made sure we support 16bits VMID only if 
> all
> -     * cores are supporting it.
> -     */
> -    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++ )
> @@ -2324,12 +2310,22 @@ 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);
> -    val |= VTCR_PS(pa_range);
> -    val |= VTCR_TG0_4K;
> +#ifdef CONFIG_ARM_64
> +    /*
> +     * cpu info sanitization made sure we support 16bits VMID only if 
> all
> +     * cores are supporting it.
> +     */
> +    if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT )
> +        max_vmid = MAX_VMID_16_BIT;
>
>      /* Set the VS bit only if 16 bit VMID is supported. */
>      if ( MAX_VMID == MAX_VMID_16_BIT )
>          val |= VTCR_VS;
> +#endif
> +
> +    val |= VTCR_PS(pa_range);
> +    val |= VTCR_TG0_4K;
> +
>      val |= VTCR_SL0(pa_range_info[pa_range].sl0);
>      val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz);
>
> @@ -2341,7 +2337,7 @@ void __init setup_virt_paging(void)
>             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);
>
> Cheers,