Message ID | 20230129213418.87978-2-shentey@gmail.com |
---|---|
State | New |
Headers | show |
Series | VIA PM Improvements | expand |
On Sun, 29 Jan 2023, Bernhard Beschow wrote: > According to the PCI specification, the hardware is not supposed to use > PCI_INTERRUPT_PIN for interrupt routing. Use the dedicated ACPI > Interrupt Select register for SCI routing instead. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++------------ > 1 file changed, 30 insertions(+), 12 deletions(-) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 3f9bd0c04d..2189be6f20 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -46,6 +46,8 @@ struct ViaPMState { > ACPIREGS ar; > APMState apm; > PMSMBus smb; > + > + qemu_irq irq; > }; > > static void pm_io_space_update(ViaPMState *s) > @@ -148,18 +150,7 @@ static void pm_update_sci(ViaPMState *s) > ACPI_BITMASK_POWER_BUTTON_ENABLE | > ACPI_BITMASK_GLOBAL_LOCK_ENABLE | > ACPI_BITMASK_TIMER_ENABLE)) != 0); > - if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { > - /* > - * FIXME: > - * Fix device model that realizes this PM device and remove > - * this work around. > - * The device model should wire SCI and setup > - * PCI_INTERRUPT_PIN properly. > - * If PIN# = 0(interrupt pin isn't used), don't raise SCI as > - * work around. > - */ > - pci_set_irq(&s->dev, sci_level); > - } > + qemu_set_irq(s->irq, sci_level); > /* schedule a timer interruption if needed */ > acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && > !(pmsts & ACPI_BITMASK_TIMER_STATUS)); > @@ -213,6 +204,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) > acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false); > } > > +static void via_pm_init(Object *obj) > +{ > + ViaPMState *s = VIA_PM(obj); > + > + qdev_init_gpio_out(DEVICE(obj), &s->irq, 1); > +} > + > typedef struct via_pm_init_info { > uint16_t device_id; > } ViaPMInitInfo; > @@ -238,6 +236,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data) > static const TypeInfo via_pm_info = { > .name = TYPE_VIA_PM, > .parent = TYPE_PCI_DEVICE, > + .instance_init = via_pm_init, > .instance_size = sizeof(ViaPMState), > .abstract = true, > .interfaces = (InterfaceInfo[]) { > @@ -568,6 +567,21 @@ static const VMStateDescription vmstate_via = { > } > }; > > +static void via_isa_set_pm_irq(void *opaque, int n, int level) > +{ > + ViaISAState *s = opaque; > + uint8_t irq = pci_get_byte(s->pm.dev.config + 0x42) & 0xf; > + > + if (irq == 2) { unlikely(irq == 2) but do we really need to log this? It really should not happen but if it does the guest is really broken and this will just flood the log so I think you can just test for it in the if below and drop the log. Regards, BALATON Zoltan > + qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved"); > + return; > + } > + > + if (irq != 0) { > + qemu_set_irq(s->isa_irqs[irq], level); > + } > +} > + > static void via_isa_init(Object *obj) > { > ViaISAState *s = VIA_ISA(obj); > @@ -578,6 +592,8 @@ static void via_isa_init(Object *obj) > object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); > object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); > object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); > + > + qdev_init_gpio_in_named(DEVICE(obj), via_isa_set_pm_irq, "sci", 1); > } > > static const TypeInfo via_isa_info = { > @@ -664,6 +680,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) > if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) { > return; > } > + qdev_connect_gpio_out(DEVICE(&s->pm), 0, > + qdev_get_gpio_in_named(DEVICE(d), "sci", 0)); > > /* Function 5: AC97 Audio */ > qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5); >
Am 31. Januar 2023 14:42:29 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sun, 29 Jan 2023, Bernhard Beschow wrote: >> According to the PCI specification, the hardware is not supposed to use >> PCI_INTERRUPT_PIN for interrupt routing. Use the dedicated ACPI >> Interrupt Select register for SCI routing instead. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++------------ >> 1 file changed, 30 insertions(+), 12 deletions(-) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 3f9bd0c04d..2189be6f20 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -46,6 +46,8 @@ struct ViaPMState { >> ACPIREGS ar; >> APMState apm; >> PMSMBus smb; >> + >> + qemu_irq irq; >> }; >> >> static void pm_io_space_update(ViaPMState *s) >> @@ -148,18 +150,7 @@ static void pm_update_sci(ViaPMState *s) >> ACPI_BITMASK_POWER_BUTTON_ENABLE | >> ACPI_BITMASK_GLOBAL_LOCK_ENABLE | >> ACPI_BITMASK_TIMER_ENABLE)) != 0); >> - if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { >> - /* >> - * FIXME: >> - * Fix device model that realizes this PM device and remove >> - * this work around. >> - * The device model should wire SCI and setup >> - * PCI_INTERRUPT_PIN properly. >> - * If PIN# = 0(interrupt pin isn't used), don't raise SCI as >> - * work around. >> - */ >> - pci_set_irq(&s->dev, sci_level); >> - } >> + qemu_set_irq(s->irq, sci_level); >> /* schedule a timer interruption if needed */ >> acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && >> !(pmsts & ACPI_BITMASK_TIMER_STATUS)); >> @@ -213,6 +204,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) >> acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false); >> } >> >> +static void via_pm_init(Object *obj) >> +{ >> + ViaPMState *s = VIA_PM(obj); >> + >> + qdev_init_gpio_out(DEVICE(obj), &s->irq, 1); >> +} >> + >> typedef struct via_pm_init_info { >> uint16_t device_id; >> } ViaPMInitInfo; >> @@ -238,6 +236,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data) >> static const TypeInfo via_pm_info = { >> .name = TYPE_VIA_PM, >> .parent = TYPE_PCI_DEVICE, >> + .instance_init = via_pm_init, >> .instance_size = sizeof(ViaPMState), >> .abstract = true, >> .interfaces = (InterfaceInfo[]) { >> @@ -568,6 +567,21 @@ static const VMStateDescription vmstate_via = { >> } >> }; >> >> +static void via_isa_set_pm_irq(void *opaque, int n, int level) >> +{ >> + ViaISAState *s = opaque; >> + uint8_t irq = pci_get_byte(s->pm.dev.config + 0x42) & 0xf; >> + >> + if (irq == 2) { > >unlikely(irq == 2) but do we really need to log this? It really should not happen but if it does the guest is really broken and this will just flood the log so I think you can just test for it in the if below and drop the log. I'd prefer to log such guest errors precisely for above reason: To detect really broken guests. If it's really too much noise one can still filter the log with external tools such as grep. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> + qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved"); >> + return; >> + } >> + >> + if (irq != 0) { >> + qemu_set_irq(s->isa_irqs[irq], level); >> + } >> +} >> + >> static void via_isa_init(Object *obj) >> { >> ViaISAState *s = VIA_ISA(obj); >> @@ -578,6 +592,8 @@ static void via_isa_init(Object *obj) >> object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); >> object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); >> object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); >> + >> + qdev_init_gpio_in_named(DEVICE(obj), via_isa_set_pm_irq, "sci", 1); >> } >> >> static const TypeInfo via_isa_info = { >> @@ -664,6 +680,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) { >> return; >> } >> + qdev_connect_gpio_out(DEVICE(&s->pm), 0, >> + qdev_get_gpio_in_named(DEVICE(d), "sci", 0)); >> >> /* Function 5: AC97 Audio */ >> qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5); >>
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 3f9bd0c04d..2189be6f20 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -46,6 +46,8 @@ struct ViaPMState { ACPIREGS ar; APMState apm; PMSMBus smb; + + qemu_irq irq; }; static void pm_io_space_update(ViaPMState *s) @@ -148,18 +150,7 @@ static void pm_update_sci(ViaPMState *s) ACPI_BITMASK_POWER_BUTTON_ENABLE | ACPI_BITMASK_GLOBAL_LOCK_ENABLE | ACPI_BITMASK_TIMER_ENABLE)) != 0); - if (pci_get_byte(s->dev.config + PCI_INTERRUPT_PIN)) { - /* - * FIXME: - * Fix device model that realizes this PM device and remove - * this work around. - * The device model should wire SCI and setup - * PCI_INTERRUPT_PIN properly. - * If PIN# = 0(interrupt pin isn't used), don't raise SCI as - * work around. - */ - pci_set_irq(&s->dev, sci_level); - } + qemu_set_irq(s->irq, sci_level); /* schedule a timer interruption if needed */ acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) && !(pmsts & ACPI_BITMASK_TIMER_STATUS)); @@ -213,6 +204,13 @@ static void via_pm_realize(PCIDevice *dev, Error **errp) acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2, false); } +static void via_pm_init(Object *obj) +{ + ViaPMState *s = VIA_PM(obj); + + qdev_init_gpio_out(DEVICE(obj), &s->irq, 1); +} + typedef struct via_pm_init_info { uint16_t device_id; } ViaPMInitInfo; @@ -238,6 +236,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data) static const TypeInfo via_pm_info = { .name = TYPE_VIA_PM, .parent = TYPE_PCI_DEVICE, + .instance_init = via_pm_init, .instance_size = sizeof(ViaPMState), .abstract = true, .interfaces = (InterfaceInfo[]) { @@ -568,6 +567,21 @@ static const VMStateDescription vmstate_via = { } }; +static void via_isa_set_pm_irq(void *opaque, int n, int level) +{ + ViaISAState *s = opaque; + uint8_t irq = pci_get_byte(s->pm.dev.config + 0x42) & 0xf; + + if (irq == 2) { + qemu_log_mask(LOG_GUEST_ERROR, "IRQ 2 for PM controller is reserved"); + return; + } + + if (irq != 0) { + qemu_set_irq(s->isa_irqs[irq], level); + } +} + static void via_isa_init(Object *obj) { ViaISAState *s = VIA_ISA(obj); @@ -578,6 +592,8 @@ static void via_isa_init(Object *obj) object_initialize_child(obj, "uhci2", &s->uhci[1], TYPE_VT82C686B_USB_UHCI); object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97); object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97); + + qdev_init_gpio_in_named(DEVICE(obj), via_isa_set_pm_irq, "sci", 1); } static const TypeInfo via_isa_info = { @@ -664,6 +680,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) { return; } + qdev_connect_gpio_out(DEVICE(&s->pm), 0, + qdev_get_gpio_in_named(DEVICE(d), "sci", 0)); /* Function 5: AC97 Audio */ qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
According to the PCI specification, the hardware is not supposed to use PCI_INTERRUPT_PIN for interrupt routing. Use the dedicated ACPI Interrupt Select register for SCI routing instead. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-)