Due to the current rwlock logic, if the CPU calling get_cpu_maps() does so from
a cpu_hotplug_{begin,done}() region the function will still return success,
because a CPU taking the rwlock in read mode after having taken it in write
mode is allowed.  Such behavior however defeats the purpose of get_cpu_maps(),
as it should always return false when called with a CPU hot{,un}plug operation
is in progress.  Otherwise the logic in send_IPI_mask() is wrong, as it could
decide to use the shorthand even when a CPU operation is in progress.

Introduce a new helper to detect whether the current caller is between a
cpu_hotplug_{begin,done}() region and use it in send_IPI_mask() to restrict
shorthand usage.

Fixes: 5500d265a2a8 ('x86/smp: use APIC ALLBUT destination shorthand when possible')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Modify send_IPI_mask() to detect CPU hotplug context.
---
 xen/arch/x86/smp.c       |  2 +-
 xen/common/cpu.c         |  5 +++++
 xen/include/xen/cpu.h    | 10 ++++++++++
 xen/include/xen/rwlock.h |  2 ++
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index 7443ad20335e..04c6a0572319 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
      * the system have been accounted for.
      */
     if ( system_state > SYS_STATE_smp_boot &&
-         !unaccounted_cpus && !disabled_cpus &&
+         !unaccounted_cpus && !disabled_cpus && !cpu_in_hotplug_context() &&
          /* NB: get_cpu_maps lock requires enabled interrupts. */
          local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
          (park_offline_cpus ||
diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 8709db4d2957..6e35b114c080 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -68,6 +68,11 @@ void cpu_hotplug_done(void)
     write_unlock(&cpu_add_remove_lock);
 }
 
+bool cpu_in_hotplug_context(void)
+{
+    return rw_is_write_locked_by_me(&cpu_add_remove_lock);
+}
+
 static NOTIFIER_HEAD(cpu_chain);
 
 void __init register_cpu_notifier(struct notifier_block *nb)
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index e1d4eb59675c..6bf578675008 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -13,6 +13,16 @@ void put_cpu_maps(void);
 void cpu_hotplug_begin(void);
 void cpu_hotplug_done(void);
 
+/*
+ * Returns true when the caller CPU is between a cpu_hotplug_{begin,done}()
+ * region.
+ *
+ * This is required to safely identify hotplug contexts, as get_cpu_maps()
+ * would otherwise succeed because a caller holding the lock in write mode is
+ * allowed to acquire the same lock in read mode.
+ */
+bool cpu_in_hotplug_context(void);
+
 /* Receive notification of CPU hotplug events. */
 void register_cpu_notifier(struct notifier_block *nb);
 
diff --git a/xen/include/xen/rwlock.h b/xen/include/xen/rwlock.h
index a2e98cad343e..4e7802821859 100644
--- a/xen/include/xen/rwlock.h
+++ b/xen/include/xen/rwlock.h
@@ -316,6 +316,8 @@ static always_inline void write_lock_irq(rwlock_t *l)
 
 #define rw_is_locked(l)               _rw_is_locked(l)
 #define rw_is_write_locked(l)         _rw_is_write_locked(l)
+#define rw_is_write_locked_by_me(l) \
+    lock_evaluate_nospec(_is_write_locked_by_me(atomic_read(&(l)->cnts)))
 
 
 typedef struct percpu_rwlock percpu_rwlock_t;
-- 
2.44.0

On 10.06.2024 16:20, Roger Pau Monne wrote:
> Due to the current rwlock logic, if the CPU calling get_cpu_maps() does so from
> a cpu_hotplug_{begin,done}() region the function will still return success,
> because a CPU taking the rwlock in read mode after having taken it in write
> mode is allowed.  Such behavior however defeats the purpose of get_cpu_maps(),

I'm not happy to see you still have this claim here. The behavior may (appear
to) defeat the purpose here, but as expressed previously I don't view that as
being a general pattern.

> as it should always return false when called with a CPU hot{,un}plug operation
> is in progress.  Otherwise the logic in send_IPI_mask() is wrong, as it could
> decide to use the shorthand even when a CPU operation is in progress.
> 
> Introduce a new helper to detect whether the current caller is between a
> cpu_hotplug_{begin,done}() region and use it in send_IPI_mask() to restrict
> shorthand usage.
> 
> Fixes: 5500d265a2a8 ('x86/smp: use APIC ALLBUT destination shorthand when possible')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Modify send_IPI_mask() to detect CPU hotplug context.
> ---
>  xen/arch/x86/smp.c       |  2 +-
>  xen/common/cpu.c         |  5 +++++
>  xen/include/xen/cpu.h    | 10 ++++++++++
>  xen/include/xen/rwlock.h |  2 ++
>  4 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> index 7443ad20335e..04c6a0572319 100644
> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
>       * the system have been accounted for.
>       */
>      if ( system_state > SYS_STATE_smp_boot &&
> -         !unaccounted_cpus && !disabled_cpus &&
> +         !unaccounted_cpus && !disabled_cpus && !cpu_in_hotplug_context() &&
>           /* NB: get_cpu_maps lock requires enabled interrupts. */
>           local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
>           (park_offline_cpus ||

Along with changing the condition you also want to update the comment to
reflect the code adjustment.

If we can agree on respective wording in both places, I'd be happy to make
respective adjustments while committing; the code changes themselves are
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On Tue, Jun 11, 2024 at 09:42:39AM +0200, Jan Beulich wrote:
> On 10.06.2024 16:20, Roger Pau Monne wrote:
> > Due to the current rwlock logic, if the CPU calling get_cpu_maps() does so from
> > a cpu_hotplug_{begin,done}() region the function will still return success,
> > because a CPU taking the rwlock in read mode after having taken it in write
> > mode is allowed.  Such behavior however defeats the purpose of get_cpu_maps(),
> 
> I'm not happy to see you still have this claim here. The behavior may (appear
> to) defeat the purpose here, but as expressed previously I don't view that as
> being a general pattern.

Right.  What about replacing the paragraph with:

"Due to the current rwlock logic, if the CPU calling get_cpu_maps() does so from
a cpu_hotplug_{begin,done}() region the function will still return success,
because a CPU taking the rwlock in read mode after having taken it in write
mode is allowed.  Such corner case makes using get_cpu_maps() alone
not enough to prevent using the shorthand in CPU hotplug regions."

I think the rest is of the commit message is not controversial.

> > as it should always return false when called with a CPU hot{,un}plug operation
> > is in progress.  Otherwise the logic in send_IPI_mask() is wrong, as it could
> > decide to use the shorthand even when a CPU operation is in progress.
> > 
> > Introduce a new helper to detect whether the current caller is between a
> > cpu_hotplug_{begin,done}() region and use it in send_IPI_mask() to restrict
> > shorthand usage.
> > 
> > Fixes: 5500d265a2a8 ('x86/smp: use APIC ALLBUT destination shorthand when possible')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >  - Modify send_IPI_mask() to detect CPU hotplug context.
> > ---
> >  xen/arch/x86/smp.c       |  2 +-
> >  xen/common/cpu.c         |  5 +++++
> >  xen/include/xen/cpu.h    | 10 ++++++++++
> >  xen/include/xen/rwlock.h |  2 ++
> >  4 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> > index 7443ad20335e..04c6a0572319 100644
> > --- a/xen/arch/x86/smp.c
> > +++ b/xen/arch/x86/smp.c
> > @@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
> >       * the system have been accounted for.
> >       */
> >      if ( system_state > SYS_STATE_smp_boot &&
> > -         !unaccounted_cpus && !disabled_cpus &&
> > +         !unaccounted_cpus && !disabled_cpus && !cpu_in_hotplug_context() &&
> >           /* NB: get_cpu_maps lock requires enabled interrupts. */
> >           local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
> >           (park_offline_cpus ||
> 
> Along with changing the condition you also want to update the comment to
> reflect the code adjustment.

I've assumed the wording in the commet that says: "no CPU hotplug or
unplug operations are taking place" would already cover the addition
of the !cpu_in_hotplug_context() check.

Thanks, Roger.

On 12.06.2024 10:09, Roger Pau Monné wrote:
> On Tue, Jun 11, 2024 at 09:42:39AM +0200, Jan Beulich wrote:
>> On 10.06.2024 16:20, Roger Pau Monne wrote:
>>> Due to the current rwlock logic, if the CPU calling get_cpu_maps() does so from
>>> a cpu_hotplug_{begin,done}() region the function will still return success,
>>> because a CPU taking the rwlock in read mode after having taken it in write
>>> mode is allowed.  Such behavior however defeats the purpose of get_cpu_maps(),
>>
>> I'm not happy to see you still have this claim here. The behavior may (appear
>> to) defeat the purpose here, but as expressed previously I don't view that as
>> being a general pattern.
> 
> Right.  What about replacing the paragraph with:
> 
> "Due to the current rwlock logic, if the CPU calling get_cpu_maps() does so from
> a cpu_hotplug_{begin,done}() region the function will still return success,
> because a CPU taking the rwlock in read mode after having taken it in write
> mode is allowed.  Such corner case makes using get_cpu_maps() alone
> not enough to prevent using the shorthand in CPU hotplug regions."

Thanks.

> I think the rest is of the commit message is not controversial.

Indeed.

>>> --- a/xen/arch/x86/smp.c
>>> +++ b/xen/arch/x86/smp.c
>>> @@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
>>>       * the system have been accounted for.
>>>       */
>>>      if ( system_state > SYS_STATE_smp_boot &&
>>> -         !unaccounted_cpus && !disabled_cpus &&
>>> +         !unaccounted_cpus && !disabled_cpus && !cpu_in_hotplug_context() &&
>>>           /* NB: get_cpu_maps lock requires enabled interrupts. */
>>>           local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
>>>           (park_offline_cpus ||
>>
>> Along with changing the condition you also want to update the comment to
>> reflect the code adjustment.
> 
> I've assumed the wording in the commet that says: "no CPU hotplug or
> unplug operations are taking place" would already cover the addition
> of the !cpu_in_hotplug_context() check.

Hmm, yes, you're right. Just needs a release-ack then to go in (with the
description adjustment).

Jan

On Wed, 2024-06-12 at 10:56 +0200, Jan Beulich wrote:
> On 12.06.2024 10:09, Roger Pau Monné wrote:
> > On Tue, Jun 11, 2024 at 09:42:39AM +0200, Jan Beulich wrote:
> > > On 10.06.2024 16:20, Roger Pau Monne wrote:
> > > > Due to the current rwlock logic, if the CPU calling
> > > > get_cpu_maps() does so from
> > > > a cpu_hotplug_{begin,done}() region the function will still
> > > > return success,
> > > > because a CPU taking the rwlock in read mode after having taken
> > > > it in write
> > > > mode is allowed.  Such behavior however defeats the purpose of
> > > > get_cpu_maps(),
> > > 
> > > I'm not happy to see you still have this claim here. The behavior
> > > may (appear
> > > to) defeat the purpose here, but as expressed previously I don't
> > > view that as
> > > being a general pattern.
> > 
> > Right.  What about replacing the paragraph with:
> > 
> > "Due to the current rwlock logic, if the CPU calling get_cpu_maps()
> > does so from
> > a cpu_hotplug_{begin,done}() region the function will still return
> > success,
> > because a CPU taking the rwlock in read mode after having taken it
> > in write
> > mode is allowed.  Such corner case makes using get_cpu_maps() alone
> > not enough to prevent using the shorthand in CPU hotplug regions."
> 
> Thanks.
> 
> > I think the rest is of the commit message is not controversial.
> 
> Indeed.
> 
> > > > --- a/xen/arch/x86/smp.c
> > > > +++ b/xen/arch/x86/smp.c
> > > > @@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int
> > > > vector)
> > > >       * the system have been accounted for.
> > > >       */
> > > >      if ( system_state > SYS_STATE_smp_boot &&
> > > > -         !unaccounted_cpus && !disabled_cpus &&
> > > > +         !unaccounted_cpus && !disabled_cpus &&
> > > > !cpu_in_hotplug_context() &&
> > > >           /* NB: get_cpu_maps lock requires enabled interrupts.
> > > > */
> > > >           local_irq_is_enabled() && (cpus_locked =
> > > > get_cpu_maps()) &&
> > > >           (park_offline_cpus ||
> > > 
> > > Along with changing the condition you also want to update the
> > > comment to
> > > reflect the code adjustment.
> > 
> > I've assumed the wording in the commet that says: "no CPU hotplug
> > or
> > unplug operations are taking place" would already cover the
> > addition
> > of the !cpu_in_hotplug_context() check.
> 
> Hmm, yes, you're right. Just needs a release-ack then to go in (with
> the
> description adjustment).
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii