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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- 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?

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.

Jan


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