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@amd.com>
---
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
#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 },
+#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
};
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
/* 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;
printk("P2M: %d-bit IPA with %d-bit PA and %d-bit VMID\n",
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);
--
2.17.1Hi,
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@amd.com>
> ---
> 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?
>
> /* 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"...').
>
> printk("P2M: %d-bit IPA with %d-bit PA and %d-bit VMID\n",
> 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,
--
Julien Grall
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@amd.com>
>> ---
>> 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.
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).
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.
Happy to hear your opinions.
- Ayan
>
>> printk("P2M: %d-bit IPA with %d-bit PA and %d-bit VMID\n",
>> 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,
>
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@amd.com>
>>> ---
>>> 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
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@amd.com>
>>>> ---
>>>> 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,
>
On 15/05/2023 11:30, Ayan Kumar Halder wrote: >> 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(). Yes. > > As I can see, p2m_restrict_ipa_bits() gets invoked much later than > setup_virt_paging(). I am afraid this is not correct. If you look at setup.c, you will notice that iommu_setup() is called before setup_virt_paging(). There is a comment on top of the former call explaining the ordering. Cheers, -- Julien Grall
Hi Julien
On 15/05/2023 11:43, Julien Grall wrote:
>
>
> On 15/05/2023 11:30, Ayan Kumar Halder wrote:
>>> 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().
>
> Yes.
>
>>
>> As I can see, p2m_restrict_ipa_bits() gets invoked much later than
>> setup_virt_paging().
>
> I am afraid this is not correct. If you look at setup.c, you will
> notice that iommu_setup() is called before setup_virt_paging(). There
> is a comment on top of the former call explaining the ordering.
Yes, you are correct.
WRT to your comment
>> 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.
Do you mean that I should update "p2m_ipa_bits" in setup_virt_paging() [
similar to what is now being done for CONFIG_ARM_64 ] ?
This will then override "p2m_ipa_bits"value set by p2m_restrict_ipa_bits().
But still it does not make sense to update "p2m_ipa_bits" in
setup_virt_paging() for ARM_32 as
/* Choose suitable "pa_range" according to the resulted
"p2m_ipa_bits". */
for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
{
if ( p2m_ipa_bits == pa_range_info[i].pabits )
{
pa_range = i;
break;
}
}
p2m_ipa_bits will be same as pa_range_info[i].pabits.
- Ayan
>
> Cheers,
>
On 15/05/2023 12:49, Ayan Kumar Halder wrote:
> Hi Julien
Hi Ayan,
> On 15/05/2023 11:43, Julien Grall wrote:
>>
>>
>> On 15/05/2023 11:30, Ayan Kumar Halder wrote:
>>>> 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().
>>
>> Yes.
>>
>>>
>>> As I can see, p2m_restrict_ipa_bits() gets invoked much later than
>>> setup_virt_paging().
>>
>> I am afraid this is not correct. If you look at setup.c, you will
>> notice that iommu_setup() is called before setup_virt_paging(). There
>> is a comment on top of the former call explaining the ordering.
>
> Yes, you are correct.
>
>
> WRT to your comment
>
> >> 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.
>
> Do you mean that I should update "p2m_ipa_bits" in setup_virt_paging() [
> similar to what is now being done for CONFIG_ARM_64 ] ?
Yes.
>
>
> This will then override "p2m_ipa_bits"value set by p2m_restrict_ipa_bits().
>
>
> But still it does not make sense to update "p2m_ipa_bits" in
> setup_virt_paging() for ARM_32 as
>
> /* Choose suitable "pa_range" according to the resulted
> "p2m_ipa_bits". */
> for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> {
> if ( p2m_ipa_bits == pa_range_info[i].pabits )
> {
> pa_range = i;
> break;
> }
> }
>
> p2m_ipa_bits will be same as pa_range_info[i].pabits.
With the current logic yes. But, in the past, we had one case where we
used a T0SZ that would not cover the full 2^pabits (see 896ebdfa3a85). I
can't rule out this will not be again the case in the future.
Cheers,
--
Julien Grall
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@amd.com>
>> ---
>> 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
I didn't add '[X] = ' because the index is not necessary for 32-bit.
Cheers,
--
Julien Grall
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@amd.com>
>>> ---
>>> 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
© 2016 - 2026 Red Hat, Inc.