* [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification @ 2021-07-19 19:35 Tony Krowiak 2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Tony Krowiak @ 2021-07-19 19:35 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david This series is actually only comprised of a single patch to replace the open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The first patch is included because it is a pre-req slotted to be merged but is not yet available in the kernel. Tony Krowiak (2): s390/vfio-ap: r/w lock for PQAP interception handler function pointer s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification arch/s390/include/asm/kvm_host.h | 8 +- arch/s390/kvm/kvm-s390.c | 28 +++++- arch/s390/kvm/priv.c | 10 +- drivers/s390/crypto/vfio_ap_ops.c | 127 +++++++++----------------- drivers/s390/crypto/vfio_ap_private.h | 4 +- 5 files changed, 77 insertions(+), 100 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak @ 2021-07-19 19:35 ` Tony Krowiak 2021-08-18 17:03 ` Christian Borntraeger 2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak 2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak 2 siblings, 1 reply; 42+ messages in thread From: Tony Krowiak @ 2021-07-19 19:35 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david 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. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> --- arch/s390/include/asm/kvm_host.h | 8 +++----- arch/s390/kvm/kvm-s390.c | 1 + arch/s390/kvm/priv.c | 10 ++++++---- drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ drivers/s390/crypto/vfio_ap_private.h | 2 +- 5 files changed, 28 insertions(+), 16 deletions(-) diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 9b4473f76e56..f18849d259e6 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model { unsigned short ibc; }; -struct kvm_s390_module_hook { - int (*hook)(struct kvm_vcpu *vcpu); - struct module *owner; -}; +typedef int (*crypto_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; + crypto_hook *pqap_hook; __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 b655a7d82bf0..a08f242a9f27 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2630,6 +2630,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..6bed9406c1f3 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) static int handle_pqap(struct kvm_vcpu *vcpu) { struct ap_queue_status status = {}; + crypto_hook pqap_hook; unsigned long reg0; int ret; uint8_t fc; @@ -657,15 +658,16 @@ 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); - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; + ret = pqap_hook(vcpu); 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..742277bc8d1c 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev) vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix); 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; + matrix_mdev->pqap_hook = handle_pqap; 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..e12218e5a629 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; + crypto_hook pqap_hook; struct mdev_device *mdev; }; -- 2.30.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak @ 2021-08-18 17:03 ` Christian Borntraeger 2021-08-18 23:25 ` Halil Pasic 2021-08-19 13:20 ` Tony Krowiak 0 siblings, 2 replies; 42+ messages in thread From: Christian Borntraeger @ 2021-08-18 17:03 UTC (permalink / raw) To: Tony Krowiak, linux-s390, linux-kernel Cc: cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On 19.07.21 21:35, 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. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > arch/s390/include/asm/kvm_host.h | 8 +++----- > arch/s390/kvm/kvm-s390.c | 1 + > arch/s390/kvm/priv.c | 10 ++++++---- > drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ > drivers/s390/crypto/vfio_ap_private.h | 2 +- > 5 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 9b4473f76e56..f18849d259e6 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model { > unsigned short ibc; > }; > > -struct kvm_s390_module_hook { > - int (*hook)(struct kvm_vcpu *vcpu); > - struct module *owner; > -}; > +typedef int (*crypto_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; > + crypto_hook *pqap_hook; > __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 b655a7d82bf0..a08f242a9f27 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2630,6 +2630,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..6bed9406c1f3 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) > static int handle_pqap(struct kvm_vcpu *vcpu) > { > struct ap_queue_status status = {}; > + crypto_hook pqap_hook; > unsigned long reg0; > int ret; > uint8_t fc; > @@ -657,15 +658,16 @@ 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); > - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); > + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; Dont we have to check for NULL here? If not can you add a comment why? Otherwise this looks good. > + ret = pqap_hook(vcpu); [...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-18 17:03 ` Christian Borntraeger @ 2021-08-18 23:25 ` Halil Pasic 2021-08-19 6:56 ` Christian Borntraeger 2021-08-19 13:36 ` Tony Krowiak 2021-08-19 13:20 ` Tony Krowiak 1 sibling, 2 replies; 42+ messages in thread From: Halil Pasic @ 2021-08-18 23:25 UTC (permalink / raw) To: Christian Borntraeger Cc: Tony Krowiak, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On Wed, 18 Aug 2021 19:03:33 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 19.07.21 21:35, 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. > > > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > arch/s390/include/asm/kvm_host.h | 8 +++----- > > arch/s390/kvm/kvm-s390.c | 1 + > > arch/s390/kvm/priv.c | 10 ++++++---- > > drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ > > drivers/s390/crypto/vfio_ap_private.h | 2 +- > > 5 files changed, 28 insertions(+), 16 deletions(-) > > > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > > index 9b4473f76e56..f18849d259e6 100644 > > --- a/arch/s390/include/asm/kvm_host.h > > +++ b/arch/s390/include/asm/kvm_host.h > > @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model { > > unsigned short ibc; > > }; > > > > -struct kvm_s390_module_hook { > > - int (*hook)(struct kvm_vcpu *vcpu); > > - struct module *owner; > > -}; > > +typedef int (*crypto_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; > > + crypto_hook *pqap_hook; > > __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 b655a7d82bf0..a08f242a9f27 100644 > > --- a/arch/s390/kvm/kvm-s390.c > > +++ b/arch/s390/kvm/kvm-s390.c > > @@ -2630,6 +2630,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..6bed9406c1f3 100644 > > --- a/arch/s390/kvm/priv.c > > +++ b/arch/s390/kvm/priv.c > > @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) > > static int handle_pqap(struct kvm_vcpu *vcpu) > > { > > struct ap_queue_status status = {}; > > + crypto_hook pqap_hook; > > unsigned long reg0; > > int ret; > > uint8_t fc; > > @@ -657,15 +658,16 @@ 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) { <--- HERE > > - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) > > - return -EOPNOTSUPP; > > - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); > > - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); > > + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > > Dont we have to check for NULL here? If not can you add a comment why? I believe we did the necessary check on the line I just marked with "<--- HERE". I find that "*" operator confusing in this context as it doesn't do any good for us. I believe this situation is described in 6.5.3.2.4 of the c11 standard. For convenience I will cite from the corresponding draft: "The unary * operator denotes indirection. If the operand points to a function, the result is a function designator; if it points to an object, the result is an lvalue designating the object. If the operand has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an invalid value has been assigned to the pointer, the behavior of the unary * operator is undefined." Frankly I also fail to see the benefit of introducing the local variable named "pqap_hook", but back then I decided to not complain about style. Regards, Halil > > > > + ret = pqap_hook(vcpu); > [...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-18 23:25 ` Halil Pasic @ 2021-08-19 6:56 ` Christian Borntraeger 2021-08-19 13:36 ` Tony Krowiak 1 sibling, 0 replies; 42+ messages in thread From: Christian Borntraeger @ 2021-08-19 6:56 UTC (permalink / raw) To: Halil Pasic Cc: Tony Krowiak, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On 19.08.21 01:25, Halil Pasic wrote: > On Wed, 18 Aug 2021 19:03:33 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 19.07.21 21:35, 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. >>> >>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> >>> --- >>> arch/s390/include/asm/kvm_host.h | 8 +++----- >>> arch/s390/kvm/kvm-s390.c | 1 + >>> arch/s390/kvm/priv.c | 10 ++++++---- >>> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ >>> drivers/s390/crypto/vfio_ap_private.h | 2 +- >>> 5 files changed, 28 insertions(+), 16 deletions(-) >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>> index 9b4473f76e56..f18849d259e6 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model { >>> unsigned short ibc; >>> }; >>> >>> -struct kvm_s390_module_hook { >>> - int (*hook)(struct kvm_vcpu *vcpu); >>> - struct module *owner; >>> -}; >>> +typedef int (*crypto_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; >>> + crypto_hook *pqap_hook; >>> __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 b655a7d82bf0..a08f242a9f27 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -2630,6 +2630,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..6bed9406c1f3 100644 >>> --- a/arch/s390/kvm/priv.c >>> +++ b/arch/s390/kvm/priv.c >>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) >>> static int handle_pqap(struct kvm_vcpu *vcpu) >>> { >>> struct ap_queue_status status = {}; >>> + crypto_hook pqap_hook; >>> unsigned long reg0; >>> int ret; >>> uint8_t fc; >>> @@ -657,15 +658,16 @@ 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) { <--- HERE >>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) >>> - return -EOPNOTSUPP; >>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); >>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); >>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; >> >> Dont we have to check for NULL here? If not can you add a comment why? > > I believe we did the necessary check on the line I just marked with > "<--- HERE". Right, I missed that. Then Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > > I find that "*" operator confusing in this context as it doesn't do > any good for us. I believe this situation is described in 6.5.3.2.4 of > the c11 standard. For convenience I will cite from the corresponding > draft: > "The unary * operator denotes indirection. If the operand points to a > function, the result is a function designator; if it points to an > object, the result is an lvalue designating the object. If the operand > has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an > invalid value has been assigned to the pointer, the behavior of the > unary * operator is undefined." > > Frankly I also fail to see the benefit of introducing the local variable > named "pqap_hook", but back then I decided to not complain about style. Right we can probably do better, but I would rather have the functional fix in now. > > Regards, > Halil > >> >> >>> + ret = pqap_hook(vcpu); >> [...] > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-18 23:25 ` Halil Pasic 2021-08-19 6:56 ` Christian Borntraeger @ 2021-08-19 13:36 ` Tony Krowiak 2021-08-19 21:42 ` Halil Pasic 1 sibling, 1 reply; 42+ messages in thread From: Tony Krowiak @ 2021-08-19 13:36 UTC (permalink / raw) To: Halil Pasic, Christian Borntraeger Cc: linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On 8/18/21 7:25 PM, Halil Pasic wrote: > On Wed, 18 Aug 2021 19:03:33 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 19.07.21 21:35, 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. >>> >>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> >>> --- >>> arch/s390/include/asm/kvm_host.h | 8 +++----- >>> arch/s390/kvm/kvm-s390.c | 1 + >>> arch/s390/kvm/priv.c | 10 ++++++---- >>> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ >>> drivers/s390/crypto/vfio_ap_private.h | 2 +- >>> 5 files changed, 28 insertions(+), 16 deletions(-) >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>> index 9b4473f76e56..f18849d259e6 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model { >>> unsigned short ibc; >>> }; >>> >>> -struct kvm_s390_module_hook { >>> - int (*hook)(struct kvm_vcpu *vcpu); >>> - struct module *owner; >>> -}; >>> +typedef int (*crypto_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; >>> + crypto_hook *pqap_hook; >>> __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 b655a7d82bf0..a08f242a9f27 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -2630,6 +2630,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..6bed9406c1f3 100644 >>> --- a/arch/s390/kvm/priv.c >>> +++ b/arch/s390/kvm/priv.c >>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) >>> static int handle_pqap(struct kvm_vcpu *vcpu) >>> { >>> struct ap_queue_status status = {}; >>> + crypto_hook pqap_hook; >>> unsigned long reg0; >>> int ret; >>> uint8_t fc; >>> @@ -657,15 +658,16 @@ 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) { <--- HERE >>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) >>> - return -EOPNOTSUPP; >>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); >>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); >>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; >> Dont we have to check for NULL here? If not can you add a comment why? > I believe we did the necessary check on the line I just marked with > "<--- HERE". > > I find that "*" operator confusing in this context as it doesn't do > any good for us. I believe this situation is described in 6.5.3.2.4 of > the c11 standard. For convenience I will cite from the corresponding > draft: > "The unary * operator denotes indirection. If the operand points to a > function, the result is a function designator; if it points to an > object, the result is an lvalue designating the object. If the operand > has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an > invalid value has been assigned to the pointer, the behavior of the > unary * operator is undefined." > > Frankly I also fail to see the benefit of introducing the local variable > named "pqap_hook", but back then I decided to not complain about style. The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function pointer. The actual function pointer is stored in matrix_mdev->pqap_hook, the reason being that the handle_pqap function in vfio_ap_ops.c retrieves the matrix_mdev via a container_of macro. The dereferencing of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was to get the function pointer. There may have been a more stylish way of doing this, but the functionality is there. > > Regards, > Halil > >> >>> + ret = pqap_hook(vcpu); >> [...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-19 13:36 ` Tony Krowiak @ 2021-08-19 21:42 ` Halil Pasic 2021-08-23 13:08 ` Tony Krowiak 0 siblings, 1 reply; 42+ messages in thread From: Halil Pasic @ 2021-08-19 21:42 UTC (permalink / raw) To: Tony Krowiak Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On Thu, 19 Aug 2021 09:36:34 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>> static int handle_pqap(struct kvm_vcpu *vcpu) > >>> { > >>> struct ap_queue_status status = {}; > >>> + crypto_hook pqap_hook; > >>> unsigned long reg0; > >>> int ret; > >>> uint8_t fc; > >>> @@ -657,15 +658,16 @@ 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) { <--- HERE > >>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) > >>> - return -EOPNOTSUPP; > >>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); > >>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); > >>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > >> Dont we have to check for NULL here? If not can you add a comment why? > > I believe we did the necessary check on the line I just marked with > > "<--- HERE". > > > > I find that "*" operator confusing in this context as it doesn't do > > any good for us. I believe this situation is described in 6.5.3.2.4 of > > the c11 standard. For convenience I will cite from the corresponding > > draft: > > "The unary * operator denotes indirection. If the operand points to a > > function, the result is a function designator; if it points to an > > object, the result is an lvalue designating the object. If the operand > > has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an > > invalid value has been assigned to the pointer, the behavior of the > > unary * operator is undefined." > > > > Frankly I also fail to see the benefit of introducing the local variable > > named "pqap_hook", but back then I decided to not complain about style. > > The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function > pointer. The actual function pointer is stored in matrix_mdev->pqap_hook, > the reason being that the handle_pqap function in vfio_ap_ops.c > retrieves the matrix_mdev via a container_of macro. The dereferencing > of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was > to get the function pointer. There may have been a more stylish > way of doing this, but the functionality is there. You are right, and I was wrong. But then we do have to distinct pointer deferences, and we check for NULL only once. I still do believe we do not have a potential null pointer dereference here, but the reason for that is that vfio-ap (the party that manages these pointers) guarantees that whenever vcpu->kvm->arch.crypto.pqap_hook != NULL is true, *vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that the function pointer is a valid one). Which is the case, because we set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch it any more. In my opinion it is worth a comment. > > > > > Regards, > > Halil > > > >> > >>> + ret = pqap_hook(vcpu); BTW the second dereference takes place here. If we wanted, we could make sure we don't dereference a null pointer here but I think that would be an overkill. Regards, Halil > >> [...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-19 21:42 ` Halil Pasic @ 2021-08-23 13:08 ` Tony Krowiak 0 siblings, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-23 13:08 UTC (permalink / raw) To: Halil Pasic Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On 8/19/21 5:42 PM, Halil Pasic wrote: > On Thu, 19 Aug 2021 09:36:34 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>>>> static int handle_pqap(struct kvm_vcpu *vcpu) >>>>> { >>>>> struct ap_queue_status status = {}; >>>>> + crypto_hook pqap_hook; >>>>> unsigned long reg0; >>>>> int ret; >>>>> uint8_t fc; >>>>> @@ -657,15 +658,16 @@ 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) { <--- HERE >>>>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner)) >>>>> - return -EOPNOTSUPP; >>>>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu); >>>>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); >>>>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; >>>> Dont we have to check for NULL here? If not can you add a comment why? >>> I believe we did the necessary check on the line I just marked with >>> "<--- HERE". >>> >>> I find that "*" operator confusing in this context as it doesn't do >>> any good for us. I believe this situation is described in 6.5.3.2.4 of >>> the c11 standard. For convenience I will cite from the corresponding >>> draft: >>> "The unary * operator denotes indirection. If the operand points to a >>> function, the result is a function designator; if it points to an >>> object, the result is an lvalue designating the object. If the operand >>> has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an >>> invalid value has been assigned to the pointer, the behavior of the >>> unary * operator is undefined." >>> >>> Frankly I also fail to see the benefit of introducing the local variable >>> named "pqap_hook", but back then I decided to not complain about style. >> The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function >> pointer. The actual function pointer is stored in matrix_mdev->pqap_hook, >> the reason being that the handle_pqap function in vfio_ap_ops.c >> retrieves the matrix_mdev via a container_of macro. The dereferencing >> of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was >> to get the function pointer. There may have been a more stylish >> way of doing this, but the functionality is there. > You are right, and I was wrong. But then we do have to distinct pointer > deferences, and we check for NULL only once. > > I still do believe we do not have a potential null pointer dereference > here, but the reason for that is that vfio-ap (the party that manages > these pointers) guarantees that whenever > vcpu->kvm->arch.crypto.pqap_hook != NULL is true, > *vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that > the function pointer is a valid one). Which is the case, because we > set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch > it any more. > > In my opinion it is worth a comment. Even I had to look at it again to respond to you, so a comment is probably called for. > > >>> Regards, >>> Halil >>> >>>> >>>>> + ret = pqap_hook(vcpu); > BTW the second dereference takes place here. > > If we wanted, we could make sure we don't dereference a null pointer > here but I think that would be an overkill. I agree, it is overkill. > > Regards, > Halil >>>> [...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-18 17:03 ` Christian Borntraeger 2021-08-18 23:25 ` Halil Pasic @ 2021-08-19 13:20 ` Tony Krowiak 2021-08-19 17:54 ` Alex Williamson 1 sibling, 1 reply; 42+ messages in thread From: Tony Krowiak @ 2021-08-19 13:20 UTC (permalink / raw) To: Christian Borntraeger, linux-s390, linux-kernel Cc: cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On 8/18/21 1:03 PM, Christian Borntraeger wrote: > On 19.07.21 21:35, 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. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> >> --- >> arch/s390/include/asm/kvm_host.h | 8 +++----- >> arch/s390/kvm/kvm-s390.c | 1 + >> arch/s390/kvm/priv.c | 10 ++++++---- >> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ >> drivers/s390/crypto/vfio_ap_private.h | 2 +- >> 5 files changed, 28 insertions(+), 16 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h >> b/arch/s390/include/asm/kvm_host.h >> index 9b4473f76e56..f18849d259e6 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model { >> unsigned short ibc; >> }; >> -struct kvm_s390_module_hook { >> - int (*hook)(struct kvm_vcpu *vcpu); >> - struct module *owner; >> -}; >> +typedef int (*crypto_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; >> + crypto_hook *pqap_hook; >> __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 b655a7d82bf0..a08f242a9f27 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2630,6 +2630,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..6bed9406c1f3 100644 >> --- a/arch/s390/kvm/priv.c >> +++ b/arch/s390/kvm/priv.c >> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) >> static int handle_pqap(struct kvm_vcpu *vcpu) >> { >> struct ap_queue_status status = {}; >> + crypto_hook pqap_hook; >> unsigned long reg0; >> int ret; >> uint8_t fc; >> @@ -657,15 +658,16 @@ 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); >> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); >> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > > Dont we have to check for NULL here? If not can you add a comment why? Take a look above the removed lines: if (vcpu->kvm->arch.crypto.pqap_hook) > > Otherwise this looks good. Also, in the cover letter I said this patch was already queued and was included here because it pre-reqs the second patch. Is this patch not already in Alex's tree? > > >> + ret = pqap_hook(vcpu); > [...] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-19 13:20 ` Tony Krowiak @ 2021-08-19 17:54 ` Alex Williamson 2021-08-19 17:58 ` Jason Gunthorpe 0 siblings, 1 reply; 42+ messages in thread From: Alex Williamson @ 2021-08-19 17:54 UTC (permalink / raw) To: Tony Krowiak Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, kwankhede, david On Thu, 19 Aug 2021 09:20:28 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 8/18/21 1:03 PM, Christian Borntraeger wrote: > > On 19.07.21 21:35, 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. > >> > >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > >> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > >> --- > >> arch/s390/include/asm/kvm_host.h | 8 +++----- > >> arch/s390/kvm/kvm-s390.c | 1 + > >> arch/s390/kvm/priv.c | 10 ++++++---- > >> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------ > >> drivers/s390/crypto/vfio_ap_private.h | 2 +- > >> 5 files changed, 28 insertions(+), 16 deletions(-) > >> > >> diff --git a/arch/s390/include/asm/kvm_host.h > >> b/arch/s390/include/asm/kvm_host.h > >> index 9b4473f76e56..f18849d259e6 100644 > >> --- a/arch/s390/include/asm/kvm_host.h > >> +++ b/arch/s390/include/asm/kvm_host.h > >> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model { > >> unsigned short ibc; > >> }; > >> -struct kvm_s390_module_hook { > >> - int (*hook)(struct kvm_vcpu *vcpu); > >> - struct module *owner; > >> -}; > >> +typedef int (*crypto_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; > >> + crypto_hook *pqap_hook; > >> __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 b655a7d82bf0..a08f242a9f27 100644 > >> --- a/arch/s390/kvm/kvm-s390.c > >> +++ b/arch/s390/kvm/kvm-s390.c > >> @@ -2630,6 +2630,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..6bed9406c1f3 100644 > >> --- a/arch/s390/kvm/priv.c > >> +++ b/arch/s390/kvm/priv.c > >> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu) > >> static int handle_pqap(struct kvm_vcpu *vcpu) > >> { > >> struct ap_queue_status status = {}; > >> + crypto_hook pqap_hook; > >> unsigned long reg0; > >> int ret; > >> uint8_t fc; > >> @@ -657,15 +658,16 @@ 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); > >> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner); > >> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > > > > Dont we have to check for NULL here? If not can you add a comment why? > > Take a look above the removed lines: if (vcpu->kvm->arch.crypto.pqap_hook) > > > > > Otherwise this looks good. > > Also, in the cover letter I said this patch was already queued and was > included here because it pre-reqs the second patch. Is this patch not > already in Alex's tree? Nope. The only requests for merges through my tree that I'm aware of were [1] and what I understand was the evolution of that here now [2]. Maybe you're thinking of [3], which I do see in mainline where this was 2/2 in that series but afaict only patch 1/2 was committed. I guess that explains why there was no respin based on comments for this patch. Thanks, Alex [1]https://lore.kernel.org/linux-s390/9c50fb1b-4574-0cfc-487c-64108d97ed73@de.ibm.com/ [2]https://lore.kernel.org/linux-s390/6d64bd83-1519-6065-a4cd-9356c6be5d1a@de.ibm.com/ [3]https://lore.kernel.org/linux-s390/e809be5b-0b24-34dc-1eae-82b58dc54545@de.ibm.com/ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-19 17:54 ` Alex Williamson @ 2021-08-19 17:58 ` Jason Gunthorpe 2021-08-20 15:59 ` Tony Krowiak 2021-08-20 22:05 ` Tony Krowiak 0 siblings, 2 replies; 42+ messages in thread From: Jason Gunthorpe @ 2021-08-19 17:58 UTC (permalink / raw) To: Alex Williamson Cc: Tony Krowiak, Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, kwankhede, david On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote: > Nope. The only requests for merges through my tree that I'm aware of > were [1] and what I understand was the evolution of that here now [2]. > Maybe you're thinking of [3], which I do see in mainline where this was > 2/2 in that series but afaict only patch 1/2 was committed. I guess > that explains why there was no respin based on comments for this patch. > Thanks, Tony, If you take Alex's tree from here: https://github.com/awilliam/linux-vfio/commits/next And rebase + repost exactly the patches you need applied it would be helpful. Jason ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-19 17:58 ` Jason Gunthorpe @ 2021-08-20 15:59 ` Tony Krowiak 2021-08-20 22:05 ` Tony Krowiak 1 sibling, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-20 15:59 UTC (permalink / raw) To: Jason Gunthorpe, Alex Williamson Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, kwankhede, david On 8/19/21 1:58 PM, Jason Gunthorpe wrote: > On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote: > >> Nope. The only requests for merges through my tree that I'm aware of >> were [1] and what I understand was the evolution of that here now [2]. >> Maybe you're thinking of [3], which I do see in mainline where this was >> 2/2 in that series but afaict only patch 1/2 was committed. I guess >> that explains why there was no respin based on comments for this patch. >> Thanks, > Tony, > > If you take Alex's tree from here: > > https://github.com/awilliam/linux-vfio/commits/next > > And rebase + repost exactly the patches you need applied it would be > helpful. Will do. > > Jason ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-19 17:58 ` Jason Gunthorpe 2021-08-20 15:59 ` Tony Krowiak @ 2021-08-20 22:05 ` Tony Krowiak 2021-08-20 22:30 ` Jason Gunthorpe 2021-08-20 22:41 ` Alex Williamson 1 sibling, 2 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-20 22:05 UTC (permalink / raw) To: Jason Gunthorpe, Alex Williamson Cc: Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, kwankhede, david On 8/19/21 1:58 PM, Jason Gunthorpe wrote: > On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote: > >> Nope. The only requests for merges through my tree that I'm aware of >> were [1] and what I understand was the evolution of that here now [2]. >> Maybe you're thinking of [3], which I do see in mainline where this was >> 2/2 in that series but afaict only patch 1/2 was committed. I guess >> that explains why there was no respin based on comments for this patch. >> Thanks, > Tony, > > If you take Alex's tree from here: > > https://github.com/awilliam/linux-vfio/commits/next I navigated to this URL and clicked the green 'Code' button. I was given the option to download the zip file or use git to checkout the code at the URL displayed 'https://github.com/awilliam/linux-vfio.git'. I cloned the repo at that URL and the code was definitely not in any way similar to my code base. In particular, the arch/s390/include/asm/kvm_host.h file did not have any of the crypto structures. I then downloaded the zip file and expanded it. The code looked legitimate, but this was not a git repository, so I had no way to cherry-pick my patches nor format patches to post to this mailing list. Next, I tried cloning from 'https://github.com/awilliam/linux-vfio-next.git', but I was prompted for uid/pw. So, the question is, how to I get the linux-vfio-next repo upon which I can rebase my patches? I apologize for my ignorance. > > And rebase + repost exactly the patches you need applied it would be > helpful. > > Jason ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-20 22:05 ` Tony Krowiak @ 2021-08-20 22:30 ` Jason Gunthorpe 2021-08-23 15:17 ` Tony Krowiak 2021-08-20 22:41 ` Alex Williamson 1 sibling, 1 reply; 42+ messages in thread From: Jason Gunthorpe @ 2021-08-20 22:30 UTC (permalink / raw) To: Tony Krowiak Cc: Alex Williamson, Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, kwankhede, david On Fri, Aug 20, 2021 at 06:05:08PM -0400, Tony Krowiak wrote: > So, the question is, how to I get the linux-vfio-next repo upon which I > can rebase my patches? I apologize for my ignorance. Get yourself a kernel git tree somehow, eg by cloning one you already have Then something like $ git fetch https://github.com/awilliam/linux-vfio.git next $ git reset --hard FETCH_HEAD Will sort it out, though there are many other varients such as adding a remote/etc. When you cloned it from github git checked out the wrong branch for you - 'git reset --hard origin/next' would fix it too. Jason ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-20 22:30 ` Jason Gunthorpe @ 2021-08-23 15:17 ` Tony Krowiak 0 siblings, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-23 15:17 UTC (permalink / raw) To: Jason Gunthorpe Cc: Alex Williamson, Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, kwankhede, david Thanks, I think I've got it now. On 8/20/21 6:30 PM, Jason Gunthorpe wrote: > git reset --hard origin/next ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-20 22:05 ` Tony Krowiak 2021-08-20 22:30 ` Jason Gunthorpe @ 2021-08-20 22:41 ` Alex Williamson 2021-08-23 20:51 ` Tony Krowiak 1 sibling, 1 reply; 42+ messages in thread From: Alex Williamson @ 2021-08-20 22:41 UTC (permalink / raw) To: Tony Krowiak Cc: Jason Gunthorpe, Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, kwankhede, david On Fri, 20 Aug 2021 18:05:08 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 8/19/21 1:58 PM, Jason Gunthorpe wrote: > > On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote: > > > >> Nope. The only requests for merges through my tree that I'm aware of > >> were [1] and what I understand was the evolution of that here now [2]. > >> Maybe you're thinking of [3], which I do see in mainline where this was > >> 2/2 in that series but afaict only patch 1/2 was committed. I guess > >> that explains why there was no respin based on comments for this patch. > >> Thanks, > > Tony, > > > > If you take Alex's tree from here: > > > > https://github.com/awilliam/linux-vfio/commits/next > > I navigated to this URL and clicked the green 'Code' > button. I was given the option to download the zip file or > use git to checkout the code at the URL displayed > 'https://github.com/awilliam/linux-vfio.git'. I cloned the > repo at that URL and the code was definitely not in any > way similar to my code base. In particular, the > arch/s390/include/asm/kvm_host.h file did not have any > of the crypto structures. > > I then downloaded the zip file and expanded it. The code > looked legitimate, but this was not a git repository, so I > had no way to cherry-pick my patches nor format patches > to post to this mailing list. > > Next, I tried cloning from > 'https://github.com/awilliam/linux-vfio-next.git', > but I was prompted for uid/pw. > > So, the question is, how to I get the linux-vfio-next repo upon which I > can rebase my patches? I apologize for my ignorance. You can use git fetch to download the objects, ex: $ git fetch git://github.com/awilliam/linux-vfio.git next $ git checkout FETCH_HEAD Or you could add a remote, ex: $ git remote add vfio git://github.com/awilliam/linux-vfio.git $ git remote update vfio $ git checkout vfio/next The former might be easier and add a lot less crufty objects to your local tree if this is a one-off activity. Thanks, Alex ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer 2021-08-20 22:41 ` Alex Williamson @ 2021-08-23 20:51 ` Tony Krowiak 0 siblings, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-23 20:51 UTC (permalink / raw) To: Alex Williamson Cc: Jason Gunthorpe, Christian Borntraeger, linux-s390, linux-kernel, cohuck, pasic, jjherne, kwankhede, david On 8/20/21 6:41 PM, Alex Williamson wrote: > On Fri, 20 Aug 2021 18:05:08 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 8/19/21 1:58 PM, Jason Gunthorpe wrote: >>> On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote: >>> >>>> Nope. The only requests for merges through my tree that I'm aware of >>>> were [1] and what I understand was the evolution of that here now [2]. >>>> Maybe you're thinking of [3], which I do see in mainline where this was >>>> 2/2 in that series but afaict only patch 1/2 was committed. I guess >>>> that explains why there was no respin based on comments for this patch. >>>> Thanks, >>> Tony, >>> >>> If you take Alex's tree from here: >>> >>> https://github.com/awilliam/linux-vfio/commits/next >> I navigated to this URL and clicked the green 'Code' >> button. I was given the option to download the zip file or >> use git to checkout the code at the URL displayed >> 'https://github.com/awilliam/linux-vfio.git'. I cloned the >> repo at that URL and the code was definitely not in any >> way similar to my code base. In particular, the >> arch/s390/include/asm/kvm_host.h file did not have any >> of the crypto structures. >> >> I then downloaded the zip file and expanded it. The code >> looked legitimate, but this was not a git repository, so I >> had no way to cherry-pick my patches nor format patches >> to post to this mailing list. >> >> Next, I tried cloning from >> 'https://github.com/awilliam/linux-vfio-next.git', >> but I was prompted for uid/pw. >> >> So, the question is, how to I get the linux-vfio-next repo upon which I >> can rebase my patches? I apologize for my ignorance. > You can use git fetch to download the objects, ex: > > $ git fetch git://github.com/awilliam/linux-vfio.git next > $ git checkout FETCH_HEAD > > Or you could add a remote, ex: > > $ git remote add vfio git://github.com/awilliam/linux-vfio.git > $ git remote update vfio > $ git checkout vfio/next > > The former might be easier and add a lot less crufty objects to your > local tree if this is a one-off activity. Thanks, > > Alex Thanks Alex, I was able to get the repo, cherry-pick my patches and build and test the kernel. Barring any anomalies, I will be posting the patches today. > ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak 2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak @ 2021-07-19 19:35 ` Tony Krowiak 2021-07-21 14:45 ` Halil Pasic 2021-07-21 19:37 ` Jason J. Herne 2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak 2 siblings, 2 replies; 42+ messages in thread From: Tony Krowiak @ 2021-07-19 19:35 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david It was pointed out during an unrelated patch review that locks should not be open coded - i.e., writing the algorithm of a standard lock in a function instead of using a lock from the standard library. The setting and testing of a busy flag and sleeping on a wait_event is the same thing a lock does. The open coded locks are invisible to lockdep, so potential locking problems are not detected. This patch removes the open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag and wait queue were introduced to resolve a possible circular locking dependency reported by lockdep when starting a secure execution guest configured with AP adapters and domains. Reversing the order in which the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the issue reported by lockdep, thus enabling the removal of the open coded locks. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> --- arch/s390/kvm/kvm-s390.c | 27 +++++- drivers/s390/crypto/vfio_ap_ops.c | 132 ++++++++------------------ drivers/s390/crypto/vfio_ap_private.h | 2 - 3 files changed, 63 insertions(+), 98 deletions(-) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index a08f242a9f27..4d2ef3a3286e 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm) kvm->arch.crypto.crycbd |= CRYCB_FORMAT1; } +/* + * kvm_arch_crypto_set_masks + * + * @kvm: a pointer to the object containing the crypto masks + * @apm: the mask identifying the accessible AP adapters + * @aqm: the mask identifying the accessible AP domains + * @adm: the mask identifying the accessible AP control domains + * + * Set the masks that identify the adapters, domains and control domains to + * which the KVM guest is granted access. + * + * Note: The kvm->lock mutex must be locked by the caller. + */ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm, unsigned long *aqm, unsigned long *adm) { struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb; - mutex_lock(&kvm->lock); kvm_s390_vcpu_block_all(kvm); switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) { @@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm, /* recreate the shadow crycb for each vcpu */ kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART); kvm_s390_vcpu_unblock_all(kvm); - mutex_unlock(&kvm->lock); } EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks); +/* + * kvm_arch_crypto_set_masks + * + * @kvm: a pointer to the object containing the crypto masks + * + * Clear the masks that identify the adapters, domains and control domains to + * which the KVM guest is granted access. + * + * Note: The kvm->lock mutex must be locked by the caller. + */ void kvm_arch_crypto_clear_masks(struct kvm *kvm) { - mutex_lock(&kvm->lock); kvm_s390_vcpu_block_all(kvm); memset(&kvm->arch.crypto.crycb->apcb0, 0, @@ -2613,7 +2633,6 @@ void kvm_arch_crypto_clear_masks(struct kvm *kvm) /* recreate the shadow crycb for each vcpu */ kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART); kvm_s390_vcpu_unblock_all(kvm); - mutex_unlock(&kvm->lock); } EXPORT_SYMBOL_GPL(kvm_arch_crypto_clear_masks); diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 742277bc8d1c..a9c041d3b95f 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -294,15 +294,6 @@ static int handle_pqap(struct kvm_vcpu *vcpu) matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook, struct ap_matrix_mdev, pqap_hook); - /* - * If the KVM pointer is in the process of being set, wait until the - * process has completed. - */ - wait_event_cmd(matrix_mdev->wait_for_kvm, - !matrix_mdev->kvm_busy, - mutex_unlock(&matrix_dev->lock), - mutex_lock(&matrix_dev->lock)); - /* If the there is no guest using the mdev, there is nothing to do */ if (!matrix_mdev->kvm) goto out_unlock; @@ -350,7 +341,6 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev) matrix_mdev->mdev = mdev; vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix); - init_waitqueue_head(&matrix_mdev->wait_for_kvm); mdev_set_drvdata(mdev, matrix_mdev); matrix_mdev->pqap_hook = handle_pqap; mutex_lock(&matrix_dev->lock); @@ -619,11 +609,8 @@ static ssize_t assign_adapter_store(struct device *dev, mutex_lock(&matrix_dev->lock); - /* - * If the KVM pointer is in flux or the guest is running, disallow - * un-assignment of adapter - */ - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { + /* If the KVM guest is running, disallow assignment of adapter */ + if (matrix_mdev->kvm) { ret = -EBUSY; goto done; } @@ -692,11 +679,8 @@ static ssize_t unassign_adapter_store(struct device *dev, mutex_lock(&matrix_dev->lock); - /* - * If the KVM pointer is in flux or the guest is running, disallow - * un-assignment of adapter - */ - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { + /* If the KVM guest is running, disallow unassignment of adapter */ + if (matrix_mdev->kvm) { ret = -EBUSY; goto done; } @@ -782,11 +766,8 @@ static ssize_t assign_domain_store(struct device *dev, mutex_lock(&matrix_dev->lock); - /* - * If the KVM pointer is in flux or the guest is running, disallow - * assignment of domain - */ - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { + /* If the KVM guest is running, disallow assignment of domain */ + if (matrix_mdev->kvm) { ret = -EBUSY; goto done; } @@ -850,11 +831,8 @@ static ssize_t unassign_domain_store(struct device *dev, mutex_lock(&matrix_dev->lock); - /* - * If the KVM pointer is in flux or the guest is running, disallow - * un-assignment of domain - */ - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { + /* If the KVM guest is running, disallow unassignment of domain */ + if (matrix_mdev->kvm) { ret = -EBUSY; goto done; } @@ -904,11 +882,8 @@ static ssize_t assign_control_domain_store(struct device *dev, mutex_lock(&matrix_dev->lock); - /* - * If the KVM pointer is in flux or the guest is running, disallow - * assignment of control domain. - */ - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { + /* If the KVM guest is running, disallow assignment of control domain */ + if (matrix_mdev->kvm) { ret = -EBUSY; goto done; } @@ -963,11 +938,8 @@ static ssize_t unassign_control_domain_store(struct device *dev, mutex_lock(&matrix_dev->lock); - /* - * If the KVM pointer is in flux or the guest is running, disallow - * un-assignment of control domain. - */ - if (matrix_mdev->kvm_busy || matrix_mdev->kvm) { + /* If a KVM guest is running, disallow unassignment of control domain */ + if (matrix_mdev->kvm) { ret = -EBUSY; goto done; } @@ -1108,28 +1080,30 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, struct ap_matrix_mdev *m; if (kvm->arch.crypto.crycbd) { + down_write(&kvm->arch.crypto.pqap_hook_rwsem); + kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook; + up_write(&kvm->arch.crypto.pqap_hook_rwsem); + + mutex_lock(&kvm->lock); + mutex_lock(&matrix_dev->lock); + list_for_each_entry(m, &matrix_dev->mdev_list, node) { - if (m != matrix_mdev && m->kvm == kvm) + if (m != matrix_mdev && m->kvm == kvm) { + mutex_unlock(&kvm->lock); + mutex_unlock(&matrix_dev->lock); return -EPERM; + } } 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); - matrix_mdev->kvm_busy = false; - wake_up_all(&matrix_mdev->wait_for_kvm); + mutex_unlock(&kvm->lock); + mutex_unlock(&matrix_dev->lock); } return 0; @@ -1179,35 +1153,24 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb, * done under the @matrix_mdev->lock. * */ -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev, + struct kvm *kvm) { - /* - * If the KVM pointer is in the process of being set, wait until the - * process has completed. - */ - wait_event_cmd(matrix_mdev->wait_for_kvm, - !matrix_mdev->kvm_busy, - mutex_unlock(&matrix_dev->lock), - mutex_lock(&matrix_dev->lock)); - - if (matrix_mdev->kvm) { - matrix_mdev->kvm_busy = true; - mutex_unlock(&matrix_dev->lock); - - 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); - } + if (kvm->arch.crypto.crycbd) { + down_write(&kvm->arch.crypto.pqap_hook_rwsem); + kvm->arch.crypto.pqap_hook = NULL; + up_write(&kvm->arch.crypto.pqap_hook_rwsem); + mutex_lock(&kvm->lock); mutex_lock(&matrix_dev->lock); + + kvm_arch_crypto_clear_masks(kvm); vfio_ap_mdev_reset_queues(matrix_mdev->mdev); - kvm_put_kvm(matrix_mdev->kvm); + kvm_put_kvm(kvm); matrix_mdev->kvm = NULL; - matrix_mdev->kvm_busy = false; - wake_up_all(&matrix_mdev->wait_for_kvm); + + mutex_unlock(&kvm->lock); + mutex_unlock(&matrix_dev->lock); } } @@ -1220,16 +1183,13 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, if (action != VFIO_GROUP_NOTIFY_SET_KVM) return NOTIFY_OK; - mutex_lock(&matrix_dev->lock); matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier); if (!data) - vfio_ap_mdev_unset_kvm(matrix_mdev); + vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); else if (vfio_ap_mdev_set_kvm(matrix_mdev, data)) notify_rc = NOTIFY_DONE; - mutex_unlock(&matrix_dev->lock); - return notify_rc; } @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev) { struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); - mutex_lock(&matrix_dev->lock); - vfio_ap_mdev_unset_kvm(matrix_mdev); - mutex_unlock(&matrix_dev->lock); - vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY, &matrix_mdev->iommu_notifier); vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, &matrix_mdev->group_notifier); + vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); module_put(THIS_MODULE); } @@ -1412,15 +1369,6 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev, break; } - /* - * If the KVM pointer is in the process of being set, wait until - * the process has completed. - */ - wait_event_cmd(matrix_mdev->wait_for_kvm, - !matrix_mdev->kvm_busy, - mutex_unlock(&matrix_dev->lock), - mutex_lock(&matrix_dev->lock)); - ret = vfio_ap_mdev_reset_queues(mdev); break; default: diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h index e12218e5a629..22d2e0ca3ae5 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -83,8 +83,6 @@ struct ap_matrix_mdev { struct ap_matrix matrix; struct notifier_block group_notifier; struct notifier_block iommu_notifier; - bool kvm_busy; - wait_queue_head_t wait_for_kvm; struct kvm *kvm; crypto_hook pqap_hook; struct mdev_device *mdev; -- 2.30.2 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak @ 2021-07-21 14:45 ` Halil Pasic 2021-07-22 13:09 ` Tony Krowiak 2021-07-21 19:37 ` Jason J. Herne 1 sibling, 1 reply; 42+ messages in thread From: Halil Pasic @ 2021-07-21 14:45 UTC (permalink / raw) To: Tony Krowiak Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On Mon, 19 Jul 2021 15:35:03 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > It was pointed out during an unrelated patch review that locks should not [..] > -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) > +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev, > + struct kvm *kvm) > { > - /* > - * If the KVM pointer is in the process of being set, wait until the > - * process has completed. > - */ > - wait_event_cmd(matrix_mdev->wait_for_kvm, > - !matrix_mdev->kvm_busy, > - mutex_unlock(&matrix_dev->lock), > - mutex_lock(&matrix_dev->lock)); > - > - if (matrix_mdev->kvm) { We used to check if matrix_mdev->kvm is null, but ... > - matrix_mdev->kvm_busy = true; > - mutex_unlock(&matrix_dev->lock); > - > - 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); > - } > + if (kvm->arch.crypto.crycbd) { ... now we just try to dereference it. And .. > + down_write(&kvm->arch.crypto.pqap_hook_rwsem); > + kvm->arch.crypto.pqap_hook = NULL; > + up_write(&kvm->arch.crypto.pqap_hook_rwsem); > > + mutex_lock(&kvm->lock); > mutex_lock(&matrix_dev->lock); > + > + kvm_arch_crypto_clear_masks(kvm); > vfio_ap_mdev_reset_queues(matrix_mdev->mdev); > - kvm_put_kvm(matrix_mdev->kvm); > + kvm_put_kvm(kvm); > matrix_mdev->kvm = NULL; > - matrix_mdev->kvm_busy = false; > - wake_up_all(&matrix_mdev->wait_for_kvm); > + > + mutex_unlock(&kvm->lock); > + mutex_unlock(&matrix_dev->lock); > } > } > [..] > @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev) > { > struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); > > - mutex_lock(&matrix_dev->lock); > - vfio_ap_mdev_unset_kvm(matrix_mdev); > - mutex_unlock(&matrix_dev->lock); > - .. before access to the matrix_mdev->kvm used to be protected by the matrix_dev->lock ... > vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY, > &matrix_mdev->iommu_notifier); > vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, > &matrix_mdev->group_notifier); > + vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); ... but it is not any more. BTW I don't think the code is guaranteed to fetch ->kvm just once. Can you please explain why can we get away with being more lax when dealing with matrix_mdev->kvm? Regards, Halil [..] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-21 14:45 ` Halil Pasic @ 2021-07-22 13:09 ` Tony Krowiak 2021-07-23 14:26 ` Halil Pasic 0 siblings, 1 reply; 42+ messages in thread From: Tony Krowiak @ 2021-07-22 13:09 UTC (permalink / raw) To: Halil Pasic Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On 7/21/21 10:45 AM, Halil Pasic wrote: > On Mon, 19 Jul 2021 15:35:03 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> It was pointed out during an unrelated patch review that locks should not > [..] > >> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) >> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev, >> + struct kvm *kvm) >> { >> - /* >> - * If the KVM pointer is in the process of being set, wait until the >> - * process has completed. >> - */ >> - wait_event_cmd(matrix_mdev->wait_for_kvm, >> - !matrix_mdev->kvm_busy, >> - mutex_unlock(&matrix_dev->lock), >> - mutex_lock(&matrix_dev->lock)); >> - >> - if (matrix_mdev->kvm) { > We used to check if matrix_mdev->kvm is null, but ... > >> - matrix_mdev->kvm_busy = true; >> - mutex_unlock(&matrix_dev->lock); >> - >> - 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); >> - } >> + if (kvm->arch.crypto.crycbd) { > ... now we just try to dereference it. And .. We used to check matrix_mdev->kvm, now the kvm pointer is passed into the function; however, having said that, the pointer passed in should be checked before de-referencing it. > >> + down_write(&kvm->arch.crypto.pqap_hook_rwsem); >> + kvm->arch.crypto.pqap_hook = NULL; >> + up_write(&kvm->arch.crypto.pqap_hook_rwsem); >> >> + mutex_lock(&kvm->lock); >> mutex_lock(&matrix_dev->lock); >> + >> + kvm_arch_crypto_clear_masks(kvm); >> vfio_ap_mdev_reset_queues(matrix_mdev->mdev); >> - kvm_put_kvm(matrix_mdev->kvm); >> + kvm_put_kvm(kvm); >> matrix_mdev->kvm = NULL; >> - matrix_mdev->kvm_busy = false; >> - wake_up_all(&matrix_mdev->wait_for_kvm); >> + >> + mutex_unlock(&kvm->lock); >> + mutex_unlock(&matrix_dev->lock); >> } >> } >> > [..] > >> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev) >> { >> struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); >> >> - mutex_lock(&matrix_dev->lock); >> - vfio_ap_mdev_unset_kvm(matrix_mdev); >> - mutex_unlock(&matrix_dev->lock); >> - > .. before access to the matrix_mdev->kvm used to be protected by > the matrix_dev->lock ... > >> vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY, >> &matrix_mdev->iommu_notifier); >> vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, >> &matrix_mdev->group_notifier); >> + vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); > ... but it is not any more. BTW I don't think the code is guaranteed > to fetch ->kvm just once. There are a couple of things to point out here: 1. The vfio_ap_mdev_unset_kvm function() is the only place where the matrix_mdev->kvm pointer is cleared. That function is called here as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM events. If you notice in the code above, the group notifier is unregistered before calling the unset function, so either the notifier will have already been invoked and the pointer cleared (which is why you are correct that the KVM pointer passed in needs to get checked in the unset function), or will get cleared here. 2. The release callback is invoked when the mdev fd is closed by userspace. The remove callback is the only place where the matrix_mdev is freed. The remove callback is not called until the mdev fd is released, so it is guaranteed the matrix_mdev will exist when the release callback is invoked. 3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function before doing any operations that modify the matrix_mdev. > Can you please explain why can we get away with being more > lax when dealing with matrix_mdev->kvm? See above. > > Regards, > Halil > > [..] ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-22 13:09 ` Tony Krowiak @ 2021-07-23 14:26 ` Halil Pasic 2021-07-23 21:24 ` Tony Krowiak 0 siblings, 1 reply; 42+ messages in thread From: Halil Pasic @ 2021-07-23 14:26 UTC (permalink / raw) To: Tony Krowiak Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On Thu, 22 Jul 2021 09:09:26 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 7/21/21 10:45 AM, Halil Pasic wrote: > > On Mon, 19 Jul 2021 15:35:03 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > >> It was pointed out during an unrelated patch review that locks should not > > [..] > > > >> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) > >> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev, > >> + struct kvm *kvm) > >> { > >> - /* > >> - * If the KVM pointer is in the process of being set, wait until the > >> - * process has completed. > >> - */ > >> - wait_event_cmd(matrix_mdev->wait_for_kvm, > >> - !matrix_mdev->kvm_busy, > >> - mutex_unlock(&matrix_dev->lock), > >> - mutex_lock(&matrix_dev->lock)); > >> - > >> - if (matrix_mdev->kvm) { > > We used to check if matrix_mdev->kvm is null, but ... > > > >> - matrix_mdev->kvm_busy = true; > >> - mutex_unlock(&matrix_dev->lock); > >> - > >> - 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); > >> - } > >> + if (kvm->arch.crypto.crycbd) { > > ... now we just try to dereference it. And .. > > We used to check matrix_mdev->kvm, now the kvm pointer is passed into > the function; however, having said that, the pointer passed in should be > checked before de-referencing it. > > > > >> + down_write(&kvm->arch.crypto.pqap_hook_rwsem); > >> + kvm->arch.crypto.pqap_hook = NULL; > >> + up_write(&kvm->arch.crypto.pqap_hook_rwsem); > >> > >> + mutex_lock(&kvm->lock); > >> mutex_lock(&matrix_dev->lock); > >> + > >> + kvm_arch_crypto_clear_masks(kvm); > >> vfio_ap_mdev_reset_queues(matrix_mdev->mdev); > >> - kvm_put_kvm(matrix_mdev->kvm); > >> + kvm_put_kvm(kvm); > >> matrix_mdev->kvm = NULL; > >> - matrix_mdev->kvm_busy = false; > >> - wake_up_all(&matrix_mdev->wait_for_kvm); > >> + > >> + mutex_unlock(&kvm->lock); > >> + mutex_unlock(&matrix_dev->lock); > >> } > >> } > >> > > [..] > > > >> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev) > >> { > >> struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); > >> > >> - mutex_lock(&matrix_dev->lock); > >> - vfio_ap_mdev_unset_kvm(matrix_mdev); > >> - mutex_unlock(&matrix_dev->lock); > >> - > > .. before access to the matrix_mdev->kvm used to be protected by > > the matrix_dev->lock ... > > > >> vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY, > >> &matrix_mdev->iommu_notifier); > >> vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, > >> &matrix_mdev->group_notifier); > >> + vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); > > ... but it is not any more. BTW I don't think the code is guaranteed > > to fetch ->kvm just once. > > There are a couple of things to point out here: > 1. The vfio_ap_mdev_unset_kvm function() is the only place where the > matrix_mdev->kvm pointer is cleared. That function is called here > as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM > events. If you notice in the code above, the group notifier is > unregistered > before calling the unset function, so either the notifier will have > already > been invoked and the pointer cleared (which is why you are correct > that the KVM pointer passed in needs to get checked in the unset > function), > or will get cleared here. Hm, vfio_unregister_notifier() indeed seems to guarantee, that by the time it returns no notifer is running. I didn't know that. But this blocking notifier chain uses an rwsem. So mutual exclusion with vfio_ap_mdev_open() is guaranteed, than it is indeed guaranteed. A quick glance at the code didn't tell me if vfio/mdev guarantees that. I mean it would make sense to me to make the init and the cleanup mutually exclusive, but I'm reluctant to just assume it is like that. Can you please point me into the right direction? > 2. The release callback is invoked when the mdev fd is closed by userspace. > The remove callback is the only place where the matrix_mdev is > freed. The > remove callback is not called until the mdev fd is released, so it > is guaranteed > the matrix_mdev will exist when the release callback is invoked. > 3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function > before doing any operations that modify the matrix_mdev. Yeah but both the reader, and the writer needs to use the same lock to have the protected by the lock type of situation. That is why I asked about the place where you read matrix_mdev members outside the matrix_dev->lock. Regards, Halil ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-23 14:26 ` Halil Pasic @ 2021-07-23 21:24 ` Tony Krowiak 2021-07-26 20:36 ` Halil Pasic 0 siblings, 1 reply; 42+ messages in thread From: Tony Krowiak @ 2021-07-23 21:24 UTC (permalink / raw) To: Halil Pasic Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On 7/23/21 10:26 AM, Halil Pasic wrote: > On Thu, 22 Jul 2021 09:09:26 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 7/21/21 10:45 AM, Halil Pasic wrote: >>> On Mon, 19 Jul 2021 15:35:03 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> It was pointed out during an unrelated patch review that locks should not >>> [..] >>> >>>> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) >>>> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev, >>>> + struct kvm *kvm) >>>> { >>>> - /* >>>> - * If the KVM pointer is in the process of being set, wait until the >>>> - * process has completed. >>>> - */ >>>> - wait_event_cmd(matrix_mdev->wait_for_kvm, >>>> - !matrix_mdev->kvm_busy, >>>> - mutex_unlock(&matrix_dev->lock), >>>> - mutex_lock(&matrix_dev->lock)); >>>> - >>>> - if (matrix_mdev->kvm) { >>> We used to check if matrix_mdev->kvm is null, but ... >>> >>>> - matrix_mdev->kvm_busy = true; >>>> - mutex_unlock(&matrix_dev->lock); >>>> - >>>> - 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); >>>> - } >>>> + if (kvm->arch.crypto.crycbd) { >>> ... now we just try to dereference it. And .. >> We used to check matrix_mdev->kvm, now the kvm pointer is passed into >> the function; however, having said that, the pointer passed in should be >> checked before de-referencing it. >> >>> >>>> + down_write(&kvm->arch.crypto.pqap_hook_rwsem); >>>> + kvm->arch.crypto.pqap_hook = NULL; >>>> + up_write(&kvm->arch.crypto.pqap_hook_rwsem); >>>> >>>> + mutex_lock(&kvm->lock); >>>> mutex_lock(&matrix_dev->lock); >>>> + >>>> + kvm_arch_crypto_clear_masks(kvm); >>>> vfio_ap_mdev_reset_queues(matrix_mdev->mdev); >>>> - kvm_put_kvm(matrix_mdev->kvm); >>>> + kvm_put_kvm(kvm); >>>> matrix_mdev->kvm = NULL; >>>> - matrix_mdev->kvm_busy = false; >>>> - wake_up_all(&matrix_mdev->wait_for_kvm); >>>> + >>>> + mutex_unlock(&kvm->lock); >>>> + mutex_unlock(&matrix_dev->lock); >>>> } >>>> } >>>> >>> [..] >>> >>>> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev) >>>> { >>>> struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); >>>> >>>> - mutex_lock(&matrix_dev->lock); >>>> - vfio_ap_mdev_unset_kvm(matrix_mdev); >>>> - mutex_unlock(&matrix_dev->lock); >>>> - >>> .. before access to the matrix_mdev->kvm used to be protected by >>> the matrix_dev->lock ... >>> >>>> vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY, >>>> &matrix_mdev->iommu_notifier); >>>> vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, >>>> &matrix_mdev->group_notifier); >>>> + vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); >>> ... but it is not any more. BTW I don't think the code is guaranteed >>> to fetch ->kvm just once. >> There are a couple of things to point out here: >> 1. The vfio_ap_mdev_unset_kvm function() is the only place where the >> matrix_mdev->kvm pointer is cleared. That function is called here >> as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM >> events. If you notice in the code above, the group notifier is >> unregistered >> before calling the unset function, so either the notifier will have >> already >> been invoked and the pointer cleared (which is why you are correct >> that the KVM pointer passed in needs to get checked in the unset >> function), >> or will get cleared here. > Hm, vfio_unregister_notifier() indeed seems to guarantee, that by the > time it returns no notifer is running. I didn't know that. But this > blocking notifier chain uses an rwsem. So mutual exclusion with > vfio_ap_mdev_open() is guaranteed, than it is indeed guaranteed. A quick > glance at the code didn't tell me if vfio/mdev guarantees that. > > I mean it would make sense to me to make the init and the cleanup > mutually exclusive, but I'm reluctant to just assume it is like that. > Can you please point me into the right direction? I'm not quite sure what you're asking for here, but I'll give it a shot. The notifier is registered by the vfio_ap_mdev_open() callback which is invoked in response to the opening of the mdev fd. Since mdev fd can't be closed unless and until it's open, there is no way for the vfio_ap_mdev_release() callback, which is invoked when the mdev fd is closed, to execute at the same time as the vfio_ap_mdev_open callback. Since the release callback unregisters the notifier and both the registration and unregistration are done under the same rwsem, I don't see how they can be anything but mutually exclusive. Am I missing something here? > > >> 2. The release callback is invoked when the mdev fd is closed by userspace. >> The remove callback is the only place where the matrix_mdev is >> freed. The >> remove callback is not called until the mdev fd is released, so it >> is guaranteed >> the matrix_mdev will exist when the release callback is invoked. >> 3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function >> before doing any operations that modify the matrix_mdev. > Yeah but both the reader, and the writer needs to use the same lock to > have the protected by the lock type of situation. That is why I asked > about the place where you read matrix_mdev members outside the > matrix_dev->lock. With this patch, the matrix_mdev is always written or read with the matrix_dev->lock mutex locked. By moving the locking of the kvm->lock mutex out of the functions that set/clear the masks in the guest's CRYCB, we are now able to lock the kvm->lock mutex before locking the matrix_dev->lock mutex in both the vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm() functions. This precludes the need to unlock the matrix_dev->lock mutex while the masks are being set or cleared and alleviates the lockdep splat for which the open coded locks were created. > > Regards, > Halil ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-23 21:24 ` Tony Krowiak @ 2021-07-26 20:36 ` Halil Pasic 2021-07-26 22:03 ` Jason Gunthorpe 0 siblings, 1 reply; 42+ messages in thread From: Halil Pasic @ 2021-07-26 20:36 UTC (permalink / raw) To: Tony Krowiak Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david On Fri, 23 Jul 2021 17:24:16 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 7/23/21 10:26 AM, Halil Pasic wrote: > > On Thu, 22 Jul 2021 09:09:26 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > >> On 7/21/21 10:45 AM, Halil Pasic wrote: > >>> On Mon, 19 Jul 2021 15:35:03 -0400 > >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>> > >>>> It was pointed out during an unrelated patch review that locks should not > >>> [..] > >>> > >>>> -static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev) > >>>> +static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev, > >>>> + struct kvm *kvm) > >>>> { > >>>> - /* > >>>> - * If the KVM pointer is in the process of being set, wait until the > >>>> - * process has completed. > >>>> - */ > >>>> - wait_event_cmd(matrix_mdev->wait_for_kvm, > >>>> - !matrix_mdev->kvm_busy, > >>>> - mutex_unlock(&matrix_dev->lock), > >>>> - mutex_lock(&matrix_dev->lock)); > >>>> - > >>>> - if (matrix_mdev->kvm) { > >>> We used to check if matrix_mdev->kvm is null, but ... > >>> > >>>> - matrix_mdev->kvm_busy = true; > >>>> - mutex_unlock(&matrix_dev->lock); > >>>> - > >>>> - 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); > >>>> - } > >>>> + if (kvm->arch.crypto.crycbd) { > >>> ... now we just try to dereference it. And .. > >> We used to check matrix_mdev->kvm, now the kvm pointer is passed into > >> the function; however, having said that, the pointer passed in should be > >> checked before de-referencing it. > >> > >>> > >>>> + down_write(&kvm->arch.crypto.pqap_hook_rwsem); > >>>> + kvm->arch.crypto.pqap_hook = NULL; > >>>> + up_write(&kvm->arch.crypto.pqap_hook_rwsem); > >>>> > >>>> + mutex_lock(&kvm->lock); > >>>> mutex_lock(&matrix_dev->lock); > >>>> + > >>>> + kvm_arch_crypto_clear_masks(kvm); > >>>> vfio_ap_mdev_reset_queues(matrix_mdev->mdev); > >>>> - kvm_put_kvm(matrix_mdev->kvm); > >>>> + kvm_put_kvm(kvm); > >>>> matrix_mdev->kvm = NULL; > >>>> - matrix_mdev->kvm_busy = false; > >>>> - wake_up_all(&matrix_mdev->wait_for_kvm); > >>>> + > >>>> + mutex_unlock(&kvm->lock); > >>>> + mutex_unlock(&matrix_dev->lock); > >>>> } > >>>> } > >>>> > >>> [..] > >>> > >>>> @@ -1363,14 +1323,11 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev) > >>>> { > >>>> struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); > >>>> > >>>> - mutex_lock(&matrix_dev->lock); > >>>> - vfio_ap_mdev_unset_kvm(matrix_mdev); > >>>> - mutex_unlock(&matrix_dev->lock); > >>>> - > >>> .. before access to the matrix_mdev->kvm used to be protected by > >>> the matrix_dev->lock ... > >>> > >>>> vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY, > >>>> &matrix_mdev->iommu_notifier); > >>>> vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, > >>>> &matrix_mdev->group_notifier); > >>>> + vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); > >>> ... but it is not any more. BTW I don't think the code is guaranteed > >>> to fetch ->kvm just once. > >> There are a couple of things to point out here: > >> 1. The vfio_ap_mdev_unset_kvm function() is the only place where the > >> matrix_mdev->kvm pointer is cleared. That function is called here > >> as well as by the group notifier callback for VFIO_GROUP_NOTIFY_SET_KVM > >> events. If you notice in the code above, the group notifier is > >> unregistered > >> before calling the unset function, so either the notifier will have > >> already > >> been invoked and the pointer cleared (which is why you are correct > >> that the KVM pointer passed in needs to get checked in the unset > >> function), > >> or will get cleared here. > > Hm, vfio_unregister_notifier() indeed seems to guarantee, that by the > > time it returns no notifer is running. I didn't know that. But this > > blocking notifier chain uses an rwsem. So mutual exclusion with > > vfio_ap_mdev_open() is guaranteed, than it is indeed guaranteed. A quick > > glance at the code didn't tell me if vfio/mdev guarantees that. > > > > I mean it would make sense to me to make the init and the cleanup > > mutually exclusive, but I'm reluctant to just assume it is like that. > > Can you please point me into the right direction? > > I'm not quite sure what you're asking for here, but I'll give it a > shot. The notifier is registered by the vfio_ap_mdev_open() > callback which is invoked in response to the opening of the mdev fd. vfio_ap_mdev_open <-vfio_group_get_device_fd > Since mdev fd can't be closed unless and until it's open, > there is no way for the vfio_ap_mdev_release() callback, which > is invoked when the mdev fd is closed, to execute at the same > time as the vfio_ap_mdev_open callback. A plain old file you can open several times over (and thus close several times over as well). So if you imagine something like: thread A thread B -------- -------- open() close() open() open() close() close() You may end up with open and close running interleaved. What I' trying to say is, to my best knowledge, normally there is no you have to close it before you open it again rule for files. Does that make sense? If there is no special mechanism to prevent such a scenario for the mdev device, I guess, the second open (B) if you like would try to register a group notifier. I'm not aware of such an mechanism. I had a brief look at vfio_group_get_device_fd but couldn't find any such logic there. Maybe Connie can actually help us with this one? > Since the release > callback unregisters the notifier and both the registration and > unregistration are done under the same rwsem, I don't see how > they can be anything but mutually exclusive. Am I missing something > here? > > > > > > >> 2. The release callback is invoked when the mdev fd is closed by userspace. > >> The remove callback is the only place where the matrix_mdev is > >> freed. The > >> remove callback is not called until the mdev fd is released, so it > >> is guaranteed > >> the matrix_mdev will exist when the release callback is invoked. > >> 3. The matrix_dev->lock is then taken in the vfio_ap_mdev_unset_kvm function > >> before doing any operations that modify the matrix_mdev. > > Yeah but both the reader, and the writer needs to use the same lock to > > have the protected by the lock type of situation. That is why I asked > > about the place where you read matrix_mdev members outside the > > matrix_dev->lock. > > With this patch, the matrix_mdev is always written or read with > the matrix_dev->lock mutex locked. Sorry I don't understand. How is the struct matrix_mdev.kvm read with the matrix_dev->lock mutex locked in: static void vfio_ap_mdev_release(struct mdev_device *mdev) { struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY, &matrix_mdev->iommu_notifier); vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, &matrix_mdev->group_notifier); vfio_ap_mdev_unset_kvm(matrix_mdev, matrix_mdev->kvm); <==== HERE! module_put(THIS_MODULE); } > By moving the locking of the > kvm->lock mutex out of the functions that set/clear the masks > in the guest's CRYCB, we are now able to lock the kvm->lock > mutex before locking the matrix_dev->lock mutex in both the > vfio_ap_mdev_set_kvm() and vfio_ap_mdev_unset_kvm() > functions. This precludes the need to unlock the > matrix_dev->lock mutex while the masks are being set or > cleared and alleviates the lockdep splat for which the open coded > locks were created. > > > > > Regards, > > Halil > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-26 20:36 ` Halil Pasic @ 2021-07-26 22:03 ` Jason Gunthorpe 2021-07-26 22:43 ` Halil Pasic 2021-07-27 6:54 ` Christoph Hellwig 0 siblings, 2 replies; 42+ messages in thread From: Jason Gunthorpe @ 2021-07-26 22:03 UTC (permalink / raw) To: Halil Pasic Cc: Tony Krowiak, linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david On Mon, Jul 26, 2021 at 10:36:28PM +0200, Halil Pasic wrote: > You may end up with open and close running interleaved. What I' > trying to say is, to my best knowledge, normally there is no > you have to close it before you open it again rule for files. This is an existing bug in this driver, I've fixed in the reflck series. open_device/close_device will not run concurrently, or out of order, afer it is fixed. Jason ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-26 22:03 ` Jason Gunthorpe @ 2021-07-26 22:43 ` Halil Pasic 2021-07-28 13:43 ` Tony Krowiak 2021-07-27 6:54 ` Christoph Hellwig 1 sibling, 1 reply; 42+ messages in thread From: Halil Pasic @ 2021-07-26 22:43 UTC (permalink / raw) To: Jason Gunthorpe Cc: Tony Krowiak, linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david On Mon, 26 Jul 2021 19:03:17 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > > You may end up with open and close running interleaved. What I' > > trying to say is, to my best knowledge, normally there is no > > you have to close it before you open it again rule for files. > > This is an existing bug in this driver, I've fixed in the reflck series. > > open_device/close_device will not run concurrently, or out of order, > afer it is fixed. Well if that is the case then provided your fix precedes this patch: Acked-by: Halil Pasic <pasic@linux.ibm.com> I'm not entirely happy with this. I did not thoroughly investigate the implications of reversing the locking order of the vfio-ap lock (driver global) and the kvm lock (guest specific). I will trust Tony and hope our KVM maintainers will scream if this is bad from interference and delay perspective. Regards, Halil ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-26 22:43 ` Halil Pasic @ 2021-07-28 13:43 ` Tony Krowiak 2021-07-28 19:42 ` Halil Pasic 0 siblings, 1 reply; 42+ messages in thread From: Tony Krowiak @ 2021-07-28 13:43 UTC (permalink / raw) To: Halil Pasic, Jason Gunthorpe Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david, pbonzini, David Hildenbrand, frankja, imbrenda On 7/26/21 6:43 PM, Halil Pasic wrote: > On Mon, 26 Jul 2021 19:03:17 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > >>> You may end up with open and close running interleaved. What I' >>> trying to say is, to my best knowledge, normally there is no >>> you have to close it before you open it again rule for files. >> This is an existing bug in this driver, I've fixed in the reflck series. >> >> open_device/close_device will not run concurrently, or out of order, >> afer it is fixed. > Well if that is the case then provided your fix precedes this patch: > > Acked-by: Halil Pasic <pasic@linux.ibm.com> > > I'm not entirely happy with this. I did not thoroughly investigate the > implications of reversing the locking order of the vfio-ap lock (driver > global) and the kvm lock (guest specific). I will trust Tony and hope > our KVM maintainers will scream if this is bad from interference and > delay perspective. This solution was suggested by Jason G and it does in fact resolve the lockdep splat encountered when starting an SE guest with access to crypto resources. There is a chance that the KVM lock can get held while waiting for the lock on the matrix_dev->mutex, but this does not seem like a grave concern to me. That would only happen during VFIO_GROUP_NOTIFY_SET_KVM notification - either when the guest is being started or terminated, or when the mdev fd is opened or closed. According to Jason, once he integrates his reflck series, the open/close will happen only once. I've copied the KVM maintainers on this response, so hopefully one of them will provide some input. > > Regards, > Halil ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-28 13:43 ` Tony Krowiak @ 2021-07-28 19:42 ` Halil Pasic 2021-07-30 13:33 ` Tony Krowiak 0 siblings, 1 reply; 42+ messages in thread From: Halil Pasic @ 2021-07-28 19:42 UTC (permalink / raw) To: Tony Krowiak Cc: Jason Gunthorpe, linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david, pbonzini, frankja, imbrenda On Wed, 28 Jul 2021 09:43:03 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > This solution was suggested by Jason G and it does in fact resolve > the lockdep splat encountered when starting an SE guest with > access to crypto resources. There is a chance that the KVM lock > can get held while waiting for the lock on the matrix_dev->mutex, > but this does not seem like a grave concern to me. Yes I agree. I was thinking along the lines: matrix modifications via the sysfs take the matrix_dev->lock so the level of contention may depend on what userspace is doing... Regards, Halil ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-28 19:42 ` Halil Pasic @ 2021-07-30 13:33 ` Tony Krowiak 0 siblings, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-07-30 13:33 UTC (permalink / raw) To: Halil Pasic Cc: Jason Gunthorpe, linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david, pbonzini, frankja, imbrenda On 7/28/21 3:42 PM, Halil Pasic wrote: > On Wed, 28 Jul 2021 09:43:03 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> This solution was suggested by Jason G and it does in fact resolve >> the lockdep splat encountered when starting an SE guest with >> access to crypto resources. There is a chance that the KVM lock >> can get held while waiting for the lock on the matrix_dev->mutex, >> but this does not seem like a grave concern to me. > Yes I agree. I was thinking along the lines: matrix modifications > via the sysfs take the matrix_dev->lock so the level of contention > may depend on what userspace is doing... The probe/remove functions also take the matrix_dev->lock as does the handle_pqap() function. In any case, while all of those are possible, in our implementation of AP queue pass-through, the two functions that take the KVM lock are invoked when the guest is starting or shutting down, or when the mdev is hot plugged/unplugged. For the cases of guest startup/shutdown, it would seem that holding the kvm->lock while waiting for the matrix_dev->lock shouldn't be a big problem since the guest will either not be fully up yet or on its way down. I suppose the hot plug/unplug case could potentially cause the guest vcpus to pause while processing, but how often do you anticipate a hot plug to take place? > > Regards, > Halil ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-26 22:03 ` Jason Gunthorpe 2021-07-26 22:43 ` Halil Pasic @ 2021-07-27 6:54 ` Christoph Hellwig 1 sibling, 0 replies; 42+ messages in thread From: Christoph Hellwig @ 2021-07-27 6:54 UTC (permalink / raw) To: Jason Gunthorpe Cc: Halil Pasic, Tony Krowiak, linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david, kvm On Mon, Jul 26, 2021 at 07:03:17PM -0300, Jason Gunthorpe wrote: > On Mon, Jul 26, 2021 at 10:36:28PM +0200, Halil Pasic wrote: > > > You may end up with open and close running interleaved. What I' > > trying to say is, to my best knowledge, normally there is no > > you have to close it before you open it again rule for files. > > This is an existing bug in this driver, I've fixed in the reflck series. > > open_device/close_device will not run concurrently, or out of order, > afer it is fixed. Btw, while I've got all your attention, I've been struggling a bit with how that SET_KVM notifier is supposed to work. The i915 gvt code simplify assumes the notification registration hits the case of KVM already being active, and gets away with that as at least qemu ensures that the KVM_DEV_VFIO_GROUP_ADD has been called before the device FDs are opened. Is that something we could generalize and never allow to actually notify for SET_KVM with non-null data beeing called at runtime and avoid the locking entirely? Similarly the removal case is a little weird: with Jason's work to only call a release function on the last reference drop which solves a lot of concurrency issues nicely this still creates a nasty corner case with a sideband release under weird locking rules. What prevents the vfio core from simply holding a refefeence to the struct kvm as long as the device is open to close that hole? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak 2021-07-21 14:45 ` Halil Pasic @ 2021-07-21 19:37 ` Jason J. Herne 2021-07-22 13:16 ` Tony Krowiak 1 sibling, 1 reply; 42+ messages in thread From: Jason J. Herne @ 2021-07-21 19:37 UTC (permalink / raw) To: Tony Krowiak, linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jgg, alex.williamson, kwankhede, david On 7/19/21 3:35 PM, Tony Krowiak wrote: > It was pointed out during an unrelated patch review that locks should not > be open coded - i.e., writing the algorithm of a standard lock in a > function instead of using a lock from the standard library. The setting and > testing of a busy flag and sleeping on a wait_event is the same thing > a lock does. The open coded locks are invisible to lockdep, so potential > locking problems are not detected. > > This patch removes the open coded locks used during > VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag > and wait queue were introduced to resolve a possible circular locking > dependency reported by lockdep when starting a secure execution guest > configured with AP adapters and domains. Reversing the order in which > the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the > issue reported by lockdep, thus enabling the removal of the open coded > locks. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 27 +++++- > drivers/s390/crypto/vfio_ap_ops.c | 132 ++++++++------------------ > drivers/s390/crypto/vfio_ap_private.h | 2 - > 3 files changed, 63 insertions(+), 98 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index a08f242a9f27..4d2ef3a3286e 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm) > kvm->arch.crypto.crycbd |= CRYCB_FORMAT1; > } > > +/* > + * kvm_arch_crypto_set_masks > + * > + * @kvm: a pointer to the object containing the crypto masks This should probably say "a pointer to the target guest's KVM struct" or something to that effect. Same comment for the comment above kvm_arch_crypto_clear_masks. > + * @apm: the mask identifying the accessible AP adapters > + * @aqm: the mask identifying the accessible AP domains > + * @adm: the mask identifying the accessible AP control domains > + * > + * Set the masks that identify the adapters, domains and control domains to > + * which the KVM guest is granted access. > + * > + * Note: The kvm->lock mutex must be locked by the caller. > + */ > void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm, > unsigned long *aqm, unsigned long *adm) > { > struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb; > > - mutex_lock(&kvm->lock); > kvm_s390_vcpu_block_all(kvm); > > switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) { > @@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm, > /* recreate the shadow crycb for each vcpu */ > kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART); > kvm_s390_vcpu_unblock_all(kvm); > - mutex_unlock(&kvm->lock); > } > EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks); > > +/* > + * kvm_arch_crypto_set_masks Copy/paste error here. Rename to kvm_arch_crypto_CLEAR_masks I did not find anything else in my review. However, I'm still very new to this code, so take that with a grain of salt :). Also, I could not apply this to master. If there is a next version do you mind rebasing? Seeing the patch in full context would be helpful. -- -- Jason J. Herne (jjherne@linux.ibm.com) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-21 19:37 ` Jason J. Herne @ 2021-07-22 13:16 ` Tony Krowiak 0 siblings, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-07-22 13:16 UTC (permalink / raw) To: jjherne, linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jgg, alex.williamson, kwankhede, david On 7/21/21 3:37 PM, Jason J. Herne wrote: > On 7/19/21 3:35 PM, Tony Krowiak wrote: >> It was pointed out during an unrelated patch review that locks should >> not >> be open coded - i.e., writing the algorithm of a standard lock in a >> function instead of using a lock from the standard library. The >> setting and >> testing of a busy flag and sleeping on a wait_event is the same thing >> a lock does. The open coded locks are invisible to lockdep, so potential >> locking problems are not detected. >> >> This patch removes the open coded locks used during >> VFIO_GROUP_NOTIFY_SET_KVM notification. The busy flag >> and wait queue were introduced to resolve a possible circular locking >> dependency reported by lockdep when starting a secure execution guest >> configured with AP adapters and domains. Reversing the order in which >> the kvm->lock mutex and matrix_dev->lock mutex are locked resolves the >> issue reported by lockdep, thus enabling the removal of the open coded >> locks. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 27 +++++- >> drivers/s390/crypto/vfio_ap_ops.c | 132 ++++++++------------------ >> drivers/s390/crypto/vfio_ap_private.h | 2 - >> 3 files changed, 63 insertions(+), 98 deletions(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index a08f242a9f27..4d2ef3a3286e 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2559,12 +2559,24 @@ static void kvm_s390_set_crycb_format(struct >> kvm *kvm) >> kvm->arch.crypto.crycbd |= CRYCB_FORMAT1; >> } >> +/* >> + * kvm_arch_crypto_set_masks >> + * >> + * @kvm: a pointer to the object containing the crypto masks > > This should probably say "a pointer to the target guest's KVM struct" > or something to that effect. Same comment for the comment above > kvm_arch_crypto_clear_masks. Will do. > >> + * @apm: the mask identifying the accessible AP adapters >> + * @aqm: the mask identifying the accessible AP domains >> + * @adm: the mask identifying the accessible AP control domains >> + * >> + * Set the masks that identify the adapters, domains and control >> domains to >> + * which the KVM guest is granted access. >> + * >> + * Note: The kvm->lock mutex must be locked by the caller. >> + */ >> void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm, >> unsigned long *aqm, unsigned long *adm) >> { >> struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb; >> - mutex_lock(&kvm->lock); >> kvm_s390_vcpu_block_all(kvm); >> switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) { >> @@ -2595,13 +2607,21 @@ void kvm_arch_crypto_set_masks(struct kvm >> *kvm, unsigned long *apm, >> /* recreate the shadow crycb for each vcpu */ >> kvm_s390_sync_request_broadcast(kvm, KVM_REQ_VSIE_RESTART); >> kvm_s390_vcpu_unblock_all(kvm); >> - mutex_unlock(&kvm->lock); >> } >> EXPORT_SYMBOL_GPL(kvm_arch_crypto_set_masks); >> +/* >> + * kvm_arch_crypto_set_masks > > Copy/paste error here. Rename to kvm_arch_crypto_CLEAR_masks I will fix it. > > I did not find anything else in my review. However, I'm still very new > to this code, so take that with a grain of salt :). The grain of salt has been ingested. > > Also, I could not apply this to master. If there is a next version do > you mind rebasing? Seeing the patch in full context would be helpful. This was rebased on the latest master branch at the time then re-tested. This is something I always do prior to submitting patches, so is it possible you were using an older version of master? > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak 2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak 2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak @ 2021-08-02 13:10 ` Tony Krowiak 2021-08-02 13:53 ` Halil Pasic 2 siblings, 1 reply; 42+ messages in thread From: Tony Krowiak @ 2021-08-02 13:10 UTC (permalink / raw) To: linux-s390, linux-kernel Cc: borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david, pbonzini, david, frankja, imbrenda PING! This patch will pre-req version 17 of a patch series I have waiting in the wings, so I'd like to get this one merged ASAP. In particular, if a KVM maintainer can take a look at the comments concerning the taking of the kvm->lock before the matrix_mdev->lock it would be greatly appreciated. Those comments begin with Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. On 7/19/21 3:35 PM, Tony Krowiak wrote: > This series is actually only comprised of a single patch to replace the > open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The > first patch is included because it is a pre-req slotted to be merged but is > not yet available in the kernel. > > Tony Krowiak (2): > s390/vfio-ap: r/w lock for PQAP interception handler function pointer > s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM > notification > > arch/s390/include/asm/kvm_host.h | 8 +- > arch/s390/kvm/kvm-s390.c | 28 +++++- > arch/s390/kvm/priv.c | 10 +- > drivers/s390/crypto/vfio_ap_ops.c | 127 +++++++++----------------- > drivers/s390/crypto/vfio_ap_private.h | 4 +- > 5 files changed, 77 insertions(+), 100 deletions(-) > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak @ 2021-08-02 13:53 ` Halil Pasic 2021-08-02 16:32 ` Tony Krowiak 0 siblings, 1 reply; 42+ messages in thread From: Halil Pasic @ 2021-08-02 13:53 UTC (permalink / raw) To: Tony Krowiak Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david, pbonzini, frankja, imbrenda On Mon, 2 Aug 2021 09:10:26 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > PING! > > This patch will pre-req version 17 of a patch series I have waiting in > the wings, > so I'd like to get this one merged ASAP. In particular, if a KVM > maintainer can > take a look at the comments concerning the taking of the kvm->lock > before the > matrix_mdev->lock it would be greatly appreciated. Those comments begin with > Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. As far as I'm concerned, we can move forward with this. Was this supposed to go in via Alex's tree? > > On 7/19/21 3:35 PM, Tony Krowiak wrote: > > This series is actually only comprised of a single patch to replace the > > open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The > > first patch is included because it is a pre-req slotted to be merged but is > > not yet available in the kernel. > > > > Tony Krowiak (2): > > s390/vfio-ap: r/w lock for PQAP interception handler function pointer > > s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM > > notification > > > > arch/s390/include/asm/kvm_host.h | 8 +- > > arch/s390/kvm/kvm-s390.c | 28 +++++- > > arch/s390/kvm/priv.c | 10 +- > > drivers/s390/crypto/vfio_ap_ops.c | 127 +++++++++----------------- > > drivers/s390/crypto/vfio_ap_private.h | 4 +- > > 5 files changed, 77 insertions(+), 100 deletions(-) > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-02 13:53 ` Halil Pasic @ 2021-08-02 16:32 ` Tony Krowiak 2021-08-03 13:08 ` Jason Gunthorpe 2021-08-18 15:59 ` Christian Borntraeger 0 siblings, 2 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-02 16:32 UTC (permalink / raw) To: Halil Pasic Cc: linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, jgg, alex.williamson, kwankhede, david, pbonzini, frankja, imbrenda On 8/2/21 9:53 AM, Halil Pasic wrote: > On Mon, 2 Aug 2021 09:10:26 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> PING! >> >> This patch will pre-req version 17 of a patch series I have waiting in >> the wings, >> so I'd like to get this one merged ASAP. In particular, if a KVM >> maintainer can >> take a look at the comments concerning the taking of the kvm->lock >> before the >> matrix_mdev->lock it would be greatly appreciated. Those comments begin with >> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. > As far as I'm concerned, we can move forward with this. Was this > supposed to go in via Alex's tree? I am not certain, Christian queued the previous patches related to this on: https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes Jason G., since this will need to be integrated with your other patches, where should this be queued? > >> On 7/19/21 3:35 PM, Tony Krowiak wrote: >>> This series is actually only comprised of a single patch to replace the >>> open coded locks used during VFIO_GROUP_NOTIFY_SET_KVM notification. The >>> first patch is included because it is a pre-req slotted to be merged but is >>> not yet available in the kernel. >>> >>> Tony Krowiak (2): >>> s390/vfio-ap: r/w lock for PQAP interception handler function pointer >>> s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM >>> notification >>> >>> arch/s390/include/asm/kvm_host.h | 8 +- >>> arch/s390/kvm/kvm-s390.c | 28 +++++- >>> arch/s390/kvm/priv.c | 10 +- >>> drivers/s390/crypto/vfio_ap_ops.c | 127 +++++++++----------------- >>> drivers/s390/crypto/vfio_ap_private.h | 4 +- >>> 5 files changed, 77 insertions(+), 100 deletions(-) >>> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-02 16:32 ` Tony Krowiak @ 2021-08-03 13:08 ` Jason Gunthorpe 2021-08-03 13:34 ` Tony Krowiak 2021-08-18 15:59 ` Christian Borntraeger 1 sibling, 1 reply; 42+ messages in thread From: Jason Gunthorpe @ 2021-08-03 13:08 UTC (permalink / raw) To: Tony Krowiak Cc: Halil Pasic, linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david, pbonzini, frankja, imbrenda On Mon, Aug 02, 2021 at 12:32:12PM -0400, Tony Krowiak wrote: > On 8/2/21 9:53 AM, Halil Pasic wrote: > > On Mon, 2 Aug 2021 09:10:26 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > > > PING! > > > > > > This patch will pre-req version 17 of a patch series I have waiting in > > > the wings, > > > so I'd like to get this one merged ASAP. In particular, if a KVM > > > maintainer can > > > take a look at the comments concerning the taking of the kvm->lock > > > before the > > > matrix_mdev->lock it would be greatly appreciated. Those comments begin with > > > Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. > > As far as I'm concerned, we can move forward with this. Was this > > supposed to go in via Alex's tree? > > I am not certain, Christian queued the previous patches related to > this on: > > > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes > > Jason G., since this will need to be integrated with your other patches, > where should this be queued? I prefer changes to the vfio stuff go to Alex as we are changing it a lot as well this cycle. Jason ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-03 13:08 ` Jason Gunthorpe @ 2021-08-03 13:34 ` Tony Krowiak 0 siblings, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-03 13:34 UTC (permalink / raw) To: Jason Gunthorpe Cc: Halil Pasic, linux-s390, linux-kernel, borntraeger, cohuck, pasic, jjherne, alex.williamson, kwankhede, david, pbonzini, frankja, imbrenda On 8/3/21 9:08 AM, Jason Gunthorpe wrote: > On Mon, Aug 02, 2021 at 12:32:12PM -0400, Tony Krowiak wrote: >> On 8/2/21 9:53 AM, Halil Pasic wrote: >>> On Mon, 2 Aug 2021 09:10:26 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> PING! >>>> >>>> This patch will pre-req version 17 of a patch series I have waiting in >>>> the wings, >>>> so I'd like to get this one merged ASAP. In particular, if a KVM >>>> maintainer can >>>> take a look at the comments concerning the taking of the kvm->lock >>>> before the >>>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with >>>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. >>> As far as I'm concerned, we can move forward with this. Was this >>> supposed to go in via Alex's tree? >> I am not certain, Christian queued the previous patches related to >> this on: >> >> >> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes >> >> Jason G., since this will need to be integrated with your other patches, >> where should this be queued? > I prefer changes to the vfio stuff go to Alex as we are changing it a > lot as well this cycle. Thanks Jason. > > Jason ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-02 16:32 ` Tony Krowiak 2021-08-03 13:08 ` Jason Gunthorpe @ 2021-08-18 15:59 ` Christian Borntraeger 2021-08-18 16:39 ` Alex Williamson 1 sibling, 1 reply; 42+ messages in thread From: Christian Borntraeger @ 2021-08-18 15:59 UTC (permalink / raw) To: Tony Krowiak, Halil Pasic, alex.williamson Cc: linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja, imbrenda On 02.08.21 18:32, Tony Krowiak wrote: > > > On 8/2/21 9:53 AM, Halil Pasic wrote: >> On Mon, 2 Aug 2021 09:10:26 -0400 >> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >> >>> PING! >>> >>> This patch will pre-req version 17 of a patch series I have waiting in >>> the wings, >>> so I'd like to get this one merged ASAP. In particular, if a KVM >>> maintainer can >>> take a look at the comments concerning the taking of the kvm->lock >>> before the >>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with >>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. >> As far as I'm concerned, we can move forward with this. Was this >> supposed to go in via Alex's tree? > > I am not certain, Christian queued the previous patches related to > this on: > > > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes > > Jason G., since this will need to be integrated with your other patches, > where should this be queued? This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is already in master. Can you respin the series with all Acks and RBs? Alex, can you then take these 2 patches via your tree? Thanks Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> for this series. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-18 15:59 ` Christian Borntraeger @ 2021-08-18 16:39 ` Alex Williamson 2021-08-18 16:50 ` Christian Borntraeger 2021-08-19 15:30 ` Cornelia Huck 0 siblings, 2 replies; 42+ messages in thread From: Alex Williamson @ 2021-08-18 16:39 UTC (permalink / raw) To: Christian Borntraeger Cc: Tony Krowiak, Halil Pasic, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja, imbrenda On Wed, 18 Aug 2021 17:59:51 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 02.08.21 18:32, Tony Krowiak wrote: > > > > > > On 8/2/21 9:53 AM, Halil Pasic wrote: > >> On Mon, 2 Aug 2021 09:10:26 -0400 > >> Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> > >>> PING! > >>> > >>> This patch will pre-req version 17 of a patch series I have waiting in > >>> the wings, > >>> so I'd like to get this one merged ASAP. In particular, if a KVM > >>> maintainer can > >>> take a look at the comments concerning the taking of the kvm->lock > >>> before the > >>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with > >>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. > >> As far as I'm concerned, we can move forward with this. Was this > >> supposed to go in via Alex's tree? > > > > I am not certain, Christian queued the previous patches related to > > this on: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes > > > > Jason G., since this will need to be integrated with your other patches, > > where should this be queued? > > > This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is > already in master. > Can you respin the series with all Acks and RBs? > > Alex, can you then take these 2 patches via your tree? Thanks > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > for this series. I see some review feedback that seems to suggest a new version would be posted: https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/ I also see in this thread: https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/ that Halil's concern's around open/close races are addressed by Jason's device_open/close series that's already in my next branch and he provided an Ack, but there's still the above question regarding the kvm->lock that was looking for a review from... I'm not sure, maybe Connie or Paolo. Christian, is this specifically what you're ack'ing? It can ultimately go in through my tree, but not being familiar with this code I'd hope for more closure. Thanks, Alex ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-18 16:39 ` Alex Williamson @ 2021-08-18 16:50 ` Christian Borntraeger 2021-08-18 22:52 ` Halil Pasic 2021-08-19 15:30 ` Cornelia Huck 1 sibling, 1 reply; 42+ messages in thread From: Christian Borntraeger @ 2021-08-18 16:50 UTC (permalink / raw) To: Alex Williamson Cc: Tony Krowiak, Halil Pasic, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja, imbrenda On 18.08.21 18:39, Alex Williamson wrote: > On Wed, 18 Aug 2021 17:59:51 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 02.08.21 18:32, Tony Krowiak wrote: >>> >>> >>> On 8/2/21 9:53 AM, Halil Pasic wrote: >>>> On Mon, 2 Aug 2021 09:10:26 -0400 >>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>>> >>>>> PING! >>>>> >>>>> This patch will pre-req version 17 of a patch series I have waiting in >>>>> the wings, >>>>> so I'd like to get this one merged ASAP. In particular, if a KVM >>>>> maintainer can >>>>> take a look at the comments concerning the taking of the kvm->lock >>>>> before the >>>>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with >>>>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. >>>> As far as I'm concerned, we can move forward with this. Was this >>>> supposed to go in via Alex's tree? >>> >>> I am not certain, Christian queued the previous patches related to >>> this on: >>> >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes >>> >>> Jason G., since this will need to be integrated with your other patches, >>> where should this be queued? >> >> >> This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is >> already in master. >> Can you respin the series with all Acks and RBs? >> >> Alex, can you then take these 2 patches via your tree? Thanks >> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >> for this series. > > > I see some review feedback that seems to suggest a new version would be > posted: > > https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/ > > I also see in this thread: > > https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/ > > that Halil's concern's around open/close races are addressed by Jason's > device_open/close series that's already in my next branch and he > provided an Ack, but there's still the above question regarding the > kvm->lock that was looking for a review from... I'm not sure, maybe > Connie or Paolo. Christian, is this specifically what you're ack'ing? My understanding was that Halil was ok in the end? I will do a review myself then if that helps. > > It can ultimately go in through my tree, but not being familiar with > this code I'd hope for more closure. Thanks, > > Alex > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-18 16:50 ` Christian Borntraeger @ 2021-08-18 22:52 ` Halil Pasic 0 siblings, 0 replies; 42+ messages in thread From: Halil Pasic @ 2021-08-18 22:52 UTC (permalink / raw) To: Christian Borntraeger Cc: Alex Williamson, Tony Krowiak, linux-s390, linux-kernel, cohuck, pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja, imbrenda On Wed, 18 Aug 2021 18:50:47 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > that Halil's concern's around open/close races are addressed by Jason's > > device_open/close series that's already in my next branch and he > > provided an Ack, but there's still the above question regarding the > > kvm->lock that was looking for a review from... I'm not sure, maybe > > Connie or Paolo. Christian, is this specifically what you're ack'ing? > > My understanding was that Halil was ok in the end? Yes, I'm OK with it provided it is merged after Jason's patch that makes it a non-issue. Regards, Halil ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-18 16:39 ` Alex Williamson 2021-08-18 16:50 ` Christian Borntraeger @ 2021-08-19 15:30 ` Cornelia Huck 2021-08-20 14:24 ` Tony Krowiak 1 sibling, 1 reply; 42+ messages in thread From: Cornelia Huck @ 2021-08-19 15:30 UTC (permalink / raw) To: Alex Williamson, Christian Borntraeger Cc: Tony Krowiak, Halil Pasic, linux-s390, linux-kernel, pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja, imbrenda On Wed, Aug 18 2021, Alex Williamson <alex.williamson@redhat.com> wrote: > On Wed, 18 Aug 2021 17:59:51 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 02.08.21 18:32, Tony Krowiak wrote: >> > >> > >> > On 8/2/21 9:53 AM, Halil Pasic wrote: >> >> On Mon, 2 Aug 2021 09:10:26 -0400 >> >> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >> >> >> >>> PING! >> >>> >> >>> This patch will pre-req version 17 of a patch series I have waiting in >> >>> the wings, >> >>> so I'd like to get this one merged ASAP. In particular, if a KVM >> >>> maintainer can >> >>> take a look at the comments concerning the taking of the kvm->lock >> >>> before the >> >>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with >> >>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. >> >> As far as I'm concerned, we can move forward with this. Was this >> >> supposed to go in via Alex's tree? >> > >> > I am not certain, Christian queued the previous patches related to >> > this on: >> > >> > >> > https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes >> > >> > Jason G., since this will need to be integrated with your other patches, >> > where should this be queued? >> >> >> This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is >> already in master. >> Can you respin the series with all Acks and RBs? >> >> Alex, can you then take these 2 patches via your tree? Thanks >> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >> for this series. > > > I see some review feedback that seems to suggest a new version would be > posted: > > https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/ Yeah, I thought so as well. But it also looks like something that could be a fixup on top. > > I also see in this thread: > > https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/ > > that Halil's concern's around open/close races are addressed by Jason's > device_open/close series that's already in my next branch and he > provided an Ack, but there's still the above question regarding the > kvm->lock that was looking for a review from... I'm not sure, maybe > Connie or Paolo. Christian, is this specifically what you're ack'ing? I'm also unsure about the kvm->lock thing. Is taking the lock buried somewhere deep in the code that will ultimately trigger the release? I would at least like a pointer. > > It can ultimately go in through my tree, but not being familiar with > this code I'd hope for more closure. Thanks, > > Alex ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification 2021-08-19 15:30 ` Cornelia Huck @ 2021-08-20 14:24 ` Tony Krowiak 0 siblings, 0 replies; 42+ messages in thread From: Tony Krowiak @ 2021-08-20 14:24 UTC (permalink / raw) To: Cornelia Huck, Alex Williamson, Christian Borntraeger Cc: Halil Pasic, linux-s390, linux-kernel, pasic, jjherne, jgg, kwankhede, david, pbonzini, frankja, imbrenda On 8/19/21 11:30 AM, Cornelia Huck wrote: > On Wed, Aug 18 2021, Alex Williamson <alex.williamson@redhat.com> wrote: > >> On Wed, 18 Aug 2021 17:59:51 +0200 >> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >> >>> On 02.08.21 18:32, Tony Krowiak wrote: >>>> >>>> On 8/2/21 9:53 AM, Halil Pasic wrote: >>>>> On Mon, 2 Aug 2021 09:10:26 -0400 >>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>>>> >>>>>> PING! >>>>>> >>>>>> This patch will pre-req version 17 of a patch series I have waiting in >>>>>> the wings, >>>>>> so I'd like to get this one merged ASAP. In particular, if a KVM >>>>>> maintainer can >>>>>> take a look at the comments concerning the taking of the kvm->lock >>>>>> before the >>>>>> matrix_mdev->lock it would be greatly appreciated. Those comments begin with >>>>>> Message ID <20210727004329.3bcc7d4f.pasic@linux.ibm.com> from Halil Pasic. >>>>> As far as I'm concerned, we can move forward with this. Was this >>>>> supposed to go in via Alex's tree? >>>> I am not certain, Christian queued the previous patches related to >>>> this on: >>>> >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=fixes >>>> >>>> Jason G., since this will need to be integrated with your other patches, >>>> where should this be queued? >>> >>> This previous patch (s390/vfio-ap: clean up mdev resources when remove callback invoked) is >>> already in master. >>> Can you respin the series with all Acks and RBs? >>> >>> Alex, can you then take these 2 patches via your tree? Thanks >>> >>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> for this series. >> >> I see some review feedback that seems to suggest a new version would be >> posted: >> >> https://lore.kernel.org/linux-s390/0f03ab0b-2dfd-e1c1-fe43-be2a59030a71@linux.ibm.com/ > Yeah, I thought so as well. But it also looks like something that could > be a fixup on top. I will post the new patch today. I was waiting for the remainder of the feedback and frankly forgot to post the patch incorporating the changes precipitated by the previous comments. > >> I also see in this thread: >> >> https://lore.kernel.org/linux-s390/20210721164550.5402fe1c.pasic@linux.ibm.com/ >> >> that Halil's concern's around open/close races are addressed by Jason's >> device_open/close series that's already in my next branch and he >> provided an Ack, but there's still the above question regarding the >> kvm->lock that was looking for a review from... I'm not sure, maybe >> Connie or Paolo. Christian, is this specifically what you're ack'ing? > I'm also unsure about the kvm->lock thing. Is taking the lock buried > somewhere deep in the code that will ultimately trigger the release? > I would at least like a pointer. I'm not quite sure what you're asking here, but if you follow the thread starting with the link above it may reveal the answer to what you are asking here. > >> It can ultimately go in through my tree, but not being familiar with >> this code I'd hope for more closure. Thanks, >> >> Alex ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2021-08-23 20:51 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-19 19:35 [PATCH 0/2] s390/vfio-ap: do not open code locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak 2021-07-19 19:35 ` [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer Tony Krowiak 2021-08-18 17:03 ` Christian Borntraeger 2021-08-18 23:25 ` Halil Pasic 2021-08-19 6:56 ` Christian Borntraeger 2021-08-19 13:36 ` Tony Krowiak 2021-08-19 21:42 ` Halil Pasic 2021-08-23 13:08 ` Tony Krowiak 2021-08-19 13:20 ` Tony Krowiak 2021-08-19 17:54 ` Alex Williamson 2021-08-19 17:58 ` Jason Gunthorpe 2021-08-20 15:59 ` Tony Krowiak 2021-08-20 22:05 ` Tony Krowiak 2021-08-20 22:30 ` Jason Gunthorpe 2021-08-23 15:17 ` Tony Krowiak 2021-08-20 22:41 ` Alex Williamson 2021-08-23 20:51 ` Tony Krowiak 2021-07-19 19:35 ` [PATCH 2/2] s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification Tony Krowiak 2021-07-21 14:45 ` Halil Pasic 2021-07-22 13:09 ` Tony Krowiak 2021-07-23 14:26 ` Halil Pasic 2021-07-23 21:24 ` Tony Krowiak 2021-07-26 20:36 ` Halil Pasic 2021-07-26 22:03 ` Jason Gunthorpe 2021-07-26 22:43 ` Halil Pasic 2021-07-28 13:43 ` Tony Krowiak 2021-07-28 19:42 ` Halil Pasic 2021-07-30 13:33 ` Tony Krowiak 2021-07-27 6:54 ` Christoph Hellwig 2021-07-21 19:37 ` Jason J. Herne 2021-07-22 13:16 ` Tony Krowiak 2021-08-02 13:10 ` [PATCH 0/2] s390/vfio-ap: do not open code " Tony Krowiak 2021-08-02 13:53 ` Halil Pasic 2021-08-02 16:32 ` Tony Krowiak 2021-08-03 13:08 ` Jason Gunthorpe 2021-08-03 13:34 ` Tony Krowiak 2021-08-18 15:59 ` Christian Borntraeger 2021-08-18 16:39 ` Alex Williamson 2021-08-18 16:50 ` Christian Borntraeger 2021-08-18 22:52 ` Halil Pasic 2021-08-19 15:30 ` Cornelia Huck 2021-08-20 14:24 ` Tony Krowiak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).