Re: [Xen-devel] [PATCH v7 1/3] x86/tlb: introduce a flush HVM ASIDs flag

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Tim Deegan <tim@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v7 1/3] x86/tlb: introduce a flush HVM ASIDs flag
Date: Fri, 20 Mar 2020 14:16:47 +0100	[thread overview]
Message-ID: <9f6c3b5b-13e7-05aa-1ff6-930724674323@suse.com> (raw)
In-Reply-To: <20200320090155.GS24458@Air-de-Roger.citrite.net>

On 20.03.2020 10:01, Roger Pau Monné wrote:
> On Fri, Mar 20, 2020 at 08:21:19AM +0100, Jan Beulich wrote:
>> On 19.03.2020 20:07, Julien Grall wrote:
>>> On 19/03/2020 18:43, Roger Pau Monné wrote:
>>>> On Thu, Mar 19, 2020 at 06:07:44PM +0000, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 19/03/2020 17:38, Roger Pau Monné wrote:
>>>>>> On Thu, Mar 19, 2020 at 04:21:23PM +0000, Julien Grall wrote:
>>>>>>   >> Why can't you keep flush_tlb_mask() here?
>>>>>>
>>>>>> Because filtered_flush_tlb_mask is used in populate_physmap, and
>>>>>> changes to the phymap require an ASID flush on AMD hardware.
>>>>>
>>>>> I am afraid this does not yet explain me why flush_tlb_mask() could not be
>>>>> updated so it flush the ASID on AMD hardware.
>>>>
>>>> Current behavior previous to this patch is to flush the ASIDs on
>>>> every TLB flush.
>>>>
>>>> flush_tlb_mask is too widely used on x86 in places where there's no
>>>> need to flush the ASIDs. This prevents using assisted flushes (by L0)
>>>> when running nested, since those assisted flushes performed by L0
>>>> don't flush the L2 guests TLBs.
>>>>
>>>> I could keep current behavior and leave flush_tlb_mask also flushing the
>>>> ASIDs, but that seems wrong as the function doesn't have anything in
>>>> it's name that suggests it also flushes the in-guest TLBs for HVM.
>>>
>>> I agree the name is confusing, I had to look at the implementation to understand what it does.
>>>
>>> How about renaming (or introducing) the function to flush_tlb_all_guests_mask() or flush_tlb_all_guests_cpumask()) ?
>>
>> And this would then flush _only_ guest TLBs?
> 
> No, I think from Julien's proposal (if I understood it correctly)
> flush_tlb_all_guests_mask would do what flush_tlb_mask currently does
> previous to this patch (flush Xen's TLBs + HVM ASIDs).
> 
>>>> I would rather prefer the default to be to not flush the
>>>> ASIDs, so that users need to specify so by passing the flag to
>>>> flusk_mask.
>>> That's x86 choice. For common, I would rather no introduce those flags until we have another arch that make use of it.
>>
>> The flags should perhaps indeed remain x86-specific, but suitable
>> wrappers usable from common code should exist (as you suggest
>> below).
> 
> I don't have a strong opinion re naming, are you OK with the names
> proposed above?

Well, no - imo a function named e.g. flush_tlb_all_guests_cpumask() is
not supposed to flush any host TLBs. But I'll also reply to Julien's
subsequent reply in a minute.

Jan

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