Re: [Xen-devel] [PATCH] [RFC] xen/arm: Restrict "pa_range" according to the IOMMU requirements

[Oleksandr <olekstysh@xxxxxxxxx>]

On 22.08.19 15:46, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien.


Thank you for the review.


> Please try to get your patch on the latest Xen before sending it. So 
> you can get the right people CCed. For instance, Volodymyr has not 
> been CCed as a reviewer.
Ooh, next time will do. Sorry for that.


> On 21/08/2019 19:17, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > There is a strict requirement for the IOMMU which wants to share
> > the P2M table with the CPU. The maximum supported by the IOMMU
> > Stage-2 input size must be greater or equal to the P2M IPA size.
> >
> > So, first initialize the IOMMU and gather the requirements and
> > then initialize the P2M. In the P2M code, take into the account
> > the IOMMU requirements and restrict "pa_range" if necessary.
> All the code you modify is arm64 specific. For arm32, the number of 
> IPA bits is hardcoded. So if you modify p2m_ipa_bits, you would end up 
> to misconfigure VTCR.
Indeed, will guard with #ifdef CONFIG_ARM_64.


> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> >
> > CC: Julien Grall <julien.grall@xxxxxxx>
> >
> > Why RFC?
> >
> > 1. Patch assumes that IPMMU support is already in.
> > 2. Not sure for the SMMU.
> >
> > If there are no objections I will craft a proper patch.
> > ---
> >   xen/arch/arm/p2m.c                       | 19 +++++++++++++++++--
> >   xen/arch/arm/setup.c                     |  4 ++--
> >   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 20 +++++---------------
> >   xen/drivers/passthrough/arm/smmu.c       | 14 +++++++-------
> >   4 files changed, 31 insertions(+), 26 deletions(-)
> >
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index c171568..1262ae9 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -34,7 +34,7 @@ static unsigned int __read_mostly max_vmid = 
> > MAX_VMID_8_BIT;
> >     #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
> >   -unsigned int __read_mostly p2m_ipa_bits;
> > +unsigned int __read_mostly p2m_ipa_bits = 0;
> Any uninitialized global variables are 0 by default. But I think 0 
> will not be correct here. You are assuming all the IOMMUs can support 
> the same number of IPA bits.
Yes, this was my assumption.


> I am not aware if such platform exists, but this is not prevented by 
> the SMMU specification. So it would be possible to have one SMMU 
> supporting only 42-bits while the other would support 48-bits.
What do you think would be the proper action at the moment?


> >     /* Helpers to lookup the properties of each level */
> >   static const paddr_t level_masks[] =
> > @@ -1981,10 +1981,25 @@ void __init setup_virt_paging(void)
> >           [7] = { 0 }  /* Invalid */
> >       };
> >   -    unsigned int cpu;
> > +    unsigned int i, cpu;
> >       unsigned int pa_range = 0x10; /* Larger than any possible value */
> >       bool vmid_8_bit = false;
> >   +    if ( iommu_enabled )
> > +    {
> > +        /* We expect "p2m_ipa_bits" to be updated by the IOMMU 
> > driver */
> I am not entirely happy that the IOMMU driver is updating p2m_ipa_bits 
> directly.
> It makes things fairly unclear when the value of p2m_ipa_bits will be 
> stable.
> I would prefer if we provide an helper that allow restricting the 
> size. Maybe p2m_restrict_ipa_bits(...).
Makes sense. Will implement.


> The helper can then decide how to deal with it.
OK. I am wondering, do we have cases when we shouldn't restrict the size 
(except cases when IOMMU wants wrong p2m_ipa_bits value)?



> > +        ASSERT(p2m_ipa_bits);
> > +
> > +        /* We need to restrict "pa_range" according to the IOMMU 
> > requirements */
> > +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
> > +        {
> > +            if ( p2m_ipa_bits >= 64 - pa_range_info[i].t0sz )
> While I know the compiler will do the right things, this is slightly 
> confusing to read. Please add 64 - pa_range_info... between parenthesis.
Will do.


> But I think you can just check against t0sz here. Also, it feels to me 
> a strict equality would be better. If p2m_ipa_bits is neither of the 
> value specified here, then most likely sharing the page tables was the 
> wrong thing to do.
I am afraid I don't completely understand why we need to check against 
t0sz. Or probably, you meant to check against pabits?

> > +                pa_range = i;
> > +            else
> > +                break;
> > +        }
> > +    }
> > +
> >       for_each_online_cpu ( cpu )
> >       {
> >           const struct cpuinfo_arm *info = &cpu_data[cpu];
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 51a6677..01cd83d 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -936,12 +936,12 @@ void __init start_xen(unsigned long 
> > boot_phys_offset,
> >       printk("Brought up %ld CPUs\n", (long)num_online_cpus());
> >       /* TODO: smp_cpus_done(); */
> >   -    setup_virt_paging();
> > -
> >       rc = iommu_setup();
> >       if ( !iommu_enabled && rc != -ENODEV )
> >           panic("Couldn't configure correctly all the IOMMUs.");
> >   +    setup_virt_paging();
> Please add a comment on top of either setup_virt_paging() (or 
> iommu_setup()) to explain the dependency between the two.
Yes, will add.


> > +
> >       do_initcalls();
> >         /*
> > diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
> > b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > index ec543c3..0dc6351 100644
> > --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > @@ -526,6 +526,7 @@ static int ipmmu_domain_init_context(struct 
> > ipmmu_vmsa_domain *domain)
> >        * to TTBR0. Use 4KB page granule. Start page table walks at 
> > first level.
> >        * Always bypass stage 1 translation.
> >        */
> > +    ASSERT(p2m_ipa_bits <= IPMMU_MAX_P2M_IPA_BITS);
> Why this ASSERT? I know that Xen is only supporting one kind of IOMMU, 
> but you could imagine a platform with various of them. So this may 
> trigger when it should not.
I didn't take into account the platform where IOMMUs could support the 
different number of IPA bits. What do you think would be the proper 
action at the moment?
I could bail out with error here (and for SMMU as well) to not allow 
creating guest domain for now...

--
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel