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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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


>  /* 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
>