[Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Eric Auger]

Dear all,

The goal of this mail is to summarize how dynamic sysbus device tree
nodes were created on ARM with "machvirt dynamic sysbus device
instantiation",
https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01626.html
and request some advises after commit "hw/arm/boot: load DTB as a ROM
image", which puts into question the current implementation.

When dynamically instantiating sysbus devices from qemu command line,
the complete device tree cannot be built at machine init. At time we
miss key information about those devices (base address, IRQ binding, ...)

dynamic sysbus devices are "realized" after the machine init when
parsing "-device" option line. This is at that time the information
about the device are collected.

The QEMU binding of the devices is performed in the platform_bus
machine_init_done_notifier. Only at that time the base address of the
device and IRQ number are chosen.

The original idea was to create the dynamic sysbus device tree nodes in
a reset callback (registered through qemu_register_reset). device tree
was fully re-created at that time and new sysbus device nodes were added
too. Finally archi specific load_dtb was called.

On ppc/e500 this works since load_dtb uses cpu_physical_memory_write.
it was the case on ARM too until recently but commit "hw/arm/boot: load
DTB as a ROM image" changed the arm load_dtb implementation. It now uses
rom_add_blob_fixed. when the reset callback is called rom_load_done()
was called by vl.c and prevents from changing the rom content. Hence
current callback mechanism does not work anymore.

A solution I foresee to fix the issue:
construct the device tree nodes in one machine_init_done_notifier,
before the rom_load_done is called. I would propose the platform bus
device (hw/core/platform-bus.c in [PATCH v3 0/7] Dynamic sysbus device
allocation support,
http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04833.html)
to register another machine_init_done_notifier whose role would be to
initiate the dt upgrade. I would add a function to the platform bus to
pass an opaque data that allows calling architecture specific dt
implementation in the notifier, if needed (on ARM only).

I understand reverting to previous cpu_physical_memory_write
implementation on ARM is not the good direction.

Do you have any comments about the proposed solution, any other suggestion?

Thanks in advance

Best Regards

Eric

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Alexander Graf]

On 23.10.14 12:10, Eric Auger wrote:
> Dear all,
> 
> The goal of this mail is to summarize how dynamic sysbus device tree
> nodes were created on ARM with "machvirt dynamic sysbus device
> instantiation",
> https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01626.html
> and request some advises after commit "hw/arm/boot: load DTB as a ROM
> image", which puts into question the current implementation.
> 
> When dynamically instantiating sysbus devices from qemu command line,
> the complete device tree cannot be built at machine init. At time we
> miss key information about those devices (base address, IRQ binding, ...)
> 
> dynamic sysbus devices are "realized" after the machine init when
> parsing "-device" option line. This is at that time the information
> about the device are collected.
> 
> The QEMU binding of the devices is performed in the platform_bus
> machine_init_done_notifier. Only at that time the base address of the
> device and IRQ number are chosen.
> 
> The original idea was to create the dynamic sysbus device tree nodes in
> a reset callback (registered through qemu_register_reset). device tree
> was fully re-created at that time and new sysbus device nodes were added
> too. Finally archi specific load_dtb was called.
> 
> On ppc/e500 this works since load_dtb uses cpu_physical_memory_write.
> it was the case on ARM too until recently but commit "hw/arm/boot: load
> DTB as a ROM image" changed the arm load_dtb implementation. It now uses
> rom_add_blob_fixed. when the reset callback is called rom_load_done()
> was called by vl.c and prevents from changing the rom content. Hence
> current callback mechanism does not work anymore.
> 
> A solution I foresee to fix the issue:
> construct the device tree nodes in one machine_init_done_notifier,
> before the rom_load_done is called. I would propose the platform bus
> device (hw/core/platform-bus.c in [PATCH v3 0/7] Dynamic sysbus device
> allocation support,
> http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04833.html)
> to register another machine_init_done_notifier whose role would be to
> initiate the dt upgrade.

The platform bus doesn't have any business in creating device trees.
Please do this in your machine file.


Alex

> I would add a function to the platform bus to
> pass an opaque data that allows calling architecture specific dt
> implementation in the notifier, if needed (on ARM only).
> 
> I understand reverting to previous cpu_physical_memory_write
> implementation on ARM is not the good direction.
> 
> Do you have any comments about the proposed solution, any other suggestion?
> 
> Thanks in advance
> 
> Best Regards
> 
> Eric
> 
> 
> 
> 
> 

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Ard Biesheuvel]

> Dear all,
>
> The goal of this mail is to summarize how dynamic sysbus device tree
> nodes were created on ARM with "machvirt dynamic sysbus device
> instantiation",
> https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01626.html
> and request some advises after commit "hw/arm/boot: load DTB as a ROM
> image", which puts into question the current implementation.
>

The reason for this change was that, before, the DTB would only be
generated once, and after a reset, the machine would go through the
kernel boot protocol as before but the DTB pointer would point to
garbage. Any idea how ppc deals with this? Do they recreate the device
tree after each reset?

-- 
Ard.

> When dynamically instantiating sysbus devices from qemu command line,
> the complete device tree cannot be built at machine init. At time we
> miss key information about those devices (base address, IRQ binding, ...)
>
> dynamic sysbus devices are "realized" after the machine init when
> parsing "-device" option line. This is at that time the information
> about the device are collected.
>
> The QEMU binding of the devices is performed in the platform_bus
> machine_init_done_notifier. Only at that time the base address of the
> device and IRQ number are chosen.
>
> The original idea was to create the dynamic sysbus device tree nodes in
> a reset callback (registered through qemu_register_reset). device tree
> was fully re-created at that time and new sysbus device nodes were added
> too. Finally archi specific load_dtb was called.
>
> On ppc/e500 this works since load_dtb uses cpu_physical_memory_write.
> it was the case on ARM too until recently but commit "hw/arm/boot: load
> DTB as a ROM image" changed the arm load_dtb implementation. It now uses
> rom_add_blob_fixed. when the reset callback is called rom_load_done()
> was called by vl.c and prevents from changing the rom content. Hence
> current callback mechanism does not work anymore.
>
> A solution I foresee to fix the issue:
> construct the device tree nodes in one machine_init_done_notifier,
> before the rom_load_done is called. I would propose the platform bus
> device (hw/core/platform-bus.c in [PATCH v3 0/7] Dynamic sysbus device
> allocation support,
> http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04833.html)
> to register another machine_init_done_notifier whose role would be to
> initiate the dt upgrade. I would add a function to the platform bus to
> pass an opaque data that allows calling architecture specific dt
> implementation in the notifier, if needed (on ARM only).
>
> I understand reverting to previous cpu_physical_memory_write
> implementation on ARM is not the good direction.
>
> Do you have any comments about the proposed solution, any other suggestion?
>
> Thanks in advance
>
> Best Regards
>
> Eric
>
>
>
>
>

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Alexander Graf]

On 23.10.14 12:19, Ard Biesheuvel wrote:
>> Dear all,
>>
>> The goal of this mail is to summarize how dynamic sysbus device tree
>> nodes were created on ARM with "machvirt dynamic sysbus device
>> instantiation",
>> https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01626.html
>> and request some advises after commit "hw/arm/boot: load DTB as a ROM
>> image", which puts into question the current implementation.
>>
> 
> The reason for this change was that, before, the DTB would only be
> generated once, and after a reset, the machine would go through the
> kernel boot protocol as before but the DTB pointer would point to
> garbage. Any idea how ppc deals with this? Do they recreate the device
> tree after each reset?

Yes, we regenerate the device tree on each reset.


Alex

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Peter Maydell]

On 23 October 2014 12:23, Alexander Graf <agraf@suse.de> wrote:
> On 23.10.14 12:19, Ard Biesheuvel wrote:
>> The reason for this change was that, before, the DTB would only be
>> generated once, and after a reset, the machine would go through the
>> kernel boot protocol as before but the DTB pointer would point to
>> garbage. Any idea how ppc deals with this? Do they recreate the device
>> tree after each reset?
>
> Yes, we regenerate the device tree on each reset.

Any particular reason? Surely it's always the same...

-- PMM

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Alexander Graf]

On 23.10.14 13:24, Peter Maydell wrote:
> On 23 October 2014 12:23, Alexander Graf <agraf@suse.de> wrote:
>> On 23.10.14 12:19, Ard Biesheuvel wrote:
>>> The reason for this change was that, before, the DTB would only be
>>> generated once, and after a reset, the machine would go through the
>>> kernel boot protocol as before but the DTB pointer would point to
>>> garbage. Any idea how ppc deals with this? Do they recreate the device
>>> tree after each reset?
>>
>> Yes, we regenerate the device tree on each reset.
> 
> Any particular reason? Surely it's always the same...

We have the code in place anyway, it's not a performance critical code
path and putting it into a rom would be a waste of RAM, as it'd keep yet
another copy of something we can easily regenerate.

It's a matter of personal preference I guess.


Alex

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Eric Auger]

Hi,

Thanks everyone for entering the thread & reading my long email.

Alex, I indeed can register the notifier in the machine file after the
platform bus instantiation. This indeed guarantees the notifiers are
called in the right order ...

Thanks

Best Regards

Eric

On 10/23/2014 01:26 PM, Alexander Graf wrote:
> 
> 
> On 23.10.14 13:24, Peter Maydell wrote:
>> On 23 October 2014 12:23, Alexander Graf <agraf@suse.de> wrote:
>>> On 23.10.14 12:19, Ard Biesheuvel wrote:
>>>> The reason for this change was that, before, the DTB would only be
>>>> generated once, and after a reset, the machine would go through the
>>>> kernel boot protocol as before but the DTB pointer would point to
>>>> garbage. Any idea how ppc deals with this? Do they recreate the device
>>>> tree after each reset?
>>>
>>> Yes, we regenerate the device tree on each reset.
>>
>> Any particular reason? Surely it's always the same...
> 
> We have the code in place anyway, it's not a performance critical code
> path and putting it into a rom would be a waste of RAM, as it'd keep yet
> another copy of something we can easily regenerate.
> 
> It's a matter of personal preference I guess.
> 
> 
> Alex
> 

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Eric Auger]

On 10/23/2014 01:41 PM, Eric Auger wrote:
> Hi,
> 
> Thanks everyone for entering the thread & reading my long email.
> 
> Alex, I indeed can register the notifier in the machine file after 
s/after/before
Eric
the
> platform bus instantiation. This indeed guarantees the notifiers are
> called in the right order ...

> 
> Thanks
> 
> Best Regards
> 
> Eric
> 
> On 10/23/2014 01:26 PM, Alexander Graf wrote:
>>
>>
>> On 23.10.14 13:24, Peter Maydell wrote:
>>> On 23 October 2014 12:23, Alexander Graf <agraf@suse.de> wrote:
>>>> On 23.10.14 12:19, Ard Biesheuvel wrote:
>>>>> The reason for this change was that, before, the DTB would only be
>>>>> generated once, and after a reset, the machine would go through the
>>>>> kernel boot protocol as before but the DTB pointer would point to
>>>>> garbage. Any idea how ppc deals with this? Do they recreate the device
>>>>> tree after each reset?
>>>>
>>>> Yes, we regenerate the device tree on each reset.
>>>
>>> Any particular reason? Surely it's always the same...
>>
>> We have the code in place anyway, it's not a performance critical code
>> path and putting it into a rom would be a waste of RAM, as it'd keep yet
>> another copy of something we can easily regenerate.
>>
>> It's a matter of personal preference I guess.
>>
>>
>> Alex
>>
> 

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[David Gibson]

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

On Thu, Oct 23, 2014 at 01:26:08PM +0200, Alexander Graf wrote:
> 
> 
> On 23.10.14 13:24, Peter Maydell wrote:
> > On 23 October 2014 12:23, Alexander Graf <agraf@suse.de> wrote:
> >> On 23.10.14 12:19, Ard Biesheuvel wrote:
> >>> The reason for this change was that, before, the DTB would only be
> >>> generated once, and after a reset, the machine would go through the
> >>> kernel boot protocol as before but the DTB pointer would point to
> >>> garbage. Any idea how ppc deals with this? Do they recreate the device
> >>> tree after each reset?
> >>
> >> Yes, we regenerate the device tree on each reset.
> > 
> > Any particular reason? Surely it's always the same...
> 
> We have the code in place anyway, it's not a performance critical code
> path and putting it into a rom would be a waste of RAM, as it'd keep yet
> another copy of something we can easily regenerate.
> 
> It's a matter of personal preference I guess.

The "pseries" machine actually uses an odd hybrid.  We create a
"template" device tree with the common portions during early init.
That's stored permanently, but is not guest visible.

At reset time we augment the template with information which could
very from one boot to another, then copy it into RAM for the SLOF
firmware to read.

It's not particularly efficient, but really, who cares.  It's once per
resent, and we're generally talking at most a few dozen kiB of device
tree.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[David Gibson]

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

On Fri, Oct 24, 2014 at 02:38:39PM +0200, David Gibson wrote:
> On Thu, Oct 23, 2014 at 01:26:08PM +0200, Alexander Graf wrote:
> > 
> > 
> > On 23.10.14 13:24, Peter Maydell wrote:
> > > On 23 October 2014 12:23, Alexander Graf <agraf@suse.de> wrote:
> > >> On 23.10.14 12:19, Ard Biesheuvel wrote:
> > >>> The reason for this change was that, before, the DTB would only be
> > >>> generated once, and after a reset, the machine would go through the
> > >>> kernel boot protocol as before but the DTB pointer would point to
> > >>> garbage. Any idea how ppc deals with this? Do they recreate the device
> > >>> tree after each reset?
> > >>
> > >> Yes, we regenerate the device tree on each reset.
> > > 
> > > Any particular reason? Surely it's always the same...
> > 
> > We have the code in place anyway, it's not a performance critical code
> > path and putting it into a rom would be a waste of RAM, as it'd keep yet
> > another copy of something we can easily regenerate.
> > 
> > It's a matter of personal preference I guess.
> 
> The "pseries" machine actually uses an odd hybrid.  We create a
> "template" device tree with the common portions during early init.
> That's stored permanently, but is not guest visible.
> 
> At reset time we augment the template with information which could
> very from one boot to another, then copy it into RAM for the SLOF
> firmware to read.
> 
> It's not particularly efficient, but really, who cares.  It's once per
> resent, and we're generally talking at most a few dozen kiB of device
> tree.

Oh, also, because we potentially have hotplug of VIO devices, it's
*not* necessarily the same every time.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] dynamic sysbus instantiation and load_dtb implementation

[Eric Auger]

On 11/13/2014 05:02 AM, David Gibson wrote:
> On Fri, Oct 24, 2014 at 02:38:39PM +0200, David Gibson wrote:
>> On Thu, Oct 23, 2014 at 01:26:08PM +0200, Alexander Graf wrote:
>>>
>>>
>>> On 23.10.14 13:24, Peter Maydell wrote:
>>>> On 23 October 2014 12:23, Alexander Graf <agraf@suse.de> wrote:
>>>>> On 23.10.14 12:19, Ard Biesheuvel wrote:
>>>>>> The reason for this change was that, before, the DTB would only be
>>>>>> generated once, and after a reset, the machine would go through the
>>>>>> kernel boot protocol as before but the DTB pointer would point to
>>>>>> garbage. Any idea how ppc deals with this? Do they recreate the device
>>>>>> tree after each reset?
>>>>>
>>>>> Yes, we regenerate the device tree on each reset.
>>>>
>>>> Any particular reason? Surely it's always the same...
>>>
>>> We have the code in place anyway, it's not a performance critical code
>>> path and putting it into a rom would be a waste of RAM, as it'd keep yet
>>> another copy of something we can easily regenerate.
>>>
>>> It's a matter of personal preference I guess.
>>
>> The "pseries" machine actually uses an odd hybrid.  We create a
>> "template" device tree with the common portions during early init.
>> That's stored permanently, but is not guest visible.
>>
>> At reset time we augment the template with information which could
>> very from one boot to another, then copy it into RAM for the SLOF
>> firmware to read.
>>
>> It's not particularly efficient, but really, who cares.  It's once per
>> resent, and we're generally talking at most a few dozen kiB of device
>> tree.
> 
> Oh, also, because we potentially have hotplug of VIO devices, it's
> *not* necessarily the same every time.
> 
Thanks for your inputs. I eventually proposed an implementation based on
machine init done notifier, still using rom_add_blob_fixed.

http://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg03812.html

By the way, besides Alex, did anyone have time to review modifs in
hw/arm/boot.c and overall process to enhance the original device tree?
Are those modifications acceptable?

Thank you in advance

Best Regards

Eric