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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 10 September 2020 15:20
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; 
> Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Wei 
> Liu <wl@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>
> Subject: Re: [PATCH v5 7/8] iommu: remove the share_p2m operation
> 
> 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);
>     }
> 
> ?

It's code movement (which IIRC you had acknowledged) but I can change it if you 
wish.

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

Sure.

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

Thanks.

  Paul

> Jan