Messages in this thread | | | Subject | Re: [PATCH v6 1/7] s390: ap: kvm: add PQAP interception for AQIC | From | Tony Krowiak <> | Date | Thu, 28 Mar 2019 11:24:30 -0400 |
| |
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 > > >
| |