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

[Laurentiu Tudor <laurentiu.tudor@xxxxxxx>]

On 10/2/2020 2:52 AM, Stefano Stabellini wrote:
> On Mon, 28 Sep 2020, laurentiu.tudor@xxxxxxx wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
>>
>> 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@xxxxxxx>
> 
> 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
>>