[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 10/14] mm / iommu: split need_iommu() into has_iommu_pt() and need_iommu_pt_sync()
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: 12 September 2018 07:45 > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx> > Cc: Brian Woods <brian.woods@xxxxxxx>; Suravee Suthikulpanit > <suravee.suthikulpanit@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; > Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>; Andrew Cooper > <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap > <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu > <wei.liu2@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian > <kevin.tian@xxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen- > devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk > <konrad.wilk@xxxxxxxxxx>; Tamas K Lengyel <tamas@xxxxxxxxxxxxx>; Tim > (Xen.org) <tim@xxxxxxx> > 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@xxxxxxxxxx> wrote: > >> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On > Behalf > >> Of Jan Beulich > >> Sent: 11 September 2018 15:31 > >> > >> >>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |