| Message ID | 20230602120754.23817-2-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for 32-bit physical address | expand |
Hi Ayan, On 02/06/2023 14:07, 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. I grepped for ARM_{32,64} in our code base and could not find any use in source files except for things introduced by this commit. While I'm ok with it in a commit message I think for consistency we should be using arm32/arm64 in the code. > > 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. > > For this, we have used struct bitfields to convert pa_range_info[].t0sz to its > ARM_32 variant. > > 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@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. > > 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. > > v7 - 1. Used struct bifield instead of union to reinterpret T0SZ bits between > ARM_32 and ARM_64. > 2. Removed the invalid entry for ARM_32. > > xen/arch/arm/include/asm/p2m.h | 6 ---- > xen/arch/arm/p2m.c | 50 +++++++++++++++++++++++++--------- > 2 files changed, 37 insertions(+), 19 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..76388ba54b 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,28 @@ 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 } > +#endif > }; > > unsigned int i; > unsigned int pa_range = 0x10; /* Larger than any possible value */ > > +#ifdef CONFIG_ARM_32 > + /* > + * Typecast pa_range_info[].t0sz into ARM_32 bit variant. > + * > + * VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32. > + * Thus, pa_range_info[].t0sz is translated to its ARM_32 variant using > + * struct bitfields. > + */ > + struct > + { > + signed int val:5; > + } t0sz_32; > +#else > + no need for this empty line > /* > * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured > * with IPA bits == PA bits, compare against "pabits". > @@ -2291,6 +2299,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++ ) > @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void) > > /* pa_range is 4 bits but we don't support all modes */ this comment makes sense really only on arm64 as it refers to PARange field of ID_AA64MMFR0_EL1. > 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); > + { > + /* > + * In case of ARM_64, we do not support this encoding of > + * ID_AA64MMFR0_EL1.PARange > + */ > + panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits); NIT: Putting variable names in messages visible by users is not a great idea IMO. "Unsupported IPA size" would read better. Furthermore, I do not think printing IPA size in hex is beneficial. I would use "%u bits" (i.e. 32 bits reads better than 0x20 bits). > + } > > +#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 > p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz; > +#else > + t0sz_32.val = pa_range_info[pa_range].t0sz; > + p2m_ipa_bits = 32 - t0sz_32.val; > +#endif > > 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.1 > > ~Michal
On 15/06/2023 09:05, Michal Orzel wrote: > Hi Ayan, Hi Michal, > > On 02/06/2023 14:07, 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. > I grepped for ARM_{32,64} in our code base and could not find any use in source files except for things > introduced by this commit. While I'm ok with it in a commit message I think for consistency we should be > using arm32/arm64 in the code. AFAIU, arm32/arm64 refers to the Architecture. ARM_32/ARM_64 refers to the configuration. If you see the original code (xen/arch/arm/include/asm/p2m.h, xen/arch/arm/p2m.c) ARM_32/ARM_64 has been used. Thus, I used ARM_32/ARM_64 in this commit. Let me know if it makes sense. The rest of your comments look sane to me. - Ayan > >> 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. >> >> For this, we have used struct bitfields to convert pa_range_info[].t0sz to its >> ARM_32 variant. >> >> 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@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. >> >> 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. >> >> v7 - 1. Used struct bifield instead of union to reinterpret T0SZ bits between >> ARM_32 and ARM_64. >> 2. Removed the invalid entry for ARM_32. >> >> xen/arch/arm/include/asm/p2m.h | 6 ---- >> xen/arch/arm/p2m.c | 50 +++++++++++++++++++++++++--------- >> 2 files changed, 37 insertions(+), 19 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..76388ba54b 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,28 @@ 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 } >> +#endif >> }; >> >> unsigned int i; >> unsigned int pa_range = 0x10; /* Larger than any possible value */ >> >> +#ifdef CONFIG_ARM_32 >> + /* >> + * Typecast pa_range_info[].t0sz into ARM_32 bit variant. >> + * >> + * VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32. >> + * Thus, pa_range_info[].t0sz is translated to its ARM_32 variant using >> + * struct bitfields. >> + */ >> + struct >> + { >> + signed int val:5; >> + } t0sz_32; >> +#else >> + > no need for this empty line > >> /* >> * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured >> * with IPA bits == PA bits, compare against "pabits". >> @@ -2291,6 +2299,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++ ) >> @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void) >> >> /* pa_range is 4 bits but we don't support all modes */ > this comment makes sense really only on arm64 as it refers to PARange field of ID_AA64MMFR0_EL1. > >> 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); >> + { >> + /* >> + * In case of ARM_64, we do not support this encoding of >> + * ID_AA64MMFR0_EL1.PARange >> + */ >> + panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits); > NIT: Putting variable names in messages visible by users is not a great idea IMO. > "Unsupported IPA size" would read better. Furthermore, I do not think printing IPA size in hex > is beneficial. I would use "%u bits" (i.e. 32 bits reads better than 0x20 bits). > >> + } >> >> +#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 >> p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz; >> +#else >> + t0sz_32.val = pa_range_info[pa_range].t0sz; >> + p2m_ipa_bits = 32 - t0sz_32.val; >> +#endif >> >> 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.1 >> >> > ~Michal
Hi Ayan, On 15/06/2023 10:29, Ayan Kumar Halder wrote: > > On 15/06/2023 09:05, Michal Orzel wrote: >> Hi Ayan, > Hi Michal, >> >> On 02/06/2023 14:07, 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. >> I grepped for ARM_{32,64} in our code base and could not find any use >> in source files except for things >> introduced by this commit. While I'm ok with it in a commit message I >> think for consistency we should be >> using arm32/arm64 in the code. > > AFAIU, arm32/arm64 refers to the Architecture. ARM_32/ARM_64 refers to > the configuration. > > If you see the original code (xen/arch/arm/include/asm/p2m.h, > xen/arch/arm/p2m.c) > > ARM_32/ARM_64 has been used. > > Thus, I used ARM_32/ARM_64 in this commit. Let me know if it makes sense. > The rest of your comments look sane to me. In text, we commonly don't use the name of the config. Instead we use the name of the architecture (i.e. arm32/arm64) because this is a strict correspondence. I agree with Michal, about using arm32/arm64 rather than ARM_32/ARM_64 in the comments at least to stay consistent with the rest of the code. Assuming there is no other changes required in this patch (I haven't looked at it yet), then I am happy to handle this request on commit if you are OK with it. Cheers,
On 15/06/2023 10:40, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 15/06/2023 10:29, Ayan Kumar Halder wrote: >> >> On 15/06/2023 09:05, Michal Orzel wrote: >>> Hi Ayan, >> Hi Michal, >>> >>> On 02/06/2023 14:07, 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. >>> I grepped for ARM_{32,64} in our code base and could not find any >>> use in source files except for things >>> introduced by this commit. While I'm ok with it in a commit message >>> I think for consistency we should be >>> using arm32/arm64 in the code. >> >> AFAIU, arm32/arm64 refers to the Architecture. ARM_32/ARM_64 refers >> to the configuration. >> >> If you see the original code (xen/arch/arm/include/asm/p2m.h, >> xen/arch/arm/p2m.c) >> >> ARM_32/ARM_64 has been used. >> >> Thus, I used ARM_32/ARM_64 in this commit. Let me know if it makes >> sense. >> The rest of your comments look sane to me. > In text, we commonly don't use the name of the config. Instead we use > the name of the architecture (i.e. arm32/arm64) because this is a > strict correspondence. > > I agree with Michal, about using arm32/arm64 rather than ARM_32/ARM_64 > in the comments at least to stay consistent with the rest of the code. I see what you mean. > > Assuming there is no other changes required in this patch (I haven't > looked at it yet), then I am happy to handle this request on commit if > you are OK with it. I see that the rest of the comments from Michal are related to style (ie no functional changes). Thus, I am happy for you to fix them on commit. - Ayan > > Cheers, >
Hi Michal, I notice you posted some comments but didn't add a Acked-by/Reviewed-by. Can you indicate if you are happy with the patch so long your comments are addressed? If so, are you OK if I deal with them on commit? Cheers,
Hi Julien, On 15/06/2023 22:32, Julien Grall wrote: > > > Hi Michal, > > I notice you posted some comments but didn't add a Acked-by/Reviewed-by. > Can you indicate if you are happy with the patch so long your comments > are addressed? > > If so, are you OK if I deal with them on commit? I thought I added my tag but clearly not. With the remarks addressed on commit: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi, On 15/06/2023 09:05, Michal Orzel wrote: >> /* >> * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured >> * with IPA bits == PA bits, compare against "pabits". >> @@ -2291,6 +2299,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++ ) >> @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void) >> >> /* pa_range is 4 bits but we don't support all modes */ > this comment makes sense really only on arm64 as it refers to PARange field of ID_AA64MMFR0_EL1. > >> 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); >> + { >> + /* >> + * In case of ARM_64, we do not support this encoding of >> + * ID_AA64MMFR0_EL1.PARange >> + */ >> + panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits); > NIT: Putting variable names in messages visible by users is not a great idea IMO. > "Unsupported IPA size" would read better. Furthermore, I do not think printing IPA size in hex > is beneficial. I would use "%u bits" (i.e. 32 bits reads better than 0x20 bits). I went with the following: - /* pa_range is 4 bits but we don't support all modes */ + /* Check if we found the associated entry in the array */ if ( pa_range >= ARRAY_SIZE(pa_range_info) || !pa_range_info[pa_range].pabits ) - { - /* - * In case of ARM_64, we do not support this encoding of - * ID_AA64MMFR0_EL1.PARange - */ - panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits); - } + panic("%-bit P2M is not supported\n", p2m_ipa_bits); This should be generic enough.
On 16/06/2023 08:59, Michal Orzel wrote: > Hi Julien, > > On 15/06/2023 22:32, Julien Grall wrote: >> >> >> Hi Michal, >> >> I notice you posted some comments but didn't add a Acked-by/Reviewed-by. >> Can you indicate if you are happy with the patch so long your comments >> are addressed? >> >> If so, are you OK if I deal with them on commit? > I thought I added my tag but clearly not. With the remarks addressed on commit: > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Thanks. The series is now committed. Cheers,
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..76388ba54b 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,28 @@ 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 } +#endif }; unsigned int i; unsigned int pa_range = 0x10; /* Larger than any possible value */ +#ifdef CONFIG_ARM_32 + /* + * Typecast pa_range_info[].t0sz into ARM_32 bit variant. + * + * VTCR.T0SZ is bits [3:0] and S(sign extension), bit[4] for ARM_32. + * Thus, pa_range_info[].t0sz is translated to its ARM_32 variant using + * struct bitfields. + */ + struct + { + signed int val:5; + } t0sz_32; +#else + /* * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured * with IPA bits == PA bits, compare against "pabits". @@ -2291,6 +2299,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++ ) @@ -2304,26 +2313,41 @@ void __init setup_virt_paging(void) /* pa_range is 4 bits but we don't support all modes */ 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); + { + /* + * In case of ARM_64, we do not support this encoding of + * ID_AA64MMFR0_EL1.PARange + */ + panic("Unsupported value for p2m_ipa_bits = 0x%x\n", p2m_ipa_bits); + } +#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 p2m_ipa_bits = 64 - pa_range_info[pa_range].t0sz; +#else + t0sz_32.val = pa_range_info[pa_range].t0sz; + p2m_ipa_bits = 32 - t0sz_32.val; +#endif 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);
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. For this, we have used struct bitfields to convert pa_range_info[].t0sz to its ARM_32 variant. 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@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. 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. v7 - 1. Used struct bifield instead of union to reinterpret T0SZ bits between ARM_32 and ARM_64. 2. Removed the invalid entry for ARM_32. xen/arch/arm/include/asm/p2m.h | 6 ---- xen/arch/arm/p2m.c | 50 +++++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 19 deletions(-)