Re: [kvm-unit-tests PATCH v3 1/2] s390x: Add specification exception test

[Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com>]

On 10/26/21 15:41, Claudio Imbrenda wrote:
> On Tue, 26 Oct 2021 14:00:31 +0200
> Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com> wrote:
> 
> [...]
> 
>> I don't think that would work, the compiler might inline the function,
>> duplicating the label.
> 
> __attribute__((noinline))
> 
> :)

+ a comment on why it's necessary and at that point I don't think it's worth it.
> 
>> I suppose I could replace the stg with an assignment in C, not sure if that's nicer.
>>
>>>> +	fixup_psw.mask = extract_psw_mask();  
>>>
>>> then you could add this here:
>>> 	fixup_psw.addr = after_lpswe;
>>>   
>>>> +	asm volatile (
>>>> +		"	larl	%[scratch],nop%=\n"
>>>> +		"	stg	%[scratch],%[addr]\n"  
>>> 	^ those two lines are no longer needed ^  
>>>> +		"	lpswe	%[psw]\n"
>>>> +		"nop%=:	nop\n"  
>>> 	".global after_lpswe \n"
>>> 	"after_lpswe:	nop"  
>>>> +		: [scratch] "=&r"(scratch),
>>>> +		  [addr] "=&T"(fixup_psw.addr)
>>>> +		: [psw] "Q"(psw)
>>>> +		: "cc", "memory"
>>>> +	);
>>>> +}
> 
> [...]
>  
>> That's nicer indeed.
>>>   
>>>> +	asm volatile ("lpq %0,%2"
>>>> +		      : "=r"(r1), "=r"(r2)  
>>>
>>> since you're ignoring the return value, can't you hardcode r6, and mark
>>> it (and r7) as clobbered? like:
>>> 		"lpq 6, %[bad]"
>>> 		: : [bad] "T"(words[1])
>>> 		: "%r6", "%r7" 
>>>   
>> Ok, btw. is there a reason bare register numbers seem to be more common
>> compared to %%rN ?
> 
> I don't know, I guess laziness?
> 
>>
>>>> +		      : "T"(*bad_aligned)
>>>> +	);
>>>> +}
>>>> +
>>>> +static void not_even(void)
>>>> +{
>>>> +	uint64_t quad[2];
>>>> +
>>>> +	register uint64_t r1 asm("7");
>>>> +	register uint64_t r2 asm("8");
>>>> +	asm volatile (".insn	rxy,0xe3000000008f,%0,%2" //lpq
>>>> %0,%2  
>>>
>>> this is even uglier. I guess you had already tried this?
>>>   
>> Yes, the assembler won't let you do that.
> 
> yeah I thought so
> 
>>
>>> 		"lpq 7, %[good]"
>>> 			: : [good] "T"(quad)
>>> 			: "%r7", "%r8"
>>>
>>> if that doesn't work, then the same but with .insn
> 
> I guess you can still try this ^ ?

Ok.
> 
>>>   
>>>> +		      : "=r"(r1), "=r"(r2)
>>>> +		      : "T"(quad)
>>>> +	);
>>>> +}
>>>> +
>>>> +struct spec_ex_trigger {
>>>> +	const char *name;
>>>> +	void (*func)(void);
>>>> +	void (*fixup)(void);
>>>> +};
>>>> +
>>>> +static const struct spec_ex_trigger spec_ex_triggers[] = {
>>>> +	{ "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw},
>>>> +	{ "bad_alignment", &bad_alignment, NULL},
>>>> +	{ "not_even", &not_even, NULL},
>>>> +	{ NULL, NULL, NULL},
>>>> +};
>>>> +  
>>>
>>> this is a lot of infrastructure for 3 tests... (or even for 5 tests,
>>> since you will add the transactions in the next patch)  
>>
>> Is it? I think we'd want a test for a "normal" specification exception,
>> and one for an invalid PSW at least. Even for just those two, I don't
>> think it would be nice to duplicate the test_spec_ex harness.
> 
> usually we do duplicate code for simple tests, so that reviewers have
> an easier time understanding what's going on, on the other hand..
> 
>>>
>>> are you planning to significantly extend this test in the future?  
>>
>> Not really, but I thought having it be easily extensible might be nice.
> 
> ..fair enough
> 
> this way it will be easier to extend this in the future, even though we
> don't have any immediate plans to do so
> 
> maybe add some words in the patch description, and some comments, to
> explain what's going on, to make it easier for others to understand
> this code

Ok.
> 
>>>   
>>>> +struct args {
>>>> +	uint64_t iterations;
>>>> +};
>>>> +
>>>> +static void test_spec_ex(struct args *args,
>>>> +			 const struct spec_ex_trigger *trigger)
>>>> +{
>>>> +	uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>>>> +	uint16_t pgm;
>>>> +	unsigned int i;
>>>> +
>>>> +	for (i = 0; i < args->iterations; i++) {
>>>> +		expect_pgm_int();
>>>> +		register_pgm_cleanup_func(trigger->fixup);
>>>> +		trigger->func();
>>>> +		register_pgm_cleanup_func(NULL);
>>>> +		pgm = clear_pgm_int();
>>>> +		if (pgm != expected_pgm) {
>>>> +			report_fail("Program interrupt: expected(%d)
>>>> == received(%d)",
>>>> +				    expected_pgm,
>>>> +				    pgm);
>>>> +			return;
>>>> +		}
>>>> +	}
>>>> +	report_pass("Program interrupt: always expected(%d) ==
>>>> received(%d)",
>>>> +		    expected_pgm,
>>>> +		    expected_pgm);
>>>> +}
>>>> +
>>>> +static struct args parse_args(int argc, char **argv)  
>>>
>>> do we _really_ need commandline arguments?
>>>   
>> No, but they can be useful.
>> The iterations argument can be used to check if interpretation happens.
>> The transaction arguments can be useful while developing a test case.
>>
>>> is it really so important to be able to control these parameters?
>>>
>>> can you find some values for the parameters so that the test works (as
>>> in, it actually tests what it's supposed to) and also so that the whole
>>> unit test ends in less than 30 seconds?  
>>
>> I think the defaults are fine for that, no?
> 
> ok so they are only for convenience in case things go wrong?

Yes, for when you want to poke at it manually for whatever reason.
> 
>>>   
>>>> +{
>>>> +	struct args args = {
>>>> +		.iterations = 1,
>>>> +	};
>>>> +	unsigned int i;
>>>> +	long arg;
>>>> +	bool no_arg;
>>>> +	char *end;
>>>> +
>>>> +	for (i = 1; i < argc; i++) {
>>>> +		no_arg = true;
>>>> +		if (i < argc - 1) {
>>>> +			no_arg = *argv[i + 1] == '\0';
>>>> +			arg = strtol(argv[i + 1], &end, 10);
>>>> +			no_arg |= *end != '\0';
>>>> +			no_arg |= arg < 0;
>>>> +		}
>>>> +
>>>> +		if (!strcmp("--iterations", argv[i])) {
>>>> +			if (no_arg)
>>>> +				report_abort("--iterations needs a
>>>> positive parameter");
>>>> +			args.iterations = arg;
>>>> +			++i;
>>>> +		} else {
>>>> +			report_abort("Unsupported parameter '%s'",
>>>> +				     argv[i]);
>>>> +		}
>>>> +	}
> 
> I wonder if we can factor out the parameter parsing

I don't think it's worth it. Only three arguments are handled the same.
Doing this might be worthwhile tho:

        for (i = 1; i < argc; i++) {                                            
                no_arg = true;                                                  
                if (i < argc - 1) {                                             
                        no_arg = *argv[i + 1] == '\0';                          
                        arg = strtol(argv[i + 1], &end, 10);                    
                        no_arg |= *end != '\0';                                 
                        no_arg |= arg < 0;                                      
                }                                                               
                                                                                
                cmp = "--iterations";                                           
                argp = &args.iterations;                                        
                if (!strcmp(cmp, argv[i])) {                                    
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                cmp = "--max-retries";                                          
                argp = &args.max_retries;                                       
                if (!strcmp(cmp, argv[i])) {                                    
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                cmp = "--suppress-info";                                        
                argp = &args.suppress_info;                                     
                if (!strcmp(cmp, argv[i])) {                                    
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                cmp = "--max-failures";                                         
                argp = &args.max_failures;                                      
                if (!strcmp(cmp, argv[i])) {                                    
                        max_failures = true;                                    
                                                                                
                        if (no_arg)                                             
                                report_abort("%s needs a positive parameter", cmp);
                        *argp = arg;                                            
                        ++i;                                                    
                        continue;                                               
                }                                                               
                if (!strcmp("--diagnose", argv[i])) {                           
                        args.diagnose = true;                                   
                        continue;
		}                                              
                if (!strcmp("--no-diagnose", argv[i])) {                        
                        args.diagnose = false;                                  
                        continue;                                               
                }                                                               
                report_abort("Unsupported parameter '%s'",                      
                             argv[i]);                                          
        }
> 
>>>> +	return args;
>>>> +}
>>>> +
>>>> +int main(int argc, char **argv)
>>>> +{
>>>> +	unsigned int i;
>>>> +
>>>> +	struct args args = parse_args(argc, argv);
>>>> +
>>>> +	report_prefix_push("specification exception");
>>>> +	for (i = 0; spec_ex_triggers[i].name; i++) {
>>>> +		report_prefix_push(spec_ex_triggers[i].name);
>>>> +		test_spec_ex(&args, &spec_ex_triggers[i]);
>>>> +		report_prefix_pop();
>>>> +	}
>>>> +	report_prefix_pop();
>>>> +
>>>> +	return report_summary();
>>>> +}
>>>> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
>>>> index 9e1802f..5f43d52 100644
>>>> --- a/s390x/unittests.cfg
>>>> +++ b/s390x/unittests.cfg
>>>> @@ -109,3 +109,6 @@ file = edat.elf
>>>>  
>>>>  [mvpg-sie]
>>>>  file = mvpg-sie.elf
>>>> +
>>>> +[spec_ex]
>>>> +file = spec_ex.elf  
>>>   
>>
>