Re: [PATCH v11 9/9] 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: 25 September 2018 11:37
> 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>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@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: [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.durrant@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