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

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

On Wed, Apr 15, 2020 at 05:42:20PM +0200, Jan Beulich wrote:
> On 15.04.2020 16:49, Roger Pau Monné wrote:
> > On Tue, Apr 14, 2020 at 05:06:23PM +0200, Jan Beulich wrote:
> >> On 14.04.2020 16:53, Roger Pau Monné wrote:
> >>> On Tue, Apr 14, 2020 at 03:50:15PM +0200, Jan Beulich wrote:
> >>>> On 14.04.2020 13:19, Roger Pau Monné wrote:
> >>>>> On Tue, Apr 14, 2020 at 12:13:04PM +0200, Jan Beulich wrote:
> >>>>>> On 14.04.2020 12:02, Roger Pau Monné wrote:
> >>>>>>> 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.
> >>>>
> >>>> I'm unconvinced: The entire section "TLB Management" in the PM gives
> >>>> the impression that "ordinary" TLB flushing covers all ASIDs anyway.
> >>>> It's not stated anywhere (I could find) explicitly though.
> >>>
> >>> Hm, I don't think so. XenRT found a myriad of issues with NPT when p2m
> >>> code wasn't modified to do ASID flushes instead of plain TLB flushes.
> >>
> >> Well, that's clear from e.g. p2m_pt_set_entry() not doing any
> >> flushing itself.
> >>
> >>> Even if it's just to stay on the safe side I would perform ASID
> >>> flushes for HVM guests with shadow running on AMD.
> >>
> >> Tim, any chance you could voice your thoughts here? To me it seems
> >> odd to do an all-ASIDs flush followed by an ASID one.
> > 
> > I've been reading a bit more into this, and section 15.16.1 states:
> > 
> > "TLB flush operations must not be assumed to affect all ASIDs."
> 
> That's the section talking about the tlb_control VMCB field. It is
> in this context that the sentence needs to be interpreted, imo.

It explicitly mentions move-to-cr3 and move-to-cr4 before that phrase:

"TLB flush operations function identically whether or not SVM is
enabled (e.g., MOV-TO-CR3 flushes non-global mappings, whereas
MOV-TO-CR4 flushes global and non-global mappings). TLB flush
operations must not be assumed to affect all ASIDs."

So my reading is that normal flush operations not involving
tlb_control VMCB field should not assume to flush all ASIDs. Again
this is of course my interpretation of the text in the PM. I will go
with my suggested approach, which is safer and should cause no
functional issues AFAICT. The only downside is the maybe redundant
flush, but that's safe.

Thanks, Roger.