[PATCH 0/4] Avoid using isa_get_irq in vt82c686 model

[PATCH 0/4] Avoid using isa_get_irq in vt82c686 model

[BALATON Zoltan]

Based-on: <cover.1634232746.git.balaton@eik.bme.hu>

This is on top of (v4-hw/usb/vt82c686-uhci-pci: Use ISA instead of
PCI) series and removes usage of isa_get_irq() from the usb and ide
functions. I managed to simplify it so it's not so bad but not sure if
it's much better either but maybe groups things a bit better this way.

Regards,
BALATON Zoltan

BALATON Zoltan (4):
  vt82c686: Move common code to via_isa_realize
  vt82c686: Add a method to VIA_ISA to raise ISA interrupts
  hw/usb/vt82c686-uhci-pci: Avoid using isa_get_irq()
  via-ide: Avoid using isa_get_irq()

 hw/ide/via.c               |  4 +-
 hw/isa/vt82c686.c          | 75 +++++++++++++++++++-------------------
 hw/usb/vt82c686-uhci-pci.c |  3 +-
 include/hw/isa/vt82c686.h  |  4 ++
 4 files changed, 45 insertions(+), 41 deletions(-)

-- 
2.21.4

[PATCH 1/4] vt82c686: Move common code to via_isa_realize

[BALATON Zoltan]

The vt82c686b_realize and vt8231_realize methods are almost identical,
factor out the common parts to a via_isa_realize function to avoid
code duplication.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 67 ++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 38 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f57f3e7067..5b41539f2c 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -542,6 +542,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
 struct ViaISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
+    ISABus *isa_bus;
     ViaSuperIOState *via_sio;
 };
 
@@ -572,6 +573,29 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
     qemu_set_irq(s->cpu_intr, level);
 }
 
+static void via_isa_realize(PCIDevice *d, Error **errp)
+{
+    ViaISAState *s = VIA_ISA(d);
+    DeviceState *dev = DEVICE(d);
+    qemu_irq *isa_irq;
+    int i;
+
+    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
+    s->isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
+                          &error_fatal);
+    isa_bus_irqs(s->isa_bus, i8259_init(s->isa_bus, *isa_irq));
+    i8254_pit_init(s->isa_bus, 0x40, 0, NULL);
+    i8257_dma_init(s->isa_bus, 0);
+    mc146818_rtc_init(s->isa_bus, 2000, NULL);
+
+    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
+        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
+            d->wmask[i] = 0;
+        }
+    }
+}
+
 /* TYPE_VT82C686B_ISA */
 
 static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
@@ -610,27 +634,10 @@ static void vt82c686b_isa_reset(DeviceState *dev)
 static void vt82c686b_realize(PCIDevice *d, Error **errp)
 {
     ViaISAState *s = VIA_ISA(d);
-    DeviceState *dev = DEVICE(d);
-    ISABus *isa_bus;
-    qemu_irq *isa_irq;
-    int i;
 
-    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
-    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
-    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
-                          &error_fatal);
-    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
-    i8254_pit_init(isa_bus, 0x40, 0, NULL);
-    i8257_dma_init(isa_bus, 0);
-    s->via_sio = VIA_SUPERIO(isa_create_simple(isa_bus,
+    via_isa_realize(d, errp);
+    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
                                                TYPE_VT82C686B_SUPERIO));
-    mc146818_rtc_init(isa_bus, 2000, NULL);
-
-    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
-        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
-            d->wmask[i] = 0;
-        }
-    }
 }
 
 static void vt82c686b_class_init(ObjectClass *klass, void *data)
