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

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

[Shmulik Ladkani]

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

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;
 }
-- 
1.9.1

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

[Marcel Apfelbaum]

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

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

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

[Shmulik Ladkani]

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

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

[Shmulik Ladkani]

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

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

[Marcel Apfelbaum]

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

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

[Marcel Apfelbaum]

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
>

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

[Shmulik Ladkani]

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