[PATCH v1] s390x: add stidp interception test

[PATCH v1] s390x: add stidp interception test

[David Hildenbrand]

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)",
+	       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 }
 };
-- 
2.9.3

Re: [PATCH v1] s390x: add stidp interception test

[Thomas Huth]

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

Re: [PATCH v1] s390x: add stidp interception test

[David Hildenbrand]

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!

-- 

Thanks,

David

Re: [PATCH v1] s390x: add stidp interception test

[Radim Krčmář]

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.

Re: [PATCH v1] s390x: add stidp interception test

[David Hildenbrand]

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. :)

-- 

Thanks,

David