Re: [PATCH v5 09/15] vtd: add lookup_page method to iommu_ops

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Tian, Kevin [mailto:kevin.tian@intel.com]
> Sent: 07 August 2018 09:31
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; George Dunlap
> <George.Dunlap@citrix.com>
> Subject: RE: [PATCH v5 09/15] vtd: add lookup_page method to iommu_ops
> 
> > From: Paul Durrant [mailto:Paul.Durrant@citrix.com]
> > Sent: Tuesday, August 7, 2018 4:22 PM
> >
> > > -----Original Message-----
> > > From: Tian, Kevin [mailto:kevin.tian@intel.com]
> > > Sent: 07 August 2018 04:25
> > > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-
> > devel@lists.xenproject.org
> > > Cc: Jan Beulich <jbeulich@suse.com>; George Dunlap
> > > <George.Dunlap@citrix.com>
> > > Subject: RE: [PATCH v5 09/15] vtd: add lookup_page method to
> > iommu_ops
> > >
> > > > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > > > Sent: Saturday, August 4, 2018 1:22 AM
> > > >
> > > > This patch adds a new method to the VT-d IOMMU implementation to
> > find
> > > > the
> > > > MFN currently mapped by the specified BFN along with a wrapper
> > function
> > > > in
> > > > generic IOMMU code to call the implementation if it exists.
> > > >
> > > > This functionality will be used by a subsequent patch.
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > Cc: George Dunlap <george.dunlap@citrix.com>
> > > >
> > > > v3:
> > > >  - Addressed comments from George.
> > > >
> > > > v2:
> > > >  - Addressed some comments from Jan.
> > > > ---
> > > >  xen/drivers/passthrough/iommu.c     | 11 +++++++++++
> > > >  xen/drivers/passthrough/vtd/iommu.c | 34
> > > > ++++++++++++++++++++++++++++++++++
> > > >  xen/drivers/passthrough/vtd/iommu.h |  3 +++
> > > >  xen/include/xen/iommu.h             |  4 ++++
> > > >  4 files changed, 52 insertions(+)
> > > >
> > > > diff --git a/xen/drivers/passthrough/iommu.c
> > > > b/xen/drivers/passthrough/iommu.c
> > > > index b10a37e5d7..9b7baca93f 100644
> > > > --- a/xen/drivers/passthrough/iommu.c
> > > > +++ b/xen/drivers/passthrough/iommu.c
> > > > @@ -305,6 +305,17 @@ int iommu_unmap_page(struct domain *d,
> > bfn_t
> > > > bfn)
> > > >      return rc;
> > > >  }
> > > >
> > > > +int iommu_lookup_page(struct domain *d, bfn_t bfn, mfn_t *mfn,
> > > > +                      unsigned int *flags)
> > > > +{
> > > > +    const struct domain_iommu *hd = dom_iommu(d);
> > > > +
> > > > +    if ( !iommu_enabled || !hd->platform_ops )
> > > > +        return -EOPNOTSUPP;
> > > > +
> > > > +    return hd->platform_ops->lookup_page(d, bfn, mfn, flags);
> > > > +}
> > > > +
> > > >  static void iommu_free_pagetables(unsigned long unused)
> > > >  {
> > > >      do {
> > > > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > > > b/xen/drivers/passthrough/vtd/iommu.c
> > > > index 282e227414..8cd3b59aa0 100644
> > > > --- a/xen/drivers/passthrough/vtd/iommu.c
> > > > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > > > @@ -1830,6 +1830,39 @@ static int __must_check
> > > > intel_iommu_unmap_page(struct domain *d,
> > > >      return dma_pte_clear_one(d, bfn_to_baddr(bfn));
> > > >  }
> > > >
> > > > +static int intel_iommu_lookup_page(struct domain *d, bfn_t bfn,
> > mfn_t
> > > > *mfn,
> > > > +                                   unsigned int *flags)
> > >
> > > Not looking at later patches yet... but in concept bfn address
> > > space is per device instead of per domain.
> >
> > Not in this case. Xen has always maintained a single IOMMU address per
> > virtual machine. That is what BFN refers to.
> >
> >   Paul
> >
> > > In default situation
> > > (w/o pvIOMMU exposed), all devices assigned to dom0 share
> > > the same address space (bfn=pfn) which is currently linked
> > > from domain structure. Then with pvIOMMU exposed, dom0
> > > starts to manage individual pfn address space (called IOVA
> > > address space within dom0) per assigned device. In that case
> > > lookup should accept a bdf number and then find the right
> > > page table...
> > >
> >
> > No. That is over-complicating things and would probably involve re-writing
> > most of the IOMMU code in Xen AFAICT.
> >
> 
> it's not over-complicating. it's about correctness.
> 
> think about the role of IOMMU. Now you expose it to dom0. Then
> dom0 wants to use it to isolate devices. What is the matter having
> an IOMMU which supports only one global address space cross all
> devices, from dom0's p.o.v? If that is the case, why does dom0 ever
> want to change bfn globally on all devices?
> 

Because even a PV dom0 would be beter off with a BFN:GFN map rather than a BFN:MFN map, since then the xen_swiotlb code and be avoided and lots of things that are contiguous in GFN space become contiguous in BFN space and hence become DMA-able without bouncing.

  Paul

> Thanks
> Kevin

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