[Qemu-devel] [PATCH v3 0/2] change type of pci_bridge_initfn()

[Qemu-devel] [PATCH v3 0/2] change type of pci_bridge_initfn()

[Cao jin]

v3 changelog:
1. Just tweaked the commit message.

Test:
1. Compiled.
2. Did a hack to test dec bridge realization, prove it is ok after patch.
   Will reply this mail to attach the screenshot.

Cao jin (2):
  dec: convert to realize()
  change type of pci_bridge_initfn() to void

 hw/pci-bridge/dec.c                | 6 +++---
 hw/pci-bridge/i82801b11.c          | 5 +----
 hw/pci-bridge/ioh3420.c            | 6 +-----
 hw/pci-bridge/pci_bridge_dev.c     | 8 +++-----
 hw/pci-bridge/xio3130_downstream.c | 6 +-----
 hw/pci-bridge/xio3130_upstream.c   | 6 +-----
 hw/pci-host/apb.c                  | 5 +----
 hw/pci/pci_bridge.c                | 3 +--
 include/hw/pci/pci_bridge.h        | 2 +-
 9 files changed, 13 insertions(+), 34 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/2] dec: convert to realize()

[Cao jin]

Also because pci_bridge_initfn() can`t fail.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pci-bridge/dec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
index 28d0ff9..be0fc46 100644
--- a/hw/pci-bridge/dec.c
+++ b/hw/pci-bridge/dec.c
@@ -51,9 +51,9 @@ static int dec_map_irq(PCIDevice *pci_dev, int irq_num)
     return irq_num;
 }
 
-static int dec_pci_bridge_initfn(PCIDevice *pci_dev)
+static void dec_pci_bridge_realize(PCIDevice *pci_dev, Error **errp)
 {
-    return pci_bridge_initfn(pci_dev, TYPE_PCI_BUS);
+    pci_bridge_initfn(pci_dev, TYPE_PCI_BUS);
 }
 
 static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data)
@@ -61,7 +61,7 @@ static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = dec_pci_bridge_initfn;
+    k->realize = dec_pci_bridge_realize;
     k->exit = pci_bridge_exitfn;
     k->vendor_id = PCI_VENDOR_ID_DEC;
     k->device_id = PCI_DEVICE_ID_DEC_21154;
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 1/2] dec: convert to realize()

[Marcel Apfelbaum]

On 01/14/2016 02:23 PM, Cao jin wrote:
> Also because pci_bridge_initfn() can`t fail.
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/pci-bridge/dec.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c
> index 28d0ff9..be0fc46 100644
> --- a/hw/pci-bridge/dec.c
> +++ b/hw/pci-bridge/dec.c
> @@ -51,9 +51,9 @@ static int dec_map_irq(PCIDevice *pci_dev, int irq_num)
>       return irq_num;
>   }
>
> -static int dec_pci_bridge_initfn(PCIDevice *pci_dev)
> +static void dec_pci_bridge_realize(PCIDevice *pci_dev, Error **errp)
>   {
> -    return pci_bridge_initfn(pci_dev, TYPE_PCI_BUS);
> +    pci_bridge_initfn(pci_dev, TYPE_PCI_BUS);
>   }
>
>   static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data)
> @@ -61,7 +61,7 @@ static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->init = dec_pci_bridge_initfn;
> +    k->realize = dec_pci_bridge_realize;
>       k->exit = pci_bridge_exitfn;
>       k->vendor_id = PCI_VENDOR_ID_DEC;
>       k->device_id = PCI_DEVICE_ID_DEC_21154;
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

[Qemu-devel] [PATCH v3 2/2] change type of pci_bridge_initfn() to void

[Cao jin]

