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

[Cornelia Huck <cohuck@redhat.com>]

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?)

> 
> >   
> >>   		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 :)

> >   
> >> +		irq = get_irq();
> >> +		report_info("next try");
> >> +	}
> >> +	enable_io_irq();