| Message ID | 20220112213629.9126-4-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | malta: Move PCI interrupt handling from gt64xxx to piix4 | expand |
On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow <shentey@gmail.com> wrote: > > Now that piix4_set_irq's opaque parameter references own PIIX4State, > piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/piix4.c | 22 +++++++++------------- > include/hw/southbridge/piix.h | 2 -- > 2 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c > index a31e9714cf..964e09cf7f 100644 > --- a/hw/isa/piix4.c > +++ b/hw/isa/piix4.c > @@ -39,14 +39,14 @@ > #include "sysemu/runstate.h" > #include "qom/object.h" > > -PCIDevice *piix4_dev; > - > struct PIIX4State { > PCIDevice dev; > qemu_irq cpu_intr; > qemu_irq *isa; > qemu_irq i8259[ISA_NUM_IRQS]; > > + int pci_irq_levels[PIIX_NUM_PIRQS]; > + I wondered how we were migrating this state, and the answer seems to be that we aren't (and weren't before, when it was a global variable, so this is a pre-existing bug). Does the malta platform support migration save/load? We should probably add this field to the vmstate struct (which will be a migration compatibility break, which is OK as the malta board isn't versioned.) -- PMM
Am 30. Januar 2022 23:53:42 MEZ schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>: >On 14/1/22 14:36, Peter Maydell wrote: >> On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow <shentey@gmail.com> wrote: >>> >>> Now that piix4_set_irq's opaque parameter references own PIIX4State, >>> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/isa/piix4.c | 22 +++++++++------------- >>> include/hw/southbridge/piix.h | 2 -- >>> 2 files changed, 9 insertions(+), 15 deletions(-) >>> >>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c >>> index a31e9714cf..964e09cf7f 100644 >>> --- a/hw/isa/piix4.c >>> +++ b/hw/isa/piix4.c >>> @@ -39,14 +39,14 @@ >>> #include "sysemu/runstate.h" >>> #include "qom/object.h" >>> >>> -PCIDevice *piix4_dev; >>> - >>> struct PIIX4State { >>> PCIDevice dev; >>> qemu_irq cpu_intr; >>> qemu_irq *isa; >>> qemu_irq i8259[ISA_NUM_IRQS]; >>> >>> + int pci_irq_levels[PIIX_NUM_PIRQS]; >>> + >> >> I wondered how we were migrating this state, and the answer >> seems to be that we aren't (and weren't before, when it was >> a global variable, so this is a pre-existing bug). > >Indeed the migrated VM starts with PCI IRQ levels zeroed. > >> Does the malta platform support migration save/load? > >Maybe a "best effort" support, but not versioned machines. > >> We should probably add this field to the vmstate struct >> (which will be a migration compatibility break, which is OK >> as the malta board isn't versioned.) > >Yeah good catch. > >Bernhard, do you mind adding it? Sure, I'll give it a try. Shall I submit a v2 of this patch series then? If so, would it be ok to change the topic of the cover letter or would that be confusing? Last but not least: How to treat the version_id and the version parameters of the new and existing fields? Regards, Bernhard.
On Thu, Feb 10, 2022 at 12:16:34AM +0100, BB wrote: > Am 30. Januar 2022 23:53:42 MEZ schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>: > >On 14/1/22 14:36, Peter Maydell wrote: > >> On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow <shentey@gmail.com> wrote: > >>> > >>> Now that piix4_set_irq's opaque parameter references own PIIX4State, > >>> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State. > >>> > >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> > >>> --- > >>> hw/isa/piix4.c | 22 +++++++++------------- > >>> include/hw/southbridge/piix.h | 2 -- > >>> 2 files changed, 9 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c > >>> index a31e9714cf..964e09cf7f 100644 > >>> --- a/hw/isa/piix4.c > >>> +++ b/hw/isa/piix4.c > >>> @@ -39,14 +39,14 @@ > >>> #include "sysemu/runstate.h" > >>> #include "qom/object.h" > >>> > >>> -PCIDevice *piix4_dev; > >>> - > >>> struct PIIX4State { > >>> PCIDevice dev; > >>> qemu_irq cpu_intr; > >>> qemu_irq *isa; > >>> qemu_irq i8259[ISA_NUM_IRQS]; > >>> > >>> + int pci_irq_levels[PIIX_NUM_PIRQS]; > >>> + > >> > >> I wondered how we were migrating this state, and the answer > >> seems to be that we aren't (and weren't before, when it was > >> a global variable, so this is a pre-existing bug). > > > >Indeed the migrated VM starts with PCI IRQ levels zeroed. > > > >> Does the malta platform support migration save/load? > > > >Maybe a "best effort" support, but not versioned machines. > > > >> We should probably add this field to the vmstate struct > >> (which will be a migration compatibility break, which is OK > >> as the malta board isn't versioned.) > > > >Yeah good catch. > > > >Bernhard, do you mind adding it? > > Sure, I'll give it a try. Shall I submit a v2 of this patch series then? If so, would it be ok to change the topic of the cover letter or would that be confusing? It's ok to change the subject of the cover letter. > Last but not least: How to treat the version_id and the version parameters of the new and existing fields? > > Regards, > > Bernhard.
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c index a31e9714cf..964e09cf7f 100644 --- a/hw/isa/piix4.c +++ b/hw/isa/piix4.c @@ -39,14 +39,14 @@ #include "sysemu/runstate.h" #include "qom/object.h" -PCIDevice *piix4_dev; - struct PIIX4State { PCIDevice dev; qemu_irq cpu_intr; qemu_irq *isa; qemu_irq i8259[ISA_NUM_IRQS]; + int pci_irq_levels[PIIX_NUM_PIRQS]; + RTCState rtc; /* Reset Control Register */ MemoryRegion rcr_mem; @@ -55,24 +55,22 @@ struct PIIX4State { OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE) -static int pci_irq_levels[4]; - static void piix4_set_irq(void *opaque, int irq_num, int level) { int i, pic_irq, pic_level; PIIX4State *s = opaque; - pci_irq_levels[irq_num] = level; + s->pci_irq_levels[irq_num] = level; /* now we change the pic irq level according to the piix irq mappings */ /* XXX: optimize */ - pic_irq = piix4_dev->config[PIIX_PIRQCA + irq_num]; - if (pic_irq < 16) { + pic_irq = s->dev.config[PIIX_PIRQCA + irq_num]; + if (pic_irq < ISA_NUM_IRQS) { /* The pic level is the logical OR of all the PCI irqs mapped to it. */ pic_level = 0; - for (i = 0; i < 4; i++) { - if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) { - pic_level |= pci_irq_levels[i]; + for (i = 0; i < PIIX_NUM_PIRQS; i++) { + if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) { + pic_level |= s->pci_irq_levels[i]; } } qemu_set_irq(s->i8259[pic_irq], pic_level); @@ -223,8 +221,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp) return; } isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ); - - piix4_dev = dev; } static void piix4_init(Object *obj) @@ -323,7 +319,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus) NULL, 0, NULL); } - pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 4); + pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS); for (int i = 0; i < ISA_NUM_IRQS; i++) { s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i); diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h index 6387f2b612..f63f83e5c6 100644 --- a/include/hw/southbridge/piix.h +++ b/include/hw/southbridge/piix.h @@ -70,8 +70,6 @@ typedef struct PIIXState PIIX3State; DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE, TYPE_PIIX3_PCI_DEVICE) -extern PCIDevice *piix4_dev; - PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus); DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
Now that piix4_set_irq's opaque parameter references own PIIX4State, piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/piix4.c | 22 +++++++++------------- include/hw/southbridge/piix.h | 2 -- 2 files changed, 9 insertions(+), 15 deletions(-)