Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[Peter Maydell]

On Sat, 12 Feb 2022 at 13:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> By the way the corresponding member in struct PIIXState in
> include/hw/southbridge/piix.h has a comment saying:
>
>      /* This member isn't used. Just for save/load compatibility */
>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>
> and only seems to be filled in piix3_pre_save() but never used. So what's
> the point of this then? Maybe piix3 also uses a bitmap to store these
> levels instead? There's a uint64_t pic_levels member above the unused
> array but I haven't checked the implementation.

I think what has happened here is that originally piix3 used
the same implementation piix4 currently does -- where it stores
locally the value of the (incoming) PCI IRQ levels, and then when it wants
to know the value of the (outgoing) PIC IRQ levels it loops round
to effectively OR together all the PCI IRQ levels for those PCI
IRQs which are configured to that particular PIC interrupt.

Then in commit e735b55a8c11 (in 2011) piix3 was changed to call
pci_bus_get_irq_level() to get the value of a PCI IRQ rather than
looking at its local cache of that information in the pci_irq_levels[]
array. This is the source of the "save/load compatibility" thing --
before doing a vmsave piix3_pre_save() fills in that field so that
it appears in the outbound data stream and thus a migration from
a new QEMU to an old pre-e735b55a8c11 QEMU will still work. (This
was important at the time, but could probably be cleaned up now.)

The next commit after that one is ab431c283e7055bcd, which
is an optimization that fixes the equivalent of the "XXX: optimize"
marker in the gt64120_pci_set_irq()/piix4 code -- this does
something slightly more complicated involving the pic_levels
field, in order to avoid having to do the "loop over all the
PCI IRQ levels" whenever it needs to set/reset a PIC interrupt.

We could pick up one or both (or none!) of these two changes
for the piix4 code. (If we're breaking migration compat anyway
we wouldn't need to include the save-load compat part of
the first change.)

thanks
-- PMM