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.
