| Message ID | cd0b323bb88df202e36014f950c0eb13a9fafd54.1677628524.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Pegasos2 fixes and audio output support | expand |
Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >MorphOS sets the ISA PIC to level sensitive mode but QEMU does not >support that so this causes a freeze if multiple devices try to raise >a shared interrupt. Work around it by lowering the interrupt before >raising it again if it is already raised. This could be reverted when >the i8259 model is fixed. > >Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >--- > hw/isa/vt82c686.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > >diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >index 018a119964..3e44a51f92 100644 >--- a/hw/isa/vt82c686.c >+++ b/hw/isa/vt82c686.c >@@ -549,6 +549,7 @@ struct ViaISAState { > PCIDevice dev; > qemu_irq cpu_intr; > qemu_irq *isa_irqs_in; >+ uint16_t isa_irqs_state; > ViaSuperIOState via_sio; > MC146818RtcState rtc; > PCIIDEState ide; >@@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) > pic_level |= pci_bus_get_irq_level(bus, i); > } > } >+ /* FIXME: workaround for i8259: level sensitive irq not supported */ >+ if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) { >+ qemu_irq_lower(s->isa_irqs_in[pic_irq]); >+ } else if (pic_level) { >+ s->isa_irqs_state |= BIT(pic_irq); >+ } else { >+ s->isa_irqs_state &= ~BIT(pic_irq); >+ } Let's not clutter the device model with workarounds which quickly snowball into unmaintainable code. Please fix the i8259 instead. > /* Now we change the pic irq level according to the via irq mappings. */ > qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level); > }
On Wed, 1 Mar 2023, Bernhard Beschow wrote: > Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> MorphOS sets the ISA PIC to level sensitive mode but QEMU does not >> support that so this causes a freeze if multiple devices try to raise >> a shared interrupt. Work around it by lowering the interrupt before >> raising it again if it is already raised. This could be reverted when >> the i8259 model is fixed. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/isa/vt82c686.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 018a119964..3e44a51f92 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -549,6 +549,7 @@ struct ViaISAState { >> PCIDevice dev; >> qemu_irq cpu_intr; >> qemu_irq *isa_irqs_in; >> + uint16_t isa_irqs_state; >> ViaSuperIOState via_sio; >> MC146818RtcState rtc; >> PCIIDEState ide; >> @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) >> pic_level |= pci_bus_get_irq_level(bus, i); >> } >> } >> + /* FIXME: workaround for i8259: level sensitive irq not supported */ >> + if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) { >> + qemu_irq_lower(s->isa_irqs_in[pic_irq]); >> + } else if (pic_level) { >> + s->isa_irqs_state |= BIT(pic_irq); >> + } else { >> + s->isa_irqs_state &= ~BIT(pic_irq); >> + } > > Let's not clutter the device model with workarounds which quickly snowball into unmaintainable code. Please fix the i8259 instead. Do you have an idea how? I don't know PC hardware well so it's not likely I can do that in one day and breaking PIC model would affect a lot of machines among them some production critical ones. Adding this workaround here only affects pegasos2 and the only partially modeled likely not used fuloong2e board (which is mostly there because it was there before and good to keep it to be able to test this device model with another machine) and I can test these two but not all the others using i8259. So I think for this release this is the best we can do and if somebody more knowledgeable about PC hardware fixes the i8259 PIC model later this can be easily reverted. It's not a big clutter so unless you can show this breaks something I'd rather have it to keep MorphOS usable on pegasos2 with sound and network or USB. If you can prove this breaks something we could drop it but not based on opinion or feelengs only some evidence. Regards, BALATON Zoltan >> /* Now we change the pic irq level according to the via irq mappings. */ >> qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level); >> } > >
On Wed, 2023-03-01 at 12:27 +0100, BALATON Zoltan wrote: > On Wed, 1 Mar 2023, Bernhard Beschow wrote: > > Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: > > > MorphOS sets the ISA PIC to level sensitive mode but QEMU does > > > not > > > support that so this causes a freeze if multiple devices try to > > > raise > > > a shared interrupt. Work around it by lowering the interrupt > > > before > > > raising it again if it is already raised. This could be reverted > > > when > > > the i8259 model is fixed. > > > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > > --- > > > hw/isa/vt82c686.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > > > index 018a119964..3e44a51f92 100644 > > > --- a/hw/isa/vt82c686.c > > > +++ b/hw/isa/vt82c686.c > > > @@ -549,6 +549,7 @@ struct ViaISAState { > > > PCIDevice dev; > > > qemu_irq cpu_intr; > > > qemu_irq *isa_irqs_in; > > > + uint16_t isa_irqs_state; > > > ViaSuperIOState via_sio; > > > MC146818RtcState rtc; > > > PCIIDEState ide; > > > @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void > > > *opaque, int irq_num, int level) > > > pic_level |= pci_bus_get_irq_level(bus, i); > > > } > > > } > > > + /* FIXME: workaround for i8259: level sensitive irq not > > > supported */ > > > + if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) { > > > + qemu_irq_lower(s->isa_irqs_in[pic_irq]); > > > + } else if (pic_level) { > > > + s->isa_irqs_state |= BIT(pic_irq); > > > + } else { > > > + s->isa_irqs_state &= ~BIT(pic_irq); > > > + } > > > > Let's not clutter the device model with workarounds which quickly > > snowball into unmaintainable code. Please fix the i8259 instead. > > Do you have an idea how? Let's start by understanding the problem completely. The i8259 *does* support level-triggered interrupts. Look at the checks for s->elcr in hw/intc/i8259.c, in pic_set_irq()... if (s->elcr & mask) { /* level triggered */ if (level) { s->irr |= mask; s->last_irr |= mask; } else { s->irr &= ~mask; s->last_irr &= ~mask; } } else { /* edge triggered */ ... and in pic_intack() ... /* We don't clear a level sensitive interrupt here */ if (!(s->elcr & (1 << irq))) { s->irr &= ~(1 << irq); } What qemu typically has an issue with is *shared* level-triggered interrupts. But that would cause a level to be "forgotten" if another input asserts and then deasserts the IRQ while one input thinks it's holding it asserted. And I don't see how your workaround above would have helped in that situation. Are you sure the PIC ELCR is actually set for the lines you're having trouble with? Is that something the Pegasos SmartFirmware would have done, and MorphOS is expecting to inherit but isn't actually setting up for itself?
On Wed, 1 Mar 2023, David Woodhouse wrote: > On Wed, 2023-03-01 at 12:27 +0100, BALATON Zoltan wrote: >> On Wed, 1 Mar 2023, Bernhard Beschow wrote: >>> Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >>>> MorphOS sets the ISA PIC to level sensitive mode but QEMU does >>>> not >>>> support that so this causes a freeze if multiple devices try to >>>> raise >>>> a shared interrupt. Work around it by lowering the interrupt >>>> before >>>> raising it again if it is already raised. This could be reverted >>>> when >>>> the i8259 model is fixed. >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> hw/isa/vt82c686.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>> index 018a119964..3e44a51f92 100644 >>>> --- a/hw/isa/vt82c686.c >>>> +++ b/hw/isa/vt82c686.c >>>> @@ -549,6 +549,7 @@ struct ViaISAState { >>>> PCIDevice dev; >>>> qemu_irq cpu_intr; >>>> qemu_irq *isa_irqs_in; >>>> + uint16_t isa_irqs_state; >>>> ViaSuperIOState via_sio; >>>> MC146818RtcState rtc; >>>> PCIIDEState ide; >>>> @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void >>>> *opaque, int irq_num, int level) >>>> pic_level |= pci_bus_get_irq_level(bus, i); >>>> } >>>> } >>>> + /* FIXME: workaround for i8259: level sensitive irq not >>>> supported */ >>>> + if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) { >>>> + qemu_irq_lower(s->isa_irqs_in[pic_irq]); >>>> + } else if (pic_level) { >>>> + s->isa_irqs_state |= BIT(pic_irq); >>>> + } else { >>>> + s->isa_irqs_state &= ~BIT(pic_irq); >>>> + } >>> >>> Let's not clutter the device model with workarounds which quickly >>> snowball into unmaintainable code. Please fix the i8259 instead. >> >> Do you have an idea how? > > Let's start by understanding the problem completely. The i8259 *does* > support level-triggered interrupts. Look at the checks for s->elcr in > hw/intc/i8259.c, in pic_set_irq()... > > if (s->elcr & mask) { > /* level triggered */ > if (level) { > s->irr |= mask; > s->last_irr |= mask; > } else { > s->irr &= ~mask; > s->last_irr &= ~mask; > } > } else { > /* edge triggered */ > > > ... and in pic_intack() ... > > > /* We don't clear a level sensitive interrupt here */ > if (!(s->elcr & (1 << irq))) { > s->irr &= ~(1 << irq); > } > > > What qemu typically has an issue with is *shared* level-triggered > interrupts. But that would cause a level to be "forgotten" if another > input asserts and then deasserts the IRQ while one input thinks it's > holding it asserted. And I don't see how your workaround above would > have helped in that situation. > > Are you sure the PIC ELCR is actually set for the lines you're having > trouble with? Is that something the Pegasos SmartFirmware would have > done, and MorphOS is expecting to inherit but isn't actually setting up > for itself? No, it works with other guests like Linux and AmigaOS that use PIC as set up by the firmware but MorphOS tries to use it in level sensitive mode and likely has an IRQ handler which expects this to work. This is where I've debugged it and came to this workaround: https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html When booting MorphOS with -d unimp I see these logs: i8259: level sensitive irq not supported i8259: level sensitive irq not supported which is I guess when it tries to set it for both PICs. (If you want to try this MorphOS iso is downloadable and instructions how to boot it is here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos Regards, BALATON Zoltan
On Wed, 2023-03-01 at 14:18 +0100, BALATON Zoltan wrote: > > Are you sure the PIC ELCR is actually set for the lines you're having > > trouble with? Is that something the Pegasos SmartFirmware would have > > done, and MorphOS is expecting to inherit but isn't actually setting up > > for itself? > > No, it works with other guests like Linux and AmigaOS that use PIC as set > up by the firmware but MorphOS tries to use it in level sensitive mode and > likely has an IRQ handler which expects this to work. This is where I've > debugged it and came to this workaround: > > https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html > > When booting MorphOS with -d unimp I see these logs: > > i8259: level sensitive irq not supported > i8259: level sensitive irq not supported > > which is I guess when it tries to set it for both PICs. (If you want to > try this MorphOS iso is downloadable and instructions how to boot it is > here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos Wow. Even looking at the PIIX3 datasheet from 1996, That 'Edge/Level Bank Select (LTIM)' bit was documented as 'This bit is disabled. Its function is replaced by the Edge/Level Triggerede Control (ELCR) Registers. We've been able to set the edge/level on a per-pin basis ever since the ELCR was introduced with the IBM PS/2, I think. 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; } } else if (val & 0x08) { if (val & 0x04) {
On Wed, 1 Mar 2023, David Woodhouse wrote: > On Wed, 2023-03-01 at 14:18 +0100, BALATON Zoltan wrote: >>> Are you sure the PIC ELCR is actually set for the lines you're having >>> trouble with? Is that something the Pegasos SmartFirmware would have >>> done, and MorphOS is expecting to inherit but isn't actually setting up >>> for itself? >> >> No, it works with other guests like Linux and AmigaOS that use PIC as set >> up by the firmware but MorphOS tries to use it in level sensitive mode and >> likely has an IRQ handler which expects this to work. This is where I've >> debugged it and came to this workaround: >> >> https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html >> >> When booting MorphOS with -d unimp I see these logs: >> >> i8259: level sensitive irq not supported >> i8259: level sensitive irq not supported >> >> which is I guess when it tries to set it for both PICs. (If you want to >> try this MorphOS iso is downloadable and instructions how to boot it is >> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos > > > Wow. Even looking at the PIIX3 datasheet from 1996, That 'Edge/Level > Bank Select (LTIM)' bit was documented as 'This bit is disabled. Its > function is replaced by the Edge/Level Triggerede Control (ELCR) > Registers. > > We've been able to set the edge/level on a per-pin basis ever since the > ELCR was introduced with the IBM PS/2, I think. > > 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. Regards, BALATON Zoltan > } > } else if (val & 0x08) { > if (val & 0x04) { > > > >
Am 1. März 2023 14:05:31 UTC schrieb David Woodhouse <dwmw2@infradead.org>: >On Wed, 2023-03-01 at 14:18 +0100, BALATON Zoltan wrote: >> > Are you sure the PIC ELCR is actually set for the lines you're having >> > trouble with? Is that something the Pegasos SmartFirmware would have >> > done, and MorphOS is expecting to inherit but isn't actually setting up >> > for itself? >> >> No, it works with other guests like Linux and AmigaOS that use PIC as set >> up by the firmware but MorphOS tries to use it in level sensitive mode and >> likely has an IRQ handler which expects this to work. This is where I've >> debugged it and came to this workaround: >> >> https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html >> >> When booting MorphOS with -d unimp I see these logs: >> >> i8259: level sensitive irq not supported >> i8259: level sensitive irq not supported >> >> which is I guess when it tries to set it for both PICs. (If you want to >> try this MorphOS iso is downloadable and instructions how to boot it is >> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos > > >Wow. Even looking at the PIIX3 datasheet from 1996, That 'Edge/Level >Bank Select (LTIM)' bit was documented as 'This bit is disabled. Its >function is replaced by the Edge/Level Triggerede Control (ELCR) >Registers. > >We've been able to set the edge/level on a per-pin basis ever since the >ELCR was introduced with the IBM PS/2, I think. > >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; Thanks so much, David! You're a genious... Best regards, Bernhard > } > } else if (val & 0x08) { > if (val & 0x04) { > > >
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 ... :) 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.
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. > >
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.
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
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 018a119964..3e44a51f92 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -549,6 +549,7 @@ struct ViaISAState { PCIDevice dev; qemu_irq cpu_intr; qemu_irq *isa_irqs_in; + uint16_t isa_irqs_state; ViaSuperIOState via_sio; MC146818RtcState rtc; PCIIDEState ide; @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) pic_level |= pci_bus_get_irq_level(bus, i); } } + /* FIXME: workaround for i8259: level sensitive irq not supported */ + if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) { + qemu_irq_lower(s->isa_irqs_in[pic_irq]); + } else if (pic_level) { + s->isa_irqs_state |= BIT(pic_irq); + } else { + s->isa_irqs_state &= ~BIT(pic_irq); + } /* Now we change the pic irq level according to the via irq mappings. */ qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level); }
MorphOS sets the ISA PIC to level sensitive mode but QEMU does not support that so this causes a freeze if multiple devices try to raise a shared interrupt. Work around it by lowering the interrupt before raising it again if it is already raised. This could be reverted when the i8259 model is fixed. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/isa/vt82c686.c | 9 +++++++++ 1 file changed, 9 insertions(+)