Re: [Qemu-devel] [PATCH v1] pci: Don't register a specialized 'config_write' if default behavior is intended

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 06/17/2015 12:36 PM, Marcel Apfelbaum wrote:
> On 06/16/2015 11:24 AM, Shmulik Ladkani wrote:
>> Few devices have their specialized 'config_write' methods which simply
>> call 'pci_default_write_config' followed by a 'msix_write_config' or
>> 'msi_write_config' calls, using exact same arguments.
>>
>> This is unnecessary as 'pci_default_write_config' already invokes
>> 'msi_write_config' and 'msix_write_config'.
>>
>> Also, since 'pci_default_write_config' is the default 'config_write'
>> handler, we can simply avoid the registration of these specialized
>> versions.
>>
>> Cc: Leonid Shatz <leonid.shatz@ravellosystems.com>
>> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
>> ---
> Hi,
>
>>
>> NOTE:
>> Not sure if my statement regarding ommitting 'config_write' holds for
>> the megasas case:
>> It's parent is TYPE_MEGASAS_BASE whose parent is TYPE_PCI_DEVICE.
>> Can we assume 'config_write' will be set to 'pci_default_write_config'
>> in this case?
> No need to assume here, you can simply add a trace and check.
> However, the do_pci_register_device method assigns config_write method
> to PCIDevice *instances* using the class method or the default pci_default_write_config.
>
> Since TYPE_MEGASAS_BASE does not define a config_write method, the field will remain NULL.
> Anyway, you are welcomed to run it and double-check.

BTW, did you notice a bug here?  If yes, can you elaborate?
Thanks,
Marcel

>
>>
>>   hw/misc/ivshmem.c    | 1 -
>>   hw/net/vmxnet3.c     | 9 ---------
>>   hw/scsi/megasas.c    | 8 --------
>>   hw/scsi/vmw_pvscsi.c | 8 --------
>>   4 files changed, 26 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 5d272c8..231c35f 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -698,7 +698,6 @@ static void ivshmem_write_config(PCIDevice *pci_dev, uint32_t address,
>>                    uint32_t val, int len)
>>   {
>>       pci_default_write_config(pci_dev, address, val, len);
>> -    msix_write_config(pci_dev, address, val, len);
>>   }
>>
>>   static int pci_ivshmem_init(PCIDevice *dev)
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 8bcdf3e..104a0f5 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -2477,14 +2477,6 @@ static const VMStateDescription vmstate_vmxnet3 = {
>>       }
>>   };
>>
>> -static void
>> -vmxnet3_write_config(PCIDevice *pci_dev, uint32_t addr, uint32_t val, int len)
>> -{
>> -    pci_default_write_config(pci_dev, addr, val, len);
>> -    msix_write_config(pci_dev, addr, val, len);
>> -    msi_write_config(pci_dev, addr, val, len);
>> -}
>> -
>>   static Property vmxnet3_properties[] = {
>>       DEFINE_NIC_PROPERTIES(VMXNET3State, conf),
>>       DEFINE_PROP_END_OF_LIST(),
>> @@ -2503,7 +2495,6 @@ static void vmxnet3_class_init(ObjectClass *class, void *data)
>>       c->class_id = PCI_CLASS_NETWORK_ETHERNET;
>>       c->subsystem_vendor_id = PCI_VENDOR_ID_VMWARE;
>>       c->subsystem_id = PCI_DEVICE_ID_VMWARE_VMXNET3;
>> -    c->config_write = vmxnet3_write_config,
>>       dc->desc = "VMWare Paravirtualized Ethernet v3";
>>       dc->reset = vmxnet3_qdev_reset;
>>       dc->vmsd = &vmstate_vmxnet3;
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 91a5d97..51ba9e0 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -2407,13 +2407,6 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>>       }
>>   }
>>
>> -static void
>> -megasas_write_config(PCIDevice *pci, uint32_t addr, uint32_t val, int len)
>> -{
>> -    pci_default_write_config(pci, addr, val, len);
>> -    msi_write_config(pci, addr, val, len);
>> -}
>> -
>>   static Property megasas_properties_gen1[] = {
>>       DEFINE_PROP_UINT32("max_sge", MegasasState, fw_sge,
>>                          MEGASAS_DEFAULT_SGE),
>> @@ -2516,7 +2509,6 @@ static void megasas_class_init(ObjectClass *oc, void *data)
>>       dc->vmsd = info->vmsd;
>>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>       dc->desc = info->desc;
>> -    pc->config_write = megasas_write_config;
>>   }
>>
>>   static const TypeInfo megasas_info = {
>> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
>> index c6148d3..9c71f31 100644
>> --- a/hw/scsi/vmw_pvscsi.c
>> +++ b/hw/scsi/vmw_pvscsi.c
>> @@ -1174,13 +1174,6 @@ static const VMStateDescription vmstate_pvscsi = {
>>       }
>>   };
>>
>> -static void
>> -pvscsi_write_config(PCIDevice *pci, uint32_t addr, uint32_t val, int len)
>> -{
>> -    pci_default_write_config(pci, addr, val, len);
>> -    msi_write_config(pci, addr, val, len);
>> -}
>> -
>>   static Property pvscsi_properties[] = {
>>       DEFINE_PROP_UINT8("use_msg", PVSCSIState, use_msg, 1),
>>       DEFINE_PROP_END_OF_LIST(),
>> @@ -1202,7 +1195,6 @@ static void pvscsi_class_init(ObjectClass *klass, void *data)
>>       dc->vmsd = &vmstate_pvscsi;
>>       dc->props = pvscsi_properties;
>>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>> -    k->config_write = pvscsi_write_config;
>>       hc->unplug = pvscsi_hot_unplug;
>>       hc->plug = pvscsi_hotplug;
>>   }
>>
>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>
> Thanks,
> Marcel
>
>
>