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

[Peter Maydell]

On Sat, 12 Feb 2022 at 17:02, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sat, 12 Feb 2022, Peter Maydell wrote:
> > 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.)
>
> I'm not sure I fully get this. Currently (before this series) PIIX4State
> does not seem to have any fields to store irq state (in hw/isa/piix4.c):
>
> struct PIIX4State {
>      PCIDevice dev;
>      qemu_irq cpu_intr;
>      qemu_irq *isa;
>
>      RTCState rtc;
>      /* Reset Control Register */
>      MemoryRegion rcr_mem;
>      uint8_t rcr;
> };
>
> Patch 1 in this series introduces that by moving it from MaltaState. Then
> we could have a patch 2 to clean it up and change to the way piix3 does it
> and skip introducing the saving of this array into the migration state. It
> may still break migration but not sure if MaltaState was saved already so
> this may be already missing from migration anyway and if we do the same as
> piix3 then maybe we don't need to change the piix4 state either (as this
> is saved as part of the PCIHost state?) but I don't know much about this
> so maybe I'm wrong.

Yeah, that would work -- we weren't saving the old gt64xxx_pci.c
pci_irq_levels[] global, so we don't break anything that wasn't
already broken by not putting the newly-introduced PIIX4State
array into migration state. Then when we do the equivalent of
e735b55a8c11 the array can just be deleted again. (We can't
conveniently switch to using pci_bus_get_irq_level() before doing
patch 1 of this series, because we need the pointer to the
piix4 device object and gt64120_pci_set_irq() is only passed
a pointer directly to a qemu_irq array.)

> In any case PIIX3 and PIIX4 state seem to be different so there's no
> reason to worry aobut compatibility between them.

Yep, that's right. The only reasons to copy changes from piix3
are (a) because they're worth having in themselves and (b)
because having the two devices be the same is maybe less
confusing. (b)'s a pretty weak reason, and (a) depends on
the individual change. In this case I think making the equivalent
of e735b55a8c11 is worthwhile because it saves us having an
extra array field and migrating it, and the change is pretty
small. For ab431c283e7055bcd you could argue either way -- it's
clearly a better way to structure the irq handling, but it's only
an optimisation, not a bug fix.

-- PMM