| Message ID | 20240814110009.45310-15-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make LMB memory map global and persistent | expand |
Hi Sughosh, On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > Introduce a function lmb_add_memory() to add available memory to the > LMB memory map. Call this function during board init once the LMB data > structures have been initialised. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > Changes since V1: > * Call the lmb_add_memory() from lmb_init() instead of > lmb_mem_regions_init(). > > > include/lmb.h | 10 ++++++++++ > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > Reviewed-by: Simon Glass <sjg@chromium.org> But this should not be weak. > diff --git a/include/lmb.h b/include/lmb.h > index f6b2a81546..a82ea63d6c 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -63,6 +63,16 @@ struct lmb { > */ > int lmb_init(void); > > +/** > + * lmb_add_memory() - Add memory range for LMB allocations > + * > + * Add the entire available memory range to the pool of memory that > + * can be used by the LMB module for allocations. > + * > + * Return: None > + */ > +void lmb_add_memory(void); > + > long lmb_add(phys_addr_t base, phys_size_t size); > long lmb_reserve(phys_addr_t base, phys_size_t size); > /** > diff --git a/lib/lmb.c b/lib/lmb.c > index 3ccee26a46..f35a94c41b 100644 > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -225,6 +225,46 @@ static void lmb_reserve_common(void *fdt_blob) > efi_lmb_reserve(); > } > > +/** > + * lmb_add_memory() - Add memory range for LMB allocations > + * > + * Add the entire available memory range to the pool of memory that > + * can be used by the LMB module for allocations. > + * > + * This can be overridden for specific boards/architectures. > + * > + * Return: None > + * > + */ > +__weak void lmb_add_memory(void) > +{ > + int i; > + phys_size_t size; > + phys_addr_t rgn_top; > + u64 ram_top = gd->ram_top; > + struct bd_info *bd = gd->bd; > + > + /* Assume a 4GB ram_top if not defined */ > + if (!ram_top) > + ram_top = 0x100000000ULL; > + > + for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { > + size = bd->bi_dram[i].size; > + if (size) { > + if (bd->bi_dram[i].start > ram_top) > + continue; > + > + rgn_top = bd->bi_dram[i].start + > + bd->bi_dram[i].size; > + > + if (rgn_top > ram_top) > + size -= rgn_top - ram_top; > + > + lmb_add(bd->bi_dram[i].start, size); > + } > + } > +} > + > static long lmb_resize_regions(struct alist *lmb_rgn_lst, > unsigned long idx_start, > phys_addr_t base, phys_size_t size) > @@ -674,5 +714,7 @@ int lmb_init(void) > return ret; > } > > + lmb_add_memory(); > + > return 0; > } > -- > 2.34.1 > Regards, Simon
On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > Introduce a function lmb_add_memory() to add available memory to the > > LMB memory map. Call this function during board init once the LMB data > > structures have been initialised. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > Changes since V1: > > * Call the lmb_add_memory() from lmb_init() instead of > > lmb_mem_regions_init(). > > > > > > include/lmb.h | 10 ++++++++++ > > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 52 insertions(+) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > But this should not be weak. This is being made weak, as there would be lmb_add_memory() definitions added for powerpc and x86 arch's in the EFI part of my patches. Moreover, the lmb_add_memory() function would be called even in the SPL stage when LMB is enabled for that stage. So I am not sure how do we get around this. You can check the relevant branch [1] on my github to check for the specific commits [2][3] that I am referring to. Thanks. -sughosh [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3 [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1 [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140 > > > diff --git a/include/lmb.h b/include/lmb.h > > index f6b2a81546..a82ea63d6c 100644 > > --- a/include/lmb.h > > +++ b/include/lmb.h > > @@ -63,6 +63,16 @@ struct lmb { > > */ > > int lmb_init(void); > > > > +/** > > + * lmb_add_memory() - Add memory range for LMB allocations > > + * > > + * Add the entire available memory range to the pool of memory that > > + * can be used by the LMB module for allocations. > > + * > > + * Return: None > > + */ > > +void lmb_add_memory(void); > > + > > long lmb_add(phys_addr_t base, phys_size_t size); > > long lmb_reserve(phys_addr_t base, phys_size_t size); > > /** > > diff --git a/lib/lmb.c b/lib/lmb.c > > index 3ccee26a46..f35a94c41b 100644 > > --- a/lib/lmb.c > > +++ b/lib/lmb.c > > @@ -225,6 +225,46 @@ static void lmb_reserve_common(void *fdt_blob) > > efi_lmb_reserve(); > > } > > > > +/** > > + * lmb_add_memory() - Add memory range for LMB allocations > > + * > > + * Add the entire available memory range to the pool of memory that > > + * can be used by the LMB module for allocations. > > + * > > + * This can be overridden for specific boards/architectures. > > + * > > + * Return: None > > + * > > + */ > > +__weak void lmb_add_memory(void) > > +{ > > + int i; > > + phys_size_t size; > > + phys_addr_t rgn_top; > > + u64 ram_top = gd->ram_top; > > + struct bd_info *bd = gd->bd; > > + > > + /* Assume a 4GB ram_top if not defined */ > > + if (!ram_top) > > + ram_top = 0x100000000ULL; > > + > > + for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { > > + size = bd->bi_dram[i].size; > > + if (size) { > > + if (bd->bi_dram[i].start > ram_top) > > + continue; > > + > > + rgn_top = bd->bi_dram[i].start + > > + bd->bi_dram[i].size; > > + > > + if (rgn_top > ram_top) > > + size -= rgn_top - ram_top; > > + > > + lmb_add(bd->bi_dram[i].start, size); > > + } > > + } > > +} > > + > > static long lmb_resize_regions(struct alist *lmb_rgn_lst, > > unsigned long idx_start, > > phys_addr_t base, phys_size_t size) > > @@ -674,5 +714,7 @@ int lmb_init(void) > > return ret; > > } > > > > + lmb_add_memory(); > > + > > return 0; > > } > > -- > > 2.34.1 > > > > Regards, > Simon
Hi Sughosh, On Tue, 20 Aug 2024 at 02:17, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > Introduce a function lmb_add_memory() to add available memory to the > > > LMB memory map. Call this function during board init once the LMB data > > > structures have been initialised. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > Changes since V1: > > > * Call the lmb_add_memory() from lmb_init() instead of > > > lmb_mem_regions_init(). > > > > > > > > > include/lmb.h | 10 ++++++++++ > > > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 52 insertions(+) > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > But this should not be weak. > > This is being made weak, as there would be lmb_add_memory() > definitions added for powerpc and x86 arch's in the EFI part of my > patches. Moreover, the lmb_add_memory() function would be called even > in the SPL stage when LMB is enabled for that stage. So I am not sure > how do we get around this. You can check the relevant branch [1] on my > github to check for the specific commits [2][3] that I am referring > to. Thanks. This is really strange. The e820 is different on each x86 board. I'm not sure we want that in the lmb. What is the purpose of that? It is somewhat circular in most cases, since U-Boot sets it up itself. Where it comes from coreboot, it looks like we are mirroring it in the EFI memory map. I'm not sure I understand all this very well. For fsl, perhaps copy the #ifdef and handle arch.resv_ram in your code? [..] Regards, Simon > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3 > [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1 > [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140 >
On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Tue, 20 Aug 2024 at 02:17, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > Introduce a function lmb_add_memory() to add available memory to the > > > > LMB memory map. Call this function during board init once the LMB data > > > > structures have been initialised. > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > Changes since V1: > > > > * Call the lmb_add_memory() from lmb_init() instead of > > > > lmb_mem_regions_init(). > > > > > > > > > > > > include/lmb.h | 10 ++++++++++ > > > > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 52 insertions(+) > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > But this should not be weak. > > > > This is being made weak, as there would be lmb_add_memory() > > definitions added for powerpc and x86 arch's in the EFI part of my > > patches. Moreover, the lmb_add_memory() function would be called even > > in the SPL stage when LMB is enabled for that stage. So I am not sure > > how do we get around this. You can check the relevant branch [1] on my > > github to check for the specific commits [2][3] that I am referring > > to. Thanks. > > This is really strange. > > The e820 is different on each x86 board. I'm not sure we want that in > the lmb. What is the purpose of that? It is somewhat circular in most > cases, since U-Boot sets it up itself. Where it comes from coreboot, > it looks like we are mirroring it in the EFI memory map. I'm not sure > I understand all this very well. Yes, me neither. And I want to keep the behaviour same as before the patches. You would know that the function efi_add_known_memory() gets the memory map from a function install_e820_map() which includes conventional memory, which is the ram memory. And I am basically now doing this as part of the lmb_add_memory() function instead. Are you sure that we can do away with this function, and instead use the ram_base and ram_top values from the global data structure instead? I believe you have boards which exercise this code? So it will be great if you can test this if I remove the function for the e820 module. > > For fsl, perhaps copy the #ifdef and handle arch.resv_ram in your code? This is for adding ram to the lmb memory map, but yes, I can check by putting an ifdef in the function. Although the function might look ugly and hackish. Thanks. -sughosh > > [..] > Regards, > Simon > > > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3 > > [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1 > > [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140 > >
On Wed, 21 Aug 2024 at 10:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Tue, 20 Aug 2024 at 02:17, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > Introduce a function lmb_add_memory() to add available memory to the > > > > > LMB memory map. Call this function during board init once the LMB data > > > > > structures have been initialised. > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > Changes since V1: > > > > > * Call the lmb_add_memory() from lmb_init() instead of > > > > > lmb_mem_regions_init(). > > > > > > > > > > > > > > > include/lmb.h | 10 ++++++++++ > > > > > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 52 insertions(+) > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > But this should not be weak. > > > > > > This is being made weak, as there would be lmb_add_memory() > > > definitions added for powerpc and x86 arch's in the EFI part of my > > > patches. Moreover, the lmb_add_memory() function would be called even > > > in the SPL stage when LMB is enabled for that stage. So I am not sure > > > how do we get around this. You can check the relevant branch [1] on my > > > github to check for the specific commits [2][3] that I am referring > > > to. Thanks. > > > > This is really strange. > > > > The e820 is different on each x86 board. I'm not sure we want that in > > the lmb. What is the purpose of that? It is somewhat circular in most > > cases, since U-Boot sets it up itself. Where it comes from coreboot, > > it looks like we are mirroring it in the EFI memory map. I'm not sure > > I understand all this very well. > > Yes, me neither. And I want to keep the behaviour same as before the > patches. You would know that the function efi_add_known_memory() gets > the memory map from a function install_e820_map() which includes > conventional memory, which is the ram memory. And I am basically now > doing this as part of the lmb_add_memory() function instead. Are you > sure that we can do away with this function, and instead use the > ram_base and ram_top values from the global data structure instead? I > believe you have boards which exercise this code? So it will be great > if you can test this if I remove the function for the e820 module. > > > > > For fsl, perhaps copy the #ifdef and handle arch.resv_ram in your code? > > This is for adding ram to the lmb memory map, but yes, I can check by > putting an ifdef in the function. Although the function might look > ugly and hackish. Thanks. I have taken an alternative approach to this, whereby boards/archs can still define their own memory map addition without using the weak attribute. Please check the relevant branch [1] on my github, and these commits [2][3][4] specifically. Thanks. -sughosh [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_noweak_v3 [2] - https://github.com/u-boot/u-boot/commit/0b5dbaf07a3615230b9df06296e40267e838f459 [3] - https://github.com/u-boot/u-boot/commit/916a36dcb5cc66d801067924607fd94d8bad2162 [4] - https://github.com/u-boot/u-boot/commit/9da75a20e9a6af00cfaac71f09e4a1f89f10a804 > > -sughosh > > > > > [..] > > Regards, > > Simon > > > > > > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3 > > > [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1 > > > [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140 > > >
+Bin Meng Hi Sughosh, On Tue, 20 Aug 2024 at 23:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Tue, 20 Aug 2024 at 02:17, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > Introduce a function lmb_add_memory() to add available memory to the > > > > > LMB memory map. Call this function during board init once the LMB data > > > > > structures have been initialised. > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > Changes since V1: > > > > > * Call the lmb_add_memory() from lmb_init() instead of > > > > > lmb_mem_regions_init(). > > > > > > > > > > > > > > > include/lmb.h | 10 ++++++++++ > > > > > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 52 insertions(+) > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > But this should not be weak. > > > > > > This is being made weak, as there would be lmb_add_memory() > > > definitions added for powerpc and x86 arch's in the EFI part of my > > > patches. Moreover, the lmb_add_memory() function would be called even > > > in the SPL stage when LMB is enabled for that stage. So I am not sure > > > how do we get around this. You can check the relevant branch [1] on my > > > github to check for the specific commits [2][3] that I am referring > > > to. Thanks. > > > > This is really strange. > > > > The e820 is different on each x86 board. I'm not sure we want that in > > the lmb. What is the purpose of that? It is somewhat circular in most > > cases, since U-Boot sets it up itself. Where it comes from coreboot, > > it looks like we are mirroring it in the EFI memory map. I'm not sure > > I understand all this very well. > > Yes, me neither. And I want to keep the behaviour same as before the > patches. You would know that the function efi_add_known_memory() gets > the memory map from a function install_e820_map() which includes > conventional memory, which is the ram memory. And I am basically now > doing this as part of the lmb_add_memory() function instead. Are you > sure that we can do away with this function, and instead use the > ram_base and ram_top values from the global data structure instead? I > believe you have boards which exercise this code? So it will be great > if you can test this if I remove the function for the e820 module. We are suffering here from too many ways to do the same thing and too much confusion about the overall goal here. Typically the e820 thing is created in U-Boot - see install_e820_map(). The ISA and PCI ranges needs to be added, so EFI needs a way to know to do that. e820 is used for the normal kernel boot flow - the table is passed directly to Linux. With EFI the kernel calls back into U-Boot to get the memory map, so we want to add the same reservations to the EFI tables. So I would favour having EFI sending an event before booting, to allow other code to add new reservations. Then we can let e820 do its thing and not affect EFI. In fact the e820 code will never be called on an EFI boot. I say we can clean this up later, so what you have here will do for now. > > > > > For fsl, perhaps copy the #ifdef and handle arch.resv_ram in your code? > > This is for adding ram to the lmb memory map, but yes, I can check by > putting an ifdef in the function. Although the function might look > ugly and hackish. Thanks. Again, we can clean this up date. It is just one arch doing strange things. > > > > [..] > > > > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3 > > > [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1 > > > [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140 > > > Regards, Simon
On Wed, 21 Aug 2024 at 19:30, Simon Glass <sjg@chromium.org> wrote: > > +Bin Meng > > Hi Sughosh, > > On Tue, 20 Aug 2024 at 23:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Tue, 20 Aug 2024 at 02:17, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > Introduce a function lmb_add_memory() to add available memory to the > > > > > > LMB memory map. Call this function during board init once the LMB data > > > > > > structures have been initialised. > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > Changes since V1: > > > > > > * Call the lmb_add_memory() from lmb_init() instead of > > > > > > lmb_mem_regions_init(). > > > > > > > > > > > > > > > > > > include/lmb.h | 10 ++++++++++ > > > > > > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > > > 2 files changed, 52 insertions(+) > > > > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > > > But this should not be weak. > > > > > > > > This is being made weak, as there would be lmb_add_memory() > > > > definitions added for powerpc and x86 arch's in the EFI part of my > > > > patches. Moreover, the lmb_add_memory() function would be called even > > > > in the SPL stage when LMB is enabled for that stage. So I am not sure > > > > how do we get around this. You can check the relevant branch [1] on my > > > > github to check for the specific commits [2][3] that I am referring > > > > to. Thanks. > > > > > > This is really strange. > > > > > > The e820 is different on each x86 board. I'm not sure we want that in > > > the lmb. What is the purpose of that? It is somewhat circular in most > > > cases, since U-Boot sets it up itself. Where it comes from coreboot, > > > it looks like we are mirroring it in the EFI memory map. I'm not sure > > > I understand all this very well. > > > > Yes, me neither. And I want to keep the behaviour same as before the > > patches. You would know that the function efi_add_known_memory() gets > > the memory map from a function install_e820_map() which includes > > conventional memory, which is the ram memory. And I am basically now > > doing this as part of the lmb_add_memory() function instead. Are you > > sure that we can do away with this function, and instead use the > > ram_base and ram_top values from the global data structure instead? I > > believe you have boards which exercise this code? So it will be great > > if you can test this if I remove the function for the e820 module. > > We are suffering here from too many ways to do the same thing and too > much confusion about the overall goal here. > > Typically the e820 thing is created in U-Boot - see > install_e820_map(). The ISA and PCI ranges needs to be added, so EFI > needs a way to know to do that. > > e820 is used for the normal kernel boot flow - the table is passed > directly to Linux. > > With EFI the kernel calls back into U-Boot to get the memory map, so > we want to add the same reservations to the EFI tables. > > So I would favour having EFI sending an event before booting, to allow > other code to add new reservations. Then we can let e820 do its thing > and not affect EFI. In fact the e820 code will never be called on an > EFI boot. > > I say we can clean this up later, so what you have here will do for now. > > > > > > > > > For fsl, perhaps copy the #ifdef and handle arch.resv_ram in your code? > > > > This is for adding ram to the lmb memory map, but yes, I can check by > > putting an ifdef in the function. Although the function might look > > ugly and hackish. Thanks. > > Again, we can clean this up date. It is just one arch doing strange things. Can you please take a look at the branch[1] which I pointed to in my earlier reply. I believe this caters to your review comment on doing away with the weak function, as well as providing the option to boards to define their own memory map. Thanks. -sughosh [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_noweak_v3 > > > > > > > [..] > > > > > > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3 > > > > [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1 > > > > [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140 > > > > > > Regards, > Simon
Hi Sughosh, On Wed, 21 Aug 2024 at 08:40, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Wed, 21 Aug 2024 at 19:30, Simon Glass <sjg@chromium.org> wrote: > > > > +Bin Meng > > > > Hi Sughosh, > > > > On Tue, 20 Aug 2024 at 23:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Tue, 20 Aug 2024 at 02:17, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > Introduce a function lmb_add_memory() to add available memory to the > > > > > > > LMB memory map. Call this function during board init once the LMB data > > > > > > > structures have been initialised. > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > > --- > > > > > > > Changes since V1: > > > > > > > * Call the lmb_add_memory() from lmb_init() instead of > > > > > > > lmb_mem_regions_init(). > > > > > > > > > > > > > > > > > > > > > include/lmb.h | 10 ++++++++++ > > > > > > > lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > > > > > > 2 files changed, 52 insertions(+) > > > > > > > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > > > > > But this should not be weak. > > > > > > > > > > This is being made weak, as there would be lmb_add_memory() > > > > > definitions added for powerpc and x86 arch's in the EFI part of my > > > > > patches. Moreover, the lmb_add_memory() function would be called even > > > > > in the SPL stage when LMB is enabled for that stage. So I am not sure > > > > > how do we get around this. You can check the relevant branch [1] on my > > > > > github to check for the specific commits [2][3] that I am referring > > > > > to. Thanks. > > > > > > > > This is really strange. > > > > > > > > The e820 is different on each x86 board. I'm not sure we want that in > > > > the lmb. What is the purpose of that? It is somewhat circular in most > > > > cases, since U-Boot sets it up itself. Where it comes from coreboot, > > > > it looks like we are mirroring it in the EFI memory map. I'm not sure > > > > I understand all this very well. > > > > > > Yes, me neither. And I want to keep the behaviour same as before the > > > patches. You would know that the function efi_add_known_memory() gets > > > the memory map from a function install_e820_map() which includes > > > conventional memory, which is the ram memory. And I am basically now > > > doing this as part of the lmb_add_memory() function instead. Are you > > > sure that we can do away with this function, and instead use the > > > ram_base and ram_top values from the global data structure instead? I > > > believe you have boards which exercise this code? So it will be great > > > if you can test this if I remove the function for the e820 module. > > > > We are suffering here from too many ways to do the same thing and too > > much confusion about the overall goal here. > > > > Typically the e820 thing is created in U-Boot - see > > install_e820_map(). The ISA and PCI ranges needs to be added, so EFI > > needs a way to know to do that. > > > > e820 is used for the normal kernel boot flow - the table is passed > > directly to Linux. > > > > With EFI the kernel calls back into U-Boot to get the memory map, so > > we want to add the same reservations to the EFI tables. > > > > So I would favour having EFI sending an event before booting, to allow > > other code to add new reservations. Then we can let e820 do its thing > > and not affect EFI. In fact the e820 code will never be called on an > > EFI boot. > > > > I say we can clean this up later, so what you have here will do for now. > > > > > > > > > > > > > For fsl, perhaps copy the #ifdef and handle arch.resv_ram in your code? > > > > > > This is for adding ram to the lmb memory map, but yes, I can check by > > > putting an ifdef in the function. Although the function might look > > > ugly and hackish. Thanks. > > > > Again, we can clean this up date. It is just one arch doing strange things. > > Can you please take a look at the branch[1] which I pointed to in my > earlier reply. I believe this caters to your review comment on doing > away with the weak function, as well as providing the option to boards > to define their own memory map. Thanks. Yes your latest patch looks fine thank you. Regards, Simon > > -sughosh > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_noweak_v3 > > > > > > > > > > > [..] > > > > > > > > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3 > > > > > [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1 > > > > > [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140 > > > > > > > > > Regards, > > Simon
diff --git a/include/lmb.h b/include/lmb.h index f6b2a81546..a82ea63d6c 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -63,6 +63,16 @@ struct lmb { */ int lmb_init(void); +/** + * lmb_add_memory() - Add memory range for LMB allocations + * + * Add the entire available memory range to the pool of memory that + * can be used by the LMB module for allocations. + * + * Return: None + */ +void lmb_add_memory(void); + long lmb_add(phys_addr_t base, phys_size_t size); long lmb_reserve(phys_addr_t base, phys_size_t size); /** diff --git a/lib/lmb.c b/lib/lmb.c index 3ccee26a46..f35a94c41b 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -225,6 +225,46 @@ static void lmb_reserve_common(void *fdt_blob) efi_lmb_reserve(); } +/** + * lmb_add_memory() - Add memory range for LMB allocations + * + * Add the entire available memory range to the pool of memory that + * can be used by the LMB module for allocations. + * + * This can be overridden for specific boards/architectures. + * + * Return: None + * + */ +__weak void lmb_add_memory(void) +{ + int i; + phys_size_t size; + phys_addr_t rgn_top; + u64 ram_top = gd->ram_top; + struct bd_info *bd = gd->bd; + + /* Assume a 4GB ram_top if not defined */ + if (!ram_top) + ram_top = 0x100000000ULL; + + for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { + size = bd->bi_dram[i].size; + if (size) { + if (bd->bi_dram[i].start > ram_top) + continue; + + rgn_top = bd->bi_dram[i].start + + bd->bi_dram[i].size; + + if (rgn_top > ram_top) + size -= rgn_top - ram_top; + + lmb_add(bd->bi_dram[i].start, size); + } + } +} + static long lmb_resize_regions(struct alist *lmb_rgn_lst, unsigned long idx_start, phys_addr_t base, phys_size_t size) @@ -674,5 +714,7 @@ int lmb_init(void) return ret; } + lmb_add_memory(); + return 0; }
Introduce a function lmb_add_memory() to add available memory to the LMB memory map. Call this function during board init once the LMB data structures have been initialised. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V1: * Call the lmb_add_memory() from lmb_init() instead of lmb_mem_regions_init(). include/lmb.h | 10 ++++++++++ lib/lmb.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+)