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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Apr 14, 2020 at 12:13:04PM +0200, Jan Beulich wrote:
> On 14.04.2020 12:02, Roger Pau Monné wrote:
> > On Tue, Apr 14, 2020 at 11:01:06AM +0200, Jan Beulich wrote:
> >> On 14.04.2020 09:52, Roger Pau Monné wrote:
> >>> On Thu, Apr 09, 2020 at 01:54:40PM +0200, Jan Beulich wrote:
> >>>> On 06.04.2020 12:57, Roger Pau Monne wrote:
> >>>>> --- 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);
> >>>>> +            hap_flush_tlb_mask(d->dirty_cpumask);
> >>>>>  
> >>>>>              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);
> >>>>> +        hap_flush_tlb_mask(d->dirty_cpumask);
> >>>>>      }
> >>>>>      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);
> >>>>> +    hap_flush_tlb_mask(d->dirty_cpumask);
> >>>>>  }
> >>>>>  
> >>>>>  /************************************************/
> >>>>> @@ -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);
> >>>>> +        hap_flush_tlb_mask(d->dirty_cpumask);
> >>>>>  
> >>>>>      paging_unlock(d);
> >>>>>  
> >>>>
> >>>> Following up on my earlier mail about paging_log_dirty_range(), I'm
> >>>> now of the opinion that all of these flushes should go away too. I
> >>>> can only assume that they got put where they are when HAP code was
> >>>> cloned from the shadow one. These are only p2m operations, and hence
> >>>> p2m level TLB flushing is all that's needed here.
> >>>
> >>> IIRC without those ASID flushes NPT won't work correctly, as I think
> >>> it relies on those flushes when updating the p2m.
> >>
> >> Hmm, yes - at least for this last one (in patch context) I definitely
> >> agree: NPT's TLB invalidation is quite different from EPT's (and I
> >> was too focused on the latter when writing the earlier reply).
> >> Therefore how about keeping hap_flush_tlb_mask() (and its uses), but
> >> teaching it to do nothing for EPT, while doing an ASID based flush
> >> for NPT?
> > 
> > I could give that a try. I'm slightly worried that EPT code might rely
> > on some of those ASID/VPID flushes. It seems like it's trying to do
> > VPID flushes when needed, but issues could be masked by the ASID/VPID
> > flushes done by the callers.
> 
> I can't seem to find any EPT code doing VPID flushes, and I'd also
> not expect to. There's VMX code doing so, yes. EPT should be fully
> agnostic to guest virtual address space.

I got confused.

> >> There may then even be the option to have a wider purpose helper,
> >> dealing with most (all?) of the flushes needed from underneath
> >> x86/mm/, setting the TLB and HVM_ASID_CORE flags as appropriate. In
> >> the EPT case the function would still be a no-op (afaict).
> > 
> > That seems nice, we would have to be careful however as reducing the
> > number of ASID/VPID flushes could uncover issues in the existing code.
> > I guess you mean something like:
> > 
> > static inline void guest_flush_tlb_mask(const struct domain *d,
> >                                         const cpumask_t *mask)
> > {
> >     flush_mask(mask, (is_pv_domain(d) || shadow_mode_enabled(d) ? FLUSH_TLB
> >                                                                 : 0) |
> >     		     (is_hvm_domain(d) && cpu_has_svm ? FLUSH_HVM_ASID_CORE
> > 		                                      : 0));
> > }
> 
> Almost - is_hvm_domain(d) && cpu_has_svm seems to wide for me. I'd
> rather use hap_enabled() && cpu_has_svm, which effectively means NPT.
> Or am I overlooking a need to do ASID flushes also in shadow mode?

I think so, I've used is_hvm_domain in order to cover for HVM domains
running in shadow mode on AMD hardware, I think those also need the
ASID flushes.

> Also I'd suggest to calculate the flags up front, to avoid calling
> flush_mask() in the first place in case (EPT) neither bit is set.
> 
> > I think this should work, but I would rather do it in a separate
> > patch.
> 
> Yes, just like the originally (wrongly, as you validly say) suggested
> full removal of them, putting this in a separate patch would indeed
> seem better.

Would you like me to resend with the requested fix to
paging_log_dirty_range (ie: drop the FLUSH_TLB and only call
flush_mask for HAP guests running on AMD) then?

Thanks, Roger.