Re: [qemu-s390x] [PATCH v5 11/12] s390-ccw: clear pending irqs

[Collin L. Walling]

On 02/06/2018 05:07 AM, Thomas Huth wrote:
> On 05.02.2018 21:57, Collin L. Walling wrote:
> > It is possible while waiting for multiple types of external
> > interrupts that we might have pending irqs remaining between
> > irq consumption and irq disabling. Those interrupts could
> > propagate to the guest after IPL completes and cause unwanted
> > behavior.
> >
> > To avoid this, we clear the write event mask to prevent further
> > service interrupts from ASCII events and then consume all pending
> > irqs for a miniscule duration. Once finished, we reset the write
> > event mask and resume business as usual.
> >
> > Signed-off-by: Collin L. Walling <address@hidden>
> > ---
> >   pc-bios/s390-ccw/menu.c | 16 ++++++++++++++++
> >   pc-bios/s390-ccw/sclp.c | 12 ++++++++++++
> >   2 files changed, 28 insertions(+)
> >
> > diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> > index 85d285f..971f6b6 100644
> > --- a/pc-bios/s390-ccw/menu.c
> > +++ b/pc-bios/s390-ccw/menu.c
> > @@ -64,6 +64,20 @@ static inline bool check_clock_int(void)
> >       return *code == 0x1004;
> >   }
> >   
> > +static void clear_pending_irqs(void)
> > +{
> > +    uint64_t time = 50 * TOD_CLOCK_SECOND / 0x3e8;
> s/0x3e8/1000/ please.


Agreed.


> > +    sclp_clear_write_mask();
> > +
> > +    set_clock_comparator(get_clock() + time);
> > +    enable_clock_int();
> > +    consume_sclp_int();
> > +    disable_clock_int();
> > +
> > +    sclp_setup(); /* re-enable write mask */
> > +}
> I'm pretty much confused by this code. First, isn't there a small chance
> that there is a clock int between consume_sclp_int() and
> disable_clock_int() (if consume_sclp_int() has consumed an SCLP
> interrupt instead of a clock interrupt) ?


By clearing the write mask, we can no longer send nor receive ASCII events
(such as prints or keystrokes). Therefore we will not have any service
interrupts originating from keystrokes.

Theoretically, the only interrupt we should be waiting for in this case
is from the clock.


> Second, if you finally enable the SCLP write mask again, doesn't that
> mean that there could be interrupts pending again afterwards?


Yes.  When the write mask is re-enabled for cp_receive mask, we can still
get pending service interrupts originating from keystrokes (or other 
external,
ASCII related interruptions). Hopefully the user does not fall asleep
and smack their head on the keyboard during the IPL process. (joking)

Ideally (as it was suggested in a previous review), we should refactor
the consume_sclp_int so we can manually enable / disable service interrupts,
but I think this is a good first step, no?

We also might be able to get away with just not setting the receive mask at
all when we re-enable the write mask. That will prevent any interrupts from
keystrokes.


>   Thomas


--
- Collin L Walling