Re: [PATCH v2] x86/flushtlb: remove flush_area check on system state

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2] x86/flushtlb: remove flush_area check on system state
Date: Wed, 25 May 2022 09:51:53 +0200	[thread overview]
Message-ID: <f14c2c10-427c-bad8-2762-985e3dc9a426@suse.com> (raw)
In-Reply-To: <Yo3e87RdD0/wXn6M@Air-de-Roger>

On 25.05.2022 09:46, Roger Pau Monné wrote:
> On Wed, May 25, 2022 at 09:34:32AM +0200, Jan Beulich wrote:
>> On 25.05.2022 09:21, Roger Pau Monné wrote:
>>> On Wed, May 25, 2022 at 08:02:17AM +0200, Jan Beulich wrote:
>>>> On 24.05.2022 18:46, Roger Pau Monné wrote:
>>>>> Would you be fine with adding:
>>>>>
>>>>> Note that FLUSH_FORCE_IPI doesn't need to be handled explicitly, as
>>>>> it's main purpose is to prevent the usage of the hypervisor assisted
>>>>> flush if available, not to force the sending of an IPI even for cases
>>>>> where it won't be sent.
>>>>
>>>> Hmm, yes, that's even more verbose than I would have expected it to
>>>> be. Just one point: I'm not sure about "main" there. Is there really
>>>> another purpose?
>>>
>>> Right, I should remove main.
>>>
>>>> Of course an alternative would be to rename the flag to properly
>>>> express what it's for (e.g. FLUSH_NO_HV_ASSIST). This would then
>>>> eliminate the need for a comment, afaic at least.
>>>
>>> I think it's likely that we also require this flag if we make use of
>>> hardware assisted flushes in the future, and hence it would better
>>> stay with the current name to avoid renaming in the future.
>>>
>>> Whether the avoidance of sending the IPI is due to hardware or
>>> hypervisor assistance is of no interest to the caller, it only cares
>>> to force a real IPI to be sent to remote processors.
>>
>> Well, then it could still be named FLUSH_NO_ASSIST, since as said
>> (and as you look to agree with) there's no IPI being forced in the
>> general case.
> 
> That would be fine but I don't think it's OK to do in this patch.
> 
> Could do as a prereq if you want, but we should keep in mind the patch
> under discussion is fixing a boot regression, the fact that it
> doesn't trigger in osstest is just because there's no hardware with
> CET Shadow Stacks support in the colo.

Sure - I'll be okay either way, with a preference to the rename over
the addition of a comment.

Jan