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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 15 March 2018 16:54
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 4/7] vtd: add lookup_page method to iommu_ops
> 
> >>> On 12.02.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. This functionality will be used
> > by a subsequent patch.
> 
> How come this is VT-d only? The same is going to be needed at least
> for the AMD IOMMU. And if you don't do it for ARM, then the hook
> should be x86-specific for the time being.

I only have VT-d h/w to test on so it seemed prudent to keep it limited to 
that. I did look at doing a speculative implementation for AMD but it was not 
sufficiently obvious to give me confidence.
I don't see any particular reason to keep the hook arch specific though... it 
would just create code churn later, assuming someone wants to do PV-IOMMU for 
ARM.

> 
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1827,6 +1827,44 @@ static int __must_check
> intel_iommu_unmap_page(struct domain *d,
> >      return dma_pte_clear_one(d, (paddr_t)bfn_x(bfn) << PAGE_SHIFT_4K);
> >  }
> >
> > +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.
> 
> > +    u64 pg_maddr;
> > +
> > +    spin_lock(&hd->arch.mapping_lock);
> 
> Depending on how frequently this is going to be used, this lock
> may need to become an r/w one.
> 
> > +    pg_maddr =
> > +        addr_to_dma_page_maddr(d, (paddr_t)bfn_x(bfn) <<
> PAGE_SHIFT_4K, 1);
> 
> Why do you request table allocation here? Lookups shouldn't
> normally alter the tables. Also this wants better line wrapping.
> 
> > +    if ( pg_maddr == 0 )
> > +    {
> > +        spin_unlock(&hd->arch.mapping_lock);
> > +        return -ENOMEM;
> > +    }
> > +    page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
> 
> Pointless cast.
> 
> > +    pte = page + (bfn_x(bfn) & LEVEL_MASK);
> > +    val = *pte;
> > +    if (!dma_pte_present(val)) {
> 
> Style (also more below).
> 
> > --- a/xen/drivers/passthrough/vtd/iommu.h
> > +++ b/xen/drivers/passthrough/vtd/iommu.h
> > @@ -272,9 +272,11 @@ struct dma_pte {
> >  #define dma_set_pte_prot(p, prot) do { \
> >          (p).val = ((p).val & ~DMA_PTE_PROT) | ((prot) & DMA_PTE_PROT); \
> >      } while (0)
> > +#define dma_get_pte_prot(p) ((p).val & DMA_PTE_PROT)
> >  #define dma_pte_addr(p) ((p).val & PADDR_MASK & PAGE_MASK_4K)
> >  #define dma_set_pte_addr(p, addr) do {\
> >              (p).val |= ((addr) & PAGE_MASK_4K); } while (0)
> > +#define dma_get_pte_addr(p) ((p).val & PAGE_MASK_4K)
> 
> Why is dma_pte_addr() not good enough?

I guess it probably is... not sure why Malcolm felt the need to add this... 
possibly concern over the AND with PADDR_MASK... but that looks like the right 
thing to do. I'll drop it in v2.

> 
> Overall this looks very much like Malcolm's original implementation;
> I'm not sure dropping his authorship / S-o-b is a valid thing to do.
> 

Yes, there's probably a little too much cut'n'paste from Malcolm's original. 
After some discussions with Andy Cooper I think I'm going to re-work things a 
bit in v2 anyway so Malcolm's s-o-b is likely to become moot at that point.

> Jan


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