| Message ID | 20230117174358.15344-12-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for 32 bit physical address | expand |
On Tue, 17 Jan 2023, Ayan Kumar Halder wrote: > VTCR.T0SZ should be set as 0x20 to support 32bit IPA. > Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, > "Virtualization Translation Control Register" for the bit descriptions. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes from - > > v1 - New patch. > > xen/arch/arm/p2m.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 948f199d84..cfdea55e71 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) > register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; > > #ifdef CONFIG_ARM_32 > - if ( p2m_ipa_bits < 40 ) > + if ( p2m_ipa_bits < PADDR_BITS ) > 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; > + printk("P2M: %u-bit IPA\n",PADDR_BITS); > + p2m_ipa_bits = PADDR_BITS; > +#ifdef CONFIG_ARM_PA_32 > + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ > +#else > val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ > +#endif > val |= VTCR_SL0(0x1); /* P2M starts at first level */ > #else /* CONFIG_ARM_64 */ > static const struct { > -- > 2.17.1 >
Hi Ayan, On 17/01/2023 17:43, Ayan Kumar Halder wrote: > VTCR.T0SZ should be set as 0x20 to support 32bit IPA. > Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, > "Virtualization Translation Control Register" for the bit descriptions. > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> > --- > Changes from - > > v1 - New patch. > > xen/arch/arm/p2m.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 948f199d84..cfdea55e71 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) > register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; > > #ifdef CONFIG_ARM_32 > - if ( p2m_ipa_bits < 40 ) > + if ( p2m_ipa_bits < PADDR_BITS ) > 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; > + printk("P2M: %u-bit IPA\n",PADDR_BITS); > + p2m_ipa_bits = PADDR_BITS; > +#ifdef CONFIG_ARM_PA_32 > + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ > +#else > val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ > +#endif I am wondering whether this is right time to switch to an array like the arm64 code? This would allow to use 32-bit IPA also when Xen support 64-bit physical address. Cheers,
On 20/01/2023 11:06, Julien Grall wrote: > Hi Ayan, Hi Julien, > > On 17/01/2023 17:43, Ayan Kumar Halder wrote: >> VTCR.T0SZ should be set as 0x20 to support 32bit IPA. >> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, >> "Virtualization Translation Control Register" for the bit descriptions. >> >> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >> --- >> Changes from - >> >> v1 - New patch. >> >> xen/arch/arm/p2m.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 948f199d84..cfdea55e71 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) >> register_t val = >> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >> #ifdef CONFIG_ARM_32 >> - if ( p2m_ipa_bits < 40 ) >> + if ( p2m_ipa_bits < PADDR_BITS ) >> 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; >> + printk("P2M: %u-bit IPA\n",PADDR_BITS); >> + p2m_ipa_bits = PADDR_BITS; >> +#ifdef CONFIG_ARM_PA_32 >> + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ >> +#else >> val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ >> +#endif > > I am wondering whether this is right time to switch to an array like > the arm64 code? This would allow to use 32-bit IPA also when Xen > support 64-bit physical address. In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical address range supported at runtime. This is then used as an index into pa_range_info[] to determine t0sz, root_order, etc. However, for AArch32 I do not see an equivalent register (similar to ID_AA64MMFR0_EL1) or any register to determine the physical address range. Thus, I will prefer to keep the code as it is unless you suggest any alternative. - Ayan > > Cheers, >
Hi, On 07/02/2023 15:34, Ayan Kumar Halder wrote: > > On 20/01/2023 11:06, Julien Grall wrote: >> Hi Ayan, > Hi Julien, >> >> On 17/01/2023 17:43, Ayan Kumar Halder wrote: >>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA. >>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, >>> "Virtualization Translation Control Register" for the bit descriptions. >>> >>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>> --- >>> Changes from - >>> >>> v1 - New patch. >>> >>> xen/arch/arm/p2m.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index 948f199d84..cfdea55e71 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) >>> register_t val = >>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >>> #ifdef CONFIG_ARM_32 >>> - if ( p2m_ipa_bits < 40 ) >>> + if ( p2m_ipa_bits < PADDR_BITS ) >>> 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; >>> + printk("P2M: %u-bit IPA\n",PADDR_BITS); >>> + p2m_ipa_bits = PADDR_BITS; >>> +#ifdef CONFIG_ARM_PA_32 >>> + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ >>> +#else >>> val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ >>> +#endif >> >> I am wondering whether this is right time to switch to an array like >> the arm64 code? This would allow to use 32-bit IPA also when Xen >> support 64-bit physical address. > > In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical > address range supported at runtime. This is then used as an index into > pa_range_info[] to determine t0sz, root_order, etc. It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to decide the size. > > However, for AArch32 I do not see an equivalent register (similar to > ID_AA64MMFR0_EL1) or any register to determine the physical address > range. Thus, I will prefer to keep the code as it is unless you suggest > any alternative. I looked at the Arm Arm and indeed it doesn't look like there are equivalent for ID_AA64MMFR0_EL1.PARange. However, my point was less about reading the system register but more about the fact we could have the code a bit more generic and avoid the assumption that PADDR_BITS is only modified when CONFIG_ARM_PA_32 is set. Cheers,
On 09/02/2023 11:45, Julien Grall wrote: > Hi, Hi Julien, > > On 07/02/2023 15:34, Ayan Kumar Halder wrote: >> >> On 20/01/2023 11:06, Julien Grall wrote: >>> Hi Ayan, >> Hi Julien, >>> >>> On 17/01/2023 17:43, Ayan Kumar Halder wrote: >>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA. >>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, >>>> "Virtualization Translation Control Register" for the bit >>>> descriptions. >>>> >>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>> --- >>>> Changes from - >>>> >>>> v1 - New patch. >>>> >>>> xen/arch/arm/p2m.c | 10 +++++++--- >>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>> index 948f199d84..cfdea55e71 100644 >>>> --- a/xen/arch/arm/p2m.c >>>> +++ b/xen/arch/arm/p2m.c >>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) >>>> register_t val = >>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >>>> #ifdef CONFIG_ARM_32 >>>> - if ( p2m_ipa_bits < 40 ) >>>> + if ( p2m_ipa_bits < PADDR_BITS ) >>>> 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; >>>> + printk("P2M: %u-bit IPA\n",PADDR_BITS); >>>> + p2m_ipa_bits = PADDR_BITS; >>>> +#ifdef CONFIG_ARM_PA_32 >>>> + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ >>>> +#else >>>> val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ >>>> +#endif >>> >>> I am wondering whether this is right time to switch to an array like >>> the arm64 code? This would allow to use 32-bit IPA also when Xen >>> support 64-bit physical address. >> >> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical >> address range supported at runtime. This is then used as an index >> into pa_range_info[] to determine t0sz, root_order, etc. > > It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to decide > the size. Ack. > >> >> However, for AArch32 I do not see an equivalent register (similar to >> ID_AA64MMFR0_EL1) or any register to determine the physical address >> range. Thus, I will prefer to keep the code as it is unless you >> suggest any alternative. > > I looked at the Arm Arm and indeed it doesn't look like there are > equivalent for ID_AA64MMFR0_EL1.PARange. > > However, my point was less about reading the system register but more > about the fact we could have the code a bit more generic and avoid the > assumption that PADDR_BITS is only modified when CONFIG_ARM_PA_32 is set. I had a rework at the patch. Please let me know if the following looks better. diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 948f199d84..bc3bdf5f3e 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void) register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; #ifdef CONFIG_ARM_32 - if ( p2m_ipa_bits < 40 ) + static const struct { + unsigned int pabits; /* Physical Address Size */ + unsigned int t0sz; /* Desired T0SZ */ + unsigned int sl0; /* Desired SL0 */ + } pa_range_info[] __initconst = { + [0] = { 32, 32, 1 }, + [1] = { 40, 24, 1 }, + }; + int i = 0; + + if ( p2m_ipa_bits < PADDR_BITS ) + panic("P2M: Not able to support %u-bit IPA at the moment\n", + p2m_ipa_bits); + + printk("P2M: %u-bit IPA\n",PADDR_BITS); + p2m_ipa_bits = PADDR_BITS; + + for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ ) + if ( p2m_ipa_bits == pa_range_info[i].pabits ) + break; + + if ( i == ARRAY_SIZE(pa_range_info) ) 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 */ + val |= VTCR_T0SZ(pa_range_info[i].t0sz); + val |= VTCR_SL0(pa_range_info[i].sl0); + - Ayan > > Cheers, >
Hi, On 10/02/2023 15:39, Ayan Kumar Halder wrote: > > On 09/02/2023 11:45, Julien Grall wrote: >> Hi, > Hi Julien, >> >> On 07/02/2023 15:34, Ayan Kumar Halder wrote: >>> >>> On 20/01/2023 11:06, Julien Grall wrote: >>>> Hi Ayan, >>> Hi Julien, >>>> >>>> On 17/01/2023 17:43, Ayan Kumar Halder wrote: >>>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA. >>>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, >>>>> "Virtualization Translation Control Register" for the bit >>>>> descriptions. >>>>> >>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>>> --- >>>>> Changes from - >>>>> >>>>> v1 - New patch. >>>>> >>>>> xen/arch/arm/p2m.c | 10 +++++++--- >>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>>> index 948f199d84..cfdea55e71 100644 >>>>> --- a/xen/arch/arm/p2m.c >>>>> +++ b/xen/arch/arm/p2m.c >>>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) >>>>> register_t val = >>>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >>>>> #ifdef CONFIG_ARM_32 >>>>> - if ( p2m_ipa_bits < 40 ) >>>>> + if ( p2m_ipa_bits < PADDR_BITS ) >>>>> 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; >>>>> + printk("P2M: %u-bit IPA\n",PADDR_BITS); >>>>> + p2m_ipa_bits = PADDR_BITS; >>>>> +#ifdef CONFIG_ARM_PA_32 >>>>> + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ >>>>> +#else >>>>> val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ >>>>> +#endif >>>> >>>> I am wondering whether this is right time to switch to an array like >>>> the arm64 code? This would allow to use 32-bit IPA also when Xen >>>> support 64-bit physical address. >>> >>> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the physical >>> address range supported at runtime. This is then used as an index >>> into pa_range_info[] to determine t0sz, root_order, etc. >> >> It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to decide >> the size. > Ack. >> >>> >>> However, for AArch32 I do not see an equivalent register (similar to >>> ID_AA64MMFR0_EL1) or any register to determine the physical address >>> range. Thus, I will prefer to keep the code as it is unless you >>> suggest any alternative. >> >> I looked at the Arm Arm and indeed it doesn't look like there are >> equivalent for ID_AA64MMFR0_EL1.PARange. >> >> However, my point was less about reading the system register but more >> about the fact we could have the code a bit more generic and avoid the >> assumption that PADDR_BITS is only modified when CONFIG_ARM_PA_32 is set. > > I had a rework at the patch. Please let me know if the following looks > better. > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 948f199d84..bc3bdf5f3e 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void) > register_t val = > VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; > > #ifdef CONFIG_ARM_32 > - if ( p2m_ipa_bits < 40 ) > + static const struct { > + unsigned int pabits; /* Physical Address Size */ > + unsigned int t0sz; /* Desired T0SZ */ > + unsigned int sl0; /* Desired SL0 */ Hmmm... Why don't you include the root order? In fact... > + } pa_range_info[] __initconst = { > + [0] = { 32, 32, 1 }, > + [1] = { 40, 24, 1 }, ... looking at the ARMv7 specification (see B3-1345 in ARM DDI 0406C.d), the root page-table is only concatenated for 40-bits. > + }; > + int i = 0; > + > + if ( p2m_ipa_bits < PADDR_BITS ) > + panic("P2M: Not able to support %u-bit IPA at the moment\n", > + p2m_ipa_bits); This check seems unnecessary now. > + > + printk("P2M: %u-bit IPA\n",PADDR_BITS); > + p2m_ipa_bits = PADDR_BITS; > + > + for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ ) > + if ( p2m_ipa_bits == pa_range_info[i].pabits ) > + break; > + > + if ( i == ARRAY_SIZE(pa_range_info) ) > 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 */ > + val |= VTCR_T0SZ(pa_range_info[i].t0sz); > + val |= VTCR_SL0(pa_range_info[i].sl0); > + I think you can share a lot of code between arm64 and arm32. A diff below (not tested): diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h index 91df922e1c9f..05f26780a29d 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 948f199d848f..6fda0b92230a 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -2264,17 +2264,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 */ @@ -2286,29 +2275,26 @@ void __init setup_virt_paging(void) [0] = { 32, 32/*32*/, 0, 1 }, [1] = { 36, 28/*28*/, 0, 1 }, [2] = { 40, 24/*24*/, 1, 1 }, +#ifdef CONFIG_ARM_64 [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 */ +#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". */ if ( pa_range_info[system_cpuinfo.mm64.pa_range].pabits < p2m_ipa_bits ) p2m_ipa_bits = pa_range_info[system_cpuinfo.mm64.pa_range].pabits; - - /* - * cpu info sanitization made sure we support 16bits VMID only if all - * cores are supporting it. - */ - 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++ ) @@ -2324,12 +2310,22 @@ 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); - val |= VTCR_PS(pa_range); - val |= VTCR_TG0_4K; +#ifdef CONFIG_ARM_64 + /* + * cpu info sanitization made sure we support 16bits VMID only if all + * cores are supporting it. + */ + if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT ) + max_vmid = MAX_VMID_16_BIT; /* Set the VS bit only if 16 bit VMID is supported. */ if ( MAX_VMID == MAX_VMID_16_BIT ) val |= VTCR_VS; +#endif + + val |= VTCR_PS(pa_range); + val |= VTCR_TG0_4K; + val |= VTCR_SL0(pa_range_info[pa_range].sl0); val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz); @@ -2341,7 +2337,7 @@ void __init setup_virt_paging(void) 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,
On 10/02/2023 16:19, Julien Grall wrote: > Hi, Hi Julien, I need some inputs. > > On 10/02/2023 15:39, Ayan Kumar Halder wrote: >> >> On 09/02/2023 11:45, Julien Grall wrote: >>> Hi, >> Hi Julien, >>> >>> On 07/02/2023 15:34, Ayan Kumar Halder wrote: >>>> >>>> On 20/01/2023 11:06, Julien Grall wrote: >>>>> Hi Ayan, >>>> Hi Julien, >>>>> >>>>> On 17/01/2023 17:43, Ayan Kumar Halder wrote: >>>>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA. >>>>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, >>>>>> "Virtualization Translation Control Register" for the bit >>>>>> descriptions. >>>>>> >>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>>>> --- >>>>>> Changes from - >>>>>> >>>>>> v1 - New patch. >>>>>> >>>>>> xen/arch/arm/p2m.c | 10 +++++++--- >>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>>>> index 948f199d84..cfdea55e71 100644 >>>>>> --- a/xen/arch/arm/p2m.c >>>>>> +++ b/xen/arch/arm/p2m.c >>>>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) >>>>>> register_t val = >>>>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >>>>>> #ifdef CONFIG_ARM_32 >>>>>> - if ( p2m_ipa_bits < 40 ) >>>>>> + if ( p2m_ipa_bits < PADDR_BITS ) >>>>>> 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; >>>>>> + printk("P2M: %u-bit IPA\n",PADDR_BITS); >>>>>> + p2m_ipa_bits = PADDR_BITS; >>>>>> +#ifdef CONFIG_ARM_PA_32 >>>>>> + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ >>>>>> +#else >>>>>> val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ >>>>>> +#endif >>>>> >>>>> I am wondering whether this is right time to switch to an array >>>>> like the arm64 code? This would allow to use 32-bit IPA also when >>>>> Xen support 64-bit physical address. >>>> >>>> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the >>>> physical address range supported at runtime. This is then used as >>>> an index into pa_range_info[] to determine t0sz, root_order, etc. >>> >>> It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to >>> decide the size. >> Ack. >>> >>>> >>>> However, for AArch32 I do not see an equivalent register (similar >>>> to ID_AA64MMFR0_EL1) or any register to determine the physical >>>> address range. Thus, I will prefer to keep the code as it is unless >>>> you suggest any alternative. >>> >>> I looked at the Arm Arm and indeed it doesn't look like there are >>> equivalent for ID_AA64MMFR0_EL1.PARange. >>> >>> However, my point was less about reading the system register but >>> more about the fact we could have the code a bit more generic and >>> avoid the assumption that PADDR_BITS is only modified when >>> CONFIG_ARM_PA_32 is set. >> >> I had a rework at the patch. Please let me know if the following >> looks better. >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 948f199d84..bc3bdf5f3e 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void) >> register_t val = >> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >> >> #ifdef CONFIG_ARM_32 >> - if ( p2m_ipa_bits < 40 ) >> + static const struct { >> + unsigned int pabits; /* Physical Address Size */ >> + unsigned int t0sz; /* Desired T0SZ */ >> + unsigned int sl0; /* Desired SL0 */ > > Hmmm... Why don't you include the root order? In fact... > >> + } pa_range_info[] __initconst = { >> + [0] = { 32, 32, 1 }, >> + [1] = { 40, 24, 1 }, > > ... looking at the ARMv7 specification (see B3-1345 in ARM DDI > 0406C.d), the root page-table is only concatenated for 40-bits. Sorry, I could not follow how you inferred this. Can you paste the relevant snippet ? > >> + }; >> + int i = 0; >> + >> + if ( p2m_ipa_bits < PADDR_BITS ) >> + panic("P2M: Not able to support %u-bit IPA at the moment\n", >> + p2m_ipa_bits); > > This check seems unnecessary now. We still need this check as arm_smmu_device_cfg_probe() invokes p2m_restrict_ipa_bits(size). And referARM IHI 0062D.cID070116 (SMMU arch version 2.0 Specification), the IPA address size can be 0. 0b0000 32-bit. 1. 0b0001 36-bit. 10. 0b0010 40-bit. 11. 0b0011 42-bit. 100. 0b0100 44-bit. 101. 0b0101 48-bit. So if p2m_ipa_bits = 36 bits and PADDR_BITS = 40, then we want to panic. - Ayan > >> + >> + printk("P2M: %u-bit IPA\n",PADDR_BITS); >> + p2m_ipa_bits = PADDR_BITS; >> + >> + for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ ) >> + if ( p2m_ipa_bits == pa_range_info[i].pabits ) >> + break; >> + >> + if ( i == ARRAY_SIZE(pa_range_info) ) >> 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 */ >> + val |= VTCR_T0SZ(pa_range_info[i].t0sz); >> + val |= VTCR_SL0(pa_range_info[i].sl0); >> + > > I think you can share a lot of code between arm64 and arm32. A diff > below (not tested): > > diff --git a/xen/arch/arm/include/asm/p2m.h > b/xen/arch/arm/include/asm/p2m.h > index 91df922e1c9f..05f26780a29d 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 948f199d848f..6fda0b92230a 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -2264,17 +2264,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 */ > @@ -2286,29 +2275,26 @@ void __init setup_virt_paging(void) > [0] = { 32, 32/*32*/, 0, 1 }, > [1] = { 36, 28/*28*/, 0, 1 }, > [2] = { 40, 24/*24*/, 1, 1 }, > +#ifdef CONFIG_ARM_64 > [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 */ > +#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". > */ > if ( pa_range_info[system_cpuinfo.mm64.pa_range].pabits < > p2m_ipa_bits ) > p2m_ipa_bits = > pa_range_info[system_cpuinfo.mm64.pa_range].pabits; > - > - /* > - * cpu info sanitization made sure we support 16bits VMID only if > all > - * cores are supporting it. > - */ > - 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++ ) > @@ -2324,12 +2310,22 @@ 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); > > - val |= VTCR_PS(pa_range); > - val |= VTCR_TG0_4K; > +#ifdef CONFIG_ARM_64 > + /* > + * cpu info sanitization made sure we support 16bits VMID only if > all > + * cores are supporting it. > + */ > + if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT ) > + max_vmid = MAX_VMID_16_BIT; > > /* Set the VS bit only if 16 bit VMID is supported. */ > if ( MAX_VMID == MAX_VMID_16_BIT ) > val |= VTCR_VS; > +#endif > + > + val |= VTCR_PS(pa_range); > + val |= VTCR_TG0_4K; > + > val |= VTCR_SL0(pa_range_info[pa_range].sl0); > val |= VTCR_T0SZ(pa_range_info[pa_range].t0sz); > > @@ -2341,7 +2337,7 @@ void __init setup_virt_paging(void) > 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, >
On 10/02/2023 17:51, Ayan Kumar Halder wrote: > > On 10/02/2023 16:19, Julien Grall wrote: >> Hi, >> On 10/02/2023 15:39, Ayan Kumar Halder wrote: >>> >>> On 09/02/2023 11:45, Julien Grall wrote: >>>> Hi, >>> Hi Julien, >>>> >>>> On 07/02/2023 15:34, Ayan Kumar Halder wrote: >>>>> >>>>> On 20/01/2023 11:06, Julien Grall wrote: >>>>>> Hi Ayan, >>>>> Hi Julien, >>>>>> >>>>>> On 17/01/2023 17:43, Ayan Kumar Halder wrote: >>>>>>> VTCR.T0SZ should be set as 0x20 to support 32bit IPA. >>>>>>> Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, >>>>>>> "Virtualization Translation Control Register" for the bit >>>>>>> descriptions. >>>>>>> >>>>>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> >>>>>>> --- >>>>>>> Changes from - >>>>>>> >>>>>>> v1 - New patch. >>>>>>> >>>>>>> xen/arch/arm/p2m.c | 10 +++++++--- >>>>>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>>>>> index 948f199d84..cfdea55e71 100644 >>>>>>> --- a/xen/arch/arm/p2m.c >>>>>>> +++ b/xen/arch/arm/p2m.c >>>>>>> @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) >>>>>>> register_t val = >>>>>>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >>>>>>> #ifdef CONFIG_ARM_32 >>>>>>> - if ( p2m_ipa_bits < 40 ) >>>>>>> + if ( p2m_ipa_bits < PADDR_BITS ) >>>>>>> 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; >>>>>>> + printk("P2M: %u-bit IPA\n",PADDR_BITS); >>>>>>> + p2m_ipa_bits = PADDR_BITS; >>>>>>> +#ifdef CONFIG_ARM_PA_32 >>>>>>> + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ >>>>>>> +#else >>>>>>> val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ >>>>>>> +#endif >>>>>> >>>>>> I am wondering whether this is right time to switch to an array >>>>>> like the arm64 code? This would allow to use 32-bit IPA also when >>>>>> Xen support 64-bit physical address. >>>>> >>>>> In AArch64, we use ID_AA64MMFR0_EL1.PARange to determine the >>>>> physical address range supported at runtime. This is then used as >>>>> an index into pa_range_info[] to determine t0sz, root_order, etc. >>>> >>>> It is using both the ID_AA64MMFR0_EL1 but also p2m_ipa_bits to >>>> decide the size. >>> Ack. >>>> >>>>> >>>>> However, for AArch32 I do not see an equivalent register (similar >>>>> to ID_AA64MMFR0_EL1) or any register to determine the physical >>>>> address range. Thus, I will prefer to keep the code as it is unless >>>>> you suggest any alternative. >>>> >>>> I looked at the Arm Arm and indeed it doesn't look like there are >>>> equivalent for ID_AA64MMFR0_EL1.PARange. >>>> >>>> However, my point was less about reading the system register but >>>> more about the fact we could have the code a bit more generic and >>>> avoid the assumption that PADDR_BITS is only modified when >>>> CONFIG_ARM_PA_32 is set. >>> >>> I had a rework at the patch. Please let me know if the following >>> looks better. >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index 948f199d84..bc3bdf5f3e 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -2266,14 +2266,35 @@ void __init setup_virt_paging(void) >>> register_t val = >>> VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; >>> >>> #ifdef CONFIG_ARM_32 >>> - if ( p2m_ipa_bits < 40 ) >>> + static const struct { >>> + unsigned int pabits; /* Physical Address Size */ >>> + unsigned int t0sz; /* Desired T0SZ */ >>> + unsigned int sl0; /* Desired SL0 */ >> >> Hmmm... Why don't you include the root order? In fact... >> >>> + } pa_range_info[] __initconst = { >>> + [0] = { 32, 32, 1 }, >>> + [1] = { 40, 24, 1 }, >> >> ... looking at the ARMv7 specification (see B3-1345 in ARM DDI >> 0406C.d), the root page-table is only concatenated for 40-bits. > Sorry, I could not follow how you inferred this. Can you paste the > relevant snippet ? Use of concatenated second-level translation tables A stage 2 translation with an input address range of 31-34 bits can start the translation either: • With a first-level lookup, accessing a first-level translation table with 2-16 entries. • With a second-level lookup, accessing a set of concatenated second-level translation tables. >> >>> + }; >>> + int i = 0; >>> + >>> + if ( p2m_ipa_bits < PADDR_BITS ) >>> + panic("P2M: Not able to support %u-bit IPA at the moment\n", >>> + p2m_ipa_bits); >> >> This check seems unnecessary now. > > We still need this check as arm_smmu_device_cfg_probe() invokes > p2m_restrict_ipa_bits(size). > > And referARM IHI 0062D.cID070116 (SMMU arch version 2.0 Specification), > the IPA address size can be > > 0. > > 0b0000 32-bit. > > 1. > > 0b0001 36-bit. > > 10. > > 0b0010 40-bit. > > 11. > > 0b0011 42-bit. > > 100. > > 0b0100 44-bit. > > 101. > > 0b0101 48-bit. > > So if p2m_ipa_bits = 36 bits and PADDR_BITS = 40, then we want to panic. We can have the same situation on Arm64 (PADDR_BITS = 48), yet we don't panic(). So I don't quite understand why we would need to differ... Or are you saying that for 64-bit we also need such check? If so, then I am still not sure why because p2m_ipa_bits represent the guest physical address space and PADDR_BITS the Xen physical address space. It is valid to have both of them differing. Cheers,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 948f199d84..cfdea55e71 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -2266,13 +2266,17 @@ void __init setup_virt_paging(void) register_t val = VTCR_RES1|VTCR_SH0_IS|VTCR_ORGN0_WBWA|VTCR_IRGN0_WBWA; #ifdef CONFIG_ARM_32 - if ( p2m_ipa_bits < 40 ) + if ( p2m_ipa_bits < PADDR_BITS ) 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; + printk("P2M: %u-bit IPA\n",PADDR_BITS); + p2m_ipa_bits = PADDR_BITS; +#ifdef CONFIG_ARM_PA_32 + val |= VTCR_T0SZ(0x20); /* 32 bit IPA */ +#else val |= VTCR_T0SZ(0x18); /* 40 bit IPA */ +#endif val |= VTCR_SL0(0x1); /* P2M starts at first level */ #else /* CONFIG_ARM_64 */ static const struct {
VTCR.T0SZ should be set as 0x20 to support 32bit IPA. Refer ARM DDI 0487I.a ID081822, G8-9824, G8.2.171, VTCR, "Virtualization Translation Control Register" for the bit descriptions. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- Changes from - v1 - New patch. xen/arch/arm/p2m.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)