[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.