| Message ID | 1427117764-23008-18-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 23, 2015 at 07:05:58PM +0530, Bharata B Rao wrote: > When supporting CPU hot removal by parking the vCPU fd and reusing > it during hotplug again, there can be cases where we try to reenable > KVM_CAP_IRQ_XICS CAP for the vCPU for which it was already enabled. > Introduce a boolean member in ICPState to track this and don't > reenable the CAP if it was already enabled earlier. > > This change allows CPU hot removal to work for sPAPR. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> Why does double enabling the capability cause problems? I would have expected it to be unnecessary, but harmless. > --- > hw/intc/xics_kvm.c | 10 ++++++++++ > include/hw/ppc/xics.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index c15453f..5b27bf8 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -331,6 +331,15 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > abort(); > } > > + /* > + * If we are reusing a parked vCPU fd corresponding to the CPU > + * which was hot-removed earlier we don't have to renable > + * KVM_CAP_IRQ_XICS capability again. > + */ > + if (ss->cap_irq_xics_enabled) { > + return; > + } > + > if (icpkvm->kernel_xics_fd != -1) { > int ret; > > @@ -343,6 +352,7 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > kvm_arch_vcpu_id(cs), strerror(errno)); > exit(1); > } > + ss->cap_irq_xics_enabled = true; > } > } > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index a214dd7..355a966 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -109,6 +109,7 @@ struct ICPState { > uint8_t pending_priority; > uint8_t mfrr; > qemu_irq output; > + bool cap_irq_xics_enabled; > }; > > #define TYPE_ICS "ics"
On Wed, Mar 25, 2015 at 04:24:39PM +1100, David Gibson wrote: > On Mon, Mar 23, 2015 at 07:05:58PM +0530, Bharata B Rao wrote: > > When supporting CPU hot removal by parking the vCPU fd and reusing > > it during hotplug again, there can be cases where we try to reenable > > KVM_CAP_IRQ_XICS CAP for the vCPU for which it was already enabled. > > Introduce a boolean member in ICPState to track this and don't > > reenable the CAP if it was already enabled earlier. > > > > This change allows CPU hot removal to work for sPAPR. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Why does double enabling the capability cause problems? I would have > expected it to be unnecessary, but harmless. We are reusing the vCPU here w/o closing its fd. As things stand currently, enabling this cap again will result in kernel trying to create and associate ICP with this vCPU and that fails since there is already an ICP associated with it. Ref: arch/powerpc/kvm/book3s_xics.c:kvmppc_xics_connect_vcpu() kernel code. So this patch will ensure that we don't renable this cap. Regards, Bharata.
On Wed, Mar 25, 2015 at 02:42:24PM +0530, Bharata B Rao wrote: > On Wed, Mar 25, 2015 at 04:24:39PM +1100, David Gibson wrote: > > On Mon, Mar 23, 2015 at 07:05:58PM +0530, Bharata B Rao wrote: > > > When supporting CPU hot removal by parking the vCPU fd and reusing > > > it during hotplug again, there can be cases where we try to reenable > > > KVM_CAP_IRQ_XICS CAP for the vCPU for which it was already enabled. > > > Introduce a boolean member in ICPState to track this and don't > > > reenable the CAP if it was already enabled earlier. > > > > > > This change allows CPU hot removal to work for sPAPR. > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > Why does double enabling the capability cause problems? I would have > > expected it to be unnecessary, but harmless. > > We are reusing the vCPU here w/o closing its fd. > > As things stand currently, enabling this cap again will result in > kernel trying to create and associate ICP with this vCPU and that > fails since there is already an ICP associated with it. > > Ref: arch/powerpc/kvm/book3s_xics.c:kvmppc_xics_connect_vcpu() kernel code. Ah, right. Sounds like a kernel bug - I would expect enabling a capability to be idempotent. But since the bug's there, we have to work around it now, so ok. > So this patch will ensure that we don't renable this cap. > > Regards, > Bharata. >
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c index c15453f..5b27bf8 100644 --- a/hw/intc/xics_kvm.c +++ b/hw/intc/xics_kvm.c @@ -331,6 +331,15 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) abort(); } + /* + * If we are reusing a parked vCPU fd corresponding to the CPU + * which was hot-removed earlier we don't have to renable + * KVM_CAP_IRQ_XICS capability again. + */ + if (ss->cap_irq_xics_enabled) { + return; + } + if (icpkvm->kernel_xics_fd != -1) { int ret; @@ -343,6 +352,7 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) kvm_arch_vcpu_id(cs), strerror(errno)); exit(1); } + ss->cap_irq_xics_enabled = true; } } diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index a214dd7..355a966 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -109,6 +109,7 @@ struct ICPState { uint8_t pending_priority; uint8_t mfrr; qemu_irq output; + bool cap_irq_xics_enabled; }; #define TYPE_ICS "ics"
When supporting CPU hot removal by parking the vCPU fd and reusing it during hotplug again, there can be cases where we try to reenable KVM_CAP_IRQ_XICS CAP for the vCPU for which it was already enabled. Introduce a boolean member in ICPState to track this and don't reenable the CAP if it was already enabled earlier. This change allows CPU hot removal to work for sPAPR. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/intc/xics_kvm.c | 10 ++++++++++ include/hw/ppc/xics.h | 1 + 2 files changed, 11 insertions(+)