Re: [kvm-unit-tests PATCH v1 3/6] s390x: lib: css: upgrading IRQ handling

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

On 3/19/21 5:09 PM, Cornelia Huck wrote:
> On Fri, 19 Mar 2021 16:55:15 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 3/19/21 12:01 PM, Cornelia Huck wrote:
>>> On Thu, 18 Mar 2021 14:26:25 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>> @@ -422,38 +464,38 @@ static struct irb irb;
>>>>    void css_irq_io(void)
>>>>    {
>>>>    	int ret = 0;
>>>> -	char *flags;
>>>> -	int sid;
>>>> +	struct irq_entry *irq;
>>>>    
>>>>    	report_prefix_push("Interrupt");
>>>> -	sid = lowcore_ptr->subsys_id_word;
>>>> +	irq = alloc_irq();
>>>> +	assert(irq);
>>>> +
>>>> +	irq->sid = lowcore_ptr->subsys_id_word;
>>>>    	/* Lowlevel set the SID as interrupt parameter. */
>>>> -	if (lowcore_ptr->io_int_param != sid) {
>>>> +	if (lowcore_ptr->io_int_param != irq->sid) {
>>>>    		report(0,
>>>>    		       "io_int_param: %x differs from subsys_id_word: %x",
>>>> -		       lowcore_ptr->io_int_param, sid);
>>>> +		       lowcore_ptr->io_int_param, irq->sid);
>>>>    		goto pop;
>>>>    	}
>>>>    	report_prefix_pop();
>>>>    
>>>>    	report_prefix_push("tsch");
>>>> -	ret = tsch(sid, &irb);
>>>> +	ret = tsch(irq->sid, &irq->irb);
>>>>    	switch (ret) {
>>>>    	case 1:
>>>> -		dump_irb(&irb);
>>>> -		flags = dump_scsw_flags(irb.scsw.ctrl);
>>>> -		report(0,
>>>> -		       "I/O interrupt, but tsch returns CC 1 for subchannel %08x. SCSW flags: %s",
>>>> -		       sid, flags);
>>>> +		report_info("no status pending on %08x : %s", irq->sid,
>>>> +			    dump_scsw_flags(irq->irb.scsw.ctrl));
>>>
>>> This is not what you are looking at here, though?
>>>
>>> The problem is that the hypervisor gave you cc 1 (stored, not status
>>> pending) while you just got an interrupt; the previous message logged
>>> that, while the new one does not. (The scsw flags are still
>>> interesting, as it gives further information about the mismatch.)
>>
>> I can keep the old message.
>> How ever I do not think it is a reason to report a failure.
>> Do you agree with replaacing report(0,) with report_info()
> 
> I don't really see how we could get an I/O interrupt for a subchannel
> that is not status pending, unless we have other code racing with this
> one that cleared the status pending already (and that would be a bug in
> our test program.) Or are you aware in anything in the architecture
> that could make the status pending go away again (other than the
> subchannel becoming not operational?)

:) no
I really messed up with this patch.
sorry, can only do better


> 
>>
>>>    
>>>>    		break;
>>>>    	case 2:
>>>>    		report(0, "tsch returns unexpected CC 2");
>>>>    		break;
>>>>    	case 3:
>>>> -		report(0, "tsch reporting sch %08x as not operational", sid);
>>>> +		report(0, "tsch reporting sch %08x as not operational", irq->sid);
>>>>    		break;
>>>>    	case 0:
>>>>    		/* Stay humble on success */
>>>> +		save_irq(irq);
>>>>    		break;
>>>>    	}
>>>>    pop:
>>>> @@ -498,47 +540,70 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags)
>>>>    int wait_and_check_io_completion(int schid)
>>>>    {
>>>>    	int ret = 0;
>>>> -
>>>> -	wait_for_interrupt(PSW_MASK_IO);
>>>> +	struct irq_entry *irq = NULL;
>>>>    
>>>>    	report_prefix_push("check I/O completion");
>>>>    
>>>> -	if (lowcore_ptr->io_int_param != schid) {
>>>> +	disable_io_irq();
>>>> +	irq = get_irq();
>>>> +	while (!irq) {
>>>> +		wait_for_interrupt(PSW_MASK_IO);
>>>> +		disable_io_irq();
>>>
>>> Isn't the disable_io_irq() redundant here?
>>
>> No because wait for interrupt re-enable the interrupts
>> I will add a comment
> 
> Hm, I thought it restored the previous status.
> 
>>
>>>
>>> (In general, I'm a bit confused about the I/O interrupt handling here.
>>> Might need to read through the whole thing again.)
> 
> But also see this comment :)
> 

Oh you mean the comment were it restores the psw mask.
yes,I see it now.
hum
yes, this patch is awful. really sorry

please do not lose more time I must really rework the all series.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen