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

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

[Roger Pau Monne]

Booting with Shadow Stacks leads to the following assert on a debug
hypervisor:

Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
CPU:    0
RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
[...]
Xen call trace:
   [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
   [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
   [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
   [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
   [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
   [<ffff82d040203344>] F __high_start+0x94/0xa0


This is due to SYS_STATE_smp_boot being set before calling
alternative_branches(), and the flush in modify_xen_mappings() then
using flush_area_all() with interrupts disabled.  Note that
alternative_branches() is called before APs are started, so the flush
must be a local one (and indeed the cpumask passed to
flush_area_mask() just contains one CPU).

Take the opportunity to simplify a bit the logic and intorduce
flush_area_all() as an alias for flush_area_mask(&cpu_online_map...),
taking into account that cpu_online_map just contains the BSP before
APs are started.  This requires widening the assert in
flush_area_mask() to allow being called with interrupts disabled as
long as it's strictly a local only flush.

The overall result is that a conditional can be removed from
flush_area().

While there also introduce an ASSERT to check that a vCPU state flush
is not issued for the local CPU only.

Fixes: (78e072bc37 'x86/mm: avoid inadvertently degrading a TLB flush to local only')
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Add an extra assert.
 - Rename flush_area() to flush_area_all().
---
 xen/arch/x86/include/asm/flushtlb.h |  3 ++-
 xen/arch/x86/mm.c                   | 32 +++++++++++------------------
 xen/arch/x86/smp.c                  |  5 ++++-
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/include/asm/flushtlb.h b/xen/arch/x86/include/asm/flushtlb.h
index 18777f1d4c..f0094bf747 100644
--- a/xen/arch/x86/include/asm/flushtlb.h
+++ b/xen/arch/x86/include/asm/flushtlb.h
@@ -146,7 +146,8 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags);
 #define flush_mask(mask, flags) flush_area_mask(mask, NULL, flags)
 
 /* Flush all CPUs' TLBs/caches */
-#define flush_area_all(va, flags) flush_area_mask(&cpu_online_map, va, flags)
+#define flush_area_all(va, flags) \
+    flush_area_mask(&cpu_online_map, (const void *)(va), flags)
 #define flush_all(flags) flush_mask(&cpu_online_map, flags)
 
 /* Flush local TLBs */
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 72dbce43b1..96d95a07cd 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5070,14 +5070,6 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
 #define l1f_to_lNf(f) (((f) & _PAGE_PRESENT) ? ((f) |  _PAGE_PSE) : (f))
 #define lNf_to_l1f(f) (((f) & _PAGE_PRESENT) ? ((f) & ~_PAGE_PSE) : (f))
 
-/*
- * map_pages_to_xen() can be called early in boot before any other
- * CPUs are online. Use flush_area_local() in this case.
- */
-#define flush_area(v,f) (system_state < SYS_STATE_smp_boot ?    \
-                         flush_area_local((const void *)v, f) : \
-                         flush_area_all((const void *)v, f))
-
 #define L3T_INIT(page) (page) = ZERO_BLOCK_PTR
 
 #define L3T_LOCK(page)        \
@@ -5213,7 +5205,7 @@ int map_pages_to_xen(
                 if ( l3e_get_flags(ol3e) & _PAGE_PSE )
                 {
                     flush_flags(lNf_to_l1f(l3e_get_flags(ol3e)));
-                    flush_area(virt, flush_flags);
+                    flush_area_all(virt, flush_flags);
                 }
                 else
                 {
@@ -5236,7 +5228,7 @@ int map_pages_to_xen(
                             unmap_domain_page(l1t);
                         }
                     }
-                    flush_area(virt, flush_flags);
+                    flush_area_all(virt, flush_flags);
                     for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
                     {
                         ol2e = l2t[i];
@@ -5310,7 +5302,7 @@ int map_pages_to_xen(
             }
             if ( locking )
                 spin_unlock(&map_pgdir_lock);
-            flush_area(virt, flush_flags);
+            flush_area_all(virt, flush_flags);
 
             free_xen_pagetable(l2mfn);
         }
@@ -5336,7 +5328,7 @@ int map_pages_to_xen(
                 if ( l2e_get_flags(ol2e) & _PAGE_PSE )
                 {
                     flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
-                    flush_area(virt, flush_flags);
+                    flush_area_all(virt, flush_flags);
                 }
                 else
                 {
@@ -5344,7 +5336,7 @@ int map_pages_to_xen(
 
                     for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
                         flush_flags(l1e_get_flags(l1t[i]));
-                    flush_area(virt, flush_flags);
+                    flush_area_all(virt, flush_flags);
                     unmap_domain_page(l1t);
                     free_xen_pagetable(l2e_get_mfn(ol2e));
                 }
@@ -5415,7 +5407,7 @@ int map_pages_to_xen(
                 }
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                flush_area(virt, flush_flags);
+                flush_area_all(virt, flush_flags);
 
                 free_xen_pagetable(l1mfn);
             }
@@ -5430,7 +5422,7 @@ int map_pages_to_xen(
                 unsigned int flush_flags = FLUSH_TLB | FLUSH_ORDER(0);
 
                 flush_flags(l1e_get_flags(ol1e));
-                flush_area(virt, flush_flags);
+                flush_area_all(virt, flush_flags);
             }
 
             virt    += 1UL << L1_PAGETABLE_SHIFT;
@@ -5481,7 +5473,7 @@ int map_pages_to_xen(
                                                         l1f_to_lNf(flags)));
                     if ( locking )
                         spin_unlock(&map_pgdir_lock);
-                    flush_area(virt - PAGE_SIZE,
+                    flush_area_all(virt - PAGE_SIZE,
                                FLUSH_TLB_GLOBAL |
                                FLUSH_ORDER(PAGETABLE_ORDER));
                     free_xen_pagetable(l2e_get_mfn(ol2e));
@@ -5532,7 +5524,7 @@ int map_pages_to_xen(
                                                     l1f_to_lNf(flags)));
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                flush_area(virt - PAGE_SIZE,
+                flush_area_all(virt - PAGE_SIZE,
                            FLUSH_TLB_GLOBAL |
                            FLUSH_ORDER(2*PAGETABLE_ORDER));
                 free_xen_pagetable(l3e_get_mfn(ol3e));
@@ -5784,7 +5776,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
                 l2e_write_atomic(pl2e, l2e_empty());
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                flush_area(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
+                flush_area_all(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
                 free_xen_pagetable(l1mfn);
             }
             else if ( locking )
@@ -5829,7 +5821,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
                 l3e_write_atomic(pl3e, l3e_empty());
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                flush_area(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
+                flush_area_all(NULL, FLUSH_TLB_GLOBAL); /* flush before free */
                 free_xen_pagetable(l2mfn);
             }
             else if ( locking )
@@ -5837,7 +5829,7 @@ int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
         }
     }
 
-    flush_area(NULL, FLUSH_TLB_GLOBAL);
+    flush_area_all(NULL, FLUSH_TLB_GLOBAL);
 
 #undef FLAGS_MASK
     rc = 0;
diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index 0a02086966..b42603c351 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -262,7 +262,10 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
 {
     unsigned int cpu = smp_processor_id();
 
-    ASSERT(local_irq_is_enabled());
+    /* Local flushes can be performed with interrupts disabled. */
+    ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)));
+    /* Exclude use of FLUSH_VCPU_STATE for the local CPU. */
+    ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));
 
     if ( (flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK)) &&
          cpumask_test_cpu(cpu, mask) )