@@ -691,26 +698,10 @@ static void vt8231_isa_reset(DeviceState *dev)
 static void vt8231_realize(PCIDevice *d, Error **errp)
 {
     ViaISAState *s = VIA_ISA(d);
-    DeviceState *dev = DEVICE(d);
-    ISABus *isa_bus;
-    qemu_irq *isa_irq;
-    int i;
-
-    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
-    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
-    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
-                          &error_fatal);
-    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
-    i8254_pit_init(isa_bus, 0x40, 0, NULL);
-    i8257_dma_init(isa_bus, 0);
-    s->via_sio = VIA_SUPERIO(isa_create_simple(isa_bus, TYPE_VT8231_SUPERIO));
-    mc146818_rtc_init(isa_bus, 2000, NULL);
 
-    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
-        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
-            d->wmask[i] = 0;
-        }
-    }
+    via_isa_realize(d, errp);
+    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
+                                               TYPE_VT8231_SUPERIO));
 }
 
 static void vt8231_class_init(ObjectClass *klass, void *data)
-- 
2.21.4

[PATCH 4/4] via-ide: Avoid using isa_get_irq()

[BALATON Zoltan]

Use via_isa_set_irq() which better encapsulates irq handling in the
vt82xx model and avoids using isa_get_irq() that has a comment saying
it should not be used.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 94cc2142c7..252d18f4ac 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -29,7 +29,7 @@
 #include "migration/vmstate.h"
 #include "qemu/module.h"
 #include "sysemu/dma.h"
-
+#include "hw/isa/vt82c686.h"
 #include "hw/ide/pci.h"
 #include "trace.h"
 
@@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
         d->config[0x70 + n * 8] &= ~0x80;
     }
 
-    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
+    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
 }
 
 static void via_ide_reset(DeviceState *dev)
-- 
2.21.4

[PATCH 2/4] vt82c686: Add a method to VIA_ISA to raise ISA interrupts

[BALATON Zoltan]

Other functions in the VT82xx chips need to raise ISA interrupts. Keep
a reference to them in the device state and add via_isa_set_irq() to
allow setting their state.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 10 +++++++++-
 include/hw/isa/vt82c686.h |  4 ++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 5b41539f2c..8f656251b8 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -542,6 +542,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
 struct ViaISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
+    qemu_irq *isa_irqs;
     ISABus *isa_bus;
     ViaSuperIOState *via_sio;
 };
@@ -567,6 +568,12 @@ static const TypeInfo via_isa_info = {
     },
 };
 
+void via_isa_set_irq(PCIDevice *d, int n, int level)
+{
+    ViaISAState *s = VIA_ISA(d);
+    qemu_set_irq(s->isa_irqs[n], level);
+}
+
 static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
 {
     ViaISAState *s = opaque;
@@ -584,7 +591,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     s->isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
-    isa_bus_irqs(s->isa_bus, i8259_init(s->isa_bus, *isa_irq));
+    s->isa_irqs = i8259_init(s->isa_bus, *isa_irq);
+    isa_bus_irqs(s->isa_bus, s->isa_irqs);
     i8254_pit_init(s->isa_bus, 0x40, 0, NULL);
     i8257_dma_init(s->isa_bus, 0);
     mc146818_rtc_init(s->isa_bus, 2000, NULL);
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 0f01aaa471..56ac141be3 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -1,6 +1,8 @@
 #ifndef HW_VT82C686_H
 #define HW_VT82C686_H
 
+#include "hw/pci/pci.h"
+
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
 #define TYPE_VT8231_ISA "vt8231-isa"
@@ -8,4 +10,6 @@
 #define TYPE_VIA_AC97 "via-ac97"
 #define TYPE_VIA_MC97 "via-mc97"
 
+void via_isa_set_irq(PCIDevice *d, int n, int level);
+
 #endif
-- 
2.21.4

[PATCH 3/4] hw/usb/vt82c686-uhci-pci: Avoid using isa_get_irq()

[BALATON Zoltan]

Use via_isa_set_irq() which better encapsulates irq handling in the
vt82xx model and avoids using isa_get_irq() that has a comment saying
it should not be used.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/vt82c686-uhci-pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index e70e739409..92479d11e5 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -1,5 +1,6 @@
 #include "qemu/osdep.h"
 #include "hw/irq.h"
+#include "hw/isa/vt82c686.h"
 #include "hcd-uhci.h"
 
 static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
@@ -7,7 +8,7 @@ static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
     UHCIState *s = opaque;
     uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
     if (irq > 0 && irq < 15) {
-        qemu_set_irq(isa_get_irq(NULL, irq), level);
+        via_isa_set_irq(pci_get_function_0(&s->dev), irq, level);
     }
 }
 
