Re: [kvm-unit-tests PATCH v10 9/9] s390x: css: ssch/tsch with sense and interrupt

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

[-- Attachment #1.1: Type: text/plain, Size: 3925 bytes --]

On 7/3/20 11:05 AM, Pierre Morel wrote:
> 
> 
> On 2020-07-03 10:41, Thomas Huth wrote:
>> On 02/07/2020 18.31, Pierre Morel wrote:
>>> After a channel is enabled we start a SENSE_ID command using
>>> the SSCH instruction to recognize the control unit and device.
>>>
>>> This tests the success of SSCH, the I/O interruption and the TSCH
>>> instructions.
>>>
>>> The SENSE_ID command response is tested to report 0xff inside
>>> its reserved field and to report the same control unit type
>>> as the cu_type kernel argument.
>>>
>>> Without the cu_type kernel argument, the test expects a device
>>> with a default control unit type of 0x3832, a.k.a virtio-net-ccw.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>> [...]
>>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>>> index 0ddceb1..9c22644 100644
>>> --- a/lib/s390x/css.h
>>> +++ b/lib/s390x/css.h
>>> @@ -11,6 +11,8 @@
>>>   #ifndef CSS_H
>>>   #define CSS_H
>>>   
>>> +#define lowcore_ptr ((struct lowcore *)0x0)
>>
>> I'd prefer if you could either put this into the css_lib.c file or in
>> lib/s390x/asm/arch_def.h.
> 
> I have a patch ready for this :)
> But I did not want to add too much new things in this series that could 
> start a new discussion.
> 
> I have 2 versions of the patch:
> - The simple one with just the declaration in arch_def.h
> - The complete one with update of all tests (but smp) using a pointer to 
> lowcore.
> 

I've seen that patch on your branch and like most maintainers I'm not
incredibly happy with patches touching a single line in a lot of files.

Maybe we can achieve a compromise and only clean up our library. The
tests can be changed when they need to be touched for other changes.

Anyway for now I think css_lib.c might be the right place. We can talk
about a lowcore cleanup next week if you want.

> 
>>
> ...snip...
> 
>>>   static inline int ssch(unsigned long schid, struct orb *addr)
>>> @@ -251,6 +271,16 @@ void dump_orb(struct orb *op);
>>>   
>>>   int css_enumerate(void);
>>>   #define MAX_ENABLE_RETRIES      5
>>> -int css_enable(int schid);
>>> +int css_enable(int schid, int isc);
>>> +
>>> +
>>
>> In case you respin: Remove one empty line?
> 
> yes
> 
>>
>>> +/* Library functions */
>>> +int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw);
> 
> ...snip...
> 
>>> +	lowcore_ptr->io_int_param = 0;
>>> +
>>> +	memset(&senseid, 0, sizeof(senseid));
>>> +	ret = start_single_ccw(test_device_sid, CCW_CMD_SENSE_ID,
>>> +			       &senseid, sizeof(senseid), CCW_F_SLI);
>>> +	if (ret) {
>>> +		report(0, "ssch failed for SENSE ID on sch %08x with cc %d",
>>> +		       test_device_sid, ret);
>>> +		goto unreg_cb;
>>> +	}
>>
>> I'd maybe rather do something like:
>>
>> 	report(ret == 0, "SENSE ID on sch %08x has good CC (%d)", ...)
>> 	if (ret)
>> 		goto unreg_cb;
>>
>> and avoid report(0, ...) statements. Also for the other tests below. But
>> maybe that's really just a matter of taste.
> 
> I prefer to use report(0,....) when an unexpected error occurs: This 
> keep the test silent when what is expected occurs.
> 
> And use report(ret == xxx, ....) as the last report to report overall 
> success or failure of the test.
> 
> Other opinions?

So, I'm not a big fan of changes to the amount of output depending on
the test PASS/FAIL. It screws with my ability to parse the output.

However this is your test, I don't expect other people touching this in
the near future and the output has lots of information where stuff went
wrong. As long as you debug the fails I'm ok with that style :)

> 
>>
>>> +	wait_for_interrupt(PSW_MASK_IO);
> 
> ...snip...
> 
>>
>> Apart from the nits, I'm fine with the patch.
>>
>> Acked-by: Thomas Huth <thuth@redhat.com>

Acked-by: Janosch Frank <frankja@de.ibm.com>

>>
> 
> Thanks,
> Pierre
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]