Messages in this thread | | | Subject | Re: [PATCH v5 2/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer | From | Christian Borntraeger <> | Date | Thu, 17 Jun 2021 14:23:33 +0200 |
| |
On 16.06.21 16:16, Tony Krowiak wrote: > The function pointer to the interception handler for the PQAP instruction > can get changed during the interception process. Let's add a > semaphore to struct kvm_s390_crypto to control read/write access to the > function pointer contained therein. > > The semaphore must be locked for write access by the vfio_ap device driver > when notified that the KVM pointer has been set or cleared. It must be > locked for read access by the interception framework when the PQAP > instruction is intercepted. >
The locking looks so much nicer now. One (optional) thing.
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > arch/s390/include/asm/kvm_host.h | 6 +++--- > arch/s390/kvm/kvm-s390.c | 1 + > arch/s390/kvm/priv.c | 6 +++--- > drivers/s390/crypto/vfio_ap_ops.c | 21 ++++++++++++++++----- > drivers/s390/crypto/vfio_ap_private.h | 2 +- > 5 files changed, 24 insertions(+), 12 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 8925f3969478..58edaa3f9602 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -803,14 +803,14 @@ struct kvm_s390_cpu_model { > unsigned short ibc; > }; > > -struct kvm_s390_module_hook { > +struct kvm_s390_crypto_hook { > int (*hook)(struct kvm_vcpu *vcpu); > - struct module *owner; > };
Can we actually get rid of the structure and use int (*pqap_hook)(struct kvm_vcpu *vcpu); > > struct kvm_s390_crypto { > struct kvm_s390_crypto_cb *crycb; > - struct kvm_s390_module_hook *pqap_hook; > + struct rw_semaphore pqap_hook_rwsem; > + struct kvm_s390_crypto_hook *pqap_hook;
here and below
> __u32 crycbd; > __u8 aes_kw; > __u8 dea_kw; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 1296fc10f80c..418d910df569 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2606,6 +2606,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm) > { > kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb; > kvm_s390_set_crycb_format(kvm); > + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem); > > if (!test_kvm_facility(kvm, 76)) > return; > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 9928f785c677..bbbd84ffe239 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -657,15 +657,15 @@ static int handle_pqap(struct kvm_vcpu *vcpu) > * Verify that the hook callback is registered, lock the owner > * and call the hook. > */ > + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > if (vcpu->kvm->arch.crypto.pqap_hook) { > - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) > - return -EOPNOTSUPP; > ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); ret = vcpu->kvm->arch.crypto.pqap_hook(vcpu);
> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); > if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) > kvm_s390_set_psw_cc(vcpu, 3); > + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > return ret; > } > + up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > /* > * A vfio_driver must register a hook. > * No hook means no driver to enable the SIE CRYCB and no queues. > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 122c85c22469..d8abe5a11e49 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -353,7 +353,6 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev) > init_waitqueue_head(&matrix_mdev->wait_for_kvm); > mdev_set_drvdata(mdev, matrix_mdev); > matrix_mdev->pqap_hook.hook = handle_pqap; > - matrix_mdev->pqap_hook.owner = THIS_MODULE; > mutex_lock(&matrix_dev->lock); > list_add(&matrix_mdev->node, &matrix_dev->mdev_list); > mutex_unlock(&matrix_dev->lock); > @@ -1115,15 +1114,20 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, > } > > kvm_get_kvm(kvm); > + matrix_mdev->kvm = kvm; > matrix_mdev->kvm_busy = true; > mutex_unlock(&matrix_dev->lock); > + > + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; > + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + > kvm_arch_crypto_set_masks(kvm, > matrix_mdev->matrix.apm, > matrix_mdev->matrix.aqm, > matrix_mdev->matrix.adm); > + > mutex_lock(&matrix_dev->lock); > - kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; > - matrix_mdev->kvm = kvm; > matrix_mdev->kvm_busy = false; > wake_up_all(&matrix_mdev->wait_for_kvm); > } > @@ -1189,10 +1193,17 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) > if (matrix_mdev->kvm) { > matrix_mdev->kvm_busy = true; > mutex_unlock(&matrix_dev->lock); > - kvm_arch_crypto_clear_masks(matrix_mdev->kvm); > + > + if (matrix_mdev->kvm->arch.crypto.crycbd) { > + down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; > + up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem); > + > + kvm_arch_crypto_clear_masks(matrix_mdev->kvm); > + } > + > mutex_lock(&matrix_dev->lock); > vfio_ap_mdev_reset_queues(matrix_mdev->mdev); > - matrix_mdev->kvm->arch.crypto.pqap_hook = NULL; > kvm_put_kvm(matrix_mdev->kvm); > matrix_mdev->kvm = NULL; > matrix_mdev->kvm_busy = false; > diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h > index f82a6396acae..5d4fe6efbc73 100644 > --- a/drivers/s390/crypto/vfio_ap_private.h > +++ b/drivers/s390/crypto/vfio_ap_private.h > @@ -86,7 +86,7 @@ struct ap_matrix_mdev { > bool kvm_busy; > wait_queue_head_t wait_for_kvm; > struct kvm *kvm; > - struct kvm_s390_module_hook pqap_hook;
and here?
> + struct kvm_s390_crypto_hook pqap_hook; > struct mdev_device *mdev; > }; > >
| |