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