Re: [kvm-unit-tests PATCH 5/6] s390x: uv-guest: Test invalid commands

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

On Tue, 16 Mar 2021 09:16:53 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's check if the commands that are not indicated as available
> produce a invalid command error.

you say this, but you don't actually check that the commands are
actually reported as unavailable

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/uv-guest.c | 44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
> index 8915b2f1..517e3c66 100644
> --- a/s390x/uv-guest.c
> +++ b/s390x/uv-guest.c
> @@ -120,16 +120,46 @@ static void test_sharing(void)
>  	report_prefix_pop();
>  }
>  
> +static struct {
> +	const char *name;
> +	uint16_t cmd;
> +	uint16_t len;
> +} invalid_cmds[] = {
> +	{ "bogus", 0x4242, sizeof(struct uv_cb_header) },
> +	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init) },
> +	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct
> uv_cb_cgc) },
> +	{ "destroy conf", UVC_CMD_DESTROY_SEC_CONF, sizeof(struct
> uv_cb_nodata) },
> +	{ "create cpu", UVC_CMD_CREATE_SEC_CPU, sizeof(struct
> uv_cb_csc) },
> +	{ "destroy cpu", UVC_CMD_DESTROY_SEC_CPU, sizeof(struct
> uv_cb_nodata) },
> +	{ "conv to", UVC_CMD_CONV_TO_SEC_STOR, sizeof(struct
> uv_cb_cts) },
> +	{ "conv from", UVC_CMD_CONV_FROM_SEC_STOR, sizeof(struct
> uv_cb_cfs) },
> +	{ "set sec conf", UVC_CMD_SET_SEC_CONF_PARAMS, sizeof(struct
> uv_cb_ssc) },
> +	{ "unpack", UVC_CMD_UNPACK_IMG, sizeof(struct uv_cb_unp) },
> +	{ "verify", UVC_CMD_VERIFY_IMG, sizeof(struct uv_cb_nodata)
> },
> +	{ "cpu reset", UVC_CMD_CPU_RESET, sizeof(struct
> uv_cb_nodata) },
> +	{ "cpu initial reset", UVC_CMD_CPU_RESET_INITIAL,
> sizeof(struct uv_cb_nodata) },
> +	{ "conf clear reset", UVC_CMD_PERF_CONF_CLEAR_RESET,
> sizeof(struct uv_cb_nodata) },
> +	{ "cpu clear reset", UVC_CMD_CPU_RESET_CLEAR, sizeof(struct
> uv_cb_nodata) },
> +	{ "cpu set state", UVC_CMD_CPU_SET_STATE, sizeof(struct
> uv_cb_cpu_set_state) },
> +	{ "pin shared", UVC_CMD_PIN_PAGE_SHARED, sizeof(struct
> uv_cb_cfs) },
> +	{ "unpin shared", UVC_CMD_UNPIN_PAGE_SHARED, sizeof(struct
> uv_cb_cts) },
> +	{ NULL, 0, 0 },
> +};
> +
>  static void test_invalid(void)
>  {
> -	struct uv_cb_header uvcb = {
> -		.len = 16,
> -		.cmd = 0x4242,
> -	};
> -	int cc;
> +	struct uv_cb_header *hdr = (void *)page;
> +	int cc, i;
>  
> -	cc = uv_call(0, (u64)&uvcb);
> -	report(cc == 1 && uvcb.rc == UVC_RC_INV_CMD, "invalid
> command");
> +	report_prefix_push("invalid");

here you just blindly loop over all the commands, without checking
their actual availability

> +	for (i = 0; invalid_cmds[i].name; i++) {

maybe you can add another field for the availability bit (or even put
them in the right order so the bit is the index) and here add something
like

if (uv_query_test_feature(i))
	continue;

so you will be sure the command is not available

> +		hdr->cmd = invalid_cmds[i].cmd;
> +		hdr->len = invalid_cmds[i].len;
> +		cc = uv_call(0, (u64)hdr);
> +		report(cc == 1 && hdr->rc == UVC_RC_INV_CMD, "%s",
> +		       invalid_cmds[i].name);
> +	}
> +	report_prefix_pop();
>  }
>  
>  int main(void)