Re: [Xen-devel] [PATCH v2 09/13] vtd: add lookup_page method to iommu_ops

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: dunlapg@xxxxxxxxx [mailto:dunlapg@xxxxxxxxx] On Behalf Of
> George Dunlap
> Sent: 11 July 2018 11:52
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v2 09/13] vtd: add lookup_page method to
> iommu_ops
> 
> On Sat, Jul 7, 2018 at 12:05 PM, Paul Durrant <paul.durrant@xxxxxxxxxx>
> wrote:
> > 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@xxxxxxxxxx>
> > ---
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > v2:
> >  - Addressed some comments from Jan.
> > ---
> >  xen/drivers/passthrough/iommu.c     | 11 ++++++++++
> >  xen/drivers/passthrough/vtd/iommu.c | 40
> +++++++++++++++++++++++++++++++++++++
> >  xen/drivers/passthrough/vtd/iommu.h |  1 +
> >  xen/include/xen/iommu.h             |  4 ++++
> >  4 files changed, 56 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> > index 3fbd3ebaf6..f25aa3f1d6 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -306,6 +306,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 7cd3813b9f..438bef670d 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1831,6 +1831,45 @@ 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)
> > +{
> > +    struct domain_iommu *hd = dom_iommu(d);
> > +    struct dma_pte *page = NULL, *pte = NULL, val;
> > +    u64 pg_maddr;
> > +
> > +    spin_lock(&hd->arch.mapping_lock);
> > +
> > +    pg_maddr = addr_to_dma_page_maddr(d, bfn_to_baddr(bfn), 0);
> > +    if ( pg_maddr == 0 )
> > +    {
> > +        spin_unlock(&hd->arch.mapping_lock);
> > +        return -ENOMEM;
> > +    }
> > +
> > +    page = map_vtd_domain_page(pg_maddr);
> > +    pte = page + (bfn_x(bfn) & LEVEL_MASK);
> > +    val = *pte;
> > +    if ( !dma_pte_present(val) )
> > +    {
> > +        unmap_vtd_domain_page(page);
> > +        spin_unlock(&hd->arch.mapping_lock);
> > +        return -ENOMEM;
> 
> Should this be -EEXIST?  Or maybe return MFN_INVALID?
> 

Do you mean ENOENT? EEXIST implies it exists but it shouldn't, right?

> Also, could you do the unmap / unlock first and then do the check,
> rather than duplicating things?
> 

Sure.

> > +    }
> > +
> > +    unmap_vtd_domain_page(page);
> > +    spin_unlock(&hd->arch.mapping_lock);
> > +
> > +    *mfn = maddr_to_mfn(dma_pte_addr(val));
> > +    *flags = 0;
> > +    if ( dma_pte_prot(val) & DMA_PTE_READ )
> > +        *flags |= IOMMUF_readable;
> > +    if ( dma_pte_prot(val) & DMA_PTE_WRITE )
> > +        *flags |= IOMMUF_writable;
> 
> This is a bit strange, since all dma_pte_prot() does is return val &
> DMA_PTE_READ|DMA_PTE_WRITE.  Would it make sense to implement
> dma_pte_read() / dma_pte_write() instead (like dma_pte_present())?
>

Yes, I can do that.
 
> Everything else looks good to me.
> 

Thanks,

  Paul

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