Re: [kvm-unit-tests PATCH v3] s390x: Add strict mode to specification exception interpretation test

[Janosch Frank <frankja@linux.ibm.com>]

On 7/5/22 13:17, Janis Schoetterl-Glausch wrote:
> While specification exception interpretation is not required to occur,
> it can be useful for automatic regression testing to fail the test if it
> does not occur.
> Add a `--strict` argument to enable this.
> `--strict` takes a list of machine types (as reported by STIDP)
> for which to enable strict mode, for example
> `--strict 3931,8562,8561,3907,3906,2965,2964`
> will enable it for models z16 - z13.
> Alternatively, strict mode can be enabled for all but the listed machine
> types by prefixing the list with a `!`, for example
> `--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
> will enable it for z/Architecture models except those older than z13.
> `--strict !` will enable it always.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

GCC 11.2.0 isn't happy

s390x/spec_ex-sie.c: In function ‘test_spec_ex_sie’:
s390x/spec_ex-sie.c:70:17: error: format not a string literal and no 
format arguments [-Werror=format-security]
    70 |                 report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
       |                 ^~~~~~
s390x/spec_ex-sie.c:72:17: error: format not a string literal and no 
format arguments [-Werror=format-security]
    72 |                 report_info(msg);
       |                 ^~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [<builtin>: s390x/spec_ex-sie.o] Error 1


Other than that the code looks ok to me.
I have to page in the discussion again to know how this fits into the 
picture. Either that or Thomas tells me it's exactly what he wants and 
I'll add it to my queue once the compile problem has been fixed one way 
or another.


> ---
> v2 -> v3
>   * rebase on master
>   * global strict bool
>   * fix style issue
> 
> Range-diff against v2:
> 1:  e9c36970 ! 1:  c707481c s390x: Add strict mode to specification exception interpretation test
>      @@ Commit message
>           Add a `--strict` argument to enable this.
>           `--strict` takes a list of machine types (as reported by STIDP)
>           for which to enable strict mode, for example
>      -    `--strict 8562,8561,3907,3906,2965,2964`
>      -    will enable it for models z15 - z13.
>      +    `--strict 3931,8562,8561,3907,3906,2965,2964`
>      +    will enable it for models z16 - z13.
>           Alternatively, strict mode can be enabled for all but the listed machine
>           types by prefixing the list with a `!`, for example
>           `--strict !1090,1091,2064,2066,2084,2086,2094,2096,2097,2098,2817,2818,2827,2828`
>      @@ s390x/spec_ex-sie.c
>        #include <sclp.h>
>        #include <asm/page.h>
>        #include <asm/arch_def.h>
>      + #include <alloc_page.h>
>      + #include <sie.h>
>      + #include <snippet.h>
>      ++#include <hardware.h>
>      +
>      + static struct vm vm;
>      + extern const char SNIPPET_NAME_START(c, spec_ex)[];
>      + extern const char SNIPPET_NAME_END(c, spec_ex)[];
>      ++static bool strict;
>      +
>      + static void setup_guest(void)
>      + {
>       @@ s390x/spec_ex-sie.c: static void reset_guest(void)
>      - 	vm.sblk->icptcode = 0;
>      - }
>        
>      --static void test_spec_ex_sie(void)
>      -+static void test_spec_ex_sie(bool strict)
>      + static void test_spec_ex_sie(void)
>        {
>       +	const char *msg;
>       +
>      @@ s390x/spec_ex-sie.c: static void test_spec_ex_sie(void)
>       +	if (list[0] == '!') {
>       +		ret = true;
>       +		list++;
>      -+	} else
>      ++	} else {
>       +		ret = false;
>      ++	}
>       +	while (true) {
>       +		long input = 0;
>       +
>      @@ s390x/spec_ex-sie.c: static void test_spec_ex_sie(void)
>       +
>        int main(int argc, char **argv)
>        {
>      ++	strict = parse_strict(argc - 1, argv + 1);
>        	if (!sclp_facilities.has_sief2) {
>      -@@ s390x/spec_ex-sie.c: int main(int argc, char **argv)
>      + 		report_skip("SIEF2 facility unavailable");
>        		goto out;
>      - 	}
>      -
>      --	test_spec_ex_sie();
>      -+	test_spec_ex_sie(parse_strict(argc - 1, argv + 1));
>      - out:
>      - 	return report_summary();
>      - }
> 
>   s390x/spec_ex-sie.c | 53 +++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/spec_ex-sie.c b/s390x/spec_ex-sie.c
> index d8e25e75..e5f39451 100644
> --- a/s390x/spec_ex-sie.c
> +++ b/s390x/spec_ex-sie.c
> @@ -7,16 +7,19 @@
>    * specification exception interpretation is off/on.
>    */
>   #include <libcflat.h>
> +#include <stdlib.h>
>   #include <sclp.h>
>   #include <asm/page.h>
>   #include <asm/arch_def.h>
>   #include <alloc_page.h>
>   #include <sie.h>
>   #include <snippet.h>
> +#include <hardware.h>
>   
>   static struct vm vm;
>   extern const char SNIPPET_NAME_START(c, spec_ex)[];
>   extern const char SNIPPET_NAME_END(c, spec_ex)[];
> +static bool strict;
>   
>   static void setup_guest(void)
>   {
> @@ -37,6 +40,8 @@ static void reset_guest(void)
>   
>   static void test_spec_ex_sie(void)
>   {
> +	const char *msg;
> +
>   	setup_guest();
>   
>   	report_prefix_push("SIE spec ex interpretation");
> @@ -60,16 +65,60 @@ static void test_spec_ex_sie(void)
>   	report(vm.sblk->icptcode == ICPT_PROGI
>   	       && vm.sblk->iprcc == PGM_INT_CODE_SPECIFICATION,
>   	       "Received specification exception intercept");
> -	if (vm.sblk->gpsw.addr == 0xdeadbeee)
> -		report_info("Interpreted initial exception, intercepted invalid program new PSW exception");
> +	msg = "Interpreted initial exception, intercepted invalid program new PSW exception";
> +	if (strict)
> +		report(vm.sblk->gpsw.addr == 0xdeadbeee, msg);
> +	else if (vm.sblk->gpsw.addr == 0xdeadbeee)
> +		report_info(msg);
>   	else
>   		report_info("Did not interpret initial exception");
>   	report_prefix_pop();
>   	report_prefix_pop();
>   }
>   
> +static bool parse_strict(int argc, char **argv)
> +{
> +	uint16_t machine_id;
> +	char *list;
> +	bool ret;
> +
> +	if (argc < 1)
> +		return false;
> +	if (strcmp("--strict", argv[0]))
> +		return false;
> +
> +	machine_id = get_machine_id();
> +	if (argc < 2) {
> +		printf("No argument to --strict, ignoring\n");
> +		return false;
> +	}
> +	list = argv[1];
> +	if (list[0] == '!') {
> +		ret = true;
> +		list++;
> +	} else {
> +		ret = false;
> +	}
> +	while (true) {
> +		long input = 0;
> +
> +		if (strlen(list) == 0)
> +			return ret;
> +		input = strtol(list, &list, 16);
> +		if (*list == ',')
> +			list++;
> +		else if (*list != '\0')
> +			break;
> +		if (input == machine_id)
> +			return !ret;
> +	}
> +	printf("Invalid --strict argument \"%s\", ignoring\n", list);
> +	return ret;
> +}
> +
>   int main(int argc, char **argv)
>   {
> +	strict = parse_strict(argc - 1, argv + 1);
>   	if (!sclp_facilities.has_sief2) {
>   		report_skip("SIEF2 facility unavailable");
>   		goto out;
> 
> base-commit: ca85dda2671e88d34acfbca6de48a9ab32b1810d