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

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

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.

> 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));
}

I think this should work, but I would rather do it in a separate
patch. I'm also not fully convinced guest_flush_tlb_mask is the best
name, but I couldn't think of anything else more descriptive of the
purpose of the function.

Thanks, Roger.