Re: [kvm-unit-tests PATCH 5/5] s390x: ap: Add reset tests

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

On 4/4/23 13:40, Janosch Frank wrote:
> On 4/3/23 16:57, Pierre Morel wrote:
>>
>> On 3/30/23 13:42, Janosch Frank wrote:
>>> Test if the IRQ enablement is turned off on a reset or zeroize PQAP.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    lib/s390x/ap.c | 68 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    lib/s390x/ap.h |  4 +++
>>>    s390x/ap.c     | 52 ++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 124 insertions(+)
>>>
>>> diff --git a/lib/s390x/ap.c b/lib/s390x/ap.c
>>> index aaf5b4b9..d969b2a5 100644
>>> --- a/lib/s390x/ap.c
>>> +++ b/lib/s390x/ap.c
>>> @@ -113,6 +113,74 @@ int ap_pqap_qci(struct ap_config_info *info)
>>>        return cc;
>>>    }
>>>    +static int pqap_reset(uint8_t ap, uint8_t qn, struct 
>>> ap_queue_status *r1,
>>> +              bool zeroize)
>>
>>
>> NIT. Personal opinion, I find using this bool a little obfuscating and I
>> would have prefer 2 different functions.
>>
>> I see you added a ap_pqap_reset() and ap_pqap_zeroize() next in the 
>> code.
>
> Yes, because the names of the functions include the zeroize parts 
> which makes it easier for developers to understand how they work 
> instead of having a bool argument where they need to look up at which 
> argument position it is.
>
>>
>> Why this intermediate level?
>
> So I don't need to repeat the function below for a different r0.fc, no?


question of taste anyway.


[...]


>>
>>
>>> +    } while (cc == 0 && apqsw.irq_enabled == 0);
>>> +    report(apqsw.irq_enabled == 1, "IRQs enabled");
>>> +
>>> +    ap_pqap_reset(apn, qn, &apqsw);
>>> +    cc = ap_pqap_tapq(apn, qn, &apqsw, &r2);
>>> +    assert(!cc);
>>> +    report(apqsw.irq_enabled == 0, "IRQs have been disabled");
>>
>> shouldn't we check that the APQ is fine apqsw.rc == 0 ?
>
> Isn't that covered by the assert above?

May be.

This is the kind of thing where I find the implementation and 
documentation not very logical.

- CC = 0 means that the instruction was processed correctly.

- APQSW reports the status of the AP queue

For any operation but TAPQ I understand that CC=3 if APQSW is != 0

but for TAPQ, if it is processed correctly it should give back the 
APQSW. Isn't it exactly what we ask the TAPQ to do?

I am probably not the only one to think that CC for TAPQ is at least not 
useful, the Linux implementation ignores it.

You are probably right but in doubt I would do as in Linux 
implementation and ignore CC,