[PATCH 0/6] VIA and general PCI IDE cleanup

[PATCH 0/6] VIA and general PCI IDE cleanup

[Bernhard Beschow]

This series is split off from a more general PCI IDE refactoring aiming for a
common implementation of the PCI IDE controller specification for all
TYPE_PCI_IDE models [1].

The first three patches resolve a circular dependency between the VIA IDE
controller and its south bridge. The next two patches resolves redundant code
accross all TYPE_PCI_IDE models. The last patch modernizes VM state setup in
PIIX IDE.

Testing done:
* `make check`
* `make check-avocado`
* `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device \
   ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso \
   -bios pegasos2.rom`
   The machine booted successfully and a startup sound was hearable
* `qemu-system-ppc -machine sam460ex -rtc base=localtime -drive \
   if=none,id=cd,file=morphos-3.17.iso,format=raw -device \
   ide-cd,drive=cd,bus=ide.1`
   The machine booted successfully into graphical desktop environment

Changes since [1]:
* Turn legacy IRQs into named GPIOs (Mark)
* Don't make VIA IDE legacy IRQs routable; just wire up in host device (Zoltan)
* Rename extracted bmdma_clear_status() (Zoltan)
   ... to bmdma_status_writeb() (Mark)

[1] https://lore.kernel.org/qemu-devel/20230422150728.176512-1-shentey@gmail.com/

Bernhard Beschow (6):
  hw/ide/pci: Expose legacy interrupts as named GPIOs
  hw/ide/via: Wire up IDE legacy interrupts in host device
  hw/isa/vt82c686: Remove via_isa_set_irq()
  hw/ide: Extract IDEBus assignment into bmdma_init()
  hw/ide: Extract bmdma_status_writeb()
  hw/ide/piix: Move registration of VMStateDescription to DeviceClass

 include/hw/ide/pci.h      |  1 +
 include/hw/isa/vt82c686.h |  2 --
 hw/ide/cmd646.c           |  3 +--
 hw/ide/pci.c              | 15 +++++++++++++++
 hw/ide/piix.c             |  8 +++-----
 hw/ide/sii3112.c          |  7 ++-----
 hw/ide/via.c              |  9 +++++----
 hw/isa/vt82c686.c         | 11 +++++------
 8 files changed, 32 insertions(+), 24 deletions(-)

-- 
2.40.1

[PATCH 1/6] hw/ide/pci: Expose legacy interrupts as named GPIOs

[Bernhard Beschow]

Exposing the legacy IDE interrupts as GPIOs allows them to be connected in the
parent device through qdev_connect_gpio_out(), i.e. without accessing private
data of TYPE_PCI_IDE.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index fc9224bbc9..9a5a7089d4 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -522,10 +522,19 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
     bm->pci_dev = d;
 }
 
+static void pci_ide_init(Object *obj)
+{
+    PCIIDEState *d = PCI_IDE(obj);
+
+    qdev_init_gpio_out_named(DEVICE(d), d->isa_irq, "isa-irq",
+                             ARRAY_SIZE(d->isa_irq));
+}
+
 static const TypeInfo pci_ide_type_info = {
     .name = TYPE_PCI_IDE,
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIIDEState),
+    .instance_init = pci_ide_init,
     .abstract = true,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-- 
2.40.1

[PATCH 2/6] hw/ide/via: Wire up IDE legacy interrupts in host device

[Bernhard Beschow]

Resolves circular depencency between IDE function and south bridge.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/via.c      | 6 ++++--
 hw/isa/vt82c686.c | 5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 177baea9a7..0caae52276 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -31,6 +31,7 @@
 #include "sysemu/dma.h"
 #include "hw/isa/vt82c686.h"
 #include "hw/ide/pci.h"
