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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 18/05/2023 16:39, 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.
> 
> VTCR.T0SZ, is bits [5:0] for ARM_64.
> VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32.
> 
> Thus, VTCR.T0SZ bits are interpreted accordingly for different architecture.
> For this, we have used union.
> 
> 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.
> 
>  xen/arch/arm/include/asm/p2m.h |  6 ------
>  xen/arch/arm/p2m.c             | 37 +++++++++++++++++++++++-----------
>  2 files changed, 25 insertions(+), 18 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..755cb86c5b 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,22 @@ 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 },
> +        { 0 },  /* Invalid */
Do we really need this invalid entry?

> +#endif
>      };
>  
>      unsigned int i;
>      unsigned int pa_range = 0x10; /* Larger than any possible value */
>  
> +    /* Typecast pa_range_info[].t0sz into ARM_32 and ARM_64 bit variants. */
This would want some explanation in the code.

> +    union {
> +        signed int t0sz_32:5;
> +        unsigned int t0sz_64:6;
> +    } t0sz;
> +
> +#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 +2293,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++ )
> @@ -2306,24 +2309,34 @@ 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;
> +#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
> +    t0sz.t0sz_64 = pa_range_info[pa_range].t0sz;
>      p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
This should be:
p2m_ipa_bits = 64 - t0sz.t0sz_64;

Another alternative would be to use anonymous unions+struct/union inside 
pa_range_info, e.g:
        union {
            unsigned int t0sz;
            struct {
                signed int t0sz_32:5;
            };
        };
so, if t0sz stores 24, t0sz_32 would automatically store -8.
This could simplify the code later on, as you could just do:

#ifdef CONFIG_ARM_64
    p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz;
#else
    p2m_ipa_bits = 32 - pa_range_info[pa_range].t0sz_32;
#endif

However, I think it would require placing extra braces around initializers, i.e:
[0] = { 32,      {32/*32*/},  0,          1 },

~Michal