Re: [kvm-unit-tests PATCH v1 4/9] s390x: smp: add test for SIGP_STORE_ADTL_STATUS order

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Wed, 23 Mar 2022 15:19:33 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> On Mon, 2022-03-21 at 15:59 +0100, Claudio Imbrenda wrote:
> > On Mon, 21 Mar 2022 11:18:59 +0100
> > Nico Boehr <nrb@linux.ibm.com> wrote:  
> > > diff --git a/s390x/smp.c b/s390x/smp.c
> > > index e5a16eb5a46a..5d3265f6be64 100644
> > > --- a/s390x/smp.c  
> [...]
> > > +/*
> > > + * We keep two structs, one for comparing when we want to assert
> > > it's not
> > > + * touched.
> > > + */
> > > +static uint8_t adtl_status[2][4096]
> > > __attribute__((aligned(4096)));  
> > 
> > it's a little bit ugly. maybe define a struct, with small buffers
> > inside
> > for the vector and gs areas? that way we would not need ugly magic
> > numbers below (see below)  
> 
> OK, will do.
> 
> [...]
> > > +static void restart_write_vector(void)
> > > +{
> > > +       uint8_t *vec_reg;
> > > +       uint8_t *vec_reg_16_31 = &expected_vec_contents[16][0];  
> > 
> > add a comment to explain that vlm only handles at most 16 registers
> > at
> > a time  
> 
> OK will do.
> 
> > > +       int i;
> > > +
> > > +       for (i = 0; i < NUM_VEC_REGISTERS; i++) {
> > > +               vec_reg = &expected_vec_contents[i][0];
> > > +               memset(vec_reg, i, VEC_REGISTER_SIZE);
> > > +       }  
> > 
> > this way vector register 0 stays 0.
> > either special case it (e.g. 16, or whatever), or put a magic value
> > somewhere in every register  
> 
> adtl_status is initalized with 0xff. Are you OK with i + 1 so we avoid
> zero?

that is fine

> 
> [...]
> > > +static void test_store_adtl_status_vector(void)
> > > +{
> > > +       uint32_t status = -1;
> > > +       struct psw psw;
> > > +       int cc;
> > > +
> > > +       report_prefix_push("store additional status vector");
> > > +
> > > +       if (!test_facility(129)) {
> > > +               report_skip("vector facility not installed");
> > > +               goto out;
> > > +       }
> > > +
> > > +       cpu_write_magic_to_vector_regs(1);
> > > +       smp_cpu_stop(1);
> > > +
> > > +       memset(adtl_status, 0xff, sizeof(adtl_status));
> > > +
> > > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > > +                 (unsigned long)adtl_status, &status);
> > > +       report(!cc, "CC = 0");
> > > +
> > > +       report(!memcmp(adtl_status, expected_vec_contents,
> > > sizeof(expected_vec_contents)),
> > > +              "additional status contents match");  
> > 
> > it would be interesting to check that nothing is stored past the end
> > of
> > the buffer.  
> 
> I will add checks to ensure reserved fields are not modified.
> 
> > moreover, I think you should also explicitly test with lc_10, to make
> > sure that works as well (no need to rerun the guest, just add another
> > sigp call)  
> 
> I will test vector with LC 0, 10, 11, 12 and guarded storage with LC 11
> and 12.
> 
> [...]
> > > +static void restart_write_gs_regs(void)
> > > +{
> > > +       const unsigned long gs_area = 0x2000000;
> > > +       const unsigned long gsc = 25; /* align = 32 M, section size
> > > = 512K */
> > > +
> > > +       ctl_set_bit(2, CTL2_GUARDED_STORAGE);
> > > +
> > > +       gs_cb.gsd = gs_area | gsc;
> > > +       gs_cb.gssm = 0xfeedc0ffe;
> > > +       gs_cb.gs_epl_a = (uint64_t) &gs_epl;
> > > +
> > > +       load_gs_cb(&gs_cb);
> > > +
> > > +       set_flag(1);
> > > +
> > > +       ctl_clear_bit(2, CTL2_GUARDED_STORAGE);  
> > 
> > what happens when the function returns? is r14 set up properly? (or
> > maybe we just don't care, since we are going to stop the CPU anyway?)  
> 
> We have an endless loop in smp_cpu_setup_state. So r14 will point there
> (verified with gdb).
> 
> In the end, I think we don't care. This is in contrast to the vector
> test, where the epilogue will clean up the floating point regs.

then add a comment explaining that :)

> 
> [...]
> > > +static void test_store_adtl_status_gs(void)
> > > +{
> > > +       const unsigned long adtl_status_lc_11 = 11;
> > > +       uint32_t status = 0;
> > > +       int cc;
> > > +
> > > +       report_prefix_push("store additional status guarded-
> > > storage");
> > > +
> > > +       if (!test_facility(133)) {
> > > +               report_skip("guarded-storage facility not
> > > installed");
> > > +               goto out;
> > > +       }
> > > +
> > > +       cpu_write_to_gs_regs(1);
> > > +       smp_cpu_stop(1);
> > > +
> > > +       memset(adtl_status, 0xff, sizeof(adtl_status));
> > > +
> > > +       cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> > > +                 (unsigned long)adtl_status | adtl_status_lc_11,
> > > &status);
> > > +       report(!cc, "CC = 0");
> > > +
> > > +       report(!memcmp(&adtl_status[0][1024], &gs_cb,
> > > sizeof(gs_cb)),  
> > 
> > e.g. the 1024 is one of those "magic number" I mentioned above   
> 
> OK, fixed.
> 
> >   
> > > +              "additional status contents match");  
> > 
> > it would be interesting to test that nothing is stored after the end
> > of
> > the buffer (i.e. everything is still 0xff in the second half of the
> > page)  
> 
> Yes, done.
> 
> [...]
> > > 
> > > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > > index 1600e714c8b9..2d0adc503917 100644
> > > --- a/s390x/unittests.cfg
> > > +++ b/s390x/unittests.cfg
> > > @@ -77,6 +77,12 @@ extra_params=-name kvm-unit-test --uuid
> > > 0fb84a86-727c-11ea-bc55-0242ac130003 -sm
> > >  [smp]
> > >  file = smp.elf
> > >  smp = 2
> > > +extra_params = -cpu host,gs=on,vx=on
> > > +
> > > +[smp-no-vec-no-gs]
> > > +file = smp.elf
> > > +smp = 2
> > > +extra_params = -cpu host,gs=off,vx=off  
> > 
> > using "host" will break TCG
> > (and using "qemu" will break secure execution)
> > 
> > there are two possible solutions:
> > 
> > use "max" and deal with the warnings, or split each testcase in two,
> > one using host cpu and "accel = kvm" and the other with "accel = tcg"
> > and qemu cpu.  
> 
> Uh, thanks for pointing out. I will split in accel = tcg and accel =
> kvm.
> 
> > what should happen if only one of the two features is installed?
> > should
> > the buffer for the unavailable feature be stored with 0 or should it
> > be
> > left untouched? is it worth testing those scenarios?  
> 
> The PoP specifies: "A facility’s registers are only
> stored in the MCESA when the facility is installed."
> 
> Maybe I miss something, but it doesn't seem worth it. It would mean
> adding yet another instance to the unittests.cfg. Since once needs to
> provide the memory for the registers even when the facility isn't
> there, there seems little risk for breaking something when we store if
> the facility isn't there.

I mean, technically we should check that nothing is stored for
facilities that are not present, but I guess it's not worth it 
and that would indeed double the number of entries in unittests.cfg