From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34761) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z59mk-00068x-74 for qemu-devel@nongnu.org; Wed, 17 Jun 2015 05:37:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z59mc-0006IO-CE for qemu-devel@nongnu.org; Wed, 17 Jun 2015 05:37:30 -0400 Received: from mail-wi0-x22d.google.com ([2a00:1450:400c:c05::22d]:35451) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z59mb-0006HJ-LX for qemu-devel@nongnu.org; Wed, 17 Jun 2015 05:37:22 -0400 Received: by wiga1 with SMTP id a1so133350612wig.0 for ; Wed, 17 Jun 2015 02:37:21 -0700 (PDT) Message-ID: <55813FCE.2090405@gmail.com> Date: Wed, 17 Jun 2015 12:37:18 +0300 From: Marcel Apfelbaum MIME-Version: 1.0 References: <1434443079-25755-1-git-send-email-shmulik.ladkani@ravellosystems.com> <55813F8E.1000105@gmail.com> In-Reply-To: <55813F8E.1000105@gmail.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/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 >> 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. 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 > > Thanks, > Marcel > > >