> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 12 September 2018 07:45
> To: Paul Durrant <[email protected]>
> Cc: Brian Woods <[email protected]>; Suravee Suthikulpanit
> <[email protected]>; Julien Grall <[email protected]>;
> Razvan Cojocaru <[email protected]>; Andrew Cooper
> <[email protected]>; George Dunlap
> <[email protected]>; Ian Jackson <[email protected]>; Wei Liu
> <[email protected]>; Jun Nakajima <[email protected]>; Kevin Tian
> <[email protected]>; Stefano Stabellini <[email protected]>; xen-
> devel <[email protected]>; Konrad Rzeszutek Wilk
> <[email protected]>; Tamas K Lengyel <[email protected]>; Tim
> (Xen.org) <[email protected]>
> 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, <[email protected]> wrote:
> >> From: Xen-devel [mailto:[email protected]] On
> Behalf
> >> Of Jan Beulich
> >> Sent: 11 September 2018 15:31
> >>
> >> >>> On 23.08.18 at 11:47, <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
