diff mbox series

[v3,1/2] x86/mm: rename FLUSH_FORCE_IPI to FLUSH_NO_ASSIST

Message ID 20220525081311.13416-2-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/mm: fix regression when booting with CET-SS | expand

Commit Message

Roger Pau Monné May 25, 2022, 8:13 a.m. UTC
Rename the flag to better note that it's not actually forcing any IPIs
to be issued if none is required, but merely avoiding the usage of TLB
flush assistance (which itself can avoid the sending of IPIs to remote
processors).

No functional change expected.

Requested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/include/asm/flushtlb.h | 16 ++++++++--------
 xen/arch/x86/mm.c                   |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

Jan Beulich May 25, 2022, 8:39 a.m. UTC | #1
On 25.05.2022 10:13, Roger Pau Monne wrote:
> Rename the flag to better note that it's not actually forcing any IPIs
> to be issued if none is required, but merely avoiding the usage of TLB
> flush assistance (which itself can avoid the sending of IPIs to remote
> processors).
> 
> No functional change expected.
> 
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Andrew Cooper May 25, 2022, 10:52 a.m. UTC | #2
On 25/05/2022 09:13, Roger Pau Monne wrote:
> Rename the flag to better note that it's not actually forcing any IPIs
> to be issued if none is required, but merely avoiding the usage of TLB
> flush assistance (which itself can avoid the sending of IPIs to remote
> processors).
>
> No functional change expected.
>
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v2:
>  - New in this version.