Since it can`t fail. Also modify the callers.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pci-bridge/i82801b11.c          | 5 +----
 hw/pci-bridge/ioh3420.c            | 6 +-----
 hw/pci-bridge/pci_bridge_dev.c     | 8 +++-----
 hw/pci-bridge/xio3130_downstream.c | 6 +-----
 hw/pci-bridge/xio3130_upstream.c   | 6 +-----
 hw/pci-host/apb.c                  | 5 +----
 hw/pci/pci_bridge.c                | 3 +--
 include/hw/pci/pci_bridge.h        | 2 +-
 8 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 7e79bc0..b21bc2c 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -61,10 +61,7 @@ static int i82801b11_bridge_initfn(PCIDevice *d)
 {
     int rc;
 
-    rc = pci_bridge_initfn(d, TYPE_PCI_BUS);
-    if (rc < 0) {
-        return rc;
-    }
+    pci_bridge_initfn(d, TYPE_PCI_BUS);
 
     rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
                                I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index cce2fdd..eead195 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -97,11 +97,7 @@ static int ioh3420_initfn(PCIDevice *d)
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
 
-    rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
-    if (rc < 0) {
-        return rc;
-    }
-
+    pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = pci_bridge_ssvid_init(d, IOH_EP_SSVID_OFFSET,
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 26aded9..bc3e1b7 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -52,10 +52,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
     PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
     int err;
 
-    err = pci_bridge_initfn(dev, TYPE_PCI_BUS);
-    if (err) {
-        goto bridge_error;
-    }
+    pci_bridge_initfn(dev, TYPE_PCI_BUS);
+
     if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
         dev->config[PCI_INTERRUPT_PIN] = 0x1;
         memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
@@ -94,7 +92,7 @@ slotid_error:
     }
 shpc_error:
     pci_bridge_exitfn(dev);
-bridge_error:
+
     return err;
 }
 
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index b3a6479..b4dd25f 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -60,11 +60,7 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     PCIESlot *s = PCIE_SLOT(d);
     int rc;
 
-    rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
-    if (rc < 0) {
-        return rc;
-    }
-
+    pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index eada582..434c8fd 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -56,11 +56,7 @@ static int xio3130_upstream_initfn(PCIDevice *d)
     PCIEPort *p = PCIE_PORT(d);
     int rc;
 
-    rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
-    if (rc < 0) {
-        return rc;
-    }
-
+    pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 599768e..e9117b9 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -636,10 +636,7 @@ static int apb_pci_bridge_initfn(PCIDevice *dev)
 {
     int rc;
 
-    rc = pci_bridge_initfn(dev, TYPE_PCI_BUS);
-    if (rc < 0) {
-        return rc;
-    }
+    pci_bridge_initfn(dev, TYPE_PCI_BUS);
 
     /*
      * command register:
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 40c97b1..5c30795 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -332,7 +332,7 @@ void pci_bridge_reset(DeviceState *qdev)
 }
 
 /* default qdev initialization function for PCI-to-PCI bridge */
-int pci_bridge_initfn(PCIDevice *dev, const char *typename)
+void pci_bridge_initfn(PCIDevice *dev, const char *typename)
 {
     PCIBus *parent = dev->bus;
     PCIBridge *br = PCI_BRIDGE(dev);
@@ -378,7 +378,6 @@ int pci_bridge_initfn(PCIDevice *dev, const char *typename)
     br->windows = pci_bridge_region_init(br);
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
-    return 0;
 }
 
 /* default qdev clean up function for PCI-to-PCI bridge */
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 93b621c..ed4aff6 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -48,7 +48,7 @@ void pci_bridge_disable_base_limit(PCIDevice *dev);
 void pci_bridge_reset_reg(PCIDevice *dev);
 void pci_bridge_reset(DeviceState *qdev);
 
-int pci_bridge_initfn(PCIDevice *pci_dev, const char *typename);
+void pci_bridge_initfn(PCIDevice *pci_dev, const char *typename);
 void pci_bridge_exitfn(PCIDevice *pci_dev);
 
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 2/2] change type of pci_bridge_initfn() to void

[Marcel Apfelbaum]

On 01/14/2016 02:23 PM, Cao jin wrote:
> Since it can`t fail. Also modify the callers.
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/pci-bridge/i82801b11.c          | 5 +----
>   hw/pci-bridge/ioh3420.c            | 6 +-----
>   hw/pci-bridge/pci_bridge_dev.c     | 8 +++-----
>   hw/pci-bridge/xio3130_downstream.c | 6 +-----
>   hw/pci-bridge/xio3130_upstream.c   | 6 +-----
>   hw/pci-host/apb.c                  | 5 +----
>   hw/pci/pci_bridge.c                | 3 +--
>   include/hw/pci/pci_bridge.h        | 2 +-
>   8 files changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index 7e79bc0..b21bc2c 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -61,10 +61,7 @@ static int i82801b11_bridge_initfn(PCIDevice *d)
>   {
>       int rc;
>
> -    rc = pci_bridge_initfn(d, TYPE_PCI_BUS);
> -    if (rc < 0) {
> -        return rc;
> -    }
> +    pci_bridge_initfn(d, TYPE_PCI_BUS);
>
>       rc = pci_bridge_ssvid_init(d, I82801ba_SSVID_OFFSET,
>                                  I82801ba_SSVID_SVID, I82801ba_SSVID_SSID);
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index cce2fdd..eead195 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -97,11 +97,7 @@ static int ioh3420_initfn(PCIDevice *d)
>       PCIESlot *s = PCIE_SLOT(d);
>       int rc;
>
> -    rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> -    if (rc < 0) {
> -        return rc;
> -    }
> -
> +    pci_bridge_initfn(d, TYPE_PCIE_BUS);
>       pcie_port_init_reg(d);
>
>       rc = pci_bridge_ssvid_init(d, IOH_EP_SSVID_OFFSET,
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 26aded9..bc3e1b7 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -52,10 +52,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>       PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>       int err;
>
> -    err = pci_bridge_initfn(dev, TYPE_PCI_BUS);
> -    if (err) {
> -        goto bridge_error;
> -    }
> +    pci_bridge_initfn(dev, TYPE_PCI_BUS);
> +
>       if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
>           dev->config[PCI_INTERRUPT_PIN] = 0x1;
>           memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
> @@ -94,7 +92,7 @@ slotid_error:
>       }
>   shpc_error:
>       pci_bridge_exitfn(dev);
> -bridge_error:
> +
>       return err;
>   }
>
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index b3a6479..b4dd25f 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -60,11 +60,7 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>       PCIESlot *s = PCIE_SLOT(d);
>       int rc;
>
> -    rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> -    if (rc < 0) {
> -        return rc;
> -    }
> -
> +    pci_bridge_initfn(d, TYPE_PCIE_BUS);
>       pcie_port_init_reg(d);
>
>       rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
> index eada582..434c8fd 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -56,11 +56,7 @@ static int xio3130_upstream_initfn(PCIDevice *d)
>       PCIEPort *p = PCIE_PORT(d);
>       int rc;
>
> -    rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> -    if (rc < 0) {
> -        return rc;
> -    }
> -
> +    pci_bridge_initfn(d, TYPE_PCIE_BUS);
>       pcie_port_init_reg(d);
>
>       rc = msi_init(d, XIO3130_MSI_OFFSET, XIO3130_MSI_NR_VECTOR,
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 599768e..e9117b9 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -636,10 +636,7 @@ static int apb_pci_bridge_initfn(PCIDevice *dev)
>   {
>       int rc;
>
> -    rc = pci_bridge_initfn(dev, TYPE_PCI_BUS);
> -    if (rc < 0) {
> -        return rc;
> -    }
> +    pci_bridge_initfn(dev, TYPE_PCI_BUS);
>

Hi,

It seems you don't need the rc variable here anymore, right?

This should break the build (unused local variable),
I suggest running configure with no params to check all the architectures,
(I am also running make check while at it, just to be sure)

Thanks,
Marcel

>       /*
>        * command register:
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 40c97b1..5c30795 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -332,7 +332,7 @@ void pci_bridge_reset(DeviceState *qdev)
>   }
>
>   /* default qdev initialization function for PCI-to-PCI bridge */
> -int pci_bridge_initfn(PCIDevice *dev, const char *typename)
> +void pci_bridge_initfn(PCIDevice *dev, const char *typename)
>   {
>       PCIBus *parent = dev->bus;
>       PCIBridge *br = PCI_BRIDGE(dev);
> @@ -378,7 +378,6 @@ int pci_bridge_initfn(PCIDevice *dev, const char *typename)
>       br->windows = pci_bridge_region_init(br);
>       QLIST_INIT(&sec_bus->child);
>       QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> -    return 0;
>   }
>
>   /* default qdev clean up function for PCI-to-PCI bridge */
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 93b621c..ed4aff6 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -48,7 +48,7 @@ void pci_bridge_disable_base_limit(PCIDevice *dev);
>   void pci_bridge_reset_reg(PCIDevice *dev);
>   void pci_bridge_reset(DeviceState *qdev);
>
> -int pci_bridge_initfn(PCIDevice *pci_dev, const char *typename);
> +void pci_bridge_initfn(PCIDevice *pci_dev, const char *typename);
>   void pci_bridge_exitfn(PCIDevice *pci_dev);
>
>
>

Re: [Qemu-devel] [PATCH v3 2/2] change type of pci_bridge_initfn() to void

[Cao jin]

On 01/14/2016 11:42 PM, Marcel Apfelbaum wrote:
> On 01/14/2016 02:23 PM, Cao jin wrote:


>> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
>> index 599768e..e9117b9 100644
>> --- a/hw/pci-host/apb.c
>> +++ b/hw/pci-host/apb.c
>> @@ -636,10 +636,7 @@ static int apb_pci_bridge_initfn(PCIDevice *dev)
>>   {
>>       int rc;
>>
>> -    rc = pci_bridge_initfn(dev, TYPE_PCI_BUS);
>> -    if (rc < 0) {
>> -        return rc;
>> -    }
>> +    pci_bridge_initfn(dev, TYPE_PCI_BUS);
>>
>
> Hi,
>
> It seems you don't need the rc variable here anymore, right?
>
> This should break the build (unused local variable),

Indeed...

> I suggest running configure with no params to check all the architectures,
> (I am also running make check while at it, just to be sure)
>

Thanks for the suggestion, really should that when touched so many device.

> Thanks,
> Marcel
>

> .
>

-- 
Yours Sincerely,

Cao jin

Re: [Qemu-devel] [PATCH v3 0/2] change type of pci_bridge_initfn()

[Cao jin]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

Did a hack to test dec realization:

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 319497e..134ab94 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -54,6 +54,7 @@
  #endif
  #include "migration/migration.h"
  #include "kvm_i386.h"
+#include "hw/pci-bridge/dec.h" //hack

  #define MAX_IDE_BUS 2

@@ -85,6 +86,7 @@ static void pc_init1(MachineState *machine,
      MemoryRegion *system_io = get_system_io();
      int i;
      PCIBus *pci_bus;
+    PCIBus *sec_bus; //hack
      ISABus *isa_bus;
      PCII440FXState *i440fx_state;
      int piix3_devfn = -1;
@@ -206,6 +208,9 @@ static void pc_init1(MachineState *machine,
      }
      isa_bus_irqs(isa_bus, gsi);

+    sec_bus = pci_dec_21154_init(pci_bus, -1);//hack
+    sec_bus = sec_bus; //suppress warning
+
      if (kvm_pic_in_kernel()) {
          i8259 = kvm_i8259_init(isa_bus);
      } else if (xen_enabled()) {

Then add "CONFIG_DEC_PCI=y" in x86_64-softmmu/config-devices.mak

Then, Can see dec bridge as attachment shows

On 01/14/2016 08:23 PM, Cao jin wrote:
> v3 changelog:
> 1. Just tweaked the commit message.
>
> Test:
> 1. Compiled.
> 2. Did a hack to test dec bridge realization, prove it is ok after patch.
>     Will reply this mail to attach the screenshot.
>
> Cao jin (2):
>    dec: convert to realize()
>    change type of pci_bridge_initfn() to void
>
>   hw/pci-bridge/dec.c                | 6 +++---
>   hw/pci-bridge/i82801b11.c          | 5 +----
>   hw/pci-bridge/ioh3420.c            | 6 +-----
>   hw/pci-bridge/pci_bridge_dev.c     | 8 +++-----
>   hw/pci-bridge/xio3130_downstream.c | 6 +-----
>   hw/pci-bridge/xio3130_upstream.c   | 6 +-----
>   hw/pci-host/apb.c                  | 5 +----
>   hw/pci/pci_bridge.c                | 3 +--
>   include/hw/pci/pci_bridge.h        | 2 +-
>   9 files changed, 13 insertions(+), 34 deletions(-)
>

-- 
Yours Sincerely,

Cao jin



[-- Attachment #2: dec_realize_test.jpg --]
[-- Type: image/jpeg, Size: 182239 bytes --]