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

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 3/28/19 8:43 AM, Pierre Morel wrote:
> On 26/03/2019 19:57, Tony Krowiak wrote:
>> On 3/22/19 10:43 AM, Pierre Morel wrote:
>>> We prepare the interception of the PQAP/AQIC instruction for
>>> the case the AQIC facility is enabled in the guest.
>>>
> 
> ...snip...
> 
>>> +/*
>>> + * 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 and TAPQ instructions, verify privilege and 
>>> specifications.
>>
>> The two paragraphs above should be described via the comments embedded
>> in the code and is not necessary here.
>>
>>> + *
>>> + * If no callback available, the queues are not available, return 
>>> this to
>>> + * the caller.
>>
>> This implies it is specified via the return code when it is in fact
>> the response code in the status word.
>>
>>> + * Else return the value returned by the callback.
>>> + */
>>
>> Given this handler may be called for any PQAP instruction sub-function,
>> I think the function doc should be more generic, providing:
>>
>> * A general description of what the function does
>> * A description of each input parameter
>> * A description of the value returned. If the return value is a return
>>    code, the possible rc values can be enumerated with a description for
>>    of the reason each particular value may be returned.
> 
> Sorry, I do not understand what you want here.
> Isn't it exactly what is done?

No, what you have provided is a description that includes details that
may not apply in the future. I'm thinking something more like this:

/*
  * handle_pqap
  *
  * @vcpu: the vcpu that executed the PQAP instruction
  *
  * Handles interception of the PQAP instruction. A specification
  * exception will be injected into the guest if the input parameters
  * to the PQAP instruction are not properly formatted.
  *
  * Returns zero if the PQAP instruction is handled successfully;
  * otherwise, returns an error.
  */

> 
> And don't you exactly say the opposite when you say that the description 
> should be done by the embedded comments?

Not really, that was directed at only the two sentences preceding the
comment.

> 
> 
>>
>>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>>> +{
>>> +    struct ap_queue_status status = {};
>>> +    unsigned long reg0;
>>> +    int ret;
>>> +    uint8_t fc;
>>> +
>>> +    /* 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;
>>> +    /*
>>> +     * The only possibly intercepted instructions when AP 
>>> instructions are
>>> +     * available for the guest are AQIC and TAPQ with the t bit set
>>> +     * since we do not set IC.3 (FIII) we currently will not intercept
>>> +     * TAPQ.
>>> +     * The following code will only treat AQIC function code.
>>> +     */
>>
>> Simplify to:
>>
>> /* The only supported PQAP function is AQIC (0x03) */
> 
> OK, but then istn't obvious from reading the code ?

It's obvious that you are verifying the function code is
0x03, but only those familiar with the architecture will
know the is the AQIC function. Besides, I was merely modifying
the comment you already had. You can leave the comment out
if you prefer.

> 
>>
>>> +    reg0 = vcpu->run->s.regs.gprs[0];
>>> +    fc = reg0 >> 24;
>>> +    if (fc != 0x03) {
>>> +        pr_warn("%s: Unexpected interception code 0x%02x\n",
>>> +            __func__, fc);

I would change the text to:
"Unexpected PQAP function code 0x%02x\n"

>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +    /* All PQAP instructions are allowed for guest kernel only */
>>
>> There is only one PQAP instruction with multiple sub-functions.
>> /* PQAP instruction is allowed for guest kernel only */
>>                          or
>> /* PQAP instruction is privileged */
> 
> OK
> 
>>
>>> +    if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
>>> +        return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>>> +    /*
>>> +     * Common tests for PQAP instructions to generate a specification
>>> +     * exception
>>> +     */
>>
>> This comment is unnecessary as the individual comments below adequately
>> do the job.
> 
> OK
> 
>>
>>> +    /* Zero bits overwrite produce a specification exception */
>>
>> This comment has no meaning unless you intimately know the architecture.
>> The following would make more sense:
>>
>>      /* Bits 41-47 must all be zeros */
>>
>> It's probably not a big deal, but since we don't support PQAP(TAPQ),
>> would it make more sense to make sure bits 40-47 are zeros (i.e.,
>> the 't' bit is not set)?
> 
> I am not sure about this one as APFT is installed in our case.
> Or do you want that we test if it is installed and test the bit 40?
> 
> We should discuss this offline because I do not find any evidence that 
> we should really do this in the documentation.

I am okay with not checking bit 40, but I would change the comment as
suggested: /* Bits 41-47 must all be zeros */

> 
>>
>>> +    if (reg0 & 0x007f0000UL)
>>> +        goto specification_except;
>>> +    /* If APXA is not installed APQN is limited */
>>
>> Wouldn't it be better to state how the APQN is limited?
>> For example:
>>
>>      /*
>>       * If APXA is not installed, then the maximum APID is
>>       * 63 (bits 48-49 of reg0 must be zero) and the maximum
>>       * APQI is 15 (bits 56-59 must be zero)
>>       */
> OK
>>
>>> +    if (!(vcpu->kvm->arch.crypto.crycbd & 0x02))
>>> +        if (reg0 & 0x000030f0UL)
>>
>> If APXA is not installed, then bits 48-49 and 56-59 must all be
>> zeros. Shouldn't this mask be 0x0000c0f0UL?
> 
> You can better count than I do ;)
> I will change this to c0f0.
> 
> ...snip...
>>
>>
>>
>>> +    status.response_code = 0x01;
>>> +    memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
> 
> hum,
> I miss a
>      kvm_s390_set_psw_cc(vcpu, 3);
> here
> and certainly wherever fault in the status response code are set.
> 
> Will be corrected in the next iteration.

Sounds good.

> 
> 
> Thanks for the comments,
> 
> regards,
> Pierre
> 
> 
>