-- 
2.21.4

[PATCH] via-ide: Set user_creatable to false

[BALATON Zoltan]

This model only works as a function of the via superio chip not as a
standalone PCI device.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
This should be before the last patch changing via-ide or squshed into
it. And similar to usb part but there I need to add it to the info
struct. I can resend with these if you think this series worth the
hassle. The previous one fixing the usb irq works without this clean up.

 hw/ide/via.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 252d18f4ac..82def819c4 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -217,6 +217,9 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
 
     dc->reset = via_ide_reset;
     dc->vmsd = &vmstate_ide_pci;
+    /* Reason: only works as function of VIA southbridge */
+    dc->user_creatable = false;
+
     k->realize = via_ide_realize;
     k->exit = via_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
-- 
2.21.4

Re: [PATCH 1/4] vt82c686: Move common code to via_isa_realize

[Jiaxun Yang]

在2021年10月15日十月 上午2:06，BALATON Zoltan写道：
> The vt82c686b_realize and vt8231_realize methods are almost identical,
> factor out the common parts to a via_isa_realize function to avoid
> code duplication.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

> ---
>  hw/isa/vt82c686.c | 67 ++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 38 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index f57f3e7067..5b41539f2c 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -542,6 +542,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
>  struct ViaISAState {
>      PCIDevice dev;
>      qemu_irq cpu_intr;
> +    ISABus *isa_bus;
>      ViaSuperIOState *via_sio;
>  };
> 
> @@ -572,6 +573,29 @@ static void via_isa_request_i8259_irq(void 
> *opaque, int irq, int level)
>      qemu_set_irq(s->cpu_intr, level);
>  }
> 
> +static void via_isa_realize(PCIDevice *d, Error **errp)
> +{
> +    ViaISAState *s = VIA_ISA(d);
> +    DeviceState *dev = DEVICE(d);
> +    qemu_irq *isa_irq;
> +    int i;
> +
> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
> +    s->isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
> +                          &error_fatal);
> +    isa_bus_irqs(s->isa_bus, i8259_init(s->isa_bus, *isa_irq));
> +    i8254_pit_init(s->isa_bus, 0x40, 0, NULL);
> +    i8257_dma_init(s->isa_bus, 0);
> +    mc146818_rtc_init(s->isa_bus, 2000, NULL);
> +
> +    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> +        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
> +            d->wmask[i] = 0;
> +        }
> +    }
> +}
> +
>  /* TYPE_VT82C686B_ISA */
> 
>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
> @@ -610,27 +634,10 @@ static void vt82c686b_isa_reset(DeviceState *dev)
>  static void vt82c686b_realize(PCIDevice *d, Error **errp)
>  {
>      ViaISAState *s = VIA_ISA(d);
> -    DeviceState *dev = DEVICE(d);
> -    ISABus *isa_bus;
> -    qemu_irq *isa_irq;
> -    int i;
> 
> -    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> -    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
> -    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
> -                          &error_fatal);
> -    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
> -    i8254_pit_init(isa_bus, 0x40, 0, NULL);
> -    i8257_dma_init(isa_bus, 0);
> -    s->via_sio = VIA_SUPERIO(isa_create_simple(isa_bus,
> +    via_isa_realize(d, errp);
> +    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
>                                                 TYPE_VT82C686B_SUPERIO));
> -    mc146818_rtc_init(isa_bus, 2000, NULL);
> -
> -    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> -        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
> -            d->wmask[i] = 0;
> -        }
> -    }
>  }
> 
>  static void vt82c686b_class_init(ObjectClass *klass, void *data)
> @@ -691,26 +698,10 @@ static void vt8231_isa_reset(DeviceState *dev)
>  static void vt8231_realize(PCIDevice *d, Error **errp)
>  {
>      ViaISAState *s = VIA_ISA(d);
> -    DeviceState *dev = DEVICE(d);
> -    ISABus *isa_bus;
> -    qemu_irq *isa_irq;
> -    int i;
> -
> -    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> -    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
> -    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
> -                          &error_fatal);
> -    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
> -    i8254_pit_init(isa_bus, 0x40, 0, NULL);
> -    i8257_dma_init(isa_bus, 0);
> -    s->via_sio = VIA_SUPERIO(isa_create_simple(isa_bus, TYPE_VT8231_SUPERIO));
> -    mc146818_rtc_init(isa_bus, 2000, NULL);
> 
> -    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> -        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
> -            d->wmask[i] = 0;
> -        }
> -    }
> +    via_isa_realize(d, errp);
> +    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> +                                               TYPE_VT8231_SUPERIO));
>  }
> 
>  static void vt8231_class_init(ObjectClass *klass, void *data)
> -- 
> 2.21.4

