Re: [kvm-unit-tests PATCH v1 4/4] s390x: add a test for SIE without MSO/MSL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux