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

[Bernhard Beschow]

Am 22. April 2023 17:34:30 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 22 Apr 2023, Bernhard Beschow wrote:
>> Resolves redundant code in every PCI IDE device model.
>
>This patch could be broken up a bit more as it seems to do unrelated changes. 
>Such as setting DEVICE_CATEGORY_STORAGE in a different way could be a separate 
>patch to make it simpler to review.

Okay, I'll slice this patch in the next iteration, moving each assignment 
separately.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> ---
>> 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);
>> -
>>     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);
>> }
>> 
>> 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 = {
>>