subject:"\[PATCH 1\/3\] hw\/isa\/vt82c686\: Fix SCI routing"

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

2023-02-07 Thread Bernhard Beschow




Am 31. Januar 2023 14:42:29 UTC schrieb BALATON Zoltan :
>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 
>> ---
>> 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(>dev, sci_level);
>> -}
>> +qemu_set_irq(s->irq, sci_level);
>> /* schedule a timer interruption if needed */
>> acpi_pm_tmr_update(>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(>ar, >io, false, false, 2, false);
>> }
>> 
>> +static void via_pm_init(Object *obj)
>> +{
>> +ViaPMState *s = VIA_PM(obj);
>> +
>> +qdev_init_gpio_out(DEVICE(obj), >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", >uhci[1], 
>> TYPE_VT82C686B_USB_UHCI);
>> object_initialize_child(obj, "ac97", >ac97, TYPE_VIA_AC97);
>> object_initialize_child(obj, "mc97", >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(>pm), BUS(pci_bus), errp)) {
>> return;
>> }
>> +qdev_connect_gpio_out(DEVICE(>pm), 0,
>> +  qdev_get_gpio_in_named(DEVICE(d), "sci", 0));
>> 
>> /* Function 5: AC97 Audio */
>> qdev_prop_set_int32(DEVICE(>ac97), "addr", d->devfn + 5);
>>

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

2023-01-31 Thread BALATON Zoltan


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 
---
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(>dev, sci_level);
-}
+qemu_set_irq(s->irq, sci_level);
/* schedule a timer interruption if needed */
acpi_pm_tmr_update(>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(>ar, >io, false, false, 2, false);
}

+static void via_pm_init(Object *obj)
+{
+ViaPMState *s = VIA_PM(obj);
+
+qdev_init_gpio_out(DEVICE(obj), >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", >uhci[1], TYPE_VT82C686B_USB_UHCI);
object_initialize_child(obj, "ac97", >ac97, TYPE_VIA_AC97);
object_initialize_child(obj, "mc97", >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(>pm), BUS(pci_bus), errp)) {
return;
}
+qdev_connect_gpio_out(DEVICE(>pm), 0,
+  qdev_get_gpio_in_named(DEVICE(d), "sci", 0));

/* Function 5: AC97 Audio */
qdev_prop_set_int32(DEVICE(>ac97), "addr", d->devfn + 5);

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

2023-01-29 Thread Bernhard Beschow

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 
---
 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(>dev, sci_level);
-}
+qemu_set_irq(s->irq, sci_level);
 /* schedule a timer interruption if needed */
 acpi_pm_tmr_update(>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(>ar, >io, false, false, 2, false);
 }
 
+static void via_pm_init(Object *obj)
+{
+ViaPMState *s = VIA_PM(obj);
+
+qdev_init_gpio_out(DEVICE(obj), >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", >uhci[1], 
TYPE_VT82C686B_USB_UHCI);
 object_initialize_child(obj, "ac97", >ac97, TYPE_VIA_AC97);
 object_initialize_child(obj, "mc97", >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(>pm), BUS(pci_bus), errp)) {
 return;
 }
+qdev_connect_gpio_out(DEVICE(>pm), 0,
+  qdev_get_gpio_in_named(DEVICE(d), "sci", 0));
 
 /* Function 5: AC97 Audio */
 qdev_prop_set_int32(DEVICE(>ac97), "addr", d->devfn + 5);
-- 
2.39.1

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information