Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Pierre Morel]

On 15/02/2019 23:02, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
> > This patch adds interception code for the PQAP instructions,
> > and a callback inside the KVM arch structure for s390.
> >
> > If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ
> > instructions, the driver will initialize the callback inside
> > the kvm_arch structure to be called when the interception of a
> > PQAP instruction occurs.
> >
> > If the callback is not initialized, the code still returns
> > -EOPNOTSUPP to let userland handle the instruction as it used to.
> >
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > ---
> > Â arch/s390/include/asm/kvm_host.h |Â 1 +
> > Â arch/s390/kvm/priv.cÂÂÂÂÂÂÂÂÂÂÂÂ | 50 
> > ++++++++++++++++++++++++++++++++++++++++
> > Â 2 files changed, 51 insertions(+)
> >
> > diff --git a/arch/s390/include/asm/kvm_host.h 
> > b/arch/s390/include/asm/kvm_host.h
> > index c5f5156..49cc8b0 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
> > Â struct kvm_s390_crypto {
> > ÂÂÂÂÂ struct kvm_s390_crypto_cb *crycb;
> > +ÂÂÂ int (*pqap_hook)(struct kvm_vcpu *vcpu);
> > ÂÂÂÂÂ __u32 crycbd;
> > ÂÂÂÂÂ __u8 aes_kw;
> > ÂÂÂÂÂ __u8 dea_kw;
> > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> > index 8679bd7..72fdc21 100644
> > --- a/arch/s390/kvm/priv.c
> > +++ b/arch/s390/kvm/priv.c
> > @@ -27,6 +27,7 @@
> > Â #include <asm/io.h>
> > Â #include <asm/ptrace.h>
> > Â #include <asm/sclp.h>
> > +#include <asm/ap.h>
> > Â #include "gaccess.h"
> > Â #include "kvm-s390.h"
> > Â #include "trace.h"
> > @@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
> > ÂÂÂÂÂ }
> > Â }
> > +/*
> > + * handle_pqap: Handling pqap interception
> > + * @vcpu: the vcpu having issue the pqap instruction
> > + *
> > + * This callback only handles PQAP/AQIC instruction and
> > + * calls a dedicated callback for this instruction if
> > + * a driver did register one in the CRYPTO satellite of the
> > + * SIE block.
> > + *
> > + * Do not change the behavior if, return -EOPNOTSUPP if:
> > + * - the hook is not used do not change the behavior.
> > + * - AP instructions are not available or not available to the guest
> > + * - the instruction is not PQAP with function code indicating
> > + *ÂÂ AQIC do not change the previous behavior.
> > + *
> > + * For PQAP/AQIC instruction, verify privilege and specifications
> > + *
> > + * return the value returned by the callback.
> > + */
> > +static int handle_pqap(struct kvm_vcpu *vcpu)
> > +{
> > +ÂÂÂ uint8_t fc;
> > +
> > +ÂÂÂ /* Verify that the hook callback is registered */
> > +ÂÂÂ if (!vcpu->kvm->arch.crypto.pqap_hook)
> > +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
> > +ÂÂÂ /* Verify that the AP instruction are available */
> > +ÂÂÂ if (!ap_instructions_available())
> > +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
> > +ÂÂÂ /* Verify that the guest is allowed to use AP instructions */
> > +ÂÂÂ if (!(vcpu->arch.sie_block->eca & ECA_APIE))
> > +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
> > +ÂÂÂ /* Verify that the function code is AQIC */
> > +ÂÂÂ fc = vcpu->run->s.regs.gprs[0] >> 24;
> > +ÂÂÂ if (fc != 0x03)
> > +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
>
> This does not belong here. Function code 3 is one of 7 function codes
> that can be sent with the PQAP instruction. This belongs in the PQAP
> hook code.

On one hand, effectively I would prefer to put the code in the VFIO 
driver code.
On the other hand, doing this would lead to export the code for 
test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Would like opinion from KVM maintainers?

> > +
> > +ÂÂÂ /* PQAP instructions are allowed for guest kernel only */
> > +ÂÂÂ if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
> > +ÂÂÂÂÂÂÂ return kvm_s390_inject_program_intkvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
> > +ÂÂÂ /* AQIC instruction is allowed only if facility 65 is available */
> > +ÂÂÂ if (!test_kvm_facility(vcpu->kvm, 65))
> > +ÂÂÂÂÂÂÂ return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> > +ÂÂÂ /* All right, call the callback */
> > +ÂÂÂ return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
> > +}
> > +
> > Â static int handle_stfl(struct kvm_vcpu *vcpu)
> > Â {
> > ÂÂÂÂÂ int rc;
> > @@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
> > ÂÂÂÂÂÂÂÂÂ return handle_sthyi(vcpu);
> > ÂÂÂÂÂ case 0x7d:
> > ÂÂÂÂÂÂÂÂÂ return handle_stsi(vcpu);
> > +ÂÂÂ case 0xaf:
> > +ÂÂÂÂÂÂÂ return handle_pqap(vcpu);
> > ÂÂÂÂÂ case 0xb1:
> > ÂÂÂÂÂÂÂÂÂ return handle_stfl(vcpu);
> > ÂÂÂÂÂ case 0xb2:


--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany