Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

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

On 7/9/21 4:22 PM, Janis Schoetterl-Glausch wrote:
> On 7/9/21 11:22 AM, Cornelia Huck wrote:
>> On Tue, Jul 06 2021, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
>>
>>> Generate specification exceptions and check that they occur.
>>> Also generate specification exceptions during a transaction,
>>> which results in another interruption code.
>>> With the iterations argument one can check if specification
>>> exception interpretation occurs, e.g. by using a high value and
>>> checking that the debugfs counters are substantially lower.
>>> The argument is also useful for estimating the performance benefit
>>> of interpretation.
>>>
>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>> ---
>>>  s390x/Makefile           |   1 +
>>>  lib/s390x/asm/arch_def.h |   1 +
>>>  s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>>>  s390x/unittests.cfg      |   3 +
>>>  4 files changed, 349 insertions(+)
>>>  create mode 100644 s390x/spec_ex.c
>>
>> (...)
>>
>>> +static void lpsw(uint64_t psw)
>>
>> Maybe call this load_psw(), as you do a bit more than a simple lpsw?
> 
> [...]
> 
>> The indentation looks a bit funny here.
> 
> [...]
> 
>> Here as well.
> 
> Ok, will fix.
>>
>>> +}
>>
>> (...)
>>
>>> +#define report_info_if(cond, fmt, ...)			\
>>> +	do {						\
>>> +		if (cond) {				\
>>> +			report_info(fmt, ##__VA_ARGS__);\
>>> +		}					\
>>> +	} while (0)
>>
>> I'm wondering whether such a wrapper function could be generally useful.
>>
> 
> I've found 9 occurrences with:
> find . -type f \( -name "*.c" -o -name "*.h" \) -exec awk '/if\s*\(.*/{i=2;f=$0} /report_info/ && i>0{print FILENAME, NR-1 ":" f;r=4} r>1{print FILENAME, NR ":" $0;r--} r==1{print "--";r=0} {i--}' '{}' \;
> 

Looking at the occurrences below most of those also do other things in
the conditional branches so for 90% we can't do a straight forward replace.

Nevertheless I see some value in it and the 6 lines won't hurt us, so
please create a separate patch for this and put the maintainers for the
other arches and Paolo on CC for that patch so they know the new wrapper
will be available.

Also I'd like having report_fail("Message"); and report_pass("Message");
functions instead of report(0, "Message"); and report(1, "Message"); ...
Those at least are straight forward replacements.


> ./lib/s390x/css_lib.c 177:      if (cc) {
> ./lib/s390x/css_lib.c 178:              report_info("stsch: updating sch %08x failed with cc=%d",
> ./lib/s390x/css_lib.c 179:                          schid, cc);
> ./lib/s390x/css_lib.c 180:              return false;
> --
> ./lib/s390x/css_lib.c 183:      if (!(pmcw->flags & PMCW_ENABLE)) {
> ./lib/s390x/css_lib.c 184:              report_info("stsch: sch %08x not enabled", schid);
> ./lib/s390x/css_lib.c 185:              return false;
> ./lib/s390x/css_lib.c 186:      }
> --
> ./lib/s390x/css_lib.c 207:      if (cc) {
> ./lib/s390x/css_lib.c 208:              report_info("stsch: sch %08x failed with cc=%d", schid, cc);
> ./lib/s390x/css_lib.c 209:              return cc;
> ./lib/s390x/css_lib.c 210:      }
> --
> ./lib/s390x/css_lib.c 213:      if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) {
> ./lib/s390x/css_lib.c 214:              report_info("stsch: sch %08x already enabled", schid);
> ./lib/s390x/css_lib.c 215:              return 0;
> ./lib/s390x/css_lib.c 216:      }
> --
> ./lib/s390x/css_lib.c 269:      if (cc) {
> ./lib/s390x/css_lib.c 270:              report_info("stsch: sch %08x failed with cc=%d", schid, cc);
> ./lib/s390x/css_lib.c 271:              return false;
> ./lib/s390x/css_lib.c 272:      }
> --
[...]