Re: [Xen-devel] [PATCH v14 7/9] vtd: add lookup_page method to iommu_ops

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 24 December 2018 15:15
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v14 7/9] vtd: add lookup_page method to
> iommu_ops
> 
> On 04/10/2018 11:45, Paul Durrant wrote:
> > This patch adds a new method to the VT-d IOMMU implementation to find
> the
> > MFN currently mapped by the specified DFN along with a wrapper function
> > in generic IOMMU code to call the implementation if it exists.
> >
> > NOTE: This patch only adds a Xen-internal interface. This will be used
> by
> >       a subsequent patch.
> >       Another subsequent patch will add similar functionality for AMD
> >       IOMMUs.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > ---
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
> >
> > v11:
> >  - Fold in patch to change failure semantics (already ack-ed by Kevin).
> >
> > v10:
> >  - Adjust the locking comment.
> >
> > v9:
> >  - Add comment about locking in xen/iommu.h.
> >
> > v8:
> >  - Remove clean-up as this is now done by a prior patch.
> >  - Make intel_iommu_lookup_page() return dfn value if using shared EPT
> >    or iommu_passthrough is set, as requested by Kevin.
> >
> > v7:
> >  - Re-base and re-name BFN -> DFN.
> >  - Add missing checks for shared EPT and iommu_passthrough.
> >  - Remove unnecessary initializers and use array-style dereference.
> >  - Drop Wei's R-b because of code churn.
> >
> > v3:
> >  - Addressed comments from George.
> >
> > v2:
> >  - Addressed some comments from Jan.
> > ---
> >  xen/drivers/passthrough/iommu.c     | 11 ++++++++++
> >  xen/drivers/passthrough/vtd/iommu.c | 41
> +++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.h |  3 +++
> >  xen/include/xen/iommu.h             | 10 +++++++++
> >  4 files changed, 65 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> > index 9187d50730..f94b522c73 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1833,6 +1833,46 @@ static int __must_check
> intel_iommu_unmap_page(struct domain *d,
> >      return dma_pte_clear_one(d, dfn_to_daddr(dfn));
> >  }
> >
> > +static int intel_iommu_lookup_page(struct domain *d, dfn_t dfn, mfn_t
> *mfn,
> > +                                   unsigned int *flags)
> > +{
> > +    struct domain_iommu *hd = dom_iommu(d);
> > +    struct dma_pte *page, val;
> > +    u64 pg_maddr;
> > +
> > +    /*
> > +     * If VT-d shares EPT page table or if the domain is the hardware
> > +     * domain and iommu_passthrough is set then pass back the dfn.
> > +     */
> > +    if ( iommu_use_hap_pt(d) ||
> > +         (iommu_hwdom_passthrough && is_hardware_domain(d)) )
> > +        return -EOPNOTSUPP;
> 
> The patch as commented no longer behaves in the way described in the
> comment, or the v8 requested change.

The landscape is shifting too. I think this is going to need to pass back a 
base dfn and order once the the map and unmap ops gain an order parameter.

> 
> What is the verdict WRT returning dfn ?
> 

Passing back the dfn as the mfn is wrong for shared EPT as dfn == gfn and the 
code at this level does not have that information. I think it is much better to 
fail in some way... EOPNOTSUPP may not be the best errno but I can't think of a 
better one.

  Paul

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