Re: [Qemu-devel] [RFC for-2.7 07/11] pseries: Move adding of fdt reserve map entries

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Apr 21, 2016 at 03:14:48PM +1000, Alexey Kardashevskiy wrote:
> On 04/20/2016 12:33 PM, David Gibson wrote:
> >The flattened device tree passed to pseries guests contains a list of
> >reserved memory areas.  Currently we construct this list early in
> >spapr_build_fdt() as we sequentially write out the fdt.
> >
> >This will be inconvenient for upcoming cleanups, so this patch moves
> >the reserve map changes to the end of fdt construction.  This changes
> >fdt_add_reservemap_entry() calls - which work when writing the fdt
> >sequentially to fdt_add_mem_rsv() calls used when altering the fdt in
> >random access mode.
> 
> 
> Looks to me like the real reason for this move is that new qdt_setprop_xxx
> API does not support memory reserve map yet. Will it, when?

Right, and it's not clear that it even should include reserve map
stuff.  The reserve map isn't really part of the device tree, it's
just included in the fdt blob for historical and implementation
reasons.

So I'd prefer to avoid managing a list of reserve entries in qdt -
instead I was thinking of just having a list of reserves passed
straight into qdt_flatten().

In the meantime, I'd prefer to defer that design decision.

> In general, when
> do you plan to get rid of _FDT()?

Once I've got rid of all the calls to libfdt functions that need error
catching.


> 
> Anyway,
> 
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> 
> 
> >
> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >---
> >  hw/ppc/spapr.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> >diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >index 5356f4d..aef44a2 100644
> >--- a/hw/ppc/spapr.c
> >+++ b/hw/ppc/spapr.c
> >@@ -733,14 +733,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> >      fdt = g_malloc0(FDT_MAX_SIZE);
> >      _FDT((fdt_create(fdt, FDT_MAX_SIZE)));
> >
> >-    if (spapr->kernel_size) {
> >-        _FDT((fdt_add_reservemap_entry(fdt, KERNEL_LOAD_ADDR,
> >-                                       spapr->kernel_size)));
> >-    }
> >-    if (spapr->initrd_size) {
> >-        _FDT((fdt_add_reservemap_entry(fdt, spapr->initrd_base,
> >-                                       spapr->initrd_size)));
> >-    }
> >      _FDT((fdt_finish_reservemap(fdt)));
> >
> >      /* Root node */
> >@@ -976,6 +968,15 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> >      }
> >
> >      g_free(bootlist);
> >+
> >+    /* Build memory reserve map */
> >+    if (spapr->kernel_size) {
> >+        _FDT((fdt_add_mem_rsv(fdt, KERNEL_LOAD_ADDR, spapr->kernel_size)));
> >+    }
> >+    if (spapr->initrd_size) {
> >+        _FDT((fdt_add_mem_rsv(fdt, spapr->initrd_base, spapr->initrd_size)));
> >+    }
> >+
> >      return fdt;
> >  }
> >
> >
> 
> 

-- 
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 --]