Re: [PATCH 1/2] s390/vfio-ap: r/w lock for PQAP interception handler function pointer

[Tony Krowiak]

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@xxxxxxxxxxxxx>
> > Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > ---
> >   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);
> [...]