[Qemu-devel] [PATCH 0/5] Convert to realize()

[Qemu-devel] [PATCH 0/5] Convert to realize()

[Cao jin]

some simple case

Cao jin (5):
  SH PCI Host: convert to realize()
  igd-passthrough-i440FX: convert to realize()
  PXB: convert to realize()
  gt64120: convert to realize()
  xen-pvdevice: convert to realize()

 hw/i386/xen/xen_pvdevice.c          | 12 ++++++------
 hw/mips/gt64xxx_pci.c               |  6 ++----
 hw/pci-bridge/pci_expander_bridge.c | 24 ++++++++++++++----------
 hw/pci-host/piix.c                  | 16 +++++++++-------
 hw/sh4/sh_pci.c                     |  5 ++---
 5 files changed, 33 insertions(+), 30 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/5] SH PCI Host: convert to realize()

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/sh4/sh_pci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
index a2f6d9e..4509053 100644
--- a/hw/sh4/sh_pci.c
+++ b/hw/sh4/sh_pci.c
@@ -151,12 +151,11 @@ static int sh_pci_device_init(SysBusDevice *dev)
     return 0;
 }
 
-static int sh_pci_host_init(PCIDevice *d)
+static void sh_pci_host_realize(PCIDevice *d, Error **errp)
 {
     pci_set_word(d->config + PCI_COMMAND, PCI_COMMAND_WAIT);
     pci_set_word(d->config + PCI_STATUS, PCI_STATUS_CAP_LIST |
                  PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM);
-    return 0;
 }
 
 static void sh_pci_host_class_init(ObjectClass *klass, void *data)
@@ -164,7 +163,7 @@ static void sh_pci_host_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->init = sh_pci_host_init;
+    k->realize = sh_pci_host_realize;
     k->vendor_id = PCI_VENDOR_ID_HITACHI;
     k->device_id = PCI_DEVICE_ID_HITACHI_SH7751R;
     /*
-- 
2.1.0

[Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci-host/piix.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..e3840f0 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
     {0xa8, 4},  /* SNB: base of GTT stolen memory */
 };
 
-static int host_pci_config_read(int pos, int len, uint32_t val)
+static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)
 {
     char path[PATH_MAX];
     int config_fd;
@@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
     int ret = 0;
 
     if (rc >= size || rc < 0) {
+        error_setg(errp, "No such device");
         return -ENODEV;
     }
 
     config_fd = open(path, O_RDWR);
     if (config_fd < 0) {
+        error_setg(errp, "No such device");
         return -ENODEV;
     }
 
     if (lseek(config_fd, pos, SEEK_SET) != pos) {
+        error_setg(errp, "lseek err: %s", strerror(errno));
         ret = -errno;
         goto out;
     }
@@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
     } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
     if (rc != len) {
         ret = -errno;
+        error_setg(errp, "read err: %s", strerror(errno));
     }
 out:
     close(config_fd);
     return ret;
 }
 
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp)
 {
     uint32_t val = 0;
     int rc, i, num;
@@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
     for (i = 0; i < num; i++) {
         pos = igd_host_bridge_infos[i].offset;
         len = igd_host_bridge_infos[i].len;
-        rc = host_pci_config_read(pos, len, val);
+        rc = host_pci_config_read(pos, len, val, errp);
         if (rc) {
-            return -ENODEV;
+            return;
         }
         pci_default_write_config(pci_dev, pos, val, len);
     }
-
-    return 0;
 }
 
 static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = igd_pt_i440fx_initfn;
+    k->realize = igd_pt_i440fx_realize;
     dc->desc = "IGD Passthrough Host bridge";
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci-bridge/pci_expander_bridge.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 57f8a37..cc975f6 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -145,19 +145,19 @@ static const TypeInfo pxb_host_info = {
  * Returns 0 on successs, -1 if i440fx host was not
  * found or the bus number is already in use.
  */
-static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus)
+static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus, Error **errp)
 {
     PCIBus *bus = dev->bus;
     int pxb_bus_num = pci_bus_num(pxb_bus);
 
     if (bus->parent_dev) {
-        error_report("PXB devices can be attached only to root bus.");
+        error_setg(errp, "PXB devices can be attached only to root bus.");
         return -1;
     }
 
     QLIST_FOREACH(bus, &bus->child, sibling) {
         if (pci_bus_num(bus) == pxb_bus_num) {
-            error_report("Bus %d is already in use.", pxb_bus_num);
+            error_setg(errp, "Bus %d is already in use.", pxb_bus_num);
             return -1;
         }
     }
@@ -193,7 +193,7 @@ static gint pxb_compare(gconstpointer a, gconstpointer b)
            0;
 }
 
-static int pxb_dev_initfn(PCIDevice *dev)
+static void pxb_dev_realize(PCIDevice *dev, Error **errp)
 {
     PXBDev *pxb = PXB_DEV(dev);
     DeviceState *ds, *bds;
@@ -202,8 +202,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
 
     if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
         pxb->numa_node >= nb_numa_nodes) {
-        error_report("Illegal numa node %d.", pxb->numa_node);
-        return -EINVAL;
+        error_setg(errp, "Illegal numa node %d.", pxb->numa_node);
+        return;
     }
 
     if (dev->qdev.id && *dev->qdev.id) {
@@ -225,8 +225,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
 
     PCI_HOST_BRIDGE(ds)->bus = bus;
 
-    if (pxb_register_bus(dev, bus)) {
-        return -EINVAL;
+    if (pxb_register_bus(dev, bus, errp)) {
+        goto err_register_bus;
     }
 
     qdev_init_nofail(ds);
@@ -237,7 +237,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
     pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
 
     pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
-    return 0;
+
+err_register_bus:
+    object_unref(OBJECT(ds));
+    object_unref(OBJECT(bds));
+    object_unref(OBJECT(bus));
 }
 
 static void pxb_dev_exitfn(PCIDevice *pci_dev)
@@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = pxb_dev_initfn;
+    k->realize = pxb_dev_realize;
     k->exit = pxb_dev_exitfn;
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
     k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
-- 
2.1.0

[Qemu-devel] [PATCH 4/5] gt64120: convert to realize()

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/mips/gt64xxx_pci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index f76a9fd..c1f3c9c 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1193,7 +1193,7 @@ static int gt64120_init(SysBusDevice *dev)
     return 0;
 }
 
-static int gt64120_pci_init(PCIDevice *d)
+static void gt64120_pci_realize(PCIDevice *d, Error **errp)
 {
     /* FIXME: Malta specific hw assumptions ahead */
     pci_set_word(d->config + PCI_COMMAND, 0);
@@ -1207,8 +1207,6 @@ static int gt64120_pci_init(PCIDevice *d)
     pci_set_long(d->config + PCI_BASE_ADDRESS_4, 0x14000000);
     pci_set_long(d->config + PCI_BASE_ADDRESS_5, 0x14000001);
     pci_set_byte(d->config + 0x3d, 0x01);
-
-    return 0;
 }
 
 static void gt64120_pci_class_init(ObjectClass *klass, void *data)
@@ -1216,7 +1214,7 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->init = gt64120_pci_init;
+    k->realize = gt64120_pci_realize;
     k->vendor_id = PCI_VENDOR_ID_MARVELL;
     k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
     k->revision = 0x10;
-- 
2.1.0

[Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Cao jin]

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/i386/xen/xen_pvdevice.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
index c218947..a6c93d0 100644
--- a/hw/i386/xen/xen_pvdevice.c
+++ b/hw/i386/xen/xen_pvdevice.c
@@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static int xen_pv_init(PCIDevice *pci_dev)
+static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
 {
     XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
     uint8_t *pci_conf;
 
     /* device-id property must always be supplied */
-    if (d->device_id == 0xffff)
-	    return -1;
+    if (d->device_id == 0xffff) {
+        error_setg(errp, "Device ID invalid, it must always be supplied")
+	return;
+    }
 
     pci_conf = pci_dev->config;
 
@@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
 
     pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
                      &d->mmio);