-- 
2.36.0

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

[Jan Beulich]

On 24.05.2022 12:50, Roger Pau Monne wrote:
> Booting with Shadow Stacks leads to the following assert on a debug
> hypervisor:
> 
> Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
> CPU:    0
> RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
> [...]
> Xen call trace:
>    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
>    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
>    [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
>    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
>    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
>    [<ffff82d040203344>] F __high_start+0x94/0xa0
> 
> 
> This is due to SYS_STATE_smp_boot being set before calling
> alternative_branches(), and the flush in modify_xen_mappings() then
> using flush_area_all() with interrupts disabled.  Note that
> alternative_branches() is called before APs are started, so the flush
> must be a local one (and indeed the cpumask passed to
> flush_area_mask() just contains one CPU).
> 
> Take the opportunity to simplify a bit the logic and intorduce
> flush_area_all() as an alias for flush_area_mask(&cpu_online_map...),

This is now stale - you don't introduce flush_area_all() here.
Sadly nothing is said to justify the addition of a cast there,
which - as said before - I think is a little risky (as many
casts are), and hence would imo better be avoided.

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -262,7 +262,10 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
>  {
>      unsigned int cpu = smp_processor_id();
>  
> -    ASSERT(local_irq_is_enabled());
> +    /* Local flushes can be performed with interrupts disabled. */
> +    ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)));
> +    /* Exclude use of FLUSH_VCPU_STATE for the local CPU. */
> +    ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));

What about FLUSH_FORCE_IPI? This won't work either with IRQs off,
I'm afraid. Or wait - that flag's name doesn't really look to
force the use of an IPI, it's still constrained to remote
requests. I think this wants mentioning in one of the comments,
not the least to also have grep match there then (right now grep
output gives the impression as if the flag wasn't consumed
anywhere).

Jan

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

[Roger Pau Monné]

