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?
[...]
> > +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.
[...]
> > +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.
