Re: [PATCH 2/2] s390x: add test for SIGP STORE_ADTL_STATUS order

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

On Mon, 2022-03-28 at 13:54 +0200, Janosch Frank wrote:
> > diff --git a/s390x/adtl_status.c b/s390x/adtl_status.c
> > new file mode 100644
> > index 000000000000..7a2bd2b07804
> > --- /dev/null
> > +++ b/s390x/adtl_status.c
[...]
> > +struct mcesa_lc12 {
> > +       uint8_t vector_reg[0x200];            /* 0x000 */
> 
> Hrm we could do:
> __uint128_t vregs[32];
> 
> or:
> uint64_t vregs[16][2];
> 
> or leave it as it is.

No strong preference about the type. uint8_t makes it easy to check the
offsets.

> 
> > +       uint8_t reserved200[0x400 - 0x200];   /* 0x200 */
> > +       struct gs_cb gs_cb;                   /* 0x400 */
> > +       uint8_t reserved420[0x800 - 0x420];   /* 0x420 */
> > +       uint8_t reserved800[0x1000 - 0x800];  /* 0x800 */
> > +};
> 
> Do we have plans to use this struct in the future for other tests?

Maybe at some point if we add checks for machine check handling, but
right now we don't have the infrastructure in kvm-unit-tests to do that
I think.

> 
> > +
> > +static struct mcesa_lc12 adtl_status
> > __attribute__((aligned(4096)));
> > +
> > +#define NUM_VEC_REGISTERS 32
> > +#define VEC_REGISTER_SIZE 16
> 
> I'd shove that into lib/s390x/asm/float.h or create a vector.h as
> #define VEC_REGISTERS_NUM 32
> #define VEC_REGISTERS_SIZE 16
> 
> Most likely vector.h since we can do both int and float with vector
> regs.

OK, will do.

[...]
> > +
> > +static int have_adtl_status(void)
> 
> bool

Changed.

[...]
> > +static void test_store_adtl_status_unavail(void)
> > +{
> > +       uint32_t status = 0;
> > +       int cc;
> > +
> > +       report_prefix_push("store additional status unvailable");
> 
> unavailable

Thanks.

[...]
> > +static void restart_write_vector(void)
> > +{
> > +       uint8_t *vec_reg;
> > +       /* vlm handles at most 16 registers at a time */
> > +       uint8_t *vec_reg_16_31 = &expected_vec_contents[16][0];
> > +       int i;
> > +
> > +       for (i = 0; i < NUM_VEC_REGISTERS; i++) {
> > +               vec_reg = &expected_vec_contents[i][0];
> > +               /* i+1 to avoid zero content */
> > +               memset(vec_reg, i + 1, VEC_REGISTER_SIZE);
> > +       }
> > +
> > +       ctl_set_bit(0, CTL0_VECTOR);
> > +
> > +       asm volatile (
> > +               "       .machine z13\n"
> > +               "       vlm 0,15, %[vec_reg_0_15]\n"
> > +               "       vlm 16,31, %[vec_reg_16_31]\n"
> > +               :
> > +               : [vec_reg_0_15] "Q"(expected_vec_contents),
> > +                 [vec_reg_16_31] "Q"(*vec_reg_16_31)
> > +               : "v0", "v1", "v2", "v3", "v4", "v5", "v6", "v7",
> > "v8", "v9",
> > +                 "v10", "v11", "v12", "v13", "v14", "v15", "v16",
> > "v17", "v18",
> > +                 "v19", "v20", "v21", "v22", "v23", "v24", "v25",
> > "v26", "v27",
> > +                 "v28", "v29", "v30", "v31", "memory"
> 
> We change memory on a load?

To my understanding, this might be neccesary if expected_vec_contents
ends up in a register, but that won't happen, so I can remove it.

> 
> > +       );
> 
> We could also move vlm as a function to vector.h and do two calls.

I think that won't work because that function might clean its float
registers in the epilogue and hence destroy the contents. Except if you
have an idea on how to avoid that?

[...]
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index 1600e714c8b9..2e65106fa140 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -78,6 +78,31 @@ extra_params=-name kvm-unit-test --uuid
> > 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
> >   file = smp.elf
> >   smp = 2
> >   
> > +[adtl_status-kvm]
> 
> Hmmmmm (TM) I don't really want to mix - and _.
> Having spec_ex-sie.c is already bad enough.

Yes, thanks.

> 
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=on,vx=on
> > +
> > +[adtl_status-no-vec-no-gs-kvm]
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = kvm
> > +extra_params = -cpu host,gs=off,vx=off
> > +
> > +[adtl_status-tcg]
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = tcg
> > +# no guarded-storage support in tcg
> > +extra_params = -cpu qemu,vx=on
> > +
> > +[adtl_status-no-vec-no-gs-tcg]
> > +file = adtl_status.elf
> > +smp = 2
> > +accel = tcg
> > +extra_params = -cpu qemu,gs=off,vx=off
> > +
> 
> Are you trying to sort this in any way?
> Normally we put new entries at the EOF.

Oh, this was a leftover from when this was still part of the smp test,
moved to the end now.