-
-    return 0;
 }
 
 static Property xen_pv_props[] = {
@@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = xen_pv_init;
+    k->realize = xen_pv_realize;
     k->class_id = PCI_CLASS_SYSTEM_OTHER;
     dc->desc = "Xen PV Device";
     dc->props = xen_pv_props;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/5] Convert to realize()

[Cao jin]

forgot to CC to: Markus

On 12/18/2015 07:03 PM, Cao jin wrote:
> some simple case
>
> Cao jin (5):
>    SH PCI Host: convert to realize()
>    igd-passthrough-i440FX: convert to realize()
>    PXB: convert to realize()
>    gt64120: convert to realize()
>    xen-pvdevice: convert to realize()
>
>   hw/i386/xen/xen_pvdevice.c          | 12 ++++++------
>   hw/mips/gt64xxx_pci.c               |  6 ++----
>   hw/pci-bridge/pci_expander_bridge.c | 24 ++++++++++++++----------
>   hw/pci-host/piix.c                  | 16 +++++++++-------
>   hw/sh4/sh_pci.c                     |  5 ++---
>   5 files changed, 33 insertions(+), 30 deletions(-)
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 1/5] SH PCI Host: convert to realize()

[Paolo Bonzini]

Cc: qemu-trivial@nongnu.org

On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/sh4/sh_pci.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c
> index a2f6d9e..4509053 100644
> --- a/hw/sh4/sh_pci.c
> +++ b/hw/sh4/sh_pci.c
> @@ -151,12 +151,11 @@ static int sh_pci_device_init(SysBusDevice *dev)
>      return 0;
>  }
>  
> -static int sh_pci_host_init(PCIDevice *d)
> +static void sh_pci_host_realize(PCIDevice *d, Error **errp)
>  {
>      pci_set_word(d->config + PCI_COMMAND, PCI_COMMAND_WAIT);
>      pci_set_word(d->config + PCI_STATUS, PCI_STATUS_CAP_LIST |
>                   PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM);
> -    return 0;
>  }
>  
>  static void sh_pci_host_class_init(ObjectClass *klass, void *data)
> @@ -164,7 +163,7 @@ static void sh_pci_host_class_init(ObjectClass *klass, void *data)
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    k->init = sh_pci_host_init;
> +    k->realize = sh_pci_host_realize;
>      k->vendor_id = PCI_VENDOR_ID_HITACHI;
>      k->device_id = PCI_DEVICE_ID_HITACHI_SH7751R;
>      /*
> 

Re: [Qemu-devel] [PATCH 4/5] gt64120: convert to realize()

[Paolo Bonzini]

Cc: qemu-trivial@nongnu.org


On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/mips/gt64xxx_pci.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index f76a9fd..c1f3c9c 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -1193,7 +1193,7 @@ static int gt64120_init(SysBusDevice *dev)
>      return 0;
>  }
>  
> -static int gt64120_pci_init(PCIDevice *d)
> +static void gt64120_pci_realize(PCIDevice *d, Error **errp)
>  {
>      /* FIXME: Malta specific hw assumptions ahead */
>      pci_set_word(d->config + PCI_COMMAND, 0);
> @@ -1207,8 +1207,6 @@ static int gt64120_pci_init(PCIDevice *d)
>      pci_set_long(d->config + PCI_BASE_ADDRESS_4, 0x14000000);
>      pci_set_long(d->config + PCI_BASE_ADDRESS_5, 0x14000001);
>      pci_set_byte(d->config + 0x3d, 0x01);
> -
> -    return 0;
>  }
>  
>  static void gt64120_pci_class_init(ObjectClass *klass, void *data)
> @@ -1216,7 +1214,7 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    k->init = gt64120_pci_init;
> +    k->realize = gt64120_pci_realize;
>      k->vendor_id = PCI_VENDOR_ID_MARVELL;
>      k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
>      k->revision = 0x10;
> 

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Paolo Bonzini]

CCing Stefano, who is the maintainer.

Paolo

On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/i386/xen/xen_pvdevice.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
> index c218947..a6c93d0 100644
> --- a/hw/i386/xen/xen_pvdevice.c
> +++ b/hw/i386/xen/xen_pvdevice.c
> @@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -static int xen_pv_init(PCIDevice *pci_dev)
> +static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
>      uint8_t *pci_conf;
>  
>      /* device-id property must always be supplied */
> -    if (d->device_id == 0xffff)
> -	    return -1;
> +    if (d->device_id == 0xffff) {
> +        error_setg(errp, "Device ID invalid, it must always be supplied")
> +	return;
> +    }
>  
>      pci_conf = pci_dev->config;
>  
> @@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
>  
>      pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
>                       &d->mmio);
> -
> -    return 0;
>  }
>  
>  static Property xen_pv_props[] = {
> @@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -    k->init = xen_pv_init;
> +    k->realize = xen_pv_realize;
>      k->class_id = PCI_CLASS_SYSTEM_OTHER;
>      dc->desc = "Xen PV Device";
>      dc->props = xen_pv_props;
> 

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Paolo Bonzini]

On 18/12/2015 12:03, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci-bridge/pci_expander_bridge.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 57f8a37..cc975f6 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -145,19 +145,19 @@ static const TypeInfo pxb_host_info = {
>   * Returns 0 on successs, -1 if i440fx host was not
>   * found or the bus number is already in use.
>   */
> -static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus)
> +static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus, Error **errp)
>  {
>      PCIBus *bus = dev->bus;
>      int pxb_bus_num = pci_bus_num(pxb_bus);
>  
>      if (bus->parent_dev) {
> -        error_report("PXB devices can be attached only to root bus.");
> +        error_setg(errp, "PXB devices can be attached only to root bus.");
>          return -1;
>      }
>  
>      QLIST_FOREACH(bus, &bus->child, sibling) {
>          if (pci_bus_num(bus) == pxb_bus_num) {
> -            error_report("Bus %d is already in use.", pxb_bus_num);
> +            error_setg(errp, "Bus %d is already in use.", pxb_bus_num);
>              return -1;
>          }
>      }
> @@ -193,7 +193,7 @@ static gint pxb_compare(gconstpointer a, gconstpointer b)
>             0;
>  }
>  
> -static int pxb_dev_initfn(PCIDevice *dev)
> +static void pxb_dev_realize(PCIDevice *dev, Error **errp)
>  {
>      PXBDev *pxb = PXB_DEV(dev);
>      DeviceState *ds, *bds;
> @@ -202,8 +202,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>  
>      if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
>          pxb->numa_node >= nb_numa_nodes) {
> -        error_report("Illegal numa node %d.", pxb->numa_node);
> -        return -EINVAL;
> +        error_setg(errp, "Illegal numa node %d.", pxb->numa_node);
> +        return;
>      }
>  
>      if (dev->qdev.id && *dev->qdev.id) {
> @@ -225,8 +225,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>  
>      PCI_HOST_BRIDGE(ds)->bus = bus;
>  
> -    if (pxb_register_bus(dev, bus)) {
> -        return -EINVAL;
> +    if (pxb_register_bus(dev, bus, errp)) {
> +        goto err_register_bus;
>      }
>  
>      qdev_init_nofail(ds);
> @@ -237,7 +237,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
>      pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>  
>      pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> -    return 0;
> +
> +err_register_bus:
> +    object_unref(OBJECT(ds));
> +    object_unref(OBJECT(bds));
> +    object_unref(OBJECT(bus));

I think these should be object_unparent, not unref.

Paolo

>  }
>  
>  static void pxb_dev_exitfn(PCIDevice *pci_dev)
> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -    k->init = pxb_dev_initfn;
> +    k->realize = pxb_dev_realize;
>      k->exit = pxb_dev_exitfn;
>      k->vendor_id = PCI_VENDOR_ID_REDHAT;
>      k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
> 

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Eduardo Habkost]

On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci-host/piix.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 715208b..e3840f0 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>      {0xa8, 4},  /* SNB: base of GTT stolen memory */
>  };
>  
> -static int host_pci_config_read(int pos, int len, uint32_t val)
> +static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)

You don't need the return value anymore, if you report errors
through the errp parameter. The function can be void, now.

