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@xxxxxxxxxx>
>> ---
>> 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
