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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On Wed, 2023-03-01 at 23:47 +0100, BALATON Zoltan wrote:
> 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?

Yeah, it is a bit weird that we store the ICW1 byte in *separate*
elements of persistent state, each of *them* a uint8_t which holds only
a single bit of ICW1:

            s->init4 = val & 1;
            s->single_mode = val & 2;
            s->ltim = val & 8;

Still, it's a pattern that's easy enough to follow.

As for the rest of the typing, I'd basically done the bulk of it
already when showing how to adjust the existing (s->elcr&mask) checks;
there was only one more of those to type.

And then the vmstate part is basically just a cut and paste of the bit
in docs/devel/migration.rst which tells you exactly how to do it.

Patch follows. It builds, but I'll let you do the actual testing,
including migration to/from the new version, checking with
scripts/analyze-migration.py that the ltim is there when it should be,
and isn't when it shouldn't, and any other review feedback.







[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]