> -----Original Message----- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 25 September 2018 11:37 > 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>; > Andrew Cooper <andrew.coop...@citrix.com>; Wei Liu <wei.l...@citrix.com>; > George Dunlap <george.dun...@citrix.com>; Ian Jackson > <ian.jack...@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: [PATCH v11 9/9] mm / iommu: split need_iommu() into > has_iommu_pt() and need_iommu_pt_sync() > > >>> On 21.09.18 at 12:56, <paul.durr...@citrix.com> wrote: > > --- a/xen/arch/x86/hvm/mtrr.c > > +++ b/xen/arch/x86/hvm/mtrr.c > > @@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, > hvm_load_mtrr_msr, 1, > > > > void memory_type_changed(struct domain *d) > > { > > - if ( need_iommu(d) && d->vcpu && d->vcpu[0] ) > > + if ( (has_iommu_pt(d) || iommu_use_hap_pt(d)) && > > + d->vcpu && d->vcpu[0] ) > > { > > p2m_memory_type_changed(d); > > flush_all(FLUSH_CACHE); > > @@ -831,7 +832,7 @@ int epte_get_entry_emt(struct domain *d, unsigned > long gfn, mfn_t mfn, > > return MTRR_TYPE_UNCACHABLE; > > } > > > > - if ( !need_iommu(d) && !cache_flush_permitted(d) ) > > + if ( !has_iommu_pt(d) && !cache_flush_permitted(d) ) > > { > > *ipat = 1; > > return MTRR_TYPE_WRBACK; > > Considering how closely the two functions are related I'm struggling to > understand why the conditions are no longer the inverse of one another. > With iommu_use_hap_pt() including a has_iommu_pt() check I think the > former can and should be simplified.
Ok. > > > --- a/xen/arch/x86/x86_64/mm.c > > +++ b/xen/arch/x86/x86_64/mm.c > > @@ -1426,8 +1426,13 @@ int memory_add(unsigned long spfn, unsigned long > epfn, unsigned int pxm) > > if ( ret ) > > goto destroy_m2p; > > > > - if ( iommu_enabled && !iommu_hwdom_passthrough && > > - !need_iommu(hardware_domain) ) > > + /* > > + * If hardware domain has IOMMU mappings but page tables are not > > + * shared, and are not being kept in sync (which is the case when > > + * in strict mode) then newly added memory needs to be mapped here. > > + */ > > + if ( has_iommu_pt(hardware_domain) && > > + !iommu_use_hap_pt(hardware_domain) && !iommu_hwdom_strict ) > > iommu_use_hap_pt() includes a hap_enabled() check, but that is valid > to be used on HVM domains only. It looks like there are other similar > improper uses elsewhere - all new and pre-existing uses need auditing. > Yes... I will check. > > --- a/xen/drivers/passthrough/pci.c > > +++ b/xen/drivers/passthrough/pci.c > > @@ -1416,7 +1416,7 @@ static int assign_device(struct domain *d, u16 > seg, u8 bus, u8 devfn, u32 flag) > > > > /* Prevent device assign if mem paging or mem sharing have been > > * enabled for this domain */ > > - if ( unlikely(!need_iommu(d) && > > + if ( unlikely(!has_iommu_pt(d) && > > (d->arch.hvm.mem_sharing_enabled || > > vm_event_check_ring(d->vm_event_paging) || > > p2m_get_hostp2m(d)->global_logdirty)) ) > > This need_iommu() check looks rather unmotivated to me - wouldn't > you better delete it instead of finding a suitable replacement? Yes, it's not clear why need_iommu() is here. I can only guess it's to avoid re-checking the second clause once the domain has pagetables, but nothing in the second clause seems to be computationally expensive. Paul > > Jan > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
