Re: [PATCH v6 10/14] mm / iommu: split need_iommu() into has_iommu_pt() and need_iommu_pt_sync()

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Jan Beulich
> Sent: 11 September 2018 15:31
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; Jun Nakajima
> <jun.nakajima@intel.com>; Razvan Cojocaru <rcojocaru@bitdefender.com>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; George Dunlap
> <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Tamas K
> Lengyel <tamas@tklengyel.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; xen-devel <xen-
> devel@lists.xenproject.org>; Brian Woods <brian.woods@amd.com>
> Subject: Re: [Xen-devel] [PATCH v6 10/14] mm / iommu: split need_iommu()
> into has_iommu_pt() and need_iommu_pt_sync()
> 
> >>> On 23.08.18 at 11:47, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/mtrr.c
> > +++ b/xen/arch/x86/hvm/mtrr.c
> > @@ -793,7 +793,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR,
> hvm_save_mtrr_msr, hvm_load_mtrr_msr,
> >
> >  void memory_type_changed(struct domain *d)
> >  {
> > -    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
> > +    if ( (need_iommu_pt_sync(d) || iommu_use_hap_pt(d)) &&
> > +         d->vcpu && d->vcpu[0] )
> >      {
> >          p2m_memory_type_changed(d);
> >          flush_all(FLUSH_CACHE);
> 
> How does need_iommu_pt_sync() come into play here? To me,
> has_iommu_pt() would seem to be a better match.

Ok.

> The question
> here solely is (iirc) whether memory types take any effect in the
> first place (or else we can skip adjusting them), which in turn
> solely depends on whether there are any pass-through devices
> in the domain. This is along the lines of ...
> 
> > @@ -841,7 +842,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;
> 
> ... this.
> 
> > --- 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. George?

> 
> 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). My aim for this patch is to avoid any visible functional change.

> > --- 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.

> > @@ -179,8 +179,10 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
> >          return;
> >
> >      register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu
> p2m table", 0);
> > -    d->need_iommu = !!iommu_dom0_strict;
> > -    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> > +
> > +    hd->status = IOMMU_STATUS_initializing;
> > +    hd->need_sync = !!iommu_dom0_strict && !iommu_use_hap_pt(d);
> 
> Pointless !! afaict.
> 

Ok, I'll drop it.

> >  int iommu_construct(struct domain *d)
> >  {
> > -    if ( need_iommu(d) > 0 )
> > +    struct domain_iommu *hd = dom_iommu(d);
> > +
> > +    if ( hd->status == IOMMU_STATUS_initialized )
> >          return 0;
> >
> > -    if ( !iommu_use_hap_pt(d) )
> > +    if ( !iommu_use_hap_pt(d) && hd->status <
> IOMMU_STATUS_initialized )
> 
> Isn't the addition here redundant with the earlier if()?
> 

Yes, it does appear to be.

> > @@ -889,9 +886,11 @@ void watchdog_domain_destroy(struct domain
> *d);
> >  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> >                             cpumask_weight((v)->cpu_hard_affinity) == 1)
> >  #ifdef CONFIG_HAS_PASSTHROUGH
> > -#define need_iommu(d)    ((d)->need_iommu)
> > +#define has_iommu_pt(d) (dom_iommu(d)->status !=
> IOMMU_STATUS_disabled)
> > +#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
> >  #else
> > -#define need_iommu(d)    (0)
> > +#define has_iommu_pt(d) (0)
> > +#define need_iommu_pt_sync(d) (0)
> 
> Please use false here (and no need for the parentheses).
> 

Ok.

  Paul

> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel