Re: [Xen-devel] [RFC PATCH 4/9] xen/arm: p2m: Update IOMMU mapping whenever possible if page table is not shared

[Julien Grall <julien.grall@xxxxxxx>]

On 21/04/17 15:18, Oleksandr Tyshchenko wrote:
> On Wed, Apr 19, 2017 at 8:46 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> > Hi Oleksandr,
> Hi, Julien
Hi Oleksandr,

> > On 15/03/17 20:05, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > >
> > > Update IOMMU mapping if the IOMMU doesn't share page table with the CPU.
> > > The best place to do so on ARM is p2m_set_entry(). Use mfn as an indicator
> > > of the required action. If mfn is valid call iommu_map_pages(),
> > > otherwise - iommu_unmap_pages().
> >
> > Can you explain in the commit message why you do this change in
> > p2m_set_entry and not __p2m_set_entry?
> ok
>
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > ---
> > >  xen/arch/arm/p2m.c | 40 +++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 39 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index 1fc6ca3..84d3a09 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -979,7 +979,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
> > >      if ( p2m_valid(orig_pte) && entry->p2m.base != orig_pte.p2m.base )
> > >          p2m_free_entry(p2m, orig_pte, level);
> > >
> > > -    if ( need_iommu(p2m->domain) && (p2m_valid(orig_pte) ||
> > > p2m_valid(*entry)) )
> > > +    if ( need_iommu(p2m->domain) && iommu_use_hap_pt(d) &&
> > > +         (p2m_valid(orig_pte) || p2m_valid(*entry)) )
> > >          rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL <<
> > > page_order);
> > >      else
> > >          rc = 0;
> > > @@ -997,6 +998,9 @@ int p2m_set_entry(struct p2m_domain *p2m,
> > >                    p2m_type_t t,
> > >                    p2m_access_t a)
> > >  {
> > > +    unsigned long orig_nr = nr;
> > > +    gfn_t orig_sgfn = sgfn;
> > > +    mfn_t orig_smfn = smfn;
> > >      int rc = 0;
> > >
> > >      while ( nr )
> > > @@ -1029,6 +1033,40 @@ int p2m_set_entry(struct p2m_domain *p2m,
> > >          nr -= (1 << order);
> > >      }
> > >
> > > +    if ( likely(!rc) )
> >
> > I would do
> >
> > if ( unlikely(rc) )
> >   return rc;
> >
> > /* IOMMU map/unmap ... */
> >
> > This would remove one indentation layer of if.
> Agree.
>
> > > +    {
> > > +        /*
> > > +         * It's time to update IOMMU mapping if the latter doesn't
> > > +         * share page table with the CPU. Pass the whole memory block to
> > > let
> > > +         * the IOMMU code decide what to do with it.
> > > +         * The reason to update IOMMU mapping outside "while loop" is
> > > that
> > > +         * the IOMMU might not support the pages/superpages the CPU can
> > > deal
> > > +         * with (4K, 2M, 1G) and as result this will lead to non-optimal
> > > +         * mapping.
> >
> > My worry with this solution is someone may decide to use __p2m_set_entry
> > (see relinquish) directly because he knows the correct order. So the IOMMU
> > would be correctly handled when page table are shared and not when they are
> > "unshared".
> As for relinquish_p2m_mapping(), I was thinking about it, but I
> decided not to remove IOMMU mapping here since
> the whole IOMMU page table would be removed during complete_domain_destroy().
> But, I agree that nothing prevent someone to use __p2m_set_entry
> directly in future.
>
> > I would probably extend __p2m_get_entry with a new parameter indication
> > whether we want to map the page in the IOMMU directly or not.
> Sorry, I didn't get your point here. Did you mean __p2m_set_entry?
Yes sorry.

> > Also, do you expect the IOMMU to support a page granularity bigger than the
> > one supported by Xen? If not, we should probably have a check somewhere, to
> > prevent potential security issue as we would map more than expected.
> At the moment I keep in mind IPMMU only. And it supports the same page
> granularity as the CPU
> (4K, 2M, 1G). Could you, please, explain what a proposed check should check?
We should check that the smallest granularity supported by the IOMMU is 
inferior or equal to PAGE_SIZE. If not, then there will be more rework 
required in Xen to support correctly those IOMMUs.
For instance, Xen PAGE_SIZE is currently 4KB. If the IOMMUs only support 
64KB, then you will end up to map a bigger chunk than expected leading a 
guest device to access memory it should not access.
Supporting such IOMMU will require much more work in Xen than this small 
changes.

> With the current solution we pass the whole memory block to the IOMMU
> code and the IOMMU driver should handle this properly.
> If we pass, for example 1,1 GB, but the IOMMU driver supports 4K pages
> only then it has to split the memory block into 4K pages and process
> them.
The IOMMU driver will likely duplicate the same logic as in 
p2m_set_entry and today does not seem to be necessary. By that I mean 
splitting into smaller chunk.
You may need to split again those chunks if the IOMMU does not support 
the granularity. But this is less likely on current hardware.
My main concern is to make sure __p2m_get_entry works as expected with 
all the configurations. Currently, what you describe will require much 
more work than this series. I will be ok whether you rework 
__p2m_get_entry or not.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel