On Thu, 19 Aug 2021 09:20:28 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> 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@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?
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@xxxxxxxxxx/
[2]https://lore.kernel.org/linux-s390/6d64bd83-1519-6065-a4cd-9356c6be5d1a@xxxxxxxxxx/
[3]https://lore.kernel.org/linux-s390/e809be5b-0b24-34dc-1eae-82b58dc54545@xxxxxxxxxx/