+#include "hw/irq.h"
 #include "trace.h"
 
 static uint64_t bmdma_read(void *opaque, hwaddr addr,
@@ -104,7 +105,8 @@ static void bmdma_setup_bar(PCIIDEState *d)
 
 static void via_ide_set_irq(void *opaque, int n, int level)
 {
-    PCIDevice *d = PCI_DEVICE(opaque);
+    PCIIDEState *s = opaque;
+    PCIDevice *d = PCI_DEVICE(s);
 
     if (level) {
         d->config[0x70 + n * 8] |= 0x80;
@@ -112,7 +114,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
         d->config[0x70 + n * 8] &= ~0x80;
     }
 
-    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
+    qemu_set_irq(s->isa_irq[n], level);
 }
 
 static void via_ide_reset(DeviceState *dev)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index ca89119ce0..8016c71315 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -692,6 +692,10 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
         return;
     }
+    for (i = 0; i < 2; i++) {
+        qdev_connect_gpio_out_named(DEVICE(&s->ide), "isa-irq", i,
+                                    s->isa_irqs_in[14 + i]);
+    }
 
     /* Functions 2-3: USB Ports */
     for (i = 0; i < ARRAY_SIZE(s->uhci); i++) {
@@ -814,6 +818,7 @@ static void vt8231_isa_reset(DeviceState *dev)
                  PCI_COMMAND_MASTER | PCI_COMMAND_SPECIAL);
     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
 
+    pci_conf[0x4c] = 0x04; /* IDE interrupt Routing */
     pci_conf[0x58] = 0x40; /* Miscellaneous Control 0 */
     pci_conf[0x67] = 0x08; /* Fast IR Config */
     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
-- 
2.40.1

[PATCH 3/6] hw/isa/vt82c686: Remove via_isa_set_irq()

[Bernhard Beschow]

Now that via_isa_set_irq() is unused it can be removed.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/hw/isa/vt82c686.h | 2 --
 hw/isa/vt82c686.c         | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index da1722daf2..b6e95b2851 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -34,6 +34,4 @@ struct ViaAC97State {
     uint32_t ac97_cmd;
 };
 
-void via_isa_set_irq(PCIDevice *d, int n, int level);
-
 #endif
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8016c71315..57bdfb4e78 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -592,12 +592,6 @@ 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_in[n], level);
-}
-
 static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
 {
     ViaISAState *s = opaque;
-- 
2.40.1

[PATCH 4/6] hw/ide: Extract IDEBus assignment into bmdma_init()

[Bernhard Beschow]

Every invocation of bmdma_init() is followed by `d->bmdma[i].bus = &d->bus[i]`.
Resolve this redundancy by extracting it into bmdma_init().

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c  | 1 -
 hw/ide/pci.c     | 1 +
 hw/ide/piix.c    | 1 -
 hw/ide/sii3112.c | 1 -
 hw/ide/via.c     | 1 -
 5 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a68357c1c5..a094a6e12a 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -297,7 +297,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
         ide_bus_init_output_irq(&d->bus[i], qdev_get_gpio_in(ds, i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
-        d->bmdma[i].bus = &d->bus[i];
         ide_bus_register_restart_cb(&d->bus[i]);
     }
 }
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 9a5a7089d4..4cf1e9c679 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -519,6 +519,7 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
     bus->dma = &bm->dma;
     bm->irq = bus->irq;
     bus->irq = qemu_allocate_irq(bmdma_irq, bm, 0);
+    bm->bus = bus;
     bm->pci_dev = d;
 }
 
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 41d60921e3..a32f7ccece 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -144,7 +144,6 @@ static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, Error **errp)
     ide_bus_init_output_irq(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
 
     bmdma_init(&d->bus[i], &d->bmdma[i], d);
-    d->bmdma[i].bus = &d->bus[i];
     ide_bus_register_restart_cb(&d->bus[i]);
 
     return true;
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index f9becdff8e..5dd3d03c29 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -287,7 +287,6 @@ static void sii3112_pci_realize(PCIDevice *dev, Error **errp)
         ide_bus_init_output_irq(&s->bus[i], qdev_get_gpio_in(ds, i));
 
         bmdma_init(&s->bus[i], &s->bmdma[i], s);
-        s->bmdma[i].bus = &s->bus[i];
         ide_bus_register_restart_cb(&s->bus[i]);
     }
 }
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 0caae52276..91253fa4ef 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -196,7 +196,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
         ide_bus_init_output_irq(&d->bus[i], qdev_get_gpio_in(ds, i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
-        d->bmdma[i].bus = &d->bus[i];
         ide_bus_register_restart_cb(&d->bus[i]);
     }
 }
