[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from u
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore |
Date: |
Wed, 17 May 2017 12:08:26 +0200 |
On Wed, 17 May 2017 07:50:42 +0200
Cédric Le Goater <address@hidden> wrote:
> On 05/16/2017 06:10 PM, Greg Kurz wrote:
> > On Tue, 16 May 2017 17:18:27 +0200
> > Cédric Le Goater <address@hidden> wrote:
> >
> >> On 05/16/2017 02:55 PM, Laurent Vivier wrote:
> >>> On 16/05/2017 14:50, Cédric Le Goater wrote:
> >>>> On 05/16/2017 02:03 PM, Laurent Vivier wrote:
> >>>>> On 26/04/2017 09:00, David Gibson wrote:
> >>>>>> From: Cédric Le Goater <address@hidden>
> >>>>>>
> >>>>>> 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 <address@hidden>
> >>>>>> Reviewed-by: David Gibson <address@hidden>
> >>>>>> Signed-off-by: David Gibson <address@hidden>
> >>>>>> ---
> >>>>>> 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?
> >>>>
> >>>> it seems that a cleanup is not done in the kernel. We are missing
> >>>> a way to call kvmppc_xics_free_icp() from QEMU. Today the only
> >>>> way is to destroy the vcpu.
> >>>
> >>> The commit introducing this hack, for reference:
> >>>
> >>> commit a45863bda90daa8ec39e5a312b9734fd4665b016
> >>> Author: Bharata B Rao <address@hidden>
> >>> Date: Thu Jul 2 16:23:20 2015 +1000
> >>>
> >>> xics_kvm: Don't enable KVM_CAP_IRQ_XICS if already enabled
> >>>
> >>> 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.
> >>>
> >>> Re-enabling this CAP should ideally work, but currently it results in
> >>> kernel trying to create and associate ICP with this vCPU and that
> >>> fails since there is already an ICP associated with it. Hence this
> >>> patch is needed to work around this problem in the kernel.
> >>>
> >>> This change allows CPU hot removal to work for sPAPR.
> >>>
> >>> Signed-off-by: Bharata B Rao <address@hidden>
> >>> Reviewed-by: David Gibson <address@hidden>
> >>> Signed-off-by: David Gibson <address@hidden>
> >>> Signed-off-by: Alexander Graf <address@hidden>
> >>
> >> OK.
> >>
> >> Greg is looking at re-adding the ICPState array because of a
> >> migration issue with older machines. We might need to do so
> >> unconditionally ...
> >>
> >
> > That would be a pity to carry on with the pre-allocated ICPStates for
> > new machine types just because of that... What about keeping track
> > of all the cap_irq_xics_enabled flags in a separate max_cpus sized
> > static array ?
>
> Could we use 'cpu->unplug' instead ?
>
No we can't because we only park the vCPU fd actually and we always pass
a freshly allocated CPUState with @unplug == false to xics_cpu_setup().
> C.
>
>
> >> But for that specific issue, I think it would have been better
> >> to clean up the kernel state. Is that possible ?
> >>
> >
> > Commit 4c055ab54fae ("cpu: Reclaim vCPU objects") gives some more details
> > on why we don't destroy the vCPU in KVM on unplug, but rather park the vCPU
> > fd for later use... so I'm not sure we can clean up the kernel state.
> >
> > But since the vCPU is still present, maybe we can find a way to tell KVM
> > that we want to reuse an already present ICP ?
> >
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>>> Else we need to reintroduce the array of icps (again) to keep some
> >>>> xics state ... but that just sucks :/ Let me think about it.
> >>>>
> >>>
> >>> Thanks,
> >>> Laurent
> >>>> C.
> >>>>
> >>>
> >>
> >
>
pgpzPeWjxWNUh.pgp
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, Laurent Vivier, 2017/05/16
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, Cédric Le Goater, 2017/05/16
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, Laurent Vivier, 2017/05/16
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, Cédric Le Goater, 2017/05/16
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, Greg Kurz, 2017/05/16
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, Cédric Le Goater, 2017/05/17
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, David Gibson, 2017/05/17
- Re: [Qemu-devel] [Qemu-ppc] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore, Greg Kurz, 2017/05/17
- Re: [Qemu-devel] [PULL 19/48] spapr: allocate the ICPState object from under sPAPRCPUCore,
Greg Kurz <=