>  {
>      char path[PATH_MAX];
>      int config_fd;
> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
>      int ret = 0;
>  
>      if (rc >= size || rc < 0) {
> +        error_setg(errp, "No such device");
>          return -ENODEV;
>      }
>  
>      config_fd = open(path, O_RDWR);
>      if (config_fd < 0) {
> +        error_setg(errp, "No such device");
>          return -ENODEV;
>      }
>  
>      if (lseek(config_fd, pos, SEEK_SET) != pos) {
> +        error_setg(errp, "lseek err: %s", strerror(errno));

What about error_setg_errno()?

>          ret = -errno;
>          goto out;
>      }
> @@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>      if (rc != len) {
>          ret = -errno;
> +        error_setg(errp, "read err: %s", strerror(errno));

Same here.

>      }
>  out:
>      close(config_fd);
>      return ret;
>  }
>  
> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp)
>  {
>      uint32_t val = 0;
>      int rc, i, num;
> @@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>      for (i = 0; i < num; i++) {
>          pos = igd_host_bridge_infos[i].offset;
>          len = igd_host_bridge_infos[i].len;
> -        rc = host_pci_config_read(pos, len, val);
> +        rc = host_pci_config_read(pos, len, val, errp);
>          if (rc) {

The usual pattern for error checking and propagation is:

    Error *err;
    host_pci_config_read(pos, len, val, &err);
    if (err) {
        error_propagate(errp, local_err);
        return;
    }

> -            return -ENODEV;
> +            return;
>          }
>          pci_default_write_config(pci_dev, pos, val, len);
>      }
> -
> -    return 0;
>  }
>  
>  static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> @@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -    k->init = igd_pt_i440fx_initfn;
> +    k->realize = igd_pt_i440fx_realize;
>      dc->desc = "IGD Passthrough Host bridge";
>  }
>  
> -- 
> 2.1.0
> 
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Markus Armbruster]

One short remark in addition to Eduardo's review.

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>  hw/pci-host/piix.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>> 
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 715208b..e3840f0 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>>      {0xa8, 4},  /* SNB: base of GTT stolen memory */
>>  };
>>  
>> -static int host_pci_config_read(int pos, int len, uint32_t val)
>> +static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)
>
> You don't need the return value anymore, if you report errors
> through the errp parameter. The function can be void, now.
>
>>  {
>>      char path[PATH_MAX];
>>      int config_fd;
>> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
>>      int ret = 0;
>>  
>>      if (rc >= size || rc < 0) {
>> +        error_setg(errp, "No such device");
>>          return -ENODEV;
>>      }
>>  
>>      config_fd = open(path, O_RDWR);
>>      if (config_fd < 0) {
>> +        error_setg(errp, "No such device");
>>          return -ENODEV;
>>      }

Can we come up with nicer error messages?

[...]

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Marcel Apfelbaum]

Hi,

On 12/18/2015 01:03 PM, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>   hw/pci-bridge/pci_expander_bridge.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 57f8a37..cc975f6 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -145,19 +145,19 @@ static const TypeInfo pxb_host_info = {
>    * Returns 0 on successs, -1 if i440fx host was not
>    * found or the bus number is already in use.
>    */
> -static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus)
> +static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus, Error **errp)

If you add an err parameter, maybe the function should return void.


>   {
>       PCIBus *bus = dev->bus;
>       int pxb_bus_num = pci_bus_num(pxb_bus);
>
>       if (bus->parent_dev) {
> -        error_report("PXB devices can be attached only to root bus.");
> +        error_setg(errp, "PXB devices can be attached only to root bus.");
>           return -1;
>       }
>
>       QLIST_FOREACH(bus, &bus->child, sibling) {
>           if (pci_bus_num(bus) == pxb_bus_num) {
> -            error_report("Bus %d is already in use.", pxb_bus_num);
> +            error_setg(errp, "Bus %d is already in use.", pxb_bus_num);
>               return -1;
>           }
>       }
> @@ -193,7 +193,7 @@ static gint pxb_compare(gconstpointer a, gconstpointer b)
>              0;
>   }
>
> -static int pxb_dev_initfn(PCIDevice *dev)
> +static void pxb_dev_realize(PCIDevice *dev, Error **errp)
>   {
>       PXBDev *pxb = PXB_DEV(dev);
>       DeviceState *ds, *bds;
> @@ -202,8 +202,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>
>       if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
>           pxb->numa_node >= nb_numa_nodes) {
> -        error_report("Illegal numa node %d.", pxb->numa_node);
> -        return -EINVAL;
> +        error_setg(errp, "Illegal numa node %d.", pxb->numa_node);
> +        return;
>       }
>
>       if (dev->qdev.id && *dev->qdev.id) {
> @@ -225,8 +225,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>
>       PCI_HOST_BRIDGE(ds)->bus = bus;
>
> -    if (pxb_register_bus(dev, bus)) {
> -        return -EINVAL;
> +    if (pxb_register_bus(dev, bus, errp)) {
> +        goto err_register_bus;
>       }
>
>       qdev_init_nofail(ds);
> @@ -237,7 +237,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
>       pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>
>       pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb, pxb_compare);
> -    return 0;
> +
> +err_register_bus:
> +    object_unref(OBJECT(ds));
> +    object_unref(OBJECT(bds));
> +    object_unref(OBJECT(bus));


The order should be in the reverse order of creation:
     bds, bus, ds


>   }
>
>   static void pxb_dev_exitfn(PCIDevice *pci_dev)
> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->init = pxb_dev_initfn;
> +    k->realize = pxb_dev_realize;
>       k->exit = pxb_dev_exitfn;

If init is converted to realize, maybe the exit should be converted to unrealize?


Thanks,
Marcel

>       k->vendor_id = PCI_VENDOR_ID_REDHAT;
>       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

Hi,

On 12/20/2015 06:22 PM, Marcel Apfelbaum wrote:
>
> Hi,
>
> On 12/18/2015 01:03 PM, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci-bridge/pci_expander_bridge.c | 24 ++++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
>> b/hw/pci-bridge/pci_expander_bridge.c
>> index 57f8a37..cc975f6 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -145,19 +145,19 @@ static const TypeInfo pxb_host_info = {
>>    * Returns 0 on successs, -1 if i440fx host was not
>>    * found or the bus number is already in use.
>>    */
>> -static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus)
>> +static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus, Error
>> **errp)
>
> If you add an err parameter, maybe the function should return void.
>

Ok, will modify it in V2. Actually, both style are fine with me:)

>
>>   {
>>       PCIBus *bus = dev->bus;
>>       int pxb_bus_num = pci_bus_num(pxb_bus);
>>
>>       if (bus->parent_dev) {
>> -        error_report("PXB devices can be attached only to root bus.");
>> +        error_setg(errp, "PXB devices can be attached only to root
>> bus.");
>>           return -1;
>>       }
>>
>>       QLIST_FOREACH(bus, &bus->child, sibling) {
>>           if (pci_bus_num(bus) == pxb_bus_num) {
>> -            error_report("Bus %d is already in use.", pxb_bus_num);
>> +            error_setg(errp, "Bus %d is already in use.", pxb_bus_num);
>>               return -1;
>>           }
>>       }
>> @@ -193,7 +193,7 @@ static gint pxb_compare(gconstpointer a,
>> gconstpointer b)
>>              0;
>>   }
>>
>> -static int pxb_dev_initfn(PCIDevice *dev)
>> +static void pxb_dev_realize(PCIDevice *dev, Error **errp)
>>   {
>>       PXBDev *pxb = PXB_DEV(dev);
>>       DeviceState *ds, *bds;
>> @@ -202,8 +202,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>
>>       if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
>>           pxb->numa_node >= nb_numa_nodes) {
>> -        error_report("Illegal numa node %d.", pxb->numa_node);
>> -        return -EINVAL;
>> +        error_setg(errp, "Illegal numa node %d.", pxb->numa_node);
>> +        return;
>>       }
>>
>>       if (dev->qdev.id && *dev->qdev.id) {
>> @@ -225,8 +225,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>
>>       PCI_HOST_BRIDGE(ds)->bus = bus;
>>
>> -    if (pxb_register_bus(dev, bus)) {
>> -        return -EINVAL;
>> +    if (pxb_register_bus(dev, bus, errp)) {
>> +        goto err_register_bus;
>>       }
>>
>>       qdev_init_nofail(ds);
>> @@ -237,7 +237,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>       pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>>
>>       pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb,
>> pxb_compare);
>> -    return 0;
>> +
>> +err_register_bus:
>> +    object_unref(OBJECT(ds));
>> +    object_unref(OBJECT(bds));
>> +    object_unref(OBJECT(bus));
>
>
> The order should be in the reverse order of creation:
>      bds, bus, ds
>

Ok, I can do that. But it seems the order here doesn`t matter? Is there 
dependency among these three?

>
>>   }
>>
>>   static void pxb_dev_exitfn(PCIDevice *pci_dev)
>> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass,
>> void *data)
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>
>> -    k->init = pxb_dev_initfn;
>> +    k->realize = pxb_dev_realize;
>>       k->exit = pxb_dev_exitfn;
>
> If init is converted to realize, maybe the exit should be converted to
> unrealize?
>

Yup, I agree with you from the point that the names should be antonym. 
But it seems there is no PCIDeviceClass.unrealize:(

And I am also not aware why there is no comment for .exit while there is 
for .init. It is appreciated if somebody could tell me the history O:-)

>
> Thanks,
> Marcel
>
>>       k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>>
>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Marcel Apfelbaum]

On 12/20/2015 12:48 PM, Cao jin wrote:
> Hi,
>
> On 12/20/2015 06:22 PM, Marcel Apfelbaum wrote:
>>
>> Hi,
>>
>> On 12/18/2015 01:03 PM, Cao jin wrote:
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---
>>>   hw/pci-bridge/pci_expander_bridge.c | 24 ++++++++++++++----------
>>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
>>> b/hw/pci-bridge/pci_expander_bridge.c
>>> index 57f8a37..cc975f6 100644
>>> --- a/hw/pci-bridge/pci_expander_bridge.c
>>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>>> @@ -145,19 +145,19 @@ static const TypeInfo pxb_host_info = {
>>>    * Returns 0 on successs, -1 if i440fx host was not
>>>    * found or the bus number is already in use.
>>>    */
>>> -static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus)
>>> +static int pxb_register_bus(PCIDevice *dev, PCIBus *pxb_bus, Error
>>> **errp)
>>
>> If you add an err parameter, maybe the function should return void.
>>
>
> Ok, will modify it in V2. Actually, both style are fine with me:)
>
>>
>>>   {
>>>       PCIBus *bus = dev->bus;
>>>       int pxb_bus_num = pci_bus_num(pxb_bus);
>>>
>>>       if (bus->parent_dev) {
>>> -        error_report("PXB devices can be attached only to root bus.");
>>> +        error_setg(errp, "PXB devices can be attached only to root
>>> bus.");
>>>           return -1;
>>>       }
>>>
>>>       QLIST_FOREACH(bus, &bus->child, sibling) {
>>>           if (pci_bus_num(bus) == pxb_bus_num) {
>>> -            error_report("Bus %d is already in use.", pxb_bus_num);
>>> +            error_setg(errp, "Bus %d is already in use.", pxb_bus_num);
>>>               return -1;
>>>           }
>>>       }
>>> @@ -193,7 +193,7 @@ static gint pxb_compare(gconstpointer a,
>>> gconstpointer b)
>>>              0;
>>>   }
>>>
>>> -static int pxb_dev_initfn(PCIDevice *dev)
>>> +static void pxb_dev_realize(PCIDevice *dev, Error **errp)
>>>   {
>>>       PXBDev *pxb = PXB_DEV(dev);
>>>       DeviceState *ds, *bds;
>>> @@ -202,8 +202,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>>
>>>       if (pxb->numa_node != NUMA_NODE_UNASSIGNED &&
>>>           pxb->numa_node >= nb_numa_nodes) {
>>> -        error_report("Illegal numa node %d.", pxb->numa_node);
>>> -        return -EINVAL;
>>> +        error_setg(errp, "Illegal numa node %d.", pxb->numa_node);
>>> +        return;
>>>       }
>>>
>>>       if (dev->qdev.id && *dev->qdev.id) {
>>> @@ -225,8 +225,8 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>>
>>>       PCI_HOST_BRIDGE(ds)->bus = bus;
>>>
>>> -    if (pxb_register_bus(dev, bus)) {
>>> -        return -EINVAL;
>>> +    if (pxb_register_bus(dev, bus, errp)) {
>>> +        goto err_register_bus;
>>>       }
>>>
>>>       qdev_init_nofail(ds);
>>> @@ -237,7 +237,11 @@ static int pxb_dev_initfn(PCIDevice *dev)
>>>       pci_config_set_class(dev->config, PCI_CLASS_BRIDGE_HOST);
>>>
>>>       pxb_dev_list = g_list_insert_sorted(pxb_dev_list, pxb,
>>> pxb_compare);
>>> -    return 0;
>>> +
>>> +err_register_bus:
>>> +    object_unref(OBJECT(ds));
>>> +    object_unref(OBJECT(bds));
>>> +    object_unref(OBJECT(bus));
>>
>>
>> The order should be in the reverse order of creation:
>>      bds, bus, ds
>>
>
> Ok, I can do that. But it seems the order here doesn`t matter? Is there dependency among these three?

Yes, there is a dependency:
At first the pxb host (ds) is created, then the bus (bus) is created as host's child (see pci_bus_new)
and in the end a pci bridge (bds) is attached to the bus (see qdev_create).

By the way, indeed you should call object_unparent at least for the pxb_host(ds), but you may want to
check the others parent relationship as well.

>
>>
>>>   }
>>>
>>>   static void pxb_dev_exitfn(PCIDevice *pci_dev)
>>> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass,
>>> void *data)
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>
>>> -    k->init = pxb_dev_initfn;
>>> +    k->realize = pxb_dev_realize;
>>>       k->exit = pxb_dev_exitfn;
>>
>> If init is converted to realize, maybe the exit should be converted to
>> unrealize?
>>
>
> Yup, I agree with you from the point that the names should be antonym. But it seems there is no PCIDeviceClass.unrealize:(

You are right. The pci_qdev_unrealize ultimately calls exit. But the same goes for init, pci_qdev_realize calls for pc->realize.
This is the reason I chose to use init/exit instead of the strange realize/exit.

But since the intention is to get rid of init, I am not against it.

>
> And I am also not aware why there is no comment for .exit while there is for .init. It is appreciated if somebody could tell me the history O:-)

I'll add Markus, Andreas and Michael (the PCI maintainer), maybe they have a better insight to this.

On the other hand you should continue with the patch and leave the "unrealize" until we'll know more :)

Thanks,
Marcel

>
>>
>> Thanks,
>> Marcel
>>
>>>       k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>>>
>>
>>
>>
>> .
>>
>

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

Hi

On 12/19/2015 02:01 AM, Paolo Bonzini wrote:
>
>
> On 18/12/2015 12:03, Cao jin wrote:
[...]
>> +
>> +err_register_bus:
>> +    object_unref(OBJECT(ds));
>> +    object_unref(OBJECT(bds));
>> +    object_unref(OBJECT(bus));
>
> I think these should be object_unparent, not unref.
>

