Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[David Woodhouse]

On Wed, 2023-03-01 at 19:01 +0100, BALATON Zoltan wrote:
> 
> > It isn't a *correct* fix without a little bit more typing, but does
> > this make it work?
> > 
> > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> > index 17910f3bcb..36ebcff025 100644
> > --- a/hw/intc/i8259.c
> > +++ b/hw/intc/i8259.c
> > @@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr 
> > addr64,
> >              if (val & 0x08) {
> >                  qemu_log_mask(LOG_UNIMP,
> >                                "i8259: level sensitive irq not 
> > supported\n");
> > +                s->elcr = 0xff;
> 
> This works too. I guess the log can be then removed too. Could you submit 
> a proper patch or you want me to do that so we can drop the workaround for 
> it? Thanks for looking into it.


Happy for you to do the rest of the typing ... :)

So, *ideally* I think you need to introduce a new field in the
PICCommonState which records the status of the LTIM bit. And fix up the
vmstate_pic_common in hw/intc/i8259_common.c to save and restore that
(with versioning for upgrade/downgrade).

Then you find those places which currently check the bit for the
specific pin in s->elcr, and make them something like:

--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
     }
 #endif
 
-    if (s->elcr & mask) {
+    if (s->ltim || (s->elcr & mask)) {
         /* level triggered */
         if (level) {
             s->irr |= mask;

It *might* be that you should make the LTIM behaviour optional, so that
only certain incarnations of the i8259 actually get it at all and it
*wouldn't* take effect if a guest tried to set it, which is what the
PIIX3 datasheet implies. But I suspect we can get away without that.

smime.p7s
Description: S/MIME cryptographic signature