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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2020 17:54, Roger Pau Monné wrote:
> 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."

Hmm, indeed. How did I not spot this already when reading this the
other day?

> 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.

Okay. And I'm sorry for having attempted to mislead you.

Jan