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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/3/23 16:57, Pierre Morel wrote:

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@xxxxxxxxxxxxx>
---
   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.

Yes, because the names of the functions include the zeroize parts which makes it easier for developers to understand how they work instead of having a bool argument where they need to look up at which argument position it is.


Why this intermediate level?

So I don't need to repeat the function below for a different r0.fc, no?

[...]

   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)

I'd much rather grab a tapq and assert that ir == 0 so if someone alters the code they are responsible for giving this function a reset queue.

I'll add a comment that we expect ir == 0 for this function.



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


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

Yes



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

Isn't that covered by the assert above?



+
+	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();




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux