Re: [kvm-unit-tests PATCH v3 7/7] s390x: Add test for STFLE interpretive execution (format-0)

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Tue, 2024-08-27 at 16:08 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2024-06-20 16:17:00)
> [...]
> > diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
> > index a66fe56a..2bad05c5 100644
> > --- a/lib/s390x/asm/facility.h
> > +++ b/lib/s390x/asm/facility.h
> > @@ -27,12 +27,20 @@ static inline void stfl(void)
> >         asm volatile("  stfl    0(0)\n" : : : "memory");
> >  }
> >  
> > -static inline void stfle(uint64_t *fac, unsigned int nb_doublewords)
> > +static inline unsigned int stfle(uint64_t *fac, unsigned int nb_doublewords)
> 
> Why unsigned int?

The return value is 1-256, the size of the type is a bit arbitrary I suppose.

> 
> [...]
> > diff --git a/s390x/snippets/c/stfle.c b/s390x/snippets/c/stfle.c
> > new file mode 100644
> > index 00000000..eb024a6a
> > --- /dev/null
> > +++ b/s390x/snippets/c/stfle.c
> [...]
> > +int main(void)
> > +{
> > +       const unsigned int max_fac_len = 8;
> > +       uint64_t res[max_fac_len + 1];
> > +
> > +       res[0] = max_fac_len - 1;
> > +       asm volatile ( "lg      0,%[len]\n"
> > +               "       stfle   %[fac]\n"
> > +               "       stg     0,%[len]\n"
> > +               : [fac] "=QS"(*(uint64_t(*)[max_fac_len])&res[1]),
> 
> Out of curiosity:
> 
> Q = Memory reference without index register and with short displacement
> S = Memory reference without index register but with long displacement
> 
> Which one is it?

Ups, just short displacement actually.

> 
> And: is long displacement even appropriate here?
> 
> The cast also is hard to understand. Since this is not super high
> performance code, do we just want to clobber memory so this gets a bit
> easier to understand?
> 
> > +                 [len] "+RT"(res[0])
> 
> Same question about RT as above.

Long, but providing a short displacement should be fine too.
Not sure if there is any benefit to letting the compiler choose.

> 
> [...]
> > diff --git a/s390x/stfle-sie.c b/s390x/stfle-sie.c
> > new file mode 100644
> > index 00000000..a3e7f1c9
> > --- /dev/null
> > +++ b/s390x/stfle-sie.c
> [...]
> > +static struct guest_stfle_res run_guest(void)
> > +{
> > +       struct guest_stfle_res res;
> > +       uint64_t guest_stfle_addr;
> > +
> > +       sie(&vm);
> > +       assert(snippet_is_force_exit_value(&vm));
> > +       guest_stfle_addr = snippet_get_force_exit_value(&vm);
> > +       res.mem = &vm.guest_mem[guest_stfle_addr];
> > +       memcpy(&res.reg, res.mem, sizeof(res.reg));
> > +       res.len = (res.reg & 0xff) + 1;
> 
> If I'm not mistaken, you subtracted 1 in the guest. Here you add it again.
> Is there a particular reason why?

No, it's the direct result of STFLE on register 0.