-- 
2.40.1

[PATCH 5/6] hw/ide: Extract bmdma_status_writeb()

[Bernhard Beschow]

Every TYPE_PCI_IDE device performs the same not-so-trivial bit manipulation by
copy'n'paste code. Extract this into bmdma_status_writeb(), mirroring
bmdma_cmd_writeb().

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/ide/pci.h | 1 +
 hw/ide/cmd646.c      | 2 +-
 hw/ide/pci.c         | 5 +++++
 hw/ide/piix.c        | 2 +-
 hw/ide/sii3112.c     | 6 ++----
 hw/ide/via.c         | 2 +-
 6 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 74c127e32f..1ff469de87 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -58,6 +58,7 @@ struct PCIIDEState {
 
 void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
 void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
+void bmdma_status_writeb(BMDMAState *bm, uint32_t val);
 extern MemoryRegionOps bmdma_addr_ioport_ops;
 void pci_ide_create_devs(PCIDevice *dev);
 
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a094a6e12a..cabe9048b1 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
         cmd646_update_irq(pci_dev);
         break;
     case 2:
-        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
+        bmdma_status_writeb(bm, val);
         break;
     case 3:
         if (bm == &bm->pci_dev->bmdma[0]) {
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4cf1e9c679..b258fd2f50 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -318,6 +318,11 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
     bm->cmd = val & 0x09;
 }
 
+void bmdma_status_writeb(BMDMAState *bm, uint32_t val)
+{
+    bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06);
+}
+
 static uint64_t bmdma_addr_read(void *opaque, hwaddr addr,
                                 unsigned width)
 {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a32f7ccece..47e0b474c3 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
         bmdma_cmd_writeb(bm, val);
         break;
     case 2:
-        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
+        bmdma_status_writeb(bm, val);
         break;
     }
 }
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 5dd3d03c29..63dc4a0494 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -149,8 +149,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
         break;
     case 0x02:
     case 0x12:
-        d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) |
-                               (d->i.bmdma[0].status & ~val & 6);
+        bmdma_status_writeb(&d->i.bmdma[0], val);
         break;
     case 0x04 ... 0x07:
         bmdma_addr_ioport_ops.write(&d->i.bmdma[0], addr - 4, val, size);
@@ -165,8 +164,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
         break;
     case 0x0a:
     case 0x1a:
-        d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) |
-                               (d->i.bmdma[1].status & ~val & 6);
+        bmdma_status_writeb(&d->i.bmdma[1], val);
         break;
     case 0x0c ... 0x0f:
         bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size);
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 91253fa4ef..fff23803a6 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
         bmdma_cmd_writeb(bm, val);
         break;
     case 2:
-        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
+        bmdma_status_writeb(bm, val);
         break;
     default:;
     }
-- 
2.40.1

[PATCH 6/6] hw/ide/piix: Move registration of VMStateDescription to DeviceClass

[Bernhard Beschow]

The modern, declarative way to set up VM state handling is to assign to
DeviceClass::vmsd attribute.

There shouldn't be any change in behavior since dc->vmsd causes
vmstate_register_with_alias_id() to be called on the instance during
the instance init phase. vmstate_register() was also called during the
instance init phase which forwards to vmstate_register_with_alias_id()
internally. Checking the migration schema before and after this patch confirms:

before:
> qemu-system-x86_64 -S
> qemu > migrate -d exec:cat>before.mig

after:
> qemu-system-x86_64 -S
> qemu > migrate -d exec:cat>after.mig

> analyze-migration.py -d desc -f before.mig > before.json
> analyze-migration.py -d desc -f after.mig > after.json
> diff before.json after.json
-> empty
---
 hw/ide/piix.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 47e0b474c3..151f206046 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -28,7 +28,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "hw/pci/pci.h"
 #include "hw/ide/piix.h"
@@ -159,8 +158,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
-    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
-
     for (unsigned i = 0; i < 2; i++) {
         if (!pci_piix_init_bus(d, i, errp)) {
             return;
@@ -186,6 +183,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     dc->reset = piix_ide_reset;
+    dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_piix_ide_realize;
     k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
@@ -208,6 +206,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     dc->reset = piix_ide_reset;
+    dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_piix_ide_realize;
     k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
-- 
2.40.1

Re: [PATCH 2/6] hw/ide/via: Wire up IDE legacy interrupts in host device

[BALATON Zoltan]

On Sun, 21 May 2023, Bernhard Beschow wrote:
> Resolves circular depencency between IDE function and south bridge.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/ide/via.c      | 6 ++++--
> hw/isa/vt82c686.c | 5 +++++
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 177baea9a7..0caae52276 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -31,6 +31,7 @@
> #include "sysemu/dma.h"
> #include "hw/isa/vt82c686.h"
> #include "hw/ide/pci.h"
> +#include "hw/irq.h"
> #include "trace.h"
>
> static uint64_t bmdma_read(void *opaque, hwaddr addr,
> @@ -104,7 +105,8 @@ static void bmdma_setup_bar(PCIIDEState *d)
>
> static void via_ide_set_irq(void *opaque, int n, int level)
> {
> -    PCIDevice *d = PCI_DEVICE(opaque);
> +    PCIIDEState *s = opaque;
> +    PCIDevice *d = PCI_DEVICE(s);

These are the same structure so can be cast into each other but for 
consistency it's better to also change

qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));

to pass the PCIIDEState so d instead of ds in via_ide_realize().

Regards,
BALATON Zoltan

>     if (level) {
>         d->config[0x70 + n * 8] |= 0x80;
> @@ -112,7 +114,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
>         d->config[0x70 + n * 8] &= ~0x80;
>     }
>
> -    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
> +    qemu_set_irq(s->isa_irq[n], level);
> }
>
> static void via_ide_reset(DeviceState *dev)
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index ca89119ce0..8016c71315 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -692,6 +692,10 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
>         return;
>     }
> +    for (i = 0; i < 2; i++) {
> +        qdev_connect_gpio_out_named(DEVICE(&s->ide), "isa-irq", i,
> +                                    s->isa_irqs_in[14 + i]);
> +    }
>
>     /* Functions 2-3: USB Ports */
>     for (i = 0; i < ARRAY_SIZE(s->uhci); i++) {
> @@ -814,6 +818,7 @@ static void vt8231_isa_reset(DeviceState *dev)
>                  PCI_COMMAND_MASTER | PCI_COMMAND_SPECIAL);
>     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
>
> +    pci_conf[0x4c] = 0x04; /* IDE interrupt Routing */
>     pci_conf[0x58] = 0x40; /* Miscellaneous Control 0 */
>     pci_conf[0x67] = 0x08; /* Fast IR Config */
>     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
>

Re: [PATCH 5/6] hw/ide: Extract bmdma_status_writeb()

[BALATON Zoltan]

On Sun, 21 May 2023, Bernhard Beschow wrote:
> Every TYPE_PCI_IDE device performs the same not-so-trivial bit manipulation by
> copy'n'paste code. Extract this into bmdma_status_writeb(), mirroring
> bmdma_cmd_writeb().
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

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

Re: [PATCH 6/6] hw/ide/piix: Move registration of VMStateDescription to DeviceClass

[BALATON Zoltan]

On Sun, 21 May 2023, Bernhard Beschow wrote:
> The modern, declarative way to set up VM state handling is to assign to
> DeviceClass::vmsd attribute.
>
> There shouldn't be any change in behavior since dc->vmsd causes
> vmstate_register_with_alias_id() to be called on the instance during
> the instance init phase. vmstate_register() was also called during the
> instance init phase which forwards to vmstate_register_with_alias_id()
> internally. Checking the migration schema before and after this patch confirms:
>
> before:
>> qemu-system-x86_64 -S
>> qemu > migrate -d exec:cat>before.mig
>
> after:
>> qemu-system-x86_64 -S
>> qemu > migrate -d exec:cat>after.mig
>
>> analyze-migration.py -d desc -f before.mig > before.json
>> analyze-migration.py -d desc -f after.mig > after.json
>> diff before.json after.json
> -> empty

