Re: [RFC PATCH v4.1 16/17] KVM: PPC: Book3S HV: XIVE: introduce a xive_devices array under the VM

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

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

On Tue, Apr 09, 2019 at 04:13:46PM +0200, Cédric Le Goater wrote:
> On P9 sPAPR guests, the interrupt mode (XICS legacy or XIVE native) is
> determine at CAS time and the chosen mode is activated after a machine
> reset. To be able to switch from one mode to another, subsequent
> patches will introduce the capability to destroy the KVM device
> without destroying the VM.
> 
> This is not considered as a safe operation as the vCPUs are still
> running and could be referencing the KVM device through their
> presenters.
> 
> To protect the system from any breakage, the kvmppc_xive objects
> representing both KVM devices are now stored in an array under the
> VM. Allocation is performed on first usage and memory is freed only
> when the VM exits.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

Although...

> ---
>  arch/powerpc/include/asm/kvm_host.h   |  1 +
>  arch/powerpc/kvm/book3s_xive.h        |  1 +
>  arch/powerpc/kvm/book3s_xive.c        | 23 +++++++++++++++++++++--
>  arch/powerpc/kvm/book3s_xive_native.c |  9 +++++++--
>  arch/powerpc/kvm/powerpc.c            |  6 ++++++
>  5 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 9cc6abdce1b9..ed059c95e56a 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -314,6 +314,7 @@ struct kvm_arch {
>  #ifdef CONFIG_KVM_XICS
>  	struct kvmppc_xics *xics;
>  	struct kvmppc_xive *xive;
> +	struct kvmppc_xive *xive_devices[2];

... as noted on the previous revision, I feel this use of a
bool-indexed array instead of separate fields makes things more
confusing than necessary, for a negligible reduction in code size.

>  	struct kvmppc_passthru_irqmap *pimap;
>  #endif
>  	struct kvmppc_ops *kvm_ops;
> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> index e011622dc038..426146332984 100644
> --- a/arch/powerpc/kvm/book3s_xive.h
> +++ b/arch/powerpc/kvm/book3s_xive.h
> @@ -283,6 +283,7 @@ void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb);
>  int kvmppc_xive_select_target(struct kvm *kvm, u32 *server, u8 prio);
>  int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
>  				  bool single_escalation);
> +struct kvmppc_xive *kvmppc_xive_get_device(struct kvm *kvm, u32 type);
>  
>  #endif /* CONFIG_KVM_XICS */
>  #endif /* _KVM_PPC_BOOK3S_XICS_H */
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 480a3fc6b9fd..4d4e1730de84 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -1846,11 +1846,30 @@ static void kvmppc_xive_free(struct kvm_device *dev)
>  	if (xive->vp_base != XIVE_INVALID_VP)
>  		xive_native_free_vp_block(xive->vp_base);
>  
> +	/*
> +	 * A reference of the kvmppc_xive pointer is now kept under
> +	 * the xive_devices[] array of the machine for reuse. It is
> +	 * freed when the VM is destroyed.
> +	 */
>  
> -	kfree(xive);
>  	kfree(dev);
>  }
>  
> +struct kvmppc_xive *kvmppc_xive_get_device(struct kvm *kvm, u32 type)
> +{
> +	struct kvmppc_xive *xive;
> +	bool xive_native_index = type == KVM_DEV_TYPE_XIVE;
> +
> +	xive = kvm->arch.xive_devices[xive_native_index];
> +
> +	if (!xive) {
> +		xive = kzalloc(sizeof(*xive), GFP_KERNEL);
> +		kvm->arch.xive_devices[xive_native_index] = xive;
> +	}
> +
> +	return xive;
> +}
> +
>  static int kvmppc_xive_create(struct kvm_device *dev, u32 type)
>  {
>  	struct kvmppc_xive *xive;
> @@ -1859,7 +1878,7 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type)
>  
>  	pr_devel("Creating xive for partition\n");
>  
> -	xive = kzalloc(sizeof(*xive), GFP_KERNEL);
> +	xive = kvmppc_xive_get_device(kvm, type);
>  	if (!xive)
>  		return -ENOMEM;
>  
> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> index 62648f833adf..092db0efe628 100644
> --- a/arch/powerpc/kvm/book3s_xive_native.c
> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> @@ -987,7 +987,12 @@ static void kvmppc_xive_native_free(struct kvm_device *dev)
>  	if (xive->vp_base != XIVE_INVALID_VP)
>  		xive_native_free_vp_block(xive->vp_base);
>  
> -	kfree(xive);
> +	/*
> +	 * A reference of the kvmppc_xive pointer is now kept under
> +	 * the xive_devices[] array of the machine for reuse. It is
> +	 * freed when the VM is destroyed.
> +	 */
> +
>  	kfree(dev);
>  }
>  
> @@ -1002,7 +1007,7 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
>  	if (kvm->arch.xive)
>  		return -EEXIST;
>  
> -	xive = kzalloc(sizeof(*xive), GFP_KERNEL);
> +	xive = kvmppc_xive_get_device(kvm, type);
>  	if (!xive)
>  		return -ENOMEM;
>  
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index f54926c78320..d0914316ddc7 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -501,6 +501,12 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  
>  	mutex_unlock(&kvm->lock);
>  
> +	for (i = 0; i < ARRAY_SIZE(kvm->arch.xive_devices); i++) {
> +		struct kvmppc_xive *xive = kvm->arch.xive_devices[i];
> +		if (xive)
> +			kfree(xive);
> +	}
> +
>  	/* drop the module reference */
>  	module_put(kvm->arch.kvm_ops->owner);
>  }

-- 
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 --]