> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 12 September 2018 07:45
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Brian Woods <brian.wo...@amd.com>; Suravee Suthikulpanit
> <suravee.suthikulpa...@amd.com>; Julien Grall <julien.gr...@arm.com>;
> Razvan Cojocaru <rcojoc...@bitdefender.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Wei Liu
> <wei.l...@citrix.com>; Jun Nakajima <jun.nakaj...@intel.com>; Kevin Tian
> <kevin.t...@intel.com>; Stefano Stabellini <sstabell...@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.w...@oracle.com>; Tamas K Lengyel <ta...@tklengyel.com>; Tim
> (Xen.org) <t...@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.durr...@citrix.com> wrote:
> >> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On
> Behalf
> >> Of Jan Beulich
> >> Sent: 11 September 2018 15:31
> >>
> >> >>> On 23.08.18 at 11:47, <paul.durr...@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
