diff mbox series

[1/3] hw/isa/vt82c686: Fix SCI routing

Message ID 20230129213418.87978-2-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series VIA PM Improvements | expand

Commit Message

Bernhard Beschow Jan. 29, 2023, 9:34 p.m. UTC 
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(-)

Comments

BALATON Zoltan Jan. 31, 2023, 2:42 p.m. UTC | #1 
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);
>
Bernhard Beschow Feb. 7, 2023, 11:13 p.m. UTC | #2 
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 mbox series

Patch

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);