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

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

On 3/28/22 17:12, Nico Boehr wrote:
> 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.

Alright, then let's leave the struct in here for now.

[...]

>>> +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?

About that:

Well, who guarantees you that the compiler won't change a fpr (and 
thereby the overlapped vrs) between the vlms here and your infinite loop 
at the end of the function? :-) gcc uses fprs and acrs in the most 
interesting places and I've just been hit by that again a few hours ago.

I.e. to be safe we'll need to implement the next few lines in assembly 
as well, no?