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

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall