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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan