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

[Janosch Frank <frankja@linux.ibm.com>]

On 7/10/23 16:29, Nico Boehr wrote:
> Quoting Janosch Frank (2023-06-05 11:57:58)
> [...]
>>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
>>> new file mode 100644
>>> index 000000000000..c490a2aa825c
> [...]
>>> +#include <libcflat.h>
>>> +#include <vmalloc.h>
>>> +#include <asm/asm-offsets.h>
>>
>> I only did a cursory glance and wasn't able to see a use for this include.
> 
> Yep, thanks, I cleaned up the includes a bit.
> 
> [...]
>>> +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);
>>
>> You could rebase on my pv_icptdata.h patch.
>> Also the report string and boolean don't really relate to each other.
> 
> Which patch are we talking about? pv_icptdata_check_diag()?
> 
> Note that this is not a PV test, so I guess it's not applicable here?

Ah right, we could extend that but for one use this should be fine.
Let's see if there'll be more SIE tests that need this before building a 
a full intercept check lib.

Could you lower-case the 0xFF00 when you re-spin?

> 
>> Not every exit needs to be a report.
>> Some should rather be asserts() or report_info()s.
> 
> Yeah, I have made report()s where it doesn't make sense to continue assert()s
> 
>>> +     contents_match = true;
>>> +     for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
>>> +             uint8_t expected_val = 42 + i;
>>
>> Just because you can doesn't mean that you have to.
>> At least leave a \n when declaring new variables...
> 
> I am a bit confused but I *guess* you wanted me to move the declaration of
> expected_val to the beginning of the function?
> 

Personally I'm not a big fan of declaring variables in the lower 
function body, they are way too easy to overlook.

I dimly remember there being a rule but when I used a few minutes to 
look for it I couldn't find it anymore. Hmmmm, maybe I'm getting old.

> [...]
>>> diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
>>> new file mode 100644
>>> index 000000000000..e156d0c36c4c
>>> --- /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)
>>> +     );
>>> +}
>>
>> It feels like these need to go into a snippet lib.
> 
> A bunch of other tests do similar things, so I'll write a TODO and tackle it in
> a seperate series.

Thanks :)

> 
> [...]
>>> +
>>> +__attribute__((section(".text"))) int main(void)
>>
>> The attribute shouldn't be needed anymore.
> 
> OK, removed.
> 
> [...]
>>> +{
>>> +     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++)
>>
>> Why is i a size_t type?
> 
> Because it's a suitable unsigned type for use as an array index.
> 
> What should it be instead?

I would have used a standard uint type but to be fair this doesn't kill 
me either.