* [Qemu-devel] [PATCH 0/5] Convert to realize()
@ 2015-12-18 11:03 Cao jin
2015-12-18 11:03 ` [Qemu-devel] [PATCH 1/5] SH PCI Host: convert " Cao jin
` (5 more replies)
0 siblings, 6 replies; 39+ messages in thread
From: Cao jin @ 2015-12-18 11:03 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 1/5] SH PCI Host: convert to realize()
2015-12-18 11:03 [Qemu-devel] [PATCH 0/5] Convert to realize() Cao jin
@ 2015-12-18 11:03 ` Cao jin
2015-12-18 17:59 ` Paolo Bonzini
2015-12-18 11:03 ` [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: " Cao jin
` (4 subsequent siblings)
5 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-18 11:03 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
2015-12-18 11:03 [Qemu-devel] [PATCH 0/5] Convert to realize() Cao jin
2015-12-18 11:03 ` [Qemu-devel] [PATCH 1/5] SH PCI Host: convert " Cao jin
@ 2015-12-18 11:03 ` Cao jin
2015-12-18 18:37 ` Eduardo Habkost
2015-12-18 11:03 ` [Qemu-devel] [PATCH 3/5] PXB: " Cao jin
` (3 subsequent siblings)
5 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-18 11:03 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-18 11:03 [Qemu-devel] [PATCH 0/5] Convert to realize() Cao jin
2015-12-18 11:03 ` [Qemu-devel] [PATCH 1/5] SH PCI Host: convert " Cao jin
2015-12-18 11:03 ` [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: " Cao jin
@ 2015-12-18 11:03 ` Cao jin
2015-12-18 18:01 ` Paolo Bonzini
2015-12-20 10:22 ` Marcel Apfelbaum
2015-12-18 11:03 ` [Qemu-devel] [PATCH 4/5] gt64120: " Cao jin
` (2 subsequent siblings)
5 siblings, 2 replies; 39+ messages in thread
From: Cao jin @ 2015-12-18 11:03 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 4/5] gt64120: convert to realize()
2015-12-18 11:03 [Qemu-devel] [PATCH 0/5] Convert to realize() Cao jin
` (2 preceding siblings ...)
2015-12-18 11:03 ` [Qemu-devel] [PATCH 3/5] PXB: " Cao jin
@ 2015-12-18 11:03 ` Cao jin
2015-12-18 17:59 ` Paolo Bonzini
2015-12-18 11:03 ` [Qemu-devel] [PATCH 5/5] xen-pvdevice: " Cao jin
2015-12-18 11:08 ` [Qemu-devel] [PATCH 0/5] Convert " Cao jin
5 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-18 11:03 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()
2015-12-18 11:03 [Qemu-devel] [PATCH 0/5] Convert to realize() Cao jin
` (3 preceding siblings ...)
2015-12-18 11:03 ` [Qemu-devel] [PATCH 4/5] gt64120: " Cao jin
@ 2015-12-18 11:03 ` Cao jin
2015-12-18 18:00 ` Paolo Bonzini
2015-12-21 15:15 ` Stefano Stabellini
2015-12-18 11:08 ` [Qemu-devel] [PATCH 0/5] Convert " Cao jin
5 siblings, 2 replies; 39+ messages in thread
From: Cao jin @ 2015-12-18 11:03 UTC (permalink / raw)
To: qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] Convert to realize()
2015-12-18 11:03 [Qemu-devel] [PATCH 0/5] Convert to realize() Cao jin
` (4 preceding siblings ...)
2015-12-18 11:03 ` [Qemu-devel] [PATCH 5/5] xen-pvdevice: " Cao jin
@ 2015-12-18 11:08 ` Cao jin
5 siblings, 0 replies; 39+ messages in thread
From: Cao jin @ 2015-12-18 11:08 UTC (permalink / raw)
To: qemu-devel
Cc: ehabkost, mst, Markus Armbruster, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] SH PCI Host: convert to realize()
2015-12-18 11:03 ` [Qemu-devel] [PATCH 1/5] SH PCI Host: convert " Cao jin
@ 2015-12-18 17:59 ` Paolo Bonzini
2016-01-10 18:13 ` Michael Tokarev
0 siblings, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2015-12-18 17:59 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: ehabkost, mst, qemu-trivial, leon.alrae, aurelien, rth
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;
> /*
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] gt64120: convert to realize()
2015-12-18 11:03 ` [Qemu-devel] [PATCH 4/5] gt64120: " Cao jin
@ 2015-12-18 17:59 ` Paolo Bonzini
2016-01-10 18:12 ` Michael Tokarev
0 siblings, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2015-12-18 17:59 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: ehabkost, mst, qemu-trivial, leon.alrae, aurelien, rth
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;
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()
2015-12-18 11:03 ` [Qemu-devel] [PATCH 5/5] xen-pvdevice: " Cao jin
@ 2015-12-18 18:00 ` Paolo Bonzini
2015-12-21 5:52 ` Cao jin
2015-12-21 15:15 ` Stefano Stabellini
1 sibling, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2015-12-18 18:00 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: ehabkost, Stefano Stabellini, mst, leon.alrae, aurelien, rth
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;
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-18 11:03 ` [Qemu-devel] [PATCH 3/5] PXB: " Cao jin
@ 2015-12-18 18:01 ` Paolo Bonzini
2015-12-20 11:38 ` Cao jin
2015-12-20 10:22 ` Marcel Apfelbaum
1 sibling, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2015-12-18 18:01 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: mst, leon.alrae, ehabkost, aurelien, rth
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;
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
2015-12-18 11:03 ` [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: " Cao jin
@ 2015-12-18 18:37 ` Eduardo Habkost
2015-12-18 21:18 ` Markus Armbruster
2015-12-20 11:56 ` Cao jin
0 siblings, 2 replies; 39+ messages in thread
From: Eduardo Habkost @ 2015-12-18 18:37 UTC (permalink / raw)
To: Cao jin; +Cc: mst, qemu-devel, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
2015-12-18 18:37 ` Eduardo Habkost
@ 2015-12-18 21:18 ` Markus Armbruster
2015-12-20 11:59 ` Cao jin
2015-12-20 11:56 ` Cao jin
1 sibling, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2015-12-18 21:18 UTC (permalink / raw)
To: Eduardo Habkost
Cc: mst, qemu-devel, Cao jin, pbonzini, leon.alrae, aurelien, rth
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?
[...]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-18 11:03 ` [Qemu-devel] [PATCH 3/5] PXB: " Cao jin
2015-12-18 18:01 ` Paolo Bonzini
@ 2015-12-20 10:22 ` Marcel Apfelbaum
2015-12-20 10:48 ` Cao jin
1 sibling, 1 reply; 39+ messages in thread
From: Marcel Apfelbaum @ 2015-12-20 10:22 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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;
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-20 10:22 ` Marcel Apfelbaum
@ 2015-12-20 10:48 ` Cao jin
2015-12-20 11:21 ` Marcel Apfelbaum
0 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-20 10:48 UTC (permalink / raw)
To: marcel, qemu-devel; +Cc: ehabkost, mst, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-20 10:48 ` Cao jin
@ 2015-12-20 11:21 ` Marcel Apfelbaum
2015-12-21 2:59 ` Cao jin
0 siblings, 1 reply; 39+ messages in thread
From: Marcel Apfelbaum @ 2015-12-20 11:21 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: ehabkost, Michael S. Tsirkin, Markus Armbruster, pbonzini,
leon.alrae, Andreas Färber, aurelien, rth
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;
>>>
>>
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-18 18:01 ` Paolo Bonzini
@ 2015-12-20 11:38 ` Cao jin
2015-12-21 10:39 ` Cao jin
2015-12-21 15:49 ` Paolo Bonzini
0 siblings, 2 replies; 39+ messages in thread
From: Cao jin @ 2015-12-20 11:38 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: mst, leon.alrae, ehabkost, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
2015-12-18 18:37 ` Eduardo Habkost
2015-12-18 21:18 ` Markus Armbruster
@ 2015-12-20 11:56 ` Cao jin
1 sibling, 0 replies; 39+ messages in thread
From: Cao jin @ 2015-12-20 11:56 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: mst, qemu-devel, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
2015-12-18 21:18 ` Markus Armbruster
@ 2015-12-20 11:59 ` Cao jin
2016-01-11 14:32 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-20 11:59 UTC (permalink / raw)
To: Markus Armbruster, Eduardo Habkost
Cc: mst, qemu-devel, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-20 11:21 ` Marcel Apfelbaum
@ 2015-12-21 2:59 ` Cao jin
2015-12-21 10:08 ` Marcel Apfelbaum
0 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-21 2:59 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel
Cc: ehabkost, mst, Markus Armbruster, pbonzini, leon.alrae,
Andreas Färber, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()
2015-12-18 18:00 ` Paolo Bonzini
@ 2015-12-21 5:52 ` Cao jin
2015-12-21 15:00 ` Stefano Stabellini
0 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-21 5:52 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: ehabkost, Stefano Stabellini, mst, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-21 2:59 ` Cao jin
@ 2015-12-21 10:08 ` Marcel Apfelbaum
2015-12-21 10:19 ` Cao jin
0 siblings, 1 reply; 39+ messages in thread
From: Marcel Apfelbaum @ 2015-12-21 10:08 UTC (permalink / raw)
To: Cao jin, qemu-devel
Cc: ehabkost, mst, Markus Armbruster, pbonzini, leon.alrae,
Andreas Färber, aurelien, rth
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;
>>>>>
>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-21 10:08 ` Marcel Apfelbaum
@ 2015-12-21 10:19 ` Cao jin
0 siblings, 0 replies; 39+ messages in thread
From: Cao jin @ 2015-12-21 10:19 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel
Cc: ehabkost, mst, Markus Armbruster, pbonzini, leon.alrae,
Andreas Färber, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-20 11:38 ` Cao jin
@ 2015-12-21 10:39 ` Cao jin
2015-12-21 15:49 ` Paolo Bonzini
1 sibling, 0 replies; 39+ messages in thread
From: Cao jin @ 2015-12-21 10:39 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: rth, leon.alrae, ehabkost, aurelien, mst
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()
2015-12-21 5:52 ` Cao jin
@ 2015-12-21 15:00 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2015-12-21 15:00 UTC (permalink / raw)
To: Cao jin
Cc: ehabkost, Stefano Stabellini, mst, qemu-devel, Paolo Bonzini,
leon.alrae, aurelien, rth
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
>
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()
2015-12-18 11:03 ` [Qemu-devel] [PATCH 5/5] xen-pvdevice: " Cao jin
2015-12-18 18:00 ` Paolo Bonzini
@ 2015-12-21 15:15 ` Stefano Stabellini
2015-12-22 1:24 ` Cao jin
1 sibling, 1 reply; 39+ messages in thread
From: Stefano Stabellini @ 2015-12-21 15:15 UTC (permalink / raw)
To: Cao jin; +Cc: ehabkost, mst, qemu-devel, pbonzini, leon.alrae, aurelien, rth
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
>
>
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-20 11:38 ` Cao jin
2015-12-21 10:39 ` Cao jin
@ 2015-12-21 15:49 ` Paolo Bonzini
2015-12-22 3:58 ` Cao jin
1 sibling, 1 reply; 39+ messages in thread
From: Paolo Bonzini @ 2015-12-21 15:49 UTC (permalink / raw)
To: Cao jin, qemu-devel; +Cc: mst, leon.alrae, ehabkost, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()
2015-12-21 15:15 ` Stefano Stabellini
@ 2015-12-22 1:24 ` Cao jin
2015-12-22 2:40 ` Cao jin
0 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-22 1:24 UTC (permalink / raw)
To: Stefano Stabellini
Cc: ehabkost, mst, qemu-devel, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] xen-pvdevice: convert to realize()
2015-12-22 1:24 ` Cao jin
@ 2015-12-22 2:40 ` Cao jin
0 siblings, 0 replies; 39+ messages in thread
From: Cao jin @ 2015-12-22 2:40 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-21 15:49 ` Paolo Bonzini
@ 2015-12-22 3:58 ` Cao jin
2015-12-22 7:34 ` Marcel Apfelbaum
0 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-22 3:58 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Marcel Apfelbaum, mst
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-22 3:58 ` Cao jin
@ 2015-12-22 7:34 ` Marcel Apfelbaum
2015-12-22 9:16 ` Cao jin
0 siblings, 1 reply; 39+ messages in thread
From: Marcel Apfelbaum @ 2015-12-22 7:34 UTC (permalink / raw)
To: Cao jin, Paolo Bonzini, qemu-devel; +Cc: Marcel Apfelbaum, mst
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
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-22 7:34 ` Marcel Apfelbaum
@ 2015-12-22 9:16 ` Cao jin
2015-12-22 9:35 ` Marcel Apfelbaum
0 siblings, 1 reply; 39+ messages in thread
From: Cao jin @ 2015-12-22 9:16 UTC (permalink / raw)
To: marcel; +Cc: Paolo Bonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-22 9:16 ` Cao jin
@ 2015-12-22 9:35 ` Marcel Apfelbaum
2015-12-22 9:52 ` Cao jin
2015-12-22 11:40 ` Cao jin
0 siblings, 2 replies; 39+ messages in thread
From: Marcel Apfelbaum @ 2015-12-22 9:35 UTC (permalink / raw)
To: Cao jin; +Cc: Paolo Bonzini, qemu-devel, mst
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
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-22 9:35 ` Marcel Apfelbaum
@ 2015-12-22 9:52 ` Cao jin
2015-12-22 11:40 ` Cao jin
1 sibling, 0 replies; 39+ messages in thread
From: Cao jin @ 2015-12-22 9:52 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: Paolo Bonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] PXB: convert to realize()
2015-12-22 9:35 ` Marcel Apfelbaum
2015-12-22 9:52 ` Cao jin
@ 2015-12-22 11:40 ` Cao jin
1 sibling, 0 replies; 39+ messages in thread
From: Cao jin @ 2015-12-22 11:40 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: Paolo Bonzini, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] gt64120: convert to realize()
2015-12-18 17:59 ` Paolo Bonzini
@ 2016-01-10 18:12 ` Michael Tokarev
0 siblings, 0 replies; 39+ messages in thread
From: Michael Tokarev @ 2016-01-10 18:12 UTC (permalink / raw)
To: Paolo Bonzini, Cao jin, qemu-devel
Cc: qemu-trivial, rth, leon.alrae, ehabkost, mst
Applied to -trivial, thanks!
/mjt
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] SH PCI Host: convert to realize()
2015-12-18 17:59 ` Paolo Bonzini
@ 2016-01-10 18:13 ` Michael Tokarev
0 siblings, 0 replies; 39+ messages in thread
From: Michael Tokarev @ 2016-01-10 18:13 UTC (permalink / raw)
To: Paolo Bonzini, Cao jin, qemu-devel
Cc: qemu-trivial, rth, leon.alrae, ehabkost, mst
Applied to -trivial, thanks!
/mjt
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
2015-12-20 11:59 ` Cao jin
@ 2016-01-11 14:32 ` Markus Armbruster
2016-01-12 2:24 ` Cao jin
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2016-01-11 14:32 UTC (permalink / raw)
To: Cao jin
Cc: Eduardo Habkost, mst, qemu-devel, pbonzini, leon.alrae, aurelien, rth
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.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: convert to realize()
2016-01-11 14:32 ` Markus Armbruster
@ 2016-01-12 2:24 ` Cao jin
0 siblings, 0 replies; 39+ messages in thread
From: Cao jin @ 2016-01-12 2:24 UTC (permalink / raw)
To: Markus Armbruster
Cc: Eduardo Habkost, mst, qemu-devel, pbonzini, leon.alrae, aurelien, rth
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
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2016-01-12 2:21 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-18 11:03 [Qemu-devel] [PATCH 0/5] Convert to realize() Cao jin
2015-12-18 11:03 ` [Qemu-devel] [PATCH 1/5] SH PCI Host: convert " Cao jin
2015-12-18 17:59 ` Paolo Bonzini
2016-01-10 18:13 ` Michael Tokarev
2015-12-18 11:03 ` [Qemu-devel] [PATCH 2/5] igd-passthrough-i440FX: " Cao jin
2015-12-18 18:37 ` Eduardo Habkost
2015-12-18 21:18 ` Markus Armbruster
2015-12-20 11:59 ` Cao jin
2016-01-11 14:32 ` Markus Armbruster
2016-01-12 2:24 ` Cao jin
2015-12-20 11:56 ` Cao jin
2015-12-18 11:03 ` [Qemu-devel] [PATCH 3/5] PXB: " Cao jin
2015-12-18 18:01 ` Paolo Bonzini
2015-12-20 11:38 ` Cao jin
2015-12-21 10:39 ` Cao jin
2015-12-21 15:49 ` Paolo Bonzini
2015-12-22 3:58 ` Cao jin
2015-12-22 7:34 ` Marcel Apfelbaum
2015-12-22 9:16 ` Cao jin
2015-12-22 9:35 ` Marcel Apfelbaum
2015-12-22 9:52 ` Cao jin
2015-12-22 11:40 ` Cao jin
2015-12-20 10:22 ` Marcel Apfelbaum
2015-12-20 10:48 ` Cao jin
2015-12-20 11:21 ` Marcel Apfelbaum
2015-12-21 2:59 ` Cao jin
2015-12-21 10:08 ` Marcel Apfelbaum
2015-12-21 10:19 ` Cao jin
2015-12-18 11:03 ` [Qemu-devel] [PATCH 4/5] gt64120: " Cao jin
2015-12-18 17:59 ` Paolo Bonzini
2016-01-10 18:12 ` Michael Tokarev
2015-12-18 11:03 ` [Qemu-devel] [PATCH 5/5] xen-pvdevice: " Cao jin
2015-12-18 18:00 ` Paolo Bonzini
2015-12-21 5:52 ` Cao jin
2015-12-21 15:00 ` Stefano Stabellini
2015-12-21 15:15 ` Stefano Stabellini
2015-12-22 1:24 ` Cao jin
2015-12-22 2:40 ` Cao jin
2015-12-18 11:08 ` [Qemu-devel] [PATCH 0/5] Convert " Cao jin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.