| Message ID | 20220212113519.69527-5-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] malta: Move PCI interrupt handling from gt64xxx to piix4 | expand |
On Sat, 12 Feb 2022, Bernhard Beschow wrote: > Now that pci_irq_levels[] is part of PIIX4State, the PCI IRQ levels can > be saved and restored via the VMState mechanism. This fixes migrated VMs > to start with PCI IRQ levels zeroed, which is a bug. > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/piix4.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c > index 964e09cf7f..a9322851db 100644 > --- a/hw/isa/piix4.c > +++ b/hw/isa/piix4.c > @@ -45,7 +45,7 @@ struct PIIX4State { > qemu_irq *isa; > qemu_irq i8259[ISA_NUM_IRQS]; > > - int pci_irq_levels[PIIX_NUM_PIRQS]; > + int32_t pci_irq_levels[PIIX_NUM_PIRQS]; Do you really need 32 bits to store a value of 0 or 1? I saw piix3 has that too but do we need to do the same here? Could this be just an uint8_t array? That's still an overkill to store 4 bits of information but less so than with int32_t. 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. Regards, BALATON Zoltan > > RTCState rtc; > /* Reset Control Register */ > @@ -128,12 +128,14 @@ static int piix4_ide_post_load(void *opaque, int version_id) > > static const VMStateDescription vmstate_piix4 = { > .name = "PIIX4", > - .version_id = 3, > + .version_id = 4, > .minimum_version_id = 2, > .post_load = piix4_ide_post_load, > .fields = (VMStateField[]) { > VMSTATE_PCI_DEVICE(dev, PIIX4State), > VMSTATE_UINT8_V(rcr, PIIX4State, 3), > + VMSTATE_INT32_ARRAY_V(pci_irq_levels, PIIX4State, > + PIIX_NUM_PIRQS, 4), > VMSTATE_END_OF_LIST() > } > }; >
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
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. In any case PIIX3 and PIIX4 state seem to be different so there's no reason to worry aobut compatibility between them. It's just confusing that there's a common piix.h which defines a PIIXState that looks like it could be common but maybe it's not used by PIIX4 but only by PIIX3 or I've missed something as I've only looked at this briefly. Regards, BALATON Zoltan
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
Am 12. Februar 2022 19:30:43 MEZ schrieb Peter Maydell <peter.maydell@linaro.org>: >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. e735b55a8c11 seems like a very elegant way for fixing migration of the IRQ levels. I'll have a look. Regards, Bernhard > >-- PMM
Am 12. Februar 2022 19:30:43 MEZ schrieb Peter Maydell <peter.maydell@linaro.org>: >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. e735b55a8c11 seems like a very elegant way for fixing migration of the IRQ levels. I'll have a look. Regards, Bernhard > >-- PMM
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c index 964e09cf7f..a9322851db 100644 --- a/hw/isa/piix4.c +++ b/hw/isa/piix4.c @@ -45,7 +45,7 @@ struct PIIX4State { qemu_irq *isa; qemu_irq i8259[ISA_NUM_IRQS]; - int pci_irq_levels[PIIX_NUM_PIRQS]; + int32_t pci_irq_levels[PIIX_NUM_PIRQS]; RTCState rtc; /* Reset Control Register */ @@ -128,12 +128,14 @@ static int piix4_ide_post_load(void *opaque, int version_id) static const VMStateDescription vmstate_piix4 = { .name = "PIIX4", - .version_id = 3, + .version_id = 4, .minimum_version_id = 2, .post_load = piix4_ide_post_load, .fields = (VMStateField[]) { VMSTATE_PCI_DEVICE(dev, PIIX4State), VMSTATE_UINT8_V(rcr, PIIX4State, 3), + VMSTATE_INT32_ARRAY_V(pci_irq_levels, PIIX4State, + PIIX_NUM_PIRQS, 4), VMSTATE_END_OF_LIST() } };
Now that pci_irq_levels[] is part of PIIX4State, the PCI IRQ levels can be saved and restored via the VMState mechanism. This fixes migrated VMs to start with PCI IRQ levels zeroed, which is a bug. Suggested-by: Peter Maydell <peter.maydell@linaro.org> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/piix4.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)