| Message ID | 20170619091538.10157-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19.06.2017 11:15, David Hildenbrand wrote: > Let's add a test case for the STORE CPU ID instruction. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > lib/s390x/asm/arch_def.h | 8 ++++++++ > s390x/intercept.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index 07d467e..72e5c60 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -143,4 +143,12 @@ struct lowcore { > #define PGM_INT_CODE_CRYPTO_OPERATION 0x119 > #define PGM_INT_CODE_TX_ABORTED_EVENT 0x200 > > +struct cpuid { > + uint64_t version : 8; > + uint64_t id : 24; > + uint64_t type : 16; > + uint64_t format : 1; > + uint64_t reserved : 15; > +}; > + > #endif > diff --git a/s390x/intercept.c b/s390x/intercept.c > index 4558860..09c9a62 100644 > --- a/s390x/intercept.c > +++ b/s390x/intercept.c > @@ -105,6 +105,32 @@ static void test_stap(void) > check_pgm_int_code(PGM_INT_CODE_ADDRESSING); > } > > +/* Test the STORE CPU ID instruction */ > +static void test_stidp(void) > +{ > + struct cpuid id = {}; > + > + asm volatile ("stidp %0\n" : "+Q"(id)); > + report("id.type != 0", id.type != 0); > + report("id.version == 0 || id.version == 0xff)", Superfluous parenthesis -----------------------------^ > + id.version == 0 || id.version == 0xff); > + report("id.reserved == 0", !id.reserved); I also think you should not use C code in the text output here. Not everybody who's running the kvm-unit-tests might be familiar with the C language syntax. So it'd be nicer to write something like "reserved bits are zero" instead of "id.reserved == 0" ? > + expect_pgm_int(); > + low_prot_enable(); > + asm volatile ("stidp 0(%0)\n" : : "r"(8)); > + low_prot_disable(); > + check_pgm_int_code(PGM_INT_CODE_PROTECTION); > + > + expect_pgm_int(); > + asm volatile ("stidp 0(%0)\n" : : "r"(1)); > + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION); > + > + expect_pgm_int(); > + asm volatile ("stidp 0(%0)\n" : : "r"(-8)); > + check_pgm_int_code(PGM_INT_CODE_ADDRESSING); > +} > + > /* Test the TEST BLOCK instruction */ > static void test_testblock(void) > { > @@ -152,6 +178,7 @@ struct { > { "stpx", test_stpx, false }, > { "spx", test_spx, false }, > { "stap", test_stap, false }, > + { "stidp", test_stidp, false }, > { "testblock", test_testblock, false }, > { NULL, NULL, false } > }; > Apart from the text output, the patch looks fine to me. Thomas
On 19.06.2017 12:44, Thomas Huth wrote: > On 19.06.2017 11:15, David Hildenbrand wrote: >> Let's add a test case for the STORE CPU ID instruction. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> lib/s390x/asm/arch_def.h | 8 ++++++++ >> s390x/intercept.c | 27 +++++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h >> index 07d467e..72e5c60 100644 >> --- a/lib/s390x/asm/arch_def.h >> +++ b/lib/s390x/asm/arch_def.h >> @@ -143,4 +143,12 @@ struct lowcore { >> #define PGM_INT_CODE_CRYPTO_OPERATION 0x119 >> #define PGM_INT_CODE_TX_ABORTED_EVENT 0x200 >> >> +struct cpuid { >> + uint64_t version : 8; >> + uint64_t id : 24; >> + uint64_t type : 16; >> + uint64_t format : 1; >> + uint64_t reserved : 15; >> +}; >> + >> #endif >> diff --git a/s390x/intercept.c b/s390x/intercept.c >> index 4558860..09c9a62 100644 >> --- a/s390x/intercept.c >> +++ b/s390x/intercept.c >> @@ -105,6 +105,32 @@ static void test_stap(void) >> check_pgm_int_code(PGM_INT_CODE_ADDRESSING); >> } >> >> +/* Test the STORE CPU ID instruction */ >> +static void test_stidp(void) >> +{ >> + struct cpuid id = {}; >> + >> + asm volatile ("stidp %0\n" : "+Q"(id)); >> + report("id.type != 0", id.type != 0); >> + report("id.version == 0 || id.version == 0xff)", > > Superfluous parenthesis -----------------------------^ Whops, yes, thanks. > >> + id.version == 0 || id.version == 0xff); >> + report("id.reserved == 0", !id.reserved); > > I also think you should not use C code in the text output here. > Not everybody who's running the kvm-unit-tests might be familiar with > the C language syntax. So it'd be nicer to write something like > "reserved bits are zero" instead of "id.reserved == 0" ? That's also what we have in s390x/selftest.c, suggested by Radim. If someone cannot read C code, that person for sure will also not be able to fix that bug. > >> + expect_pgm_int(); >> + low_prot_enable(); >> + asm volatile ("stidp 0(%0)\n" : : "r"(8)); >> + low_prot_disable(); >> + check_pgm_int_code(PGM_INT_CODE_PROTECTION); >> + >> + expect_pgm_int(); >> + asm volatile ("stidp 0(%0)\n" : : "r"(1)); >> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION); >> + >> + expect_pgm_int(); >> + asm volatile ("stidp 0(%0)\n" : : "r"(-8)); >> + check_pgm_int_code(PGM_INT_CODE_ADDRESSING); >> +} >> + >> /* Test the TEST BLOCK instruction */ >> static void test_testblock(void) >> { >> @@ -152,6 +178,7 @@ struct { >> { "stpx", test_stpx, false }, >> { "spx", test_spx, false }, >> { "stap", test_stap, false }, >> + { "stidp", test_stidp, false }, >> { "testblock", test_testblock, false }, >> { NULL, NULL, false } >> }; >> > > Apart from the text output, the patch looks fine to me. > > Thomas > Thanks!
2017-06-19 14:21+0200, David Hildenbrand: > On 19.06.2017 12:44, Thomas Huth wrote: > > On 19.06.2017 11:15, David Hildenbrand wrote: > >> @@ -105,6 +105,32 @@ static void test_stap(void) > >> + report("id.version == 0 || id.version == 0xff)", > > > > Superfluous parenthesis -----------------------------^ > > Whops, yes, thanks. This pattern could use a macro ;) #define report(x) report(#x, x) > >> + id.version == 0 || id.version == 0xff); > >> + report("id.reserved == 0", !id.reserved); > > > > I also think you should not use C code in the text output here. > > Not everybody who's running the kvm-unit-tests might be familiar with > > the C language syntax. So it'd be nicer to write something like > > "reserved bits are zero" instead of "id.reserved == 0" ? > > That's also what we have in s390x/selftest.c, suggested by Radim. The main purpose is to find the place that failed in the code (any unique string qualifies), but it is desirable to explain what is tested. Please use a simplification in natural language if you feel that it provides better information. (For s390x/selftest.c, I was mainly proposing a different code structure and unique messages. Using C-like code to describe the format of arguments just allowed the thing to fit on one line while remaining understandable, IMO.) > If someone cannot read C code, that person for sure will also not be > able to fix that bug. Well said. The audience is expected to understand the test and the source code of the condition looks ok here.
On 19.06.2017 19:35, Radim Krčmář wrote: > 2017-06-19 14:21+0200, David Hildenbrand: >> On 19.06.2017 12:44, Thomas Huth wrote: >>> On 19.06.2017 11:15, David Hildenbrand wrote: >>>> @@ -105,6 +105,32 @@ static void test_stap(void) >>>> + report("id.version == 0 || id.version == 0xff)", >>> >>> Superfluous parenthesis -----------------------------^ >> >> Whops, yes, thanks. > > This pattern could use a macro ;) > > #define report(x) report(#x, x) After thinking 5 minutes about a possible name for this and not liking the results, I decided to simply go for a text output as Thomas suggested for now. :)
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h index 07d467e..72e5c60 100644 --- a/lib/s390x/asm/arch_def.h +++ b/lib/s390x/asm/arch_def.h @@ -143,4 +143,12 @@ struct lowcore { #define PGM_INT_CODE_CRYPTO_OPERATION 0x119 #define PGM_INT_CODE_TX_ABORTED_EVENT 0x200 +struct cpuid { + uint64_t version : 8; + uint64_t id : 24; + uint64_t type : 16; + uint64_t format : 1; + uint64_t reserved : 15; +}; + #endif diff --git a/s390x/intercept.c b/s390x/intercept.c index 4558860..09c9a62 100644 --- a/s390x/intercept.c +++ b/s390x/intercept.c @@ -105,6 +105,32 @@ static void test_stap(void) check_pgm_int_code(PGM_INT_CODE_ADDRESSING); } +/* Test the STORE CPU ID instruction */ +static void test_stidp(void) +{ + struct cpuid id = {}; + + asm volatile ("stidp %0\n" : "+Q"(id)); + report("id.type != 0", id.type != 0); + report("id.version == 0 || id.version == 0xff)", + id.version == 0 || id.version == 0xff); + report("id.reserved == 0", !id.reserved); + + expect_pgm_int(); + low_prot_enable(); + asm volatile ("stidp 0(%0)\n" : : "r"(8)); + low_prot_disable(); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); + + expect_pgm_int(); + asm volatile ("stidp 0(%0)\n" : : "r"(1)); + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION); + + expect_pgm_int(); + asm volatile ("stidp 0(%0)\n" : : "r"(-8)); + check_pgm_int_code(PGM_INT_CODE_ADDRESSING); +} + /* Test the TEST BLOCK instruction */ static void test_testblock(void) { @@ -152,6 +178,7 @@ struct { { "stpx", test_stpx, false }, { "spx", test_spx, false }, { "stap", test_stap, false }, + { "stidp", test_stidp, false }, { "testblock", test_testblock, false }, { NULL, NULL, false } };
Let's add a test case for the STORE CPU ID instruction. Signed-off-by: David Hildenbrand <david@redhat.com> --- lib/s390x/asm/arch_def.h | 8 ++++++++ s390x/intercept.c | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)