On Thu, 11 Feb 2021 12:35:49 +0100
Thomas Huth <thuth@xxxxxxxxxx> wrote:
> On 09/02/2021 15.38, Claudio Imbrenda wrote:
> > Simple EDAT test.
> >
> > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>
> > + */
> > +#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
>