On Tue, May 24, 2022 at 05:27:35PM +0200, Jan Beulich wrote:
> On 24.05.2022 12:50, Roger Pau Monne wrote:
> > Booting with Shadow Stacks leads to the following assert on a debug
> > hypervisor:
> > 
> > Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> > ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
> > CPU:    0
> > RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
> > [...]
> > Xen call trace:
> >    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
> >    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
> >    [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
> >    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
> >    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
> >    [<ffff82d040203344>] F __high_start+0x94/0xa0
> > 
> > 
> > This is due to SYS_STATE_smp_boot being set before calling
> > alternative_branches(), and the flush in modify_xen_mappings() then
> > using flush_area_all() with interrupts disabled.  Note that
> > alternative_branches() is called before APs are started, so the flush
> > must be a local one (and indeed the cpumask passed to
> > flush_area_mask() just contains one CPU).
> > 
> > Take the opportunity to simplify a bit the logic and intorduce
> > flush_area_all() as an alias for flush_area_mask(&cpu_online_map...),
> 
> This is now stale - you don't introduce flush_area_all() here.
> Sadly nothing is said to justify the addition of a cast there,
> which - as said before - I think is a little risky (as many
> casts are), and hence would imo better be avoided.

So prior to this change there are no direct callers to
flush_area_all(), and hence all callers use flush_area() which has the
cast.  Now that I remove flush_area() and modify callers to use
flush_area_all() directly it seems natural to also move the cast
there.  While I agree that having casts is not desirable, I wouldn't
consider this change as adding them.  Merely moving them but the
result is that the callers get the cast like they used to do.

> 
> > --- a/xen/arch/x86/smp.c
> > +++ b/xen/arch/x86/smp.c
> > @@ -262,7 +262,10 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
> >  {
> >      unsigned int cpu = smp_processor_id();
> >  
> > -    ASSERT(local_irq_is_enabled());
> > +    /* Local flushes can be performed with interrupts disabled. */
> > +    ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)));
> > +    /* Exclude use of FLUSH_VCPU_STATE for the local CPU. */
> > +    ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));
> 
> What about FLUSH_FORCE_IPI? This won't work either with IRQs off,
> I'm afraid. Or wait - that flag's name doesn't really look to
> force the use of an IPI, it's still constrained to remote
> requests. I think this wants mentioning in one of the comments,
> not the least to also have grep match there then (right now grep
> output gives the impression as if the flag wasn't consumed
> anywhere).

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.

Thanks, Roger.

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

[Jan Beulich]

On 24.05.2022 18:46, Roger Pau Monné wrote:
> On Tue, May 24, 2022 at 05:27:35PM +0200, Jan Beulich wrote:
>> On 24.05.2022 12:50, Roger Pau Monne wrote:
>>> Booting with Shadow Stacks leads to the following assert on a debug
>>> hypervisor:
>>>
>>> Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
>>> ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
>>> CPU:    0
>>> RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
>>> [...]
>>> Xen call trace:
>>>    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
>>>    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
>>>    [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
>>>    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
>>>    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
>>>    [<ffff82d040203344>] F __high_start+0x94/0xa0
>>>
>>>
>>> This is due to SYS_STATE_smp_boot being set before calling
>>> alternative_branches(), and the flush in modify_xen_mappings() then
>>> using flush_area_all() with interrupts disabled.  Note that
>>> alternative_branches() is called before APs are started, so the flush
>>> must be a local one (and indeed the cpumask passed to
>>> flush_area_mask() just contains one CPU).
>>>
>>> Take the opportunity to simplify a bit the logic and intorduce
>>> flush_area_all() as an alias for flush_area_mask(&cpu_online_map...),
>>
>> This is now stale - you don't introduce flush_area_all() here.
>> Sadly nothing is said to justify the addition of a cast there,
>> which - as said before - I think is a little risky (as many
>> casts are), and hence would imo better be avoided.
> 
> So prior to this change there are no direct callers to
> flush_area_all(), and hence all callers use flush_area() which has the
> cast.  Now that I remove flush_area() and modify callers to use
> flush_area_all() directly it seems natural to also move the cast
> there.  While I agree that having casts is not desirable, I wouldn't
> consider this change as adding them.  Merely moving them but the
> result is that the callers get the cast like they used to do.

I'd agree with all of this if the change was local to mm.c. As I'd
like to see the macro in flushtlb.h left unchanged, did you consider
retaining flush_area() as a wrapper in mm.c, reduced to merely
invoking flush_area_all() with the cast added? That would also
reduce the code churn of the patch.

>>> --- a/xen/arch/x86/smp.c
>>> +++ b/xen/arch/x86/smp.c
>>> @@ -262,7 +262,10 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
>>>  {
>>>      unsigned int cpu = smp_processor_id();
>>>  
>>> -    ASSERT(local_irq_is_enabled());
>>> +    /* Local flushes can be performed with interrupts disabled. */
>>> +    ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)));
>>> +    /* Exclude use of FLUSH_VCPU_STATE for the local CPU. */
>>> +    ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));
>>
>> What about FLUSH_FORCE_IPI? This won't work either with IRQs off,
>> I'm afraid. Or wait - that flag's name doesn't really look to
>> force the use of an IPI, it's still constrained to remote
>> requests. I think this wants mentioning in one of the comments,
>> not the least to also have grep match there then (right now grep
>> output gives the impression as if the flag wasn't consumed
>> anywhere).
> 
> 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?

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.

