Implement CPU hotplug on Arm

Currently on Arm the desc->affinity mask of an irq is never updated, which makes it hard to know the actual affinity of an interrupt.

Fix this by updating the field in irq_set_affinity.

Signed-off-by Mykyta Poturai <[email protected]>

---
v4->v5:
* add locking

v3->v4:
* patch introduced
---

xen/arch/arm/gic-vgic.c |  2 ++
xen/arch/arm/irq.c      |  9 +++++++--
xen/arch/arm/vgic.c     | 14 ++++++++++++--
3 files changed, 21 insertions(+), 4 deletions(-)

xen/arch/arm/gic-vgic.c +2 -0

index ea48c5375a..5253caf002 100644
@@ -232,7 +232,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
            if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
            {
                struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
+                spin_lock(&p->desc->lock);
                irq_set_affinity(p->desc, cpumask_of(v_target->processor));
+                spin_unlock(&p->desc->lock);
                clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
            }
        }

xen/arch/arm/irq.c +7 -2

index 73e58a5108..7204bc2b68 100644
@@ -216,10 +216,15 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
    return irq_get_guest_info(desc)->d;
 }
 
+/* Must be called with desc->lock held */
 void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
 {
-    if ( desc != NULL )
-        desc->handler->set_affinity(desc, mask);
+    if ( desc == NULL )
+        return;
+
+    ASSERT(spin_is_locked(&desc->lock));
+    cpumask_copy(desc->affinity, mask);
+    desc->handler->set_affinity(desc, mask);
 }
 
 int request_irq(unsigned int irq, unsigned int irqflags,

xen/arch/arm/vgic.c +12 -2

index 6647071ad4..c59f6873db 100644
@@ -445,7 +445,9 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 
    if ( list_empty(&p->inflight) )
    {
+        spin_lock(&p->desc->lock);
        irq_set_affinity(p->desc, cpumask_of(new->processor));
+        spin_unlock(&p->desc->lock);
        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
        return true;
    }

@@ -453,7 +455,9 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
    if ( !list_empty(&p->lr_queue) )
    {
        vgic_remove_irq_from_queues(old, p);
+        spin_lock(&p->desc->lock);
        irq_set_affinity(p->desc, cpumask_of(new->processor));
+        spin_unlock(&p->desc->lock);
        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
        vgic_inject_irq(new->domain, new, irq, true);
        return true;

@@ -473,6 +477,7 @@ void arch_move_irqs(struct vcpu *v)
    struct domain *d = v->domain;
    struct pending_irq *p;
    struct vcpu *v_target;
+    unsigned long flags;
    int i;
 
    /*

@@ -494,7 +499,13 @@ void arch_move_irqs(struct vcpu *v)
        p = irq_to_pending(v_target, virq);
 
        if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+        {
+            if ( !p->desc )
+                continue;
+            spin_lock_irqsave(&p->desc->lock, flags);
            irq_set_affinity(p->desc, cpu_mask);
+            spin_unlock_irqrestore(&p->desc->lock, flags);
+        }
    }
 }
 

@@ -574,8 +585,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n)
        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
        if ( p->desc != NULL )
        {
-            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
            spin_lock_irqsave(&p->desc->lock, flags);
+            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
            /*
             * The irq cannot be a PPI, we only support delivery of SPIs
             * to guests.

@@ -944,4 +955,3 @@ void vgic_check_inflight_irqs_pending(struct vcpu *v, unsigned int rank, uint32_
 * indent-tabs-mode: nil
 * End:
 */
-

Signature · 1 line

2.51.2

Move IRQs from dying CPU to the online ones when a CPU is getting offlined. When onlining, rebalance all IRQs in a round-robin fashion. Guest-bound IRQs are already handled by scheduler in the process of moving vCPUs to active pCPUs, so we only need to handle IRQs used by Xen itself.

Signed-off-by Mykyta Poturai <[email protected]>

---
v4->v5:
* handle CPU onlining as well
* more comments
* fix crash when ESPI is disabled
* don't assume CPU 0 is a boot CPU
* use insigned int for irq number
* remove assumption that all irqs a bound to CPU 0 by default from the commit message

v3->v4:
* patch introduced
---

xen/arch/arm/include/asm/irq.h |  2 ++
xen/arch/arm/irq.c             | 54 ++++++++++++++++++++++++++++++++++
xen/arch/arm/smpboot.c         |  6 ++++
3 files changed, 62 insertions(+)

xen/arch/arm/include/asm/irq.h +2 -0

index 09788dbfeb..a0250bac85 100644
@@ -126,6 +126,8 @@ bool irq_type_set_by_domain(const struct domain *d);
 void irq_end_none(struct irq_desc *irq);
 #define irq_end_none irq_end_none
 
+void rebalance_irqs(unsigned int from, bool up);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
 * Local variables:

xen/arch/arm/irq.c +18 -0

index 7204bc2b68..a32dc729f8 100644
@@ -158,6 +158,58 @@ static int init_local_irq_data(unsigned int cpu)
    return 0;
 }
 
+static int cpu_next;
+
+static void balance_irq(int irq, unsigned int from, bool up)
+{
+    struct irq_desc *desc = irq_to_desc(irq);
+    unsigned long flags;
+
+    ASSERT(!cpumask_empty(&cpu_online_map));
+
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( likely(!desc->action) )
+        goto out;
+
+    if ( likely(test_bit(_IRQ_GUEST, &desc->status) ||
+                test_bit(_IRQ_MOVE_PENDING, &desc->status)) )
+        goto out;
+
+    /*

xen/arch/arm/irq.c +26 -0

+
+    cpu_next = cpumask_cycle(cpu_next, &cpu_online_map);
+    irq_set_affinity(desc, cpumask_of(cpu_next));
+
+out:
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
+void rebalance_irqs(unsigned int from, bool up)
+{
+    int irq;
+
+    if ( cpumask_empty(&cpu_online_map) )
+        return;
+
+    for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ )
+        balance_irq(irq, from, up);
+
+#ifdef CONFIG_GICV3_ESPI
+    for ( irq = ESPI_BASE_INTID; irq < ESPI_MAX_INTID; irq++ )
+        balance_irq(irq, from, up);
+#endif
+}
+
 static int cpu_callback(struct notifier_block *nfb, unsigned long action,
                        void *hcpu)
 {

@@ -172,6 +224,8 @@ static int cpu_callback(struct notifier_block *nfb, unsigned long action,
            printk(XENLOG_ERR "Unable to allocate local IRQ for CPU%u\n",
                   cpu);
        break;
+    case CPU_ONLINE:
+        rebalance_irqs(cpu, true);
    }
 
    return notifier_from_errno(rc);

xen/arch/arm/smpboot.c +0 -0

index 7f3cfa812e..e1b9f94458 100644

xen/arch/arm/smpboot.c +0 -0

    /* Return to caller; eventually the IPI mechanism will unwind and the 
     * scheduler will drop to the idle loop, which will call stop_cpu(). */
 }

Signature · 1 line

2.51.2

Hi Mykyta.

Quoted 75 lines · Mykyta Poturai

Move IRQs from dying CPU to the online ones when a CPU is getting offlined. When onlining, rebalance all IRQs in a round-robin fashion. Guest-bound IRQs are already handled by scheduler in the process of moving vCPUs to active pCPUs, so we only need to handle IRQs used by Xen itself.

Signed-off-by Mykyta Poturai <[email protected]>

---
v4->v5:
* handle CPU onlining as well
* more comments
* fix crash when ESPI is disabled
* don't assume CPU 0 is a boot CPU
* use insigned int for irq number
* remove assumption that all irqs a bound to CPU 0 by default from the commit message

v3->v4:
* patch introduced
---

xen/arch/arm/include/asm/irq.h |  2 ++
xen/arch/arm/irq.c             | 54 ++++++++++++++++++++++++++++++++++
xen/arch/arm/smpboot.c         |  6 ++++
3 files changed, 62 insertions(+)

xen/arch/arm/include/asm/irq.h +2 -0

index 09788dbfeb..a0250bac85 100644
@@ -126,6 +126,8 @@ bool irq_type_set_by_domain(const struct domain *d);
 void irq_end_none(struct irq_desc *irq);
 #define irq_end_none irq_end_none
 
+void rebalance_irqs(unsigned int from, bool up);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:

xen/arch/arm/irq.c +27 -0

index 7204bc2b68..a32dc729f8 100644
@@ -158,6 +158,58 @@ static int init_local_irq_data(unsigned int cpu)
     return 0;
 }
 
