Re: [kvm-unit-tests PATCH v4 1/7] lib: s390x: Add ap library

[Janosch Frank <frankja@linux.ibm.com>]

On 2/5/24 19:15, Anthony Krowiak wrote:
> I made a few comments and suggestions. I am not very well-versed in the
> inline assembly code, so I'll leave that up to someone who is more
> knowledgeable. I copied @Harald since I believe it was him who wrote it.
> 
> On 2/2/24 9:59 AM, Janosch Frank wrote:
>> Add functions and definitions needed to test the Adjunct
>> Processor (AP) crypto interface.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

[...]

>> +/* Will later be extended to a proper setup function */
>> +bool ap_setup(void)
>> +{
>> +	/*
>> +	 * Base AP support has no STFLE or SCLP feature bit but the
>> +	 * PQAP QCI support is indicated via stfle bit 12. As this
>> +	 * library relies on QCI we bail out if it's not available.
>> +	 */
>> +	if (!test_facility(12))
>> +		return false;
> 
> 
> The STFLE.12 can be turned off when starting the guest, so this may not
> be a valid test.
> 
> We use the ap_instructions_available function (in ap.h) which executes
> the TAPQ command to verify whether the AP instructions are installed or
> not. Maybe you can do something similar here:

This library relies on QCI, hence we only check for stfle.
I see no sense in manually probing the whole APQN space.


If stfle 12 is indicated I'd expect AP instructions to not generate 
exceptions or do they in a sane CPU model?


>> +
>> +	return true;
>> +}
>> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
>> new file mode 100644
>> index 00000000..b806513f
>> --- /dev/null
>> +++ b/lib/s390x/ap.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * AP definitions
>> + *
>> + * Some parts taken from the Linux AP driver.
>> + *
>> + * Copyright IBM Corp. 2024
>> + * Author: Janosch Frank <frankja@linux.ibm.com>
>> + *	   Tony Krowiak <akrowia@linux.ibm.com>
>> + *	   Martin Schwidefsky <schwidefsky@de.ibm.com>
>> + *	   Harald Freudenberger <freude@de.ibm.com>
>> + */
>> +
>> +#ifndef _S390X_AP_H_
>> +#define _S390X_AP_H_
>> +
>> +enum PQAP_FC {
>> +	PQAP_TEST_APQ,
>> +	PQAP_RESET_APQ,
>> +	PQAP_ZEROIZE_APQ,
>> +	PQAP_QUEUE_INT_CONTRL,
>> +	PQAP_QUERY_AP_CONF_INFO,
>> +	PQAP_QUERY_AP_COMP_TYPE,
>> +	PQAP_BEST_AP,
> 
> 
> Maybe use abbreviations like your function names above?
> 
> 	PQAP_TAPQ,
> 	PQAP_RAPQ,
> 	PQAP_ZAPQ,
> 	PQAP_AQIC,
> 	PQAP_QCI,
> 	PQAP_QACT,
> 	PQAP_QBAP
> 

Hmmmmmmm(TM)
My guess is that I tried making these constants readable without 
consulting architecture documents. But another option is using the 
constants that you suggested and adding comments with a long version.

Will do

[...]

>> +struct pqap_r0 {
>> +	uint32_t pad0;
>> +	uint8_t fc;
>> +	uint8_t t : 1;		/* Test facilities (TAPQ)*/
>> +	uint8_t pad1 : 7;
>> +	uint8_t ap;
> 
> 
> This is the APID part of an APQN, so how about renaming to 'apid'
> 
> 
>> +	uint8_t qn;
> 
> 
> This is the APQI  part of an APQN, so how about renaming to 'apqi'

Hmm Linux uses qid
I'll change it to the Linux naming convention, might take me a while though

> 
> 
>> +} __attribute__((packed))  __attribute__((aligned(8)));
>> +
>> +struct pqap_r2 {
>> +	uint8_t s : 1;		/* Special Command facility */
>> +	uint8_t m : 1;		/* AP4KM */
>> +	uint8_t c : 1;		/* AP4KC */
>> +	uint8_t cop : 1;	/* AP is in coprocessor mode */
>> +	uint8_t acc : 1;	/* AP is in accelerator mode */
>> +	uint8_t xcp : 1;	/* AP is in XCP-mode */
>> +	uint8_t n : 1;		/* AP extended addressing facility */
>> +	uint8_t pad_0 : 1;
>> +	uint8_t pad_1[3];
> 
> 
> Is there a reason why the 'Classification'  field is left out?
> 

It's not used in this library and therefore I chose to not name it to 
make structs a bit more readable.

> 
>> +	uint8_t at;
>> +	uint8_t nd;
>> +	uint8_t pad_6;
>> +	uint8_t pad_7 : 4;
>> +	uint8_t qd : 4;
>> +} __attribute__((packed))  __attribute__((aligned(8)));
>> +_Static_assert(sizeof(struct pqap_r2) == sizeof(uint64_t), "pqap_r2 size");
>> +
>> +bool ap_setup(void);
>> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>> +		 struct pqap_r2 *r2);
>> +int ap_pqap_qci(struct ap_config_info *info);
>> +#endif
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 7fce9f9d..4f6c627d 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -110,6 +110,7 @@ cflatobjs += lib/s390x/malloc_io.o
>>    cflatobjs += lib/s390x/uv.o
>>    cflatobjs += lib/s390x/sie.o
>>    cflatobjs += lib/s390x/fault.o
>> +cflatobjs += lib/s390x/ap.o
>>    
>>    OBJDIRS += lib/s390x
>>