[PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[Bernhard Beschow]

According to the datasheet, SCI interrupts of the power management function
aren't triggered through the PCI pins but rather directly to the integrated PIC.
The routing is configurable through the ACPI interrupt select register at offset
42 in the PCI configuration space of the ISA function.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 57bdfb4e78..2988ad1eda 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,9 +567,25 @@ 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_in[irq], level);
+    }
+}
+
 static void via_isa_init(Object *obj)
 {
     ViaISAState *s = VIA_ISA(obj);
+    DeviceState *dev = DEVICE(s);
 
     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
@@ -578,6 +593,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(dev, via_isa_set_pm_irq, "sci", 1);
 }
 
 static const TypeInfo via_isa_info = {
@@ -704,6 +721,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);
-- 
2.42.0

Re: [PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[Philippe Mathieu-Daudé]

Hi Bernhard,

On 3/10/23 23:44, Bernhard Beschow wrote:
> According to the datasheet, SCI interrupts of the power management function
> aren't triggered through the PCI pins but rather directly to the integrated PIC.
> The routing is configurable through the ACPI interrupt select register at offset
> 42 in the PCI configuration space of the ISA function.

You describe 42 but use 0x42 (66). Clearer would be to add a definition,
maybe:

   #define PCI_ACPI_INTR_SELECT_OFS 0x42
   #define PCI_ACPI_INTR_SELECT_MSK 0xf

Alternatively self-document with function name:

   static unsigned via_isa_get_pm_irq_index(ViaISAState *s)
   {
       return pci_get_byte(s->pm.dev.config + 0x42) & 0xf;
   }

Otherwise LGTM.

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
>   1 file changed, 31 insertions(+), 12 deletions(-)

> +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_in[irq], level);
> +    }
> +}

Re: [PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[BALATON Zoltan]

On Tue, 3 Oct 2023, Bernhard Beschow wrote:
> According to the datasheet, SCI interrupts of the power management function
> aren't triggered through the PCI pins but rather directly to the integrated PIC.
> The routing is configurable through the ACPI interrupt select register at offset
> 42 in the PCI configuration space of the ISA function.

This should be config reg 42 of the PM function 4 not ISA function 0 but 
the code seems to do it right just this description is wrong.

>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

You could cc me on vt82c686 too as now I have two machines using these 
(one is not yet upstream).

> ---
> hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
> 1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 57bdfb4e78..2988ad1eda 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -46,6 +46,8 @@ struct ViaPMState {
>     ACPIREGS ar;
>     APMState apm;
>     PMSMBus smb;
> +

No empty line needed here. It you want to, you could add an empty line 
after the first member and rename that to parent_obj as per new convention 
while you're changing this object state.

> +    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,9 +567,25 @@ 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_in[irq], level);
> +    }
> +}
> +
> static void via_isa_init(Object *obj)
> {
>     ViaISAState *s = VIA_ISA(obj);
> +    DeviceState *dev = DEVICE(s);

No need to add local variable for single use unless you expect to have 
more of these later but for single use you caould just use DEVICE(obj or 
s) in the call below.

Other than these small nits:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

>
>     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
> @@ -578,6 +593,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(dev, via_isa_set_pm_irq, "sci", 1);
> }
>
> static const TypeInfo via_isa_info = {
> @@ -704,6 +721,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);
>

