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

[BALATON Zoltan]

On Thu, 2 Mar 2023, David Woodhouse wrote:
> 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.

I've tested that it fixes the problem with MorphOS on pegasos2 and checked 
that a migration file created while at the firmware, before MorphOS sets 
ltim does not have the subsection while migrate after MorphOS boot has it:

                {
                    "name": "single_mode",
                    "type": "uint8",
                    "size": 1
                },
                {
                    "name": "elcr",
                    "type": "uint8",
                    "size": 1
                }
            ],
            "subsections": [
                {
                    "vmsd_name": "i8259/ltim",
                    "version": 1,
                    "fields": [
                        {
                            "name": "ltim",
                            "type": "uint8",
                            "size": 1
                        }
                    ]
                }
            ]
        },
        {
            "name": "i8259",
            "instance_id": 1,
            "vmsd_name": "i8259",
            "version": 1,
            "fields": [
                {
                    "name": "last_irr",
                    "type": "uint8",
                    "size": 1
                },

A migration file saved from the qemu-system-x86_64 default pc machine also 
lacks the subsection so it should not affect migration there unless 
something sets the bit. Is this enough testing or should I try something 
else? I've updated the commit message but did not change the comment.

Regards,
BALATON Zoltan