[Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[xiaoqiang zhao]

This is some QOM'ify work relate with ppc.
See each commit message for details.

xiaoqiang zhao (4):
  hw/gpio: QOM'ify mpc8xxx.c
  hw/ppc: QOM'ify e500.c
  hw/ppc: QOM'ify ppce500_spin.c
  hw/ppc: QOM'ify spapr_vio.c

 hw/gpio/mpc8xxx.c     | 20 +++++++++++---------
 hw/ppc/e500.c         | 17 ++++-------------
 hw/ppc/ppce500_spin.c | 18 ++++++++----------
 hw/ppc/spapr_vio.c    |  2 --
 4 files changed, 23 insertions(+), 34 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/4] hw/gpio: QOM'ify mpc8xxx.c

[xiaoqiang zhao]

* Drop the old SysBus init function and use instance_init
* Change mpc8xxx_gpio_reset to a DeviceClass::reset function

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/gpio/mpc8xxx.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/gpio/mpc8xxx.c b/hw/gpio/mpc8xxx.c
index d149719469..e12edb4933 100644
--- a/hw/gpio/mpc8xxx.c
+++ b/hw/gpio/mpc8xxx.c
@@ -143,8 +143,10 @@ static void mpc8xxx_gpio_write(void *opaque, hwaddr offset,
     mpc8xxx_gpio_update(s);
 }
 
-static void mpc8xxx_gpio_reset(MPC8XXXGPIOState *s)
+static void mpc8xxx_gpio_reset(DeviceState *dev)
 {
+    MPC8XXXGPIOState *s = MPC8XXX_GPIO(dev);
+
     s->dir = 0;
     s->odr = 0;
     s->dat = 0;
@@ -180,33 +182,33 @@ static const MemoryRegionOps mpc8xxx_gpio_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static int mpc8xxx_gpio_initfn(SysBusDevice *sbd)
+static void mpc8xxx_gpio_initfn(Object *obj)
 {
-    DeviceState *dev = DEVICE(sbd);
-    MPC8XXXGPIOState *s = MPC8XXX_GPIO(dev);
+    DeviceState *dev = DEVICE(obj);
+    MPC8XXXGPIOState *s = MPC8XXX_GPIO(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    memory_region_init_io(&s->iomem, OBJECT(s), &mpc8xxx_gpio_ops, s, "mpc8xxx_gpio", 0x1000);
+    memory_region_init_io(&s->iomem, obj, &mpc8xxx_gpio_ops,
+                          s, "mpc8xxx_gpio", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
     sysbus_init_irq(sbd, &s->irq);
     qdev_init_gpio_in(dev, mpc8xxx_gpio_set_irq, 32);
     qdev_init_gpio_out(dev, s->out, 32);
-    mpc8xxx_gpio_reset(s);
-    return 0;
 }
 
 static void mpc8xxx_gpio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-    k->init = mpc8xxx_gpio_initfn;
     dc->vmsd = &vmstate_mpc8xxx_gpio;
+    dc->reset = mpc8xxx_gpio_reset;
 }
 
 static const TypeInfo mpc8xxx_gpio_info = {
     .name          = TYPE_MPC8XXX_GPIO,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(MPC8XXXGPIOState),
+    .instance_init = mpc8xxx_gpio_initfn,
     .class_init    = mpc8xxx_gpio_class_init,
 };
 
-- 
2.11.0

[Qemu-devel] [PATCH 2/4] hw/ppc: QOM'ify e500.c

[xiaoqiang zhao]

Drop the old SysBus init function and use instance_init

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/ppc/e500.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index cf8b122afe..792bd79d39 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1049,27 +1049,18 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
     boot_info->dt_size = dt_size;
 }
 
-static int e500_ccsr_initfn(SysBusDevice *dev)
+static void e500_ccsr_initfn(Object *obj)
 {
-    PPCE500CCSRState *ccsr;
-
-    ccsr = CCSR(dev);
-    memory_region_init(&ccsr->ccsr_space, OBJECT(ccsr), "e500-ccsr",
+    PPCE500CCSRState *ccsr = CCSR(obj);
+    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
                        MPC8544_CCSRBAR_SIZE);
-    return 0;
-}
-
-static void e500_ccsr_class_init(ObjectClass *klass, void *data)
-{
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-    k->init = e500_ccsr_initfn;
 }
 
 static const TypeInfo e500_ccsr_info = {
     .name          = TYPE_CCSR,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(PPCE500CCSRState),
-    .class_init    = e500_ccsr_class_init,
+    .instance_init = e500_ccsr_initfn,
 };
 
 static void e500_register_types(void)
-- 
2.11.0

[Qemu-devel] [PATCH 3/4] hw/ppc: QOM'ify ppce500_spin.c

[xiaoqiang zhao]

Drop the old SysBus init function and use instance_init

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/ppc/ppce500_spin.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
index cf958a9e00..958536f6c6 100644
--- a/hw/ppc/ppce500_spin.c
+++ b/hw/ppc/ppce500_spin.c
@@ -54,9 +54,9 @@ typedef struct SpinState {
     SpinInfo spin[MAX_CPUS];
 } SpinState;
 
-static void spin_reset(void *opaque)
+static void spin_reset(DeviceState *dev)
 {
-    SpinState *s = opaque;
+    SpinState *s = E500_SPIN(dev);
     int i;
 
     for (i = 0; i < MAX_CPUS; i++) {
@@ -174,30 +174,28 @@ static const MemoryRegionOps spin_rw_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static int ppce500_spin_initfn(SysBusDevice *dev)
+static void ppce500_spin_initfn(Object *obj)
 {
+    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
     SpinState *s = E500_SPIN(dev);
 
-    memory_region_init_io(&s->iomem, OBJECT(s), &spin_rw_ops, s,
+    memory_region_init_io(&s->iomem, obj, &spin_rw_ops, s,
                           "e500 spin pv device", sizeof(SpinInfo) * MAX_CPUS);
     sysbus_init_mmio(dev, &s->iomem);
-
-    qemu_register_reset(spin_reset, s);
-
-    return 0;
 }
 
 static void ppce500_spin_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->init = ppce500_spin_initfn;
+    dc->reset = spin_reset;
 }
 
 static const TypeInfo ppce500_spin_info = {
     .name          = TYPE_E500_SPIN,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SpinState),
+    .instance_init = ppce500_spin_initfn,
     .class_init    = ppce500_spin_class_init,
 };
 
-- 
2.11.0

[Qemu-devel] [PATCH 4/4] hw/ppc: QOM'ify spapr_vio.c

[xiaoqiang zhao]

Drop the old and empty SysBus init

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/ppc/spapr_vio.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index cc1e09c568..1739b73a13 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -548,11 +548,9 @@ static int spapr_vio_bridge_init(SysBusDevice *dev)
 
 static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
 {
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->fw_name = "vdevice";
-    k->init = spapr_vio_bridge_init;
 }
 
 static const TypeInfo spapr_vio_bridge_info = {
-- 
2.11.0

Re: [Qemu-devel] [PATCH 4/4] hw/ppc: QOM'ify spapr_vio.c

[David Gibson]

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

On Sat, Dec 31, 2016 at 09:18:31AM +0800, xiaoqiang zhao wrote:
> Drop the old and empty SysBus init
> 
> Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>

This should also actually remove the empty function.

> ---
>  hw/ppc/spapr_vio.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index cc1e09c568..1739b73a13 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -548,11 +548,9 @@ static int spapr_vio_bridge_init(SysBusDevice *dev)
>  
>  static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
>  {
> -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->fw_name = "vdevice";
> -    k->init = spapr_vio_bridge_init;
>  }
>  
>  static const TypeInfo spapr_vio_bridge_info = {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[David Gibson]

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

On Sat, Dec 31, 2016 at 09:18:27AM +0800, xiaoqiang zhao wrote:
> This is some QOM'ify work relate with ppc.
> See each commit message for details.
> 
> xiaoqiang zhao (4):
>   hw/gpio: QOM'ify mpc8xxx.c
>   hw/ppc: QOM'ify e500.c
>   hw/ppc: QOM'ify ppce500_spin.c
>   hw/ppc: QOM'ify spapr_vio.c
> 
>  hw/gpio/mpc8xxx.c     | 20 +++++++++++---------
>  hw/ppc/e500.c         | 17 ++++-------------
>  hw/ppc/ppce500_spin.c | 18 ++++++++----------
>  hw/ppc/spapr_vio.c    |  2 --
>  4 files changed, 23 insertions(+), 34 deletions(-)

Patches 1-3 all have the same problem - they move memory region
initialization and similar to an instance_init function.  This is not
how things are generally done in the qdev model.  Instead that phase
of initialization should be done from a dc->realize() function.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[赵小强]

Hi，david：

   To my understanding，what must be put in the realize function  is code which depends on property values. What's the benefit of moving memory region initialization into realize function?  I can not figure out, can you make some explanations?
    Thanks for your review.

Best wishes ！

> 在 2017年1月3日，06:28，David Gibson <david@gibson.dropbear.id.au> 写道：
> 
>> On Sat, Dec 31, 2016 at 09:18:27AM +0800, xiaoqiang zhao wrote:
>> This is some QOM'ify work relate with ppc.
>> See each commit message for details.
>> 
>> xiaoqiang zhao (4):
>>  hw/gpio: QOM'ify mpc8xxx.c
>>  hw/ppc: QOM'ify e500.c
>>  hw/ppc: QOM'ify ppce500_spin.c
>>  hw/ppc: QOM'ify spapr_vio.c
>> 
>> hw/gpio/mpc8xxx.c     | 20 +++++++++++---------
>> hw/ppc/e500.c         | 17 ++++-------------
>> hw/ppc/ppce500_spin.c | 18 ++++++++----------
>> hw/ppc/spapr_vio.c    |  2 --
>> 4 files changed, 23 insertions(+), 34 deletions(-)
> 
> Patches 1-3 all have the same problem - they move memory region
> initialization and similar to an instance_init function.  This is not
> how things are generally done in the qdev model.  Instead that phase
> of initialization should be done from a dc->realize() function.
> 
> -- 
> David Gibson            | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au    | minimalist, thank you.  NOT _the_ _other_
>                | _way_ _around_!
> http://www.ozlabs.org/~dgibson
> [attachment]
> 
> signature.asc
> download: http://preview.mail.163.com/xdownload?filename=signature.asc&mid=1tbiqBVTxlc67OfNrwABst&part=2&sign=95585277ead1794d396d48ba29e73240&time=1483402980&uid=zxq_yx_007%40163.com
> 
> preview: http://preview.mail.163.com/preview?mid=1tbiqBVTxlc67OfNrwABst&part=2&sign=95585277ead1794d396d48ba29e73240&time=1483402980&uid=zxq_yx_007%40163.com
> 

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[David Gibson]

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

On Tue, Jan 03, 2017 at 10:02:21PM +0800, 赵小强 wrote:
> Hi，david：
> 
>    To my understanding，what must be put in the realize function  is
>    code which depends on property values. What's the benefit of
>    moving memory region initialization into realize function?  I can
>    not figure out, can you make some explanations?

If nothing else it's better in realize() for consistency with other
devices.

I'm not familiar enough with the details to be sure, but I also think
it's not safe in instance_init.  Once memory regions are registered,
the device can potentially interact with other devices in the virtual
machine.  realize() is sequenced to expect that, instance_init is not.

>     Thanks for your review.
> 
> Best wishes ！
> 
> > 在 2017年1月3日，06:28，David Gibson <david@gibson.dropbear.id.au> 写道：
> > 
> >> On Sat, Dec 31, 2016 at 09:18:27AM +0800, xiaoqiang zhao wrote:
> >> This is some QOM'ify work relate with ppc.
> >> See each commit message for details.
> >> 
> >> xiaoqiang zhao (4):
> >>  hw/gpio: QOM'ify mpc8xxx.c
> >>  hw/ppc: QOM'ify e500.c
> >>  hw/ppc: QOM'ify ppce500_spin.c
> >>  hw/ppc: QOM'ify spapr_vio.c
> >> 
> >> hw/gpio/mpc8xxx.c     | 20 +++++++++++---------
> >> hw/ppc/e500.c         | 17 ++++-------------
> >> hw/ppc/ppce500_spin.c | 18 ++++++++----------
> >> hw/ppc/spapr_vio.c    |  2 --
> >> 4 files changed, 23 insertions(+), 34 deletions(-)
> > 
> > Patches 1-3 all have the same problem - they move memory region
> > initialization and similar to an instance_init function.  This is not
> > how things are generally done in the qdev model.  Instead that phase
> > of initialization should be done from a dc->realize() function.
> > 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[赵小强]

Hmm，sounds reasonable. I will take a look.

PS: guys, some other comments about this?

Best wishes ！

>> 在 2017年1月4日，11:28，David Gibson <david@gibson.dropbear.id.au> 写道：
>> 
>> On Tue, Jan 03, 2017 at 10:02:21PM +0800, 赵小强 wrote:
>> Hi，david：
>> 
>>  To my understanding，what must be put in the realize function  is
>>  code which depends on property values. What's the benefit of
>>  moving memory region initialization into realize function?  I can
>>  not figure out, can you make some explanations?
> 
> If nothing else it's better in realize() for consistency with other
> devices.
> 
> I'm not familiar enough with the details to be sure, but I also think
> it's not safe in instance_init.  Once memory regions are registered,
> the device can potentially interact with other devices in the virtual
> machine.  realize() is sequenced to expect that, instance_init is not.
> 
>>   Thanks for your review.
>> 
>> Best wishes ！
>> 
>>>> 在 2017年1月3日，06:28，David Gibson <david@gibson.dropbear.id.au> 写道：
>>>> 
>>>> On Sat, Dec 31, 2016 at 09:18:27AM +0800, xiaoqiang zhao wrote:
>>>> This is some QOM'ify work relate with ppc.
>>>> See each commit message for details.
>>>> 
>>>> xiaoqiang zhao (4):
>>>> hw/gpio: QOM'ify mpc8xxx.c
>>>> hw/ppc: QOM'ify e500.c
>>>> hw/ppc: QOM'ify ppce500_spin.c
>>>> hw/ppc: QOM'ify spapr_vio.c
>>>> 
>>>> hw/gpio/mpc8xxx.c     | 20 +++++++++++---------
>>>> hw/ppc/e500.c         | 17 ++++-------------
>>>> hw/ppc/ppce500_spin.c | 18 ++++++++----------
>>>> hw/ppc/spapr_vio.c    |  2 --
>>>> 4 files changed, 23 insertions(+), 34 deletions(-)
>>> 
>>> Patches 1-3 all have the same problem - they move memory region
>>> initialization and similar to an instance_init function.  This is not
>>> how things are generally done in the qdev model.  Instead that phase
>>> of initialization should be done from a dc->realize() function.
> 
> -- 
> David Gibson            | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au    | minimalist, thank you.  NOT _the_ _other_
>               | _way_ _around_!
> http://www.ozlabs.org/~dgibson
> 【来自网易邮箱的超大附件】
> 邮件带有附件预览链接，若您转发或回复此邮件时不希望对方预览附件，建议您手动删除链接。
> 
> signature.asc
> 下载: http://u.163.com/t0/wilj3q882UKg
> 
> 预览: http://u.163.com/t0/KpNY8oHelqjD
> 

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[Peter Maydell]

On 4 January 2017 at 03:28, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Jan 03, 2017 at 10:02:21PM +0800, 赵小强 wrote:
>> Hi，david：
>>
>>    To my understanding，what must be put in the realize function  is
>>    code which depends on property values. What's the benefit of
>>    moving memory region initialization into realize function?  I can
>>    not figure out, can you make some explanations?
>
> If nothing else it's better in realize() for consistency with other
> devices.

I'm not sure we're terribly consistent at all, really. My understanding
was about the same as 赵小强 -- put stuff in init unless it has to
go in realize because it depends on properties or might fail or
has permanent effects on the simulation.
Lots of existing devices do memory_region_init* calls in
their init functions.
We should probably write down our preferences somewhere, perhaps
http://wiki.qemu.org/Documentation/QOMConventions

> I'm not familiar enough with the details to be sure, but I also think
> it's not safe in instance_init.  Once memory regions are registered,
> the device can potentially interact with other devices in the virtual
> machine.  realize() is sequenced to expect that, instance_init is not.

Hmm, that doesn't sound right to me. The other devices will only
interact with the memory regions when the calling code has
finished doing the create/realize/map memory regions sequence --
an MR on its own doesn't do anything unless somebody maps it into
an address space.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[David Gibson]

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

On Wed, Jan 04, 2017 at 05:04:02PM +0000, Peter Maydell wrote:
> On 4 January 2017 at 03:28, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Tue, Jan 03, 2017 at 10:02:21PM +0800, 赵小强 wrote:
> >> Hi，david：
> >>
> >>    To my understanding，what must be put in the realize function  is
> >>    code which depends on property values. What's the benefit of
> >>    moving memory region initialization into realize function?  I can
> >>    not figure out, can you make some explanations?
> >
> > If nothing else it's better in realize() for consistency with other
> > devices.
> 
> I'm not sure we're terribly consistent at all, really. My understanding
> was about the same as 赵小强 -- put stuff in init unless it has to
> go in realize because it depends on properties or might fail or
> has permanent effects on the simulation.
> Lots of existing devices do memory_region_init* calls in
> their init functions.
> We should probably write down our preferences somewhere, perhaps
> http://wiki.qemu.org/Documentation/QOMConventions
> 
> > I'm not familiar enough with the details to be sure, but I also think
> > it's not safe in instance_init.  Once memory regions are registered,
> > the device can potentially interact with other devices in the virtual
> > machine.  realize() is sequenced to expect that, instance_init is not.
> 
> Hmm, that doesn't sound right to me. The other devices will only
> interact with the memory regions when the calling code has
> finished doing the create/realize/map memory regions sequence --
> an MR on its own doesn't do anything unless somebody maps it into
> an address space.

Huh.  Ok, I guess I was wrong.

Alright, 赵小强, feel free to repost addressing just the other
comments and I'll merge.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[赵小强]

Ok！

Just one more comment：
After check the code flow, It's clear that the initialized memory region must be add to address space by calling memory_region_add_subregion in platform code before it can be accessed.

Best wishes ！

> 在 2017年1月5日，08:20，David Gibson <david@gibson.dropbear.id.au> 写道：
> 
>> On Wed, Jan 04, 2017 at 05:04:02PM +0000, Peter Maydell wrote:
>>> On 4 January 2017 at 03:28, David Gibson <david@gibson.dropbear.id.au> wrote:
>>>> On Tue, Jan 03, 2017 at 10:02:21PM +0800, 赵小强 wrote:
>>>> Hi，david：
>>>> 
>>>>   To my understanding，what must be put in the realize function  is
>>>>   code which depends on property values. What's the benefit of
>>>>   moving memory region initialization into realize function?  I can
>>>>   not figure out, can you make some explanations?
>>> 
>>> If nothing else it's better in realize() for consistency with other
>>> devices.
>> 
>> I'm not sure we're terribly consistent at all, really. My understanding
>> was about the same as 赵小强 -- put stuff in init unless it has to
>> go in realize because it depends on properties or might fail or
>> has permanent effects on the simulation.
>> Lots of existing devices do memory_region_init* calls in
>> their init functions.
>> We should probably write down our preferences somewhere, perhaps
>> http://wiki.qemu.org/Documentation/QOMConventions
>> 
>>> I'm not familiar enough with the details to be sure, but I also think
>>> it's not safe in instance_init.  Once memory regions are registered,
>>> the device can potentially interact with other devices in the virtual
>>> machine.  realize() is sequenced to expect that, instance_init is not.
>> 
>> Hmm, that doesn't sound right to me. The other devices will only
>> interact with the memory regions when the calling code has
>> finished doing the create/realize/map memory regions sequence --
>> an MR on its own doesn't do anything unless somebody maps it into
>> an address space.
> 
> Huh.  Ok, I guess I was wrong.
> 
> Alright, 赵小强, feel free to repost addressing just the other
> comments and I'll merge.
> 
> -- 
> David Gibson            | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au    | minimalist, thank you.  NOT _the_ _other_
>                | _way_ _around_!
> http://www.ozlabs.org/~dgibson
> 【来自网易邮箱的超大附件】
> 邮件带有附件预览链接，若您转发或回复此邮件时不希望对方预览附件，建议您手动删除链接。
> 
> signature.asc
> 下载: http://u.163.com/t0/EPKVtbajXUHs
> 
> 预览: http://u.163.com/t0/VUdsq6tC
> 

Re: [Qemu-devel] [PATCH 0/4] QOM'ify work for ppc

[David Gibson]

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

On Thu, Jan 05, 2017 at 08:53:07AM +0800, 赵小强 wrote:
> Ok！
> 
> Just one more comment：
> After check the code flow, It's clear that the initialized memory
> region must be add to address space by calling
> memory_region_add_subregion in platform code before it can be
> accessed.

Yes.. but I don't see how that's relevant to the discussion at hand.

> 
> Best wishes ！
> 
> > 在 2017年1月5日，08:20，David Gibson <david@gibson.dropbear.id.au> 写道：
> > 
> >> On Wed, Jan 04, 2017 at 05:04:02PM +0000, Peter Maydell wrote:
> >>> On 4 January 2017 at 03:28, David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>> On Tue, Jan 03, 2017 at 10:02:21PM +0800, 赵小强 wrote:
> >>>> Hi，david：
> >>>> 
> >>>>   To my understanding，what must be put in the realize function  is
> >>>>   code which depends on property values. What's the benefit of
> >>>>   moving memory region initialization into realize function?  I can
> >>>>   not figure out, can you make some explanations?
> >>> 
> >>> If nothing else it's better in realize() for consistency with other
> >>> devices.
> >> 
> >> I'm not sure we're terribly consistent at all, really. My understanding
> >> was about the same as 赵小强 -- put stuff in init unless it has to
> >> go in realize because it depends on properties or might fail or
> >> has permanent effects on the simulation.
> >> Lots of existing devices do memory_region_init* calls in
> >> their init functions.
> >> We should probably write down our preferences somewhere, perhaps
> >> http://wiki.qemu.org/Documentation/QOMConventions
> >> 
> >>> I'm not familiar enough with the details to be sure, but I also think
> >>> it's not safe in instance_init.  Once memory regions are registered,
> >>> the device can potentially interact with other devices in the virtual
> >>> machine.  realize() is sequenced to expect that, instance_init is not.
> >> 
> >> Hmm, that doesn't sound right to me. The other devices will only
> >> interact with the memory regions when the calling code has
> >> finished doing the create/realize/map memory regions sequence --
> >> an MR on its own doesn't do anything unless somebody maps it into
> >> an address space.
> > 
> > Huh.  Ok, I guess I was wrong.
> > 
> > Alright, 赵小强, feel free to repost addressing just the other
> > comments and I'll merge.
> > 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]