| Message ID | 20200928135157.3170-1-laurentiu.tudor@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm,smmu: match start level of page table walk with P2M | expand |
On Mon, 28 Sep 2020, laurentiu.tudor@nxp.com wrote: > From: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > Don't hardcode the lookup start level of the page table walk to 1 > and instead match the one used in P2M. This should fix scenarios > involving SMMU where the start level is different than 1. > > Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> Thank you for the patch, I think it is correct, except that smmu.c today can be enabled even on arm32 builds, where p2m_root_level would be uninitialized. We need to initialize p2m_root_level at the beginning of setup_virt_paging under the #ifdef CONFIG_ARM_32. We can statically initialize it to 1 in that case. Or... > --- > xen/arch/arm/p2m.c | 2 +- > xen/drivers/passthrough/arm/smmu.c | 2 +- > xen/include/asm-arm/p2m.h | 1 + > 3 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index ce59f2b503..0181b09dc0 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -18,7 +18,6 @@ > > #ifdef CONFIG_ARM_64 > static unsigned int __read_mostly p2m_root_order; > -static unsigned int __read_mostly p2m_root_level; > #define P2M_ROOT_ORDER p2m_root_order > #define P2M_ROOT_LEVEL p2m_root_level > static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; > @@ -39,6 +38,7 @@ static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; > * restricted by external entity (e.g. IOMMU). > */ > unsigned int __read_mostly p2m_ipa_bits = 64; > +unsigned int __read_mostly p2m_root_level; ... we could p2m_root_level = 1; here > /* Helpers to lookup the properties of each level */ > static const paddr_t level_masks[] = > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index 94662a8501..85709a136f 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -1152,7 +1152,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain) > (TTBCR_RGN_WBWA << TTBCR_IRGN0_SHIFT); > > if (!stage1) > - reg |= (TTBCR_SL0_LVL_1 << TTBCR_SL0_SHIFT); > + reg |= (2 - p2m_root_level) << TTBCR_SL0_SHIFT; > > writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR); > > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 5fdb6e8183..97b5eada2b 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -12,6 +12,7 @@ > > /* Holds the bit size of IPAs in p2m tables. */ > extern unsigned int p2m_ipa_bits; > +extern unsigned int p2m_root_level; > > struct domain; > > -- > 2.17.1 >
On 10/2/2020 2:52 AM, Stefano Stabellini wrote: > On Mon, 28 Sep 2020, laurentiu.tudor@nxp.com wrote: >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> >> Don't hardcode the lookup start level of the page table walk to 1 >> and instead match the one used in P2M. This should fix scenarios >> involving SMMU where the start level is different than 1. >> >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > Thank you for the patch, I think it is correct, except that smmu.c today > can be enabled even on arm32 builds, where p2m_root_level would be > uninitialized. > > We need to initialize p2m_root_level at the beginning of > setup_virt_paging under the #ifdef CONFIG_ARM_32. We can statically > initialize it to 1 in that case. Or... > > >> --- >> xen/arch/arm/p2m.c | 2 +- >> xen/drivers/passthrough/arm/smmu.c | 2 +- >> xen/include/asm-arm/p2m.h | 1 + >> 3 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index ce59f2b503..0181b09dc0 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -18,7 +18,6 @@ >> >> #ifdef CONFIG_ARM_64 >> static unsigned int __read_mostly p2m_root_order; >> -static unsigned int __read_mostly p2m_root_level; >> #define P2M_ROOT_ORDER p2m_root_order >> #define P2M_ROOT_LEVEL p2m_root_level >> static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; >> @@ -39,6 +38,7 @@ static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; >> * restricted by external entity (e.g. IOMMU). >> */ >> unsigned int __read_mostly p2m_ipa_bits = 64; >> +unsigned int __read_mostly p2m_root_level; > > ... we could p2m_root_level = 1; here > This looks straight forward and in line with what we do with p2m_ipa_bits, I'll send a v2 right away. Thanks for the review. --- Best Regards, Laurentiu >> /* Helpers to lookup the properties of each level */ >> static const paddr_t level_masks[] = >> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c >> index 94662a8501..85709a136f 100644 >> --- a/xen/drivers/passthrough/arm/smmu.c >> +++ b/xen/drivers/passthrough/arm/smmu.c >> @@ -1152,7 +1152,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain) >> (TTBCR_RGN_WBWA << TTBCR_IRGN0_SHIFT); >> >> if (!stage1) >> - reg |= (TTBCR_SL0_LVL_1 << TTBCR_SL0_SHIFT); >> + reg |= (2 - p2m_root_level) << TTBCR_SL0_SHIFT; >> >> writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR); >> >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >> index 5fdb6e8183..97b5eada2b 100644 >> --- a/xen/include/asm-arm/p2m.h >> +++ b/xen/include/asm-arm/p2m.h >> @@ -12,6 +12,7 @@ >> >> /* Holds the bit size of IPAs in p2m tables. */ >> extern unsigned int p2m_ipa_bits; >> +extern unsigned int p2m_root_level; >> >> struct domain; >> >> -- >> 2.17.1 >>
Hi, On 02/10/2020 00:52, Stefano Stabellini wrote: > On Mon, 28 Sep 2020, laurentiu.tudor@nxp.com wrote: >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> >> Don't hardcode the lookup start level of the page table walk to 1 >> and instead match the one used in P2M. This should fix scenarios >> involving SMMU where the start level is different than 1. >> >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> > > Thank you for the patch, I think it is correct, except that smmu.c today > can be enabled even on arm32 builds, where p2m_root_level would be > uninitialized. > > We need to initialize p2m_root_level at the beginning of > setup_virt_paging under the #ifdef CONFIG_ARM_32. We can statically > initialize it to 1 in that case. Or... > > >> --- >> xen/arch/arm/p2m.c | 2 +- >> xen/drivers/passthrough/arm/smmu.c | 2 +- >> xen/include/asm-arm/p2m.h | 1 + >> 3 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index ce59f2b503..0181b09dc0 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -18,7 +18,6 @@ >> >> #ifdef CONFIG_ARM_64 >> static unsigned int __read_mostly p2m_root_order; >> -static unsigned int __read_mostly p2m_root_level; >> #define P2M_ROOT_ORDER p2m_root_order >> #define P2M_ROOT_LEVEL p2m_root_level >> static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; >> @@ -39,6 +38,7 @@ static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; >> * restricted by external entity (e.g. IOMMU). >> */ >> unsigned int __read_mostly p2m_ipa_bits = 64; >> +unsigned int __read_mostly p2m_root_level; > > ... we could p2m_root_level = 1; here IMHO, this is going to make the code quite confusing given that only the SMMU would use this variable for arm32. The P2M root level also cannot be changed by the SMMU (at least for now). So I would suggest to introduce a helper (maybe p2m_get_root_level()) and use it in the SMMU code. An alternative would be to move the definition of P2M_ROOT_{ORDER, LEVEL} in p2m.h Cheers,
On 10/2/2020 11:18 AM, Julien Grall wrote: > Hi, > > On 02/10/2020 00:52, Stefano Stabellini wrote: >> On Mon, 28 Sep 2020, laurentiu.tudor@nxp.com wrote: >>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>> >>> Don't hardcode the lookup start level of the page table walk to 1 >>> and instead match the one used in P2M. This should fix scenarios >>> involving SMMU where the start level is different than 1. >>> >>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> >> >> Thank you for the patch, I think it is correct, except that smmu.c today >> can be enabled even on arm32 builds, where p2m_root_level would be >> uninitialized. >> >> We need to initialize p2m_root_level at the beginning of >> setup_virt_paging under the #ifdef CONFIG_ARM_32. We can statically >> initialize it to 1 in that case. Or... >> >> >>> --- >>> xen/arch/arm/p2m.c | 2 +- >>> xen/drivers/passthrough/arm/smmu.c | 2 +- >>> xen/include/asm-arm/p2m.h | 1 + >>> 3 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index ce59f2b503..0181b09dc0 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -18,7 +18,6 @@ >>> #ifdef CONFIG_ARM_64 >>> static unsigned int __read_mostly p2m_root_order; >>> -static unsigned int __read_mostly p2m_root_level; >>> #define P2M_ROOT_ORDER p2m_root_order >>> #define P2M_ROOT_LEVEL p2m_root_level >>> static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; >>> @@ -39,6 +38,7 @@ static unsigned int __read_mostly max_vmid = >>> MAX_VMID_8_BIT; >>> * restricted by external entity (e.g. IOMMU). >>> */ >>> unsigned int __read_mostly p2m_ipa_bits = 64; >>> +unsigned int __read_mostly p2m_root_level; >> >> ... we could p2m_root_level = 1; here > > IMHO, this is going to make the code quite confusing given that only the > SMMU would use this variable for arm32. > > The P2M root level also cannot be changed by the SMMU (at least for > now). So I would suggest to introduce a helper (maybe > p2m_get_root_level()) and use it in the SMMU code. > > An alternative would be to move the definition of P2M_ROOT_{ORDER, > LEVEL} in p2m.h Alright, I'll go with this second option if that's ok with you. --- Thanks & Best Regards, Laurentiu
Hi, On 02/10/2020 10:29, Laurentiu Tudor wrote: > > > On 10/2/2020 11:18 AM, Julien Grall wrote: >> Hi, >> >> On 02/10/2020 00:52, Stefano Stabellini wrote: >>> On Mon, 28 Sep 2020, laurentiu.tudor@nxp.com wrote: >>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>>> >>>> Don't hardcode the lookup start level of the page table walk to 1 >>>> and instead match the one used in P2M. This should fix scenarios >>>> involving SMMU where the start level is different than 1. >>>> >>>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com> >>> >>> Thank you for the patch, I think it is correct, except that smmu.c today >>> can be enabled even on arm32 builds, where p2m_root_level would be >>> uninitialized. >>> >>> We need to initialize p2m_root_level at the beginning of >>> setup_virt_paging under the #ifdef CONFIG_ARM_32. We can statically >>> initialize it to 1 in that case. Or... >>> >>> >>>> --- >>>> xen/arch/arm/p2m.c | 2 +- >>>> xen/drivers/passthrough/arm/smmu.c | 2 +- >>>> xen/include/asm-arm/p2m.h | 1 + >>>> 3 files changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>> index ce59f2b503..0181b09dc0 100644 >>>> --- a/xen/arch/arm/p2m.c >>>> +++ b/xen/arch/arm/p2m.c >>>> @@ -18,7 +18,6 @@ >>>> #ifdef CONFIG_ARM_64 >>>> static unsigned int __read_mostly p2m_root_order; >>>> -static unsigned int __read_mostly p2m_root_level; >>>> #define P2M_ROOT_ORDER p2m_root_order >>>> #define P2M_ROOT_LEVEL p2m_root_level >>>> static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; >>>> @@ -39,6 +38,7 @@ static unsigned int __read_mostly max_vmid = >>>> MAX_VMID_8_BIT; >>>> * restricted by external entity (e.g. IOMMU). >>>> */ >>>> unsigned int __read_mostly p2m_ipa_bits = 64; >>>> +unsigned int __read_mostly p2m_root_level; >>> >>> ... we could p2m_root_level = 1; here >> >> IMHO, this is going to make the code quite confusing given that only the >> SMMU would use this variable for arm32. >> >> The P2M root level also cannot be changed by the SMMU (at least for >> now). So I would suggest to introduce a helper (maybe >> p2m_get_root_level()) and use it in the SMMU code. >> >> An alternative would be to move the definition of P2M_ROOT_{ORDER, >> LEVEL} in p2m.h > > Alright, I'll go with this second option if that's ok with you. I am fine with that. Cheers,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index ce59f2b503..0181b09dc0 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -18,7 +18,6 @@ #ifdef CONFIG_ARM_64 static unsigned int __read_mostly p2m_root_order; -static unsigned int __read_mostly p2m_root_level; #define P2M_ROOT_ORDER p2m_root_order #define P2M_ROOT_LEVEL p2m_root_level static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; @@ -39,6 +38,7 @@ static unsigned int __read_mostly max_vmid = MAX_VMID_8_BIT; * restricted by external entity (e.g. IOMMU). */ unsigned int __read_mostly p2m_ipa_bits = 64; +unsigned int __read_mostly p2m_root_level; /* Helpers to lookup the properties of each level */ static const paddr_t level_masks[] = diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 94662a8501..85709a136f 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -1152,7 +1152,7 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain) (TTBCR_RGN_WBWA << TTBCR_IRGN0_SHIFT); if (!stage1) - reg |= (TTBCR_SL0_LVL_1 << TTBCR_SL0_SHIFT); + reg |= (2 - p2m_root_level) << TTBCR_SL0_SHIFT; writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBCR); diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 5fdb6e8183..97b5eada2b 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -12,6 +12,7 @@ /* Holds the bit size of IPAs in p2m tables. */ extern unsigned int p2m_ipa_bits; +extern unsigned int p2m_root_level; struct domain;