From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33918) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z59le-0004de-0y for qemu-devel@nongnu.org; Wed, 17 Jun 2015 05:36:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z59lb-0005bl-8f for qemu-devel@nongnu.org; Wed, 17 Jun 2015 05:36:21 -0400 Received: from mail-wg0-x22f.google.com ([2a00:1450:400c:c00::22f]:33866) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z59la-0005bR-UD for qemu-devel@nongnu.org; Wed, 17 Jun 2015 05:36:19 -0400 Received: by wgv5 with SMTP id 5so31930347wgv.1 for ; Wed, 17 Jun 2015 02:36:18 -0700 (PDT) Message-ID: <55813F8E.1000105@gmail.com> Date: Wed, 17 Jun 2015 12:36:14 +0300 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1434443079-25755-1-git-send-email-shmulik.ladkani@ravellosystems.com> In-Reply-To: <1434443079-25755-1-git-send-email-shmulik.ladkani@ravellosystems.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1] pci: Don't register a specialized 'config_write' if default behavior is intended Reply-To: marcel@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shmulik Ladkani , qemu-devel@nongnu.org, "Michael S. Tsirkin" Cc: Leonid Shatz , Idan Brown , Hannes Reinecke 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 > Signed-off-by: Shmulik Ladkani > --- 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 Thanks, Marcel