qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 05/13] hw/ide: Extract pci_ide_class_init()

From: Mark Cave-Ayland
Subject: Re: [PATCH 05/13] hw/ide: Extract pci_ide_class_init()
Date: Wed, 26 Apr 2023 12:04:30 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 
On 22/04/2023 16:07, Bernhard Beschow wrote:


Resolves redundant code in every PCI IDE device model.
I think this needs to mention that it's moving the PCIDeviceClass::exit() function from all of the PCI IDE controller implementations to a common implementation in the parent PCI_IDE type. 


---
  include/hw/ide/pci.h |  1 -
  hw/ide/cmd646.c      | 15 ---------------
  hw/ide/pci.c         | 25 ++++++++++++++++++++++++-
  hw/ide/piix.c        | 19 -------------------
  hw/ide/sii3112.c     |  3 ++-
  hw/ide/via.c         | 15 ---------------
  6 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 74c127e32f..7bc4e53d02 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -61,7 +61,6 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
-extern const VMStateDescription vmstate_ide_pci; 
  extern const MemoryRegionOps pci_ide_cmd_le_ops;
  extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a094a6e12a..9aabf80e52 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -301,17 +301,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
      }
  }
-static void pci_cmd646_ide_exitfn(PCIDevice *dev) 
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < 2; ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
  static Property cmd646_ide_properties[] = {
      DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
      DEFINE_PROP_END_OF_LIST(),
@@ -323,17 +312,13 @@ static void cmd646_ide_class_init(ObjectClass *klass, 
void *data)
      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
dc->reset = cmd646_reset; 
-    dc->vmsd = &vmstate_ide_pci;
      k->realize = pci_cmd646_ide_realize;
-    k->exit = pci_cmd646_ide_exitfn;
      k->vendor_id = PCI_VENDOR_ID_CMD;
      k->device_id = PCI_DEVICE_ID_CMD_646;
      k->revision = 0x07;
-    k->class_id = PCI_CLASS_STORAGE_IDE;
      k->config_read = cmd646_pci_config_read;
      k->config_write = cmd646_pci_config_write;
      device_class_set_props(dc, cmd646_ide_properties);
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
  }
static const TypeInfo cmd646_ide_info = { 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 67e0998ff0..8bea92e394 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -467,7 +467,7 @@ static int ide_pci_post_load(void *opaque, int version_id)
      return 0;
  }
-const VMStateDescription vmstate_ide_pci = { 
+static const VMStateDescription vmstate_ide_pci = {
      .name = "ide",
      .version_id = 3,
      .minimum_version_id = 0,
@@ -530,11 +530,34 @@ static void pci_ide_init(Object *obj)
      qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq));
  }
+static void pci_ide_exitfn(PCIDevice *dev) 
+{
+    PCIIDEState *d = PCI_IDE(dev);
+    unsigned i;
+
+    for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
+        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
+        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
+    }
+}
+
+static void pci_ide_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_ide_pci;
+    k->exit = pci_ide_exitfn;
+    k->class_id = PCI_CLASS_STORAGE_IDE;
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+}
+
  static const TypeInfo pci_ide_type_info = {
      .name = TYPE_PCI_IDE,
      .parent = TYPE_PCI_DEVICE,
      .instance_size = sizeof(PCIIDEState),
      .instance_init = pci_ide_init,
+    .class_init = pci_ide_class_init,
      .abstract = true,
      .interfaces = (InterfaceInfo[]) {
          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a32f7ccece..4e6ca99123 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -159,8 +159,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); 
-
Presumably this still survives migration between a pre-series and post-series QEMU using the PIIX IDE controller? 


      for (unsigned i = 0; i < 2; i++) {
          if (!pci_piix_init_bus(d, i, errp)) {
              return;
@@ -168,17 +166,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
      }
  }
-static void pci_piix_ide_exitfn(PCIDevice *dev) 
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < 2; ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
  /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
  static void piix3_ide_class_init(ObjectClass *klass, void *data)
  {
@@ -187,11 +174,8 @@ static void piix3_ide_class_init(ObjectClass *klass, void 
*data)
dc->reset = piix_ide_reset; 
      k->realize = pci_piix_ide_realize;
-    k->exit = pci_piix_ide_exitfn;
      k->vendor_id = PCI_VENDOR_ID_INTEL;
      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
-    k->class_id = PCI_CLASS_STORAGE_IDE;
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
      dc->hotpluggable = false;
  }
@@ -209,11 +193,8 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data) dc->reset = piix_ide_reset; 
      k->realize = pci_piix_ide_realize;
-    k->exit = pci_piix_ide_exitfn;
      k->vendor_id = PCI_VENDOR_ID_INTEL;
      k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
-    k->class_id = PCI_CLASS_STORAGE_IDE;
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
      dc->hotpluggable = false;
  }
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c 
index 5dd3d03c29..0af897a9ef 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -301,9 +301,10 @@ static void sii3112_pci_class_init(ObjectClass *klass, 
void *data)
      pd->class_id = PCI_CLASS_STORAGE_RAID;
      pd->revision = 1;
      pd->realize = sii3112_pci_realize;
+    pd->exit = NULL;
      dc->reset = sii3112_reset;
+    dc->vmsd = NULL;
      dc->desc = "SiI3112A SATA controller";
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
  }
No need to set explicit NULLs here: class/object structures are all zeroed before init (unless you're deliberately trying to prevent the common PCIDeviceClass::exit() function from being called here temporarily?) 


  static const TypeInfo sii3112_pci_info = {
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 91253fa4ef..287143a005 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -200,34 +200,19 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
      }
  }
-static void via_ide_exitfn(PCIDevice *dev) 
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
  static void via_ide_class_init(ObjectClass *klass, void *data)
  {
      DeviceClass *dc = DEVICE_CLASS(klass);
      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
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;
      k->device_id = PCI_DEVICE_ID_VIA_IDE;
      k->revision = 0x06;
-    k->class_id = PCI_CLASS_STORAGE_IDE;
-    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
  }
static const TypeInfo via_ide_info = { 

A couple of queries, but generally looks good to me.


ATB,

Mark.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]