Re: [PATCH v8 1/3] x86/tlb: introduce a flush HVM ASIDs flag

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 31, 2020 at 05:40:59PM +0200, Jan Beulich wrote:
> On 20.03.2020 19:42, Roger Pau Monne wrote:
> > Introduce a specific flag to request a HVM guest linear TLB flush,
> > which is an ASID/VPID tickle that forces a guest linear to guest
> > physical TLB flush for all HVM guests.
> > 
> > This was previously unconditionally done in each pre_flush call, but
> > that's not required: HVM guests not using shadow don't require linear
> > TLB flushes as Xen doesn't modify the guest page tables in that case
> > (ie: when using HAP). Note that shadow paging code already takes care
> > of issuing the necessary flushes when the shadow page tables are
> > modified.
> > 
> > In order to keep the previous behavior modify all shadow code TLB
> > flushes to also flush the guest linear to physical TLB. I haven't
> > looked at each specific shadow code TLB flush in order to figure out
> > whether it actually requires a guest TLB flush or not, so there might
> > be room for improvement in that regard.
> > 
> > Also perform ASID/VPIT flushes when modifying the p2m tables as it's a
> > requirement for AMD hardware. Finally keep the flush in
> > switch_cr3_cr4, as it's not clear whether code could rely on
> > switch_cr3_cr4 also performing a guest linear TLB flush. A following
> > patch can remove the ASID/VPIT tickle from switch_cr3_cr4 if found to
> > not be necessary.
> 
> s/VPIT/VPID/ in this paragraph?

Right, sorry.

> > --- a/xen/arch/x86/mm/hap/hap.c
> > +++ b/xen/arch/x86/mm/hap/hap.c
> > @@ -118,7 +118,7 @@ int hap_track_dirty_vram(struct domain *d,
> >              p2m_change_type_range(d, begin_pfn, begin_pfn + nr,
> >                                    p2m_ram_rw, p2m_ram_logdirty);
> >  
> > -            flush_tlb_mask(d->dirty_cpumask);
> > +            flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> >  
> >              memset(dirty_bitmap, 0xff, size); /* consider all pages dirty 
> > */
> >          }
> > @@ -205,7 +205,7 @@ static int hap_enable_log_dirty(struct domain *d, 
> > bool_t log_global)
> >           * to be read-only, or via hardware-assisted log-dirty.
> >           */
> >          p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
> > -        flush_tlb_mask(d->dirty_cpumask);
> > +        flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> >      }
> >      return 0;
> >  }
> > @@ -234,7 +234,7 @@ static void hap_clean_dirty_bitmap(struct domain *d)
> >       * be read-only, or via hardware-assisted log-dirty.
> >       */
> >      p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty);
> > -    flush_tlb_mask(d->dirty_cpumask);
> > +    flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> >  }
> >  
> >  /************************************************/
> > @@ -798,7 +798,7 @@ hap_write_p2m_entry(struct p2m_domain *p2m, unsigned 
> > long gfn, l1_pgentry_t *p,
> >  
> >      safe_write_pte(p, new);
> >      if ( old_flags & _PAGE_PRESENT )
> > -        flush_tlb_mask(d->dirty_cpumask);
> > +        flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> 
> For all four - why FLUSH_TLB? Doesn't the flushing here solely care
> about guest translations?

Not on AMD, at least to my understanding, the AMD SDM states:

"If a hypervisor modifies a nested page table by decreasing permission
levels, clearing present bits, or changing address translations and
intends to return to the same ASID, it should use either TLB command
011b or 001b."

It's in section 15.16.1.

This to my understanding implies that on AMD hardware modifications to
the NPT require an ASID flush. I assume that on AMD ASIDs also cache
combined translations, guest linear -> host physical.

In fact without doing such flushes when modifying the nested page
tables XenRT was seeing multiple issues on AMD hardware.

> > --- a/xen/arch/x86/mm/hap/nested_hap.c
> > +++ b/xen/arch/x86/mm/hap/nested_hap.c
> > @@ -84,7 +84,7 @@ nestedp2m_write_p2m_entry(struct p2m_domain *p2m, 
> > unsigned long gfn,
> >      safe_write_pte(p, new);
> >  
> >      if (old_flags & _PAGE_PRESENT)
> > -        flush_tlb_mask(p2m->dirty_cpumask);
> > +        flush_mask(p2m->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> 
> Same here then I guess.
> 
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -896,7 +896,8 @@ static void p2m_pt_change_entry_type_global(struct 
> > p2m_domain *p2m,
> >      unmap_domain_page(tab);
> >  
> >      if ( changed )
> > -         flush_tlb_mask(p2m->domain->dirty_cpumask);
> > +         flush_mask(p2m->domain->dirty_cpumask,
> > +                    FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> 
> Given that this code is used in shadow mode as well, perhaps
> better to keep it here. Albeit maybe FLUSH_TLB could be dependent
> upon !hap_enabled()?
> 
> > --- a/xen/arch/x86/mm/paging.c
> > +++ b/xen/arch/x86/mm/paging.c
> > @@ -613,7 +613,7 @@ void paging_log_dirty_range(struct domain *d,
> >  
> >      p2m_unlock(p2m);
> >  
> > -    flush_tlb_mask(d->dirty_cpumask);
> > +    flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> 
> Same here?

I'm fine with doing further refinements, but I would like to be on the
conservative side and keep such flushes.

> > @@ -993,7 +993,7 @@ static void shadow_blow_tables(struct domain *d)
> >                                 pagetable_get_mfn(v->arch.shadow_table[i]), 
> > 0);
> >  
> >      /* Make sure everyone sees the unshadowings */
> > -    flush_tlb_mask(d->dirty_cpumask);
> > +    flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE);
> 
> Taking this as example, wouldn't it be more consistent overall if
> paths not being HVM-only would specify FLUSH_HVM_ASID_CORE only
> for HVM domains?

I think there's indeed room for improvement here, as it's likely
possible to drop some of the ASID/VPID flushes. Given that previous to
this patch we would flush ASIDs on every TLB flush I think the current
approach is safe, and as said above further improvements can be done
afterwards.

> Also, seeing the large number of conversions, perhaps have another
> wrapper, e.g. flush_tlb_mask_hvm(), at least for the cases where
> both flags get specified unconditionally?

That's fine for me, if you agree with the proposed naming
(flush_tlb_mask_hvm) I'm happy to introduce the helper.

Thanks, Roger.