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

[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:
   -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:
   -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 = 0x57db7c40
bds parent = (nil)

So, I am gonna prepar V3:)


Thanks,
Marcel


[...]
--
Yours Sincerely,

Cao Jin

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]

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

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

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

[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 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 3/5] PXB: convert to realize()

[Marcel Apfelbaum]

Hi,

On 12/18/2015 01:03 PM, Cao jin wrote:

Signed-off-by: Cao jin 
---
  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, >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()

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

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

[Cao jin]

Signed-off-by: Cao jin 
---
 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, >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

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