Re: [PATCH v5 7/8] iommu: remove the share_p2m operation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2020 09:40, 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, unsigned int nr_pt_levels)
> +{
> +    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));

I thought I had already asked about this odd going through a pagetable_t
a 2nd time without a clear need. Why not simply

    if ( iommu_use_hap_pt(d) )
    {
        pagetable_t pgt = p2m_get_pagetable(p2m_get_hostp2m(d));

        return pagetable_get_paddr(pgt);
    }

?

> +    }
> +
> +    if ( !hd->arch.vtd.pgd_maddr )
> +    {
> +        addr_to_dma_page_maddr(d, 0, 1);

Ahead of this, would you mind retaining ...

> @@ -1340,37 +1382,18 @@ int domain_context_mapping_one(
>      if ( iommu_hwdom_passthrough && is_hardware_domain(domain) )
>      {
>          context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
> -        agaw = level_to_agaw(iommu->nr_pt_levels);
>      }
>      else
>      {
>          spin_lock(&hd->arch.mapping_lock);
>  
> -        /* Ensure we have pagetables allocated down to leaf PTE. */
> -        if ( hd->arch.vtd.pgd_maddr == 0 )
> +        pgd_maddr = domain_pgd_maddr(domain, iommu->nr_pt_levels);
> +        if ( !pgd_maddr )
>          {
> -            addr_to_dma_page_maddr(domain, 0, 1);

... the comment you remove here?

With these adjustments
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan