Re: [PATCH 3/3] s390x: mvpg: Add SIE mvpg test

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

On 6/24/21 6:27 PM, Thomas Huth wrote:
> On 24/06/2021 14.01, Janosch Frank wrote:
>> Let's also check the PEI values to make sure our VSIE implementation
>> is correct.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   s390x/Makefile                  |   2 +
>>   s390x/mvpg-sie.c                | 150 ++++++++++++++++++++++++++++++++
>>   s390x/snippets/c/mvpg-snippet.c |  33 +++++++
>>   s390x/unittests.cfg             |   3 +
>>   4 files changed, 188 insertions(+)
>>   create mode 100644 s390x/mvpg-sie.c
>>   create mode 100644 s390x/snippets/c/mvpg-snippet.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index ba32f4c..07af26d 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -23,6 +23,7 @@ tests += $(TEST_DIR)/sie.elf
>>   tests += $(TEST_DIR)/mvpg.elf
>>   tests += $(TEST_DIR)/uv-host.elf
>>   tests += $(TEST_DIR)/edat.elf
>> +tests += $(TEST_DIR)/mvpg-sie.elf
>>   
>>   tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>   ifneq ($(HOST_KEY_DOCUMENT),)
>> @@ -82,6 +83,7 @@ snippet_asmlib = $(SNIPPET_DIR)/c/cstart.o
>>   
>>   # perquisites (=guests) for the snippet hosts.
>>   # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
>> +$(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin
>>   
>>   $(SNIPPET_DIR)/asm/%.gbin: $(SNIPPET_DIR)/asm/%.o $(FLATLIBS)
>>   	$(OBJCOPY) -O binary $(patsubst %.gbin,%.o,$@) $@
>> diff --git a/s390x/mvpg-sie.c b/s390x/mvpg-sie.c
>> new file mode 100644
>> index 0000000..a18c1b0
>> --- /dev/null
>> +++ b/s390x/mvpg-sie.c
>> @@ -0,0 +1,150 @@
>> +#include <libcflat.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm-generic/barrier.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/pgtable.h>
>> +#include <mmu.h>
>> +#include <asm/page.h>
>> +#include <asm/facility.h>
>> +#include <asm/mem.h>
>> +#include <asm/sigp.h>
>> +#include <smp.h>
>> +#include <alloc_page.h>
>> +#include <bitops.h>
>> +#include <vm.h>
>> +#include <sclp.h>
>> +#include <sie.h>
> 
> The list of headers that get included here is rather long for this file that 
> is rather short ... e.g. do we really need such headers like sigp.h and 
> sclp.h here?

sclp.h is used for the sie facility check.
I've removed asm/sigp.h, smp.h and bitops.h

> 
>> +static u8 *guest;
>> +static u8 *guest_instr;
>> +static struct vm vm;
>> +
>> +static uint8_t *src;
>> +static uint8_t *dst;
>> +static uint8_t *cmp;
>> +
>> +extern const char _binary_s390x_snippets_c_mvpg_snippet_gbin_start[];
>> +extern const char _binary_s390x_snippets_c_mvpg_snippet_gbin_end[];
>> +int binary_size;
>> +
>> +static void sie(struct vm *vm)
>> +{
>> +	/* Reset icptcode so we don't trip below */
>> +	vm->sblk->icptcode = 0;
>> +
>> +	while (vm->sblk->icptcode == 0) {
>> +		sie64a(vm->sblk, &vm->save_area);
>> +		if (vm->sblk->icptcode == ICPT_VALIDITY)
>> +			assert(0);
> 
> 
>                  assert(vm->sblk->icptcode != ICPT_VALIDITY)
> 
> ?
> 
>> +	}
>> +	vm->save_area.guest.grs[14] = vm->sblk->gg14;
>> +	vm->save_area.guest.grs[15] = vm->sblk->gg15;
>> +}
>> +
>> +static void test_mvpg_pei(void)
>> +{
>> +	uint64_t **pei_dst = (uint64_t **)((uintptr_t) vm.sblk + 0xc0);
>> +	uint64_t **pei_src = (uint64_t **)((uintptr_t) vm.sblk + 0xc8);
> 
> Hmm, magic values ... according to the public SA22-7095-0 from 1984, the 
> array at offset 0xc0 is called "Interruption parameters" ... could we maybe 
> at least use a similar name in our
> kvm_s390_sie_block?

That's very much a thing that moved over from KVM and the reason for
that is that the interruption parameters contain a lot of things
especially when PV is added into the mix.

We can now either choose to make the interruption parameters an
interesing looking union or we define the offsets properly. I'm also not
too happy about the VSIE PEI code using "u64 *pei_block =
&vsie_page->scb_o->mcic;" for that.

#define SBLK_OFF_PEI_DST 0xc0
#define SBLK_OFF_PEI_SRC 0xc8

> 
>> +	report_prefix_push("pei");
>> +
>> +	report_prefix_push("src");
>> +	memset(dst, 0, PAGE_SIZE);
>> +	protect_page(src, PAGE_ENTRY_I);
>> +	sie(&vm);
>> +	report(vm.sblk->icptcode == ICPT_PARTEXEC, "Partial execution");
>> +	report((uintptr_t)**pei_src == ((uintptr_t)vm.sblk->mso) + 0x6000 + PAGE_ENTRY_I, "PEI_SRC correct");
>> +	report((uintptr_t)**pei_dst == vm.sblk->mso + 0x5000, "PEI_DST correct");
>> +	unprotect_page(src, PAGE_ENTRY_I);
>> +	report(!memcmp(cmp, dst, PAGE_SIZE), "Destination intact");
>> +	/* Jump over the diag44 */

How about:

/*
 * We need to execute the diag44 which is used as a blocker behind the
 * mvpg. It makes sure we fail the tests above if the mvpg wouldn't
* have intercepted.
*/

>> +	sie(&vm);

And:

/* Make sure we intercepted for the diag44 and noting else */

>> +	assert(vm.sblk->icptcode == ICPT_INST &&
>> +	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000);
> 
> Maybe add a comment about the magic values?

Since these will be used extensively in the future I think it might make
sense to introduce constants.

#define SIE_ICPT_IPA_DIAG 0x8300

> 
>> +	report_prefix_pop();
>> +
>> +	/* Clear PEI data for next check */
>> +	report_prefix_push("dst");
>> +	memset((uint64_t *)((uintptr_t) vm.sblk + 0xc0), 0, 16);
>> +	memset(dst, 0, PAGE_SIZE);
>> +	protect_page(dst, PAGE_ENTRY_I);
>> +	sie(&vm);
>> +	report(vm.sblk->icptcode == ICPT_PARTEXEC, "Partial execution");
>> +	report((uintptr_t)**pei_src == vm.sblk->mso + 0x6000, "PEI_SRC correct");
>> +	report((uintptr_t)**pei_dst == vm.sblk->mso + 0x5000 + PAGE_ENTRY_I, "PEI_DST correct");
>> +	/* Needed for the memcmp and general cleanup */
>> +	unprotect_page(dst, PAGE_ENTRY_I);
>> +	report(!memcmp(cmp, dst, PAGE_SIZE), "Destination intact");
>> +	report_prefix_pop();
>> +
>> +	report_prefix_pop();
>> +}
>> +
>> +static void test_mvpg(void)
>> +{
>> +	int binary_size = ((uintptr_t)_binary_s390x_snippets_c_mvpg_snippet_gbin_end -
>> +			   (uintptr_t)_binary_s390x_snippets_c_mvpg_snippet_gbin_start);
>> +
>> +	memcpy(guest, _binary_s390x_snippets_c_mvpg_snippet_gbin_start, binary_size);
>> +	memset(src, 0x42, PAGE_SIZE);
>> +	memset(dst, 0x43, PAGE_SIZE);
>> +	sie(&vm);
>> +	mb();
>> +	report(!memcmp(src, dst, PAGE_SIZE) && *dst == 0x42, "Page moved");
>> +}
>> +
>> +static void setup_guest(void)
>> +{
>> +	setup_vm();
>> +
>> +	/* Allocate 1MB as guest memory */
>> +	guest = alloc_pages(8);
>> +	/* The first two pages are the lowcore */
>> +	guest_instr = guest + PAGE_SIZE * 2;
>> +
>> +	vm.sblk = alloc_page();
>> +
>> +	vm.sblk->cpuflags = CPUSTAT_ZARCH | CPUSTAT_RUNNING;
>> +	vm.sblk->prefix = 0;
>> +	/*
>> +	 * Pageable guest with the same ASCE as the test programm, but
>> +	 * the guest memory 0x0 is offset to start at the allocated
>> +	 * guest pages and end after 1MB.
>> +	 *
>> +	 * It's not pretty but faster and easier than managing guest ASCEs.
>> +	 */
>> +	vm.sblk->mso = (u64)guest;
>> +	vm.sblk->msl = (u64)guest;
>> +	vm.sblk->ihcpu = 0xffff;
>> +
>> +	vm.sblk->crycbd = (uint64_t)alloc_page();
>> +
>> +	vm.sblk->gpsw.addr = PAGE_SIZE * 4;
>> +	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
>> +	vm.sblk->ictl = ICTL_OPEREXC | ICTL_PINT;
>> +	/* Enable MVPG interpretation as we want to test KVM and not ourselves */
>> +	vm.sblk->eca = ECA_MVPGI;
>> +
>> +	src = guest + PAGE_SIZE * 6;
>> +	dst = guest + PAGE_SIZE * 5;
>> +	cmp = alloc_page();
>> +	memset(cmp, 0, PAGE_SIZE);
>> +}
>> +
>> +int main(void)
>> +{
>> +	report_prefix_push("mvpg-sie");
>> +	if (!sclp_facilities.has_sief2) {
>> +		report_skip("SIEF2 facility unavailable");
>> +		goto done;
>> +	}
>> +
>> +	setup_guest();
>> +	test_mvpg();
>> +	test_mvpg_pei();
>> +
>> +done:
>> +	report_prefix_pop();
>> +	return report_summary();
>> +
>> +}
>> diff --git a/s390x/snippets/c/mvpg-snippet.c b/s390x/snippets/c/mvpg-snippet.c
>> new file mode 100644
>> index 0000000..96b70c9
>> --- /dev/null
>> +++ b/s390x/snippets/c/mvpg-snippet.c
>> @@ -0,0 +1,33 @@
>> +#include <libcflat.h>
>> +
>> +static inline void force_exit(void)
>> +{
>> +	asm volatile("	diag	0,0,0x44\n");
>> +}
>> +
>> +static inline int mvpg(unsigned long r0, void *dest, void *src)
>> +{
>> +	register unsigned long reg0 asm ("0") = r0;
>> +	int cc;
>> +
>> +	asm volatile("	mvpg    %1,%2\n"
>> +		     "	ipm     %0\n"
>> +		     "	srl     %0,28"
>> +		     : "=&d" (cc) : "a" (dest), "a" (src), "d" (reg0)
> 
> Why is cc marked with "&" here?

I copied that from s390x/mvpg.c

> 
>   Thomas
>