Re: [PATCH v3 17/17] KVM: PPC: Book3S HV: XIVE: clear the vCPU interrupt presenters

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5652 bytes --]

On Fri, Mar 15, 2019 at 01:06:09PM +0100, 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, the
> previous KVM interrupt device is 'destroyed' before the chosen one is
> created. Upon destruction, the vCPU interrupt presenters using the KVM
> device should be cleared first, the machine will reconnect them later
> to the new device after it is created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> 
>  Changes since v2 :
> 
>  - removed comments on possible race in kvmppc_native_connect_vcpu()
>    for the XIVE KVM device. This is still an issue in the
>    XICS-over-XIVE device.
> 
>  arch/powerpc/kvm/book3s_xics.c        | 19 +++++++++++++
>  arch/powerpc/kvm/book3s_xive.c        | 39 +++++++++++++++++++++++++--
>  arch/powerpc/kvm/book3s_xive_native.c | 12 +++++++++
>  3 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
> index f27ee57ab46e..81cdabf4295f 100644
> --- a/arch/powerpc/kvm/book3s_xics.c
> +++ b/arch/powerpc/kvm/book3s_xics.c
> @@ -1342,6 +1342,25 @@ static void kvmppc_xics_free(struct kvm_device *dev)
>  	struct kvmppc_xics *xics = dev->private;
>  	int i;
>  	struct kvm *kvm = xics->kvm;
> +	struct kvm_vcpu *vcpu;
> +
> +	/*
> +	 * When destroying the VM, the vCPUs are destroyed first and
> +	 * the vCPU list should be empty. If this is not the case,
> +	 * then we are simply destroying the device and we should
> +	 * clean up the vCPU interrupt presenters first.
> +	 */
> +	if (atomic_read(&kvm->online_vcpus) != 0) {
> +		/*
> +		 * 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();
> +
> +		kvm_for_each_vcpu(i, vcpu, kvm)
> +			kvmppc_xics_free_icp(vcpu);
> +	}
>  
>  	debugfs_remove(xics->dentry);
>  
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 480a3fc6b9fd..cf6a4c6c5a28 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");
> @@ -1828,8 +1840,31 @@ static void kvmppc_xive_free(struct kvm_device *dev)
>  {
>  	struct kvmppc_xive *xive = dev->private;
>  	struct kvm *kvm = xive->kvm;
> +	struct kvm_vcpu *vcpu;
>  	int i;
>  
> +	/*
> +	 * When destroying the VM, the vCPUs are destroyed first and
> +	 * the vCPU list should be empty. If this is not the case,
> +	 * then we are simply destroying the device and we should
> +	 * clean up the vCPU interrupt presenters first.
> +	 */
> +	if (atomic_read(&kvm->online_vcpus) != 0) {
> +		/*
> +		 * 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();
> +
> +		/*
> +		 * TODO: There is still a race window with the early
> +		 * checks in kvmppc_native_connect_vcpu()
> +		 */
> +		kvm_for_each_vcpu(i, vcpu, kvm)
> +			kvmppc_xive_cleanup_vcpu(vcpu);
> +	}
> +
>  	debugfs_remove(xive->dentry);
>  
>  	if (kvm)
> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> index 67a1bb26a4cc..8f7be5e23177 100644
> --- a/arch/powerpc/kvm/book3s_xive_native.c
> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> @@ -956,8 +956,20 @@ static void kvmppc_xive_native_free(struct kvm_device *dev)
>  {
>  	struct kvmppc_xive *xive = dev->private;
>  	struct kvm *kvm = xive->kvm;
> +	struct kvm_vcpu *vcpu;
>  	int i;
>  
> +	/*
> +	 * When destroying the VM, the vCPUs are destroyed first and
> +	 * the vCPU list should be empty. If this is not the case,
> +	 * then we are simply destroying the device and we should
> +	 * clean up the vCPU interrupt presenters first.
> +	 */
> +	if (atomic_read(&kvm->online_vcpus) != 0) {
> +		kvm_for_each_vcpu(i, vcpu, kvm)
> +			kvmppc_xive_native_cleanup_vcpu(vcpu);
> +	}
> +
>  	debugfs_remove(xive->dentry);
>  
>  	pr_devel("Destroying xive native device\n");

-- 
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: 833 bytes --]