Re: [kvm-unit-tests PATCH v2 9/9] s390x: css: ping pong

[Pierre Morel <pmorel@linux.ibm.com>]

On 2019-12-02 19:25, Pierre Morel wrote:
> 
> 
> On 2019-12-02 16:03, Cornelia Huck wrote:
>> On Thu, 28 Nov 2019 13:46:07 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> To test a write command with the SSCH instruction we need a QEMU device,
>>> with control unit type 0xC0CA. The PONG device is such a device.
>>
>> "We want to test some read/write ccws via the SSCH instruction with a
>> QEMU device with control unit type 0xC0CA." ?
>>
>>>
>>> This type of device respond to PONG_WRITE requests by incrementing an
>>
>> s/respond/responds/
> 
> oiiinnnn..... but yes
> 
>>
>>> integer, stored as a string at offset 0 of the CCW data.
>>>
>>> This is only a success test, no error expected.
>>
>> Nobody expects the Spanish Inquisition^W^W^W an error :)
>>
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   s390x/css.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/s390x/css.c b/s390x/css.c
>>> index 534864f..0761e70 100644
>>> --- a/s390x/css.c
>>> +++ b/s390x/css.c
>>> @@ -23,6 +23,10 @@
>>>   #define SID_ONE        0x00010000
>>>   #define PSW_PRG_MASK (PSW_MASK_IO | PSW_MASK_EA | PSW_MASK_BA)
>>> +/* Local Channel Commands */
>>
>> /* Channel commands for the PONG device */
>>
>> ?
> 
> better, thanks
> 
>>
>>> +#define PONG_WRITE    0x21 /* Write */
>>> +#define PONG_READ    0x22 /* Read buffer */
>>> +
>>>   struct lowcore *lowcore = (void *)0x0;
>>>   static struct schib schib;
>>> @@ -31,7 +35,8 @@ static struct ccw ccw[NB_CCW];
>>>   #define NB_ORB  100
>>>   static struct orb orb[NB_ORB];
>>>   static struct irb irb;
>>> -static char buffer[0x1000] __attribute__ ((aligned(8)));
>>> +#define BUF_SZ    0x1000
>>> +static char buffer[BUF_SZ] __attribute__ ((aligned(8)));
>>
>> Merge this with the introduction of this variable?
> 
> yes, better too
> 
>>
>>>   static struct senseid senseid;
>>>   static const char *Channel_type[3] = {
>>> @@ -224,6 +229,44 @@ static void test_sense(void)
>>>           report("cu_type: expect c0ca, got %04x", 0, senseid.cu_type);
>>>   }
>>> +static void test_ping(void)
>>> +{
>>> +    int success, result;
>>> +    int cnt = 0, max = 4;
>>> +
>>> +    if (senseid.cu_type != PONG_CU) {
>>> +        report_skip("No PONG, no ping-pong");
>>
>> :D
>>
>>> +        return;
>>> +    }
>>> +
>>> +    enable_io_irq();
>>
>> Hasn't that been enabled already for doing SenseID?
> 
> Yes, but same remark as before, the sub tests here are independant, 
> started from the test[] table.
> If the sense test is commented out, the...

hum, coming back here again, the tests depends one from the other and 
are absolutely not independent as I pretended. so...
You are right I do not need to enable IRQ here.

> 
>>
>>> +
>>> +    while (cnt++ < max) {
>>> +report_info("cnt..: %08x", cnt);
>>
>> Wrong indentation?
> 
> wrong report !
> forgotten from a test.
> 
> Thanks for the review,
> Regards,
> Pierre
> 
> 

-- 
Pierre Morel
IBM Lab Boeblingen