* Re: [Qemu-devel] [PATCH] xics_kvm: cache already enabled vCPU ids
2017-05-17 14:38 [Qemu-devel] [PATCH] xics_kvm: cache already enabled vCPU ids Greg Kurz
@ 2017-05-17 15:18 ` Laurent Vivier
2017-05-17 15:24 ` Laurent Vivier
2017-05-17 15:36 ` Cédric Le Goater
2017-05-18 5:56 ` David Gibson
2 siblings, 1 reply; 5+ messages in thread
From: Laurent Vivier @ 2017-05-17 15:18 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Bharata B Rao, qemu-ppc, Cedric Le Goater, David Gibson
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;
> };
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] xics_kvm: cache already enabled vCPU ids
2017-05-17 15:18 ` Laurent Vivier
@ 2017-05-17 15:24 ` Laurent Vivier
0 siblings, 0 replies; 5+ messages in thread
From: Laurent Vivier @ 2017-05-17 15:24 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: David Gibson, qemu-ppc, Cedric Le Goater, Bharata B Rao
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] xics_kvm: cache already enabled vCPU ids
2017-05-17 14:38 [Qemu-devel] [PATCH] xics_kvm: cache already enabled vCPU ids Greg Kurz
2017-05-17 15:18 ` Laurent Vivier
@ 2017-05-17 15:36 ` Cédric Le Goater
2017-05-18 5:56 ` David Gibson
2 siblings, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2017-05-17 15:36 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: Laurent Vivier, Bharata B Rao, qemu-ppc, David Gibson
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;
> };
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] xics_kvm: cache already enabled vCPU ids
2017-05-17 14:38 [Qemu-devel] [PATCH] xics_kvm: cache already enabled vCPU ids Greg Kurz
2017-05-17 15:18 ` Laurent Vivier
2017-05-17 15:36 ` Cédric Le Goater
@ 2017-05-18 5:56 ` David Gibson
2 siblings, 0 replies; 5+ messages in thread
From: David Gibson @ 2017-05-18 5:56 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Laurent Vivier, Bharata B Rao, qemu-ppc,
Cedric Le Goater
[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread