Re: [XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr

[Julien Grall <julien@xxxxxxx>]

Hi,

Each adaptations are distinct, so I would prefer if they are in in 
separate patch.
This will also make clear which components you touched because I would 
be surprised if this is really the only place where we need adaptation. 
Maybe that's because you didn't compile everything (which is fine).
On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> 1. Supersections are supported only for paddr greater than 32 bits.
Two questions:
 * Can you outline why we can't keep the code around?
 * Can you give a pointer to the Arm Arm that says supersection is not 
supported?
> 2. Use 0 wherever physical addresses are right shifted for 32 bit > 3. Use 
> PRIx64 to print u64
It would be good to explain that the current use was buggy because we 
are printing a PTE and not a physical address.
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
>   xen/arch/arm/guest_walk.c          | 2 ++
>   xen/arch/arm/mm.c                  | 2 +-
>   xen/drivers/passthrough/arm/smmu.c | 5 +++++
>   3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index 43d3215304..4384068285 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -149,6 +149,7 @@ static bool guest_walk_sd(const struct vcpu *v,
>               mask = (1ULL << L1DESC_SECTION_SHIFT) - 1;
>               *ipa = ((paddr_t)pte.sec.base << L1DESC_SECTION_SHIFT) | (gva & 
> mask);
>           }
> +#ifndef CONFIG_ARM_PA_32
A "malicious" guest can still set that bit. So now, you will return from 
guest_walk_sd() with 'ipa' not set at all.
If this is effectively not supported, then we should return 'false' 
rather than claiming the translation was successful.
>           else /* Supersection */
>           {
>               mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
> @@ -157,6 +158,7 @@ static bool guest_walk_sd(const struct vcpu *v,
>               *ipa |= (paddr_t)(pte.supersec.extbase1) << 
> L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
>               *ipa |= (paddr_t)(pte.supersec.extbase2) << 
> L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
>           }
> +#endif
>   
>           /* Set permissions so that the caller can check the flags by herself. */
>           if ( !pte.sec.ro )
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index be939fb106..3bc9894008 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -229,7 +229,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>   
>           pte = mapping[offsets[level]];
>   
> -        printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
> +        printk("%s[0x%03x] = 0x%"PRIx64"\n",
>                  level_strs[level], offsets[level], pte.bits);
>   
>           if ( level == 3 || !pte.walk.valid || !pte.walk.table )
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index 5ae180a4cc..522a478ccf 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1184,7 +1184,12 @@ static void arm_smmu_init_context_bank(struct 
> arm_smmu_domain *smmu_domain)
>   
>   	reg = (p2maddr & ((1ULL << 32) - 1));
>         writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
> +
> +#ifndef CONFIG_ARM_PA_32
>         reg = (p2maddr >> 32);
> +#else
> +       reg = 0;
> +#endif
I think it would be better if we implement writeq(). This will also 
avoid the now strange ((1ULL << 32) - 1) when p2maddr is a paddr_t.
>         if (stage1)
>                 reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
>         writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
Cheers,

--
Julien Grall