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

[Harald Freudenberger <freude@linux.ibm.com>]

On 2024-02-05 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>
>> ---
>>   lib/s390x/ap.c | 97 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/ap.h | 88 +++++++++++++++++++++++++++++++++++++++++++++
>>   s390x/Makefile |  1 +
>>   3 files changed, 186 insertions(+)
>>   create mode 100644 lib/s390x/ap.c
>>   create mode 100644 lib/s390x/ap.h
>> 
>> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
>> new file mode 100644
>> index 00000000..9560ff64
>> --- /dev/null
>> +++ b/lib/s390x/ap.c
>> @@ -0,0 +1,97 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * AP crypto functions
>> + *
>> + * 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>
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <interrupt.h>
>> +#include <ap.h>
>> +#include <asm/time.h>
>> +#include <asm/facility.h>
>> +
>> +int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status 
>> *apqsw,
>> +		 struct pqap_r2 *r2)
>> +{
>> +	struct pqap_r0 r0 = {
>> +		.ap = ap,
>> +		.qn = qn,
>> +		.fc = PQAP_TEST_APQ
>> +	};
>> +	uint64_t bogus_cc = 2;
>> +	int cc;
>> +
>> +	/*
>> +	 * Test AP Queue
>> +	 *
>> +	 * Writes AP configuration information to the memory pointed
>> +	 * at by GR2.
>> +	 *
>> +	 * Inputs: GR0
>> +	 * Outputs: GR1 (APQSW), GR2 (tapq data)
>> +	 * Synchronous
>> +	 */
>> +	asm volatile(
>> +		"       tmll	%[bogus_cc],3\n"
>> +		"	lgr	0,%[r0]\n"
>> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
>> +		"	stg	1,%[apqsw]\n"
>> +		"	stg	2,%[r2]\n"
>> +		"	ipm	%[cc]\n"
>> +		"	srl	%[cc],28\n"
>> +		: [apqsw] "=&T" (*apqsw), [r2] "=&T" (*r2), [cc] "=&d" (cc)
>> +		: [r0] "d" (r0), [bogus_cc] "d" (bogus_cc));
>> +
>> +	return cc;
>> +}
>> +
>> +int ap_pqap_qci(struct ap_config_info *info)
>> +{
>> +	struct pqap_r0 r0 = { .fc = PQAP_QUERY_AP_CONF_INFO };
>> +	uint64_t bogus_cc = 2;
>> +	int cc;
>> +
>> +	/*
>> +	 * Query AP Configuration Information
>> +	 *
>> +	 * Writes AP configuration information to the memory pointed
>> +	 * at by GR2.
>> +	 *
>> +	 * Inputs: GR0, GR2 (QCI block address)
>> +	 * Outputs: memory at GR2 address
>> +	 * Synchronous
>> +	 */
>> +	asm volatile(
>> +		"       tmll	%[bogus_cc],3\n"
>> +		"	lgr	0,%[r0]\n"
>> +		"	lgr	2,%[info]\n"
>> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
>> +		"	ipm	%[cc]\n"
>> +		"	srl	%[cc],28\n"
>> +		: [cc] "=&d" (cc)
>> +		: [r0] "d" (r0), [info] "d" (info), [bogus_cc] "d" (bogus_cc)
>> +		: "cc", "memory", "0", "2");
>> +
>> +	return cc;
>> +}
>> +
>> +/* 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:
> 
> 
> static inline bool ap_instructions_available(void)
> 
> {
>     unsigned long reg0 = 0x0000;
>     unsigned long reg1 = 0;
> 
>     asm volatile(
>         "    lgr    0,%[reg0]\n"        /* qid into gr0 */
>         "    lghi    1,0\n"            /* 0 into gr1 */
>         "    lghi    2,0\n"            /* 0 into gr2 */
>         "    .insn    rre,0xb2af0000,0,0\n"    /* PQAP(TAPQ) */
>         "0:    la    %[reg1],1\n"        /* 1 into reg1 */
>         "1:\n"
>         EX_TABLE(0b, 1b)
>         : [reg1] "+&d" (reg1)
>         : [reg0] "d" (reg0)
>         : "cc", "0", "1", "2");
>     return reg1 != 0;
> }

Well, as Janos wrote - this lib functions rely on having QCI available.
However, be carefully using the above function as it is setting up
an exception table entry which is to catch the illegal instruction
which will arise when no AP bus support is available.

>> +
>> +	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,

What is that       ^^^^^^^^^^^^^ ?
The QACT ? But that's only one PQAP subfuntion.

>> +	PQAP_BEST_AP,

And this ^^^^^^^^^^^^ ? Never heard about.

> 
> 
> Maybe use abbreviations like your function names above?
> 
> 	PQAP_TAPQ,
> 	PQAP_RAPQ,
> 	PQAP_ZAPQ,
> 	PQAP_AQIC,
> 	PQAP_QCI,
> 	PQAP_QACT,
> 	PQAP_QBAP
> 
> 
>> +};
>> +
>> +struct ap_queue_status {
>> +	uint32_t pad0;			/* Ignored padding for zArch  */
> 
> 
> Bit 0 is the E (queue empty) bit.
> 
> 
>> +	uint32_t empty		: 1;
>> +	uint32_t replies_waiting: 1;
>> +	uint32_t full		: 1;
>> +	uint32_t pad1		: 4;
>> +	uint32_t irq_enabled	: 1;
>> +	uint32_t rc		: 8;
>> +	uint32_t pad2		: 16;
>> +} __attribute__((packed))  __attribute__((aligned(8)));
>> +_Static_assert(sizeof(struct ap_queue_status) == sizeof(uint64_t), 
>> "APQSW size");
>> +
>> +struct ap_config_info {
>> +	uint8_t apsc	 : 1;	/* S bit */
>> +	uint8_t apxa	 : 1;	/* N bit */
>> +	uint8_t qact	 : 1;	/* C bit */
>> +	uint8_t rc8a	 : 1;	/* R bit */
>> +	uint8_t l	 : 1;	/* L bit */
>> +	uint8_t lext	 : 3;	/* Lext bits */
>> +	uint8_t reserved2[3];
>> +	uint8_t Na;		/* max # of APs - 1 */
>> +	uint8_t Nd;		/* max # of Domains - 1 */
>> +	uint8_t reserved6[10];
>> +	uint32_t apm[8];	/* AP ID mask */
>> +	uint32_t aqm[8];	/* AP (usage) queue mask */
>> +	uint32_t adm[8];	/* AP (control) domain mask */
>> +	uint8_t _reserved4[16];
>> +} __attribute__((aligned(8))) __attribute__ ((__packed__));
>> +_Static_assert(sizeof(struct ap_config_info) == 128, "PQAP QCI 
>> size");
>> +
>> +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'
> 
> 
>> +} __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?
> 
> 
>> +	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");
>> +

Isn't this all going into the kernel? So consider using the
arch/s390/include/asm/ap.h header file instead of re-defining the 
structs.
Feel free to improve this file with reworked structs and field names if
that's required.

>> +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
>>