| Message ID | 1461119601-4936-8-git-send-email-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? In general, when do you plan to get rid of _FDT()? 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; > } > >
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; > > } > > > > > >
On 04/21/2016 03:52 PM, David Gibson wrote: > 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. Ok. >> 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. I meant timeframe :) Like "2.7 release" or so.
On Thu, Apr 21, 2016 at 04:03:12PM +1000, Alexey Kardashevskiy wrote: > On 04/21/2016 03:52 PM, David Gibson wrote: > >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. > > > Ok. > > >>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. > > I meant timeframe :) Like "2.7 release" or so. Well.. it'd be nice to do this before the 2.7 release, but it really depends how much time I have to do this cleanup stuff.
On 20.04.2016 04:33, 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. > > 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; > } Reviewed-by: Thomas Huth <thuth@redhat.com>
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; }
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. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)