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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/10/2020 10:47, 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.
> In order for the SMMU driver to also compile on arm32 move the
> P2M_ROOT_LEVEL in the p2m header file (while at it, for
> consistency also P2M_ROOT_ORDER) and use the macro in the smmu
> driver.
>
> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
> ---
> Changes in v2:
>   - made smmu driver compile on arm32
>
>   xen/arch/arm/p2m.c                 |  7 +------
>   xen/drivers/passthrough/arm/smmu.c |  2 +-
>   xen/include/asm-arm/p2m.h          | 10 ++++++++++
>   3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index ce59f2b503..bb75f12486 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -18,16 +18,10 @@
>   
>   #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;
>   /* VMID is by default 8 bit width on AArch64 */
>   #define MAX_VMID       max_vmid
>   #else
> -/* First level P2M is always 2 consecutive pages */
> -#define P2M_ROOT_LEVEL 1
> -#define P2M_ROOT_ORDER    1
>   /* VMID is always 8 bit width on AArch32 */
>   #define MAX_VMID        MAX_VMID_8_BIT
>   #endif
> @@ -39,6 +33,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;
This wants to stay in the #ifdef CONFIG_ARM_64 above and...

>   
>   /* 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..4ba6d3ab94 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..ab02b36a03 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -12,6 +12,16 @@
>   
>   /* Holds the bit size of IPAs in p2m tables.  */
>   extern unsigned int p2m_ipa_bits;
> +extern unsigned int p2m_root_level;
... this wants to be in part of the #ifdef below.

> +
> +#ifdef CONFIG_ARM_64
> +#define P2M_ROOT_ORDER    p2m_root_order
As you move the define here, you should also move 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;
Cheers,

--
Julien Grall