Missing Signed-off-by line.

Regards,
BALATON Zoltah

> ---
> hw/ide/piix.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 47e0b474c3..151f206046 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -28,7 +28,6 @@
>  */
>
> #include "qemu/osdep.h"
> -#include "migration/vmstate.h"
> #include "qapi/error.h"
> #include "hw/pci/pci.h"
> #include "hw/ide/piix.h"
> @@ -159,8 +158,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>     bmdma_setup_bar(d);
>     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>
> -    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
> -
>     for (unsigned i = 0; i < 2; i++) {
>         if (!pci_piix_init_bus(d, i, errp)) {
>             return;
> @@ -186,6 +183,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
>     dc->reset = piix_ide_reset;
> +    dc->vmsd = &vmstate_ide_pci;
>     k->realize = pci_piix_ide_realize;
>     k->exit = pci_piix_ide_exitfn;
>     k->vendor_id = PCI_VENDOR_ID_INTEL;
> @@ -208,6 +206,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
>     dc->reset = piix_ide_reset;
> +    dc->vmsd = &vmstate_ide_pci;
>     k->realize = pci_piix_ide_realize;
>     k->exit = pci_piix_ide_exitfn;
>     k->vendor_id = PCI_VENDOR_ID_INTEL;
>

Re: [PATCH 2/6] hw/ide/via: Wire up IDE legacy interrupts in host device

[BALATON Zoltan]

On Sun, 21 May 2023, BALATON Zoltan wrote:
> On Sun, 21 May 2023, Bernhard Beschow wrote:
>> Resolves circular depencency between IDE function and south bridge.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/ide/via.c      | 6 ++++--
>> hw/isa/vt82c686.c | 5 +++++
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 177baea9a7..0caae52276 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -31,6 +31,7 @@
>> #include "sysemu/dma.h"
>> #include "hw/isa/vt82c686.h"
>> #include "hw/ide/pci.h"
>> +#include "hw/irq.h"
>> #include "trace.h"
>> 
>> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>> @@ -104,7 +105,8 @@ static void bmdma_setup_bar(PCIIDEState *d)
>> 
>> static void via_ide_set_irq(void *opaque, int n, int level)
>> {
>> -    PCIDevice *d = PCI_DEVICE(opaque);
>> +    PCIIDEState *s = opaque;
>> +    PCIDevice *d = PCI_DEVICE(s);
>
> These are the same structure so can be cast into each other but for 
> consistency it's better to also change
>
> qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));
>
> to pass the PCIIDEState so d instead of ds in via_ide_realize().

Ignore this. That function takes a DeviceState and seems no separate 
argument for the opaque pointer so no change is needed.

Regards,
BALATON Zoltan

Re: [PATCH 5/6] hw/ide: Extract bmdma_status_writeb()

[Mark Cave-Ayland]

On 21/05/2023 12:15, Bernhard Beschow wrote:
> Every TYPE_PCI_IDE device performs the same not-so-trivial bit manipulation by
> copy'n'paste code. Extract this into bmdma_status_writeb(), mirroring
> bmdma_cmd_writeb().
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/ide/pci.h | 1 +
>   hw/ide/cmd646.c      | 2 +-
>   hw/ide/pci.c         | 5 +++++
>   hw/ide/piix.c        | 2 +-
>   hw/ide/sii3112.c     | 6 ++----
>   hw/ide/via.c         | 2 +-
>   6 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index 74c127e32f..1ff469de87 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -58,6 +58,7 @@ struct PCIIDEState {
>   
>   void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
>   void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
> +void bmdma_status_writeb(BMDMAState *bm, uint32_t val);
>   extern MemoryRegionOps bmdma_addr_ioport_ops;
>   void pci_ide_create_devs(PCIDevice *dev);
>   
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index a094a6e12a..cabe9048b1 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
>           cmd646_update_irq(pci_dev);
>           break;
>       case 2:
> -        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
> +        bmdma_status_writeb(bm, val);
>           break;
>       case 3:
>           if (bm == &bm->pci_dev->bmdma[0]) {
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 4cf1e9c679..b258fd2f50 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -318,6 +318,11 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>       bm->cmd = val & 0x09;
>   }
>   
> +void bmdma_status_writeb(BMDMAState *bm, uint32_t val)
> +{
> +    bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06);

Does this fit the 80 char limit if you run the series through scripts/checkpatch.pl?

> +}
> +
>   static uint64_t bmdma_addr_read(void *opaque, hwaddr addr,
>                                   unsigned width)
>   {
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index a32f7ccece..47e0b474c3 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
>           bmdma_cmd_writeb(bm, val);
>           break;
>       case 2:
> -        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
> +        bmdma_status_writeb(bm, val);
>           break;
>       }
>   }
> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> index 5dd3d03c29..63dc4a0494 100644
> --- a/hw/ide/sii3112.c
> +++ b/hw/ide/sii3112.c
> @@ -149,8 +149,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
>           break;
>       case 0x02:
>       case 0x12:
> -        d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) |
> -                               (d->i.bmdma[0].status & ~val & 6);
> +        bmdma_status_writeb(&d->i.bmdma[0], val);
>           break;
>       case 0x04 ... 0x07:
>           bmdma_addr_ioport_ops.write(&d->i.bmdma[0], addr - 4, val, size);
> @@ -165,8 +164,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
>           break;
>       case 0x0a:
>       case 0x1a:
> -        d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) |
> -                               (d->i.bmdma[1].status & ~val & 6);
> +        bmdma_status_writeb(&d->i.bmdma[1], val);
>           break;
>       case 0x0c ... 0x0f:
>           bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size);
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 91253fa4ef..fff23803a6 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
>           bmdma_cmd_writeb(bm, val);
>           break;
>       case 2:
> -        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
> +        bmdma_status_writeb(bm, val);
>           break;
>       default:;
>       }

