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

[Julien Grall <julien.grall@xxxxxxx>]

On 8/22/19 6:06 PM, Oleksandr wrote:
> On 22.08.19 15:46, Julien Grall wrote:
> > Hi Oleksandr,
> Hi Julien.
Hi,

> > 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.
I would rather no guard any use in the IOMMU code with CONFIG_ARM_64. 
The problem is exactly the same if Arm32.
Rather than trying to limit it, I would just check that the IOMMU are 
able to support at least 40 bits. This can be done in setup_virt_paging().
> > > 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?
We should compute the minimum of the maximum IPA bits that any IOMMU can 
support. In the example I gave, it would be 42-bits.
> > >     /* 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)?
I don't think we should check in that helper whether the p2m_ipa_bits is 
wrong. This is up to the setup_virt_paging() to decide.
In this helper, we would only compute the minimum of the maximum. My 
point here is we can then decide whether we use p2m_ipa_bits or use a 
separate variable that is not exposed outside of p2m.c.
> > > +        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?
I meant pabits. Sorry.

Since commit, 896ebdfa3a "xen/arm: p2m: configure stage-2 page table to 
support upto 42-bit PA systems", stage-2 will always be configured with 
PA bits == IPA bits. So 64 - t0sz == pabits.
We should probably have a cleanup patch to remove t0sz and making 
clearer the correlation between the two.
> > > +                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...
See my suggestion above.

Cheers,

--
Julien Grall

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