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?
[...]
> > +
> > +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.