Re: [PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[Bernhard Beschow]

Am 4. Oktober 2023 12:08:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 3 Oct 2023, Bernhard Beschow wrote:
>> According to the datasheet, SCI interrupts of the power management function
>> aren't triggered through the PCI pins but rather directly to the integrated PIC.
>> The routing is configurable through the ACPI interrupt select register at offset
>> 42 in the PCI configuration space of the ISA function.
>
>This should be config reg 42 of the PM function 4 not ISA function 0 but the code seems to do it right just this description is wrong.

Notice via_isa_set_pm_irq() using ViaISAState for routing the SCI to the PIC. Hence, the description seems correct to me.

>
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>
>You could cc me on vt82c686 too as now I have two machines using these (one is not yet upstream).

Usually I let git-publish handle the cc which derives it from the MAINTAINERS file. You could add yourself there to get cc'ed automatically.

I'm curious what the other machine not yet upstreamed is?

>
>> ---
>> hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
>> 1 file changed, 31 insertions(+), 12 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 57bdfb4e78..2988ad1eda 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -46,6 +46,8 @@ struct ViaPMState {
>>     ACPIREGS ar;
>>     APMState apm;
>>     PMSMBus smb;
>> +
>
>No empty line needed here.

Here I took inspiration from struct PIIX4PMState which separates the qemu_irqs, too. The long term plan is to also add an smi_irq attribute in order to bring both device models to feature parity. So I'd rather leave it as is.

> It you want to, you could add an empty line after the first member and rename that to parent_obj as per new convention while you're changing this object state.

I didn't want to add this style fix in this single commit series. I think this would deserve its own commit where all device models in this file are fixed.

>
>> +    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,9 +567,25 @@ 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_in[irq], level);
>> +    }
>> +}
>> +
>> static void via_isa_init(Object *obj)
>> {
>>     ViaISAState *s = VIA_ISA(obj);
>> +    DeviceState *dev = DEVICE(s);
>
>No need to add local variable for single use unless you expect to have more of these later but for single use you caould just use DEVICE(obj or s) in the call below.

I have one more user on my pc-via branch for wiring the ISA interrupts.

Best regards,
Bernhard

>
>Other than these small nits:
>
>Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>
>> 
>>     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
>> @@ -578,6 +593,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(dev, via_isa_set_pm_irq, "sci", 1);
>> }
>> 
>> static const TypeInfo via_isa_info = {
>> @@ -704,6 +721,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);
>> 

Re: [PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[BALATON Zoltan]

On Wed, 4 Oct 2023, Bernhard Beschow wrote:
> Am 4. Oktober 2023 12:08:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Tue, 3 Oct 2023, Bernhard Beschow wrote:
>>> According to the datasheet, SCI interrupts of the power management function
>>> aren't triggered through the PCI pins but rather directly to the integrated PIC.
>>> The routing is configurable through the ACPI interrupt select register at offset
>>> 42 in the PCI configuration space of the ISA function.
>>
>> This should be config reg 42 of the PM function 4 not ISA function 0 but the code seems to do it right just this description is wrong.
>
> Notice via_isa_set_pm_irq() using ViaISAState for routing the SCI to the PIC. Hence, the description seems correct to me.

It uses s->pm.dev.config + 0x42 so that's the PM device not s->dev.config 
which is a different register so I think the description should also talk 
about offset 0x42 of PCI configuration of the PM function, shouldn't it?

>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>
>> You could cc me on vt82c686 too as now I have two machines using these (one is not yet upstream).
>
> Usually I let git-publish handle the cc which derives it from the MAINTAINERS file. You could add yourself there to get cc'ed automatically.

I thought about sending a patch splitting off vt82c686 from fuloong2e so I 
can add myself as reviewer there. I may do that later.

> I'm curious what the other machine not yet upstreamed is?

Tou can read about it here:

https://www.amigans.net/modules/newbb/viewtopic.php?topic_id=9282

I'll plan to send it when it's better tested but it needs a firmware image 
that I could not yet solve.

>>> ---
>>> hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
>>> 1 file changed, 31 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 57bdfb4e78..2988ad1eda 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -46,6 +46,8 @@ struct ViaPMState {
>>>     ACPIREGS ar;
>>>     APMState apm;
>>>     PMSMBus smb;
>>> +
>>
>> No empty line needed here.
>
> Here I took inspiration from struct PIIX4PMState which separates the 
> qemu_irqs, too. The long term plan is to also add an smi_irq attribute 
> in order to bring both device models to feature parity. So I'd rather 
> leave it as is.

Maybe better name it sci_irq then to avoid confusion. If you add smi later 
as well then maybe it's time to think now how to best model this. Does sci 
needs to be exposed as named qemu_gpio on function 0 or should that be smi 
and sci routed to smi optionally whcch seems to be what the real chip 
does? I don't know how these work just looked at the data sheet briefly so 
I have more questions than answers but looks like exposing sci is not 
needed and this patch could just add an irq to PM func only that could set 
ISA IRQ and then later when smi_irq is added that might need a qemu_gpio 
in func 0 modelling the SMI pin of the chip.

>> It you want to, you could add an empty line after the first member and rename that to parent_obj as per new convention while you're changing this object state.
>
> I didn't want to add this style fix in this single commit series. I 
> think this would deserve its own commit where all device models in this 
> file are fixed.

That works for me too, no need to do it here, just mentioned that's where 
I think a new line may be acceptable but not really needed elsewhere.

>>
>>> +    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,9 +567,25 @@ 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_in[irq], level);
>>> +    }
>>> +}
>>> +
>>> static void via_isa_init(Object *obj)
>>> {
>>>     ViaISAState *s = VIA_ISA(obj);
>>> +    DeviceState *dev = DEVICE(s);
>>
>> No need to add local variable for single use unless you expect to have more of these later but for single use you caould just use DEVICE(obj or s) in the call below.
>
> I have one more user on my pc-via branch for wiring the ISA interrupts.

OK as said if you expect more usages then a local variable can be useful 
just seems unnecessary for a single use but if you'll follow up with 
something that will use it it can be added now.

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
>
>>
>> Other than these small nits:
>>
>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>>
>>>
>>>     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
>>> @@ -578,6 +593,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(dev, via_isa_set_pm_irq, "sci", 1);
>>> }
>>>
>>> static const TypeInfo via_isa_info = {
>>> @@ -704,6 +721,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);
>>>
>
>

