Re: [PATCH kernel] RFC: prom_init: Fetch flatten device tree from the system firmware

On 16/10/17 17:46, David Gibson wrote:

On Mon, Oct 16, 2017 at 05:22:55PM +1100, Alexey Kardashevskiy wrote:

quoted

On 16/10/17 17:11, David Gibson wrote:

quoted

On Mon, Oct 16, 2017 at 04:49:17PM +1100, Alexey Kardashevskiy wrote:

quoted

At the moment, on 256CPU + 256 PCI devices guest, it takes the guest
about 8.5sec to read the entire device tree. Some explanation can be
found here: https://patchwork.ozlabs.org/patch/826124/ but mostly it is
because the kernel traverses the tree twice and it calls "getprop" for
each properly which is really SLOF as it searches from the linked list
beginning every time.

Since SLOF has just learned to build FDT and this takes less than 0.5sec
for such a big guest, this makes use of the proposed client interface
method - "fdt-fetch".

If "fdt-fetch" is not available, the old method is used.

Signed-off-by: Alexey Kardashevskiy <redacted>

I like the concept, few details though..

quoted

---
 arch/powerpc/kernel/prom_init.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 02190e90c7ae..daa50a153737 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -2498,6 +2498,31 @@ static void __init flatten_device_tree(void)
 		prom_panic("Can't allocate initial device-tree chunk\n");
 	mem_end = mem_start + room;
 
+	if (!call_prom_ret("fdt-fetch", 2, 1, NULL, mem_start,
+			   room - sizeof(mem_reserve_map))) {
+		u32 size;
+
+		hdr = (void *) mem_start;
+
+		/* Fixup the boot cpuid */
+		hdr->boot_cpuid_phys = cpu_to_be32(prom.cpu);

If SLOF is generating a tree it really should get this header field
right as well.

Ah, I did not realize it is just a phandle from /chosen/cpu. Will
fix.

It's not a phandle.  It's just the "address" (i.e. reg value) of the
boot cpu.

Well, it is "reg" of a CPU with phandle==/chosen/cpu so my fdt code needs
to look there to pick the right "reg" rather than just plain 0. I'll fix
this but in general can it possibly be not a zero in QEMU/SLOF?

quoted

quoted

quoted

+		/* Append the reserved map to the end of the blob */
+		hdr->off_mem_rsvmap = hdr->totalsize;
+		size = be32_to_cpu(hdr->totalsize);
+		rsvmap = (void *) hdr + size;
+		hdr->totalsize = cpu_to_be32(size + sizeof(mem_reserve_map));
+		memcpy(rsvmap, mem_reserve_map, sizeof(mem_reserve_map));

.. and the reserve map for that matter.  I don't really understand
what you're doing here. 

? Get the blob, increase the FDT size by sizeof(mem_reserve_map), fix up
totalsize and off_mem_rsvmap, copy mem_reserve_map to the end of the blob
(the actual order is slightly different, may be a bit confusing).

Right.. but where is mem_reserve_map coming from, if it hasn't come
from an FDT?

quoted

Asking SLOF to reserve the space seems to be unnecessary complication of
the interface - SLOF does not provide any reserved memory records.

Ah.. right, the reservations are coming from the pre-prom kernel, not
from the firmware itself.  Yeah, that makes sense.  Ok, this makes
sense then...

Right, the reservations are added via reserve_mem() in
arch/powerpc/kernel/prom_init.c

quoted

quoted

Note also that the reserve map is required to
be 8-byte aligned, which totalsize might not be.

Ah, good point.

..at least with that fixed and maybe some comments to make what's
gonig on clearer.

quoted

quoted

quoted

+		/* Store the DT address */
+		dt_header_start = mem_start;
+
+#ifdef DEBUG_PROM
+		prom_printf("Fetched DTB: %d bytes to @%x\n", size, mem_start);
+#endif
+		goto print_exit;
+	}
+
 	/* Get root of tree */
 	root = call_prom("peer", 1, 1, (phandle)0);
 	if (root == (phandle)0)

@@ -2548,6 +2573,7 @@ static void __init flatten_device_tree(void)
 	/* Copy the reserve map in */
 	memcpy(rsvmap, mem_reserve_map, sizeof(mem_reserve_map));
 
+print_exit:
 #ifdef DEBUG_PROM
 	{
 		int i;

-- 
Alexey