[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: Wed, 1 Mar 2023 23:47:01 +0100 (CET)

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?

Thank you,

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)

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

reply via email to

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