Re: [PATCH v6 10/14] mm / iommu: split need_iommu() into has_iommu_pt() and need_iommu_pt_sync()

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 12 September 2018 07:45
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Julien Grall <julien.grall@arm.com>;
> Razvan Cojocaru <rcojocaru@bitdefender.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
> <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tamas K Lengyel <tamas@tklengyel.com>; Tim
> (Xen.org) <tim@xen.org>
> Subject: RE: [Xen-devel] [PATCH v6 10/14] mm / iommu: split need_iommu()
> into has_iommu_pt() and need_iommu_pt_sync()
> 
> >>> On 11.09.18 at 17:40, <Paul.Durrant@citrix.com> wrote:
> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> Behalf
> >> Of Jan Beulich
> >> Sent: 11 September 2018 15:31
> >>
> >> >>> On 23.08.18 at 11:47, <paul.durrant@citrix.com> wrote:
> >> > --- a/xen/arch/x86/x86_64/mm.c
> >> > +++ b/xen/arch/x86/x86_64/mm.c
> >> > @@ -1426,7 +1426,8 @@ int memory_add(unsigned long spfn, unsigned
> >> long epfn, unsigned int pxm)
> >> >      if ( ret )
> >> >          goto destroy_m2p;
> >> >
> >> > -    if ( iommu_enabled && !iommu_passthrough &&
> >> !need_iommu(hardware_domain) )
> >> > +    if ( iommu_enabled && !iommu_passthrough &&
> >> > +         !need_iommu_pt_sync(hardware_domain) )
> >> >      {
> >> >          for ( i = spfn; i < epfn; i++ )
> >> >              if ( iommu_map_page(hardware_domain, _bfn(i), _mfn(i),
> >>
> >> I'm confused - the condition you change looks to be inverted. Wouldn't
> >> we better fix this?
> >
> > I don't think it is inverted. I think this is to add new hotplugged memory
> > to the 1:1 map in the case that dom0 is not in strict mode. I could be wrong.
> 
> Oh, I think you're right. It is just rather confusing to see an
> iommu_map_page() call qualified by !need_iommu(). But that's
> as confusing (to me) as the setup logic for Dom0's page tables.
> 

I think it's generally confusing. I'll stick a comment in to explain.

> >> And then I again wonder whether you've chosen the right predicate:
> >> Where would the equivalent mappings come from in the opposite case?
> >
> > If dom0 is in strict mode then I assume that the synchronization is handled
> > when the calls are made to add memory into the p2m (which IIRC happens
> even
> > for PV guests).
> 
> Right you are.
> 
> > My aim for this patch is to avoid any visible functional  change.
> 
> Sure - I didn't mean anything here (if at all) to be done in this patch
> (or perhaps even series), I've merely noticed this as an apparent
> oddity (which if I were right would perhaps better have been fixed
> before your transformations).
> 
> >> > --- a/xen/common/memory.c
> >> > +++ b/xen/common/memory.c
> >> > @@ -805,8 +805,8 @@ int xenmem_add_to_physmap(struct domain
> *d,
> >> struct xen_add_to_physmap *xatp,
> >> >      xatp->size -= start;
> >> >
> >> >  #ifdef CONFIG_HAS_PASSTHROUGH
> >> > -    if ( need_iommu(d) )
> >> > -        this_cpu(iommu_dont_flush_iotlb) = 1;
> >> > +    if ( need_iommu_pt_sync(d) || iommu_use_hap_pt(d) )
> >> > +       this_cpu(iommu_dont_flush_iotlb) = 1;
> >> >  #endif
> >>
> >> Rather than making the conditional more complicated, perhaps
> >> simply drop it (and move the reset-to-false code out of ...
> >>
> >> > @@ -828,7 +828,7 @@ int xenmem_add_to_physmap(struct domain
> *d,
> >> struct xen_add_to_physmap *xatp,
> >> >      }
> >> >
> >> >  #ifdef CONFIG_HAS_PASSTHROUGH
> >> > -    if ( need_iommu(d) )
> >> > +    if ( need_iommu_pt_sync(d) || iommu_use_hap_pt(d) )
> >> >      {
> >> >          int ret;
> >>
> >> ... this if())?
> >>
> >> Also it looks to me as if here you've got confused by the meaning
> >> you've assigned to need_iommu_pt_sync(): According to the
> >> description, it is about sync-ing of page tables. Here, however,
> >> we're checking whether to flush TLBs.
> >
> > Yes, I may be confused here but it would seem to me that flushing the
> IOTLB
> > would be necessary even in the case where the page tables are shared. I'll
> > check the logic again.
> 
> Flushing is necessary always, and my comment didn't go in that
> direction. What I was trying to point out is that the value of
> iommu_dont_flush_iotlb doesn't matter when no flushing
> happens anyway. I.e. setting it to true unconditionally should
> not have any bad effect (but the non-strict-mode-Dom0 case
> may need double checking, albeit even in that case suppressing
> individual page flushing would be desirable, in which case - if
> needed - the second if() might need adjustment, independent
> of the change you're doing here).
> 

Ok. I'll see if this needs correction and put a pre-requisite patch in if need be.

  Paul

> Jan
> 


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