Re: [PATCH v4 10/14] iommu: remove the share_p2m operation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2020 15:42, Paul Durrant wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -318,6 +318,48 @@ static u64 addr_to_dma_page_maddr(struct domain *domain, 
> u64 addr, int alloc)
>      return pte_maddr;
>  }
>  
> +static uint64_t domain_pgd_maddr(struct domain *d, struct vtd_iommu *iommu)

The 2nd param can be const, and I wonder whether it wouldn't better be
named e.g. "vtd". Then again all you're after is iommu->nr_pt_levels,
so maybe the caller would better pass in that value (removing the
appearance of there being some further dependency about the specific
IOMMU's properties)?

> +{
> +    struct domain_iommu *hd = dom_iommu(d);
> +    uint64_t pgd_maddr;
> +    unsigned int agaw;
> +
> +    ASSERT(spin_is_locked(&hd->arch.mapping_lock));
> +
> +    if ( iommu_use_hap_pt(d) )
> +    {
> +        mfn_t pgd_mfn =
> +            pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
> +
> +        return pagetable_get_paddr(pagetable_from_mfn(pgd_mfn));

Why the pagetable -> MFN -> pagetable -> paddr transformation? I.e. just

        return pagetable_get_paddr(p2m_get_pagetable(p2m_get_hostp2m(d)));

? Oh, I've now realized that's how the old code was written.

> +    }
> +
> +    if ( !hd->arch.vtd.pgd_maddr )
> +    {
> +        addr_to_dma_page_maddr(d, 0, 1);
> +
> +        if ( !hd->arch.vtd.pgd_maddr )
> +            return 0;
> +    }
> +
> +    pgd_maddr = hd->arch.vtd.pgd_maddr;
> +
> +    /* Skip top levels of page tables for 2- and 3-level DRHDs. */
> +    for ( agaw = level_to_agaw(4);
> +          agaw != level_to_agaw(iommu->nr_pt_levels);
> +          agaw-- )
> +    {
> +        struct dma_pte *p = map_vtd_domain_page(pgd_maddr);

const?

> +
> +        pgd_maddr = dma_pte_addr(*p);
> +        unmap_vtd_domain_page(p);
> +        if ( !pgd_maddr )
> +            return 0;
> +    }
> +
> +    return pgd_maddr;
> +}

Jan