+static int cpu_next;
+
+static void balance_irq(int irq, unsigned int from, bool up)
+{
+    struct irq_desc *desc = irq_to_desc(irq);
+    unsigned long flags;
+
+    ASSERT(!cpumask_empty(&cpu_online_map));
+
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( likely(!desc->action) )
+        goto out;
+
+    if ( likely(test_bit(_IRQ_GUEST, &desc->status) ||
+                test_bit(_IRQ_MOVE_PENDING, &desc->status)) )
+        goto out;
+
+    /*
+     * Setting affinity to a mask of multiple CPUs causes the GIC drivers to
+     * select one CPU from that mask. If the dying CPU was included in the IRQ's
+     * affinity mask, we cannot determine exactly which CPU the interrupt is
+     * currently routed to, as GIC drivers lack a concrete get_affinity API. So
+     * to be safe we must reroute it to a new, definitely online, CPU. In the
+     * case of CPU going down, we move only the interrupt that could reside on
+     * it. Otherwise, we rearrange all interrupts in a round-robin fashion.
+     */
+    if ( !up && !cpumask_test_cpu(from, desc->affinity) )

xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 188

Quoted 1 line

xen/arch/arm/irq.c +1 -0

+        goto out;

I am a bit lost here on what you are trying to do in the case where a cpu is coming up here, it feels like you are trying to change the affinity of all interrupts in this case to cycle everything. Is it really what is expected ?
If affinity was set by a VM on its interrupts, I would not expect Xen to round-robin everything each time a cpu comes up.

Quoted 44 lines

xen/arch/arm/irq.c +26 -0

+
+    cpu_next = cpumask_cycle(cpu_next, &cpu_online_map);
+    irq_set_affinity(desc, cpumask_of(cpu_next));
+
+out:
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
+void rebalance_irqs(unsigned int from, bool up)
+{
+    int irq;
+
+    if ( cpumask_empty(&cpu_online_map) )
+        return;
+
+    for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ )
+        balance_irq(irq, from, up);
+
+#ifdef CONFIG_GICV3_ESPI
+    for ( irq = ESPI_BASE_INTID; irq < ESPI_MAX_INTID; irq++ )
+        balance_irq(irq, from, up);
+#endif
+}
+
 static int cpu_callback(struct notifier_block *nfb, unsigned long action,
                         void *hcpu)
 {

@@ -172,6 +224,8 @@ static int cpu_callback(struct notifier_block *nfb, unsigned long action,
             printk(XENLOG_ERR "Unable to allocate local IRQ for CPU%u\n",
                    cpu);
         break;
+    case CPU_ONLINE:
+        rebalance_irqs(cpu, true);
     }
 
     return notifier_from_errno(rc);

xen/arch/arm/smpboot.c +0 -0

index 7f3cfa812e..e1b9f94458 100644
@@ -425,6 +425,12 @@ void __cpu_disable(void)
 
     smp_mb();
 

xen/arch/arm/smpboot.c void __cpu_disable(void) · line 433

Quoted 6 lines

xen/arch/arm/smpboot.c +6 -0

+    /*
+     * Now that the interrupts are cleared and the CPU marked as offline,
+     * move interrupts out of it
+     */
+    rebalance_irqs(cpu, false);
+

I would expect this to only be useful when HOTPLUG is enabled, maybe we could have a static inline doing nothing when HOTPLUG is not on and only do something if HOTPLUG is enabled here ?

Cheers
Bertrand

Quoted 5 lines

xen/arch/arm/smpboot.c +0 -0

     /* Return to caller; eventually the IPI mechanism will unwind and the
      * scheduler will drop to the idle loop, which will call stop_cpu(). */
 }

Signature · 1 line

2.51.2

Quoted 77 lines · Bertrand Marquis

Hi Mykyta.

Quoted 75 lines · Mykyta Poturai

Move IRQs from dying CPU to the online ones when a CPU is getting offlined. When onlining, rebalance all IRQs in a round-robin fashion. Guest-bound IRQs are already handled by scheduler in the process of moving vCPUs to active pCPUs, so we only need to handle IRQs used by Xen itself.

Signed-off-by Mykyta Poturai <[email protected]>

---
v4->v5:
* handle CPU onlining as well
* more comments
* fix crash when ESPI is disabled
* don't assume CPU 0 is a boot CPU
* use insigned int for irq number
* remove assumption that all irqs a bound to CPU 0 by default from the commit message

v3->v4:
* patch introduced
---

xen/arch/arm/include/asm/irq.h |  2 ++
xen/arch/arm/irq.c             | 54 ++++++++++++++++++++++++++++++++++
xen/arch/arm/smpboot.c         |  6 ++++
3 files changed, 62 insertions(+)

xen/arch/arm/include/asm/irq.h +2 -0

index 09788dbfeb..a0250bac85 100644
@@ -126,6 +126,8 @@ bool irq_type_set_by_domain(const struct domain *d);
 void irq_end_none(struct irq_desc *irq);
 #define irq_end_none irq_end_none
 
+void rebalance_irqs(unsigned int from, bool up);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
 * Local variables:

xen/arch/arm/irq.c +27 -0

index 7204bc2b68..a32dc729f8 100644
@@ -158,6 +158,58 @@ static int init_local_irq_data(unsigned int cpu)
    return 0;
 }
 
+static int cpu_next;
+
+static void balance_irq(int irq, unsigned int from, bool up)
+{
+    struct irq_desc *desc = irq_to_desc(irq);
+    unsigned long flags;
+
+    ASSERT(!cpumask_empty(&cpu_online_map));
+
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( likely(!desc->action) )
+        goto out;
+
+    if ( likely(test_bit(_IRQ_GUEST, &desc->status) ||
+                test_bit(_IRQ_MOVE_PENDING, &desc->status)) )
+        goto out;
+
+    /*
+     * Setting affinity to a mask of multiple CPUs causes the GIC drivers to
+     * select one CPU from that mask. If the dying CPU was included in the IRQ's
+     * affinity mask, we cannot determine exactly which CPU the interrupt is
+     * currently routed to, as GIC drivers lack a concrete get_affinity API. So
+     * to be safe we must reroute it to a new, definitely online, CPU. In the
+     * case of CPU going down, we move only the interrupt that could reside on
+     * it. Otherwise, we rearrange all interrupts in a round-robin fashion.
+     */
+    if ( !up && !cpumask_test_cpu(from, desc->affinity) )

Quoted 9 lines

xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 188

Quoted 1 line

xen/arch/arm/irq.c +1 -0

+        goto out;

I am a bit lost here on what you are trying to do in the case where a cpu is coming up here, it feels like you are trying to change the affinity of all interrupts in this case to cycle everything. Is it really what is expected ?
If affinity was set by a VM on its interrupts, I would not expect Xen to round-robin everything each time a cpu comes up.

The idea is to evenly spread interrupts between CPUs when the new ones are being brought online. This is needed to prevent Xen-bound IRQs from piling up on CPU 0 when other cores are being offlined and then onlined back. It shouldn’t mess with guest affinities, as the code skips everything that is assigned to guests and leaves it to be handled by the scheduler/vgic. Performance-wise, it should also be okay, as from what I’ve seen, there are not many interrupts used by Xen, and I expect CPU hotplug operations to be fairly infrequent.

Quoted 45 lines

Quoted 45 lines

xen/arch/arm/irq.c +26 -0

+
+    cpu_next = cpumask_cycle(cpu_next, &cpu_online_map);
+    irq_set_affinity(desc, cpumask_of(cpu_next));
+
+out:
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
+void rebalance_irqs(unsigned int from, bool up)
+{
+    int irq;
+
+    if ( cpumask_empty(&cpu_online_map) )
+        return;
+
+    for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ )
+        balance_irq(irq, from, up);
+
+#ifdef CONFIG_GICV3_ESPI
+    for ( irq = ESPI_BASE_INTID; irq < ESPI_MAX_INTID; irq++ )
+        balance_irq(irq, from, up);
+#endif
+}
+
 static int cpu_callback(struct notifier_block *nfb, unsigned long action,
                        void *hcpu)
 {

@@ -172,6 +224,8 @@ static int cpu_callback(struct notifier_block *nfb, unsigned long action,
            printk(XENLOG_ERR "Unable to allocate local IRQ for CPU%u\n",
                   cpu);
        break;
+    case CPU_ONLINE:
+        rebalance_irqs(cpu, true);
    }
 
    return notifier_from_errno(rc);

xen/arch/arm/smpboot.c +0 -0

index 7f3cfa812e..e1b9f94458 100644
@@ -425,6 +425,12 @@ void __cpu_disable(void)
 
    smp_mb();
 

Quoted 11 lines

xen/arch/arm/smpboot.c void __cpu_disable(void) · line 433

Quoted 6 lines

xen/arch/arm/smpboot.c +6 -0

+    /*
+     * Now that the interrupts are cleared and the CPU marked as offline,
+     * move interrupts out of it
+     */
+    rebalance_irqs(cpu, false);
+

I would expect this to only be useful when HOTPLUG is enabled, maybe we could have a static inline doing nothing when HOTPLUG is not on and only do something if HOTPLUG is enabled here ?

Yes I will add this in the next version.

Quoted 9 lines

Cheers
Bertrand

Quoted 5 lines

xen/arch/arm/smpboot.c +0 -0

    /* Return to caller; eventually the IPI mechanism will unwind and the
     * scheduler will drop to the idle loop, which will call stop_cpu(). */
 }

Signature · 1 line

2.51.2

Signature · 1 line

Mykyta

Quoted 90 lines · Mykyta Poturai

Quoted 77 lines · Bertrand Marquis

Hi Mykyta.

Quoted 75 lines · Mykyta Poturai

Move IRQs from dying CPU to the online ones when a CPU is getting offlined. When onlining, rebalance all IRQs in a round-robin fashion. Guest-bound IRQs are already handled by scheduler in the process of moving vCPUs to active pCPUs, so we only need to handle IRQs used by Xen itself.

Signed-off-by Mykyta Poturai <[email protected]>

---
v4->v5:
* handle CPU onlining as well
* more comments
* fix crash when ESPI is disabled
* don't assume CPU 0 is a boot CPU
* use insigned int for irq number
* remove assumption that all irqs a bound to CPU 0 by default from the commit message

v3->v4:
* patch introduced
---

xen/arch/arm/include/asm/irq.h |  2 ++
xen/arch/arm/irq.c             | 54 ++++++++++++++++++++++++++++++++++
xen/arch/arm/smpboot.c         |  6 ++++
3 files changed, 62 insertions(+)

xen/arch/arm/include/asm/irq.h +2 -0

index 09788dbfeb..a0250bac85 100644
@@ -126,6 +126,8 @@ bool irq_type_set_by_domain(const struct domain *d);
 void irq_end_none(struct irq_desc *irq);
 #define irq_end_none irq_end_none
 
+void rebalance_irqs(unsigned int from, bool up);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
 * Local variables:

xen/arch/arm/irq.c +27 -0

index 7204bc2b68..a32dc729f8 100644
@@ -158,6 +158,58 @@ static int init_local_irq_data(unsigned int cpu)
    return 0;
 }
 
+static int cpu_next;
+
+static void balance_irq(int irq, unsigned int from, bool up)
+{
+    struct irq_desc *desc = irq_to_desc(irq);
+    unsigned long flags;
+
+    ASSERT(!cpumask_empty(&cpu_online_map));
+
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( likely(!desc->action) )
+        goto out;
+
+    if ( likely(test_bit(_IRQ_GUEST, &desc->status) ||
+                test_bit(_IRQ_MOVE_PENDING, &desc->status)) )
+        goto out;
+
+    /*
+     * Setting affinity to a mask of multiple CPUs causes the GIC drivers to
+     * select one CPU from that mask. If the dying CPU was included in the IRQ's
+     * affinity mask, we cannot determine exactly which CPU the interrupt is
+     * currently routed to, as GIC drivers lack a concrete get_affinity API. So
+     * to be safe we must reroute it to a new, definitely online, CPU. In the
+     * case of CPU going down, we move only the interrupt that could reside on
+     * it. Otherwise, we rearrange all interrupts in a round-robin fashion.
+     */
+    if ( !up && !cpumask_test_cpu(from, desc->affinity) )

Quoted 9 lines

xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 188

Quoted 1 line

xen/arch/arm/irq.c +1 -0

+        goto out;

I am a bit lost here on what you are trying to do in the case where a cpu is coming up here, it feels like you are trying to change the affinity of all interrupts in this case to cycle everything. Is it really what is expected ?
If affinity was set by a VM on its interrupts, I would not expect Xen to round-robin everything each time a cpu comes up.

Quoted 8 lines

The idea is to evenly spread interrupts between CPUs when the new ones are being brought online. This is needed to prevent Xen-bound IRQs from piling up on CPU 0 when other cores are being offlined and then onlined back. It shouldn’t mess with guest affinities, as the code skips everything that is assigned to guests and leaves it to be handled by the scheduler/vgic. Performance-wise, it should also be okay, as from what I’ve seen, there are not many interrupts used by Xen, and I expect CPU hotplug operations to be fairly infrequent.

My fear here is a bit that by removing and adding a cpu we will completely change irq affinities. I am not so sure that those kind of random assignments are compatible with embedded or safety use cases and here there is no way to configure this.

@Julien, Stefano and Michal: What do you think here ?

Move XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} handlers to common code to allow for enabling/disabling CPU cores in runtime on Arm64.

SMT-disable enforcement check is moved into a separate architecture-specific function.

For now this operations only support Arm64. For proper Arm32 support, there needs to be a mechanism to free per-cpu page tables, allocated in init_domheap_mappings. Also, hotplug is not supported if ITS, FFA, or TEE is enabled, as they use non-static IRQ actions.

xen/common/Kconfig +4 -0

+    {
+        ret = cpu_down_helper(data);
+        if ( ret )
+            printk("Could not re-offline CPU%u (%d)\n", cpu, ret);

xen/common/sysctl.c +17 -0

+    {
+        unsigned int cpu = op->u.cpu_hotplug.cpu;
+        unsigned int hp_op = op->u.cpu_hotplug.op;
+        bool plug;
+        long (*fn)(void *data);
+        void *hcpu;
+
+        switch ( hp_op )
+        {
+        case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
+            plug = true;
+            fn = cpu_up_helper;
+            hcpu = _p(cpu);
+            break;
+
+        case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
+            plug = false;

xen/common/sysctl.c +4 -0

+        }
+
+        if ( !ret )
+            ret = plug ? xsm_resource_plug_core(XSM_HOOK)

Quoted 10 lines · Mykyta Poturai

Move XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} handlers to common code to allow for enabling/disabling CPU cores in runtime on Arm64.

SMT-disable enforcement check is moved into a separate architecture-specific function.

For now this operations only support Arm64. For proper Arm32 support, there needs to be a mechanism to free per-cpu page tables, allocated in init_domheap_mappings. Also, hotplug is not supported if ITS, FFA, or TEE is enabled, as they use non-static IRQ actions.

For all of these "not supported" cases, what if a user nevertheless tries? Wouldn't the request better be outright denied, rather leaving the system in a questionable state? Hmm, I see you ...

xen/arch/arm/Kconfig config ARM_64 · line 10

Quoted 7 lines

xen/arch/arm/Kconfig +1 -0

@@ -7,6 +7,7 @@ config ARM_64
  	def_bool y
  	depends on !ARM_32
  	select 64BIT
+	select CPU_HOTPLUG if !TEE && !FFA && !HAS_ITS

... make the select conditional. But do TEE, FFA, and HAS_ITS each mean the feature is actually in use when the hypervisor runs?

Quoted 5 lines

xen/arch/arm/Kconfig +5 -0

+    int ret = cpu_up(cpu);
+
+    /* Have one more go on EBUSY. */
+    if ( ret == -EBUSY )
+        ret = cpu_up(cpu);

xen/arch/arm/Kconfig

Quoted 2 lines

xen/arch/arm/Kconfig +2 -0

+
+    if ( !ret && arch_smt_cpu_disable(cpu) )

As you validly note in a comment in do_sysctl(), SMT is an arch-specific concept and perhaps even an arch-specific term. Hence using it in the name of an arch hook feels inappropriate. Plus - the hook could be used for other purposes. What the arch needs to indicate is whether the CPU that was brought up may actually stay online. That more generic purpose is what imo the name wants to cover.

xen/arch/arm/Kconfig

Quoted 6 lines

xen/arch/arm/Kconfig +6 -0

+        case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
+        case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
+            /* Use arch specific handlers as SMT is very arch-dependent */
+            ret = arch_do_sysctl(op, u_sysctl);
+            copyback = 0;
+            goto out;

I wonder if it wouldn't be neater for this and actually also ...

xen/arch/arm/Kconfig

Quoted 3 lines

xen/arch/arm/Kconfig +3 -0

+        default:
+            ret = -EOPNOTSUPP;
+            break;

... this to fall through to ...

Quoted 4 lines

xen/arch/arm/Kconfig +4 -0

+        }
+
+        if ( !ret )
+            ret = plug ? xsm_resource_plug_core(XSM_HOOK)

xen/arch/arm/Kconfig

Quoted 10 lines

xen/arch/arm/Kconfig +8 -0

+                       : xsm_resource_unplug_core(XSM_HOOK);
+
+        if ( !ret )
+            ret = continue_hypercall_on_cpu(0, fn, hcpu);
+        break;
+    }
+#endif
+
      default:
          ret = arch_do_sysctl(op, u_sysctl);

... here. (Minimally the earlier default case wants uniformly forwarding to the arch handler, or else arch-specific additions would always require adjustment here.)

Jan

Quoted 16 lines · Jan Beulich

Quoted 10 lines · Mykyta Poturai

Move XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} handlers to common code to allow for enabling/disabling CPU cores in runtime on Arm64.

SMT-disable enforcement check is moved into a separate architecture-specific function.

For now this operations only support Arm64. For proper Arm32 support, there needs to be a mechanism to free per-cpu page tables, allocated in init_domheap_mappings. Also, hotplug is not supported if ITS, FFA, or TEE is enabled, as they use non-static IRQ actions.

For all of these "not supported" cases, what if a user nevertheless tries? Wouldn't the request better be outright denied, rather leaving the system in a questionable state? Hmm, I see you ...

Quoted 11 lines

xen/arch/arm/Kconfig config ARM_64 · line 10

Quoted 7 lines

xen/arch/arm/Kconfig +1 -0

@@ -7,6 +7,7 @@ config ARM_64
  	def_bool y
  	depends on !ARM_32
  	select 64BIT
+	select CPU_HOTPLUG if !TEE && !FFA && !HAS_ITS

... make the select conditional. But do TEE, FFA, and HAS_ITS each mean the feature is actually in use when the hypervisor runs?

The way interrupts are requested in these modules causes Xen to crash when trying to offline a cpu. It’s a fairly simple fix and I plan to send them eventually. I’ve decided to omit them now and do these fixes only for supported code to keep the series from ballooning with too many patches.

Quoted 5 lines

Quoted 5 lines

xen/arch/arm/Kconfig +5 -0

+    int ret = cpu_up(cpu);
+
+    /* Have one more go on EBUSY. */
+    if ( ret == -EBUSY )
+        ret = cpu_up(cpu);

Quoted 8 lines

xen/arch/arm/Kconfig

Quoted 2 lines

xen/arch/arm/Kconfig +2 -0

+
+    if ( !ret && arch_smt_cpu_disable(cpu) )

As you validly note in a comment in do_sysctl(), SMT is an arch-specific concept and perhaps even an arch-specific term. Hence using it in the name of an arch hook feels inappropriate. Plus - the hook could be used for other purposes. What the arch needs to indicate is whether the CPU that was brought up may actually stay online. That more generic purpose is what imo the name wants to cover.

Would arch_cpu_online_allowed() be okay, or maybe you have something more specific in mind?

Quoted 25 lines

xen/arch/arm/Kconfig

Quoted 6 lines

xen/arch/arm/Kconfig +6 -0

+        case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
+        case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
+            /* Use arch specific handlers as SMT is very arch-dependent */
+            ret = arch_do_sysctl(op, u_sysctl);
+            copyback = 0;
+            goto out;

I wonder if it wouldn't be neater for this and actually also ...

xen/arch/arm/Kconfig

Quoted 3 lines

xen/arch/arm/Kconfig +3 -0

+        default:
+            ret = -EOPNOTSUPP;
+            break;

... this to fall through to ...

Quoted 10 lines

xen/arch/arm/Kconfig +10 -0

+        }
+
+        if ( !ret )
+            ret = plug ? xsm_resource_plug_core(XSM_HOOK)
+                       : xsm_resource_unplug_core(XSM_HOOK);
+
+        if ( !ret )
+            ret = continue_hypercall_on_cpu(0, fn, hcpu);
+        break;
+    }

Quoted 8 lines

xen/arch/arm/Kconfig

Quoted 4 lines

xen/arch/arm/Kconfig +2 -0

+#endif
+
      default:
          ret = arch_do_sysctl(op, u_sysctl);

... here. (Minimally the earlier default case wants uniformly forwarding to the arch handler, or else arch-specific additions would always require adjustment here.)

Would it be okay to mix goto and switch like this, or is it too convoluted?

         case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
             plug = false;
             fn = cpu_down_helper;
             hcpu = _p(cpu);
             break;

         default:
             goto outer_default;
         }

         if ( !ret )
             ret = plug ? xsm_resource_plug_core(XSM_HOOK)
                        : xsm_resource_unplug_core(XSM_HOOK);

         if ( !ret )
             ret = continue_hypercall_on_cpu(0, fn, hcpu);
         break;

     }

#endif

default:
outer_default:

         ret = arch_do_sysctl(op, u_sysctl);
         copyback = 0;
         break;
     }

tools/misc/Makefile +0 -1

@@ -16,7 +16,6 @@ INSTALL_BIN                    += xencov_split
 INSTALL_BIN += $(INSTALL_BIN-y)
 
 # Everything to be installed in regular sbin/
-INSTALL_SBIN-$(CONFIG_MIGRATE) += xen-hptool
 INSTALL_SBIN-$(CONFIG_X86)     += xen-hvmcrash
 INSTALL_SBIN-$(CONFIG_X86)     += xen-hvmctx
 INSTALL_SBIN-$(CONFIG_X86)     += xen-lowmemd

This series implements support for CPU hotplug/unplug on Arm. To achieve this, several things need to be done:

1. XEN_SYSCTL_CPU_HOTPLUG_* calls implemented on Arm64.
2. Enabled building of xen-hptool.
3. Migration of irqs from dying CPUs implemented.

Tested on QEMU.

v4->v5:
* drop merged patches
* combine "smp: Move cpu_up/down helpers to common code" with "arm/sysctl: Implement cpu hotplug ops"
* see individual patches

v3->v4:
* add irq migration patches
* see individual patches

v2->v3:
* add docs

v1->v2:
* see individual patches

Mykyta Poturai (5):
arm/irq: Keep track of irq affinities
arm/irq: Migrate IRQs during CPU up/down operations arm/sysctl: Implement cpu hotplug ops
tools: Allow building xen-hptool without CONFIG_MIGRATE
docs: Document CPU hotplug

SUPPORT.md                       |  1 +
docs/misc/cpu-hotplug.txt        | 50 +++++++++++++++++++++++++
tools/libs/guest/Makefile.common |  2 +-
tools/misc/Makefile              |  2 +-
xen/arch/arm/Kconfig             |  1 +
xen/arch/arm/gic-vgic.c          |  2 +
xen/arch/arm/include/asm/irq.h   |  2 +
xen/arch/arm/irq.c               | 63 +++++++++++++++++++++++++++++++-
xen/arch/arm/smp.c               |  9 +++++
xen/arch/arm/smpboot.c           |  6 +++
xen/arch/arm/vgic.c              | 14 ++++++-
xen/arch/ppc/stubs.c             |  4 ++
xen/arch/riscv/stubs.c           |  5 +++
xen/arch/x86/Kconfig             |  1 +
xen/arch/x86/include/asm/smp.h   |  3 --
xen/arch/x86/smp.c               | 33 ++---------------
xen/arch/x86/sysctl.c            | 12 ++----
xen/common/Kconfig               |  3 ++
xen/common/smp.c                 | 34 +++++++++++++++++
xen/common/sysctl.c              | 45 +++++++++++++++++++++++
xen/include/xen/smp.h            |  4 ++
21 files changed, 248 insertions(+), 48 deletions(-)

create mode 100644 docs/misc/cpu-hotplug.txt

Signature · 1 line

2.51.2

Hi Mykyta,

Quoted 10 lines · Mykyta Poturai

This series implements support for CPU hotplug/unplug on Arm. To achieve this, several things need to be done:

1. XEN_SYSCTL_CPU_HOTPLUG_* calls implemented on Arm64.
2. Enabled building of xen-hptool.
3. Migration of irqs from dying CPUs implemented.

Tested on QEMU.

Do you have a way to validate this on real hardware ?
Doing hotplug/unplug might have side effects (caches, coherency, interrupts, etc) that I would not be completely confident to have validated by only testing this in qemu.

Could you explain how you validated this (what kind of tests you did).

I would be interested to know how the system behave in the following cases:
- stopping cpu with guests pinned to specific cores
- stopping a cpu with a guest pinned and only running on that one
- checking with passthrough devices (for irq balancing)

Then a general remark i have is that most of this code (if not all) is always in Xen even if CONFIG_CPU_HOTPLUG is not activated in xen config which will probably generate some dead code. Could we protect some of the code not used when HOTPLUG is not activated with if (is_enabled(CONFIG_CPU_HOTPLUG)) or equivalent forms ?

Cheers
Bertrand

Quoted 24 lines · Bertrand Marquis

Hi Mykyta,

Quoted 10 lines · Mykyta Poturai

This series implements support for CPU hotplug/unplug on Arm. To achieve this, several things need to be done:

1. XEN_SYSCTL_CPU_HOTPLUG_* calls implemented on Arm64.
2. Enabled building of xen-hptool.
3. Migration of irqs from dying CPUs implemented.

Tested on QEMU.

Do you have a way to validate this on real hardware ?
Doing hotplug/unplug might have side effects (caches, coherency, interrupts, etc) that I would not be completely confident to have validated by only testing this in qemu.

Could you explain how you validated this (what kind of tests you did).

I would be interested to know how the system behave in the following cases:
- stopping cpu with guests pinned to specific cores
- stopping a cpu with a guest pinned and only running on that one
- checking with passthrough devices (for irq balancing)

Quoted 9 lines

Then a general remark i have is that most of this code (if not all) is always in Xen even if CONFIG_CPU_HOTPLUG is not activated in xen config which will probably generate some dead code. Could we protect some of the code not used when HOTPLUG is not activated with if (is_enabled(CONFIG_CPU_HOTPLUG)) or equivalent forms ?

Cheers
Bertrand

Hi Bertrand

I’ve tested various pinning scenarios, and all of them work fine, as Xen forcibly unpins everything that was pinned to the offlined CPU. I’ve also explicitly checked that interrupts are getting moved to and from cores when needed.

I will update the next cover letter to explain testing better and also try to test on some hardware to be sure.

Can you please elaborate a little on what exactly you mean by “checking with passthrough devices”? I suspect I am not fully getting the idea.

Signature · 2 lines

-- 
Mykyta