Re: [RFC PATCH v4 17/17] KVM: PPC: Book3S HV: XIVE: introduce a 'release' device operation

From: "Cédric Le Goater" <clg@kaod.org>
To: Paul Mackerras <paulus@ozlabs.org>,
	David Gibson <david@gibson.dropbear.id.au>
Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [RFC PATCH v4 17/17] KVM: PPC: Book3S HV: XIVE: introduce a 'release' device operation
Date: Mon, 15 Apr 2019 15:56:02 +0200	[thread overview]
Message-ID: <c8d4b6d6-9f97-f985-6d5c-420469014f41@kaod.org> (raw)
In-Reply-To: <20190415092513.GA8139@blackberry>

On 4/15/19 11:25 AM, Paul Mackerras wrote:
> On Mon, Apr 15, 2019 at 01:32:19PM +1000, David Gibson wrote:
>> On Tue, Apr 09, 2019 at 04:13:47PM +0200, Cédric Le Goater wrote:
>>> When the VM boots, the CAS negotiation process determines which
>>> interrupt mode to use and invokes a machine reset. At that time, any
>>> links to the previous KVM interrupt device should be 'destroyed'
>>> before the new chosen one is created.
>>>
>>> To perform the necessary cleanups in KVM, we extend the KVM device
>>> interface with a new 'release' operation which is called when the file
>>> descriptor of the device is closed.
>>>
>>> Such operations are defined for the XICS-on-XIVE and the XIVE native
>>> KVM devices. They clear the vCPU interrupt presenters that could be
>>> attached and then destroy the device.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  include/linux/kvm_host.h              |  1 +
>>>  arch/powerpc/kvm/book3s_xive.c        | 50 +++++++++++++++++++++++++--
>>>  arch/powerpc/kvm/book3s_xive_native.c | 23 ++++++++++++
>>>  virt/kvm/kvm_main.c                   | 13 +++++++
>>>  4 files changed, 85 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>> index 831d963451d8..3b444620d8fc 100644
>>> --- a/include/linux/kvm_host.h
>>> +++ b/include/linux/kvm_host.h
>>> @@ -1246,6 +1246,7 @@ struct kvm_device_ops {
>>>  	long (*ioctl)(struct kvm_device *dev, unsigned int ioctl,
>>>  		      unsigned long arg);
>>>  	int (*mmap)(struct kvm_device *dev, struct vm_area_struct *vma);
>>> +	void (*release)(struct kvm_device *dev);
>>>  };
>>>  
>>>  void kvm_device_get(struct kvm_device *dev);
>>> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
>>> index 4d4e1730de84..ba777db849d7 100644
>>> --- a/arch/powerpc/kvm/book3s_xive.c
>>> +++ b/arch/powerpc/kvm/book3s_xive.c
>>> @@ -1100,11 +1100,19 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
>>>  void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>>> -	struct kvmppc_xive *xive = xc->xive;
>>> +	struct kvmppc_xive *xive;
>>>  	int i;
>>>  
>>> +	if (!kvmppc_xics_enabled(vcpu))
>>> +		return;
>>> +
>>> +	if (!xc)
>>> +		return;
>>> +
>>>  	pr_devel("cleanup_vcpu(cpu=%d)\n", xc->server_num);
>>>  
>>> +	xive = xc->xive;
>>> +
>>>  	/* Ensure no interrupt is still routed to that VP */
>>>  	xc->valid = false;
>>>  	kvmppc_xive_disable_vcpu_interrupts(vcpu);
>>> @@ -1141,6 +1149,10 @@ void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
>>>  	}
>>>  	/* Free the VP */
>>>  	kfree(xc);
>>> +
>>> +	/* Cleanup the vcpu */
>>> +	vcpu->arch.irq_type = KVMPPC_IRQ_DEFAULT;
>>> +	vcpu->arch.xive_vcpu = NULL;
>>>  }
>>>  
>>>  int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>>> @@ -1158,7 +1170,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>>>  	}
>>>  	if (xive->kvm != vcpu->kvm)
>>>  		return -EPERM;
>>> -	if (vcpu->arch.irq_type)
>>> +	if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT)
>>>  		return -EBUSY;
>>>  	if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {
>>>  		pr_devel("Duplicate !\n");
>>> @@ -1855,6 +1867,39 @@ static void kvmppc_xive_free(struct kvm_device *dev)
>>>  	kfree(dev);
>>>  }
>>>  
>>> +static void kvmppc_xive_release(struct kvm_device *dev)
>>> +{
>>> +	struct kvmppc_xive *xive = dev->private;
>>> +	struct kvm *kvm = xive->kvm;
>>> +	struct kvm_vcpu *vcpu;
>>> +	int i;
>>> +
>>> +	pr_devel("Releasing xive device\n");
>>> +
>>> +	/*
>>> +	 * When releasing the KVM device fd, the vCPUs can still be
>>> +	 * running and we should clean up the vCPU interrupt
>>> +	 * presenters first.
>>> +	 */
>>> +	if (atomic_read(&kvm->online_vcpus) != 0) {
>>
>> What prevents online_vcpus from becoming non-zero after this test, but
>> before the kvmppc_xive_free()?
>>
>> Is the test actually necessary?  The operations below should be safe
>> even if there are no online cpus, yes?
> 
> Right... Similarly, the kick_all_cpus_sync() without anything having
> been done before it that we want the other vcpus to notice made me
> wonder what the point of it was.  In other places where it is used we
> have done something such as set kvm->arch.mmu_ready to 0 first.

This part is more dubious. It comes from my understanding of the routine 
kvm_arch_destroy_vm() that makes sure all IPIs have been handled before 
clearing  the VCPUs structures. commit e17769eb8c89 is a bit cryptic and 
looks like an optimization that the release operation can ignore ?

Thanks,

C.   

>>> +		/*
>>> +		 * call kick_all_cpus_sync() to ensure that all CPUs
>>> +		 * have executed any pending interrupts
>>> +		 */
>>> +		if (is_kvmppc_hv_enabled(kvm))
>>> +			kick_all_cpus_sync();
> 
> Paul.
> 