-- 
- Jiaxun

Re: [PATCH 2/4] vt82c686: Add a method to VIA_ISA to raise ISA interrupts

[Jiaxun Yang]

在2021年10月15日十月 上午2:06，BALATON Zoltan写道：
> Other functions in the VT82xx chips need to raise ISA interrupts. Keep
> a reference to them in the device state and add via_isa_set_irq() to
> allow setting their state.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

> ---

-- 
- Jiaxun

Re: [PATCH] via-ide: Set user_creatable to false

[Philippe Mathieu-Daudé]

On 10/15/21 11:16, BALATON Zoltan wrote:
> This model only works as a function of the via superio chip not as a
> standalone PCI device.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> This should be before the last patch changing via-ide or squshed into
> it. And similar to usb part but there I need to add it to the info
> struct. I can resend with these if you think this series worth the
> hassle. The previous one fixing the usb irq works without this clean up.
> 
>  hw/ide/via.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 1/4] vt82c686: Move common code to via_isa_realize

[Philippe Mathieu-Daudé]

On 10/15/21 03:06, BALATON Zoltan wrote:
> The vt82c686b_realize and vt8231_realize methods are almost identical,
> factor out the common parts to a via_isa_realize function to avoid
> code duplication.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 67 ++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 38 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/4] vt82c686: Add a method to VIA_ISA to raise ISA interrupts

[Philippe Mathieu-Daudé]

On 10/15/21 03:06, BALATON Zoltan wrote:
> Other functions in the VT82xx chips need to raise ISA interrupts. Keep
> a reference to them in the device state and add via_isa_set_irq() to
> allow setting their state.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c         | 10 +++++++++-
>  include/hw/isa/vt82c686.h |  4 ++++
>  2 files changed, 13 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 3/4] hw/usb/vt82c686-uhci-pci: Avoid using isa_get_irq()

[Philippe Mathieu-Daudé]

On 10/15/21 03:06, BALATON Zoltan wrote:
> Use via_isa_set_irq() which better encapsulates irq handling in the
> vt82xx model and avoids using isa_get_irq() that has a comment saying
> it should not be used.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/usb/vt82c686-uhci-pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[Philippe Mathieu-Daudé]

On 10/15/21 03:06, BALATON Zoltan wrote:
> Use via_isa_set_irq() which better encapsulates irq handling in the
> vt82xx model and avoids using isa_get_irq() that has a comment saying
> it should not be used.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ide/via.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 94cc2142c7..252d18f4ac 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -29,7 +29,7 @@
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
>  #include "sysemu/dma.h"
> -
> +#include "hw/isa/vt82c686.h"
>  #include "hw/ide/pci.h"
>  #include "trace.h"
>  
> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>          d->config[0x70 + n * 8] &= ~0x80;
>      }
>  
> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);

