Re: [kvm-unit-tests PATCH v4 2/7] s390x: Add guest 2 AP test

[Janosch Frank <frankja@linux.ibm.com>]

On 2/20/24 17:38, Anthony Krowiak wrote:
> I made a couple of function name change suggestions, but those are not
> critical:
> 
> Acked-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> 
> On 2/2/24 9:59 AM, Janosch Frank wrote:
>> Add a test that checks the exceptions for the PQAP, NQAP and DQAP
>> adjunct processor (AP) crypto instructions.
>>
>> Since triggering the exceptions doesn't require actual AP hardware,
>> this test can run without complicated setup.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/Makefile      |   1 +
>>    s390x/ap.c          | 309 ++++++++++++++++++++++++++++++++++++++++++++
>>    s390x/unittests.cfg |   3 +
>>    3 files changed, 313 insertions(+)
>>    create mode 100644 s390x/ap.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 4f6c627d..6d28a5bf 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -42,6 +42,7 @@ tests += $(TEST_DIR)/exittime.elf
>>    tests += $(TEST_DIR)/ex.elf
>>    tests += $(TEST_DIR)/topology.elf
>>    tests += $(TEST_DIR)/sie-dat.elf
>> +tests += $(TEST_DIR)/ap.elf
>>    
>>    pv-tests += $(TEST_DIR)/pv-diags.elf
>>    pv-tests += $(TEST_DIR)/pv-icptcode.elf
>> diff --git a/s390x/ap.c b/s390x/ap.c
>> new file mode 100644
>> index 00000000..b3cee37a
>> --- /dev/null
>> +++ b/s390x/ap.c
>> @@ -0,0 +1,309 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * AP instruction G2 tests
>> + *
>> + * Copyright (c) 2024 IBM Corp
>> + *
>> + * Authors:
>> + *  Janosch Frank <frankja@linux.ibm.com>
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <interrupt.h>
>> +#include <bitops.h>
>> +#include <alloc_page.h>
>> +#include <asm/facility.h>
>> +#include <asm/time.h>
>> +#include <ap.h>
>> +
>> +/* For PQAP PGM checks where we need full control over the input */
>> +static void pqap(unsigned long grs[3])
>> +{
>> +	asm volatile(
>> +		"	lgr	0,%[r0]\n"
>> +		"	lgr	1,%[r1]\n"
>> +		"	lgr	2,%[r2]\n"
>> +		"	.insn	rre,0xb2af0000,0,0\n" /* PQAP */
>> +		::  [r0] "d" (grs[0]), [r1] "d" (grs[1]), [r2] "d" (grs[2])
>> +		: "cc", "memory", "0", "1", "2");
>> +}
>> +
>> +static void test_pgms_pqap(void)
> 
> 
> If I saw this function name without having read the patch description, I
> wouldn't have any idea what is being tested.
> 
> Maybe test_pqap_pgm_chk?

If I'm not mistaken, then it should be consistent with the pgm checks 
for other instructions but I don't mind renaming it.

> 
> 
>> +{
>> +	unsigned long grs[3] = {};
>> +	struct pqap_r0 *r0 = (struct pqap_r0 *)grs;
>> +	uint8_t *data = alloc_page();
>> +	uint16_t pgm;
>> +	int fails = 0;
>> +	int i;
>> +
>> +	report_prefix_push("pqap");
>> +
>> +	/* Wrong FC code */
>> +	report_prefix_push("invalid fc");
>> +	r0->fc = 42;
> 
> 
> Just out of curiosity, why 42? Why not some ridiculous number that will
> never be used for a function code, like 4294967295?

No particular reason.

Increasing the number would increase the buffer zone of invalid values 
but there's currently only a hand full of values defined anyway.
I might add a couple of 0s to this for the next version and call it a day.