Re: [PATCH v5 1/7] s390: ap: kvm: add PQAP interception for AQIC

[Pierre Morel]

On 15/03/2019 14:26, Pierre Morel wrote:
> On 15/03/2019 11:20, Cornelia Huck wrote:
> > On Wed, 13 Mar 2019 17:04:58 +0100
> > Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
> >
> > > +/*
> > > + * handle_pqap: Handling pqap interception
> > > + * @vcpu: the vcpu having issue the pqap instruction
> > > + *
> > > + * We now support PQAP/AQIC instructions and we need to correctly
> > > + * answer the guest even if no dedicated driver's hook is available.
> > > + *
> > > + * The intercepting code calls a dedicated callback for this 
> > > instruction
> > > + * if a driver did register one in the CRYPTO satellite of the
> > > + * SIE block.
> > > + *
> > > + * For PQAP/AQIC instructions only, verify privilege and 
> > > specifications.
> > > + *
> > > + * If no callback available, the queues are not available, return 
> > > this to
> > > + * the caller.
> > > + * Else return the value returned by the callback.
> > > + */
> > > +static int handle_pqap(struct kvm_vcpu *vcpu)
> > > +{
> > > +ÂÂÂ uint8_t fc;
> > > +ÂÂÂ struct ap_queue_status status = {};
> > > +ÂÂÂ int ret;
> > > +ÂÂÂ /* 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;
> > > +ÂÂÂ /* We do not want to change the behavior we had before this patch*/
> > > +ÂÂÂ if (fc != 0x03)
> > > +ÂÂÂÂÂÂÂ return -EOPNOTSUPP;
> > > +
> > > +ÂÂÂ /* PQAP instructions are allowed for guest kernel only */
> > > +ÂÂÂ if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
> > > +ÂÂÂÂÂÂÂ return kvm_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);
> > > +ÂÂÂ /* Verify that the hook callback is registered and call it */
> > > +ÂÂÂ 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);
> > > +ÂÂÂÂÂÂÂ return ret;
> > > +ÂÂÂ }
> > > +ÂÂÂ /*
> > > +ÂÂÂÂ * It is the duty of the vfio_driver to register a hook
> > > +ÂÂÂÂ * If it does not and we get an exception on AQIC we must
> > > +ÂÂÂÂ * guess that there is no vfio_ap_driver at all and no one
> > > +ÂÂÂÂ * to handle the guests's CRYCB and the CRYCB is empty.
> > > +ÂÂÂÂ */
> > > +ÂÂÂ status.response_code = 0x01;
> >
> > I'm still confused here, sorry. From previous discussions I recall that
> > this indicates "no crypto device" (please correct me if I'm wrong.)
> >
> > Before this patch, we had:
> > - guest issues PQAP/AQIC -> drop to userspace
> >
> > With a correct implementation, we get:
> > - guest issues PQAP/AQIC -> callback does what needs to be done
> >
> > With an incorrect implementation (no callback), we get:
> > - guest issues PQAP/AQIC -> guest gets response code 0x01
> >
> > Why not drop to userspace in that case?
>
> This is what I had in the previous patches.
> Hum, I do not remember which discussion lead me to modify this.
>
> Anyway, now that you put the finger on this problem, I think the problem 
> is worse.
>
> The behavior with old / new Linux, vfio driver and qemu is:
>
> LINUXÂÂÂ VFIO_APÂÂÂ QEMUÂÂÂ PGM
> OLDÂÂÂ xÂÂÂ xÂÂÂ OPERATION
> NEWÂÂÂ -ÂÂÂ OLDÂÂÂ SPECIFICATION
> NEWÂÂÂ -ÂÂÂ NEW/aqic=offÂÂÂ SPECIFICATION
> NEWÂÂÂ xÂÂÂ NEW/aqic=onÂÂÂ -
>
> x = whatever
> - = absent/none
>
> So yes there is a change in behavior for the userland for the case QEMU 
> do not set the AQIC facility 65, OLD QEMU or NEW QEMU wanting to behave 
> like an older one.
>
> I fear we have the same problem with the privileged operation...
>
> For the last case, when the kvm_facility(65) is set, the explication is 
> the following:
>
> This is related to the handling of PQAP AQIC which is now authorized by 
> this patch series.
> If we authorize PQAP AQIC, by setting the bit for facility 65, the guest 
> can use this instruction.
> If the instruction follows the specifications we must answer something 
> realistic and since there is nothing in the CRYCB (no driver) we answer 
> that there is no queue.
>
> Conclusion:Â we must handle this in userland, it will have the benefit 
> to keep old behavior when there is no callback.
> OLD QEMU will not see change as they will not set aqic facility
> NEW QEMU will handle this correctly.

Sorry, wrong conclusion, handling this in userland will bring us much 
too far if we want to answer correctly for the case the hook is not 
there but QEMU accepted the facility for AQIC.

The alternative is easier, we just continue to respond with the 
OPERATION exception here and only handle the specification and 
privileged exception cases in QEMU and in the hook.

So, I think the discussion will go on until you come back :)

Regards,
Pierre

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