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

[Marcel Apfelbaum]

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 <address@hidden>
> > Signed-off-by: Shmulik Ladkani <address@hidden>
> > ---
> 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 <address@hidden>
>
> Thanks,
> Marcel