Jan

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

[Roger Pau Monné]

On Wed, May 25, 2022 at 08:02:17AM +0200, Jan Beulich wrote:
> On 24.05.2022 18:46, Roger Pau Monné wrote:
> > On Tue, May 24, 2022 at 05:27:35PM +0200, Jan Beulich wrote:
> >> On 24.05.2022 12:50, Roger Pau Monne wrote:
> >>> Booting with Shadow Stacks leads to the following assert on a debug
> >>> hypervisor:
> >>>
> >>> Assertion 'local_irq_is_enabled()' failed at arch/x86/smp.c:265
> >>> ----[ Xen-4.17.0-10.24-d  x86_64  debug=y  Not tainted ]----
> >>> CPU:    0
> >>> RIP:    e008:[<ffff82d040345300>] flush_area_mask+0x40/0x13e
> >>> [...]
> >>> Xen call trace:
> >>>    [<ffff82d040345300>] R flush_area_mask+0x40/0x13e
> >>>    [<ffff82d040338a40>] F modify_xen_mappings+0xc5/0x958
> >>>    [<ffff82d0404474f9>] F arch/x86/alternative.c#_alternative_instructions+0xb7/0xb9
> >>>    [<ffff82d0404476cc>] F alternative_branches+0xf/0x12
> >>>    [<ffff82d04044e37d>] F __start_xen+0x1ef4/0x2776
> >>>    [<ffff82d040203344>] F __high_start+0x94/0xa0
> >>>
> >>>
> >>> This is due to SYS_STATE_smp_boot being set before calling
> >>> alternative_branches(), and the flush in modify_xen_mappings() then
> >>> using flush_area_all() with interrupts disabled.  Note that
> >>> alternative_branches() is called before APs are started, so the flush
> >>> must be a local one (and indeed the cpumask passed to
> >>> flush_area_mask() just contains one CPU).
> >>>
> >>> Take the opportunity to simplify a bit the logic and intorduce
> >>> flush_area_all() as an alias for flush_area_mask(&cpu_online_map...),
> >>
> >> This is now stale - you don't introduce flush_area_all() here.
> >> Sadly nothing is said to justify the addition of a cast there,
> >> which - as said before - I think is a little risky (as many
> >> casts are), and hence would imo better be avoided.
> > 
> > So prior to this change there are no direct callers to
> > flush_area_all(), and hence all callers use flush_area() which has the
> > cast.  Now that I remove flush_area() and modify callers to use
> > flush_area_all() directly it seems natural to also move the cast
> > there.  While I agree that having casts is not desirable, I wouldn't
> > consider this change as adding them.  Merely moving them but the
> > result is that the callers get the cast like they used to do.
> 
> I'd agree with all of this if the change was local to mm.c. As I'd
> like to see the macro in flushtlb.h left unchanged, did you consider
> retaining flush_area() as a wrapper in mm.c, reduced to merely
> invoking flush_area_all() with the cast added? That would also
> reduce the code churn of the patch.

Hm, yes, didn't consider this, but could do.  I didn't want to keep
flush_area() globally, but adding to mm.c only could be OK in order to
limit the cast.

> >>> --- a/xen/arch/x86/smp.c
> >>> +++ b/xen/arch/x86/smp.c
> >>> @@ -262,7 +262,10 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
> >>>  {
> >>>      unsigned int cpu = smp_processor_id();
> >>>  
> >>> -    ASSERT(local_irq_is_enabled());
> >>> +    /* Local flushes can be performed with interrupts disabled. */
> >>> +    ASSERT(local_irq_is_enabled() || cpumask_subset(mask, cpumask_of(cpu)));
> >>> +    /* Exclude use of FLUSH_VCPU_STATE for the local CPU. */
> >>> +    ASSERT(!cpumask_test_cpu(cpu, mask) || !(flags & FLUSH_VCPU_STATE));
> >>
> >> What about FLUSH_FORCE_IPI? This won't work either with IRQs off,
> >> I'm afraid. Or wait - that flag's name doesn't really look to
> >> force the use of an IPI, it's still constrained to remote
> >> requests. I think this wants mentioning in one of the comments,
> >> not the least to also have grep match there then (right now grep
> >> output gives the impression as if the flag wasn't consumed
> >> anywhere).
> > 
> > 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.

Thanks, Roger.

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

[Jan Beulich]

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.

Jan

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Jan Beulich]

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