| Message ID | 20210209143835.1031617-5-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390: Add support for large pages | expand |
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. > + if (~teid & TEID_M) I'd maybe rather write this as: if (!(teid & TEID_M)) ... but it's just a matter of taste. > + 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? > + 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. > +} > + > +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? > + report(p[0] == 42, "pmd, fc=1, r/w"); > + unprotect_dat_entry(m, SEGMENT_ENTRY_FC, 4); > + p[0] = 0; > + > + /* 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...? > + protect_dat_entry(m, REGION3_ENTRY_FC, 3); > + p[0] = 42; > + 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
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 >
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; + if (~teid & TEID_M) + return 1; + return (~teid & TEID_A) && + ((teid & TEID_ADDR) == ((uint64_t)ptr & PAGE_MASK)) && + !(teid & TEID_AI); +} + +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; + protect_dat_entry(m, SEGMENT_ENTRY_FC, 4); + report(p[0] == 42, "pmd, fc=1, r/w"); + unprotect_dat_entry(m, SEGMENT_ENTRY_FC, 4); + p[0] = 0; + + /* 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*/ + protect_dat_entry(m, REGION3_ENTRY_FC, 3); + p[0] = 42; + 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(); +} + +static void test_edat1(void) +{ + report_prefix_push("edat1"); + /* Enable EDAT */ + ctl_set_bit(0, 23); + p[0] = 0; + + /* Segment protection should work */ + expect_dat_fault(); + protect_dat_entry(m, SEGMENT_ENTRY_P, 4); + p[0] = 42; + report(!p[0] && check_pgm_prot(m), "pmd, ro"); + unprotect_dat_entry(m, SEGMENT_ENTRY_P, 4); + + /* Region1/2/3 protection should work now */ + expect_dat_fault(); + protect_dat_entry(m, REGION_ENTRY_P, 3); + p[0] = 42; + report(!p[0] && check_pgm_prot(m), "pud, ro"); + unprotect_dat_entry(m, REGION_ENTRY_P, 3); + + expect_dat_fault(); + protect_dat_entry(m, REGION_ENTRY_P, 2); + p[0] = 42; + report(!p[0] && check_pgm_prot(m), "p4d, ro"); + unprotect_dat_entry(m, REGION_ENTRY_P, 2); + + expect_dat_fault(); + protect_dat_entry(m, REGION_ENTRY_P, 1); + p[0] = 42; + report(!p[0] && check_pgm_prot(m), "pgd, ro"); + unprotect_dat_entry(m, REGION_ENTRY_P, 1); + + /* Large pages should work */ + p[0] = 42; + install_large_page(root, 0, mem); + report(p[0] == 42, "pmd, large"); + + /* Prefixing should not work with large pages */ + report(!memcmp(0, VIRT(prefix_buf), LC_SIZE) && + !memcmp(prefix_buf, mem, LC_SIZE), + "pmd, large, prefixing"); + + report_prefix_pop(); +} + +static void test_edat2(void) +{ + report_prefix_push("edat2"); + p[0] = 42; + + /* Huge pages should work */ + install_huge_page(root, 0, mem); + report(p[0] == 42, "pud, huge"); + + /* Prefixing should not work with huge pages */ + report(!memcmp(0, VIRT(prefix_buf), LC_SIZE) && + !memcmp(prefix_buf, mem, LC_SIZE), + "pmd, large, prefixing"); + + report_prefix_pop(); +} + +static unsigned int setup(void) +{ + bool has_edat1 = test_facility(8); + bool has_edat2 = test_facility(78); + unsigned long pa, va; + + if (has_edat2 && !has_edat1) + report_abort("EDAT2 available, but EDAT1 not available"); + + /* Setup DAT 1:1 mapping and memory management */ + setup_vm(); + root = (void *)(stctg(1) & PAGE_MASK); + + /* + * Get a pgd worth of virtual memory, so we can test things later + * without interfering with the test code or the interrupt handler + */ + mem = alloc_vpages_aligned(BIT_ULL(41), 41); + assert(mem); + va = (unsigned long)mem; + + /* Map the first 1GB of real memory */ + for (pa = 0; pa < SZ_1G; pa += PAGE_SIZE, va += PAGE_SIZE) + install_page(root, pa, (void *)va); + + /* Move the lowcore to a known non-zero location */ + assert((unsigned long)&prefix_buf < SZ_2G); + memcpy(prefix_buf, 0, LC_SIZE); + set_prefix((uint32_t)(intptr_t)prefix_buf); + /* Clear the old copy */ + memset(prefix_buf, 0, LC_SIZE); + + /* m will point to tmp through the new virtual mapping */ + m = VIRT(&tmp); + /* p is the same as m but volatile */ + p = (volatile unsigned int *)m; + + return has_edat1 + has_edat2; +} + +int main(void) +{ + unsigned int edat; + + report_prefix_push("edat"); + edat = setup(); + + test_dat(); + + if (edat) + test_edat1(); + else + report_skip("EDAT not available"); + + if (edat >= 2) + test_edat2(); + else + report_skip("EDAT2 not available"); + + report_prefix_pop(); + return report_summary(); +} diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index 2298be6c..8da4a0a3 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -99,3 +99,6 @@ file = uv-guest.elf [sie] file = sie.elf + +[edat] +file = edat.elf
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