| Message ID | 20230302224058.43315-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/ide: Untangle ISA/PCI abuses of ide_init_ioport() | expand |
On Thu, Mar 02, 2023 at 11:40:40PM +0100, Philippe Mathieu-Daudé wrote: > Since v2: rebased > > I'm posting this series as it to not block Bernhard's PIIX > cleanup work. I don't have code change planned, but eventually > reword / improve commit descriptions. > > Tested commit after commit to be sure it is bisectable. Sadly > this was before Zoltan & Thomas report a problem with commit > bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). > > Background thread: > https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/ Acked-by: Michael S. Tsirkin <mst@redhat.com> who's merging this you? I am unsure about interdependencies between all these patchsets at this point. > Philippe Mathieu-Daudé (18): > hw/ide/piix: Expose output IRQ as properties for late object > population > hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function > hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15 > hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15 > hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa > hw/ide/piix: Ensure IDE output IRQs are wired at realization > hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq() > hw/ide: Introduce generic ide_init_ioport() > hw/ide/piix: Use generic ide_bus_init_ioport() > hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device > hw/isa: Simplify isa_address_space[_io]() > hw/isa: Reduce 'isabus' singleton scope to isa_bus_new() > exec/ioport: Factor portio_list_register_flush_coalesced() out > exec/ioport: Factor portio_list_register() out > hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro > hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro > hw/isa/piix: Unify QOM type name of PIIX ISA function > hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases > > hw/audio/adlib.c | 4 +-- > hw/display/qxl.c | 7 ++-- > hw/display/vga.c | 9 +++-- > hw/dma/i82374.c | 7 ++-- > hw/i386/pc_piix.c | 13 +++++--- > hw/ide/ioport.c | 15 +++++++-- > hw/ide/isa.c | 2 +- > hw/ide/piix.c | 54 +++++++++++++++++++++++------- > hw/isa/isa-bus.c | 36 ++++++++------------ > hw/isa/piix3.c | 63 +++++++++++++++-------------------- > hw/isa/piix4.c | 12 ++++--- > hw/mips/malta.c | 2 +- > hw/watchdog/wdt_ib700.c | 4 +-- > include/exec/ioport.h | 15 +++++---- > include/hw/ide/internal.h | 3 +- > include/hw/ide/isa.h | 3 ++ > include/hw/ide/piix.h | 4 +++ > include/hw/isa/isa.h | 3 +- > include/hw/southbridge/piix.h | 14 ++++---- > softmmu/ioport.c | 48 +++++++++++++++++++------- > softmmu/qdev-monitor.c | 3 ++ > 21 files changed, 190 insertions(+), 131 deletions(-) > > -- > 2.38.1 > > >
On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >Since v2: rebased > >I'm posting this series as it to not block Bernhard's PIIX >cleanup work. I don't have code change planned, but eventually >reword / improve commit descriptions. > >Tested commit after commit to be sure it is bisectable. Sadly >this was before Zoltan & Thomas report a problem with commit >bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). However much I stare at the partial revert which fixes it, I just cannot believe that the change could make any difference at all. There's got to be something weird going on there. I was going to ask if the level mode for the PIT made any difference, but this is the output IRQ from the PIT to the CPU itself so I don't see how it would. Would like to see a report with tracing from pic_update_irq, the CPU interrupt "handler" and the intermediate IRQ handler. With the intermediate present and without it. To compare the two.
On 03/03/2023 06:58, David Woodhouse wrote: > On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >> Since v2: rebased >> >> I'm posting this series as it to not block Bernhard's PIIX >> cleanup work. I don't have code change planned, but eventually >> reword / improve commit descriptions. >> >> Tested commit after commit to be sure it is bisectable. Sadly >> this was before Zoltan & Thomas report a problem with commit >> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). > > However much I stare at the partial revert which fixes it, I just cannot believe that the change could make any difference at all. There's got to be something weird going on there. > > I was going to ask if the level mode for the PIT made any difference, but this is the output IRQ from the PIT to the CPU itself so I don't see how it would. > > Would like to see a report with tracing from pic_update_irq, the CPU interrupt "handler" and the intermediate IRQ handler. With the intermediate present and without it. To compare the two. I suspect it's related to the removal of the allocation of the qemu_irq: qdev gpios work by adding a child IRQ object to the device, so it could be possible that something in the gpio internals isn't being updated correctly when the value is overwritten directly. Is the problem picked up when running a binary built with --enable-sanitizers? That's normally quite good at detecting this kind of issue. ATB, Mark.
On Fri, 3 Mar 2023, David Woodhouse wrote: > On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >> Since v2: rebased >> >> I'm posting this series as it to not block Bernhard's PIIX >> cleanup work. I don't have code change planned, but eventually >> reword / improve commit descriptions. >> >> Tested commit after commit to be sure it is bisectable. Sadly >> this was before Zoltan & Thomas report a problem with commit >> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). > > However much I stare at the partial revert which fixes it, I just cannot > believe that the change could make any difference at all. There's got to > be something weird going on there. > > I was going to ask if the level mode for the PIT made any difference, > but this is the output IRQ from the PIT to the CPU itself so I don't see > how it would. This is independent of the ltim patch and I've found this even before you've sent that patch as this is in my v5 series which you've replied to. I've found this problem before when I've first written this model back in 2019 and did not understand why it's needed but as now shown also with the prep machine there's some other problem somewhere that makes this necessary. As the way we had before works for the last few years reverting it is the safest bet now but we can try to find out and clean up eventually. > Would like to see a report with tracing from pic_update_irq, the CPU > interrupt "handler" and the intermediate IRQ handler. With the > intermediate present and without it. To compare the two. I'll try to collect such trace when I'll have time but if you want to experiment debian 8.11.0 netinstall cd should boot either with -kernel or with the -bios pegasos2.rom (type boot cd install/pegasos at the ok prompt in that case but the rom needs to be extracted from an updater as it's not freely distributable). I think Thomas Huth also reproduced the same with prep or 40p firmware after a similar change on that machine now. In any case it's unrelated to level sensitive mode which is only needed by MorphOS on pegasos2 to fix simultaneous interrupts e.g.with sound and USB or PCI cards which all share IRQ9 on that machine. Other guests don't even enable that ltim bit so it should not affect anything else. Regards, BALATON Zoltan
On 3/3/23 08:46, Mark Cave-Ayland wrote: > On 03/03/2023 06:58, David Woodhouse wrote: > >> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" >> <philmd@linaro.org> wrote: >>> Since v2: rebased >>> >>> I'm posting this series as it to not block Bernhard's PIIX >>> cleanup work. I don't have code change planned, but eventually >>> reword / improve commit descriptions. >>> >>> Tested commit after commit to be sure it is bisectable. Sadly >>> this was before Zoltan & Thomas report a problem with commit >>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). >> >> However much I stare at the partial revert which fixes it, I just >> cannot believe that the change could make any difference at all. >> There's got to be something weird going on there. >> >> I was going to ask if the level mode for the PIT made any difference, >> but this is the output IRQ from the PIT to the CPU itself so I don't >> see how it would. >> >> Would like to see a report with tracing from pic_update_irq, the CPU >> interrupt "handler" and the intermediate IRQ handler. With the >> intermediate present and without it. To compare the two. > > I suspect it's related to the removal of the allocation of the qemu_irq: > qdev gpios work by adding a child IRQ object to the device, so it could > be possible that something in the gpio internals isn't being updated > correctly when the value is overwritten directly. > > Is the problem picked up when running a binary built with > --enable-sanitizers? That's normally quite good at detecting this kind > of issue. No ASan warning. However I see (before/after bb98e0f59c): qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 00) qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 84) qemu-system-ppc: pc87312: unsupported device reconfiguration (09 01 84)
On Fri, 3 Mar 2023, Philippe Mathieu-Daudé wrote: > On 3/3/23 08:46, Mark Cave-Ayland wrote: >> On 03/03/2023 06:58, David Woodhouse wrote: >> >>> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> >>> wrote: >>>> Since v2: rebased >>>> >>>> I'm posting this series as it to not block Bernhard's PIIX >>>> cleanup work. I don't have code change planned, but eventually >>>> reword / improve commit descriptions. >>>> >>>> Tested commit after commit to be sure it is bisectable. Sadly >>>> this was before Zoltan & Thomas report a problem with commit >>>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). >>> >>> However much I stare at the partial revert which fixes it, I just cannot >>> believe that the change could make any difference at all. There's got to >>> be something weird going on there. >>> >>> I was going to ask if the level mode for the PIT made any difference, but >>> this is the output IRQ from the PIT to the CPU itself so I don't see how >>> it would. >>> >>> Would like to see a report with tracing from pic_update_irq, the CPU >>> interrupt "handler" and the intermediate IRQ handler. With the >>> intermediate present and without it. To compare the two. >> >> I suspect it's related to the removal of the allocation of the qemu_irq: >> qdev gpios work by adding a child IRQ object to the device, so it could be >> possible that something in the gpio internals isn't being updated correctly >> when the value is overwritten directly. >> >> Is the problem picked up when running a binary built with >> --enable-sanitizers? That's normally quite good at detecting this kind of >> issue. > > No ASan warning. However I see (before/after bb98e0f59c): > > qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 00) > qemu-system-ppc: pc87312: unsupported device reconfiguration (0f 11 84) > qemu-system-ppc: pc87312: unsupported device reconfiguration (09 01 84) This does not seem related at all especially if you also see it before because we have the same problem in vt82c686 and this error above rather looks liek it should be a qemu_log_mask(LOG_UNIMP) as that's all that function seems to do where this is printed so looks like it's just unimplemented functionality. Regards, BALATON Zoltan
On Fri, 3 Mar 2023, David Woodhouse wrote: > Would like to see a report with tracing from pic_update_irq, the CPU > interrupt "handler" and the intermediate IRQ handler. With the > intermediate present and without it. To compare the two. Here it is witout revert when it hangs after printing: 0.536| Memory used before SYS_Init: 9MB 0.606| 0.606| 0.606| ABox 1.30 (2.7.2018) © 1999-2022 by Ralph Schmidt, Emmanuel Lesueur, Teemu Suikki, Harry Sintonen 1.257| PCI ATA/ATAPI Driver@2: PIO Mode 4 1.257| PCI ATA/ATAPI Driver@2: UDMA Mode 5 U pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 D pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 M pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 A pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 M pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 o pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 d pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 e pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 5 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 [more of the above repeating some more then] pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 16 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 144 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 144 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 0 imr 45 irr 144 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 pic_update_irq master 1 imr 248 irr 7 padd 0 and there seems to be no more interrupts with the revert when it boots this should print: 1.513| PCI ATA/ATAPI Driver@2: PIO Mode 4 1.514| PCI ATA/ATAPI Driver@2: UDMA Mode 5 1.517| ide.device@2: QEMU QEMU DVD-ROM <CDROM> 1.523| ide.device@2: CDRom <CD001>,<MORPHOS > found, bootable 1.525| ide.device@2: Mount <CD0> 1.526| ide.device@2: Partition <CD0> DosType 0x43444653 BootPri 127 and the trace including the irq forwarder func in vt82c686 isa: M via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 o via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 d via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 e via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 5 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 . via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 2 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 8 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 2 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 | via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 i via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 d via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 e via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 . via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 d via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 e via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 v via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 i via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 c via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 e via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 @ via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 2 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 : via_isa_request_i8259_irq: irq=0 level=0 [...] < via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 C via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 D via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 R via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 O via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 M via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 > via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 pic_update_irq master 0 imr 45 irr 128 padd 0 pic_update_irq master 1 imr 248 irr 4 padd 1 via_isa_request_i8259_irq: irq=0 level=1 via_isa_request_i8259_irq: irq=0 level=0 via_isa_request_i8259_irq: irq=0 level=0 and so on. Neither of the above had your ltim patch applied yet. Regards, BALATON Zoltan
Am 3. März 2023 07:46:31 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >On 03/03/2023 06:58, David Woodhouse wrote: > >> On 2 March 2023 22:40:40 GMT, "Philippe Mathieu-Daudé" <philmd@linaro.org> wrote: >>> Since v2: rebased >>> >>> I'm posting this series as it to not block Bernhard's PIIX >>> cleanup work. I don't have code change planned, but eventually >>> reword / improve commit descriptions. >>> >>> Tested commit after commit to be sure it is bisectable. Sadly >>> this was before Zoltan & Thomas report a problem with commit >>> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). >> >> However much I stare at the partial revert which fixes it, I just cannot believe that the change could make any difference at all. There's got to be something weird going on there. >> >> I was going to ask if the level mode for the PIT made any difference, but this is the output IRQ from the PIT to the CPU itself so I don't see how it would. >> >> Would like to see a report with tracing from pic_update_irq, the CPU interrupt "handler" and the intermediate IRQ handler. With the intermediate present and without it. To compare the two. > >I suspect it's related to the removal of the allocation of the qemu_irq: qdev gpios work by adding a child IRQ object to the device, so it could be possible that something in the gpio internals isn't being updated correctly when the value is overwritten directly. I've just sent a series fixing the issue. The problem was that cpu_intr gets populated by qdev_connect_gpio_out() in board code which happens after via's realize method has been executed. So in via's realize method cpu_intr is still NULL which causes a NULL qemu_irq to be passed to the i8259. One way to fix this is to move qdev_connect_gpio_out() in board code between pci_new_multifunction() and pci_realize_and_unref(). By having an intermediate IRQ handler the problem didn't appear since the (non-NULL) qemu_irq holding the intermediate handler is passed to the i8259. The intermediate handler delays reading cpu_intr to runtime, so initializing it after realize() is no problem. The price, however, is that an indirection occurs at runtime every time cpu_intr is triggered. BTW, the PIC proxy in my PIIX consolidation series attempted to solve the same problem: The ISABus IRQs need to be already populated in piix-ide's realize method, otherwise NULL qemu_irqs are used. As long as piix-ide is realized in board code, separately from the piix south bridge, the ISABus IRQs can be populated in between. However, once piix-ide is realized in the south bridge, the ISA IRQs must be populated before the south bridge's realize(). The PIC proxy solved this by introducing intermediate ISA IRQs while the latest incarnation of the PIIX consolidation series uses the same approach as described above. Best regards, Bernhard > >Is the problem picked up when running a binary built with --enable-sanitizers? That's normally quite good at detecting this kind of issue. > > >ATB, > >Mark.
On Thu, Mar 02, 2023 at 11:40:40PM +0100, Philippe Mathieu-Daudé wrote: > Since v2: rebased > > I'm posting this series as it to not block Bernhard's PIIX > cleanup work. I don't have code change planned, but eventually > reword / improve commit descriptions. > Tested commit after commit to be sure it is bisectable. Sadly > this was before Zoltan & Thomas report a problem with commit > bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). I'm not sure what this implies, or how do you want to resolve the conflicts with Bernhard's work. did my best to review, series: Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Background thread: > https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/ > > Philippe Mathieu-Daudé (18): > hw/ide/piix: Expose output IRQ as properties for late object > population > hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function > hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15 > hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15 > hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa > hw/ide/piix: Ensure IDE output IRQs are wired at realization > hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq() > hw/ide: Introduce generic ide_init_ioport() > hw/ide/piix: Use generic ide_bus_init_ioport() > hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device > hw/isa: Simplify isa_address_space[_io]() > hw/isa: Reduce 'isabus' singleton scope to isa_bus_new() > exec/ioport: Factor portio_list_register_flush_coalesced() out > exec/ioport: Factor portio_list_register() out > hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro > hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro > hw/isa/piix: Unify QOM type name of PIIX ISA function > hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases > > hw/audio/adlib.c | 4 +-- > hw/display/qxl.c | 7 ++-- > hw/display/vga.c | 9 +++-- > hw/dma/i82374.c | 7 ++-- > hw/i386/pc_piix.c | 13 +++++--- > hw/ide/ioport.c | 15 +++++++-- > hw/ide/isa.c | 2 +- > hw/ide/piix.c | 54 +++++++++++++++++++++++------- > hw/isa/isa-bus.c | 36 ++++++++------------ > hw/isa/piix3.c | 63 +++++++++++++++-------------------- > hw/isa/piix4.c | 12 ++++--- > hw/mips/malta.c | 2 +- > hw/watchdog/wdt_ib700.c | 4 +-- > include/exec/ioport.h | 15 +++++---- > include/hw/ide/internal.h | 3 +- > include/hw/ide/isa.h | 3 ++ > include/hw/ide/piix.h | 4 +++ > include/hw/isa/isa.h | 3 +- > include/hw/southbridge/piix.h | 14 ++++---- > softmmu/ioport.c | 48 +++++++++++++++++++------- > softmmu/qdev-monitor.c | 3 ++ > 21 files changed, 190 insertions(+), 131 deletions(-) > > -- > 2.38.1 > > >
Am 2. März 2023 22:40:40 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >Since v2: rebased > >I'm posting this series as it to not block Bernhard's PIIX >cleanup work. I don't have code change planned, but eventually >reword / improve commit descriptions. > >Tested commit after commit to be sure it is bisectable. Sadly >this was before Zoltan & Thomas report a problem with commit >bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). > >Background thread: >https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/ Hi, I've just sent yet another proposal which might make some renamings done in this series appear unneccessary. Best regards, Bernhard > >Philippe Mathieu-Daudé (18): > hw/ide/piix: Expose output IRQ as properties for late object > population > hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function > hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15 > hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15 > hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa > hw/ide/piix: Ensure IDE output IRQs are wired at realization > hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq() > hw/ide: Introduce generic ide_init_ioport() > hw/ide/piix: Use generic ide_bus_init_ioport() > hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device > hw/isa: Simplify isa_address_space[_io]() > hw/isa: Reduce 'isabus' singleton scope to isa_bus_new() > exec/ioport: Factor portio_list_register_flush_coalesced() out > exec/ioport: Factor portio_list_register() out > hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro > hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro > hw/isa/piix: Unify QOM type name of PIIX ISA function > hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases > > hw/audio/adlib.c | 4 +-- > hw/display/qxl.c | 7 ++-- > hw/display/vga.c | 9 +++-- > hw/dma/i82374.c | 7 ++-- > hw/i386/pc_piix.c | 13 +++++--- > hw/ide/ioport.c | 15 +++++++-- > hw/ide/isa.c | 2 +- > hw/ide/piix.c | 54 +++++++++++++++++++++++------- > hw/isa/isa-bus.c | 36 ++++++++------------ > hw/isa/piix3.c | 63 +++++++++++++++-------------------- > hw/isa/piix4.c | 12 ++++--- > hw/mips/malta.c | 2 +- > hw/watchdog/wdt_ib700.c | 4 +-- > include/exec/ioport.h | 15 +++++---- > include/hw/ide/internal.h | 3 +- > include/hw/ide/isa.h | 3 ++ > include/hw/ide/piix.h | 4 +++ > include/hw/isa/isa.h | 3 +- > include/hw/southbridge/piix.h | 14 ++++---- > softmmu/ioport.c | 48 +++++++++++++++++++------- > softmmu/qdev-monitor.c | 3 ++ > 21 files changed, 190 insertions(+), 131 deletions(-) >
On 21/04/2023 09:25, Michael S. Tsirkin wrote: > On Thu, Mar 02, 2023 at 11:40:40PM +0100, Philippe Mathieu-Daudé wrote: >> Since v2: rebased >> >> I'm posting this series as it to not block Bernhard's PIIX >> cleanup work. I don't have code change planned, but eventually >> reword / improve commit descriptions. > >> Tested commit after commit to be sure it is bisectable. Sadly >> this was before Zoltan & Thomas report a problem with commit >> bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). > > I'm not sure what this implies, or how do you want to > resolve the conflicts with Bernhard's work. > > did my best to review, series: > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Phil: I revisited this series after our discussion earlier in the week re: device IRQs, and I realised that there is some overlap with this series, Bernhard's latest series, and some of my local patches for switching between legacy/native PCI IDE IRQ routing. I've now gone through both series and replied where I think we should now be going with this, but I can see that the final series will likely pull from all 3 sources. How do you think we should best move forward from here? Once you've replied to the various comments I can potentially pull everything together into an updated series if that helps? >> Background thread: >> https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/ >> >> Philippe Mathieu-Daudé (18): >> hw/ide/piix: Expose output IRQ as properties for late object >> population >> hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function >> hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15 >> hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15 >> hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa >> hw/ide/piix: Ensure IDE output IRQs are wired at realization >> hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq() >> hw/ide: Introduce generic ide_init_ioport() >> hw/ide/piix: Use generic ide_bus_init_ioport() >> hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device >> hw/isa: Simplify isa_address_space[_io]() >> hw/isa: Reduce 'isabus' singleton scope to isa_bus_new() >> exec/ioport: Factor portio_list_register_flush_coalesced() out >> exec/ioport: Factor portio_list_register() out >> hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro >> hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro >> hw/isa/piix: Unify QOM type name of PIIX ISA function >> hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases >> >> hw/audio/adlib.c | 4 +-- >> hw/display/qxl.c | 7 ++-- >> hw/display/vga.c | 9 +++-- >> hw/dma/i82374.c | 7 ++-- >> hw/i386/pc_piix.c | 13 +++++--- >> hw/ide/ioport.c | 15 +++++++-- >> hw/ide/isa.c | 2 +- >> hw/ide/piix.c | 54 +++++++++++++++++++++++------- >> hw/isa/isa-bus.c | 36 ++++++++------------ >> hw/isa/piix3.c | 63 +++++++++++++++-------------------- >> hw/isa/piix4.c | 12 ++++--- >> hw/mips/malta.c | 2 +- >> hw/watchdog/wdt_ib700.c | 4 +-- >> include/exec/ioport.h | 15 +++++---- >> include/hw/ide/internal.h | 3 +- >> include/hw/ide/isa.h | 3 ++ >> include/hw/ide/piix.h | 4 +++ >> include/hw/isa/isa.h | 3 +- >> include/hw/southbridge/piix.h | 14 ++++---- >> softmmu/ioport.c | 48 +++++++++++++++++++------- >> softmmu/qdev-monitor.c | 3 ++ >> 21 files changed, 190 insertions(+), 131 deletions(-) >> >> -- >> 2.38.1 ATB, Mark.
Since v2: rebased I'm posting this series as it to not block Bernhard's PIIX cleanup work. I don't have code change planned, but eventually reword / improve commit descriptions. Tested commit after commit to be sure it is bisectable. Sadly this was before Zoltan & Thomas report a problem with commit bb98e0f59c ("hw/isa/vt82c686: Remove intermediate IRQ forwarder"). Background thread: https://lore.kernel.org/qemu-devel/5095dffc-309b-6c72-d255-8cdaa6fd3d52@ilande.co.uk/ Philippe Mathieu-Daudé (18): hw/ide/piix: Expose output IRQ as properties for late object population hw/ide/piix: Allow using PIIX3-IDE as standalone PCI function hw/i386/pc_piix: Wire PIIX3 IDE ouput IRQs to ISA bus IRQs 14/15 hw/isa/piix4: Wire PIIX4 IDE ouput IRQs to ISA bus IRQs 14/15 hw/ide: Rename ISA specific ide_init_ioport -> ide_bus_init_ioport_isa hw/ide/piix: Ensure IDE output IRQs are wired at realization hw/isa: Deprecate isa_get_irq() in favor of isa_bus_get_irq() hw/ide: Introduce generic ide_init_ioport() hw/ide/piix: Use generic ide_bus_init_ioport() hw/isa: Ensure isa_register_portio_list() do not get NULL ISA device hw/isa: Simplify isa_address_space[_io]() hw/isa: Reduce 'isabus' singleton scope to isa_bus_new() exec/ioport: Factor portio_list_register_flush_coalesced() out exec/ioport: Factor portio_list_register() out hw/southbridge/piix: Use OBJECT_DECLARE_SIMPLE_TYPE() macro hw/isa/piix: Batch register QOM types using DEFINE_TYPES() macro hw/isa/piix: Unify QOM type name of PIIX ISA function hw/isa/piix: Unify PIIX-ISA QOM type names using qdev aliases hw/audio/adlib.c | 4 +-- hw/display/qxl.c | 7 ++-- hw/display/vga.c | 9 +++-- hw/dma/i82374.c | 7 ++-- hw/i386/pc_piix.c | 13 +++++--- hw/ide/ioport.c | 15 +++++++-- hw/ide/isa.c | 2 +- hw/ide/piix.c | 54 +++++++++++++++++++++++------- hw/isa/isa-bus.c | 36 ++++++++------------ hw/isa/piix3.c | 63 +++++++++++++++-------------------- hw/isa/piix4.c | 12 ++++--- hw/mips/malta.c | 2 +- hw/watchdog/wdt_ib700.c | 4 +-- include/exec/ioport.h | 15 +++++---- include/hw/ide/internal.h | 3 +- include/hw/ide/isa.h | 3 ++ include/hw/ide/piix.h | 4 +++ include/hw/isa/isa.h | 3 +- include/hw/southbridge/piix.h | 14 ++++---- softmmu/ioport.c | 48 +++++++++++++++++++------- softmmu/qdev-monitor.c | 3 ++ 21 files changed, 190 insertions(+), 131 deletions(-)