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

[David Gibson]

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  写道：
> > 
> >> On Wed, Jan 04, 2017 at 05:04:02PM +, Peter Maydell wrote:
> >>> On 4 January 2017 at 03:28, David Gibson  
> >>> 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
Description: PGP signature

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  写道：
> 
>> On Wed, Jan 04, 2017 at 05:04:02PM +, Peter Maydell wrote:
>>> On 4 January 2017 at 03:28, David Gibson  
>>> 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]

On Wed, Jan 04, 2017 at 05:04:02PM +, Peter Maydell wrote:
> On 4 January 2017 at 03:28, David Gibson  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
Description: PGP signature

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

[Peter Maydell]

On 4 January 2017 at 03:28, David Gibson  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

[赵小强]

Hmm，sounds reasonable. I will take a look.

PS: guys, some other comments about this?

Best wishes ！

>> 在 2017年1月4日，11:28，David Gibson  写道：
>> 
>> 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  写道：
 
 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

[David Gibson]

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  写道：
> > 
> >> 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
Description: PGP signature

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  写道：
> 
>> 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=1tbiqBVTxlc67OfNrwABst=2=95585277ead1794d396d48ba29e73240=1483402980=zxq_yx_007%40163.com
> 
> preview: 
> http://preview.mail.163.com/preview?mid=1tbiqBVTxlc67OfNrwABst=2=95585277ead1794d396d48ba29e73240=1483402980=zxq_yx_007%40163.com
> 

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

[David Gibson]

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
Description: PGP signature

[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