Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore

[Laurent Vivier <lvivier@redhat.com>]

On 26/04/2017 09:00, David Gibson wrote:
> From: Cédric Le Goater <clg@kaod.org>
> 
> Today, all the ICPs are created before the CPUs, stored in an array
> under the sPAPR machine and linked to the CPU when the core threads
> are realized. This modeling brings some complexity when a lookup in
> the array is required and it can be simplified by allocating the ICPs
> when the CPUs are.
> 
> This is the purpose of this proposal which introduces a new 'icp_type'
> field under the machine and creates the ICP objects of the right type
> (KVM or not) before the PowerPCCPU object are.
> 
> This change allows more cleanups : the removal of the icps array under
> the sPAPR machine and the removal of the xics_get_cpu_index_by_dt_id()
> helper.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/intc/xics.c          | 11 -----------
>  hw/ppc/spapr.c          | 47 ++++++++++++++---------------------------------
>  hw/ppc/spapr_cpu_core.c | 18 ++++++++++++++----
>  include/hw/ppc/spapr.h  |  2 +-
>  include/hw/ppc/xics.h   |  2 --
>  5 files changed, 29 insertions(+), 51 deletions(-)
> 

This commit breaks CPU re-hotplugging with KVM

the sequence "device_add, device_del, device_add" brings to the
following error message:

    Unable to connect CPUx to kernel XICS: Device or resource busy

It comes from icp_kvm_cpu_setup():

...
    ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd,
                              kvm_arch_vcpu_id(cs));
    if (ret < 0) {
        error_report("Unable to connect CPU%ld to kernel XICS: %s",
                     kvm_arch_vcpu_id(cs), strerror(errno));
        exit(1);
    }
..

It should be protected by cap_irq_xics_enabled:

...
    /*
     * 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 (icp->cap_irq_xics_enabled) {
        return;
    }
...
    ret = kvm_vcpu_enable_cap(...);
...
    icp->cap_irq_xics_enabled = true;
...

But since this commit, "icp" is a new object on each call:

spapr_cpu_core_realize_child()
...
    obj = object_new(spapr->icp_type);
...
    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
    ...
            icpc->cpu_setup(icp, cpu); -> icp_kvm_cpu_setup()
    ...
...

and "cap_irq_xics_enabled" is reinitialized.

Any idea how to fix that?

Thanks,
Laurent