Changelog v2->v3:
   * drop already merged patches
       'arm: always start from  first_cpu when registering loader cpu reset callback'
       'ppc: e500: switch E500 based  machines to full machine definition'
   * add small not related cleanup
       'arm: boot: set boot_info starting from first_cpu'
   * add extra suggested patch
       'make sure that we aren't overwriting mc->get_hotplug_handler by accident'
   * make sure that dtb_limit initialized to 0
   * drop stale comment
   * drop not needed line movement
   * drop not needed extra new line
Changelog v1->v2:
   * drop "arm: reuse  arm_boot_address_space() in armv7m_load_kernel()"
   * move "arm: always start from first_cpu  when registering loader cpu reset callback"
     at the begigning of series and rebase
   * add "ppc: e500: switch E500 based machines to full  machine definition"
     and rebase 4/5 on top of it
   * fixup typo in virt_machine_get_hotpug_handler() name
   * add doc comment to skip_dtb_autoload field
   * 1-2/5 are queued in respective arm/ppc trees and are included for series
     completness so it would be easier to test, I expect series to go through
     arm tree
   

While working on cpu hotplug for mach-virt, I've noticed that DTB
is generated incrementally across whole machvirt_init(). While it's
fine for machines with static DTB, it won't really work in case of
cpu hotplug and followed up reset since machine will load old DTB
that doesn't account for hotplugged CPUs.
So I've set on a quest to consolidate DTB generation and make it
reentrant so that on reset guest would see update DTB.

It's preliminary series which makes possible to call load_dtb()
later outside of arm_load_kernel() and in process of it drops
several machine_done notifiers, that were introduced to make
plaform-bus-devices work. Hopefully it makes code easier to follow.
It replaces machine_done notifiers with device hotplug framework
to allow for dynamic sysbus devices mapping at the moment they
are created instead of waiting for machine_done time and trying to
juggle with notifiers order to do initialization in propper order.

Mostly 'make check' tested +
manually with "ppce500" machine type and eTSEC device
(eTSEC is still initialized with the same IRQs as before series)


CC: qemu-arm@nongnu.org
CC: peter.maydell@linaro.org
CC: eric.auger@redhat.com


Igor Mammedov (5):
  pc: simplify MachineClass::get_hotplug_handler handling
  platform-bus-device: use device plug callback instead of machine_done
    notifier
  arm/boot: split load_dtb() from arm_load_kernel()
  arm: boot: set boot_info starting from first_cpu
  make sure that we aren't overwriting mc->get_hotplug_handler by
    accident

 hw/ppc/e500.h               |  5 +++
 include/hw/arm/arm.h        | 45 +++++++++++++++------
 include/hw/arm/sysbus-fdt.h | 37 ++++-------------
 include/hw/arm/virt.h       |  1 +
 include/hw/i386/pc.h        |  8 ----
 include/hw/platform-bus.h   |  4 +-
 hw/arm/boot.c               | 74 ++++++++++------------------------
 hw/arm/sysbus-fdt.c         | 64 ++----------------------------
 hw/arm/virt.c               | 96 +++++++++++++++++++++++++++++----------------
 hw/core/platform-bus.c      | 29 +++-----------
 hw/i386/pc.c                |  7 +---
 hw/ppc/e500.c               | 38 ++++++++----------
 hw/ppc/e500plat.c           | 32 +++++++++++++++
 hw/ppc/spapr.c              |  1 +
 hw/s390x/s390-virtio-ccw.c  |  1 +
 15 files changed, 193 insertions(+), 249 deletions(-)

-- 
2.7.4

On 1 May 2018 at 13:08, Igor Mammedov <imammedo@redhat.com> wrote:
>
> Changelog v2->v3:
>    * drop already merged patches
>        'arm: always start from  first_cpu when registering loader cpu reset callback'
>        'ppc: e500: switch E500 based  machines to full machine definition'
>    * add small not related cleanup
>        'arm: boot: set boot_info starting from first_cpu'
>    * add extra suggested patch
>        'make sure that we aren't overwriting mc->get_hotplug_handler by accident'
>    * make sure that dtb_limit initialized to 0
>    * drop stale comment
>    * drop not needed line movement
>    * drop not needed extra new line
> Changelog v1->v2:
>    * drop "arm: reuse  arm_boot_address_space() in armv7m_load_kernel()"
>    * move "arm: always start from first_cpu  when registering loader cpu reset callback"
>      at the begigning of series and rebase
>    * add "ppc: e500: switch E500 based machines to full  machine definition"
>      and rebase 4/5 on top of it
>    * fixup typo in virt_machine_get_hotpug_handler() name
>    * add doc comment to skip_dtb_autoload field
>    * 1-2/5 are queued in respective arm/ppc trees and are included for series
>      completness so it would be easier to test, I expect series to go through
>      arm tree
>

Applied to target-arm.next, thanks.
-- PMM

On 3 May 2018 at 16:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 May 2018 at 13:08, Igor Mammedov <imammedo@redhat.com> wrote:
>>
>> Changelog v2->v3:
>>    * drop already merged patches
>>        'arm: always start from  first_cpu when registering loader cpu reset callback'
>>        'ppc: e500: switch E500 based  machines to full machine definition'
>>    * add small not related cleanup
>>        'arm: boot: set boot_info starting from first_cpu'
>>    * add extra suggested patch
>>        'make sure that we aren't overwriting mc->get_hotplug_handler by accident'
>>    * make sure that dtb_limit initialized to 0
>>    * drop stale comment
>>    * drop not needed line movement
>>    * drop not needed extra new line
>> Changelog v1->v2:
>>    * drop "arm: reuse  arm_boot_address_space() in armv7m_load_kernel()"
>>    * move "arm: always start from first_cpu  when registering loader cpu reset callback"
>>      at the begigning of series and rebase
>>    * add "ppc: e500: switch E500 based machines to full  machine definition"
>>      and rebase 4/5 on top of it
>>    * fixup typo in virt_machine_get_hotpug_handler() name
>>    * add doc comment to skip_dtb_autoload field
>>    * 1-2/5 are queued in respective arm/ppc trees and are included for series
>>      completness so it would be easier to test, I expect series to go through
>>      arm tree
>>
>
> Applied to target-arm.next, thanks.

Doing further testing within target-arm.next shows that this series (and
specifically patch 3/5) causes segfaults for the "no DTB provided" case.

$ gdb --args ./build/x86/arm-softmmu/qemu-system-arm -M vexpress-a15
-kernel /dev/null
GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1
[...]
(gdb) r
[...]
Thread 1 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in  ()
#1  0x000055555596565e in arm_load_dtb (addr=0, binfo=0x5555566dec00
<a15_daughterboard>, addr_limit=0, as=0x555556fa08a0) at
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/boot.c:515
#2  0x0000555555966d7f in arm_load_kernel (cpu=0x7ffff7fd6010,
info=0x5555566dec00 <a15_daughterboard>)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/boot.c:1156
#3  0x0000555555986f5b in vexpress_common_init (machine=0x555556e04060)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/arm/vexpress.c:707
#4  0x0000555555b10341 in machine_run_board_init (machine=0x555556e04060)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/hw/core/machine.c:829
#5  0x0000555555a4cbab in main (argc=5, argv=0x7fffffffe448,
envp=0x7fffffffe478)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/vl.c:4624

arm_load_dtb() is trying to call the binfo->get_dtb() hook and is not
handling the case where it is NULL (which it is on pretty much every
board except "virt").

(You get a segfault with an actual guest kernel as well; I just used
/dev/null here as an easy no-dependencies-required repro case.)

I've left patch 4 in target-arm.next as that was a standalone bugfix,
but have dropped the rest of the series for now.

thanks
-- PMM

On Fri, 4 May 2018 17:28:55 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 3 May 2018 at 16:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 1 May 2018 at 13:08, Igor Mammedov <imammedo@redhat.com> wrote:  
> >>
> >> Changelog v2->v3:
> >>    * drop already merged patches
> >>        'arm: always start from  first_cpu when registering loader cpu reset callback'
> >>        'ppc: e500: switch E500 based  machines to full machine definition'
> >>    * add small not related cleanup
> >>        'arm: boot: set boot_info starting from first_cpu'
> >>    * add extra suggested patch
> >>        'make sure that we aren't overwriting mc->get_hotplug_handler by accident'
> >>    * make sure that dtb_limit initialized to 0
> >>    * drop stale comment
> >>    * drop not needed line movement
> >>    * drop not needed extra new line
> >> Changelog v1->v2:
> >>    * drop "arm: reuse  arm_boot_address_space() in armv7m_load_kernel()"
> >>    * move "arm: always start from first_cpu  when registering loader cpu reset callback"
> >>      at the begigning of series and rebase
> >>    * add "ppc: e500: switch E500 based machines to full  machine definition"
> >>      and rebase 4/5 on top of it
> >>    * fixup typo in virt_machine_get_hotpug_handler() name
> >>    * add doc comment to skip_dtb_autoload field
> >>    * 1-2/5 are queued in respective arm/ppc trees and are included for series
> >>      completness so it would be easier to test, I expect series to go through
> >>      arm tree
> >>  
> >
> > Applied to target-arm.next, thanks.  
> 
> Doing further testing within target-arm.next shows that this series (and
> specifically patch 3/5) causes segfaults for the "no DTB provided" case.
> 
> $ gdb --args ./build/x86/arm-softmmu/qemu-system-arm -M vexpress-a15
[...]

> arm_load_dtb() is trying to call the binfo->get_dtb() hook and is not
> handling the case where it is NULL (which it is on pretty much every
> board except "virt").
> 
> (You get a segfault with an actual guest kernel as well; I just used
> /dev/null here as an easy no-dependencies-required repro case.)
> 
> I've left patch 4 in target-arm.next as that was a standalone bugfix,
> but have dropped the rest of the series for now.

Fix is really trivial, I've lost have_dtb() guard, while consolidating
multiple load_dtb() calls into one.
I'll respin v4 shortly with fixed 3/5.


> thanks
> -- PMM