Re: [PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[Bernhard Beschow]

Am 4. Oktober 2023 14:52:14 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 4 Oct 2023, Bernhard Beschow wrote:
>> Am 4. Oktober 2023 12:08:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Tue, 3 Oct 2023, Bernhard Beschow wrote:
>>>> According to the datasheet, SCI interrupts of the power management function
>>>> aren't triggered through the PCI pins but rather directly to the integrated PIC.
>>>> The routing is configurable through the ACPI interrupt select register at offset
>>>> 42 in the PCI configuration space of the ISA function.
>>> 
>>> This should be config reg 42 of the PM function 4 not ISA function 0 but the code seems to do it right just this description is wrong.
>> 
>> Notice via_isa_set_pm_irq() using ViaISAState for routing the SCI to the PIC. Hence, the description seems correct to me.
>
>It uses s->pm.dev.config + 0x42 so that's the PM device not s->dev.config which is a different register so I think the description should also talk about offset 0x42 of PCI configuration of the PM function, shouldn't it?

Oh right, I'll fix the commit message.

>
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> 
>>> You could cc me on vt82c686 too as now I have two machines using these (one is not yet upstream).
>> 
>> Usually I let git-publish handle the cc which derives it from the MAINTAINERS file. You could add yourself there to get cc'ed automatically.
>
>I thought about sending a patch splitting off vt82c686 from fuloong2e so I can add myself as reviewer there. I may do that later.
>
>> I'm curious what the other machine not yet upstreamed is?
>
>Tou can read about it here:
>
>https://www.amigans.net/modules/newbb/viewtopic.php?topic_id=9282
>
>I'll plan to send it when it's better tested but it needs a firmware image that I could not yet solve.
>
>>>> ---
>>>> hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
>>>> 1 file changed, 31 insertions(+), 12 deletions(-)
>>>> 
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 57bdfb4e78..2988ad1eda 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -46,6 +46,8 @@ struct ViaPMState {
>>>>     ACPIREGS ar;
>>>>     APMState apm;
>>>>     PMSMBus smb;
>>>> +
>>> 
>>> No empty line needed here.
>> 
>> Here I took inspiration from struct PIIX4PMState which separates the qemu_irqs, too. The long term plan is to also add an smi_irq attribute in order to bring both device models to feature parity. So I'd rather leave it as is.
>
>Maybe better name it sci_irq then to avoid confusion.

Will do. And I'll turn it into a named GPIO.

> If you add smi later as well then maybe it's time to think now how to best model this. Does sci needs to be exposed as named qemu_gpio on function 0 or should that be smi and sci routed to smi optionally whcch seems to be what the real chip does? I don't know how these work just looked at the data sheet briefly so I have more questions than answers but looks like exposing sci is not needed and this patch could just add an irq to PM func only that could set ISA IRQ and then later when smi_irq is added that might need a qemu_gpio in func 0 modelling the SMI pin of the chip.

Right, I think the SCI doesn't need to be exposed since it is handled internally in the chip. This is what this patch implements.

The SMI will need to be exposed since it puts an x86 CPU in system management mode via a dedicated pin.

Best regards,
Bernhard

>
>>> It you want to, you could add an empty line after the first member and rename that to parent_obj as per new convention while you're changing this object state.
>> 
>> I didn't want to add this style fix in this single commit series. I think this would deserve its own commit where all device models in this file are fixed.
>
>That works for me too, no need to do it here, just mentioned that's where I think a new line may be acceptable but not really needed elsewhere.
>
>>> 
>>>> +    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,9 +567,25 @@ 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_in[irq], level);
>>>> +    }
>>>> +}
>>>> +
>>>> static void via_isa_init(Object *obj)
>>>> {
>>>>     ViaISAState *s = VIA_ISA(obj);
>>>> +    DeviceState *dev = DEVICE(s);
>>> 
>>> No need to add local variable for single use unless you expect to have more of these later but for single use you caould just use DEVICE(obj or s) in the call below.
>> 
>> I have one more user on my pc-via branch for wiring the ISA interrupts.
>
>OK as said if you expect more usages then a local variable can be useful just seems unnecessary for a single use but if you'll follow up with something that will use it it can be added now.
>
>Regards,
>BALATON Zoltan
>
>> Best regards,
>> Bernhard
>> 
>>> 
>>> Other than these small nits:
>>> 
>>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> 
>>>> 
>>>>     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
>>>> @@ -578,6 +593,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(dev, via_isa_set_pm_irq, "sci", 1);
>>>> }
>>>> 
>>>> static const TypeInfo via_isa_info = {
>>>> @@ -704,6 +721,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);
>>>> 
>> 
>> 

