Message ID | 1434443079-25755-1-git-send-email-shmulik.ladkani@ravellosystems.com |
---|---|
State | New |
Headers | show |
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
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 > > >
Hi, On Wed, 17 Jun 2015 12:36:14 +0300, marcel.apfelbaum@gmail.com wrote: > > 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. Verified; do_pci_register_device indeed sets it to pci_default_write_config. Thanks, Shmulik
Hi,
On Wed, 17 Jun 2015 12:37:18 +0300, marcel.apfelbaum@gmail.com wrote:
> BTW, did you notice a bug here? If yes, can you elaborate?
No, not a direct bug.
We noticed this while working on related code areas.
There's some history behind this.
In 95d6580 'msi: Invoke msi/msix_write_config from PCI core', the calls
to msi[x]_write_config have been added into pci_default_write_config,
and many specialized 'config_write' methods have been eliminated.
However there was a bug in 95d6580 - the values written to msi/msix
were always 0.
This was recently fixed in d7efb7e
'pci: avoid losing config updates to MSI/MSIX cap regs'
I assume that device authors were either (1) unware of the
generalization, thus kept invoking msi[x]_write_config explicitly, or
(2) trying to overcome the "lost writes".
Anyway, I'm no PCI expert here, but I assume the side-effect invoking
msi[x]_write_config twice (explicitly from the specialized config_write,
then implicitly from pci_default_write_config) isn't desired.
Meaning, the suggested patch follows the spirit of 95d6580.
Let me know if my analysis is flawed.
Regards,
Shmulik
On 06/17/2015 09:46 PM, Shmulik Ladkani wrote: > Hi, > > On Wed, 17 Jun 2015 12:36:14 +0300, marcel.apfelbaum@gmail.com wrote: >>> 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. > > Verified; do_pci_register_device indeed sets it to pci_default_write_config. > > Thanks, > Shmulik > Cool! Thanks, Marcel
On 06/17/2015 10:17 PM, Shmulik Ladkani wrote: > Hi, > > On Wed, 17 Jun 2015 12:37:18 +0300, marcel.apfelbaum@gmail.com wrote: >> BTW, did you notice a bug here? If yes, can you elaborate? > > No, not a direct bug. > We noticed this while working on related code areas. > > There's some history behind this. > > In 95d6580 'msi: Invoke msi/msix_write_config from PCI core', the calls > to msi[x]_write_config have been added into pci_default_write_config, > and many specialized 'config_write' methods have been eliminated. > > However there was a bug in 95d6580 - the values written to msi/msix > were always 0. > This was recently fixed in d7efb7e > 'pci: avoid losing config updates to MSI/MSIX cap regs' Got it. > > I assume that device authors were either (1) unware of the > generalization, thus kept invoking msi[x]_write_config explicitly, or > (2) trying to overcome the "lost writes". > > Anyway, I'm no PCI expert here, but I assume the side-effect invoking > msi[x]_write_config twice (explicitly from the specialized config_write, > then implicitly from pci_default_write_config) isn't desired. Of course. > > Meaning, the suggested patch follows the spirit of 95d6580. > Let me know if my analysis is flawed. Thank you for the patch, you are completely right. My 'Reviewed-by' tag is there, I think Michael, the PCI maintainer, will take it shortly. Thanks, Marcel > > Regards, > Shmulik >
On Sun, 21 Jun 2015 11:20:18 +0300, marcel.apfelbaum@gmail.com wrote: > Thank you for the patch, you are completely right. > My 'Reviewed-by' tag is there, I think Michael, the PCI > maintainer, will take it shortly. Was already pulled ;-)
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; }
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> --- 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? 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(-)