Since commit a45863bda90d ("xics_kvm: Don't enable KVM_CAP_IRQ_XICS if
already enabled"), we were able to re-hotplug a vCPU that had been hot-
unplugged ealier, thanks to a boolean flag in ICPState that we set when
enabling KVM_CAP_IRQ_XICS.
This could work because the lifecycle of all ICPState objects was the
same as the machine. Commit 5bc8d26de20c ("spapr: allocate the ICPState
object from under sPAPRCPUCore") broke this assumption and now we always
pass a freshly allocated ICPState object (ie, with the flag unset) to
icp_kvm_cpu_setup().
This cause re-hotplug to fail with:
Unable to connect CPU8 to kernel XICS: Device or resource busy
Let's fix this by caching all the vCPU ids for which KVM_CAP_IRQ_XICS was
enabled. This also drops the now useless boolean flag from ICPState.
Reported-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/intc/xics_kvm.c | 27 ++++++++++++++++++++-------
include/hw/ppc/xics.h | 1 -
2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index dd93531ae376..dd7f29846235 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -42,6 +42,14 @@
static int kernel_xics_fd = -1;
+typedef struct KVMEnabledICP {
+ unsigned long vcpu_id;
+ QLIST_ENTRY(KVMEnabledICP) node;
+} KVMEnabledICP;
+
+static QLIST_HEAD(, KVMEnabledICP)
+ kvm_enabled_icps = QLIST_HEAD_INITIALIZER(&kvm_enabled_icps);
+
/*
* ICP-KVM
*/
@@ -121,6 +129,8 @@ static void icp_kvm_reset(void *dev)
static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
{
CPUState *cs = CPU(cpu);
+ KVMEnabledICP *enabled_icp;
+ unsigned long vcpu_id = kvm_arch_vcpu_id(cs);
int ret;
if (kernel_xics_fd == -1) {
@@ -132,18 +142,21 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
* which was hot-removed earlier we don't have to renable
* KVM_CAP_IRQ_XICS capability again.
*/
- if (icp->cap_irq_xics_enabled) {
- return;
+ QLIST_FOREACH(enabled_icp, &kvm_enabled_icps, node) {
+ if (enabled_icp->vcpu_id == vcpu_id) {
+ return;
+ }
}
- ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd,
- kvm_arch_vcpu_id(cs));
+ ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id);
if (ret < 0) {
- error_report("Unable to connect CPU%ld to kernel XICS: %s",
- kvm_arch_vcpu_id(cs), strerror(errno));
+ error_report("Unable to connect CPU%ld to kernel XICS: %s", vcpu_id,
+ strerror(errno));
exit(1);
}
- icp->cap_irq_xics_enabled = true;
+ enabled_icp = g_malloc(sizeof(*enabled_icp));
+ enabled_icp->vcpu_id = vcpu_id;
+ QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
}
static void icp_kvm_realize(DeviceState *dev, Error **errp)
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index d6cb51f3ad5d..a3073f90533a 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -81,7 +81,6 @@ struct ICPState {
uint8_t pending_priority;
uint8_t mfrr;
qemu_irq output;
- bool cap_irq_xics_enabled;
XICSFabric *xics;
};
On 17/05/2017 16:38, Greg Kurz wrote: > Since commit a45863bda90d ("xics_kvm: Don't enable KVM_CAP_IRQ_XICS if > already enabled"), we were able to re-hotplug a vCPU that had been hot- > unplugged ealier, thanks to a boolean flag in ICPState that we set when > enabling KVM_CAP_IRQ_XICS. > > This could work because the lifecycle of all ICPState objects was the > same as the machine. Commit 5bc8d26de20c ("spapr: allocate the ICPState > object from under sPAPRCPUCore") broke this assumption and now we always > pass a freshly allocated ICPState object (ie, with the flag unset) to > icp_kvm_cpu_setup(). > > This cause re-hotplug to fail with: > > Unable to connect CPU8 to kernel XICS: Device or resource busy > > Let's fix this by caching all the vCPU ids for which KVM_CAP_IRQ_XICS was > enabled. This also drops the now useless boolean flag from ICPState. > > Reported-by: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Greg Kurz <groug@kaod.org> Tested-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/intc/xics_kvm.c | 27 ++++++++++++++++++++------- > include/hw/ppc/xics.h | 1 - > 2 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index dd93531ae376..dd7f29846235 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -42,6 +42,14 @@ > > static int kernel_xics_fd = -1; > > +typedef struct KVMEnabledICP { > + unsigned long vcpu_id; > + QLIST_ENTRY(KVMEnabledICP) node; > +} KVMEnabledICP; > + > +static QLIST_HEAD(, KVMEnabledICP) > + kvm_enabled_icps = QLIST_HEAD_INITIALIZER(&kvm_enabled_icps); > + > /* > * ICP-KVM > */ > @@ -121,6 +129,8 @@ static void icp_kvm_reset(void *dev) > static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) > { > CPUState *cs = CPU(cpu); > + KVMEnabledICP *enabled_icp; > + unsigned long vcpu_id = kvm_arch_vcpu_id(cs); > int ret; > > if (kernel_xics_fd == -1) { > @@ -132,18 +142,21 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) > * which was hot-removed earlier we don't have to renable > * KVM_CAP_IRQ_XICS capability again. > */ > - if (icp->cap_irq_xics_enabled) { > - return; > + QLIST_FOREACH(enabled_icp, &kvm_enabled_icps, node) { > + if (enabled_icp->vcpu_id == vcpu_id) { > + return; > + } > } > > - ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, > - kvm_arch_vcpu_id(cs)); > + ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id); > if (ret < 0) { > - error_report("Unable to connect CPU%ld to kernel XICS: %s", > - kvm_arch_vcpu_id(cs), strerror(errno)); > + error_report("Unable to connect CPU%ld to kernel XICS: %s", vcpu_id, > + strerror(errno)); > exit(1); > } > - icp->cap_irq_xics_enabled = true; > + enabled_icp = g_malloc(sizeof(*enabled_icp)); > + enabled_icp->vcpu_id = vcpu_id; > + QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node); > } > > static void icp_kvm_realize(DeviceState *dev, Error **errp) > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index d6cb51f3ad5d..a3073f90533a 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -81,7 +81,6 @@ struct ICPState { > uint8_t pending_priority; > uint8_t mfrr; > qemu_irq output; > - bool cap_irq_xics_enabled; > > XICSFabric *xics; > }; >
On 17/05/2017 17:18, Laurent Vivier wrote: > On 17/05/2017 16:38, Greg Kurz wrote: >> Since commit a45863bda90d ("xics_kvm: Don't enable KVM_CAP_IRQ_XICS if >> already enabled"), we were able to re-hotplug a vCPU that had been hot- >> unplugged ealier, thanks to a boolean flag in ICPState that we set when >> enabling KVM_CAP_IRQ_XICS. >> >> This could work because the lifecycle of all ICPState objects was the >> same as the machine. Commit 5bc8d26de20c ("spapr: allocate the ICPState >> object from under sPAPRCPUCore") broke this assumption and now we always >> pass a freshly allocated ICPState object (ie, with the flag unset) to >> icp_kvm_cpu_setup(). >> >> This cause re-hotplug to fail with: >> >> Unable to connect CPU8 to kernel XICS: Device or resource busy >> >> Let's fix this by caching all the vCPU ids for which KVM_CAP_IRQ_XICS was >> enabled. This also drops the now useless boolean flag from ICPState. >> >> Reported-by: Laurent Vivier <lvivier@redhat.com> >> Signed-off-by: Greg Kurz <groug@kaod.org> > > Tested-by: Laurent Vivier <lvivier@redhat.com> and: Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On 05/17/2017 04:38 PM, Greg Kurz wrote: > Since commit a45863bda90d ("xics_kvm: Don't enable KVM_CAP_IRQ_XICS if > already enabled"), we were able to re-hotplug a vCPU that had been hot- > unplugged ealier, thanks to a boolean flag in ICPState that we set when > enabling KVM_CAP_IRQ_XICS. > > This could work because the lifecycle of all ICPState objects was the > same as the machine. Commit 5bc8d26de20c ("spapr: allocate the ICPState > object from under sPAPRCPUCore") broke this assumption and now we always > pass a freshly allocated ICPState object (ie, with the flag unset) to > icp_kvm_cpu_setup(). > > This cause re-hotplug to fail with: > > Unable to connect CPU8 to kernel XICS: Device or resource busy > > Let's fix this by caching all the vCPU ids for which KVM_CAP_IRQ_XICS was > enabled. This also drops the now useless boolean flag from ICPState. > > Reported-by: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/intc/xics_kvm.c | 27 ++++++++++++++++++++------- > include/hw/ppc/xics.h | 1 - > 2 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index dd93531ae376..dd7f29846235 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -42,6 +42,14 @@ > > static int kernel_xics_fd = -1; > > +typedef struct KVMEnabledICP { > + unsigned long vcpu_id; > + QLIST_ENTRY(KVMEnabledICP) node; > +} KVMEnabledICP; > + > +static QLIST_HEAD(, KVMEnabledICP) > + kvm_enabled_icps = QLIST_HEAD_INITIALIZER(&kvm_enabled_icps); > + > /* > * ICP-KVM > */ > @@ -121,6 +129,8 @@ static void icp_kvm_reset(void *dev) > static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) > { > CPUState *cs = CPU(cpu); > + KVMEnabledICP *enabled_icp; > + unsigned long vcpu_id = kvm_arch_vcpu_id(cs); > int ret; > > if (kernel_xics_fd == -1) { > @@ -132,18 +142,21 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) > * which was hot-removed earlier we don't have to renable > * KVM_CAP_IRQ_XICS capability again. > */ > - if (icp->cap_irq_xics_enabled) { > - return; > + QLIST_FOREACH(enabled_icp, &kvm_enabled_icps, node) { > + if (enabled_icp->vcpu_id == vcpu_id) { > + return; > + } > } > > - ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, > - kvm_arch_vcpu_id(cs)); > + ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id); > if (ret < 0) { > - error_report("Unable to connect CPU%ld to kernel XICS: %s", > - kvm_arch_vcpu_id(cs), strerror(errno)); > + error_report("Unable to connect CPU%ld to kernel XICS: %s", vcpu_id, > + strerror(errno)); > exit(1); > } > - icp->cap_irq_xics_enabled = true; > + enabled_icp = g_malloc(sizeof(*enabled_icp)); > + enabled_icp->vcpu_id = vcpu_id; > + QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node); > } > > static void icp_kvm_realize(DeviceState *dev, Error **errp) > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index d6cb51f3ad5d..a3073f90533a 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -81,7 +81,6 @@ struct ICPState { > uint8_t pending_priority; > uint8_t mfrr; > qemu_irq output; > - bool cap_irq_xics_enabled; > > XICSFabric *xics; > }; >
On Wed, May 17, 2017 at 04:38:20PM +0200, Greg Kurz wrote: > Since commit a45863bda90d ("xics_kvm: Don't enable KVM_CAP_IRQ_XICS if > already enabled"), we were able to re-hotplug a vCPU that had been hot- > unplugged ealier, thanks to a boolean flag in ICPState that we set when > enabling KVM_CAP_IRQ_XICS. > > This could work because the lifecycle of all ICPState objects was the > same as the machine. Commit 5bc8d26de20c ("spapr: allocate the ICPState > object from under sPAPRCPUCore") broke this assumption and now we always > pass a freshly allocated ICPState object (ie, with the flag unset) to > icp_kvm_cpu_setup(). > > This cause re-hotplug to fail with: > > Unable to connect CPU8 to kernel XICS: Device or resource busy > > Let's fix this by caching all the vCPU ids for which KVM_CAP_IRQ_XICS was > enabled. This also drops the now useless boolean flag from ICPState. > > Reported-by: Laurent Vivier <lvivier@redhat.com> > Signed-off-by: Greg Kurz <groug@kaod.org> Applied to ppc-for-2.10. > --- > hw/intc/xics_kvm.c | 27 ++++++++++++++++++++------- > include/hw/ppc/xics.h | 1 - > 2 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index dd93531ae376..dd7f29846235 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -42,6 +42,14 @@ > > static int kernel_xics_fd = -1; > > +typedef struct KVMEnabledICP { > + unsigned long vcpu_id; > + QLIST_ENTRY(KVMEnabledICP) node; > +} KVMEnabledICP; > + > +static QLIST_HEAD(, KVMEnabledICP) > + kvm_enabled_icps = QLIST_HEAD_INITIALIZER(&kvm_enabled_icps); > + > /* > * ICP-KVM > */ > @@ -121,6 +129,8 @@ static void icp_kvm_reset(void *dev) > static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) > { > CPUState *cs = CPU(cpu); > + KVMEnabledICP *enabled_icp; > + unsigned long vcpu_id = kvm_arch_vcpu_id(cs); > int ret; > > if (kernel_xics_fd == -1) { > @@ -132,18 +142,21 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu) > * which was hot-removed earlier we don't have to renable > * KVM_CAP_IRQ_XICS capability again. > */ > - if (icp->cap_irq_xics_enabled) { > - return; > + QLIST_FOREACH(enabled_icp, &kvm_enabled_icps, node) { > + if (enabled_icp->vcpu_id == vcpu_id) { > + return; > + } > } > > - ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, > - kvm_arch_vcpu_id(cs)); > + ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id); > if (ret < 0) { > - error_report("Unable to connect CPU%ld to kernel XICS: %s", > - kvm_arch_vcpu_id(cs), strerror(errno)); > + error_report("Unable to connect CPU%ld to kernel XICS: %s", vcpu_id, > + strerror(errno)); > exit(1); > } > - icp->cap_irq_xics_enabled = true; > + enabled_icp = g_malloc(sizeof(*enabled_icp)); > + enabled_icp->vcpu_id = vcpu_id; > + QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node); > } > > static void icp_kvm_realize(DeviceState *dev, Error **errp) > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index d6cb51f3ad5d..a3073f90533a 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -81,7 +81,6 @@ struct ICPState { > uint8_t pending_priority; > uint8_t mfrr; > qemu_irq output; > - bool cap_irq_xics_enabled; > > XICSFabric *xics; > }; > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
© 2016 - 2024 Red Hat, Inc.