On Fri, 15 Mar 2019 14:26:34 +0100
Pierre Morel <pmorel@xxxxxxxxxxxxx> 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
Isn't OPERATION a bad answer if ap=on? It should not happen
with a well behaved guest because facility 65 is not indicated,
but if it does, I guess we give the wrong answer.
> NEW - OLD SPECIFICATION
> NEW - NEW/aqic=off SPECIFICATION
> NEW x NEW/aqic=on -
>
AFAICT with LINUX == NEW we get the correct answer. OPERATION exception
is only good if ap=off.
> 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...
>
IMHO this boils down to:
* either OLD QEMU or
* OLD LINUX
should have taken care of handling the mandatory intercept for PQAP/AQIC
if ap=on (i.e. guest has AP instructions), and does not have facility 65
which was the case for OLD.
Things get complicated when one considers that ECA.28 is an effective
control.
> 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
That would mean we remain quirky.
> NEW QEMU will handle this correctly.
>
> In this case we also do not need to handle all other tests here but can
> move it to the callback as Tony wanted.
>
> Would you agree with something simple like:
>
> static int handle_pqap(struct kvm_vcpu *vcpu)
> {
> int ret = -EOPNOTSUPP;
>
> /* Verify that the hook callback is registered and call it */
> if (pqap_hook)
> if (try_module_get(pqap_hook->owner)) {
> ret = pqap_hook->hook(vcpu);
> module_put(pqap_hook->owner);
> }
> return ret;
> }
>
> All other tests in QEMU and in the callback.
>
You stated in another email that the conclusion is wrong. I'm not sure
what is the cleanest solution here. This effective control thing does
make my head spin.
Regards,
Halil
