| Message ID | 20190822111100.4444-1-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] s390x: Add diag308 subcode 0 testing | expand |
On 22.08.19 13:11, Janosch Frank wrote: > By adding a load reset routine to cstart.S we can also test the clear > reset done by subcode 0, as we now can restore our registers again. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > I managed to extract this from another bigger test, so let's add it to the bunch. > I'd be very happy about assembly review :-) > --- > s390x/cstart64.S | 27 +++++++++++++++++++++++++++ > s390x/diag308.c | 31 ++++++++++--------------------- > 2 files changed, 37 insertions(+), 21 deletions(-) > > diff --git a/s390x/cstart64.S b/s390x/cstart64.S > index dedfe80..47045e1 100644 > --- a/s390x/cstart64.S > +++ b/s390x/cstart64.S > @@ -145,6 +145,33 @@ memsetxc: > .endm > > .section .text > +/* > + * load_reset calling convention: > + * %r2 subcode (0 or 1) > + */ > +.globl load_reset > +load_reset: > + SAVE_REGS > + /* Save the first PSW word to the IPL PSW */ > + epsw %r0, %r1 > + st %r0, 0 > + /* Store the address and the bit for 31 bit addressing */ > + larl %r0, 0f > + oilh %r0, 0x8000 > + st %r0, 0x4 > + /* Do the reset */ > + diag %r0,%r2,0x308 > + /* Failure path */ > + xgr %r2, %r2 > + br %r14 > + /* Success path */ > + /* We lost cr0 due to the reset */ > +0: larl %r1, initial_cr0 > + lctlg %c0, %c0, 0(%r1) > + RESTORE_REGS > + lhi %r2, 1 > + br %r14 > + > pgm_int: > SAVE_REGS > brasl %r14, handle_pgm_int > diff --git a/s390x/diag308.c b/s390x/diag308.c > index f085b1a..baf9fd3 100644 > --- a/s390x/diag308.c > +++ b/s390x/diag308.c > @@ -21,32 +21,20 @@ static void test_priv(void) > check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION); > } > > + > /* > - * Check that diag308 with subcode 1 loads the PSW at address 0, i.e. > + * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e. > * that we can put a pointer into address 4 which then gets executed. > */ > +extern int load_reset(u64); > +static void test_subcode0(void) > +{ > + report("load modified clear done", load_reset(0)); > +} > + > static void test_subcode1(void) > { > - uint64_t saved_psw = *(uint64_t *)0; > - long subcode = 1; > - long ret, tmp; > - > - asm volatile ( > - " epsw %0,%1\n" > - " st %0,0\n" > - " larl %0,0f\n" > - " oilh %0,0x8000\n" > - " st %0,4\n" > - " diag 0,%2,0x308\n" > - " lghi %0,0\n" > - " j 1f\n" > - "0: lghi %0,1\n" > - "1:" > - : "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory"); > - > - *(uint64_t *)0 = saved_psw; > - > - report("load normal reset done", ret == 1); > + report("load normal reset done", load_reset(1)); > } > > /* Expect a specification exception when using an uneven register */ > @@ -107,6 +95,7 @@ static struct { > void (*func)(void); > } tests[] = { > { "privileged", test_priv }, > + { "subcode 0", test_subcode0 }, > { "subcode 1", test_subcode1 }, > { "subcode 5", test_subcode5 }, > { "subcode 6", test_subcode6 }, > So, in general I am wondering if we should restore the original IPL_PSW after we used it - is there any chance we might require the old value again (I guess we're fine with cpu resets)?
On 8/23/19 1:00 PM, David Hildenbrand wrote: > On 22.08.19 13:11, Janosch Frank wrote: >> By adding a load reset routine to cstart.S we can also test the clear >> reset done by subcode 0, as we now can restore our registers again. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> I managed to extract this from another bigger test, so let's add it to the bunch. >> I'd be very happy about assembly review :-) >> --- >> s390x/cstart64.S | 27 +++++++++++++++++++++++++++ >> s390x/diag308.c | 31 ++++++++++--------------------- >> 2 files changed, 37 insertions(+), 21 deletions(-) >> >> diff --git a/s390x/cstart64.S b/s390x/cstart64.S >> index dedfe80..47045e1 100644 >> --- a/s390x/cstart64.S >> +++ b/s390x/cstart64.S >> @@ -145,6 +145,33 @@ memsetxc: >> .endm >> >> .section .text >> +/* >> + * load_reset calling convention: >> + * %r2 subcode (0 or 1) >> + */ >> +.globl load_reset >> +load_reset: >> + SAVE_REGS >> + /* Save the first PSW word to the IPL PSW */ >> + epsw %r0, %r1 >> + st %r0, 0 >> + /* Store the address and the bit for 31 bit addressing */ >> + larl %r0, 0f >> + oilh %r0, 0x8000 >> + st %r0, 0x4 >> + /* Do the reset */ >> + diag %r0,%r2,0x308 >> + /* Failure path */ >> + xgr %r2, %r2 >> + br %r14 >> + /* Success path */ >> + /* We lost cr0 due to the reset */ >> +0: larl %r1, initial_cr0 >> + lctlg %c0, %c0, 0(%r1) >> + RESTORE_REGS >> + lhi %r2, 1 >> + br %r14 >> + >> pgm_int: >> SAVE_REGS >> brasl %r14, handle_pgm_int >> diff --git a/s390x/diag308.c b/s390x/diag308.c >> index f085b1a..baf9fd3 100644 >> --- a/s390x/diag308.c >> +++ b/s390x/diag308.c >> @@ -21,32 +21,20 @@ static void test_priv(void) >> check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION); >> } >> >> + >> /* >> - * Check that diag308 with subcode 1 loads the PSW at address 0, i.e. >> + * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e. >> * that we can put a pointer into address 4 which then gets executed. >> */ >> +extern int load_reset(u64); >> +static void test_subcode0(void) >> +{ >> + report("load modified clear done", load_reset(0)); >> +} >> + >> static void test_subcode1(void) >> { >> - uint64_t saved_psw = *(uint64_t *)0; >> - long subcode = 1; >> - long ret, tmp; >> - >> - asm volatile ( >> - " epsw %0,%1\n" >> - " st %0,0\n" >> - " larl %0,0f\n" >> - " oilh %0,0x8000\n" >> - " st %0,4\n" >> - " diag 0,%2,0x308\n" >> - " lghi %0,0\n" >> - " j 1f\n" >> - "0: lghi %0,1\n" >> - "1:" >> - : "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory"); >> - >> - *(uint64_t *)0 = saved_psw; >> - >> - report("load normal reset done", ret == 1); >> + report("load normal reset done", load_reset(1)); >> } >> >> /* Expect a specification exception when using an uneven register */ >> @@ -107,6 +95,7 @@ static struct { >> void (*func)(void); >> } tests[] = { >> { "privileged", test_priv }, >> + { "subcode 0", test_subcode0 }, >> { "subcode 1", test_subcode1 }, >> { "subcode 5", test_subcode5 }, >> { "subcode 6", test_subcode6 }, >> > > So, in general I am wondering if we should restore the original IPL_PSW > after we used it - is there any chance we might require the old value > again (I guess we're fine with cpu resets)? I currently don't see a need, but we could cache it in the restart old psw address. Or we just store back the two word constant.
On 23.08.19 13:33, Janosch Frank wrote: > On 8/23/19 1:00 PM, David Hildenbrand wrote: >> On 22.08.19 13:11, Janosch Frank wrote: >>> By adding a load reset routine to cstart.S we can also test the clear >>> reset done by subcode 0, as we now can restore our registers again. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> I managed to extract this from another bigger test, so let's add it to the bunch. >>> I'd be very happy about assembly review :-) >>> --- >>> s390x/cstart64.S | 27 +++++++++++++++++++++++++++ >>> s390x/diag308.c | 31 ++++++++++--------------------- >>> 2 files changed, 37 insertions(+), 21 deletions(-) >>> >>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S >>> index dedfe80..47045e1 100644 >>> --- a/s390x/cstart64.S >>> +++ b/s390x/cstart64.S >>> @@ -145,6 +145,33 @@ memsetxc: >>> .endm >>> >>> .section .text >>> +/* >>> + * load_reset calling convention: >>> + * %r2 subcode (0 or 1) >>> + */ >>> +.globl load_reset >>> +load_reset: >>> + SAVE_REGS >>> + /* Save the first PSW word to the IPL PSW */ >>> + epsw %r0, %r1 >>> + st %r0, 0 >>> + /* Store the address and the bit for 31 bit addressing */ >>> + larl %r0, 0f >>> + oilh %r0, 0x8000 >>> + st %r0, 0x4 >>> + /* Do the reset */ >>> + diag %r0,%r2,0x308 >>> + /* Failure path */ >>> + xgr %r2, %r2 >>> + br %r14 >>> + /* Success path */ >>> + /* We lost cr0 due to the reset */ >>> +0: larl %r1, initial_cr0 >>> + lctlg %c0, %c0, 0(%r1) >>> + RESTORE_REGS >>> + lhi %r2, 1 >>> + br %r14 >>> + >>> pgm_int: >>> SAVE_REGS >>> brasl %r14, handle_pgm_int >>> diff --git a/s390x/diag308.c b/s390x/diag308.c >>> index f085b1a..baf9fd3 100644 >>> --- a/s390x/diag308.c >>> +++ b/s390x/diag308.c >>> @@ -21,32 +21,20 @@ static void test_priv(void) >>> check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION); >>> } >>> >>> + >>> /* >>> - * Check that diag308 with subcode 1 loads the PSW at address 0, i.e. >>> + * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e. >>> * that we can put a pointer into address 4 which then gets executed. >>> */ >>> +extern int load_reset(u64); >>> +static void test_subcode0(void) >>> +{ >>> + report("load modified clear done", load_reset(0)); >>> +} >>> + >>> static void test_subcode1(void) >>> { >>> - uint64_t saved_psw = *(uint64_t *)0; >>> - long subcode = 1; >>> - long ret, tmp; >>> - >>> - asm volatile ( >>> - " epsw %0,%1\n" >>> - " st %0,0\n" >>> - " larl %0,0f\n" >>> - " oilh %0,0x8000\n" >>> - " st %0,4\n" >>> - " diag 0,%2,0x308\n" >>> - " lghi %0,0\n" >>> - " j 1f\n" >>> - "0: lghi %0,1\n" >>> - "1:" >>> - : "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory"); >>> - >>> - *(uint64_t *)0 = saved_psw; >>> - >>> - report("load normal reset done", ret == 1); >>> + report("load normal reset done", load_reset(1)); >>> } >>> >>> /* Expect a specification exception when using an uneven register */ >>> @@ -107,6 +95,7 @@ static struct { >>> void (*func)(void); >>> } tests[] = { >>> { "privileged", test_priv }, >>> + { "subcode 0", test_subcode0 }, >>> { "subcode 1", test_subcode1 }, >>> { "subcode 5", test_subcode5 }, >>> { "subcode 6", test_subcode6 }, >>> >> >> So, in general I am wondering if we should restore the original IPL_PSW >> after we used it - is there any chance we might require the old value >> again (I guess we're fine with cpu resets)? > > I currently don't see a need, but we could cache it in the restart old > psw address. Or we just store back the two word constant. > If there's no need right no, I guess we can skip that. Was just wondering.
On 8/22/19 1:11 PM, Janosch Frank wrote: > By adding a load reset routine to cstart.S we can also test the clear > reset done by subcode 0, as we now can restore our registers again. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > I managed to extract this from another bigger test, so let's add it to the bunch. > I'd be very happy about assembly review :-) FWIW, the assembly code looks fine to me. > --- > s390x/cstart64.S | 27 +++++++++++++++++++++++++++ > s390x/diag308.c | 31 ++++++++++--------------------- > 2 files changed, 37 insertions(+), 21 deletions(-) > > diff --git a/s390x/cstart64.S b/s390x/cstart64.S > index dedfe80..47045e1 100644 > --- a/s390x/cstart64.S > +++ b/s390x/cstart64.S > @@ -145,6 +145,33 @@ memsetxc: > .endm > > .section .text > +/* > + * load_reset calling convention: > + * %r2 subcode (0 or 1) > + */ > +.globl load_reset > +load_reset: Maybe rather name the function diag308_load_reset so that it is clear that it belongs to the diag308 test? Or are you going to re-use this function in other tests later? Anyway, Reviewed-by: Thomas Huth <thuth@redhat.com>
On 8/23/19 4:12 PM, Thomas Huth wrote: > On 8/22/19 1:11 PM, Janosch Frank wrote: >> By adding a load reset routine to cstart.S we can also test the clear >> reset done by subcode 0, as we now can restore our registers again. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> I managed to extract this from another bigger test, so let's add it to the bunch. >> I'd be very happy about assembly review :-) > > FWIW, the assembly code looks fine to me. > >> --- >> s390x/cstart64.S | 27 +++++++++++++++++++++++++++ >> s390x/diag308.c | 31 ++++++++++--------------------- >> 2 files changed, 37 insertions(+), 21 deletions(-) >> >> diff --git a/s390x/cstart64.S b/s390x/cstart64.S >> index dedfe80..47045e1 100644 >> --- a/s390x/cstart64.S >> +++ b/s390x/cstart64.S >> @@ -145,6 +145,33 @@ memsetxc: >> .endm >> >> .section .text >> +/* >> + * load_reset calling convention: >> + * %r2 subcode (0 or 1) >> + */ >> +.globl load_reset >> +load_reset: > > Maybe rather name the function diag308_load_reset so that it is clear > that it belongs to the diag308 test? Sure > Or are you going to re-use this function in other tests later? I currently have no such plans But I'm thinking about a way to check the CPU registers in combination with smp. So it might be extended. > > Anyway, > Reviewed-by: Thomas Huth <thuth@redhat.com> > Thanks!
diff --git a/s390x/cstart64.S b/s390x/cstart64.S index dedfe80..47045e1 100644 --- a/s390x/cstart64.S +++ b/s390x/cstart64.S @@ -145,6 +145,33 @@ memsetxc: .endm .section .text +/* + * load_reset calling convention: + * %r2 subcode (0 or 1) + */ +.globl load_reset +load_reset: + SAVE_REGS + /* Save the first PSW word to the IPL PSW */ + epsw %r0, %r1 + st %r0, 0 + /* Store the address and the bit for 31 bit addressing */ + larl %r0, 0f + oilh %r0, 0x8000 + st %r0, 0x4 + /* Do the reset */ + diag %r0,%r2,0x308 + /* Failure path */ + xgr %r2, %r2 + br %r14 + /* Success path */ + /* We lost cr0 due to the reset */ +0: larl %r1, initial_cr0 + lctlg %c0, %c0, 0(%r1) + RESTORE_REGS + lhi %r2, 1 + br %r14 + pgm_int: SAVE_REGS brasl %r14, handle_pgm_int diff --git a/s390x/diag308.c b/s390x/diag308.c index f085b1a..baf9fd3 100644 --- a/s390x/diag308.c +++ b/s390x/diag308.c @@ -21,32 +21,20 @@ static void test_priv(void) check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION); } + /* - * Check that diag308 with subcode 1 loads the PSW at address 0, i.e. + * Check that diag308 with subcode 0 and 1 loads the PSW at address 0, i.e. * that we can put a pointer into address 4 which then gets executed. */ +extern int load_reset(u64); +static void test_subcode0(void) +{ + report("load modified clear done", load_reset(0)); +} + static void test_subcode1(void) { - uint64_t saved_psw = *(uint64_t *)0; - long subcode = 1; - long ret, tmp; - - asm volatile ( - " epsw %0,%1\n" - " st %0,0\n" - " larl %0,0f\n" - " oilh %0,0x8000\n" - " st %0,4\n" - " diag 0,%2,0x308\n" - " lghi %0,0\n" - " j 1f\n" - "0: lghi %0,1\n" - "1:" - : "=&d"(ret), "=&d"(tmp) : "d"(subcode) : "memory"); - - *(uint64_t *)0 = saved_psw; - - report("load normal reset done", ret == 1); + report("load normal reset done", load_reset(1)); } /* Expect a specification exception when using an uneven register */ @@ -107,6 +95,7 @@ static struct { void (*func)(void); } tests[] = { { "privileged", test_priv }, + { "subcode 0", test_subcode0 }, { "subcode 1", test_subcode1 }, { "subcode 5", test_subcode5 }, { "subcode 6", test_subcode6 },
By adding a load reset routine to cstart.S we can also test the clear reset done by subcode 0, as we now can restore our registers again. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- I managed to extract this from another bigger test, so let's add it to the bunch. I'd be very happy about assembly review :-) --- s390x/cstart64.S | 27 +++++++++++++++++++++++++++ s390x/diag308.c | 31 ++++++++++--------------------- 2 files changed, 37 insertions(+), 21 deletions(-)