Re: [PATCH] hw/isa/vt82c686: Respect SCI interrupt assignment

[BALATON Zoltan]

On Thu, 5 Oct 2023, Bernhard Beschow wrote:
> Am 4. Oktober 2023 14:52:14 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Wed, 4 Oct 2023, Bernhard Beschow wrote:
>>> Am 4. Oktober 2023 12:08:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> On Tue, 3 Oct 2023, Bernhard Beschow wrote:
>>>>> According to the datasheet, SCI interrupts of the power management function
>>>>> aren't triggered through the PCI pins but rather directly to the integrated PIC.
>>>>> The routing is configurable through the ACPI interrupt select register at offset
>>>>> 42 in the PCI configuration space of the ISA function.
>>>>
>>>> This should be config reg 42 of the PM function 4 not ISA function 0 but the code seems to do it right just this description is wrong.
>>>
>>> Notice via_isa_set_pm_irq() using ViaISAState for routing the SCI to the PIC. Hence, the description seems correct to me.
>>
>> It uses s->pm.dev.config + 0x42 so that's the PM device not s->dev.config which is a different register so I think the description should also talk about offset 0x42 of PCI configuration of the PM function, shouldn't it?
>
> Oh right, I'll fix the commit message.
>
>>
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>
>>>> You could cc me on vt82c686 too as now I have two machines using these (one is not yet upstream).
>>>
>>> Usually I let git-publish handle the cc which derives it from the MAINTAINERS file. You could add yourself there to get cc'ed automatically.
>>
>> I thought about sending a patch splitting off vt82c686 from fuloong2e so I can add myself as reviewer there. I may do that later.
>>
>>> I'm curious what the other machine not yet upstreamed is?
>>
>> Tou can read about it here:
>>
>> https://www.amigans.net/modules/newbb/viewtopic.php?topic_id=9282
>>
>> I'll plan to send it when it's better tested but it needs a firmware image that I could not yet solve.
>>
>>>>> ---
>>>>> hw/isa/vt82c686.c | 43 +++++++++++++++++++++++++++++++------------
>>>>> 1 file changed, 31 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>> index 57bdfb4e78..2988ad1eda 100644
>>>>> --- a/hw/isa/vt82c686.c
>>>>> +++ b/hw/isa/vt82c686.c
>>>>> @@ -46,6 +46,8 @@ struct ViaPMState {
>>>>>     ACPIREGS ar;
>>>>>     APMState apm;
>>>>>     PMSMBus smb;
>>>>> +
>>>>
>>>> No empty line needed here.
>>>
>>> Here I took inspiration from struct PIIX4PMState which separates the qemu_irqs, too. The long term plan is to also add an smi_irq attribute in order to bring both device models to feature parity. So I'd rather leave it as is.
>>
>> Maybe better name it sci_irq then to avoid confusion.
>
> Will do. And I'll turn it into a named GPIO.
>
>> If you add smi later as well then maybe it's time to think now how to best model this. Does sci needs to be exposed as named qemu_gpio on function 0 or should that be smi and sci routed to smi optionally whcch seems to be what the real chip does? I don't know how these work just looked at the data sheet briefly so I have more questions than answers but looks like exposing sci is not needed and this patch could just add an irq to PM func only that could set ISA IRQ and then later when smi_irq is added that might need a qemu_gpio in func 0 modelling the SMI pin of the chip.
>
> Right, I think the SCI doesn't need to be exposed since it is handled internally in the chip. This is what this patch implements.

So then no need to add a named gpio for it and this patch does noe need to 
touch the isa part and can handle it within PM part. See reply to next 
message why I think so.

Regards,
BALATON Zoltan

> The SMI will need to be exposed since it puts an x86 CPU in system management mode via a dedicated pin.
>
> Best regards,
> Bernhard
>
>>
>>>> It you want to, you could add an empty line after the first member and rename that to parent_obj as per new convention while you're changing this object state.
>>>
>>> I didn't want to add this style fix in this single commit series. I think this would deserve its own commit where all device models in this file are fixed.
>>
>> That works for me too, no need to do it here, just mentioned that's where I think a new line may be acceptable but not really needed elsewhere.
>>
>>>>
>>>>> +    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,9 +567,25 @@ 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_in[irq], level);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> static void via_isa_init(Object *obj)
>>>>> {
>>>>>     ViaISAState *s = VIA_ISA(obj);
>>>>> +    DeviceState *dev = DEVICE(s);
>>>>
>>>> No need to add local variable for single use unless you expect to have more of these later but for single use you caould just use DEVICE(obj or s) in the call below.
>>>
>>> I have one more user on my pc-via branch for wiring the ISA interrupts.
>>
>> OK as said if you expect more usages then a local variable can be useful just seems unnecessary for a single use but if you'll follow up with something that will use it it can be added now.
>>
>> Regards,
>> BALATON Zoltan
>>
>>> Best regards,
>>> Bernhard
>>>
>>>>
>>>> Other than these small nits:
>>>>
>>>> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>
>>>>>
>>>>>     object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>>     object_initialize_child(obj, "ide", &s->ide, TYPE_VIA_IDE);
>>>>> @@ -578,6 +593,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(dev, via_isa_set_pm_irq, "sci", 1);
>>>>> }
>>>>>
>>>>> static const TypeInfo via_isa_info = {
>>>>> @@ -704,6 +721,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);
>>>>>
>>>
>>>
>
>