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

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