Re: [kvm-unit-tests PATCH 5/5] s390x: ap: Add reset tests

[Pierre Morel <pmorel@linux.ibm.com>]

On 3/30/23 13:42, Janosch Frank wrote:
> Test if the IRQ enablement is turned off on a reset or zeroize PQAP.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/ap.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/ap.h |  4 +++
>   s390x/ap.c     | 52 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 124 insertions(+)
>
> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
> index aaf5b4b9..d969b2a5 100644
> --- a/lib/s390x/ap.c
> +++ b/lib/s390x/ap.c
> @@ -113,6 +113,74 @@ int ap_pqap_qci(struct ap_config_info *info)
>   	return cc;
>   }
>   
> +static int pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *r1,
> +		      bool zeroize)


NIT. Personal opinion, I find using this bool a little obfuscating and I 
would have prefer 2 different functions.

I see you added a ap_pqap_reset() and ap_pqap_zeroize() next in the code.

Why this intermediate level?


> +{
> +	struct pqap_r0 r0 = {};
> +	int cc;
> +
> +	/*
> +	 * Reset/zeroize AP Queue
> +	 *
> +	 * Resets/zeroizes a queue and disables IRQs
> +	 *
> +	 * Inputs: 0
> +	 * Outputs: 1
> +	 * Asynchronous
> +	 */
> +	r0.ap = ap;
> +	r0.qn = qn;
> +	r0.fc = zeroize ? PQAP_ZEROIZE_APQ : PQAP_RESET_APQ;
> +	asm volatile(
> +		"	lgr	0,%[r0]\n"
> +		"	lgr	1,%[r1]\n"
> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28\n"
> +		: [r1] "+&d" (r1), [cc] "=&d" (cc)
> +		: [r0] "d" (r0)
> +		: "memory");
> +
> +	return cc;
> +}
> +
> +static int pqap_reset_wait(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
> +			   bool zeroize)
> +{
> +	struct pqap_r2 r2 = {};
> +	int cc;
> +
> +	cc = pqap_reset(ap, qn, apqsw, zeroize);
> +	/* On a cc == 3 / error we don't need to wait */
> +	if (cc)
> +		return cc;
> +
> +	/*
> +	 * TAPQ returns AP_RC_RESET_IN_PROGRESS if a reset is being
> +	 * processed
> +	 */
> +	do {
> +		cc = ap_pqap_tapq(ap, qn, apqsw, &r2);
> +		/* Give it some time to process before the retry */
> +		mdelay(20);
> +	} while (apqsw->rc == AP_RC_RESET_IN_PROGRESS);
> +
> +	if (apqsw->rc)
> +		printf("Wait for reset failed on ap %d queue %d with tapq rc %d.",
> +			ap, qn, apqsw->rc);
> +	return cc;
> +}
> +
> +int ap_pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw)
> +{
> +	return pqap_reset_wait(ap, qn, apqsw, false);
> +}
> +
> +int ap_pqap_reset_zeroize(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw)
> +{
> +	return pqap_reset_wait(ap, qn, apqsw, true);
> +}
> +
>   static int ap_get_apqn(uint8_t *ap, uint8_t *qn)
>   {
>   	unsigned long *ptr;
> diff --git a/lib/s390x/ap.h b/lib/s390x/ap.h
> index 3f9e2eb6..f9343b5f 100644
> --- a/lib/s390x/ap.h
> +++ b/lib/s390x/ap.h
> @@ -12,6 +12,8 @@
>   #ifndef _S390X_AP_H_
>   #define _S390X_AP_H_
>   
> +#define AP_RC_RESET_IN_PROGRESS	0x02
> +
>   enum PQAP_FC {
>   	PQAP_TEST_APQ,
>   	PQAP_RESET_APQ,
> @@ -94,6 +96,8 @@ _Static_assert(sizeof(struct ap_qirq_ctrl) == sizeof(uint64_t),
>   int ap_setup(uint8_t *ap, uint8_t *qn);
>   int ap_pqap_tapq(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>   		 struct pqap_r2 *r2);
> +int ap_pqap_reset(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
> +int ap_pqap_reset_zeroize(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw);
>   int ap_pqap_qci(struct ap_config_info *info);
>   int ap_pqap_aqic(uint8_t ap, uint8_t qn, struct ap_queue_status *apqsw,
>   		 struct ap_qirq_ctrl aqic, unsigned long addr);
> diff --git a/s390x/ap.c b/s390x/ap.c
> index 31dcfe29..47b4f832 100644
> --- a/s390x/ap.c
> +++ b/s390x/ap.c
> @@ -341,6 +341,57 @@ static void test_pqap_aqic(void)
>   	report_prefix_pop();
>   }
>   
> +static void test_pqap_resets(void)
> +{
> +	struct ap_queue_status apqsw = {};
> +	static uint8_t not_ind_byte;
> +	struct ap_qirq_ctrl aqic = {};
> +	struct pqap_r2 r2 = {};
> +
> +	int cc;
> +
> +	report_prefix_push("pqap");
> +	report_prefix_push("rapq");
> +
> +	/* Enable IRQs which the resets will disable */
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 0 && apqsw.rc == 0, "enable");

Depending on history I think we could have apqsw == 07 here.

(interrupt already set as requested)


> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);


may be a little delay before retry as you do above for ap_reset_wait()?


> +	} while (cc == 0 && apqsw.irq_enabled == 0);
> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
> +
> +	ap_pqap_reset(apn, qn, &apqsw);
> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	assert(!cc);
> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");

shouldn't we check that the APQ is fine apqsw.rc == 0 ?


> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("zapq");
> +
> +	/* Enable IRQs which the resets will disable */
> +	aqic.ir = 1;
> +	cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)&not_ind_byte);
> +	report(cc == 0 && apqsw.rc == 0, "enable");
> +
> +	do {
> +		cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	} while (cc == 0 && apqsw.irq_enabled == 0);
> +	report(apqsw.irq_enabled == 1, "IRQs enabled");
> +
> +	ap_pqap_reset_zeroize(apn, qn, &apqsw);
> +	cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
> +	assert(!cc);
> +	report(apqsw.irq_enabled == 0, "IRQs have been disabled");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +}
> +
>   int main(void)
>   {
>   	int setup_rc = ap_setup(&apn, &qn);
> @@ -362,6 +413,7 @@ int main(void)
>   		goto done;
>   	}
>   	test_pqap_aqic();
> +	test_pqap_resets();
>   
>   done:
>   	report_prefix_pop();