Messages in this thread | | | Subject | Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer | From | Tony Krowiak <> | Date | Thu, 19 Aug 2021 09:20:28 -0400 |
| |
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); > [...]
| |