Re: [PATCH 07/23] KVM: PPC: Book3S: Allow reuse of vCPU object

From: Alexander Graf <agraf@suse.de>
To: Bharata B Rao <bharata.rao@gmail.com>
Cc: bharata <bharata@linux.vnet.ibm.com>,
	Paul Mackerras <paulus@samba.org>,
	kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH 07/23] KVM: PPC: Book3S: Allow reuse of vCPU object
Date: Mon, 23 Mar 2015 09:31:12 +0100	[thread overview]
Message-ID: <550FCF50.7090300@suse.de> (raw)
In-Reply-To: <CAGZKiBqaQmCN734d1Mb2TXgHjoUmLzr0HpyGkvR8VegqeZkkNw@mail.gmail.com>

On 23.03.15 08:50, Bharata B Rao wrote:
> On Sat, Mar 21, 2015 at 8:28 PM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 20.03.15 16:51, Bharata B Rao wrote:
>>> On Fri, Mar 20, 2015 at 12:34:18PM +0100, Alexander Graf wrote:
>>>>
>>>>
>>>> On 20.03.15 12:26, Paul Mackerras wrote:
>>>>> On Fri, Mar 20, 2015 at 12:01:32PM +0100, Alexander Graf wrote:
>>>>>>
>>>>>>
>>>>>> On 20.03.15 10:39, Paul Mackerras wrote:
>>>>>>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>>>>>>
>>>>>>> Since KVM isn't equipped to handle closure of vcpu fd from userspace(QEMU)
>>>>>>> correctly, certain work arounds have to be employed to allow reuse of
>>>>>>> vcpu array slot in KVM during cpu hot plug/unplug from guest. One such
>>>>>>> proposed workaround is to park the vcpu fd in userspace during cpu unplug
>>>>>>> and reuse it later during next hotplug.
>>>>>>>
>>>>>>> More details can be found here:
>>>>>>> KVM: https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
>>>>>>> QEMU: http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg00859.html
>>>>>>>
>>>>>>> In order to support this workaround with PowerPC KVM, don't create or
>>>>>>> initialize ICP if the vCPU is found to be already associated with an ICP.
>>>>>>>
>>>>>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>>>>>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>>>>>>
>>>>>> This probably makes some sense, but please make sure that user space has
>>>>>> some way to figure out whether hotplug works at all.
>>>>>
>>>>> Bharata is working on the qemu side of all this, so I assume he has
>>>>> that covered.
>>>>
>>>> Well, so far the kernel doesn't expose anything he can query, so I
>>>> suppose he just blindly assumes that older host kernels will randomly
>>>> break and nobody cares. I'd rather prefer to see a CAP exposed that qemu
>>>> can check on.
>>>
>>> I see that you have already taken this into your tree. I have an updated
>>> patch to expose a CAP. If the below patch looks ok, then let me know how
>>> you would prefer to take this patch in.
>>>
>>> Regards,
>>> Bharata.
>>>
>>> KVM: PPC: BOOK3S: Allow reuse of vCPU object
>>>
>>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>>
>>> Since KVM isn't equipped to handle closure of vcpu fd from userspace(QEMU)
>>> correctly, certain work arounds have to be employed to allow reuse of
>>> vcpu array slot in KVM during cpu hot plug/unplug from guest. One such
>>> proposed workaround is to park the vcpu fd in userspace during cpu unplug
>>> and reuse it later during next hotplug.
>>>
>>> More details can be found here:
>>> KVM: https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
>>> QEMU: http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg00859.html
>>>
>>> In order to support this workaround with PowerPC KVM, don't create or
>>> initialize ICP if the vCPU is found to be already associated with an ICP.
>>> User space (QEMU) can reuse the vCPU after checking for the availability
>>> of KVM_CAP_SPAPR_REUSE_VCPU capability.
>>>
>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>> ---
>>>  arch/powerpc/kvm/book3s_xics.c |    9 +++++++--
>>>  arch/powerpc/kvm/powerpc.c     |   12 ++++++++++++
>>>  include/uapi/linux/kvm.h       |    1 +
>>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
>>> index a4a8d9f..ead3a35 100644
>>> --- a/arch/powerpc/kvm/book3s_xics.c
>>> +++ b/arch/powerpc/kvm/book3s_xics.c
>>> @@ -1313,8 +1313,13 @@ int kvmppc_xics_connect_vcpu(struct kvm_device *dev, struct kvm_vcpu *vcpu,
>>>               return -EPERM;
>>>       if (xics->kvm != vcpu->kvm)
>>>               return -EPERM;
>>> -     if (vcpu->arch.irq_type)
>>> -             return -EBUSY;
>>> +
>>> +     /*
>>> +      * If irq_type is already set, don't reinialize but
>>> +      * return success allowing this vcpu to be reused.
>>> +      */
>>> +     if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT)
>>> +             return 0;
>>>
>>>       r = kvmppc_xics_create_icp(vcpu, xcpu);
>>>       if (!r)
>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>> index 27c0fac..5b7007c 100644
>>> --- a/arch/powerpc/kvm/powerpc.c
>>> +++ b/arch/powerpc/kvm/powerpc.c
>>> @@ -564,6 +564,18 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>               r = 1;
>>>               break;
>>>  #endif
>>> +     case KVM_CAP_SPAPR_REUSE_VCPU:
>>> +             /*
>>> +              * Kernel currently doesn't support closing of vCPU fd from
>>> +              * user space (QEMU) correctly. Hence the option available
>>> +              * is to park the vCPU fd in user space whenever a guest
>>> +              * CPU is hot removed and reuse the same later when another
>>> +              * guest CPU is hotplugged. This capability determines whether
>>> +              * it is safe to assume if parking of vCPU fd and reuse from
>>> +              * user space works for sPAPR guests.
>>
>> I don't see how the code you're changing here has anything to do with
>> parking vcpus. It's all about being able to call connect on an already
>> connected vcpu and not erroring out. Please reflect this in the cap name
>> and description.
>>
>> You also need to update Documentation/virtual/kvm/api.txt.
>>
>> Furthermore, thinking about this a bit more, I might still miss the
>> exact case why you need this. Why is QEMU issuing a connect again? Could
>> it maybe just not do it?
> 
> Thinking more, I see that I can handle this entirely in user space. So
> no need for this patch.
> 
> Sorry for the noise :(

Perfect, removed this patch from my queue again :).

Alex