Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Nico Boehr <nrb@linux.ibm.com>]

On Fri, 2022-05-13 at 13:04 +0200, Janis Schoetterl-Glausch wrote:
[...]
> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > new file mode 100644
> > index 000000000000..6f3053d8ab40
> > --- /dev/null
> > +++ b/s390x/migration-skey.c
[...]
> > +static void test_migration(void)
> > +{
> > +       int i, key_to_set;
> > +       uint8_t *page;
> > +       union skey expected_key, actual_key, mismatching_key;
> 
> I would tend to scope those to the bodies of the respective loop,
> but I don't know if that's in accordance with the coding style.

Seems to me the more common thing is to declare variables outside. But sure can change that, what do the maintainers say?

> > +
> > +       for (i = 0; i < NUM_PAGES; i++) {
> > +               /*
> > +                * Storage keys are 7 bit, lowest bit is always
> > returned as zero
> > +                * by iske
> > +                */
> > +               key_to_set = i * 2;
> > +               set_storage_key(pagebuf + i, key_to_set, 1);
> 
> Why not just pagebuf[i]?

Works as well and looks nicer, changed, thanks.

[...]
> > +       for (i = 0; i < NUM_PAGES; i++) {
[...]
> > +               expect_pgm_int();
> > +               asm volatile (
> > +                       /* set access key */
> > +                       "spka 0(%[mismatching_key])\n"
> > +                       /* try to write page */
> > +                       "mvi 0(%[page]), 42\n"
> > +                       /* reset access key */
> > +                       "spka 0\n"
> > +                       :
> > +                       : [mismatching_key]
> > "a"(mismatching_key.val),
> > +                         [page] "a"(page)
> > +                       : "memory"
> > +               );
> > +               check_pgm_int_code_xfail(host_is_tcg(),
> > PGM_INT_CODE_PROTECTION);
> > +               report_xfail(host_is_tcg(), *page == 0xff, "no
> > store occured");
> 
> What are you testing with this bit? If storage keys are really
> effective after the migration?

Yes.

> I'm wondering if using tprot would not be better, it should simplify
> the code a lot.

Hmm, good point. If I am not mistaken, tprot is intercepted, am I? Then it might make sense to actually do both, won't it?

> Plus you'd easily test for fetch protection, too.
> > +
> > +               report_prefix_pop();
> > +       }
> > +}
> > +
> > +int main(void)
> > +{
> > +       report_prefix_push("migration-skey");
> > +       if (test_facility(169)) {
> > +               report_skip("storage key removal facility is
> > active");
> > +
> > +               /*
> > +                * If we just exit and don't ask migrate_cmd to
> > migrate us, it
> > +                * will just hang forever. Hence, also ask for
> > migration when we
> > +                * skip this test alltogether.
> 
> s/alltogether/altogether/

Thanks fixed.