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

[Halil Pasic]

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