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

[BALATON Zoltan]

On Wed, 1 Mar 2023, David Woodhouse wrote:
> 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 ... :)

I don't mind the typing but this is quite a bit more involved than I 
expected. You've lost me at the vmstate stuff which I don't quite know how 
to change or test. If these were stored as bits in an ISW1 register as 
described by the docs I've looked at now then no change in migration would 
be needed but this isn't how it seems to be in QEMU so I give up on that 
in this case. Could you please do the typing then?

Thank you,
BALATON Zoltan

> 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.