Otherwise looks good to me:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH 6/6] hw/ide/piix: Move registration of VMStateDescription to DeviceClass

[Mark Cave-Ayland]

On 21/05/2023 12:15, Bernhard Beschow wrote:

> The modern, declarative way to set up VM state handling is to assign to
> DeviceClass::vmsd attribute.
> 
> There shouldn't be any change in behavior since dc->vmsd causes
> vmstate_register_with_alias_id() to be called on the instance during
> the instance init phase. vmstate_register() was also called during the
> instance init phase which forwards to vmstate_register_with_alias_id()
> internally. Checking the migration schema before and after this patch confirms:
> 
> before:
>> qemu-system-x86_64 -S
>> qemu > migrate -d exec:cat>before.mig
> 
> after:
>> qemu-system-x86_64 -S
>> qemu > migrate -d exec:cat>after.mig
> 
>> analyze-migration.py -d desc -f before.mig > before.json
>> analyze-migration.py -d desc -f after.mig > after.json
>> diff before.json after.json
> -> empty
> ---
>   hw/ide/piix.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 47e0b474c3..151f206046 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -28,7 +28,6 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "migration/vmstate.h"
>   #include "qapi/error.h"
>   #include "hw/pci/pci.h"
>   #include "hw/ide/piix.h"
> @@ -159,8 +158,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>       bmdma_setup_bar(d);
>       pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>   
> -    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
> -
>       for (unsigned i = 0; i < 2; i++) {
>           if (!pci_piix_init_bus(d, i, errp)) {
>               return;
> @@ -186,6 +183,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>   
>       dc->reset = piix_ide_reset;
> +    dc->vmsd = &vmstate_ide_pci;
>       k->realize = pci_piix_ide_realize;
>       k->exit = pci_piix_ide_exitfn;
>       k->vendor_id = PCI_VENDOR_ID_INTEL;
> @@ -208,6 +206,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>   
>       dc->reset = piix_ide_reset;
> +    dc->vmsd = &vmstate_ide_pci;
>       k->realize = pci_piix_ide_realize;
>       k->exit = pci_piix_ide_exitfn;
>       k->vendor_id = PCI_VENDOR_ID_INTEL;

Thanks for confirming that the migration still works!

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.