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

[Alex Williamson <alex.williamson@redhat.com>]

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/