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@xxxxxxxxxxxxx> > > > --- > > > 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.
