[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model
Date: Thu, 2 Mar 2023 22:46:21 +0100 (CET)

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) {
                               "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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]