diff mbox series

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

Message ID 20220321101904.387640-5-nrb@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: Further extend instruction interception tests | expand

Commit Message

Nico Boehr March 21, 2022, 10:18 a.m. UTC
Add a test for SIGP_STORE_ADDITIONAL_STATUS order.

There are several cases to cover:
- when neither vector nor guarded-storage facility is available, check
  the order is rejected.
- when one of the facilities is there, test the order is rejected and
  adtl_status is not touched when the target CPU is running or when an
  invalid CPU address is specified. Also check the order is rejected
  in case of invalid alignment.
- when the vector facility is there, write some data to the CPU's
  vector registers and check we get the right contents.
- when the guarded-storage facility is there, populate the CPU's
  guarded-storage registers with some data and again check we get the
  right contents.

To make sure we cover all these cases, adjust unittests.cfg to run the
smp tests with both guarded-storage and vector facility off and on.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/smp.c         | 259 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   6 +
 2 files changed, 265 insertions(+)

Comments

Claudio Imbrenda March 21, 2022, 2:59 p.m. UTC | #1
On Mon, 21 Mar 2022 11:18:59 +0100
Nico Boehr <nrb@linux.ibm.com> wrote:

> Add a test for SIGP_STORE_ADDITIONAL_STATUS order.
> 
> There are several cases to cover:
> - when neither vector nor guarded-storage facility is available, check
>   the order is rejected.
> - when one of the facilities is there, test the order is rejected and
>   adtl_status is not touched when the target CPU is running or when an
>   invalid CPU address is specified. Also check the order is rejected
>   in case of invalid alignment.
> - when the vector facility is there, write some data to the CPU's
>   vector registers and check we get the right contents.
> - when the guarded-storage facility is there, populate the CPU's
>   guarded-storage registers with some data and again check we get the
>   right contents.
> 
> To make sure we cover all these cases, adjust unittests.cfg to run the
> smp tests with both guarded-storage and vector facility off and on.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/smp.c         | 259 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   6 +
>  2 files changed, 265 insertions(+)
> 
> diff --git a/s390x/smp.c b/s390x/smp.c
> index e5a16eb5a46a..5d3265f6be64 100644
> --- a/s390x/smp.c
> +++ b/s390x/smp.c
> @@ -16,6 +16,7 @@
>  #include <asm/sigp.h>
>  
>  #include <smp.h>
> +#include <gs.h>
>  #include <alloc_page.h>
>  
>  static int testflag = 0;
> @@ -37,6 +38,19 @@ static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
>  	{ INVALID_ORDER_CODE,         "invalid order code" },
>  };
>  
> +/*
> + * 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)

> +
> +#define NUM_VEC_REGISTERS 32
> +#define VEC_REGISTER_SIZE 16
> +static uint8_t expected_vec_contents[NUM_VEC_REGISTERS][VEC_REGISTER_SIZE];
> +
> +static struct gs_cb gs_cb;
> +static struct gs_epl gs_epl;
> +
>  static void test_invalid(void)
>  {
>  	const struct sigp_invalid_cases *c;
> @@ -200,6 +214,247 @@ static void test_store_status(void)
>  	report_prefix_pop();
>  }
>  
> +static int have_adtl_status(void)
> +{
> +	return test_facility(133) || test_facility(129);
> +}
> +
> +static void test_store_adtl_status(void)
> +{
> +	uint32_t status = -1;
> +	int cc;
> +
> +	report_prefix_push("store additional status");
> +
> +	if (!have_adtl_status()) {
> +		report_skip("no guarded-storage or vector facility installed");
> +		goto out;
> +	}
> +
> +	memset(adtl_status, 0xff, sizeof(adtl_status));
> +
> +	report_prefix_push("running");
> +	smp_cpu_restart(1);
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status, &status);
> +
> +	report(cc == 1, "CC = 1");
> +	report(status == SIGP_STATUS_INCORRECT_STATE, "status = INCORRECT_STATE");
> +	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
> +	       "additional status not touched");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("invalid CPU address");
> +
> +	cc = sigp(INVALID_CPU_ADDRESS, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status, &status);
> +	report(cc == 3, "CC = 3");
> +	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
> +	       "additional status not touched");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("unaligned");
> +	smp_cpu_stop(1);
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status + 256, &status);
> +	report(cc == 1, "CC = 1");
> +	report(status == SIGP_STATUS_INVALID_PARAMETER, "status = INVALID_PARAMETER");
> +
> +	report_prefix_pop();
> +
> +out:
> +	report_prefix_pop();
> +}
> +
> +static void test_store_adtl_status_unavail(void)
> +{
> +	uint32_t status = 0;
> +	int cc;
> +
> +	report_prefix_push("store additional status unvailable");
> +
> +	if (have_adtl_status()) {
> +		report_skip("guarded-storage or vector facility installed");
> +		goto out;
> +	}
> +
> +	report_prefix_push("not accepted");
> +	smp_cpu_stop(1);
> +
> +	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
> +		  (unsigned long)adtl_status, &status);
> +
> +	report(cc == 1, "CC = 1");
> +	report(status == SIGP_STATUS_INVALID_ORDER,
> +	       "status = INVALID_ORDER");
> +
> +	report_prefix_pop();
> +
> +out:
> +	report_prefix_pop();
> +}
> +
> +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

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

> +
> +	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"
> +	);
> +
> +	ctl_clear_bit(0, CTL0_VECTOR);
> +
> +	set_flag(1);
> +
> +	/*
> +	 * function epilogue will restore floating point registers and hence
> +	 * destroy vector register contents
> +	 */
> +	while (1)
> +		;
> +}
> +
> +static void cpu_write_magic_to_vector_regs(uint16_t cpu_idx)
> +{
> +	struct psw new_psw;
> +
> +	smp_cpu_stop(cpu_idx);
> +
> +	new_psw.mask = extract_psw_mask();
> +	new_psw.addr = (unsigned long)restart_write_vector;
> +
> +	set_flag(0);
> +
> +	smp_cpu_start(cpu_idx, new_psw);
> +
> +	wait_for_flag();
> +}
> +
> +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.

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)

> +
> +	/*
> +	 * To avoid the floating point/vector registers being cleaned up, we
> +	 * stopped CPU1 right in the middle of a function. Hence the cleanup of
> +	 * the function didn't run yet and the stackpointer is messed up.
> +	 * Destroy and re-initalize the CPU to fix that.
> +	 */
> +	smp_cpu_destroy(1);
> +	psw.mask = extract_psw_mask();
> +	psw.addr = (unsigned long)test_func;
> +	smp_cpu_setup(1, psw);
> +
> +out:
> +	report_prefix_pop();
> +}
> +
> +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?)

> +}
> +
> +static void cpu_write_to_gs_regs(uint16_t cpu_idx)
> +{
> +	struct psw new_psw;
> +
> +	smp_cpu_stop(cpu_idx);
> +
> +	new_psw.mask = extract_psw_mask();
> +	new_psw.addr = (unsigned long)restart_write_gs_regs;
> +
> +	set_flag(0);
> +
> +	smp_cpu_start(cpu_idx, new_psw);
> +
> +	wait_for_flag();
> +}
> +
> +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 

> +	       "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)

> +
> +out:
> +	report_prefix_pop();
> +}
> +
>  static void ecall(void)
>  {
>  	unsigned long mask;
> @@ -388,6 +643,10 @@ int main(void)
>  	test_stop();
>  	test_stop_store_status();
>  	test_store_status();
> +	test_store_adtl_status_unavail();
> +	test_store_adtl_status_vector();
> +	test_store_adtl_status_gs();
> +	test_store_adtl_status();
>  	test_ecall();
>  	test_emcall();
>  	test_sense_running();
> 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.

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?

>  
>  [sclp-1g]
>  file = sclp.elf
Nico Boehr March 23, 2022, 2:19 p.m. UTC | #2
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?

[...]
> > +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.
Claudio Imbrenda March 23, 2022, 3:47 p.m. UTC | #3
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
diff mbox series

Patch

diff --git a/s390x/smp.c b/s390x/smp.c
index e5a16eb5a46a..5d3265f6be64 100644
--- a/s390x/smp.c
+++ b/s390x/smp.c
@@ -16,6 +16,7 @@ 
 #include <asm/sigp.h>
 
 #include <smp.h>
+#include <gs.h>
 #include <alloc_page.h>
 
 static int testflag = 0;
@@ -37,6 +38,19 @@  static const struct sigp_invalid_cases cases_valid_cpu_addr[] = {
 	{ INVALID_ORDER_CODE,         "invalid order code" },
 };
 
+/*
+ * 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)));
+
+#define NUM_VEC_REGISTERS 32
+#define VEC_REGISTER_SIZE 16
+static uint8_t expected_vec_contents[NUM_VEC_REGISTERS][VEC_REGISTER_SIZE];
+
+static struct gs_cb gs_cb;
+static struct gs_epl gs_epl;
+
 static void test_invalid(void)
 {
 	const struct sigp_invalid_cases *c;
@@ -200,6 +214,247 @@  static void test_store_status(void)
 	report_prefix_pop();
 }
 
+static int have_adtl_status(void)
+{
+	return test_facility(133) || test_facility(129);
+}
+
+static void test_store_adtl_status(void)
+{
+	uint32_t status = -1;
+	int cc;
+
+	report_prefix_push("store additional status");
+
+	if (!have_adtl_status()) {
+		report_skip("no guarded-storage or vector facility installed");
+		goto out;
+	}
+
+	memset(adtl_status, 0xff, sizeof(adtl_status));
+
+	report_prefix_push("running");
+	smp_cpu_restart(1);
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status, &status);
+
+	report(cc == 1, "CC = 1");
+	report(status == SIGP_STATUS_INCORRECT_STATE, "status = INCORRECT_STATE");
+	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
+	       "additional status not touched");
+
+	report_prefix_pop();
+
+	report_prefix_push("invalid CPU address");
+
+	cc = sigp(INVALID_CPU_ADDRESS, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status, &status);
+	report(cc == 3, "CC = 3");
+	report(!memcmp(adtl_status[0], adtl_status[1], sizeof(adtl_status[0])),
+	       "additional status not touched");
+
+	report_prefix_pop();
+
+	report_prefix_push("unaligned");
+	smp_cpu_stop(1);
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status + 256, &status);
+	report(cc == 1, "CC = 1");
+	report(status == SIGP_STATUS_INVALID_PARAMETER, "status = INVALID_PARAMETER");
+
+	report_prefix_pop();
+
+out:
+	report_prefix_pop();
+}
+
+static void test_store_adtl_status_unavail(void)
+{
+	uint32_t status = 0;
+	int cc;
+
+	report_prefix_push("store additional status unvailable");
+
+	if (have_adtl_status()) {
+		report_skip("guarded-storage or vector facility installed");
+		goto out;
+	}
+
+	report_prefix_push("not accepted");
+	smp_cpu_stop(1);
+
+	cc = smp_sigp(1, SIGP_STORE_ADDITIONAL_STATUS,
+		  (unsigned long)adtl_status, &status);
+
+	report(cc == 1, "CC = 1");
+	report(status == SIGP_STATUS_INVALID_ORDER,
+	       "status = INVALID_ORDER");
+
+	report_prefix_pop();
+
+out:
+	report_prefix_pop();
+}
+
+static void restart_write_vector(void)
+{
+	uint8_t *vec_reg;
+	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];
+		memset(vec_reg, i, 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"
+	);
+
+	ctl_clear_bit(0, CTL0_VECTOR);
+
+	set_flag(1);
+
+	/*
+	 * function epilogue will restore floating point registers and hence
+	 * destroy vector register contents
+	 */
+	while (1)
+		;
+}
+
+static void cpu_write_magic_to_vector_regs(uint16_t cpu_idx)
+{
+	struct psw new_psw;
+
+	smp_cpu_stop(cpu_idx);
+
+	new_psw.mask = extract_psw_mask();
+	new_psw.addr = (unsigned long)restart_write_vector;
+
+	set_flag(0);
+
+	smp_cpu_start(cpu_idx, new_psw);
+
+	wait_for_flag();
+}
+
+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");
+
+	/*
+	 * To avoid the floating point/vector registers being cleaned up, we
+	 * stopped CPU1 right in the middle of a function. Hence the cleanup of
+	 * the function didn't run yet and the stackpointer is messed up.
+	 * Destroy and re-initalize the CPU to fix that.
+	 */
+	smp_cpu_destroy(1);
+	psw.mask = extract_psw_mask();
+	psw.addr = (unsigned long)test_func;
+	smp_cpu_setup(1, psw);
+
+out:
+	report_prefix_pop();
+}
+
+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);
+}
+
+static void cpu_write_to_gs_regs(uint16_t cpu_idx)
+{
+	struct psw new_psw;
+
+	smp_cpu_stop(cpu_idx);
+
+	new_psw.mask = extract_psw_mask();
+	new_psw.addr = (unsigned long)restart_write_gs_regs;
+
+	set_flag(0);
+
+	smp_cpu_start(cpu_idx, new_psw);
+
+	wait_for_flag();
+}
+
+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)),
+	       "additional status contents match");
+
+out:
+	report_prefix_pop();
+}
+
 static void ecall(void)
 {
 	unsigned long mask;
@@ -388,6 +643,10 @@  int main(void)
 	test_stop();
 	test_stop_store_status();
 	test_store_status();
+	test_store_adtl_status_unavail();
+	test_store_adtl_status_vector();
+	test_store_adtl_status_gs();
+	test_store_adtl_status();
 	test_ecall();
 	test_emcall();
 	test_sense_running();
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
 
 [sclp-1g]
 file = sclp.elf