But, it seems these 3 objects isn`t added as a child-property via 
object_property_add_child() during creation, so OBJECT(ds)->parent(so 
does the other 2) will be NULL, and so object_unparent will do nothing?

Or am I missing something?

> Paolo
>
>>   }
>>
>>   static void pxb_dev_exitfn(PCIDevice *pci_dev)
>> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass, void *data)
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>
>> -    k->init = pxb_dev_initfn;
>> +    k->realize = pxb_dev_realize;
>>       k->exit = pxb_dev_exitfn;
>>       k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Cao jin]

On 12/19/2015 02:37 AM, Eduardo Habkost wrote:
> On Fri, Dec 18, 2015 at 07:03:49PM +0800, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci-host/piix.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 715208b..e3840f0 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>>       {0xa8, 4},  /* SNB: base of GTT stolen memory */
>>   };
>>
>> -static int host_pci_config_read(int pos, int len, uint32_t val)
>> +static int host_pci_config_read(int pos, int len, uint32_t val, Error **errp)
>
> You don't need the return value anymore, if you report errors
> through the errp parameter. The function can be void, now.

Ok, will modify that in next version, as many people mentioned in the 
other patch...

>
>>   {
>>       char path[PATH_MAX];
>>       int config_fd;
>> @@ -772,15 +772,18 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
>>       int ret = 0;
>>
>>       if (rc >= size || rc < 0) {
>> +        error_setg(errp, "No such device");
>>           return -ENODEV;
>>       }
>>
>>       config_fd = open(path, O_RDWR);
>>       if (config_fd < 0) {
>> +        error_setg(errp, "No such device");
>>           return -ENODEV;
>>       }
>>
>>       if (lseek(config_fd, pos, SEEK_SET) != pos) {
>> +        error_setg(errp, "lseek err: %s", strerror(errno));
>
> What about error_setg_errno()?

Ok, I am not conscious that there exist error_setg_errno() :p

>
>>           ret = -errno;
>>           goto out;
>>       }
>> @@ -789,13 +792,14 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>       if (rc != len) {
>>           ret = -errno;
>> +        error_setg(errp, "read err: %s", strerror(errno));
>
> Same here.
OK.
>
>>       }
>>   out:
>>       close(config_fd);
>>       return ret;
>>   }
>>
>> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>> +static void igd_pt_i440fx_realize(struct PCIDevice *pci_dev, Error **errp)
>>   {
>>       uint32_t val = 0;
>>       int rc, i, num;
>> @@ -805,14 +809,12 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>>       for (i = 0; i < num; i++) {
>>           pos = igd_host_bridge_infos[i].offset;
>>           len = igd_host_bridge_infos[i].len;
>> -        rc = host_pci_config_read(pos, len, val);
>> +        rc = host_pci_config_read(pos, len, val, errp);
>>           if (rc) {
>
> The usual pattern for error checking and propagation is:
>
>      Error *err;
>      host_pci_config_read(pos, len, val, &err);
>      if (err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>

Yup. I see

>> -            return -ENODEV;
>> +            return;
>>           }
>>           pci_default_write_config(pci_dev, pos, val, len);
>>       }
>> -
>> -    return 0;
>>   }
>>
>>   static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>> @@ -820,7 +822,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>
>> -    k->init = igd_pt_i440fx_initfn;
>> +    k->realize = igd_pt_i440fx_realize;
>>       dc->desc = "IGD Passthrough Host bridge";
>>   }
>>
>> --
>> 2.1.0
>>
>>
>>
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Cao jin]

On 12/19/2015 05:18 AM, Markus Armbruster wrote:
> One short remark in addition to Eduardo's review.
>
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
[...]
>>>
>>>       config_fd = open(path, O_RDWR);
>>>       if (config_fd < 0) {
>>> +        error_setg(errp, "No such device");
>>>           return -ENODEV;
>>>       }
>
> Can we come up with nicer error messages?
>

Ok...will change the error msg to strerror(errno)

> [...]
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

On 12/20/2015 07:21 PM, Marcel Apfelbaum wrote:
> On 12/20/2015 12:48 PM, Cao jin wrote:
>> Hi,
>>
>> On 12/20/2015 06:22 PM, Marcel Apfelbaum wrote:
[...]
>>>> +
>>>> +err_register_bus:
>>>> +    object_unref(OBJECT(ds));
>>>> +    object_unref(OBJECT(bds));
>>>> +    object_unref(OBJECT(bus));
>>>
>>>
>>> The order should be in the reverse order of creation:
>>>      bds, bus, ds
>>>
>>
>> Ok, I can do that. But it seems the order here doesn`t matter? Is
>> there dependency among these three?
>
> Yes, there is a dependency:
> At first the pxb host (ds) is created, then the bus (bus) is created as
> host's child (see pci_bus_new)
> and in the end a pci bridge (bds) is attached to the bus (see qdev_create).
>

Yup...thanks for reminding, I did read the code trying to find the 
parent relationship...but seem I didn`t read it thoroughly:-[

> By the way, indeed you should call object_unparent at least for the
> pxb_host(ds), but you may want to
> check the others parent relationship as well.
>

yes, but I think you are saying: object_unparent(bus), right? the 
relationship seems is:
   pxb host-->(child property)bus-->(link property)bds

Another question: because some of this series is CCed to 
qemu-trivial(which means: reviewed-by?) by other maintainer, so next 
time, do I need to send the whole series with "v2", or the rest?

>>
>>>
>>>>   }
>>>>
>>>>   static void pxb_dev_exitfn(PCIDevice *pci_dev)
>>>> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass,
>>>> void *data)
>>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>
>>>> -    k->init = pxb_dev_initfn;
>>>> +    k->realize = pxb_dev_realize;
>>>>       k->exit = pxb_dev_exitfn;
>>>
>>> If init is converted to realize, maybe the exit should be converted to
>>> unrealize?
>>>
>>
>> Yup, I agree with you from the point that the names should be antonym.
>> But it seems there is no PCIDeviceClass.unrealize:(
>
> You are right. The pci_qdev_unrealize ultimately calls exit. But the
> same goes for init, pci_qdev_realize calls for pc->realize.
> This is the reason I chose to use init/exit instead of the strange
> realize/exit.
>
> But since the intention is to get rid of init, I am not against it.
>
>>
>> And I am also not aware why there is no comment for .exit while there
>> is for .init. It is appreciated if somebody could tell me the history
>> O:-)
>
> I'll add Markus, Andreas and Michael (the PCI maintainer), maybe they
> have a better insight to this.
>
> On the other hand you should continue with the patch and leave the
> "unrealize" until we'll know more :)

Got it;)

>
> Thanks,
> Marcel
>
>>
>>>
>>> Thanks,
>>> Marcel
>>>
>>>>       k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>>       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>>>>
>>>
>>>
>>>
>>> .
>>>
>>
>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Cao jin]

On 12/19/2015 02:00 AM, Paolo Bonzini wrote:
> CCing Stefano, who is the maintainer.
>

"Stefano" isn`t in the list when I use scripts/get_maintainer.pl...

> Paolo
>
> On 18/12/2015 12:03, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/xen/xen_pvdevice.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
>> index c218947..a6c93d0 100644
>> --- a/hw/i386/xen/xen_pvdevice.c
>> +++ b/hw/i386/xen/xen_pvdevice.c
>> @@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
>>       .endianness = DEVICE_LITTLE_ENDIAN,
>>   };
>>
>> -static int xen_pv_init(PCIDevice *pci_dev)
>> +static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
>>   {
>>       XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
>>       uint8_t *pci_conf;
>>
>>       /* device-id property must always be supplied */
>> -    if (d->device_id == 0xffff)
>> -	    return -1;
>> +    if (d->device_id == 0xffff) {
>> +        error_setg(errp, "Device ID invalid, it must always be supplied")
>> +	return;
>> +    }
>>
>>       pci_conf = pci_dev->config;
>>
>> @@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
>>
>>       pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
>>                        &d->mmio);
>> -
>> -    return 0;
>>   }
>>
>>   static Property xen_pv_props[] = {
>> @@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void *data)
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>
>> -    k->init = xen_pv_init;
>> +    k->realize = xen_pv_realize;
>>       k->class_id = PCI_CLASS_SYSTEM_OTHER;
>>       dc->desc = "Xen PV Device";
>>       dc->props = xen_pv_props;
>>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Marcel Apfelbaum]

On 12/21/2015 04:59 AM, Cao jin wrote:
>
>
> On 12/20/2015 07:21 PM, Marcel Apfelbaum wrote:
>> On 12/20/2015 12:48 PM, Cao jin wrote:
>>> Hi,
>>>
>>> On 12/20/2015 06:22 PM, Marcel Apfelbaum wrote:
> [...]
>>>>> +
>>>>> +err_register_bus:
>>>>> +    object_unref(OBJECT(ds));
>>>>> +    object_unref(OBJECT(bds));
>>>>> +    object_unref(OBJECT(bus));
>>>>
>>>>
>>>> The order should be in the reverse order of creation:
>>>>      bds, bus, ds
>>>>
>>>
>>> Ok, I can do that. But it seems the order here doesn`t matter? Is
>>> there dependency among these three?
>>
>> Yes, there is a dependency:
>> At first the pxb host (ds) is created, then the bus (bus) is created as
>> host's child (see pci_bus_new)
>> and in the end a pci bridge (bds) is attached to the bus (see qdev_create).
>>
>
> Yup...thanks for reminding, I did read the code trying to find the parent relationship...but seem I didn`t read it thoroughly:-[
>
>> By the way, indeed you should call object_unparent at least for the
>> pxb_host(ds), but you may want to
>> check the others parent relationship as well.
>>
>
> yes, but I think you are saying: object_unparent(bus), right? the relationship seems is:
>    pxb host-->(child property)bus-->(link property)bds
>
> Another question: because some of this series is CCed to qemu-trivial(which means: reviewed-by?) by other maintainer, so next time, do I need to send the whole series with "v2", or the rest?

Hi,

Since the patches are not related, the ones cc-ed to qemu-trivial will be taken by the maintainer of trivial patches,
for the rest you should prepare a V2 to be reviewed by the corresponding sub-tree maintainer.

CC to qemu-trivial does not mean "reviewed-by", it just implies the
patch is simple enough to go through the trivial tree and does not need to go through the sub-tree maintainer.

Thanks,
Marcel

>
>>>
>>>>
>>>>>   }
>>>>>
>>>>>   static void pxb_dev_exitfn(PCIDevice *pci_dev)
>>>>> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass *klass,
>>>>> void *data)
>>>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>>
>>>>> -    k->init = pxb_dev_initfn;
>>>>> +    k->realize = pxb_dev_realize;
>>>>>       k->exit = pxb_dev_exitfn;
>>>>
>>>> If init is converted to realize, maybe the exit should be converted to
>>>> unrealize?
>>>>
>>>
>>> Yup, I agree with you from the point that the names should be antonym.
>>> But it seems there is no PCIDeviceClass.unrealize:(
>>
>> You are right. The pci_qdev_unrealize ultimately calls exit. But the
>> same goes for init, pci_qdev_realize calls for pc->realize.
>> This is the reason I chose to use init/exit instead of the strange
>> realize/exit.
>>
>> But since the intention is to get rid of init, I am not against it.
>>
>>>
>>> And I am also not aware why there is no comment for .exit while there
>>> is for .init. It is appreciated if somebody could tell me the history
>>> O:-)
>>
>> I'll add Markus, Andreas and Michael (the PCI maintainer), maybe they
>> have a better insight to this.
>>
>> On the other hand you should continue with the patch and leave the
>> "unrealize" until we'll know more :)
>
> Got it;)
>
>>
>> Thanks,
>> Marcel
>>
>>>
>>>>
>>>> Thanks,
>>>> Marcel
>>>>
>>>>>       k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>>>       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>>>>>
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>>
>> .
>>
>

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

On 12/21/2015 06:08 PM, Marcel Apfelbaum wrote:
> On 12/21/2015 04:59 AM, Cao jin wrote:
>>
>>
[...]
>>
>> Another question: because some of this series is CCed to
>> qemu-trivial(which means: reviewed-by?) by other maintainer, so next
>> time, do I need to send the whole series with "v2", or the rest?
>
> Hi,
>
> Since the patches are not related, the ones cc-ed to qemu-trivial will
> be taken by the maintainer of trivial patches,
> for the rest you should prepare a V2 to be reviewed by the corresponding
> sub-tree maintainer.
>
> CC to qemu-trivial does not mean "reviewed-by", it just implies the
> patch is simple enough to go through the trivial tree and does not need
> to go through the sub-tree maintainer.
>

Got it, thanks:)

> Thanks,
> Marcel
>
>>
[...]
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

Hi Paolo

On 12/20/2015 07:38 PM, Cao jin wrote:
> Hi
>
> On 12/19/2015 02:01 AM, Paolo Bonzini wrote:
>>
>>
>> On 18/12/2015 12:03, Cao jin wrote:
> [...]
>>> +
>>> +err_register_bus:
>>> +    object_unref(OBJECT(ds));
>>> +    object_unref(OBJECT(bds));
>>> +    object_unref(OBJECT(bus));
>>
>> I think these should be object_unparent, not unref.
>>
>
> But, it seems these 3 objects isn`t added as a child-property via
> object_property_add_child() during creation, so OBJECT(ds)->parent(so
> does the other 2) will be NULL, and so object_unparent will do nothing?
>
> Or am I missing something?
>

I finally find what I missed...Yes you are right...In qom, seems all 
devices are attached to container:"peripheral", or "peripheral-anon", or 
"unattached" or anything I don`t see until now...Thanks a lot:)

>> Paolo
>>
>>>   }
>>>
>>>   static void pxb_dev_exitfn(PCIDevice *pci_dev)
>>> @@ -259,7 +263,7 @@ static void pxb_dev_class_init(ObjectClass
>>> *klass, void *data)
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>
>>> -    k->init = pxb_dev_initfn;
>>> +    k->realize = pxb_dev_realize;
>>>       k->exit = pxb_dev_exitfn;
>>>       k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>       k->device_id = PCI_DEVICE_ID_REDHAT_PXB;
>>>
>>
>>
>> .
>>
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Stefano Stabellini]

On Mon, 21 Dec 2015, Cao jin wrote:
> On 12/19/2015 02:00 AM, Paolo Bonzini wrote:
> > CCing Stefano, who is the maintainer.
> > 
> 
> "Stefano" isn`t in the list when I use scripts/get_maintainer.pl...

Thanks Cao. I have just sent a patch to update the MAINTAINERS file.

http://marc.info/?l=qemu-devel&m=145070996302064&w=2



> > 
> > On 18/12/2015 12:03, Cao jin wrote:
> > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> > > ---
> > >   hw/i386/xen/xen_pvdevice.c | 12 ++++++------
> > >   1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
> > > index c218947..a6c93d0 100644
> > > --- a/hw/i386/xen/xen_pvdevice.c
> > > +++ b/hw/i386/xen/xen_pvdevice.c
> > > @@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
> > >       .endianness = DEVICE_LITTLE_ENDIAN,
> > >   };
> > > 
> > > -static int xen_pv_init(PCIDevice *pci_dev)
> > > +static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
> > >   {
> > >       XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
> > >       uint8_t *pci_conf;
> > > 
> > >       /* device-id property must always be supplied */
> > > -    if (d->device_id == 0xffff)
> > > -	    return -1;
> > > +    if (d->device_id == 0xffff) {
> > > +        error_setg(errp, "Device ID invalid, it must always be supplied")
> > > +	return;
> > > +    }
> > > 
> > >       pci_conf = pci_dev->config;
> > > 
> > > @@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
> > > 
> > >       pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
> > >                        &d->mmio);
> > > -
> > > -    return 0;
> > >   }
> > > 
> > >   static Property xen_pv_props[] = {
> > > @@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void
> > > *data)
> > >       DeviceClass *dc = DEVICE_CLASS(klass);
> > >       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > 
> > > -    k->init = xen_pv_init;
> > > +    k->realize = xen_pv_realize;
> > >       k->class_id = PCI_CLASS_SYSTEM_OTHER;
> > >       dc->desc = "Xen PV Device";
> > >       dc->props = xen_pv_props;
> > > 
> > 
> > 
> > .
> > 
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin
> 
> 
> 

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Stefano Stabellini]

On Fri, 18 Dec 2015, Cao jin wrote:
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/i386/xen/xen_pvdevice.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
> index c218947..a6c93d0 100644
> --- a/hw/i386/xen/xen_pvdevice.c
> +++ b/hw/i386/xen/xen_pvdevice.c
> @@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -static int xen_pv_init(PCIDevice *pci_dev)
> +static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
>      uint8_t *pci_conf;
>  
>      /* device-id property must always be supplied */
> -    if (d->device_id == 0xffff)
> -	    return -1;
> +    if (d->device_id == 0xffff) {
> +        error_setg(errp, "Device ID invalid, it must always be supplied")

This doesn't even compile: you are missing a ';'

Please at least build test patches.


> +	return;

I realize that there was a tab before there, but please use spaces for
indentation.


> +    }
>  
>      pci_conf = pci_dev->config;
>  
> @@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
>  
>      pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
>                       &d->mmio);
> -
> -    return 0;
>  }
>  
>  static Property xen_pv_props[] = {
> @@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> -    k->init = xen_pv_init;
> +    k->realize = xen_pv_realize;
>      k->class_id = PCI_CLASS_SYSTEM_OTHER;
>      dc->desc = "Xen PV Device";
>      dc->props = xen_pv_props;
> -- 
> 2.1.0
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Paolo Bonzini]

On 20/12/2015 12:38, Cao jin wrote:
>>>
>>> +    object_unref(OBJECT(ds));
>>> +    object_unref(OBJECT(bds));
>>> +    object_unref(OBJECT(bus));
>>
>> I think these should be object_unparent, not unref.
>>
> 
> But, it seems these 3 objects isn`t added as a child-property via
> object_property_add_child() during creation, so OBJECT(ds)->parent(so
> does the other 2) will be NULL, and so object_unparent will do nothing?

