| Message ID | 20230330114244.35559-6-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add base AP support | expand |
On Thu, 30 Mar 2023 11:42:44 +0000 Janosch Frank <frankja@linux.ibm.com> wrote: > Test if the IRQ enablement is turned off on a reset or zeroize PQAP. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- [...] > 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)¬_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); same story here as in the previous patch, waiting for interrupts > + 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"); > + > + 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)¬_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); and here > + 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();
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)¬_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)¬_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();
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@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. 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)¬_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)¬_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();
On 4/4/23 13:40, Janosch Frank wrote: > 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@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. > > 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? question of taste anyway. [...] >> >> >>> + } 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? May be. This is the kind of thing where I find the implementation and documentation not very logical. - CC = 0 means that the instruction was processed correctly. - APQSW reports the status of the AP queue For any operation but TAPQ I understand that CC=3 if APQSW is != 0 but for TAPQ, if it is processed correctly it should give back the APQSW. Isn't it exactly what we ask the TAPQ to do? I am probably not the only one to think that CC for TAPQ is at least not useful, the Linux implementation ignores it. You are probably right but in doubt I would do as in Linux implementation and ignore CC,
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) +{ + 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)¬_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(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_push("zapq"); + + /* Enable IRQs which the resets will disable */ + aqic.ir = 1; + cc = ap_pqap_aqic(apn, qn, &apqsw, aqic, (uintptr_t)¬_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();
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(+)