Message ID | 20230712114149.1291580-7-nrb@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | s390x: Add support for running guests without MSO/MSL | expand |
On 12/07/2023 13.41, Nico Boehr wrote: > Since we now have the ability to run guests without MSO/MSL, add a test > to make sure this doesn't break. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > s390x/Makefile | 2 + > s390x/sie-dat.c | 115 +++++++++++++++++++++++++++++++++++++ > s390x/snippets/c/sie-dat.c | 58 +++++++++++++++++++ > s390x/unittests.cfg | 3 + > 4 files changed, 178 insertions(+) > create mode 100644 s390x/sie-dat.c > create mode 100644 s390x/snippets/c/sie-dat.c > > diff --git a/s390x/Makefile b/s390x/Makefile > index a80db538810e..4921669ee4c3 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -40,6 +40,7 @@ tests += $(TEST_DIR)/panic-loop-pgm.elf > tests += $(TEST_DIR)/migration-sck.elf > tests += $(TEST_DIR)/exittime.elf > tests += $(TEST_DIR)/ex.elf > +tests += $(TEST_DIR)/sie-dat.elf > > pv-tests += $(TEST_DIR)/pv-diags.elf > > @@ -120,6 +121,7 @@ snippet_lib = $(snippet_asmlib) lib/auxinfo.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 > +$(TEST_DIR)/sie-dat.elf: snippets = $(SNIPPET_DIR)/c/sie-dat.gbin > $(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin > > $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/snippet-pv-diag-yield.gbin > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > new file mode 100644 > index 000000000000..b326995dfa85 > --- /dev/null > +++ b/s390x/sie-dat.c > @@ -0,0 +1,115 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Tests SIE with paging. > + * > + * Copyright 2023 IBM Corp. > + * > + * Authors: > + * Nico Boehr <nrb@linux.ibm.com> > + */ > +#include <libcflat.h> > +#include <vmalloc.h> > +#include <asm/pgtable.h> > +#include <mmu.h> > +#include <asm/page.h> > +#include <asm/interrupt.h> > +#include <alloc_page.h> > +#include <sclp.h> > +#include <sie.h> > +#include <snippet.h> > + > +static struct vm vm; > +static pgd_t *guest_root; > + > +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > +#define GUEST_TEST_PAGE_COUNT 10 > + > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > +#define GUEST_TOTAL_PAGE_COUNT 256 I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h and include that header here and in the snippet C code. Apart from that, test looks good to me: Reviewed-by: Thomas Huth <thuth@redhat.com>
Quoting Thomas Huth (2023-07-13 10:29:48) [...] > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > > new file mode 100644 > > index 000000000000..b326995dfa85 > > --- /dev/null > > +++ b/s390x/sie-dat.c > > @@ -0,0 +1,115 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Tests SIE with paging. > > + * > > + * Copyright 2023 IBM Corp. > > + * > > + * Authors: > > + * Nico Boehr <nrb@linux.ibm.com> > > + */ > > +#include <libcflat.h> > > +#include <vmalloc.h> > > +#include <asm/pgtable.h> > > +#include <mmu.h> > > +#include <asm/page.h> > > +#include <asm/interrupt.h> > > +#include <alloc_page.h> > > +#include <sclp.h> > > +#include <sie.h> > > +#include <snippet.h> > > + > > +static struct vm vm; > > +static pgd_t *guest_root; > > + > > +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > > +#define GUEST_TEST_PAGE_COUNT 10 > > + > > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > > +#define GUEST_TOTAL_PAGE_COUNT 256 > > I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h > and include that header here and in the snippet C code. I'd have to #include "../s390x/snippets/c/sie-dat.h" and it feels like I shouldn't be doing this, should I? Or move the include to lib, but we agreed we don't want test-specific stuff in the lib.
On 14/07/2023 10.35, Nico Boehr wrote: > Quoting Thomas Huth (2023-07-13 10:29:48) > [...] >>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c >>> new file mode 100644 >>> index 000000000000..b326995dfa85 >>> --- /dev/null >>> +++ b/s390x/sie-dat.c >>> @@ -0,0 +1,115 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>> +/* >>> + * Tests SIE with paging. >>> + * >>> + * Copyright 2023 IBM Corp. >>> + * >>> + * Authors: >>> + * Nico Boehr <nrb@linux.ibm.com> >>> + */ >>> +#include <libcflat.h> >>> +#include <vmalloc.h> >>> +#include <asm/pgtable.h> >>> +#include <mmu.h> >>> +#include <asm/page.h> >>> +#include <asm/interrupt.h> >>> +#include <alloc_page.h> >>> +#include <sclp.h> >>> +#include <sie.h> >>> +#include <snippet.h> >>> + >>> +static struct vm vm; >>> +static pgd_t *guest_root; >>> + >>> +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ >>> +#define GUEST_TEST_PAGE_COUNT 10 >>> + >>> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ >>> +#define GUEST_TOTAL_PAGE_COUNT 256 >> >> I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h >> and include that header here and in the snippet C code. > > I'd have to > > #include "../s390x/snippets/c/sie-dat.h" > > and it feels like I shouldn't be doing this, should I? Why "../s390x/" ? Isn't #include "snippets/c/sie-dat.h" enough? ... that would look reasonable to me. Thomas
Quoting Thomas Huth (2023-07-14 10:40:28) > On 14/07/2023 10.35, Nico Boehr wrote: > > Quoting Thomas Huth (2023-07-13 10:29:48) > > [...] > >>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > >>> new file mode 100644 > >>> index 000000000000..b326995dfa85 > >>> --- /dev/null > >>> +++ b/s390x/sie-dat.c > >>> @@ -0,0 +1,115 @@ > >>> +/* SPDX-License-Identifier: GPL-2.0-only */ > >>> +/* > >>> + * Tests SIE with paging. > >>> + * > >>> + * Copyright 2023 IBM Corp. > >>> + * > >>> + * Authors: > >>> + * Nico Boehr <nrb@linux.ibm.com> > >>> + */ > >>> +#include <libcflat.h> > >>> +#include <vmalloc.h> > >>> +#include <asm/pgtable.h> > >>> +#include <mmu.h> > >>> +#include <asm/page.h> > >>> +#include <asm/interrupt.h> > >>> +#include <alloc_page.h> > >>> +#include <sclp.h> > >>> +#include <sie.h> > >>> +#include <snippet.h> > >>> + > >>> +static struct vm vm; > >>> +static pgd_t *guest_root; > >>> + > >>> +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > >>> +#define GUEST_TEST_PAGE_COUNT 10 > >>> + > >>> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > >>> +#define GUEST_TOTAL_PAGE_COUNT 256 > >> > >> I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h > >> and include that header here and in the snippet C code. > > > > I'd have to > > > > #include "../s390x/snippets/c/sie-dat.h" > > > > and it feels like I shouldn't be doing this, should I? > > Why "../s390x/" ? Isn't #include "snippets/c/sie-dat.h" enough? ... that > would look reasonable to me. No, it isn't at least on my box: s390x/snippets/c/sie-dat.c:15:10: fatal error: snippets/c/sie-dat.h: No such file or directory 15 | #include "snippets/c/sie-dat.h" | ^~~~~~~~~~~~~~~~~~~~~~ compilation terminated.
On 14/07/2023 12.39, Nico Boehr wrote: > Quoting Thomas Huth (2023-07-14 10:40:28) >> On 14/07/2023 10.35, Nico Boehr wrote: >>> Quoting Thomas Huth (2023-07-13 10:29:48) >>> [...] >>>>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c >>>>> new file mode 100644 >>>>> index 000000000000..b326995dfa85 >>>>> --- /dev/null >>>>> +++ b/s390x/sie-dat.c >>>>> @@ -0,0 +1,115 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>>> +/* >>>>> + * Tests SIE with paging. >>>>> + * >>>>> + * Copyright 2023 IBM Corp. >>>>> + * >>>>> + * Authors: >>>>> + * Nico Boehr <nrb@linux.ibm.com> >>>>> + */ >>>>> +#include <libcflat.h> >>>>> +#include <vmalloc.h> >>>>> +#include <asm/pgtable.h> >>>>> +#include <mmu.h> >>>>> +#include <asm/page.h> >>>>> +#include <asm/interrupt.h> >>>>> +#include <alloc_page.h> >>>>> +#include <sclp.h> >>>>> +#include <sie.h> >>>>> +#include <snippet.h> >>>>> + >>>>> +static struct vm vm; >>>>> +static pgd_t *guest_root; >>>>> + >>>>> +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ >>>>> +#define GUEST_TEST_PAGE_COUNT 10 >>>>> + >>>>> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ >>>>> +#define GUEST_TOTAL_PAGE_COUNT 256 >>>> >>>> I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h >>>> and include that header here and in the snippet C code. >>> >>> I'd have to >>> >>> #include "../s390x/snippets/c/sie-dat.h" >>> >>> and it feels like I shouldn't be doing this, should I? >> >> Why "../s390x/" ? Isn't #include "snippets/c/sie-dat.h" enough? ... that >> would look reasonable to me. > > No, it isn't at least on my box: > > s390x/snippets/c/sie-dat.c:15:10: fatal error: snippets/c/sie-dat.h: No such file or directory > 15 | #include "snippets/c/sie-dat.h" > | ^~~~~~~~~~~~~~~~~~~~~~ > compilation terminated. Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ? Thomas
Quoting Thomas Huth (2023-07-14 12:52:59)
[...]
> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ?
Yeah, that would work, but do we want that? I'd assume that it is a
concious decision not to have tests depend on one another.
On 01/08/2023 08.51, Nico Boehr wrote: > Quoting Thomas Huth (2023-07-14 12:52:59) > [...] >> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ? > > Yeah, that would work, but do we want that? I'd assume that it is a > concious decision not to have tests depend on one another. IMHO this would still be OK ... Janosch, Claudio, what's your opinion on this? Thomas
On 8/14/23 16:59, Thomas Huth wrote: > On 01/08/2023 08.51, Nico Boehr wrote: >> Quoting Thomas Huth (2023-07-14 12:52:59) >> [...] >>> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ? >> >> Yeah, that would work, but do we want that? I'd assume that it is a >> concious decision not to have tests depend on one another. > > IMHO this would still be OK ... Janosch, Claudio, what's your opinion on this? And the headers are then ONLY available via snippets/* ? Pardon my question, not enough cycles, too much work.
On 15/08/2023 13.30, Janosch Frank wrote: > On 8/14/23 16:59, Thomas Huth wrote: >> On 01/08/2023 08.51, Nico Boehr wrote: >>> Quoting Thomas Huth (2023-07-14 12:52:59) >>> [...] >>>> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ? >>> >>> Yeah, that would work, but do we want that? I'd assume that it is a >>> concious decision not to have tests depend on one another. >> >> IMHO this would still be OK ... Janosch, Claudio, what's your opinion on >> this? > > And the headers are then ONLY available via snippets/* ? > Pardon my question, not enough cycles, too much work. No, it's about being able to #include "snippets/c/sie-dat.h" from s390x/sie-dat.c, so that guest and host code can share some #defines. Thomas
On 8/15/23 16:07, Thomas Huth wrote: > On 15/08/2023 13.30, Janosch Frank wrote: >> On 8/14/23 16:59, Thomas Huth wrote: >>> On 01/08/2023 08.51, Nico Boehr wrote: >>>> Quoting Thomas Huth (2023-07-14 12:52:59) >>>> [...] >>>>> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ? >>>> >>>> Yeah, that would work, but do we want that? I'd assume that it is a >>>> concious decision not to have tests depend on one another. >>> >>> IMHO this would still be OK ... Janosch, Claudio, what's your opinion on >>> this? >> >> And the headers are then ONLY available via snippets/* ? >> Pardon my question, not enough cycles, too much work. > > No, it's about being able to #include "snippets/c/sie-dat.h" from > s390x/sie-dat.c, so that guest and host code can share some #defines. > > Thomas > I'm fine with that.
diff --git a/s390x/Makefile b/s390x/Makefile index a80db538810e..4921669ee4c3 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -40,6 +40,7 @@ tests += $(TEST_DIR)/panic-loop-pgm.elf tests += $(TEST_DIR)/migration-sck.elf tests += $(TEST_DIR)/exittime.elf tests += $(TEST_DIR)/ex.elf +tests += $(TEST_DIR)/sie-dat.elf pv-tests += $(TEST_DIR)/pv-diags.elf @@ -120,6 +121,7 @@ snippet_lib = $(snippet_asmlib) lib/auxinfo.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 +$(TEST_DIR)/sie-dat.elf: snippets = $(SNIPPET_DIR)/c/sie-dat.gbin $(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/snippet-pv-diag-yield.gbin diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c new file mode 100644 index 000000000000..b326995dfa85 --- /dev/null +++ b/s390x/sie-dat.c @@ -0,0 +1,115 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Tests SIE with paging. + * + * Copyright 2023 IBM Corp. + * + * Authors: + * Nico Boehr <nrb@linux.ibm.com> + */ +#include <libcflat.h> +#include <vmalloc.h> +#include <asm/pgtable.h> +#include <mmu.h> +#include <asm/page.h> +#include <asm/interrupt.h> +#include <alloc_page.h> +#include <sclp.h> +#include <sie.h> +#include <snippet.h> + +static struct vm vm; +static pgd_t *guest_root; + +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ +#define GUEST_TEST_PAGE_COUNT 10 + +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ +#define GUEST_TOTAL_PAGE_COUNT 256 + +static void test_sie_dat(void) +{ + uint64_t test_page_gpa, test_page_hpa; + uint8_t *test_page_hva, expected_val; + bool contents_match; + uint8_t r1; + + /* guest will tell us the guest physical address of the test buffer */ + sie(&vm); + + r1 = (vm.sblk->ipa & 0xf0) >> 4; + test_page_gpa = vm.save_area.guest.grs[r1]; + test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa); + test_page_hva = __va(test_page_hpa); + assert(vm.sblk->icptcode == ICPT_INST && + (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000); + report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva); + + /* guest will now write to the test buffer and we verify the contents */ + sie(&vm); + assert(vm.sblk->icptcode == ICPT_INST && + vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000); + + contents_match = true; + for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) { + expected_val = 42 + i; + if (test_page_hva[i * PAGE_SIZE] != expected_val) { + report_fail("page %u mismatch actual_val=%x expected_val=%x", + i, test_page_hva[i], expected_val); + contents_match = false; + } + } + report(contents_match, "test buffer contents match"); + + /* the guest will now write to an unmapped address and we check that this causes a segment translation exception */ + report_prefix_push("guest write to unmapped"); + expect_pgm_int(); + sie(&vm); + check_pgm_int_code(PGM_INT_CODE_SEGMENT_TRANSLATION); + report_prefix_pop(); +} + +static void setup_guest(void) +{ + extern const char SNIPPET_NAME_START(c, sie_dat)[]; + extern const char SNIPPET_NAME_END(c, sie_dat)[]; + uint64_t guest_max_addr; + + setup_vm(); + snippet_setup_guest(&vm, false); + + /* allocate a region-1 table */ + guest_root = pgd_alloc_one(); + + /* map guest memory 1:1 */ + guest_max_addr = GUEST_TOTAL_PAGE_COUNT * PAGE_SIZE; + for (uint64_t i = 0; i < guest_max_addr; i += PAGE_SIZE) + install_page(guest_root, __pa(vm.guest_mem + i), (void *)i); + + /* set up storage limit supression - leave mso and msl intact they are ignored anyways */ + vm.sblk->cpuflags |= CPUSTAT_SM; + + /* set up the guest asce */ + vm.save_area.guest.asce = __pa(guest_root) | ASCE_DT_REGION1 | REGION_TABLE_LENGTH; + + snippet_init(&vm, SNIPPET_NAME_START(c, sie_dat), + SNIPPET_LEN(c, sie_dat), SNIPPET_UNPACK_OFF); +} + +int main(void) +{ + report_prefix_push("sie-dat"); + if (!sclp_facilities.has_sief2) { + report_skip("SIEF2 facility unavailable"); + goto done; + } + + setup_guest(); + test_sie_dat(); + sie_guest_destroy(&vm); + +done: + report_prefix_pop(); + return report_summary(); + +} diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c new file mode 100644 index 000000000000..0505e5aba62b --- /dev/null +++ b/s390x/snippets/c/sie-dat.c @@ -0,0 +1,58 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Snippet used by the sie-dat.c test to verify paging without MSO/MSL + * + * Copyright (c) 2023 IBM Corp + * + * Authors: + * Nico Boehr <nrb@linux.ibm.com> + */ +#include <stddef.h> +#include <inttypes.h> +#include <string.h> +#include <asm-generic/page.h> + +/* keep in sync with GUEST_TEST_PAGE_COUNT in s390x/sie-dat.c */ +#define TEST_PAGE_COUNT 10 +static uint8_t test_page[TEST_PAGE_COUNT * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE))); + +/* keep in sync with GUEST_TOTAL_PAGE_COUNT in s390x/sie-dat.c */ +#define TOTAL_PAGE_COUNT 256 + +static inline void force_exit(void) +{ + asm volatile("diag 0,0,0x44\n"); +} + +static inline void force_exit_value(uint64_t val) +{ + asm volatile( + "diag %[val],0,0x9c\n" + : : [val] "d"(val) + ); +} + +int main(void) +{ + uint8_t *invalid_ptr; + + memset(test_page, 0, sizeof(test_page)); + /* tell the host the page's physical address (we're running DAT off) */ + force_exit_value((uint64_t)test_page); + + /* write some value to the page so the host can verify it */ + for (size_t i = 0; i < TEST_PAGE_COUNT; i++) + test_page[i * PAGE_SIZE] = 42 + i; + + /* indicate we've written all pages */ + force_exit(); + + /* the first unmapped address */ + invalid_ptr = (uint8_t *)(TOTAL_PAGE_COUNT * PAGE_SIZE); + *invalid_ptr = 42; + + /* indicate we've written the non-allowed page (should never get here) */ + force_exit(); + + return 0; +} diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index b61faf0737c3..24cd27202a08 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -218,3 +218,6 @@ extra_params = -append '--parallel' [execute] file = ex.elf + +[sie-dat] +file = sie-dat.elf
Since we now have the ability to run guests without MSO/MSL, add a test to make sure this doesn't break. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- s390x/Makefile | 2 + s390x/sie-dat.c | 115 +++++++++++++++++++++++++++++++++++++ s390x/snippets/c/sie-dat.c | 58 +++++++++++++++++++ s390x/unittests.cfg | 3 + 4 files changed, 178 insertions(+) create mode 100644 s390x/sie-dat.c create mode 100644 s390x/snippets/c/sie-dat.c