| Message ID | 20230327082118.2177-5-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add support for running guests without MSO/MSL | expand |
On Mon, 2023-03-27 at 10:21 +0200, 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 | 121 +++++++++++++++++++++++++++++++++++++ > s390x/snippets/c/sie-dat.c | 58 ++++++++++++++++++ > s390x/unittests.cfg | 3 + > 4 files changed, 184 insertions(+) > create mode 100644 s390x/sie-dat.c > create mode 100644 s390x/snippets/c/sie-dat.c > Test looks good to me. Some comments below. [...] > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > new file mode 100644 > index 000000000000..37e46386181c > --- /dev/null > +++ b/s390x/sie-dat.c > @@ -0,0 +1,121 @@ > [...] > + > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > +#define GUEST_TOTAL_PAGE_COUNT 256 This (1M) is the maximum snippet size (see snippet_setup_guest), is this intentional? In that case the comment is inaccurate, since you'd want to sync it with the maximum snippet size. You also know the actual snippet size SNIPPET_LEN(c, sie_dat) so I don't see why you'd need a define at all. > + > +static void test_sie_dat(void) > +{ > [...] > + > + /* the guest will now write to an unmapped address and we check that this causes a segment translation */ I'd prefer "causes a segment translation exception" [...] > diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c > new file mode 100644 > index 000000000000..c9f7af0f3a56 > --- /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"); Pretty sure the compiler will generate a leading tab, so this will be doubly indented. > +} > + > +static inline void force_exit_value(uint64_t val) > +{ > + asm volatile( > + " diag %[val],0,0x9c\n" > + : : [val] "d"(val) > + ); > +} > + > +__attribute__((section(".text"))) int main(void) Why is the attribute necessary? I know all the snippets have it, but I don't see why it's necessary. @Janosch ? > +{ > + 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); Why not just use an address high enough you know it will not be mapped? -1 should do just fine. > + *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 d97eb5e943c8..aab0e670f2d4 100644 > --- a/s390x/unittests.cfg > +++ b/s390x/unittests.cfg > @@ -215,3 +215,6 @@ file = migration-skey.elf > smp = 2 > groups = migration > extra_params = -append '--parallel' > + > +[sie-dat] > +file = sie-dat.elf
On 4/5/23 21:55, Nina Schoetterl-Glausch wrote: > On Mon, 2023-03-27 at 10:21 +0200, 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> >> --- [...] >> +static inline void force_exit_value(uint64_t val) >> +{ >> + asm volatile( >> + " diag %[val],0,0x9c\n" >> + : : [val] "d"(val) >> + ); >> +} >> + >> +__attribute__((section(".text"))) int main(void) > > Why is the attribute necessary? I know all the snippets have it, but I don't see > why it's necessary. > @Janosch ? "Historical growth" :) If it doesn't work without it then we need to find a way to fix it. If it does work without it then we should remove the attribute from all snippets. But this issue is a low priority for me. > >> +{ >> + 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); > > Why not just use an address high enough you know it will not be mapped? > -1 should do just fine. > >> + *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 d97eb5e943c8..aab0e670f2d4 100644 >> --- a/s390x/unittests.cfg >> +++ b/s390x/unittests.cfg >> @@ -215,3 +215,6 @@ file = migration-skey.elf >> smp = 2 >> groups = migration >> extra_params = -append '--parallel' >> + >> +[sie-dat] >> +file = sie-dat.elf >
Quoting Nina Schoetterl-Glausch (2023-04-05 21:55:01) > On Mon, 2023-03-27 at 10:21 +0200, 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 | 121 +++++++++++++++++++++++++++++++++++++ > > s390x/snippets/c/sie-dat.c | 58 ++++++++++++++++++ > > s390x/unittests.cfg | 3 + > > 4 files changed, 184 insertions(+) > > create mode 100644 s390x/sie-dat.c > > create mode 100644 s390x/snippets/c/sie-dat.c > > > > Test looks good to me. Some comments below. > [...] > > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > > new file mode 100644 > > index 000000000000..37e46386181c > > --- /dev/null > > +++ b/s390x/sie-dat.c > > @@ -0,0 +1,121 @@ > > > [...] > > + > > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > > +#define GUEST_TOTAL_PAGE_COUNT 256 > > This (1M) is the maximum snippet size (see snippet_setup_guest), is this intentional? It is by accident the maximum snippet size. It is completely fine to stay at 256 pages when we increase the maximum snippet size. > In that case the comment is inaccurate, since you'd want to sync it with the maximum snippet size. > You also know the actual snippet size SNIPPET_LEN(c, sie_dat) so I don't see why you'd need a define > at all. The snippet size is not the same as the number of mapped pages in the guest, no? [...] > > + > > +static void test_sie_dat(void) > > +{ > > > [...] > > + > > + /* the guest will now write to an unmapped address and we check that this causes a segment translation */ > > I'd prefer "causes a segment translation exception" OK [...] > > diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c > > new file mode 100644 > > index 000000000000..c9f7af0f3a56 > > --- /dev/null > > +++ b/s390x/snippets/c/sie-dat.c [...] > > +static inline void force_exit(void) > > +{ > > + asm volatile(" diag 0,0,0x44\n"); > > Pretty sure the compiler will generate a leading tab, so this will be doubly indented. Copy-paste artifact, I can remove it. [...] > > +{ > > + 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); > > Why not just use an address high enough you know it will not be mapped? > -1 should do just fine. I wanted to make sure exactly the right amount of pages is mapped and no more. -1 would defeat that purpose.
On Thu, 2023-04-13 at 11:43 +0200, Nico Boehr wrote: > Quoting Nina Schoetterl-Glausch (2023-04-05 21:55:01) > > On Mon, 2023-03-27 at 10:21 +0200, 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 | 121 +++++++++++++++++++++++++++++++++++++ > > > s390x/snippets/c/sie-dat.c | 58 ++++++++++++++++++ > > > s390x/unittests.cfg | 3 + > > > 4 files changed, 184 insertions(+) > > > create mode 100644 s390x/sie-dat.c > > > create mode 100644 s390x/snippets/c/sie-dat.c > > > > > > > Test looks good to me. Some comments below. > > [...] > > > > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > > > new file mode 100644 > > > index 000000000000..37e46386181c > > > --- /dev/null > > > +++ b/s390x/sie-dat.c > > > @@ -0,0 +1,121 @@ > > > > > [...] > > > + > > > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */ > > > +#define GUEST_TOTAL_PAGE_COUNT 256 > > > > This (1M) is the maximum snippet size (see snippet_setup_guest), is this intentional? > > It is by accident the maximum snippet size. It is completely fine to stay at 256 pages when we increase the maximum snippet size. > > > In that case the comment is inaccurate, since you'd want to sync it with the maximum snippet size. > > You also know the actual snippet size SNIPPET_LEN(c, sie_dat) so I don't see why you'd need a define > > at all. > > The snippet size is not the same as the number of mapped pages in the guest, no? No, but one probably wouldn't want to map less that the snippet size. And there probably isn't a reason to map more either. [...] > > > > + /* the first unmapped address */ > > > + invalid_ptr = (uint8_t *)(TOTAL_PAGE_COUNT * PAGE_SIZE); > > > > Why not just use an address high enough you know it will not be mapped? > > -1 should do just fine. > > I wanted to make sure exactly the right amount of pages is mapped and no more. > > -1 would defeat that purpose. You want to touch the first page that isn't mapped? Do you also want to map a certain number of pages? I.e fill an entire page table and have the invalid pointer be mapped by another segment table entry? With a small linker script change the snippet could know it's own length. Then you could map just the required number of pages and don't need to keep those numbers in sync.
Quoting Nina Schoetterl-Glausch (2023-04-13 18:33:50) [...] > With a small linker script change the snippet could know it's own length. > Then you could map just the required number of pages and don't need to keep those numbers in sync. Maybe it's because my knowledge about linker scripts is really limited or I don't get it, but I fail to see the advantage of the additional complexity. My assumption would be that the number of pages mapped for the guest memory will never really change. Keeping a define in sync seems more pragmatic than going through linker script magic.
On Fri, 2023-04-14 at 12:10 +0200, Nico Boehr wrote: > Quoting Nina Schoetterl-Glausch (2023-04-13 18:33:50) > [...] > > With a small linker script change the snippet could know it's own length. > > Then you could map just the required number of pages and don't need to keep those numbers in sync. > > Maybe it's because my knowledge about linker scripts is really limited or I > don't get it, but I fail to see the advantage of the additional complexity. > > My assumption would be that the number of pages mapped for the guest memory will > never really change. Keeping a define in sync seems more pragmatic than going > through linker script magic. I think just + . = ALIGN(4K); + esnippet = .; /* End data */ at the bottom of the snippet linker script would suffice. Then you could use esnippet as an extern symbol in the C code. But yeah, might not be worth it unless it would help with other snippets too.
diff --git a/s390x/Makefile b/s390x/Makefile index 97a616111680..e27650d7811b 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf tests += $(TEST_DIR)/panic-loop-pgm.elf tests += $(TEST_DIR)/migration-sck.elf tests += $(TEST_DIR)/exittime.elf +tests += $(TEST_DIR)/sie-dat.elf pv-tests += $(TEST_DIR)/pv-diags.elf @@ -114,6 +115,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..37e46386181c --- /dev/null +++ b/s390x/sie-dat.c @@ -0,0 +1,121 @@ +/* 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/asm-offsets.h> +#include <asm-generic/barrier.h> +#include <asm/pgtable.h> +#include <mmu.h> +#include <asm/page.h> +#include <asm/facility.h> +#include <asm/interrupt.h> +#include <asm/mem.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) +{ + uint8_t r1; + bool contents_match; + uint64_t test_page_gpa, test_page_hpa; + uint8_t *test_page_hva; + + /* 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); + report(vm.sblk->icptcode == ICPT_INST && + (vm.sblk->ipa & 0xFF00) == 0x8300 && vm.sblk->ipb == 0x9c0000, + "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); + report(vm.sblk->icptcode == ICPT_INST && + vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000, + "guest wrote to test buffer"); + + contents_match = true; + for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) { + uint8_t 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 */ + report_prefix_push("guest write to unmapped"); + expect_pgm_int(); + sie(&vm); + check_pgm_int_code(PGM_INT_CODE_SEGMENT_TRANSLATION); + report(sie_had_pgm_int(&vm), "pgm int occured in sie"); + 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..c9f7af0f3a56 --- /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) + ); +} + +__attribute__((section(".text"))) 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 d97eb5e943c8..aab0e670f2d4 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -215,3 +215,6 @@ file = migration-skey.elf smp = 2 groups = migration extra_params = -append '--parallel' + +[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 | 121 +++++++++++++++++++++++++++++++++++++ s390x/snippets/c/sie-dat.c | 58 ++++++++++++++++++ s390x/unittests.cfg | 3 + 4 files changed, 184 insertions(+) create mode 100644 s390x/sie-dat.c create mode 100644 s390x/snippets/c/sie-dat.c