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

[Bernhard Beschow]

Am 26. April 2023 11:04:30 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
>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.

I'll completely rework this patch.

>
>> ---
>>   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.