On Wed, 23 Mar 2022 15:19:33 +0100
Nico Boehr <nrb@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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
