1. Patchew
  2. Xen
  3. View series

[PATCH] arm,smmu: match start level of page table walk with P2M

laurentiu.tudor@nxp.com posted 1 patch 3 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200928135157.3170-1-laurentiu.tudor@nxp.com
Maintainers: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>, Stefano Stabellini <sstabellini@kernel.org>, Julien Grall <julien@xen.org>
There is a newer version of this series
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(-)
[PATCH] arm,smmu: match start level of page table walk with P2M
Posted by laurentiu.tudor@nxp.com 3 years, 6 months ago 
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>
---
 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;
 
 /* 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
Re: [PATCH] arm,smmu: match start level of page table walk with P2M
Posted by Stefano Stabellini 3 years, 6 months ago 
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
>
Re: [PATCH] arm,smmu: match start level of page table walk with P2M
Posted by Laurentiu Tudor 3 years, 6 months ago 

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
>>
Re: [PATCH] arm,smmu: match start level of page table walk with P2M
Posted by Julien Grall 3 years, 6 months ago 
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,

-- 
Julien Grall
Re: [PATCH] arm,smmu: match start level of page table walk with P2M
Posted by Laurentiu Tudor 3 years, 6 months ago 

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
Re: [PATCH] arm,smmu: match start level of page table walk with P2M
Posted by Julien Grall 3 years, 6 months ago 
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,

-- 
Julien Grall

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.