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

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

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


> +};
> +
> +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");
> +
> +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
>