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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 02/10/2020 10:29, Laurentiu Tudor wrote:
> On 10/2/2020 11:18 AM, Julien Grall wrote:
> > Hi,
> >
> > On 02/10/2020 00:52, 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
> > IMHO, this is going to make the code quite confusing given that only the
> > SMMU would use this variable for arm32.
> >
> > The P2M root level also cannot be changed by the SMMU (at least for
> > now). So I would suggest to introduce a helper (maybe
> > p2m_get_root_level()) and use it in the SMMU code.
> >
> > An alternative would be to move the definition of P2M_ROOT_{ORDER,
> > LEVEL} in p2m.h
> Alright, I'll go with this second option if that's ok with you.
I am fine with that.

Cheers,

--
Julien Grall