Re: [kvm-unit-tests PATCH v1 4/4] s390x: edat test

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Thu, 11 Feb 2021 12:35:49 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 09/02/2021 15.38, Claudio Imbrenda wrote:
> > Simple EDAT test.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >   s390x/Makefile      |   1 +
> >   s390x/edat.c        | 238
> > ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg |
> >  3 + 3 files changed, 242 insertions(+)
> >   create mode 100644 s390x/edat.c
> > 
> > diff --git a/s390x/Makefile b/s390x/Makefile
> > index 08d85c9f..fc885150 100644
> > --- a/s390x/Makefile
> > +++ b/s390x/Makefile
> > @@ -20,6 +20,7 @@ tests += $(TEST_DIR)/sclp.elf
> >   tests += $(TEST_DIR)/css.elf
> >   tests += $(TEST_DIR)/uv-guest.elf
> >   tests += $(TEST_DIR)/sie.elf
> > +tests += $(TEST_DIR)/edat.elf
> >   
> >   tests_binary = $(patsubst %.elf,%.bin,$(tests))
> >   ifneq ($(HOST_KEY_DOCUMENT),)
> > diff --git a/s390x/edat.c b/s390x/edat.c
> > new file mode 100644
> > index 00000000..504a1501
> > --- /dev/null
> > +++ b/s390x/edat.c
> > @@ -0,0 +1,238 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * EDAT test.
> > + *
> > + * Copyright (c) 2021 IBM Corp
> > + *
> > + * Authors:
> > + *	Claudio Imbrenda <imbrenda@linux.ibm.com>
> > + */
> > +#include <libcflat.h>
> > +#include <vmalloc.h>
> > +#include <asm/facility.h>
> > +#include <asm/interrupt.h>
> > +#include <mmu.h>
> > +#include <asm/pgtable.h>
> > +#include <asm-generic/barrier.h>
> > +
> > +#define TEID_ADDR	PAGE_MASK
> > +#define TEID_AI		0x003
> > +#define TEID_M		0x004
> > +#define TEID_A		0x008
> > +#define TEID_FS		0xc00
> > +
> > +#define LC_SIZE	(2 * PAGE_SIZE)
> > +#define VIRT(x)	((void *)((unsigned long)(x) + (unsigned
> > long)mem)) +
> > +static uint8_t prefix_buf[LC_SIZE]
> > __attribute__((aligned(LC_SIZE))); +static unsigned int tmp[1024]
> > __attribute__((aligned(PAGE_SIZE))); +static void *root, *mem, *m;
> > +static struct lowcore *lc;
> > +volatile unsigned int *p;
> > +
> > +/* Expect a program interrupt, and clear the TEID */
> > +static void expect_dat_fault(void)
> > +{
> > +	expect_pgm_int();
> > +	lc->trans_exc_id = 0;
> > +}
> > +
> > +/* Check if a protection exception happened for the given address
> > */ +static bool check_pgm_prot(void *ptr)
> > +{
> > +	unsigned long teid = lc->trans_exc_id;
> > +
> > +	if (lc->pgm_int_code != PGM_INT_CODE_PROTECTION)
> > +		return 0;  
> 
> return false.
> It's a bool return type.

yeah, that looks cleaner, I'll fix it

> > +	if (~teid & TEID_M)  
> 
> I'd maybe rather write this as:
> 
>          if (!(teid & TEID_M))
> 
> ... but it's just a matter of taste.

yes, I actually had it that way in the beginning, but using ~ is
shorter and does not need parentheses

> > +		return 1;  
> 
>                  return true;
> 
> So this is for backward compatiblity with older Z systems that do not
> have the corresponding facility? Should there be a corresponding
> facility check somewhere? Or maybe add at least a comment?

no, it's not for backwards compatibility as far as I know. If I read
the documentation correctly, that bit might be zero under some
circumstances, and here I will just give up instead of checking if the
circumstances were actually correct.

> > +	return (~teid & TEID_A) &&
> > +		((teid & TEID_ADDR) == ((uint64_t)ptr &
> > PAGE_MASK)) &&
> > +		!(teid & TEID_AI);  
> 
> So you're checking for one specific type of protection exception here
> only ... please add an appropriate comment.

more or less, but I'll add a comment to explain what's going on

> > +}
> > +
> > +static void test_dat(void)
> > +{
> > +	report_prefix_push("edat off");
> > +	/* disable EDAT */
> > +	ctl_clear_bit(0, 23);
> > +
> > +	/* Check some basics */
> > +	p[0] = 42;
> > +	report(p[0] == 42, "pte, r/w");
> > +	p[0] = 0;
> > +
> > +	protect_page(m, PAGE_ENTRY_P);
> > +	expect_dat_fault();
> > +	p[0] = 42;
> > +	unprotect_page(m, PAGE_ENTRY_P);
> > +	report(!p[0] && check_pgm_prot(m), "pte, ro");
> > +
> > +	/* The FC bit should be ignored because EDAT is off */
> > +	p[0] = 42;  
> 
> I'd suggest to set p[0] = 0 here...
> 
> > +	protect_dat_entry(m, SEGMENT_ENTRY_FC, 4);  
> 
> ... and change the value to 42 after enabling the protection ...
> otherwise you don't really test the non-working write protection
> here, do you?

but this is not the write protection. here I'm setting the bit for
large pages. so first I write something, then I set the bit, then I
check if I can still read it. if not, it means that the FC bit was not
ignored (i.e. the entry was considered as a large page instead of a
normal segment table entry pointing to a page table)

Write protection for segment entries _should_ work even with EDAT off,
and that is in fact what the next test checks...

> > +	report(p[0] == 42, "pmd, fc=1, r/w");
> > +	unprotect_dat_entry(m, SEGMENT_ENTRY_FC, 4);
> > +	p[0] = 0;
> > +

... this one here:

> > +	/* Segment protection should work even with EDAT off */
> > +	protect_dat_entry(m, SEGMENT_ENTRY_P, 4);
> > +	expect_dat_fault();
> > +	p[0] = 42;
> > +	report(!p[0] && check_pgm_prot(m), "pmd, ro");
> > +	unprotect_dat_entry(m, SEGMENT_ENTRY_P, 4);
> > +
> > +	/* The FC bit should be ignored because EDAT is off*/  
> 
> Set p[0] to 0 again before enabling the protection? Or maybe use a
> different value than 42 below...?

why? we already checked that p[0] == 0, and if p[0] somehow still is
42, we are going to set it to 42 again

> > +	protect_dat_entry(m, REGION3_ENTRY_FC, 3);
> > +	p[0] = 42;

but! we should set it to 42 BEFORE setting the FC bit!
I will fix this

and maybe add a few more comments to explain what's going on

> > +	report(p[0] == 42, "pud, fc=1, r/w");
> > +	unprotect_dat_entry(m, REGION3_ENTRY_FC, 3);
> > +	p[0] = 0;
> > +
> > +	/* Region1/2/3 protection should not work, because EDAT is
> > off */
> > +	protect_dat_entry(m, REGION_ENTRY_P, 3);
> > +	p[0] = 42;
> > +	report(p[0] == 42, "pud, ro");
> > +	unprotect_dat_entry(m, REGION_ENTRY_P, 3);
> > +	p[0] = 0;
> > +
> > +	protect_dat_entry(m, REGION_ENTRY_P, 2);
> > +	p[0] = 42;
> > +	report(p[0] == 42, "p4d, ro");
> > +	unprotect_dat_entry(m, REGION_ENTRY_P, 2);
> > +	p[0] = 0;
> > +
> > +	protect_dat_entry(m, REGION_ENTRY_P, 1);
> > +	p[0] = 42;
> > +	report(p[0] == 42, "pgd, ro");
> > +	unprotect_dat_entry(m, REGION_ENTRY_P, 1);
> > +	p[0] = 0;
> > +
> > +	report_prefix_pop();
> > +}  
> 
>   Thomas
>