On Fri, 24 May 2019 15:20:30 +0200
Cédric Le Goater <clg@xxxxxxxx> wrote:
> The XICS-on-XIVE KVM device needs to allocate XIVE event queues when a
> priority is used by the OS. This is referred as EQ provisioning and it
> is done under the hood when :
>
> 1. a CPU is hot-plugged in the VM
> 2. the "set-xive" is called at VM startup
> 3. sources are restored at VM restore
>
> The kvm->lock mutex is used to protect the different XIVE structures
> being modified but in some contextes, kvm->lock is taken under the
> vcpu->mutex which is a forbidden sequence by KVM.
>
> Introduce a new mutex 'lock' for the KVM devices for them to
> synchronize accesses to the XIVE device structures.
>
> Signed-off-by: Cédric Le Goater <clg@xxxxxxxx>
> ---
> arch/powerpc/kvm/book3s_xive.h | 1 +
> arch/powerpc/kvm/book3s_xive.c | 23 +++++++++++++----------
> arch/powerpc/kvm/book3s_xive_native.c | 15 ++++++++-------
> 3 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> index 426146332984..862c2c9650ae 100644
> --- a/arch/powerpc/kvm/book3s_xive.h
> +++ b/arch/powerpc/kvm/book3s_xive.h
> @@ -141,6 +141,7 @@ struct kvmppc_xive {
> struct kvmppc_xive_ops *ops;
> struct address_space *mapping;
> struct mutex mapping_lock;
> + struct mutex lock;
> };
>
> #define KVMPPC_XIVE_Q_COUNT 8
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index f623451ec0a3..12c8a36dd980 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -271,14 +271,14 @@ static int xive_provision_queue(struct kvm_vcpu *vcpu, u8 prio)
> return rc;
> }
>
> -/* Called with kvm_lock held */
> +/* Called with xive->lock held */
> static int xive_check_provisioning(struct kvm *kvm, u8 prio)
> {
> struct kvmppc_xive *xive = kvm->arch.xive;
Since the kvm_lock isn't protecting kvm->arch anymore, this looks weird.
Passing xive instead of kvm and using xive->kvm would make more sense IMHO.
Maybe fold the following into your patch ?
============================================================================
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c
@@ -272,9 +272,9 @@ static int xive_provision_queue(struct kvm_vcpu *vcpu, u8 prio)
}
/* Called with xive->lock held */
-static int xive_check_provisioning(struct kvm *kvm, u8 prio)
+static int xive_check_provisioning(struct kvmppc_xive *xive, u8 prio)
{
- struct kvmppc_xive *xive = kvm->arch.xive;
+ struct kvm *kvm = xive->kvm;
struct kvm_vcpu *vcpu;
int i, rc;
@@ -623,7 +623,7 @@ int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
/* First, check provisioning of queues */
if (priority != MASKED) {
mutex_lock(&xive->lock);
- rc = xive_check_provisioning(xive->kvm,
+ rc = xive_check_provisioning(xive,
xive_prio_from_guest(priority));
mutex_unlock(&xive->lock);
}
@@ -1673,7 +1673,7 @@ static int xive_set_source(struct kvmppc_xive *xive, long irq, u64 addr)
if (act_prio != MASKED) {
/* First, check provisioning of queues */
mutex_lock(&xive->lock);
- rc = xive_check_provisioning(xive->kvm, act_prio);
+ rc = xive_check_provisioning(xive, act_prio);
mutex_unlock(&xive->lock);
/* Target interrupt */
============================================================================
The rest looks good.
> struct kvm_vcpu *vcpu;
> int i, rc;
>
> - lockdep_assert_held(&kvm->lock);
> + lockdep_assert_held(&xive->lock);
>
> /* Already provisioned ? */
> if (xive->qmap & (1 << prio))
> @@ -621,9 +621,12 @@ int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
> irq, server, priority);
>
> /* First, check provisioning of queues */
> - if (priority != MASKED)
> + if (priority != MASKED) {
> + mutex_lock(&xive->lock);
> rc = xive_check_provisioning(xive->kvm,
> xive_prio_from_guest(priority));
> + mutex_unlock(&xive->lock);
> + }
> if (rc) {
> pr_devel(" provisioning failure %d !\n", rc);
> return rc;
> @@ -1199,7 +1202,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> return -ENOMEM;
>
> /* We need to synchronize with queue provisioning */
> - mutex_lock(&vcpu->kvm->lock);
> + mutex_lock(&xive->lock);
> vcpu->arch.xive_vcpu = xc;
> xc->xive = xive;
> xc->vcpu = vcpu;
> @@ -1283,7 +1286,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> xive_vm_esb_load(&xc->vp_ipi_data, XIVE_ESB_SET_PQ_00);
>
> bail:
> - mutex_unlock(&vcpu->kvm->lock);
> + mutex_unlock(&xive->lock);
> if (r) {
> kvmppc_xive_cleanup_vcpu(vcpu);
> return r;
> @@ -1527,13 +1530,12 @@ static int xive_get_source(struct kvmppc_xive *xive, long irq, u64 addr)
> struct kvmppc_xive_src_block *kvmppc_xive_create_src_block(
> struct kvmppc_xive *xive, int irq)
> {
> - struct kvm *kvm = xive->kvm;
> struct kvmppc_xive_src_block *sb;
> int i, bid;
>
> bid = irq >> KVMPPC_XICS_ICS_SHIFT;
>
> - mutex_lock(&kvm->lock);
> + mutex_lock(&xive->lock);
>
> /* block already exists - somebody else got here first */
> if (xive->src_blocks[bid])
> @@ -1560,7 +1562,7 @@ struct kvmppc_xive_src_block *kvmppc_xive_create_src_block(
> xive->max_sbid = bid;
>
> out:
> - mutex_unlock(&kvm->lock);
> + mutex_unlock(&xive->lock);
> return xive->src_blocks[bid];
> }
>
> @@ -1670,9 +1672,9 @@ static int xive_set_source(struct kvmppc_xive *xive, long irq, u64 addr)
> /* If we have a priority target the interrupt */
> if (act_prio != MASKED) {
> /* First, check provisioning of queues */
> - mutex_lock(&xive->kvm->lock);
> + mutex_lock(&xive->lock);
> rc = xive_check_provisioning(xive->kvm, act_prio);
> - mutex_unlock(&xive->kvm->lock);
> + mutex_unlock(&xive->lock);
>
> /* Target interrupt */
> if (rc == 0)
> @@ -1963,6 +1965,7 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type)
> dev->private = xive;
> xive->dev = dev;
> xive->kvm = kvm;
> + mutex_init(&xive->lock);
>
> /* Already there ? */
> if (kvm->arch.xive)
> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> index cdce9f94738e..684619517d67 100644
> --- a/arch/powerpc/kvm/book3s_xive_native.c
> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> @@ -114,7 +114,7 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> return -EINVAL;
> }
>
> - mutex_lock(&vcpu->kvm->lock);
> + mutex_lock(&xive->lock);
>
> if (kvmppc_xive_find_server(vcpu->kvm, server_num)) {
> pr_devel("Duplicate !\n");
> @@ -159,7 +159,7 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>
> /* TODO: reset all queues to a clean state ? */
> bail:
> - mutex_unlock(&vcpu->kvm->lock);
> + mutex_unlock(&xive->lock);
> if (rc)
> kvmppc_xive_native_cleanup_vcpu(vcpu);
>
> @@ -772,7 +772,7 @@ static int kvmppc_xive_reset(struct kvmppc_xive *xive)
>
> pr_devel("%s\n", __func__);
>
> - mutex_lock(&kvm->lock);
> + mutex_lock(&xive->lock);
>
> kvm_for_each_vcpu(i, vcpu, kvm) {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> @@ -810,7 +810,7 @@ static int kvmppc_xive_reset(struct kvmppc_xive *xive)
> }
> }
>
> - mutex_unlock(&kvm->lock);
> + mutex_unlock(&xive->lock);
>
> return 0;
> }
> @@ -878,7 +878,7 @@ static int kvmppc_xive_native_eq_sync(struct kvmppc_xive *xive)
>
> pr_devel("%s\n", __func__);
>
> - mutex_lock(&kvm->lock);
> + mutex_lock(&xive->lock);
> for (i = 0; i <= xive->max_sbid; i++) {
> struct kvmppc_xive_src_block *sb = xive->src_blocks[i];
>
> @@ -892,7 +892,7 @@ static int kvmppc_xive_native_eq_sync(struct kvmppc_xive *xive)
> kvm_for_each_vcpu(i, vcpu, kvm) {
> kvmppc_xive_native_vcpu_eq_sync(vcpu);
> }
> - mutex_unlock(&kvm->lock);
> + mutex_unlock(&xive->lock);
>
> return 0;
> }
> @@ -965,7 +965,7 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
> }
>
> /*
> - * Called when device fd is closed
> + * Called when device fd is closed. kvm->lock is held.
> */
> static void kvmppc_xive_native_release(struct kvm_device *dev)
> {
> @@ -1064,6 +1064,7 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
> xive->kvm = kvm;
> kvm->arch.xive = xive;
> mutex_init(&xive->mapping_lock);
> + mutex_init(&xive->lock);
>
> /*
> * Allocate a bunch of VPs. KVM_MAX_VCPUS is a large value for
