With the waitqueue logic updated to not use an absolute stack pointer
reference, the vCPU can safely be resumed anywhere.

Remove VCPU_AFFINITY_WAIT completely, getting rid of two domain crashes, and a
logical corner case where resetting the vcpu with an oustanding waitqueue
would crash the domain.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Juergen Gross <jgross@suse.com>
CC: Dario Faggioli <dfaggioli@suse.com>
---
 xen/common/domain.c     |  2 --
 xen/common/sched/core.c |  4 +---
 xen/common/wait.c       | 23 -----------------------
 xen/include/xen/sched.h |  1 -
 4 files changed, 1 insertion(+), 29 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 618410e3b257..323b92102cce 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1428,8 +1428,6 @@ int vcpu_reset(struct vcpu *v)
     v->is_initialised  = 0;
     if ( v->affinity_broken & VCPU_AFFINITY_OVERRIDE )
         vcpu_temporary_affinity(v, NR_CPUS, VCPU_AFFINITY_OVERRIDE);
-    if ( v->affinity_broken & VCPU_AFFINITY_WAIT )
-        vcpu_temporary_affinity(v, NR_CPUS, VCPU_AFFINITY_WAIT);
     clear_bit(_VPF_blocked, &v->pause_flags);
     clear_bit(_VPF_in_reset, &v->pause_flags);
 
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index f689b55783f7..cff8e59aba7c 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1610,12 +1610,10 @@ void watchdog_domain_destroy(struct domain *d)
 /*
  * Pin a vcpu temporarily to a specific CPU (or restore old pinning state if
  * cpu is NR_CPUS).
- * Temporary pinning can be done due to two reasons, which may be nested:
+ * Temporary pinning can be done for a number of reasons, which may be nested:
  * - VCPU_AFFINITY_OVERRIDE (requested by guest): is allowed to fail in case
  *   of a conflict (e.g. in case cpupool doesn't include requested CPU, or
  *   another conflicting temporary pinning is already in effect.
- * - VCPU_AFFINITY_WAIT (called by wait_event()): only used to pin vcpu to the
- *   CPU it is just running on. Can't fail if used properly.
  */
 int vcpu_temporary_affinity(struct vcpu *v, unsigned int cpu, uint8_t reason)
 {
diff --git a/xen/common/wait.c b/xen/common/wait.c
index 4f1daf650bc4..bd6f09662ac0 100644
--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -127,16 +127,6 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
 
     ASSERT(wqv->used == 0);
 
-    /* Save current VCPU affinity; force wakeup on *this* CPU only. */
-    if ( vcpu_temporary_affinity(curr, smp_processor_id(), VCPU_AFFINITY_WAIT) )
-    {
-        gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
-        domain_crash(curr->domain);
-
-        for ( ; ; )
-            do_softirq();
-    }
-
     /*
      * Hand-rolled setjmp().
      *
@@ -187,7 +177,6 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
 static void __finish_wait(struct waitqueue_vcpu *wqv)
 {
     wqv->used = 0;
-    vcpu_temporary_affinity(current, NR_CPUS, VCPU_AFFINITY_WAIT);
 }
 
 void check_wakeup_from_wait(void)
@@ -201,18 +190,6 @@ void check_wakeup_from_wait(void)
     if ( likely(!wqv->used) )
         return;
 
-    /* Check if we are still pinned. */
-    if ( unlikely(!(curr->affinity_broken & VCPU_AFFINITY_WAIT)) )
-    {
-        gdprintk(XENLOG_ERR, "vcpu affinity lost\n");
-        domain_crash(curr->domain);
-
-        /* Re-initiate scheduler and don't longjmp(). */
-        raise_softirq(SCHEDULE_SOFTIRQ);
-        for ( ; ; )
-            do_softirq();
-    }
-
     /*
      * Hand-rolled longjmp().
      *
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b9515eb497de..ba859a4abed3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -223,7 +223,6 @@ struct vcpu
     /* VCPU need affinity restored */
     uint8_t          affinity_broken;
 #define VCPU_AFFINITY_OVERRIDE    0x01
-#define VCPU_AFFINITY_WAIT        0x02
 
     /* A hypercall has been preempted. */
     bool             hcall_preempted;
-- 
2.11.0

On 18.07.2022 09:18, Andrew Cooper wrote:
> With the waitqueue logic updated to not use an absolute stack pointer
> reference, the vCPU can safely be resumed anywhere.
> 
> Remove VCPU_AFFINITY_WAIT completely, getting rid of two domain crashes,

I understand you mean two domain_crash() invocations here, but ...

> and a
> logical corner case where resetting the vcpu with an oustanding waitqueue
> would crash the domain.

... some other domain crash here?

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I assume you've checked thoroughly that calling code hasn't
grown dependencies on execution coming back on the same CPU?

Jan

On 18/07/2022 11:48, Jan Beulich wrote:
> On 18.07.2022 09:18, Andrew Cooper wrote:
>> With the waitqueue logic updated to not use an absolute stack pointer
>> reference, the vCPU can safely be resumed anywhere.
>>
>> Remove VCPU_AFFINITY_WAIT completely, getting rid of two domain crashes,
> I understand you mean two domain_crash() invocations here, but ...
>
>> and a
>> logical corner case where resetting the vcpu with an oustanding waitqueue
>> would crash the domain.
> ... some other domain crash here?

One of the two above.  It's more that resetting (would have) broken the
affinity and would have triggered the domain crash.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I assume you've checked thoroughly that calling code hasn't
> grown dependencies on execution coming back on the same CPU?

Urgh yes, my trivial test case didn't encounter it, but anything with an
smp_processor_id() stashed on the stack is going to end up unhappy.

I'm going to have to retract half this series.  (I'll follow up on the
0/$N with the longer term plan to remove this mess).

~Andrew