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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 03/05/2023 14:35, Julien Grall wrote:
> 
> 
> On 03/05/2023 13:20, Julien Grall wrote:
>> Hi,
>>
>> 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
> 
> Looking at the next patch. An alternative would be to go back
> duplicating the lines. So after the two patches we would have
> 
> #ifdef CONFIG_ARM_64
>      [0] ...
>      [7] ...
> #else
>      { /* 32-bit */ }
>      { /* 40-bit */ }
> #endif
+1 for this approach

~Michal