:(  This needs reverting.

It is specific to IPIs, because of our current choice of algorithm for
freeing pagetables.

"no assist" excludes ipi-helper hypercalls which invoke
INVALIDATE_TLB_VECTOR.  Such hypercalls do exist and specifically would
be improvement that we ought to use.

Furthermore, we do want to work around the limitation that created
FLUSH_FORCE_IPI, because we absolutely do want to be able to use
hypercalls/remote TLB flushing capabilities when available.

I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole
lot less bad than NO_ASSIST.

~Andrew
Jan Beulich May 25, 2022, 11:14 a.m. UTC | #3
On 25.05.2022 12:52, Andrew Cooper wrote:
> On 25/05/2022 09:13, Roger Pau Monne wrote:
>> Rename the flag to better note that it's not actually forcing any IPIs
>> to be issued if none is required, but merely avoiding the usage of TLB
>> flush assistance (which itself can avoid the sending of IPIs to remote
>> processors).
>>
>> No functional change expected.
>>
>> Requested-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Changes since v2:
>>  - New in this version.
> 
> :(  This needs reverting.
> 
> It is specific to IPIs, because of our current choice of algorithm for
> freeing pagetables.
> 
> "no assist" excludes ipi-helper hypercalls which invoke
> INVALIDATE_TLB_VECTOR.  Such hypercalls do exist and specifically would
> be improvement that we ought to use.
> 
> Furthermore, we do want to work around the limitation that created
> FLUSH_FORCE_IPI, because we absolutely do want to be able to use
> hypercalls/remote TLB flushing capabilities when available.
> 
> I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole
> lot less bad than NO_ASSIST.

But FORCE_IPI has caused actual confusion while reviewing patch 2.
If NO_ASSIST doesn't suit you and FORCE_IPI is also wrong, can you
suggest a better name fitting both aspects?

Jan
Andrew Cooper May 25, 2022, 2:33 p.m. UTC | #4
On 25/05/2022 12:14, Jan Beulich wrote:
> On 25.05.2022 12:52, Andrew Cooper wrote:
>> On 25/05/2022 09:13, Roger Pau Monne wrote:
>>> Rename the flag to better note that it's not actually forcing any IPIs
>>> to be issued if none is required, but merely avoiding the usage of TLB
>>> flush assistance (which itself can avoid the sending of IPIs to remote
>>> processors).
>>>
>>> No functional change expected.
>>>
>>> Requested-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Changes since v2:
>>>  - New in this version.
>> :(  This needs reverting.
>>
>> It is specific to IPIs, because of our current choice of algorithm for
>> freeing pagetables.
>>
>> "no assist" excludes ipi-helper hypercalls which invoke
>> INVALIDATE_TLB_VECTOR.  Such hypercalls do exist and specifically would
>> be improvement that we ought to use.
>>
>> Furthermore, we do want to work around the limitation that created
>> FLUSH_FORCE_IPI, because we absolutely do want to be able to use
>> hypercalls/remote TLB flushing capabilities when available.
>>
>> I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole
>> lot less bad than NO_ASSIST.
> But FORCE_IPI has caused actual confusion while reviewing patch 2.
> If NO_ASSIST doesn't suit you and FORCE_IPI is also wrong, can you
> suggest a better name fitting both aspects?

I don't actually agree that FORCE_IPI is unclear to begin with.

The safety property required is "if you need to contact a remote CPU, it
must be by IPI to interlock with Xen's #PF handler".

FORCE_IPI is very close to meaning this.  If anything else is unclear,
it can be clarified in the adjacent comment.

~Andrew
Roger Pau Monné May 25, 2022, 2:40 p.m. UTC | #5
On Wed, May 25, 2022 at 10:52:48AM +0000, Andrew Cooper wrote:
> On 25/05/2022 09:13, Roger Pau Monne wrote:
> > Rename the flag to better note that it's not actually forcing any IPIs
> > to be issued if none is required, but merely avoiding the usage of TLB
> > flush assistance (which itself can avoid the sending of IPIs to remote
> > processors).
> >
> > No functional change expected.
> >
> > Requested-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v2:
> >  - New in this version.
> 
> :(  This needs reverting.
> 
> It is specific to IPIs, because of our current choice of algorithm for
> freeing pagetables.
> 
> "no assist" excludes ipi-helper hypercalls which invoke
> INVALIDATE_TLB_VECTOR.  Such hypercalls do exist and specifically would
> be improvement that we ought to use.

So the improvement of that mechanism is that you can pass a cpumask
parameter to an hypercall in order to avoid having to issue repeated
wrmsrs (or APIC MMIO accesses) to send IPIs to different vCPUs?

But that could be seen as generic assistance for triggering the
execution of remote IDT handlers, and as such won't be restricted by
the NO_ASSIST flag (also because it would be implemented in
send_IPI_mask() rather than flush_area_mask() IMO).

> Furthermore, we do want to work around the limitation that created
> FLUSH_FORCE_IPI, because we absolutely do want to be able to use
> hypercalls/remote TLB flushing capabilities when available.

I agree, we need to get rid of FLUSH_FORCE_IPI.

> I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole
> lot less bad than NO_ASSIST.

I'm happy for you and Jan to decide on a different name that you both
agree.

Thanks, Roger.
Jan Beulich May 25, 2022, 2:41 p.m. UTC | #6
On 25.05.2022 16:33, Andrew Cooper wrote:
> On 25/05/2022 12:14, Jan Beulich wrote:
>> On 25.05.2022 12:52, Andrew Cooper wrote:
>>> On 25/05/2022 09:13, Roger Pau Monne wrote:
>>>> Rename the flag to better note that it's not actually forcing any IPIs
>>>> to be issued if none is required, but merely avoiding the usage of TLB
>>>> flush assistance (which itself can avoid the sending of IPIs to remote
>>>> processors).
>>>>
>>>> No functional change expected.
>>>>
>>>> Requested-by: Jan Beulich <jbeulich@suse.com>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> ---
>>>> Changes since v2:
>>>>  - New in this version.
>>> :(  This needs reverting.
>>>
>>> It is specific to IPIs, because of our current choice of algorithm for
>>> freeing pagetables.
>>>
>>> "no assist" excludes ipi-helper hypercalls which invoke
>>> INVALIDATE_TLB_VECTOR.  Such hypercalls do exist and specifically would
>>> be improvement that we ought to use.
>>>
>>> Furthermore, we do want to work around the limitation that created
>>> FLUSH_FORCE_IPI, because we absolutely do want to be able to use
>>> hypercalls/remote TLB flushing capabilities when available.
>>>
>>> I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole
>>> lot less bad than NO_ASSIST.
>> But FORCE_IPI has caused actual confusion while reviewing patch 2.
>> If NO_ASSIST doesn't suit you and FORCE_IPI is also wrong, can you
>> suggest a better name fitting both aspects?
> 
> I don't actually agree that FORCE_IPI is unclear to begin with.

You did see the earlier communication with Roger, didn't you? To
me the name pretty clearly suggests "always IPI" (hence "force"),
i.e. ...

> The safety property required is "if you need to contact a remote CPU, it
> must be by IPI to interlock with Xen's #PF handler".

... not in any way limited to remote CPUs. Yet patch 2 is about
cases where things are safe because no IPI will be needed (not
even a self-IPI).

> FORCE_IPI is very close to meaning this.  If anything else is unclear,
> it can be clarified in the adjacent comment.

I'm afraid I don't think a comment is what would help here.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/include/asm/flushtlb.h b/xen/arch/x86/include/asm/flushtlb.h
index 18777f1d4c..a461ee36ff 100644
--- a/xen/arch/x86/include/asm/flushtlb.h
+++ b/xen/arch/x86/include/asm/flushtlb.h
@@ -128,13 +128,12 @@  void switch_cr3_cr4(unsigned long cr3, unsigned long cr4);
 #endif
 #if defined(CONFIG_PV) || defined(CONFIG_SHADOW_PAGING)
 /*
- * Force an IPI to be sent. Note that adding this to the flags passed to
- * flush_area_mask will prevent using the assisted flush without having any
- * other side effect.
+ * Adding this to the flags passed to flush_area_mask will prevent using the
+ * assisted flush without having any other side effect.
  */
-# define FLUSH_FORCE_IPI 0x8000
+# define FLUSH_NO_ASSIST 0x8000
 #else
-# define FLUSH_FORCE_IPI 0
+# define FLUSH_NO_ASSIST 0
 #endif
 
 /* Flush local TLBs/caches. */
@@ -162,11 +161,12 @@  void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags);
     flush_area_mask(mask, (const void *)(v), FLUSH_TLB|FLUSH_ORDER(0))
 
 /*
- * Make the common code TLB flush helper force use of an IPI in order to be
- * on the safe side. Note that not all calls from common code strictly require
+ * Make the common code TLB flush helper disallow the usage of any flush
+ * assistance in order to be on the safe side and interrupt remote processors
+ * requiring a flush. Note that not all calls from common code strictly require
  * this.
  */
-#define arch_flush_tlb_mask(mask) flush_mask(mask, FLUSH_TLB | FLUSH_FORCE_IPI)
+#define arch_flush_tlb_mask(mask) flush_mask(mask, FLUSH_TLB | FLUSH_NO_ASSIST)
 
 /* Flush all CPUs' TLBs */
 #define flush_tlb_all()                         \
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 72dbce43b1..bbb834c3fb 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2988,7 +2988,7 @@  static int _get_page_type(struct page_info *page, unsigned long type,
                     flush_mask(mask,
                                (x & PGT_type_mask) &&
                                (x & PGT_type_mask) <= PGT_root_page_table
-                               ? FLUSH_TLB | FLUSH_FORCE_IPI
+                               ? FLUSH_TLB | FLUSH_NO_ASSIST
                                : FLUSH_TLB);
                 }