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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 16/12/2022 10:23, Julien Grall wrote:
> Hi,
Hi Julien,
> 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?
For PA_32, the following bitoperation will overflow :-

            *ipa |= (paddr_t)(pte.supersec.extbase1) << 
L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
            *ipa |= (paddr_t)(pte.supersec.extbase2) << 
L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
Also, pte.supersec.extbase1 and pte.supersec.extbase2 are not valid for 
PA_32. Refer xen/arch/arm/include/asm/short-desc.h
unsigned int extbase2:4;    /* Extended base address, PA[39:36] */

unsigned int extbase1:4;    /* Extended base address, PA[35:32] */

>  * Can you give a pointer to the Arm Arm that says supersection is not 
> supported?
I could not find any sentence in Arm Arm which says supersection is 
**not** supported on 32 bit PA.
However,

Refer"ARM DDI 0487I.a ID081822", G5.4 "The VMSAv8-32 Short-descriptor 
translation table format", G5-9163
"Support for Supersections is **optional**, except that an 
implementation that supports more than 32 bits of PA must also support 
Supersections to provide access to the entire PA space."
Also, G5.1.3 "Address spaces in VMSAv8-32", G5-9149

"AArch32 defines two translation table formats. The Long-descriptor 
format gives access to the full 40-bit IPA or PA space at a granularity 
of 4KB. The Short-descriptor format:
   Gives access to a 32-bit PA space at 4KB granularity.
   Gives access to a 40-bit PA space, but only at 16MB granularity, by 
the use of Supersections."
from the above 2 snippets, I inferred that supersections are only 
supported for PAs greater than 32 bits.
I could not find any evidence of supersections supported for 32 bit PA.

- Ayan

> > 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,