Since pci_get_function_0() is expensive, we should cache
'PCIDevice *func0' in PCIIDEState, setting the pointer in
via_ide_realize(). Do you mind sending a follow-up patch?

>  }

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/15/21 03:06, BALATON Zoltan wrote:
>> Use via_isa_set_irq() which better encapsulates irq handling in the
>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>> it should not be used.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 94cc2142c7..252d18f4ac 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -29,7 +29,7 @@
>>  #include "migration/vmstate.h"
>>  #include "qemu/module.h"
>>  #include "sysemu/dma.h"
>> -
>> +#include "hw/isa/vt82c686.h"
>>  #include "hw/ide/pci.h"
>>  #include "trace.h"
>>
>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>>          d->config[0x70 + n * 8] &= ~0x80;
>>      }
>>
>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>
> Since pci_get_function_0() is expensive, we should cache
> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
> via_ide_realize(). Do you mind sending a follow-up patch?

I can do that but waiting for a decision on how to proceed. Will Gerd take 
my first series this is based on as is then this should be a separate 
series doing the clean up using pci_get_function_0 or should these two 
series be merged? I'd also squash setting user_creatable = false into this 
patch (and do similar for the usb one) unless you guys think it should be 
a separate patch?

Regards,
BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/15/21 03:06, BALATON Zoltan wrote:
>> Use via_isa_set_irq() which better encapsulates irq handling in the
>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>> it should not be used.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 94cc2142c7..252d18f4ac 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -29,7 +29,7 @@
>>  #include "migration/vmstate.h"
>>  #include "qemu/module.h"
>>  #include "sysemu/dma.h"
>> -
>> +#include "hw/isa/vt82c686.h"
>>  #include "hw/ide/pci.h"
>>  #include "trace.h"
>>
>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>>          d->config[0x70 + n * 8] &= ~0x80;
>>      }
>>
>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>
> Since pci_get_function_0() is expensive, we should cache
> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
> via_ide_realize(). Do you mind sending a follow-up patch?

On the other hand, IMO PCIIDEState should be about PCI IDE stuff so to 
keep this clean this would need subclassing it to VIAIDEState and put the 
func0 pointer there. But then we probably need to cast between VIAIDE and 
PCIIDE and likely we're back to the same level of expensiveness. The main 
source why of pci_get_function_0 is expensive is probably the QOM cast to 
PCI_BUS in pci_get_bus() the rest is just pointer and array index 
dereferences which should not be too bad. And the reason why QOM casts are 
expensive is because we have --enable-qom-debug enabled by default. I've 
tried to send a patch before to disable this on release builds and only 
enable it with --enable-debug or when explicitly asked but it was rejected 
saying that these asserts are useful. Maybe we can just live with 
qemu_set_irq and not bother and drop this series. (You can cherry pick the 
first patch removing code duplication from via isa if you want.)

Regards,
BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[Philippe Mathieu-Daudé]

On 10/17/21 21:39, BALATON Zoltan wrote:
> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>> it should not be used.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ide/via.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index 94cc2142c7..252d18f4ac 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -29,7 +29,7 @@
>>>  #include "migration/vmstate.h"
>>>  #include "qemu/module.h"
>>>  #include "sysemu/dma.h"
>>> -
>>> +#include "hw/isa/vt82c686.h"
>>>  #include "hw/ide/pci.h"
>>>  #include "trace.h"
>>>
>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>> int level)
>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>      }
>>>
>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>
>> Since pci_get_function_0() is expensive, we should cache
>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>> via_ide_realize(). Do you mind sending a follow-up patch?
> 
> On the other hand, IMO PCIIDEState should be about PCI IDE stuff so to
> keep this clean this would need subclassing it to VIAIDEState and put
> the func0 pointer there.