qdev_init_nofail adds them (qdev_init_nofail -> object_property_set_bool
-> device_set_realized -> object_property_add_child).

If you haven't reached qdev_init_nofail, you should indeed unref ds and
bds instead.  However, the bus should be unparented because pci_bus_new
makes it a child of ds (pci_bus_new -> qbus_create -> qbus_realize ->
object_property_add_child).

Paolo

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Cao jin]

On 12/21/2015 11:15 PM, Stefano Stabellini wrote:
> On Fri, 18 Dec 2015, Cao jin wrote:
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/xen/xen_pvdevice.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i386/xen/xen_pvdevice.c b/hw/i386/xen/xen_pvdevice.c
>> index c218947..a6c93d0 100644
>> --- a/hw/i386/xen/xen_pvdevice.c
>> +++ b/hw/i386/xen/xen_pvdevice.c
>> @@ -69,14 +69,16 @@ static const MemoryRegionOps xen_pv_mmio_ops = {
>>       .endianness = DEVICE_LITTLE_ENDIAN,
>>   };
>>
>> -static int xen_pv_init(PCIDevice *pci_dev)
>> +static void xen_pv_realize(PCIDevice *pci_dev, Error **errp)
>>   {
>>       XenPVDevice *d = XEN_PV_DEVICE(pci_dev);
>>       uint8_t *pci_conf;
>>
>>       /* device-id property must always be supplied */
>> -    if (d->device_id == 0xffff)
>> -	    return -1;
>> +    if (d->device_id == 0xffff) {
>> +        error_setg(errp, "Device ID invalid, it must always be supplied")
>
> This doesn't even compile: you are missing a ';'
>
> Please at least build test patches.
>

Yup...sorry for the silly mistake...but weird, I did build and didn`t 
got error...

>
>> +	return;
>
> I realize that there was a tab before there, but please use spaces for
> indentation.
>

surprised that I didn`t recognized the tab...V2 is coming soon.

>
>> +    }
>>
>>       pci_conf = pci_dev->config;
>>
>> @@ -97,8 +99,6 @@ static int xen_pv_init(PCIDevice *pci_dev)
>>
>>       pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_MEM_PREFETCH,
>>                        &d->mmio);
>> -
>> -    return 0;
>>   }
>>
>>   static Property xen_pv_props[] = {
>> @@ -114,7 +114,7 @@ static void xen_pv_class_init(ObjectClass *klass, void *data)
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>
>> -    k->init = xen_pv_init;
>> +    k->realize = xen_pv_realize;
>>       k->class_id = PCI_CLASS_SYSTEM_OTHER;
>>       dc->desc = "Xen PV Device";
>>       dc->props = xen_pv_props;
>> --
>> 2.1.0
>>
>>
>>
>>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()

[Cao jin]

Hi

On 12/22/2015 09:24 AM, Cao jin wrote:
>
>
> On 12/21/2015 11:15 PM, Stefano Stabellini wrote:
>> On Fri, 18 Dec 2015, Cao jin wrote:
[...]
>>
>> This doesn't even compile: you are missing a ';'
>>
>> Please at least build test patches.
>>
>
> Yup...sorry for the silly mistake...but weird, I did build and didn`t
> got error...
>

I test with:
$ readelf -s qemu-system-x86_64 | grep xen_pv_*
got nothing, which make me realized that I don`t have xen support on my 
computer... maybe that`s why I didn`t got compile error

>>
[...]

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

On 12/21/2015 11:49 PM, Paolo Bonzini wrote:
>
>
> On 20/12/2015 12:38, Cao jin wrote:
>>>>
>>>> +    object_unref(OBJECT(ds));
>>>> +    object_unref(OBJECT(bds));
>>>> +    object_unref(OBJECT(bus));
>>>
>>> I think these should be object_unparent, not unref.
>>>
>>
>> But, it seems these 3 objects isn`t added as a child-property via
>> object_property_add_child() during creation, so OBJECT(ds)->parent(so
>> does the other 2) will be NULL, and so object_unparent will do nothing?
>
> qdev_init_nofail adds them (qdev_init_nofail -> object_property_set_bool
> -> device_set_realized -> object_property_add_child).
>
> If you haven't reached qdev_init_nofail, you should indeed unref ds and
> bds instead.  However, the bus should be unparented because pci_bus_new
> makes it a child of ds (pci_bus_new -> qbus_create -> qbus_realize ->
> object_property_add_child).
>

Yes...that`s true.

and @Marcel, I think maybe this is final decision?

> Paolo
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Marcel Apfelbaum]

On 12/22/2015 05:58 AM, Cao jin wrote:
>
>
> On 12/21/2015 11:49 PM, Paolo Bonzini wrote:
>>
>>
>> On 20/12/2015 12:38, Cao jin wrote:
>>>>>
>>>>> +    object_unref(OBJECT(ds));
>>>>> +    object_unref(OBJECT(bds));
>>>>> +    object_unref(OBJECT(bus));
>>>>
>>>> I think these should be object_unparent, not unref.
>>>>
>>>
>>> But, it seems these 3 objects isn`t added as a child-property via
>>> object_property_add_child() during creation, so OBJECT(ds)->parent(so
>>> does the other 2) will be NULL, and so object_unparent will do nothing?
>>
>> qdev_init_nofail adds them (qdev_init_nofail -> object_property_set_bool
>> -> device_set_realized -> object_property_add_child).
>>
>> If you haven't reached qdev_init_nofail, you should indeed unref ds and
>> bds instead.  However, the bus should be unparented because pci_bus_new
>> makes it a child of ds (pci_bus_new -> qbus_create -> qbus_realize ->
>> object_property_add_child).
>>
>
> Yes...that`s true.
>
> and @Marcel, I think maybe this is final decision?


I say add a debug trace line before pxb_register_bus (or use the debugger)
and check ds->parent, bds->parent and bus->parent.

Run the qemu with -device pxb,bus=80,... and for every one that its parent
is not null add unparent. :)

Thanks,
Marcel




>
>> Paolo
>>
>>
>> .
>>
>

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

On 12/22/2015 03:34 PM, Marcel Apfelbaum wrote:
> On 12/22/2015 05:58 AM, Cao jin wrote:
>>
>>
>> On 12/21/2015 11:49 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 20/12/2015 12:38, Cao jin wrote:
>>>>>>
>>>>>> +    object_unref(OBJECT(ds));
>>>>>> +    object_unref(OBJECT(bds));
>>>>>> +    object_unref(OBJECT(bus));
>>>>>
>>>>> I think these should be object_unparent, not unref.
>>>>>
>>>>
>>>> But, it seems these 3 objects isn`t added as a child-property via
>>>> object_property_add_child() during creation, so OBJECT(ds)->parent(so
>>>> does the other 2) will be NULL, and so object_unparent will do nothing?
>>>
>>> qdev_init_nofail adds them (qdev_init_nofail -> object_property_set_bool
>>> -> device_set_realized -> object_property_add_child).
>>>
>>> If you haven't reached qdev_init_nofail, you should indeed unref ds and
>>> bds instead.  However, the bus should be unparented because pci_bus_new
>>> makes it a child of ds (pci_bus_new -> qbus_create -> qbus_realize ->
>>> object_property_add_child).
>>>
>>
>> Yes...that`s true.
>>
>> and @Marcel, I think maybe this is final decision?
>
>
> I say add a debug trace line before pxb_register_bus (or use the debugger)
> and check ds->parent, bds->parent and bus->parent.
>

uh..sorry I don`t get it, what does the debug trace line/use debugger mean?

> Run the qemu with -device pxb,bus=80,... and for every one that its parent
> is not null add unparent. :)

don`t get it too, could you detail it?

>
> Thanks,
> Marcel
>
>
>
>
>>
>>> Paolo
>>>
>>>
>>> .
>>>
>>
>
>
>
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Marcel Apfelbaum]

On 12/22/2015 11:16 AM, Cao jin wrote:
>
>
> On 12/22/2015 03:34 PM, Marcel Apfelbaum wrote:
>> On 12/22/2015 05:58 AM, Cao jin wrote:
>>>
>>>
>>> On 12/21/2015 11:49 PM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 20/12/2015 12:38, Cao jin wrote:
>>>>>>>
>>>>>>> +    object_unref(OBJECT(ds));
>>>>>>> +    object_unref(OBJECT(bds));
>>>>>>> +    object_unref(OBJECT(bus));
>>>>>>
>>>>>> I think these should be object_unparent, not unref.
>>>>>>
>>>>>
>>>>> But, it seems these 3 objects isn`t added as a child-property via
>>>>> object_property_add_child() during creation, so OBJECT(ds)->parent(so
>>>>> does the other 2) will be NULL, and so object_unparent will do nothing?
>>>>
>>>> qdev_init_nofail adds them (qdev_init_nofail -> object_property_set_bool
>>>> -> device_set_realized -> object_property_add_child).
>>>>
>>>> If you haven't reached qdev_init_nofail, you should indeed unref ds and
>>>> bds instead.  However, the bus should be unparented because pci_bus_new
>>>> makes it a child of ds (pci_bus_new -> qbus_create -> qbus_realize ->
>>>> object_property_add_child).
>>>>
>>>
>>> Yes...that`s true.
>>>
>>> and @Marcel, I think maybe this is final decision?
>>
>>
>> I say add a debug trace line before pxb_register_bus (or use the debugger)
>> and check ds->parent, bds->parent and bus->parent.
>>
>
> uh..sorry I don`t get it, what does the debug trace line/use debugger mean?
>
>> Run the qemu with -device pxb,bus=80,... and for every one that its parent
>> is not null add unparent. :)
>
> don`t get it too, could you detail it?


Sure, just add something like:

     fprintf(stderr, "ds parent: %p, bus parent... ", ds->parent ...)

Compile and run QEMU with a pxb device:
     <qemu-bin>  -device pxb,bus=80,...

And look for which object has a parent :)

Thanks,
Marcel

>
>>
>> Thanks,
>> Marcel
>>
>>
>>
>>
>>>
>>>> Paolo
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>>
>>
>

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

On 12/22/2015 05:35 PM, Marcel Apfelbaum wrote:
> On 12/22/2015 11:16 AM, Cao jin wrote:
>>
>>
>> On 12/22/2015 03:34 PM, Marcel Apfelbaum wrote:
>>> On 12/22/2015 05:58 AM, Cao jin wrote:
>>>>
>>>>
>>>> On 12/21/2015 11:49 PM, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 20/12/2015 12:38, Cao jin wrote:
>>>>>>>>
>>>>>>>> +    object_unref(OBJECT(ds));
>>>>>>>> +    object_unref(OBJECT(bds));
>>>>>>>> +    object_unref(OBJECT(bus));
>>>>>>>
>>>>>>> I think these should be object_unparent, not unref.
>>>>>>>
>>>>>>
>>>>>> But, it seems these 3 objects isn`t added as a child-property via
>>>>>> object_property_add_child() during creation, so OBJECT(ds)->parent(so
>>>>>> does the other 2) will be NULL, and so object_unparent will do
>>>>>> nothing?
>>>>>
>>>>> qdev_init_nofail adds them (qdev_init_nofail ->
>>>>> object_property_set_bool
>>>>> -> device_set_realized -> object_property_add_child).
>>>>>
>>>>> If you haven't reached qdev_init_nofail, you should indeed unref ds
>>>>> and
>>>>> bds instead.  However, the bus should be unparented because
>>>>> pci_bus_new
>>>>> makes it a child of ds (pci_bus_new -> qbus_create -> qbus_realize ->
>>>>> object_property_add_child).
>>>>>
>>>>
>>>> Yes...that`s true.
>>>>
>>>> and @Marcel, I think maybe this is final decision?
>>>
>>>
>>> I say add a debug trace line before pxb_register_bus (or use the
>>> debugger)
>>> and check ds->parent, bds->parent and bus->parent.
>>>
>>
>> uh..sorry I don`t get it, what does the debug trace line/use debugger
>> mean?
>>
>>> Run the qemu with -device pxb,bus=80,... and for every one that its
>>> parent
>>> is not null add unparent. :)
>>
>> don`t get it too, could you detail it?
>
>
> Sure, just add something like:
>
>      fprintf(stderr, "ds parent: %p, bus parent... ", ds->parent ...)
>
> Compile and run QEMU with a pxb device:
>      <qemu-bin>  -device pxb,bus=80,...
>
> And look for which object has a parent :)
>

Oh... my bad understanding:p I see now. I thought maybe you mean like this;)

if (bds->parent)
     object_unparent(bds);
else
     object_unref(bds)

> Thanks,
> Marcel
>
>>
>>>
>>> Thanks,
>>> Marcel
>>>
>>>
>>>
>>>
>>>>
>>>>> Paolo
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>
>
>
>
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()

[Cao jin]

Hi, Marcel

On 12/22/2015 05:35 PM, Marcel Apfelbaum wrote:
> On 12/22/2015 11:16 AM, Cao jin wrote:
[...]
>
>
> Sure, just add something like:
>
>      fprintf(stderr, "ds parent: %p, bus parent... ", ds->parent ...)
>
> Compile and run QEMU with a pxb device:
>      <qemu-bin>  -device pxb,bus=80,...
>
> And look for which object has a parent :)
>

got the result, the same as Paolo says. see:
   code:
     fprintf(stderr, "ds parent = %p\n", OBJECT(ds)->parent);
     fprintf(stderr, "bus parent = %p\n", OBJECT(bus)->parent);
     fprintf(stderr, "bds parent = %p\n", OBJECT(bds)->parent);

   got
     ds parent = (nil)
     bus parent = 0x555557db7c40
     bds parent = (nil)

So, I am gonna prepar V3:)

> Thanks,
> Marcel
>
[...]
-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH 4/5] gt64120: convert to realize()

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [PATCH 1/5] SH PCI Host: convert to realize()

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> On 12/19/2015 05:18 AM, Markus Armbruster wrote:
>> One short remark in addition to Eduardo's review.
>>
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
> [...]
>>>>
>>>>       config_fd = open(path, O_RDWR);
>>>>       if (config_fd < 0) {
>>>> +        error_setg(errp, "No such device");
>>>>           return -ENODEV;
>>>>       }
>>
>> Can we come up with nicer error messages?
>>
>
> Ok...will change the error msg to strerror(errno)

There's also error_setg_file_open(), not sure it fits here, but check it
out.

Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()

[Cao jin]

On 01/11/2016 10:32 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
[...]
>>>
>>
>> Ok...will change the error msg to strerror(errno)
>
> There's also error_setg_file_open(), not sure it fits here, but check it
> out.
>

Thanks for reminding. Gerd Hoffmann has whole 
patchset(http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00245.html) 
which covers this, so maybe this doesn`t need to be updated.

-- 
Yours Sincerely,

Cao jin