Re: [Xen-devel] [PATCH v6 08/14] vtd: add lookup_page method to iommu_ops

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 07 September 2018 12:11
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v6 08/14] vtd: add lookup_page method to iommu_ops
> 
> >>> On 23.08.18 at 11:47, <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.
> 
> For this to go in, I think the AMD side of it wants to also be implemented.
> 
> > --- 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);
> > +}
> 
> Shouldn't this be restricted to PV guests? HVM ones aren't supposed
> to know about MFNs.

Agreed, but I think this is the wrong level to be applying such a check: 
iommu_map_page() is supplied an MFN regardless of whether the domain is PV or 
HVM, so I think it is reasonable for a lookup function to work in terms of MFNs.

> 
> > +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;
> 
> Pointless initializers. I also question the usefulness of "pte":
> 

Yes, I'll tidy up the initializers.

> > +    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;
> 
>     val = page[bfn_x(bfn) & LEVEL_MASK];
> 

Yes, that's neater.

  Paul

> Jan
> 


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