I expect any multi-function PCI embedding an IDE controller to route
its IRQs via Func#0, but I'm not a PCI expert.

> But then we probably need to cast between
> VIAIDE and PCIIDE and likely we're back to the same level of
> expensiveness.

realize() is called once, get_irq() multiple times.

> The main source why of pci_get_function_0 is expensive is
> probably the QOM cast to PCI_BUS in pci_get_bus() the rest is just
> pointer and array index dereferences which should not be too bad. And
> the reason why QOM casts are expensive is because we have
> --enable-qom-debug enabled by default. I've tried to send a patch before
> to disable this on release builds and only enable it with --enable-debug
> or when explicitly asked but it was rejected saying that these asserts
> are useful. Maybe we can just live with qemu_set_irq and not bother and
> drop this series. (You can cherry pick the first patch removing code
> duplication from via isa if you want.)
> 
> Regards,
> BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[Philippe Mathieu-Daudé]

On 10/17/21 20:44, BALATON Zoltan wrote:
> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>> it should not be used.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ide/via.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index 94cc2142c7..252d18f4ac 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -29,7 +29,7 @@
>>>  #include "migration/vmstate.h"
>>>  #include "qemu/module.h"
>>>  #include "sysemu/dma.h"
>>> -
>>> +#include "hw/isa/vt82c686.h"
>>>  #include "hw/ide/pci.h"
>>>  #include "trace.h"
>>>
>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>> int level)
>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>      }
>>>
>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>
>> Since pci_get_function_0() is expensive, we should cache
>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>> via_ide_realize(). Do you mind sending a follow-up patch?
> 
> I can do that but waiting for a decision on how to proceed. Will Gerd
> take my first series this is based on as is then this should be a
> separate series doing the clean up using pci_get_function_0 or should
> these two series be merged? I'd also squash setting user_creatable =
> false into this patch (and do similar for the usb one) unless you guys
> think it should be a separate patch?

I don't know what Gerd will do with the USB patches.
Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
and extra user_creatable) via mips-next.

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2958 bytes --]

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/17/21 21:39, BALATON Zoltan wrote:
>> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>>> it should not be used.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ide/via.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index 94cc2142c7..252d18f4ac 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -29,7 +29,7 @@
>>>>  #include "migration/vmstate.h"
>>>>  #include "qemu/module.h"
>>>>  #include "sysemu/dma.h"
>>>> -
>>>> +#include "hw/isa/vt82c686.h"
>>>>  #include "hw/ide/pci.h"
>>>>  #include "trace.h"
>>>>
>>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>>> int level)
>>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>>      }
>>>>
>>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>>
>>> Since pci_get_function_0() is expensive, we should cache
>>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>>> via_ide_realize(). Do you mind sending a follow-up patch?
>>
>> On the other hand, IMO PCIIDEState should be about PCI IDE stuff so to
>> keep this clean this would need subclassing it to VIAIDEState and put
>> the func0 pointer there.
>
> I expect any multi-function PCI embedding an IDE controller to route
> its IRQs via Func#0, but I'm not a PCI expert.

We don't have many in QEMU, I think only via and piix and not sure what 
piix does (currently using isa_get_irq, that's where I got this from for 
via).

>> But then we probably need to cast between
>> VIAIDE and PCIIDE and likely we're back to the same level of
>> expensiveness.
>
> realize() is called once, get_irq() multiple times.

Sure but we need to pass something to set_irq which will be then 
VIAIDEState that maybe we'll need to cast to something else but now we 
also cast it to PCI_DEVICE so maybe we could cache that too?

Regards,
BALATON Zoltan

>> The main source why of pci_get_function_0 is expensive is
>> probably the QOM cast to PCI_BUS in pci_get_bus() the rest is just
>> pointer and array index dereferences which should not be too bad. And
>> the reason why QOM casts are expensive is because we have
>> --enable-qom-debug enabled by default. I've tried to send a patch before
>> to disable this on release builds and only enable it with --enable-debug
>> or when explicitly asked but it was rejected saying that these asserts
>> are useful. Maybe we can just live with qemu_set_irq and not bother and
>> drop this series. (You can cherry pick the first patch removing code
>> duplication from via isa if you want.)
>>
>> Regards,
>> BALATON Zoltan
>
>

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/17/21 20:44, BALATON Zoltan wrote:
>> On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/15/21 03:06, BALATON Zoltan wrote:
>>>> Use via_isa_set_irq() which better encapsulates irq handling in the
>>>> vt82xx model and avoids using isa_get_irq() that has a comment saying
>>>> it should not be used.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ide/via.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index 94cc2142c7..252d18f4ac 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -29,7 +29,7 @@
>>>>  #include "migration/vmstate.h"
>>>>  #include "qemu/module.h"
>>>>  #include "sysemu/dma.h"
>>>> -
>>>> +#include "hw/isa/vt82c686.h"
>>>>  #include "hw/ide/pci.h"
>>>>  #include "trace.h"
>>>>
>>>> @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
>>>> int level)
>>>>          d->config[0x70 + n * 8] &= ~0x80;
>>>>      }
>>>>
>>>> -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
>>>> +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
>>>
>>> Since pci_get_function_0() is expensive, we should cache
>>> 'PCIDevice *func0' in PCIIDEState, setting the pointer in
>>> via_ide_realize(). Do you mind sending a follow-up patch?
>>
>> I can do that but waiting for a decision on how to proceed. Will Gerd
>> take my first series this is based on as is then this should be a
>> separate series doing the clean up using pci_get_function_0 or should
>> these two series be merged? I'd also squash setting user_creatable =
>> false into this patch (and do similar for the usb one) unless you guys
>> think it should be a separate patch?
>
> I don't know what Gerd will do with the USB patches.
> Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
> and extra user_creatable) via mips-next.

I'm confused. So you don't need another version from the last two at 
least? And patch 2 is useless without the rest of the series that's why I 
said you can cherry pick 1.

Regards,
BALATON Zoltan

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[Gerd Hoffmann]

  Hi,

> > I can do that but waiting for a decision on how to proceed. Will Gerd
> > take my first series this is based on as is then this should be a
> > separate series doing the clean up using pci_get_function_0 or should
> > these two series be merged? I'd also squash setting user_creatable =
> > false into this patch (and do similar for the usb one) unless you guys
> > think it should be a separate patch?
> 
> I don't know what Gerd will do with the USB patches.

Latest revision of usb patches is fine.  I'll go stick them into the
next usb pull request ...

> Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
> and extra user_creatable) via mips-next.

.. unless someone else is faster with merging ;)

Feel free to add my "Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>"
to the usb patches and merge the complete pack via mips-next.

take care,
  Gerd

Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[BALATON Zoltan]

Hello,

On Mon, 18 Oct 2021, Gerd Hoffmann wrote:
>  Hi,
>
>>> I can do that but waiting for a decision on how to proceed. Will Gerd
>>> take my first series this is based on as is then this should be a
>>> separate series doing the clean up using pci_get_function_0 or should
>>> these two series be merged? I'd also squash setting user_creatable =
>>> false into this patch (and do similar for the usb one) unless you guys
>>> think it should be a separate patch?
>>
>> I don't know what Gerd will do with the USB patches.
>
> Latest revision of usb patches is fine.  I'll go stick them into the
> next usb pull request ...
>
>> Your VIA patches are orthogonal, so I'm queuing them (1, 2, 4
>> and extra user_creatable) via mips-next.
>
> .. unless someone else is faster with merging ;)
>
> Feel free to add my "Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>"
> to the usb patches and merge the complete pack via mips-next.

Wait, the usb patch needs adding user_creatable = false too to prevent 
crashing when user adds the device without having function 0. I'll resend 
with that.

Regards,
BALATON Zoltan