[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v6 11/12] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64
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 4KBrequires a total of 28 bits of address resolution. Therefore, a stage 2translation that supports a 40-bit input address range requires two concatenatedfirst-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 inputaddress region is 2^(32-T0SZ) bytes."Thus VTCR.SL0 = 1 (maximum value) and VTCR.T0SZ = -8 when the size of inputaddress 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 aswell 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 for32-bit IPA on Arm32. 3. Some sanity fixes. v5 - pa_range_info is indexed by system_cpuinfo.mm64.pa_range. iewhen PARange is 0, the PA size is 32, 1 -> 36 and so on. So pa_range_info[] hasbeen updated accordingly.For ARM_32 pa_range_info[0] = 0 and pa_range_info[1] = 0 as we do not support32-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.hindex 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_orderThis 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; +#endifWhy 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). 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.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. 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |