Re: [Xen-devel] [PATCH v5 4/7] x86/tlb: introduce a flush guests TLB flag

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.03.2020 17:50, Roger Pau Monné wrote:
> On Mon, Mar 02, 2020 at 11:31:23AM +0100, Jan Beulich wrote:
>> On 28.02.2020 17:50, Roger Pau Monné wrote:
>>> On Fri, Feb 28, 2020 at 05:14:05PM +0100, Jan Beulich wrote:
>>>> On 19.02.2020 18:43, Roger Pau Monne wrote:
>>>>> This was previously unconditionally done in each pre_flush call, but
>>>>> that's not required: HVM guests not using shadow don't require linear
>>>>> TLB flushes as Xen doesn't modify the guest page tables in that case
>>>>> (ie: when using HAP).
>>>>
>>>> This explains the correctness in one direction. What about the
>>>> removal of this from the switch_cr3_cr4() path?
>>>
>>> AFAICT that's never used by shadow code to modify cr3 or cr4, and
>>> hence doesn't require a guest linear TLB flush.
>>
>> XSA-294 tells me to be very conservative here. It is not necessarily
>> the direct use by shadow code that matters; toggle_guest_*() isn't
>> used directly by it, either.
> 
> toggle_guest_{mode/pt} seems to be exclusively used by PV guests. I'm
> fine with adding extra flushes to be on the safe side, but those
> functions are never used against a HVM guest AFAICT. The only reason
> to flush a HVM guest 'internal' TLB is when using shadow, and in that
> case the shadow code must already take care of issuing such flushes.

What I'm asking for primarily is to extend the description. If it
is clear enough, it ought to also be clear enough that no flushes
need inserting anywhere.

>>>> I admit the names of the involved functions
>>>> (hvm_flush_guest_tlbs(), hvm_asid_flush_core()) are somewhat
>>>> misleading, as they don't actually do any flushing, they merely
>>>> arrange for what is in the TLB to no longer be able to be used,
>>>> so giving this a suitable name is "historically" complicated.
>>>> What if we did away with the hvm_flush_guest_tlbs() wrapper,
>>>> naming the constant here then after hvm_asid_flush_core(), e.g.
>>>> FLUSH_ASID_CORE?
>>>
>>> I'm not opposed to renaming. The comment before the definition was
>>> also meant to clarify it's usage, and hence the explicit mention of
>>> ASID/VPID.
>>
>> ... there's also one more argument for renaming: The present
>> name doesn't convey at all that this operation is HVM-only
>> (i.e. PV guests wouldn't have their TLBs [as far as one can
>> call them "their"] flushed).
> 
> Do you think FLUSH_ASID_CORE is clear enough, or would you prefer
> FLUSH_HVM_ASID_CORE?

While in principle in our code base ASID implies HVM, perhaps the
latter would still be even slightly better.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel