Implement CPU hotplug on Arm
v4 cover Implement CPU hotplug on Arm
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.
2. timer and GIC maintenance interrupts switched to static irqactions to remove the need for freeing them during release_irq.
3. Enabled the build of xen-hptool on Arm.
4. Migration of irqs from dying CPUs implemented.
Tested on QEMU.
Note: As there are currently no Xen-used IRQs on non-zero CPUs, I used a hack that changed default affinity of IRQs in setup_irq to properly test IRQ migration. The hack consisted of changing smp_processor_id call to some hard-coded non-zero number.
v3->v4:
* add irq migration patches
* see individual patches
v2->v3:
* add docs
v1->v2:
* see individual patches
Mykyta Poturai (5):
arm/time: Use static irqaction
arm/gic: Use static irqaction
arm/irq: Keep track of irq affinities
arm/irq: Migrate IRQs from dyings CPUs
smp: Move cpu_up/down helpers to common code arm/sysctl: Implement cpu hotplug ops
tools: Allow building xen-hptool without CONFIG_MIGRATE
docs: Document CPU hotplug
config/Tools.mk.in | 1 + docs/misc/cpu-hotplug.txt | 51 ++++++++++++++++++++++++++++++++ tools/configure | 30 +++++++++++++++++++ tools/configure.ac | 1 + tools/libs/guest/Makefile.common | 4 +++ tools/misc/Makefile | 2 +- xen/arch/arm/Kconfig | 4 +++ xen/arch/arm/gic.c | 11 +++++-- xen/arch/arm/include/asm/irq.h | 2 ++ xen/arch/arm/irq.c | 42 ++++++++++++++++++++++++++ xen/arch/arm/smp.c | 6 ++++ xen/arch/arm/smpboot.c | 2 ++ xen/arch/arm/sysctl.c | 32 ++++++++++++++++++++ xen/arch/arm/time.c | 21 ++++++++++--- xen/arch/ppc/stubs.c | 4 +++ xen/arch/riscv/stubs.c | 5 ++++ xen/arch/x86/include/asm/smp.h | 3 -- xen/arch/x86/smp.c | 33 ++------------------- xen/common/smp.c | 32 ++++++++++++++++++++ xen/include/xen/smp.h | 4 +++ 20 files changed, 250 insertions(+), 40 deletions(-) create mode 100644 docs/misc/cpu-hotplug.txt mode change 100755 => 100644 tools/configure
Signature · 1 line
2.51.2
PATCH v4 1/8 arm/time: Use static irqaction
When stopping a core deinit_timer_interrupt is called in non-alloc context, which causes xfree in release_irq to fail an assert.
To fix this, switch to a statically allocated irqaction that does not need to be freed in release_irq.
v3->v4:
* make irqactions static
* collect RBs
v2->v3:
* no changes
v1->v2:
* use percpu actions
--- xen/arch/arm/time.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
xen/arch/arm/time.c +17 -4
@@ -303,9 +303,15 @@ static void check_timer_irq_cfg(unsigned int irq, const char *which) "WARNING: %s-timer IRQ%u is not level triggered.\n", which, irq); } +static DEFINE_PER_CPU_READ_MOSTLY(struct irqaction, irq_hyp); +static DEFINE_PER_CPU_READ_MOSTLY(struct irqaction, irq_virt); +/* Set up the timer interrupt on this CPU */ void init_timer_interrupt (void ) { +struct irqaction *hyp_action = &this_cpu (irq_hyp); +struct irqaction *virt_action = &this_cpu (irq_virt); +/* Sensible defaults */ WRITE_SYSREG64 (0 ,CNTVOFF_EL2 );/* No VM-specific offset */ /* Do not let the VMs program the physical timer, only read the physical counter */
@@ -314,10 +320,17 @@ void init_timer_interrupt(void)WRITE_SYSREG (0 ,CNTHP_CTL_EL2 );/* Hypervisor's timer disabled */ isb (); -request_irq (timer_irq[TIMER_HYP_PPI ],0 , htimer_interrupt, -"hyptimer" ,NULL ); -request_irq (timer_irq[TIMER_VIRT_PPI ],0 , vtimer_interrupt, -"virtimer" ,NULL ); + hyp_action->name ="hyptimer" ; + hyp_action->handler = htimer_interrupt; + hyp_action->dev_id =NULL ; + hyp_action->free_on_release =0 ; +setup_irq (timer_irq[TIMER_HYP_PPI ],0 , hyp_action); + + virt_action->name ="virtimer" ; + virt_action->handler = vtimer_interrupt; + virt_action->dev_id =NULL ; + virt_action->free_on_release =0 ; +setup_irq (timer_irq[TIMER_VIRT_PPI ],0 , virt_action);check_timer_irq_cfg (timer_irq[TIMER_HYP_PPI ],"hypervisor" );check_timer_irq_cfg (timer_irq[TIMER_VIRT_PPI ],"virtual" );
Signature · 1 line
2.51.2
PATCH v4 2/8 arm/gic: Use static irqaction
In reply to Mykyta PoturaiWhen stopping a core cpu_gic_callback is called in non-alloc context, which causes xfree in release_irq to fail an assert.
To fix this, switch to a statically allocated irqaction that does not need to be freed in release_irq.
v3->v4:
* make irqactions static
* collect RBs
v2->v3:
* no changes
v1->v2:
* use percpu actions
--- xen/arch/arm/gic.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
xen/arch/arm/gic.c +9 -2
@@ -386,10 +386,17 @@ void gic_dump_info(struct vcpu *v) gic_hw_ops->dump_state (v); } +static DEFINE_PER_CPU_READ_MOSTLY(struct irqaction, irq_maintenance); +void init_maintenance_interrupt (void ) { -request_irq (gic_hw_ops->info->maintenance_irq,0 , maintenance_interrupt, -"irq-maintenance" ,NULL ); +struct irqaction *maintenance = &this_cpu (irq_maintenance); + + maintenance->name ="irq-maintenance" ; + maintenance->handler = maintenance_interrupt; + maintenance->dev_id =NULL ; + maintenance->free_on_release =0 ; +setup_irq (gic_hw_ops->info->maintenance_irq,0 , maintenance); } int gic_make_hwdom_dt_node(const struct domain *d,
Signature · 1 line
2.51.2
PATCH v4 3/8 arm/irq: Keep track of irq affinities
In reply to Mykyta PoturaiCurrently 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.
v3->v4:
* patch introduced
--- xen/arch/arm/irq.c | 3 +++ 1 file changed, 3 insertions(+)
xen/arch/arm/irq.c +3 -0
@@ -219,7 +219,10 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)void irq_set_affinity (struct irq_desc *desc,const cpumask_t *mask) {if ( desc !=NULL ) + { +cpumask_copy (desc->affinity, mask); desc->handler->set_affinity (desc, mask); + } } int request_irq(unsigned int irq, unsigned int irqflags,
Signature · 1 line
2.51.2
Re: [PATCH v4 3/8] arm/irq: Keep track of irq affinities
Hi Mykyta,
Quoted 18 lines · Mykyta Poturai
Currently on Arm the
desc->affinitymask 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]>
v3->v4:
* patch introduced--- xen/arch/arm/irq.c | 3 +++ 1 file changed, 3 insertions(+)xen/arch/arm/irq.c +0
-0
xen/arch/arm/irq.c static inline struct domain *irq_get_domain(struct irq_desc *desc) · line 223
Quoted 5 lines
xen/arch/arm/irq.c +2
-0@@ -219,7 +219,10 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)void irq_set_affinity (struct irq_desc *desc,const cpumask_t *mask) {if ( desc !=NULL ) + { +cpumask_copy (desc->affinity, mask);
irq_set_affinity() doesn't always seems to be called with ``desc->lock`` taken (see vgic_migrate_irq(), arch_move_irqs()). So I am not sure this is safe. You probably need to update the callers as well to always take the lock. irq_set_affinity() probably want to gain an ASSERT(spin_is_locked()) with some documentation on top of the function.
xen/arch/arm/irq.c static inline struct domain *irq_get_domain(struct irq_desc *desc) · line 225
Quoted 2 lines
xen/arch/arm/irq.c +1
-0desc->handler->set_affinity (desc, mask); + }
NIT: As there are multiple lines in the ``if``, I would consider reworking the logic so we return early when desc is NULL.
xen/arch/arm/irq.c static inline struct domain *irq_get_domain(struct irq_desc *desc) · line 228
Quoted 3 lines
xen/arch/arm/irq.c +0
-0} int request_irq(unsigned int irq, unsigned int irqflags,
Cheers,
Signature · 1 line
Julien Grall
PATCH v4 4/8 arm/irq: Migrate IRQs from dyings CPUs
In reply to Mykyta PoturaiMove IRQs from dying CPU to the online ones.
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.
If IRQ is to be migrated, it's affinity is set to a mask of all online CPUs. With current GIC implementation, this means they are routed to a random online CPU. This may cause extra moves if multiple cores are disabled in sequence, but should prevent all interrupts from piling up on CPU0 in case of repeated up-down cycles on different cores.
IRQs from CPU 0 are never migrated, as dying CPU 0 means we are either shutting down compeletely or entering system suspend.
Considering that all Xen-used IRQs are currently allocated during init on CPU 0, and setup_irq uses smp_processor_id for the initial affinity. This change is not strictly required for correct operation for now, but it should future-proof cpu hotplug and system suspend support in case some kind if IRQ balancing is implemented later.
v3->v4:
* patch introduced
--- xen/arch/arm/include/asm/irq.h | 2 ++ xen/arch/arm/irq.c | 39 ++++++++++++++++++++++++++++++++++ xen/arch/arm/smpboot.c | 2 ++ 3 files changed, 43 insertions(+)
xen/arch/arm/include/asm/irq.h +2 -0
@@ -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 evacuate_irqs (unsigned int from); +#endif /* _ASM_HW_IRQ_H */ /* * Local variables:
xen/arch/arm/irq.c +39 -0
@@ -158,6 +158,45 @@ static int init_local_irq_data(unsigned int cpu)return 0 ; } +static void evacuate_irq (int irq,unsigned int from) +{ +struct irq_desc *desc =irq_to_desc (irq); +unsigned long flags; + +/* Don't move irqs from CPU 0 as it is always last to be disabled */ +if ( from ==0 ) +return ; + +ASSERT (!cpumask_empty (&cpu_online_map)); +ASSERT (!cpumask_test_cpu (from, &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; + +if (cpumask_test_cpu (from, desc->affinity) ) +irq_set_affinity (desc, &cpu_online_map); + +out: +spin_unlock_irqrestore (&desc->lock, flags); +return ; +} + +void evacuate_irqs (unsigned int from) +{ +int irq; + +for ( irq =NR_LOCAL_IRQS ; irq <NR_IRQS ; irq++ ) +evacuate_irq (irq, from); + +for ( irq =ESPI_BASE_INTID ; irq <ESPI_MAX_INTID ; irq++ ) +evacuate_irq (irq, from); +} +static int cpu_callback (struct notifier_block *nfb,unsigned long action,void *hcpu) {
xen/arch/arm/smpboot.c +2 -0
@@ -425,6 +425,8 @@ void __cpu_disable(void)smp_mb (); +evacuate_irqs (cpu); +/* 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
Re: [PATCH v4 4/8] arm/irq: Migrate IRQs from dyings CPUs
Hi,
Quoted 10 lines · Mykyta Poturai
Move IRQs from dying CPU to the online ones.
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.If IRQ is to be migrated, it's affinity is set to a mask of all online CPUs. With current GIC implementation, this means they are routed to a random online CPU. This may cause extra moves if multiple cores are disabled in sequence, but should prevent all interrupts from piling up on CPU0 in case of repeated up-down cycles on different cores.
Wouldn't they eventually all move to CPU0 in the case of suspend/resume or if all the CPUs but CPU0 are turned off and then off? If so, shouldn't we try to rebalance the interrupts?
Quoted 3 lines
IRQs from CPU 0 are never migrated, as dying CPU 0 means we are either shutting down compeletely or entering system suspend.
I can't find a place where __cpu_disable() is called on CPU0. Do you have any pointer? In any case, I am not sure I want to bake that assumption in more places of the code.
Quoted 3 lines
Considering that all Xen-used IRQs are currently allocated during init on CPU 0, and
setup_irqusessmp_processor_idfor the initial affinity.
Looking at the SMMU driver, we seems to request IRQs at the time the device is attached. So are you sure about this?
Quoted 32 lines
This change is not strictly required for correct operation for now, but it should future-proof cpu hotplug and system suspend support in case some kind if IRQ balancing is implemented later.
Signed-off-by Mykyta Poturai <[email protected]>
v3->v4:
* patch introduced--- xen/arch/arm/include/asm/irq.h | 2 ++ xen/arch/arm/irq.c | 39 ++++++++++++++++++++++++++++++++++ xen/arch/arm/smpboot.c | 2 ++ 3 files changed, 43 insertions(+)xen/arch/arm/include/asm/irq.h +2
-0@@ -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 evacuate_irqs (unsigned int from); +#endif /* _ASM_HW_IRQ_H */ /* * Local variables: xen/arch/arm/irq.c +0
-0
xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 161
Quoted 4 lines
xen/arch/arm/irq.c +1
-0@@ -158,6 +158,45 @@ static int init_local_irq_data(unsigned int cpu)return 0 ; } +static void evacuate_irq (int irq,unsigned int from)
Any reason why the 'irq' is signed?
xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 166
Quoted 5 lines
xen/arch/arm/irq.c +5
-0+{ +struct irq_desc *desc =irq_to_desc (irq); +unsigned long flags; + +/* Don't move irqs from CPU 0 as it is always last to be disabled */
Per above, I am not convinced that we should special case CPU 0. But if we do, then shouldn't this be part of evacuate_irqs() so we don't pointlessly go through all the IRQs?
Quoted 6 lines
xen/arch/arm/irq.c +6
-0+if ( from ==0 ) +return ; + +ASSERT (!cpumask_empty (&cpu_online_map)); +ASSERT (!cpumask_test_cpu (from, &cpu_online_map)); +
xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 182
Quoted 10 lines
xen/arch/arm/irq.c +10
-0+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; + +if (cpumask_test_cpu (from, desc->affinity) ) +irq_set_affinity (desc, &cpu_online_map);
I think it would be worth explaining why we are routing to any CPU online rather than checking whether the affinity has other online CPUs.
Just to note, I don't have strong opinion either way. It mainly needs to be documented.
xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 196
Quoted 13 lines
xen/arch/arm/irq.c +13
-0+ +out: +spin_unlock_irqrestore (&desc->lock, flags); +return ; +} + +void evacuate_irqs (unsigned int from) +{ +int irq; +> + for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) +evacuate_irq (irq, from); + +for ( irq =ESPI_BASE_INTID ; irq <ESPI_MAX_INTID ; irq++ )
AFAICT, irq_to_desc() would not be able to cope with ESPI interrupts when CONFIG_GICV3_ESPI is not set. Has this been tested?
Quoted 11 lines
xen/arch/arm/irq.c +3
-0+evacuate_irq (irq, from); +} +static int cpu_callback (struct notifier_block *nfb,unsigned long action,void *hcpu) {xen/arch/arm/smpboot.c +0
-0
xen/arch/arm/smpboot.c void __cpu_disable(void) · line 428
Quoted 4 lines
xen/arch/arm/smpboot.c +1
-0@@ -425,6 +425,8 @@ void __cpu_disable(void)smp_mb (); +evacuate_irqs (cpu);
I think it would be worth explaining why evacuate_irqs() is called this late in the process.
xen/arch/arm/smpboot.c void __cpu_disable(void) · line 432
Quoted 3 lines
xen/arch/arm/smpboot.c +1
-0+> /* Return to caller; eventually the IPI mechanism will unwind and the* scheduler will drop to the idle loop, which will call stop_cpu(). */ }
Cheers,
Signature · 1 line
Julien Grall
Re: [PATCH v4 4/8] arm/irq: Migrate IRQs from dyings CPUs
Quoted 14 lines · Julien Grall
Hi,
Quoted 10 lines · Mykyta Poturai
Move IRQs from dying CPU to the online ones.
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.If IRQ is to be migrated, it's affinity is set to a mask of all online CPUs. With current GIC implementation, this means they are routed to a random online CPU. This may cause extra moves if multiple cores are disabled in sequence, but should prevent all interrupts from piling up on CPU0 in case of repeated up-down cycles on different cores.
Quoted 4 lines
Wouldn't they eventually all move to CPU0 in the case of suspend/resume or if all the CPUs but CPU0 are turned off and then off? If so, shouldn't we try to rebalance the interrupts?
In case of disabling/enabling all cores in a sequence, yes. This was designed with the idea of achieving some balancing when enabling/disabling some cores for power saving reasons. I agree that proper balancing should be implemented, but it is a complex task on its own and requires a substantial amount of testing on different hardware to prove it is close to optimal. So I think implementing it is out of scope for what I’m trying to do here.
If this would be okay, I can implement a relatively simple solution of just adding onlined CPUs back to the affinity mask for now. I think it should improve the situation for the “switching all cores” case.
Quoted 8 lines
Quoted 3 lines
IRQs from CPU 0 are never migrated, as dying CPU 0 means we are either shutting down compeletely or entering system suspend.
I can't find a place where
__cpu_disable()is called on CPU0. Do you have any pointer? In any case, I am not sure I want to bake that assumption in more places of the code.
I assume it would be called when suspend is implemented. In any case, I will rework this to replace the hard check for CPU 0 with the “is it the last CPU online” one.
Quoted 7 lines
Quoted 3 lines
Considering that all Xen-used IRQs are currently allocated during init on CPU 0, and
setup_irqusessmp_processor_idfor the initial affinity.Looking at the SMMU driver, we seems to request IRQs at the time the device is attached. So are you sure about this?
Indeed, I have missed that one. I will remove those statements then.
Quoted 114 lines
Quoted 32 lines
This change is not strictly required for correct operation for now, but it should future-proof cpu hotplug and system suspend support in case some kind if IRQ balancing is implemented later.
Signed-off-by Mykyta Poturai <[email protected]>
v3->v4:
* patch introduced--- xen/arch/arm/include/asm/irq.h | 2 ++ xen/arch/arm/irq.c | 39 ++++++++++++++++++++++++++++++++++ xen/arch/arm/smpboot.c | 2 ++ 3 files changed, 43 insertions(+)xen/arch/arm/include/ renamed from xen/arch/arm/include/asm/irq.h +0
-0
asm/irq.h
index09788dbfeb..6e6e27bb80100644xen/arch/arm/include/asm/irq.h +2
-0@@ -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 evacuate_irqs (unsigned int from); +#endif /* _ASM_HW_IRQ_H */ /* * Local variables: xen/arch/arm/irq.c +0
-0xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 160
Quoted 3 lines
xen/arch/arm/irq.c +1
-0@@ -158,6 +158,45 @@ static int init_local_irq_data(unsigned int cpu)return 0 ; } +static void evacuate_irq (int irq,unsigned int from)Any reason why the 'irq' is signed?
xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 166
Quoted 5 lines
xen/arch/arm/irq.c +5
-0+{ +struct irq_desc *desc =irq_to_desc (irq); +unsigned long flags; + +/* Don't move irqs from CPU 0 as it is always last to be disabled */ Per above, I am not convinced that we should special case CPU 0. But if we do, then shouldn't this be part of
evacuate_irqs()so we don't pointlessly go through all the IRQs?Quoted 6 lines
xen/arch/arm/irq.c +6
-0+if ( from ==0 ) +return ; + +ASSERT (!cpumask_empty (&cpu_online_map)); +ASSERT (!cpumask_test_cpu (from, &cpu_online_map)); +xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 182
Quoted 10 lines
xen/arch/arm/irq.c +10
-0+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; + +if (cpumask_test_cpu (from, desc->affinity) ) +irq_set_affinity (desc, &cpu_online_map);I think it would be worth explaining why we are routing to any CPU online rather than checking whether the affinity has other online CPUs.
Just to note, I don't have strong opinion either way. It mainly needs to be documented.
xen/arch/arm/irq.c static int init_local_irq_data(unsigned int cpu) · line 196
Quoted 13 lines
xen/arch/arm/irq.c +13
-0+ +out: +spin_unlock_irqrestore (&desc->lock, flags); +return ; +} + +void evacuate_irqs (unsigned int from) +{ +int irq; +> + for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) +evacuate_irq (irq, from); + +for ( irq =ESPI_BASE_INTID ; irq <ESPI_MAX_INTID ; irq++ )AFAICT,
irq_to_desc()would not be able to cope with ESPI interrupts whenCONFIG_GICV3_ESPIis not set. Has this been tested?Quoted 12 lines
xen/arch/arm/irq.c +3
-0+evacuate_irq (irq, from); +} +static int cpu_callback (struct notifier_block *nfb,unsigned long action,void *hcpu) {xen/arch/arm/smpboot.c +0
-0xen/arch/arm/smpboot.c void __cpu_disable(void) · line 426
Quoted 2 lines
xen/arch/arm/smpboot.c +1
-0@@ -425,6 +425,8 @@ void __cpu_disable(void)smp_mb (); +evacuate_irqs (cpu);I think it would be worth explaining why
evacuate_irqs()is called this late in the process.xen/arch/arm/smpboot.c void __cpu_disable(void) · line 427
Quoted 1 line
xen/arch/arm/smpboot.c +1
-0+> /* Return to caller; eventually the IPI mechanism willunwind and the
Quoted 3 lines
xen/arch/arm/smpboot.c +0
-0* scheduler will drop to the idle loop, which will call
stop_cpu(). */
}Cheers,
Signature · 1 line
Mykyta
Re: [PATCH v4 4/8] arm/irq: Migrate IRQs from dyings CPUs
Hi,
Quoted 15 lines · Mykyta Poturai
Quoted 14 lines · Julien Grall
Hi,
Quoted 10 lines · Mykyta Poturai
Move IRQs from dying CPU to the online ones.
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.If IRQ is to be migrated, it's affinity is set to a mask of all online CPUs. With current GIC implementation, this means they are routed to a random online CPU. This may cause extra moves if multiple cores are disabled in sequence, but should prevent all interrupts from piling up on CPU0 in case of repeated up-down cycles on different cores.
Quoted 8 lines
Quoted 4 lines
Wouldn't they eventually all move to CPU0 in the case of suspend/resume or if all the CPUs but CPU0 are turned off and then off? If so, shouldn't we try to rebalance the interrupts?
In case of disabling/enabling all cores in a sequence, yes. This was designed with the idea of achieving some balancing when enabling/disabling some cores for power saving reasons.
I understand how this may balance when disabling some cores. But I don't understand how it helps for enabling cores. Can you provide more details?
Quoted 5 lines
I agree that
proper balancing should be implemented, but it is a complex task on its own and requires a substantial amount of testing on different hardware to prove it is close to optimal. So I think implementing it is out of scope for what I’m trying to do here.
Can you provide some details about what you are trying and why it would be ok to avoid the balancing?
Quoted 3 lines
Quoted 1 line
If this would be okay, I can implement a relatively simple solution of
just adding onlined CPUs back to the affinity mask for now. I think it should improve the situation for the “switching all cores” case.
Do you mean calling gic_irq_set_affinity() with the CPU? If so, Xen is still going to get one CPU from the affinity mask.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
IRQs from CPU 0 are never migrated, as dying CPU 0 means we are either shutting down compeletely or entering system suspend.
Quoted 8 lines
Quoted 4 lines
I can't find a place where
__cpu_disable()is called on CPU0. Do you have any pointer? In any case, I am not sure I want to bake that assumption in more places of the code.I assume it would be called when suspend is implemented. In any case, I will rework this to replace the hard check for CPU 0 with the “is it the last CPU online” one.
AFAIK __cpu_disable() is not going to be called during suspend/resume for CPU0. So the only case is shutting down the platform.
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
Considering that all Xen-used IRQs are currently allocated during init on CPU 0, and
setup_irqusessmp_processor_idfor the initial affinity.Looking at the SMMU driver, we seems to request IRQs at the time the device is attached. So are you sure about this?
Indeed, I have missed that one. I will remove those statements then.
I think you need to have something in the commit message explaining why you are ignoring the balancing for the SMMU.
Cheers,
Signature · 1 line
Julien Grall
PATCH v4 5/8 smp: Move cpu_up/down helpers to common code
In reply to Mykyta PoturaiThis will reduce code duplication for the upcoming cpu hotplug support on Arm64 patch.
SMT-disable enforcement check is moved into a separate architecture-specific function.
v3->v4:
* patch introduced
--- xen/arch/arm/smp.c | 6 ++++++ xen/arch/ppc/stubs.c | 4 ++++ xen/arch/riscv/stubs.c | 5 +++++ xen/arch/x86/include/asm/smp.h | 3 --- xen/arch/x86/smp.c | 33 +++------------------------------ xen/common/smp.c | 32 ++++++++++++++++++++++++++++++++ xen/include/xen/smp.h | 4 ++++ 7 files changed, 54 insertions(+), 33 deletions(-)
xen/arch/arm/smp.c +6 -0
@@ -44,6 +44,12 @@ void smp_send_call_function_mask(const cpumask_t *mask) } } +/* ARM don't have SMT so we don't need any special logic for CPU disabling */ +bool arch_smt_cpu_disable (unsigned int cpu) +{ +return false; +} +/* * Local variables: * mode: C
xen/arch/ppc/stubs.c +4 -0
@@ -101,6 +101,10 @@ void smp_send_call_function_mask(const cpumask_t *mask)BUG_ON ("unimplemented" ); } +bool arch_smt_cpu_disable (unsigned int cpu) +{ +BUG_ON ("unimplemented" ); +}/* irq.c */ void irq_ack_none (struct irq_desc *desc)
xen/arch/riscv/stubs.c +5 -0
@@ -80,6 +80,11 @@ void smp_send_call_function_mask(const cpumask_t *mask)BUG_ON ("unimplemented" ); } +bool arch_smt_cpu_disable (unsigned int cpu) +{ +BUG_ON ("unimplemented" ); +} +/* irq.c */ void irq_ack_none (struct irq_desc *desc)
xen/arch/x86/include/asm/smp.h +0 -3
@@ -50,9 +50,6 @@ int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t pxm);void __stop_this_cpu (void ); -long cf_check cpu_up_helper (void *data); -long cf_check cpu_down_helper (void *data); -long cf_check core_parking_helper (void *data);bool core_parking_remove (unsigned int cpu);uint32_t get_cur_idle_nums (void );
xen/arch/x86/smp.c +3 -30
@@ -418,35 +418,8 @@ void cf_check call_function_interrupt(void)smp_call_function_interrupt (); } -long cf_check cpu_up_helper (void *data) +bool arch_smt_cpu_disable (unsigned int cpu) { -unsigned int cpu = (unsigned long )data; -int ret =cpu_up (cpu); - -/* Have one more go on EBUSY. */ -if ( ret == -EBUSY ) - ret =cpu_up (cpu); - -if ( !ret && !opt_smt && - cpu_data[cpu].compute_unit_id ==INVALID_CUID && -cpumask_weight (per_cpu (cpu_sibling_mask, cpu)) >1 ) - { - ret =cpu_down_helper (data); -if ( ret ) -printk ("Could not re-offline CPU%u (%d)\n" , cpu, ret); -else - ret = -EPERM ; - } - -return ret; -} - -long cf_check cpu_down_helper (void *data) -{ -int cpu = (unsigned long )data; -int ret =cpu_down (cpu); -/* Have one more go on EBUSY. */ -if ( ret == -EBUSY ) - ret =cpu_down (cpu); -return ret; +return !opt_smt && cpu_data[cpu].compute_unit_id ==INVALID_CUID && +cpumask_weight (per_cpu (cpu_sibling_mask, cpu)) >1 ; }
xen/common/smp.c +32 -0
@@ -16,6 +16,7 @@* GNU General Public License for more details. */ +#include <xen/cpu.h> #include <asm/hardirq.h> #include <asm/processor.h> #include <xen/spinlock.h>
@@ -104,6 +105,37 @@ void smp_call_function_interrupt(void)irq_exit (); } +long cf_check cpu_up_helper (void *data) +{ +unsigned int cpu = (unsigned long )data; +int ret =cpu_up (cpu); + +/* Have one more go on EBUSY. */ +if ( ret == -EBUSY ) + ret =cpu_up (cpu); + +if ( !ret &&arch_smt_cpu_disable (cpu) ) + { + ret =cpu_down_helper (data); +if ( ret ) +printk ("Could not re-offline CPU%u (%d)\n" , cpu, ret); +else + ret = -EPERM ; + } + +return ret; +} + +long cf_check cpu_down_helper (void *data) +{ +int cpu = (unsigned long )data; +int ret =cpu_down (cpu); +/* Have one more go on EBUSY. */ +if ( ret == -EBUSY ) + ret =cpu_down (cpu); +return ret; +} +/* * Local variables: * mode: C
xen/include/xen/smp.h +4 -0
@@ -76,4 +76,8 @@ extern void *stack_base[NR_CPUS];void initialize_cpu_data (unsigned int cpu);int setup_cpu_root_pgt (unsigned int cpu); +bool arch_smt_cpu_disable (unsigned int cpu); +long cf_check cpu_up_helper (void *data); +long cf_check cpu_down_helper (void *data); +#endif /* __XEN_SMP_H__ */
Signature · 1 line
2.51.2
Re: [PATCH v4 5/8] smp: Move cpu_up/down helpers to common code
Quoted 7 lines · Mykyta Poturai
This will reduce code duplication for the upcoming cpu hotplug support on Arm64 patch.
SMT-disable enforcement check is moved into a separate architecture-specific function.
Signed-off-by Mykyta Poturai <[email protected]>
Solely from an x86 perspective this looks okay to me, but on Arm you introduce
...
Quoted 34 lines
xen/common/smp.c +21
-0@@ -16,6 +16,7 @@* GNU General Public License for more details. */ +#include <xen/cpu.h> #include <asm/hardirq.h> #include <asm/processor.h> #include <xen/spinlock.h> @@ -104,6 +105,37 @@ void smp_call_function_interrupt(void)irq_exit (); } +long cf_check cpu_up_helper (void *data) +{ +unsigned int cpu = (unsigned long )data; +int ret =cpu_up (cpu); + +/* Have one more go on EBUSY. */ +if ( ret == -EBUSY ) + ret =cpu_up (cpu); + +if ( !ret &&arch_smt_cpu_disable (cpu) ) + { + ret =cpu_down_helper (data); +if ( ret ) +printk ("Could not re-offline CPU%u (%d)\n" , cpu, ret); +else + ret = -EPERM ; + } + +return ret; +}
xen/common/smp.c void smp_call_function_interrupt(void) · line 137
Quoted 10 lines
xen/common/smp.c +10
-0+ +long cf_check cpu_down_helper (void *data) +{ +int cpu = (unsigned long )data; +int ret =cpu_down (cpu); +/* Have one more go on EBUSY. */ +if ( ret == -EBUSY ) + ret =cpu_down (cpu); +return ret; +}
...unreachable code, which - for the case when RUNTIME_CPU_CONTROL=n - won't even be rectified by the next patch.
Jan
Re: [PATCH v4 5/8] smp: Move cpu_up/down helpers to common code
In reply to Mykyta PoturaiHi,
Quoted 25 lines · Mykyta Poturai
This will reduce code duplication for the upcoming cpu hotplug support on Arm64 patch.
SMT-disable enforcement check is moved into a separate architecture-specific function.
Signed-off-by Mykyta Poturai <[email protected]>
v3->v4:
* patch introduced--- xen/arch/arm/smp.c | 6 ++++++ xen/arch/ppc/stubs.c | 4 ++++ xen/arch/riscv/stubs.c | 5 +++++ xen/arch/x86/include/asm/smp.h | 3 --- xen/arch/x86/smp.c | 33 +++------------------------------ xen/common/smp.c | 32 ++++++++++++++++++++++++++++++++ xen/include/xen/smp.h | 4 ++++ 7 files changed, 54 insertions(+), 33 deletions(-)xen/arch/arm/smp.c +0
-0
xen/arch/arm/smp.c void smp_send_call_function_mask(const cpumask_t *mask) · line 47
Quoted 4 lines
xen/arch/arm/smp.c +1
-0@@ -44,6 +44,12 @@ void smp_send_call_function_mask(const cpumask_t *mask) } } +/* ARM don't have SMT so we don't need any special logic for CPU disabling */
Xen doesn't support SMT on Arm. But some of the cores may support SMT. So would reword this to:
"We currently don't support SMT"
Cheers,
Signature · 1 line
Julien Grall
PATCH v4 6/8 arm/sysctl: Implement cpu hotplug ops
In reply to Mykyta PoturaiImplement XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} calls to allow for enabling/disabling CPU cores in runtime.
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.
Create a Kconfig option RUNTIME_CPU_CONTROL that reflects this constraints.
v3->v4:
* don't reimplement cpu_up/down helpers
* add Kconfig option
* fixup formatting
v2->v3:
* no changes
v1->v2:
* remove SMT ops
* remove cpu == 0 checks
* add XSM hooks
* only implement for 64bit Arm
--- xen/arch/arm/Kconfig | 4 ++++ xen/arch/arm/sysctl.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)
xen/arch/arm/Kconfig +4 -0
@@ -274,6 +274,10 @@ config PCI_PASSTHROUGH help This option enables PCI device passthrough +config RUNTIME_CPU_CONTROL + def_bool y + depends on ARM_64 && !TEE && !FFA && !HAS_ITS + endmenu menu "ARM errata workaround via the alternative framework"
xen/arch/arm/sysctl.c +32 -0
@@ -12,6 +12,7 @@#include <xen/dt-overlay.h> #include <xen/errno.h> #include <xen/hypercall.h> +#include <xsm/xsm.h> #include <asm/arm64/sve.h> #include <public/sysctl.h>
@@ -23,6 +24,33 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); } +static long cpu_hotplug_sysctl (struct xen_sysctl_cpu_hotplug *hotplug) +{ +#ifdef CONFIG_RUNTIME_CPU_CONTROL +int ret; + +switch ( hotplug->op ) + { +case XEN_SYSCTL_CPU_HOTPLUG_ONLINE : + ret =xsm_resource_plug_core (XSM_HOOK ); +if ( ret ) +return ret; +return continue_hypercall_on_cpu (0 , cpu_up_helper,_p (hotplug->cpu)); + +case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE : + ret =xsm_resource_unplug_core (XSM_HOOK ); +if ( ret ) +return ret; +return continue_hypercall_on_cpu (0 , cpu_down_helper,_p (hotplug->cpu)); + +default : +return -EOPNOTSUPP ; + } +#else +return -EOPNOTSUPP ; +#endif +} +long arch_do_sysctl (struct xen_sysctl *sysctl,XEN_GUEST_HANDLE_PARAM (xen_sysctl_t) u_sysctl) {
@@ -34,6 +62,10 @@ long arch_do_sysctl(struct xen_sysctl *sysctl, ret =dt_overlay_sysctl (&sysctl->u.dt_overlay);break ; +case XEN_SYSCTL_cpu_hotplug: + ret =cpu_hotplug_sysctl (&sysctl->u.cpu_hotplug); +break ; +default : ret = -ENOSYS ;break ;
Signature · 1 line
2.51.2
Re: [PATCH v4 6/8] arm/sysctl: Implement cpu hotplug ops
Quoted 37 lines · Mykyta Poturai
Implement XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} calls to allow for enabling/disabling CPU cores in runtime.
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.Create a
KconfigoptionRUNTIME_CPU_CONTROLthat reflects this constraints.Signed-off-by Mykyta Poturai <[email protected]>
v3->v4:
* don't reimplement cpu_up/down helpers
* addKconfigoption
* fixup formatting
v2->v3:
* no changes
v1->v2:
* remove SMT ops
* remove cpu == 0 checks
* add XSM hooks
* only implement for 64bit Arm--- xen/arch/arm/Kconfig | 4 ++++ xen/arch/arm/sysctl.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)xen/arch/arm/Kconfig +0
-0
xen/arch/arm/Kconfig config PCI_PASSTHROUGH · line 280
Quoted 7 lines
xen/arch/arm/Kconfig +4
-0@@ -274,6 +274,10 @@ config PCI_PASSTHROUGH help This option enables PCI device passthrough +config RUNTIME_CPU_CONTROL + def_bool y + depends on ARM_64 && !TEE && !FFA && !HAS_ITS +
Having it in arch code I think right as Arch can have own deps (Linux does it this way), but name has to be fixed and documented (docs).
may be more common name HOTPLUG_CPU (like in linux)? naming is up to you.
Quoted 50 lines
xen/arch/arm/Kconfig +0
-0endmenu menu "ARM errata workaround via the alternative framework"xen/arch/arm/sysctl.c +28
-0@@ -12,6 +12,7 @@#include <xen/dt-overlay.h> #include <xen/errno.h> #include <xen/hypercall.h> +#include <xsm/xsm.h> #include <asm/arm64/sve.h> #include <public/sysctl.h> @@ -23,6 +24,33 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); } +static long cpu_hotplug_sysctl (struct xen_sysctl_cpu_hotplug *hotplug) +{ +#ifdef CONFIG_RUNTIME_CPU_CONTROL +int ret; + +switch ( hotplug->op ) + { +case XEN_SYSCTL_CPU_HOTPLUG_ONLINE : + ret =xsm_resource_plug_core (XSM_HOOK ); +if ( ret ) +return ret; +return continue_hypercall_on_cpu (0 , cpu_up_helper,_p (hotplug->cpu)); + +case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE : + ret =xsm_resource_unplug_core (XSM_HOOK ); +if ( ret ) +return ret; +return continue_hypercall_on_cpu (0 , cpu_down_helper,_p (hotplug->cpu)); + +default : +return -EOPNOTSUPP ; + } +#else +return -EOPNOTSUPP ; +#endif +} +long arch_do_sysctl (struct xen_sysctl *sysctl,XEN_GUEST_HANDLE_PARAM (xen_sysctl_t) u_sysctl) {
xen/arch/arm/sysctl.c long arch_do_sysctl(struct xen_sysctl *sysctl, · line 71
Quoted 10 lines
xen/arch/arm/sysctl.c +4
-0@@ -34,6 +62,10 @@ long arch_do_sysctl(struct xen_sysctl *sysctl, ret =dt_overlay_sysctl (&sysctl->u.dt_overlay);break ; +case XEN_SYSCTL_cpu_hotplug: + ret =cpu_hotplug_sysctl (&sysctl->u.cpu_hotplug); +break ; +default : ret = -ENOSYS ;break ;
Common code need to be placed under same config guards.
Signature · 2 lines
Best regards, -grygorii
Re: [PATCH v4 6/8] arm/sysctl: Implement cpu hotplug ops
Quoted 50 lines · Grygorii Strashko
Quoted 37 lines · Mykyta Poturai
Implement XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} calls to allow for enabling/disabling CPU cores in runtime.
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.Create a
KconfigoptionRUNTIME_CPU_CONTROLthat reflects this constraints.Signed-off-by Mykyta Poturai <[email protected]>
v3->v4:
* don't reimplement cpu_up/down helpers
* addKconfigoption
* fixup formatting
v2->v3:
* no changes
v1->v2:
* remove SMT ops
* remove cpu == 0 checks
* add XSM hooks
* only implement for 64bit Arm--- xen/arch/arm/Kconfig | 4 ++++ xen/arch/arm/sysctl.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)xen/arch/arm/Kconfig +0
-0xen/arch/arm/Kconfig config PCI_PASSTHROUGH · line 279
Quoted 6 lines
xen/arch/arm/Kconfig +4
-0@@ -274,6 +274,10 @@ config PCI_PASSTHROUGH help This option enables PCI device passthrough +config RUNTIME_CPU_CONTROL + def_bool y + depends on ARM_64 && !TEE && !FFA && !HAS_ITS +Having it in arch code I think right as Arch can have own deps (Linux does it this way), but name has to be fixed and documented (docs).
Quoted 3 lines
may be more common name
HOTPLUG_CPU(like in linux)? naming is up to you.
I will change the config name to "CPU_HOTPLUG" in the next version if everyone is okay with that.
Signature · 1 line
Mykyta
Re: [PATCH v4 6/8] arm/sysctl: Implement cpu hotplug ops
In reply to Mykyta PoturaiHi,
Quoted 60 lines · Mykyta Poturai
Implement XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} calls to allow for enabling/disabling CPU cores in runtime.
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.Create a
KconfigoptionRUNTIME_CPU_CONTROLthat reflects this constraints.Signed-off-by Mykyta Poturai <[email protected]>
v3->v4:
* don't reimplement cpu_up/down helpers
* addKconfigoption
* fixup formatting
v2->v3:
* no changes
v1->v2:
* remove SMT ops
* remove cpu == 0 checks
* add XSM hooks
* only implement for 64bit Arm--- xen/arch/arm/Kconfig | 4 ++++ xen/arch/arm/sysctl.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)xen/arch/arm/Kconfig +4
-0@@ -274,6 +274,10 @@ config PCI_PASSTHROUGH help This option enables PCI device passthrough +config RUNTIME_CPU_CONTROL + def_bool y + depends on ARM_64 && !TEE && !FFA && !HAS_ITS + endmenu menu "ARM errata workaround via the alternative framework"xen/arch/arm/sysctl.c +1
-0@@ -12,6 +12,7 @@#include <xen/dt-overlay.h> #include <xen/errno.h> #include <xen/hypercall.h> +#include <xsm/xsm.h> #include <asm/arm64/sve.h> #include <public/sysctl.h>
xen/arch/arm/sysctl.c void arch_do_physinfo(struct xen_sysctl_physinfo *pi) · line 27
Quoted 4 lines
xen/arch/arm/sysctl.c +1
-0@@ -23,6 +24,33 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK); } +static long cpu_hotplug_sysctl (struct xen_sysctl_cpu_hotplug *hotplug)
As you moved the helper in common code. I was expecting the logic to handle CPU_HOTPLUG_ONLINE and CPU_HOTPLUG_OFFLINE to also move in common. Can you explain why this wasn't done?
Quoted 30 lines
xen/arch/arm/sysctl.c +26
-0+{ +#ifdef CONFIG_RUNTIME_CPU_CONTROL +int ret; + +switch ( hotplug->op ) + { +case XEN_SYSCTL_CPU_HOTPLUG_ONLINE : + ret =xsm_resource_plug_core (XSM_HOOK ); +if ( ret ) +return ret; +return continue_hypercall_on_cpu (0 , cpu_up_helper,_p (hotplug->cpu)); + +case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE : + ret =xsm_resource_unplug_core (XSM_HOOK ); +if ( ret ) +return ret; +return continue_hypercall_on_cpu (0 , cpu_down_helper,_p (hotplug->cpu)); + +default : +return -EOPNOTSUPP ; + } +#else +return -EOPNOTSUPP ; +#endif +} +long arch_do_sysctl (struct xen_sysctl *sysctl,XEN_GUEST_HANDLE_PARAM (xen_sysctl_t) u_sysctl) {
xen/arch/arm/sysctl.c long arch_do_sysctl(struct xen_sysctl *sysctl, · line 71
Quoted 10 lines
xen/arch/arm/sysctl.c +4
-0@@ -34,6 +62,10 @@ long arch_do_sysctl(struct xen_sysctl *sysctl, ret =dt_overlay_sysctl (&sysctl->u.dt_overlay);break ; +case XEN_SYSCTL_cpu_hotplug: + ret =cpu_hotplug_sysctl (&sysctl->u.cpu_hotplug); +break ; +default : ret = -ENOSYS ;break ;
Cheers,
Signature · 1 line
Julien Grall
PATCH v4 7/8 tools: Allow building xen-hptool without CONFIG_MIGRATE
In reply to Mykyta PoturaiWith CPU hotplug sysctls implemented on Arm it becomes useful to have a tool for calling them. Introduce a new congifure option "hptool" to allow building hptool separately from other migration tools, and enable it by default.
v3->v4:
* no changes
v2->v3:
* no changes
v1->v2:
* switch to configure from legacy config
--- config/Tools.mk.in | 1 + tools/configure | 30 ++++++++++++++++++++++++++++++ tools/configure.ac | 1 + tools/libs/guest/Makefile.common | 4 ++++ tools/misc/Makefile | 2 +- 5 files changed, 37 insertions(+), 1 deletion(-) mode change 100755 => 100644 tools/configure
config/Tools.mk.in +1 -0
@@ -49,6 +49,7 @@ CONFIG_LIBNL := @libnl@ CONFIG_GOLANG := @golang@ CONFIG_PYGRUB := @pygrub@ CONFIG_LIBFSIMAGE := @libfsimage@ +CONFIG_HPTOOL := @hptool@ CONFIG_SYSTEMD := @systemd@ XEN_SYSTEMD_DIR := @SYSTEMD_DIR@
tools/configure +30 -0
@@ -731,6 +731,7 @@ LD86 AS86 ipxe LINUX_BACKEND_MODULES +hptool pygrub golang seabios
@@ -837,6 +838,7 @@ enable_ovmf enable_seabios enable_golang enable_pygrub +enable_hptool with_linux_backend_modules enable_ipxe with_system_ipxe
@@ -1524,6 +1526,7 @@ Optional Features: --disable-seabios Disable SeaBIOS (default is ENABLED) --disable-golang Disable Go tools (default is ENABLED) --disable-pygrub Disable pygrub (default is ENABLED) + --disable-hptool Disable hptool (default is ENABLED) --enable-ipxe Enable in-tree IPXE, (DEFAULT is off, see also --with-system-ipxe) --enable-rombios Enable ROMBIOS, (DEFAULT is on if ipxe is enabled,
@@ -4816,6 +4819,33 @@ pygrub=$ax_cv_pygrub +# Check whether --enable-hptool was given. +if test ${enable_hptool+y} +then : + enableval=$enable_hptool; +fi + + +if test "x$enable_hptool" = "xno" +then : + + ax_cv_hptool="n" + +elif test "x$enable_hptool" = "xyes" +then : + + ax_cv_hptool="y" + +elif test -z $ax_cv_hptool +then : + + ax_cv_hptool="y" + +fi +hptool=$ax_cv_hptool + + + # Check whether --with-linux-backend-modules was given. if test ${with_linux_backend_modules+y}
tools/configure.ac +1 -0
@@ -90,6 +90,7 @@ AX_ARG_DEFAULT_DISABLE([ovmf], [Enable OVMF]) AX_ARG_DEFAULT_ENABLE([seabios], [Disable SeaBIOS]) AX_ARG_DEFAULT_ENABLE([golang], [Disable Go tools]) AX_ARG_DEFAULT_ENABLE([pygrub], [Disable pygrub]) +AX_ARG_DEFAULT_ENABLE([hptool], [Disable hptool]) AC_ARG_WITH([linux-backend-modules], AS_HELP_STRING([--with-linux-backend-modules="mod1 mod2"],
tools/libs/guest/Makefile.common +4 -0
@@ -25,6 +25,10 @@ OBJS-y += xg_core.o OBJS-$(CONFIG_X86) += xg_core_x86.o OBJS-$(CONFIG_ARM) += xg_core_arm.o +ifneq (,$(filter y,$(CONFIG_MIGRATE)$(CONFIG_HPTOOL))) +OBJS-y += xg_offline_page.o +endif + vpath %.c ../../../xen/common/libelf LIBELF_OBJS += libelf-tools.o libelf-loader.o
tools/misc/Makefile +1 -1
@@ -16,7 +16,7 @@ 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_HPTOOL) += xen-hptool INSTALL_SBIN-$(CONFIG_X86) += xen-hvmcrash INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx INSTALL_SBIN-$(CONFIG_X86) += xen-lowmemd
Signature · 1 line
2.51.2
Re: [PATCH v4 7/8] tools: Allow building xen-hptool without CONFIG_MIGRATE
Quoted 7 lines · Mykyta Poturai
With CPU hotplug sysctls implemented on Arm it becomes useful to have a tool for calling them. Introduce a new congifure option "hptool" to allow building hptool separately from other migration tools, and enable it by default.
Signed-off-by Mykyta Poturai <[email protected]>
You should had a line with just "---" around here, right after the Signed-off-by, and before the changelog. That way, when we commit the patch, git will automatically discard this patch changelog. Notice that git do just that before adding stats about file changes.
Quoted 46 lines
v3->v4:
* no changes
v2->v3:
* no changes
v1->v2:
* switch to configure from legacy config--- config/Tools.mk.in | 1 + tools/configure | 30 ++++++++++++++++++++++++++++++ tools/configure.ac | 1 + tools/libs/guest/Makefile.common | 4 ++++ tools/misc/Makefile | 2 +- 5 files changed, 37 insertions(+), 1 deletion(-) mode change 100755 => 100644 tools/configureconfig/Tools.mk.in +1
-0@@ -49,6 +49,7 @@ CONFIG_LIBNL := @libnl@ CONFIG_GOLANG := @golang@ CONFIG_PYGRUB := @pygrub@ CONFIG_LIBFSIMAGE := @libfsimage@ +CONFIG_HPTOOL := @hptool@ CONFIG_SYSTEMD := @systemd@ XEN_SYSTEMD_DIR := @SYSTEMD_DIR@tools/configure.ac +1
-0@@ -90,6 +90,7 @@ AX_ARG_DEFAULT_DISABLE([ovmf], [Enable OVMF]) AX_ARG_DEFAULT_ENABLE([seabios], [Disable SeaBIOS]) AX_ARG_DEFAULT_ENABLE([golang], [Disable Go tools]) AX_ARG_DEFAULT_ENABLE([pygrub], [Disable pygrub]) +AX_ARG_DEFAULT_ENABLE([hptool], [Disable hptool]) AC_ARG_WITH([linux-backend-modules], AS_HELP_STRING([--with-linux-backend-modules="mod1 mod2"],tools/libs/guest/Makefile.common +0
-0
tools/libs/guest/Makefile.common OBJS-y += xg_core.o · line 28
Quoted 4 lines
tools/libs/guest/Makefile.common +1
-0@@ -25,6 +25,10 @@ OBJS-y += xg_core.o OBJS-$(CONFIG_X86) += xg_core_x86.o OBJS-$(CONFIG_ARM) += xg_core_arm.o +ifneq (,$(filter y,$(CONFIG_MIGRATE)$(CONFIG_HPTOOL)))
How is this supposed to work?
Quoted 11 lines
tools/libs/guest/Makefile.common +3
-0+OBJS-y += xg_offline_page.o +endif + vpath %.c ../../../xen/common/libelf LIBELF_OBJS += libelf-tools.o libelf-loader.otools/misc/Makefile +0
-0
tools/misc/Makefile INSTALL_BIN += xencov_split · line 19
Quoted 5 lines
tools/misc/Makefile +1
-1@@ -16,7 +16,7 @@ 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_HPTOOL) += xen-hptool
So, I've look at the history as to why "xen-hptool" was only compiled for CONFIG_MIGRATE, and it was because "xc_offline_page.c" didn't build on ia64, commit 6b0b6e01b967 ("tools: disable xen-hptool on ia64").
And hiding xc_offline_page.c behind CONFIG_MIGRATE was probably just a short-cut, 310311cd8863 ("libxc: fix link error on ia64") because I guess the needed functions where behind this value at the time.
("xc_offline_page.c" is now "xg_offline_page.c")
This patch now build xen-hptool, and "xg_offline_page.c" by default. Does this actually build on every architecture? We don't really have per-binary configuration option, and having a library (libxenguest) which get different functionality depending on which tool we want to build doesn't seems to be the right thing to do.
If xg_offline_page.c can now be built on every architecture, how about building it by default? Then, we can always build xen-hptool.
Thanks,
Signature · 1 line
Anthony PERARD
PATCH v4 8/8 docs: Document CPU hotplug
In reply to Mykyta Poturaiv3->v4:
* update configuration section
v2->v3:
* patch introduced
--- docs/misc/cpu-hotplug.txt | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 docs/misc/cpu-hotplug.txt
docs/misc/cpu-hotplug.txt new file +51 -0
@@ -0,0 +1,51 @@ +CPU Hotplug +=========== + +CPU hotplug is a feature that allows pCPU cores to be added to or removed from a +running system without requiring a reboot. It is supported on x86 and Arm64 +architectures. + +Implementation Details +---------------------- + +CPU hotplug is implemented through the `XEN_SYSCTL_CPU_HOTPLUG_*` sysctl calls. +The specific calls are: + +- `XEN_SYSCTL_CPU_HOTPLUG_ONLINE`: Brings a pCPU online +- `XEN_SYSCTL_CPU_HOTPLUG_OFFLINE`: Takes a pCPU offline +- `XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE`: Enables SMT threads (x86 only) +- `XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE`: Disables SMT threads (x86 only) + +All cores can be disabled, assuming hardware support, except for core 0. Sysctl +calls are routed to core 0 before doing any actual up/down operations on other +cores. + +Configuration +------------- + +Sysctl handlers are enabled unconditionally on x86 architecture. On Arm64, +handlers are enabled by default when ITS, FFA, and TEE configs are disabled. +Building of the userspace tool "hptool" is controlled by the "hptool" flag in +the configure script. It is enabled by default and can be disabled with +--disable-hptool command line option. + +Usage +----- + +Disable core: + +$ xen-hptool cpu-offline 2 +Prepare to offline CPU 2 +(XEN) Removing cpu 2 from runqueue 0 +CPU 2 offlined successfully + +Enable core: + +$ xen-hptool cpu-online 2 +Prepare to online CPU 2 +(XEN) Bringing up CPU2 +(XEN) GICv3: CPU2: Found redistributor in region 0 @00000a004005c000 +(XEN) CPU2: Guest atomics will try 1 times before pausing the domain +(XEN) CPU 2 booted. +(XEN) Adding cpu 2 to runqueue 0 +CPU 2 onlined successfully
Signature · 1 line
2.51.2
Re: [PATCH v4 8/8] docs: Document CPU hotplug
Hi,
Quoted 18 lines · Mykyta Poturai
Signed-off-by Mykyta Poturai <[email protected]>
v3->v4:
* update configuration section
v2->v3:
* patch introduced--- docs/misc/cpu-hotplug.txt | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)create mode 100644
docs/misc/cpu-hotplug.txtdocs/misc/cpu-hotplug.txt new file +0
-0
docs/misc/cpu-hotplug.txt line 6
Quoted 6 lines
docs/misc/cpu-hotplug.txt new file +6
-0@@ -0,0 +1,51 @@ +CPU Hotplug +=========== + +CPU hotplug is a feature that allows pCPU cores to be added to or removed from a +running system without requiring a reboot. It is supported on x86 and Arm64 +architectures.
"supported" usually implies that we will backport fixes and issues XSA. Is this what you intended to say?
Asking because SUPPORT.md has the following:
### ACPI CPU Hotplug
Status, x86: Experimental
There is also no mention for Arm in SUPPORT.md. As you enable the feature, we should clarify what's its state.
> +
Quoted 34 lines
docs/misc/cpu-hotplug.txt new file +34
-0+Implementation Details +---------------------- + +CPU hotplug is implemented through the `XEN_SYSCTL_CPU_HOTPLUG_*` sysctl calls. +The specific calls are: + +- `XEN_SYSCTL_CPU_HOTPLUG_ONLINE`: Brings a pCPU online +- `XEN_SYSCTL_CPU_HOTPLUG_OFFLINE`: Takes a pCPU offline +- `XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE`: Enables SMT threads (x86 only) +- `XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE`: Disables SMT threads (x86 only) + +All cores can be disabled, assuming hardware support, except for core 0. Sysctl +calls are routed to core 0 before doing any actual up/down operations on other +cores. + +Configuration +------------- + +Sysctl handlers are enabled unconditionally on x86 architecture. On Arm64, +handlers are enabled by default when ITS, FFA, and TEE configs are disabled. +Building of the userspace tool "hptool" is controlled by the "hptool" flag in +the configure script. It is enabled by default and can be disabled with +--disable-hptool command line option. + +Usage +----- + +Disable core: + +$ xen-hptool cpu-offline 2 +Prepare to offline CPU 2 +(XEN) Removing cpu 2 from runqueue 0 +CPU 2 offlined successfully +
docs/misc/cpu-hotplug.txt line 51
Quoted 10 lines
docs/misc/cpu-hotplug.txt new file +10
-0+Enable core: + +$ xen-hptool cpu-online 2 +Prepare to online CPU 2 +(XEN) Bringing up CPU2 +(XEN) GICv3: CPU2: Found redistributor in region 0 @00000a004005c000 +(XEN) CPU2: Guest atomics will try 1 times before pausing the domain +(XEN) CPU 2 booted. +(XEN) Adding cpu 2 to runqueue 0 +CPU 2 onlined successfully
Cheers,
Signature · 1 line
Julien Grall