Re: [kvm-unit-tests PATCH v11 8/9] s390x: css: msch, enable test

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

On 2020-07-09 16:22, Cornelia Huck wrote:
> On Thu, 9 Jul 2020 15:58:07 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 2020-07-09 15:52, Cornelia Huck wrote:
>>> On Thu, 9 Jul 2020 15:41:56 +0200
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> On 2020-07-09 15:30, Cornelia Huck wrote:
>>>>> On Thu, 9 Jul 2020 15:12:05 +0200
>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>>       
>>>>>> On 2020-07-09 13:40, Cornelia Huck wrote:
>>>>>>> On Thu,  9 Jul 2020 10:07:47 +0200
>>>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>>>>> (...)
>>>>>>>          
>>>>>>>> +/*
>>>>>>>> + * css_msch: enable subchannel and set with specified ISC
>>>>>>>
>>>>>>> "css_enable: enable the subchannel with the specified ISC"
>>>>>>>
>>>>>>> ?
>>>>>>>          
>>>>>>>> + * @schid: Subchannel Identifier
>>>>>>>> + * @isc  : number of the interruption subclass to use
>>>>>>>> + * Return value:
>>>>>>>> + *   On success: 0
>>>>>>>> + *   On error the CC of the faulty instruction
>>>>>>>> + *      or -1 if the retry count is exceeded.
>>>>>>>> + */
>>>>>>>> +int css_enable(int schid, int isc)
>>>>>>>> +{
>>>>>>>> +	struct pmcw *pmcw = &schib.pmcw;
>>>>>>>> +	int retry_count = 0;
>>>>>>>> +	uint16_t flags;
>>>>>>>> +	int cc;
>>>>>>>> +
>>>>>>>> +	/* Read the SCHIB for this subchannel */
>>>>>>>> +	cc = stsch(schid, &schib);
>>>>>>>> +	if (cc) {
>>>>>>>> +		report_info("stsch: sch %08x failed with cc=%d", schid, cc);
>>>>>>>> +		return cc;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT);
>>>>>>>> +	if ((pmcw->flags & flags) == flags) {
>>>>>>>
>>>>>>> I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this
>>>>>>> catches the case of "subchannel has been enabled before, but with a
>>>>>>> different isc".
>>>>>>
>>>>>> If with a different ISC, we need to modify the ISC.
>>>>>> Don't we ?
>>>>>
>>>>> I think that's a policy decision (I would probably fail and require a
>>>>> disable before setting another isc, but that's a matter of taste).
>>>>>
>>>>> Regardless, I think the current check doesn't even catch the 'different
>>>>> isc' case?
>>>>
>>>> hum, right.
>>>> If it is OK I remove this one.
>>>> And I must rework the same test I do later
>>>>     in this patch.
>>>
>>> So, you mean checking for PMCW_ENABLE? Or not at all?
>>>
>>> (I'd check for PMCW_ENABLE.)
>>>    
>>
>> -       if ((pmcw->flags & flags) == flags) {
>> +       if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) {
>>                   report_info("stsch: sch %08x already enabled", schid);
>>                   return 0;
>>           }
>>
>> I keep both, otherwise I return 0 without setting the ISC.
> 
> Ah, I missed the 'return 0'.
> 
>>    then I have another error:
>>
>>    retry:
>>           /* Update the SCHIB to enable the channel and set the ISC */
>> +       pmcw->flags &= ~(PMCW_ISC_MASK | PMCW_ENABLE);
> 
> Maybe ~PMCW_ISC_MASK is enough?

yes

> 
>>           pmcw->flags |= flags;
>>
>> and finaly the same as the first later...
>>
>> -       if ((pmcw->flags & flags) == flags) {
>> +       if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) {
> 
> I think you can keep that as-is.

I don't thing so, I just stored the pmcw.
if ISC is stored as 3 and I want 1 it is a false positive.
Same error as you showed me before.
?

> 
>>                   report_info("stsch: sch %08x successfully modified
>> after %d retries",
>>                               schid, retry_count);
>>
>>
>> is better I think.
>> What do you think?
> 
> It's probably the right direction.
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen