| Message ID | 20240905082811.1585467-5-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make EFI memory allocations synchronous with LMB | expand |
On 9/5/24 10:27, Sughosh Ganu wrote: > Add an event which would be used for notifying changes in the > LMB modules' memory map. This is to be used for having a > synchronous view of the memory that is currently in use, and that is > available for allocations. The synchronous view problem only exists because we are duplicating data. Store the EFI memory type in LMB and the problem vanishes. The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. Best regards Heinrich > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > common/event.c | 2 ++ > include/event.h | 14 ++++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/common/event.c b/common/event.c > index dda569d447..fc8002603c 100644 > --- a/common/event.c > +++ b/common/event.c > @@ -48,6 +48,8 @@ const char *const type_name[] = { > > /* main loop events */ > "main_loop", > + > + "lmb_map_update", > }; > > _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > diff --git a/include/event.h b/include/event.h > index fb353ad623..fce7e96170 100644 > --- a/include/event.h > +++ b/include/event.h > @@ -153,6 +153,14 @@ enum event_t { > */ > EVT_MAIN_LOOP, > > + /** > + * @EVT_LMB_MAP_UPDATE: > + * This event is triggered on an update to the LMB reserved memory > + * region. This can be used to notify about any LMB memory allocation > + * or freeing of memory having occurred. > + */ > + EVT_LMB_MAP_UPDATE, > + > /** > * @EVT_COUNT: > * This constants holds the maximum event number + 1 and is used when > @@ -203,6 +211,12 @@ union event_data { > oftree tree; > struct bootm_headers *images; > } ft_fixup; > + > + struct event_lmb_map_update { > + u64 base; > + u64 size; > + u8 op; > + } lmb_map; > }; > > /**
On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 9/5/24 10:27, Sughosh Ganu wrote: > > Add an event which would be used for notifying changes in the > > LMB modules' memory map. This is to be used for having a > > synchronous view of the memory that is currently in use, and that is > > available for allocations. > > The synchronous view problem only exists because we are duplicating > data. Store the EFI memory type in LMB and the problem vanishes. The LMB module only concerns itself with RAM memory. If I understand correctly, you are proposing maintaining the EFI memory map within the LMB module ? That would mean handling memory types other than conventional memory in LMB. -sughosh > > The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. > > Best regards > > Heinrich > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > common/event.c | 2 ++ > > include/event.h | 14 ++++++++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/common/event.c b/common/event.c > > index dda569d447..fc8002603c 100644 > > --- a/common/event.c > > +++ b/common/event.c > > @@ -48,6 +48,8 @@ const char *const type_name[] = { > > > > /* main loop events */ > > "main_loop", > > + > > + "lmb_map_update", > > }; > > > > _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > > diff --git a/include/event.h b/include/event.h > > index fb353ad623..fce7e96170 100644 > > --- a/include/event.h > > +++ b/include/event.h > > @@ -153,6 +153,14 @@ enum event_t { > > */ > > EVT_MAIN_LOOP, > > > > + /** > > + * @EVT_LMB_MAP_UPDATE: > > + * This event is triggered on an update to the LMB reserved memory > > + * region. This can be used to notify about any LMB memory allocation > > + * or freeing of memory having occurred. > > + */ > > + EVT_LMB_MAP_UPDATE, > > + > > /** > > * @EVT_COUNT: > > * This constants holds the maximum event number + 1 and is used when > > @@ -203,6 +211,12 @@ union event_data { > > oftree tree; > > struct bootm_headers *images; > > } ft_fixup; > > + > > + struct event_lmb_map_update { > > + u64 base; > > + u64 size; > > + u8 op; > > + } lmb_map; > > }; > > > > /** >
Hi Sughosh, On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > Add an event which would be used for notifying changes in the > > > LMB modules' memory map. This is to be used for having a > > > synchronous view of the memory that is currently in use, and that is > > > available for allocations. > > > > The synchronous view problem only exists because we are duplicating > > data. Store the EFI memory type in LMB and the problem vanishes. > > The LMB module only concerns itself with RAM memory. If I understand > correctly, you are proposing maintaining the EFI memory map within the > LMB module ? That would mean handling memory types other than > conventional memory in LMB. I am pretty sure I've asked this before, but do these *always* need to be in sync? The efi allocators will call LMB now. So when we allocate something gtom EFI, even if any potential changes from LMB haven't been published to EFI, we won't have any memory corruptions. Can't we just opportunistically update the memory map once someone requests it? Thanks /Ilias > > -sughosh > > > > > The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. > > > > Best regards > > > > Heinrich > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > common/event.c | 2 ++ > > > include/event.h | 14 ++++++++++++++ > > > 2 files changed, 16 insertions(+) > > > > > > diff --git a/common/event.c b/common/event.c > > > index dda569d447..fc8002603c 100644 > > > --- a/common/event.c > > > +++ b/common/event.c > > > @@ -48,6 +48,8 @@ const char *const type_name[] = { > > > > > > /* main loop events */ > > > "main_loop", > > > + > > > + "lmb_map_update", > > > }; > > > > > > _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > > > diff --git a/include/event.h b/include/event.h > > > index fb353ad623..fce7e96170 100644 > > > --- a/include/event.h > > > +++ b/include/event.h > > > @@ -153,6 +153,14 @@ enum event_t { > > > */ > > > EVT_MAIN_LOOP, > > > > > > + /** > > > + * @EVT_LMB_MAP_UPDATE: > > > + * This event is triggered on an update to the LMB reserved memory > > > + * region. This can be used to notify about any LMB memory allocation > > > + * or freeing of memory having occurred. > > > + */ > > > + EVT_LMB_MAP_UPDATE, > > > + > > > /** > > > * @EVT_COUNT: > > > * This constants holds the maximum event number + 1 and is used when > > > @@ -203,6 +211,12 @@ union event_data { > > > oftree tree; > > > struct bootm_headers *images; > > > } ft_fixup; > > > + > > > + struct event_lmb_map_update { > > > + u64 base; > > > + u64 size; > > > + u8 op; > > > + } lmb_map; > > > }; > > > > > > /** > >
On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sughosh, > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > Add an event which would be used for notifying changes in the > > > > LMB modules' memory map. This is to be used for having a > > > > synchronous view of the memory that is currently in use, and that is > > > > available for allocations. > > > > > > The synchronous view problem only exists because we are duplicating > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > The LMB module only concerns itself with RAM memory. If I understand > > correctly, you are proposing maintaining the EFI memory map within the > > LMB module ? That would mean handling memory types other than > > conventional memory in LMB. > > I am pretty sure I've asked this before, but do these *always* need to > be in sync? > > The efi allocators will call LMB now. So when we allocate something > gtom EFI, even if any potential changes from LMB haven't been > published to EFI, we won't have any memory corruptions. Can't we just > opportunistically update the memory map once someone requests it? I have given this a thought. Because what you mention, Simon has a similar comment. But to achieve this, it would require generating a new efi memory map afresh every time such a requirement comes up. This would mean, create a new memory map, put in the conventional memory, and add other memory types that were part of the existing memory map. And then remove the older memory map. This would need to be done every time a memory map is needed to be generated. And that would also include instances when a user enters a command to get the current memory map. I think notifying any changes to the lmb memory map to the efi memory module is easier, and less error prone. -sughosh > > Thanks > /Ilias > > > > -sughosh > > > > > > > > The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. > > > > > > Best regards > > > > > > Heinrich > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > common/event.c | 2 ++ > > > > include/event.h | 14 ++++++++++++++ > > > > 2 files changed, 16 insertions(+) > > > > > > > > diff --git a/common/event.c b/common/event.c > > > > index dda569d447..fc8002603c 100644 > > > > --- a/common/event.c > > > > +++ b/common/event.c > > > > @@ -48,6 +48,8 @@ const char *const type_name[] = { > > > > > > > > /* main loop events */ > > > > "main_loop", > > > > + > > > > + "lmb_map_update", > > > > }; > > > > > > > > _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > > > > diff --git a/include/event.h b/include/event.h > > > > index fb353ad623..fce7e96170 100644 > > > > --- a/include/event.h > > > > +++ b/include/event.h > > > > @@ -153,6 +153,14 @@ enum event_t { > > > > */ > > > > EVT_MAIN_LOOP, > > > > > > > > + /** > > > > + * @EVT_LMB_MAP_UPDATE: > > > > + * This event is triggered on an update to the LMB reserved memory > > > > + * region. This can be used to notify about any LMB memory allocation > > > > + * or freeing of memory having occurred. > > > > + */ > > > > + EVT_LMB_MAP_UPDATE, > > > > + > > > > /** > > > > * @EVT_COUNT: > > > > * This constants holds the maximum event number + 1 and is used when > > > > @@ -203,6 +211,12 @@ union event_data { > > > > oftree tree; > > > > struct bootm_headers *images; > > > > } ft_fixup; > > > > + > > > > + struct event_lmb_map_update { > > > > + u64 base; > > > > + u64 size; > > > > + u8 op; > > > > + } lmb_map; > > > > }; > > > > > > > > /** > > >
Hi Sughosh, On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Sughosh, > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > Add an event which would be used for notifying changes in the > > > > > LMB modules' memory map. This is to be used for having a > > > > > synchronous view of the memory that is currently in use, and that is > > > > > available for allocations. > > > > > > > > The synchronous view problem only exists because we are duplicating > > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > > > The LMB module only concerns itself with RAM memory. If I understand > > > correctly, you are proposing maintaining the EFI memory map within the > > > LMB module ? That would mean handling memory types other than > > > conventional memory in LMB. > > > > I am pretty sure I've asked this before, but do these *always* need to > > be in sync? > > > > The efi allocators will call LMB now. So when we allocate something > > gtom EFI, even if any potential changes from LMB haven't been > > published to EFI, we won't have any memory corruptions. Can't we just > > opportunistically update the memory map once someone requests it? > > I have given this a thought. Because what you mention, Simon has a > similar comment. But to achieve this, it would require generating a > new efi memory map afresh every time such a requirement comes up. This > would mean, create a new memory map, put in the conventional memory, > and add other memory types that were part of the existing memory map. > And then remove the older memory map. This would need to be done every > time a memory map is needed to be generated. And that would also > include instances when a user enters a command to get the current > memory map. I think notifying any changes to the lmb memory map to the > efi memory module is easier, and less error prone. We need to get some agreement on my memory-allocation patches first. I don't believe we are on the same page on those, despite some weeks of discussion. We need to resolve that issue first. I did try right from the start to first agree on the problem to be solved. We skipped that, so now we are having to do it now... Regards, Simon > > -sughosh > > > > > Thanks > > /Ilias > > > > > > -sughosh > > > > > > > > > > > The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. > > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > common/event.c | 2 ++ > > > > > include/event.h | 14 ++++++++++++++ > > > > > 2 files changed, 16 insertions(+) > > > > > > > > > > diff --git a/common/event.c b/common/event.c > > > > > index dda569d447..fc8002603c 100644 > > > > > --- a/common/event.c > > > > > +++ b/common/event.c > > > > > @@ -48,6 +48,8 @@ const char *const type_name[] = { > > > > > > > > > > /* main loop events */ > > > > > "main_loop", > > > > > + > > > > > + "lmb_map_update", > > > > > }; > > > > > > > > > > _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > > > > > diff --git a/include/event.h b/include/event.h > > > > > index fb353ad623..fce7e96170 100644 > > > > > --- a/include/event.h > > > > > +++ b/include/event.h > > > > > @@ -153,6 +153,14 @@ enum event_t { > > > > > */ > > > > > EVT_MAIN_LOOP, > > > > > > > > > > + /** > > > > > + * @EVT_LMB_MAP_UPDATE: > > > > > + * This event is triggered on an update to the LMB reserved memory > > > > > + * region. This can be used to notify about any LMB memory allocation > > > > > + * or freeing of memory having occurred. > > > > > + */ > > > > > + EVT_LMB_MAP_UPDATE, > > > > > + > > > > > /** > > > > > * @EVT_COUNT: > > > > > * This constants holds the maximum event number + 1 and is used when > > > > > @@ -203,6 +211,12 @@ union event_data { > > > > > oftree tree; > > > > > struct bootm_headers *images; > > > > > } ft_fixup; > > > > > + > > > > > + struct event_lmb_map_update { > > > > > + u64 base; > > > > > + u64 size; > > > > > + u8 op; > > > > > + } lmb_map; > > > > > }; > > > > > > > > > > /** > > > >
On Wed, 25 Sept 2024 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > > Add an event which would be used for notifying changes in the > > > > > > LMB modules' memory map. This is to be used for having a > > > > > > synchronous view of the memory that is currently in use, and that is > > > > > > available for allocations. > > > > > > > > > > The synchronous view problem only exists because we are duplicating > > > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > > > > > The LMB module only concerns itself with RAM memory. If I understand > > > > correctly, you are proposing maintaining the EFI memory map within the > > > > LMB module ? That would mean handling memory types other than > > > > conventional memory in LMB. > > > > > > I am pretty sure I've asked this before, but do these *always* need to > > > be in sync? > > > > > > The efi allocators will call LMB now. So when we allocate something > > > gtom EFI, even if any potential changes from LMB haven't been > > > published to EFI, we won't have any memory corruptions. Can't we just > > > opportunistically update the memory map once someone requests it? > > > > I have given this a thought. Because what you mention, Simon has a > > similar comment. But to achieve this, it would require generating a > > new efi memory map afresh every time such a requirement comes up. This > > would mean, create a new memory map, put in the conventional memory, > > and add other memory types that were part of the existing memory map. > > And then remove the older memory map. This would need to be done every > > time a memory map is needed to be generated. And that would also > > include instances when a user enters a command to get the current > > memory map. I think notifying any changes to the lmb memory map to the > > efi memory module is easier, and less error prone. > > We need to get some agreement on my memory-allocation patches first. I > don't believe we are on the same page on those, despite some weeks of > discussion. We need to resolve that issue first. I did try right from > the start to first agree on the problem to be solved. We skipped that, > so now we are having to do it now... These patches are currently under review. But fwiw, I think you are aware that these patches are not related to what your patch series is attempting. Your patches are related to the efi_allocate_pool() function, whereas this series is trying to use LMB as the backend for allocating pages, as requested from efi_allocate_pages(). So these are not related. But like I said, you are aware of these details :) -sughosh > > Regards, > Simon > > > > > > -sughosh > > > > > > > > Thanks > > > /Ilias > > > > > > > > -sughosh > > > > > > > > > > > > > > The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. > > > > > > > > > > Best regards > > > > > > > > > > Heinrich > > > > > > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > common/event.c | 2 ++ > > > > > > include/event.h | 14 ++++++++++++++ > > > > > > 2 files changed, 16 insertions(+) > > > > > > > > > > > > diff --git a/common/event.c b/common/event.c > > > > > > index dda569d447..fc8002603c 100644 > > > > > > --- a/common/event.c > > > > > > +++ b/common/event.c > > > > > > @@ -48,6 +48,8 @@ const char *const type_name[] = { > > > > > > > > > > > > /* main loop events */ > > > > > > "main_loop", > > > > > > + > > > > > > + "lmb_map_update", > > > > > > }; > > > > > > > > > > > > _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > > > > > > diff --git a/include/event.h b/include/event.h > > > > > > index fb353ad623..fce7e96170 100644 > > > > > > --- a/include/event.h > > > > > > +++ b/include/event.h > > > > > > @@ -153,6 +153,14 @@ enum event_t { > > > > > > */ > > > > > > EVT_MAIN_LOOP, > > > > > > > > > > > > + /** > > > > > > + * @EVT_LMB_MAP_UPDATE: > > > > > > + * This event is triggered on an update to the LMB reserved memory > > > > > > + * region. This can be used to notify about any LMB memory allocation > > > > > > + * or freeing of memory having occurred. > > > > > > + */ > > > > > > + EVT_LMB_MAP_UPDATE, > > > > > > + > > > > > > /** > > > > > > * @EVT_COUNT: > > > > > > * This constants holds the maximum event number + 1 and is used when > > > > > > @@ -203,6 +211,12 @@ union event_data { > > > > > > oftree tree; > > > > > > struct bootm_headers *images; > > > > > > } ft_fixup; > > > > > > + > > > > > > + struct event_lmb_map_update { > > > > > > + u64 base; > > > > > > + u64 size; > > > > > > + u8 op; > > > > > > + } lmb_map; > > > > > > }; > > > > > > > > > > > > /** > > > > >
Hi Sughosh, On Thu, 26 Sept 2024 at 09:12, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Wed, 25 Sept 2024 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > > > Add an event which would be used for notifying changes in the > > > > > > > LMB modules' memory map. This is to be used for having a > > > > > > > synchronous view of the memory that is currently in use, and that is > > > > > > > available for allocations. > > > > > > > > > > > > The synchronous view problem only exists because we are duplicating > > > > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > > > > > > > The LMB module only concerns itself with RAM memory. If I understand > > > > > correctly, you are proposing maintaining the EFI memory map within the > > > > > LMB module ? That would mean handling memory types other than > > > > > conventional memory in LMB. > > > > > > > > I am pretty sure I've asked this before, but do these *always* need to > > > > be in sync? > > > > > > > > The efi allocators will call LMB now. So when we allocate something > > > > gtom EFI, even if any potential changes from LMB haven't been > > > > published to EFI, we won't have any memory corruptions. Can't we just > > > > opportunistically update the memory map once someone requests it? > > > > > > I have given this a thought. Because what you mention, Simon has a > > > similar comment. But to achieve this, it would require generating a > > > new efi memory map afresh every time such a requirement comes up. This > > > would mean, create a new memory map, put in the conventional memory, > > > and add other memory types that were part of the existing memory map. > > > And then remove the older memory map. This would need to be done every > > > time a memory map is needed to be generated. And that would also > > > include instances when a user enters a command to get the current > > > memory map. I think notifying any changes to the lmb memory map to the > > > efi memory module is easier, and less error prone. > > > > We need to get some agreement on my memory-allocation patches first. I > > don't believe we are on the same page on those, despite some weeks of > > discussion. We need to resolve that issue first. I did try right from > > the start to first agree on the problem to be solved. We skipped that, > > so now we are having to do it now... > > These patches are currently under review. But fwiw, I think you are > aware that these patches are not related to what your patch series is > attempting. Your patches are related to the efi_allocate_pool() > function, whereas this series is trying to use LMB as the backend for > allocating pages, as requested from efi_allocate_pages(). So these are > not related. But like I said, you are aware of these details :) The thing is, if we actually tidy up the EFI allocations then there will be no use of allocate-pages before the app starts. So we won't need to 'sync' the two different sets of memory tables. There will just be one. If I were confident that we could apply your series and then clean it up later, I would be fine with it. But given that my two series have sat here for a considerable period of time and are still not really making progress, I lack confidence. Regards, Simon [..]
On Thu, 26 Sept 2024 at 16:38, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 26 Sept 2024 at 09:12, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Wed, 25 Sept 2024 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > > > > Add an event which would be used for notifying changes in the > > > > > > > > LMB modules' memory map. This is to be used for having a > > > > > > > > synchronous view of the memory that is currently in use, and that is > > > > > > > > available for allocations. > > > > > > > > > > > > > > The synchronous view problem only exists because we are duplicating > > > > > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > > > > > > > > > The LMB module only concerns itself with RAM memory. If I understand > > > > > > correctly, you are proposing maintaining the EFI memory map within the > > > > > > LMB module ? That would mean handling memory types other than > > > > > > conventional memory in LMB. > > > > > > > > > > I am pretty sure I've asked this before, but do these *always* need to > > > > > be in sync? > > > > > > > > > > The efi allocators will call LMB now. So when we allocate something > > > > > gtom EFI, even if any potential changes from LMB haven't been > > > > > published to EFI, we won't have any memory corruptions. Can't we just > > > > > opportunistically update the memory map once someone requests it? > > > > > > > > I have given this a thought. Because what you mention, Simon has a > > > > similar comment. But to achieve this, it would require generating a > > > > new efi memory map afresh every time such a requirement comes up. This > > > > would mean, create a new memory map, put in the conventional memory, > > > > and add other memory types that were part of the existing memory map. > > > > And then remove the older memory map. This would need to be done every > > > > time a memory map is needed to be generated. And that would also > > > > include instances when a user enters a command to get the current > > > > memory map. I think notifying any changes to the lmb memory map to the > > > > efi memory module is easier, and less error prone. > > > > > > We need to get some agreement on my memory-allocation patches first. I > > > don't believe we are on the same page on those, despite some weeks of > > > discussion. We need to resolve that issue first. I did try right from > > > the start to first agree on the problem to be solved. We skipped that, > > > so now we are having to do it now... > > > > These patches are currently under review. But fwiw, I think you are > > aware that these patches are not related to what your patch series is > > attempting. Your patches are related to the efi_allocate_pool() > > function, whereas this series is trying to use LMB as the backend for > > allocating pages, as requested from efi_allocate_pages(). So these are > > not related. But like I said, you are aware of these details :) > > The thing is, if we actually tidy up the EFI allocations then there > will be no use of allocate-pages before the app starts. So we won't > need to 'sync' the two different sets of memory tables. There will > just be one. How do we get rid of using efi_allocate_pages() in U-Boot ? This function is used, among other things, to allocate memory used for loading images to memory. This can be a significant amount of memory. Are you proposing using malloc() even for loading EFI images ? Moreover, the syncing of the memory maps is currently needed primarily as we support printing the EFI memory map through a command. And I mentioned in one of my earlier replies that not syncing the changes in the LMB memory map with the EFI module would mean that the EFI memory map would have to be generated afresh whenever there is a need for the EFI memory map -- this would be either when requested from an EFI app, or when a user asks through a command. Re-generating the EFI memory map is not a trivial task, and syncing the changes in the LMB map as and when they happen is much easier. -sughosh > > If I were confident that we could apply your series and then clean it > up later, I would be fine with it. But given that my two series have > sat here for a considerable period of time and are still not really > making progress, I lack confidence. > > Regards, > Simon > [..]
Hi Heinrich, On Sat, 14 Sept 2024 at 18:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 9/5/24 10:27, Sughosh Ganu wrote: > > Add an event which would be used for notifying changes in the > > LMB modules' memory map. This is to be used for having a > > synchronous view of the memory that is currently in use, and that is > > available for allocations. > > The synchronous view problem only exists because we are duplicating > data. Store the EFI memory type in LMB and the problem vanishes. We could, but I wonder if that would be enough. Apart from the EFI memory types, we plan on fixing the EFI memory attributes. I'll go read the spec again and take look at EDK2 but IIRC, you can't correlate memory types to memory attributes (at least not for all of them). If that's true we'll also need the attributes in LMB + perhaps logic for 64kb page size OSes and 4KB page firmware, which might be a bit too much. In that case, I think the sync is fine, but I really don't see the point of making an event out of it. Perhaps we could just update the efi memory map directly from LMB. Thanks /Ilias > > The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. > > Best regards > > Heinrich > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > common/event.c | 2 ++ > > include/event.h | 14 ++++++++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/common/event.c b/common/event.c > > index dda569d447..fc8002603c 100644 > > --- a/common/event.c > > +++ b/common/event.c > > @@ -48,6 +48,8 @@ const char *const type_name[] = { > > > > /* main loop events */ > > "main_loop", > > + > > + "lmb_map_update", > > }; > > > > _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > > diff --git a/include/event.h b/include/event.h > > index fb353ad623..fce7e96170 100644 > > --- a/include/event.h > > +++ b/include/event.h > > @@ -153,6 +153,14 @@ enum event_t { > > */ > > EVT_MAIN_LOOP, > > > > + /** > > + * @EVT_LMB_MAP_UPDATE: > > + * This event is triggered on an update to the LMB reserved memory > > + * region. This can be used to notify about any LMB memory allocation > > + * or freeing of memory having occurred. > > + */ > > + EVT_LMB_MAP_UPDATE, > > + > > /** > > * @EVT_COUNT: > > * This constants holds the maximum event number + 1 and is used when > > @@ -203,6 +211,12 @@ union event_data { > > oftree tree; > > struct bootm_headers *images; > > } ft_fixup; > > + > > + struct event_lmb_map_update { > > + u64 base; > > + u64 size; > > + u8 op; > > + } lmb_map; > > }; > > > > /** >
On 26.09.24 14:58, Ilias Apalodimas wrote: > Hi Heinrich, > > On Sat, 14 Sept 2024 at 18:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 9/5/24 10:27, Sughosh Ganu wrote: >>> Add an event which would be used for notifying changes in the >>> LMB modules' memory map. This is to be used for having a >>> synchronous view of the memory that is currently in use, and that is >>> available for allocations. >> >> The synchronous view problem only exists because we are duplicating >> data. Store the EFI memory type in LMB and the problem vanishes. > > We could, but I wonder if that would be enough. > Apart from the EFI memory types, we plan on fixing the EFI memory > attributes. I'll go read the spec again and take look at EDK2 but > IIRC, you can't correlate memory types to memory attributes (at least > not for all of them). > > If that's true we'll also need the attributes in LMB + perhaps logic > for 64kb page size OSes and 4KB page firmware, which might be a bit > too much. In that case, I think the sync is fine, but I really don't > see the point of making an event out of it. Perhaps we could just > update the efi memory map directly from LMB. Yes, memory attributes should be stored in LMB too if UEFI is enabled. I would suggest: * GetMemoryMap() calls into LMB to generate the map on the fly. * The EFI_MEMORY_ATTRIBUTES_TABLE is generated from LMB information. * The EFI_MEMORY_ATTRIBUTE_PROTOCOL calls into LMB. Best regards Heinrich > > Thanks > /Ilias >> >> The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. >> >> Best regards >> >> Heinrich >> >>> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >>> --- >>> common/event.c | 2 ++ >>> include/event.h | 14 ++++++++++++++ >>> 2 files changed, 16 insertions(+) >>> >>> diff --git a/common/event.c b/common/event.c >>> index dda569d447..fc8002603c 100644 >>> --- a/common/event.c >>> +++ b/common/event.c >>> @@ -48,6 +48,8 @@ const char *const type_name[] = { >>> >>> /* main loop events */ >>> "main_loop", >>> + >>> + "lmb_map_update", >>> }; >>> >>> _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); >>> diff --git a/include/event.h b/include/event.h >>> index fb353ad623..fce7e96170 100644 >>> --- a/include/event.h >>> +++ b/include/event.h >>> @@ -153,6 +153,14 @@ enum event_t { >>> */ >>> EVT_MAIN_LOOP, >>> >>> + /** >>> + * @EVT_LMB_MAP_UPDATE: >>> + * This event is triggered on an update to the LMB reserved memory >>> + * region. This can be used to notify about any LMB memory allocation >>> + * or freeing of memory having occurred. >>> + */ >>> + EVT_LMB_MAP_UPDATE, >>> + >>> /** >>> * @EVT_COUNT: >>> * This constants holds the maximum event number + 1 and is used when >>> @@ -203,6 +211,12 @@ union event_data { >>> oftree tree; >>> struct bootm_headers *images; >>> } ft_fixup; >>> + >>> + struct event_lmb_map_update { >>> + u64 base; >>> + u64 size; >>> + u8 op; >>> + } lmb_map; >>> }; >>> >>> /** >>
Hi Heinrich, On Thu, 26 Sept 2024 at 16:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 26.09.24 14:58, Ilias Apalodimas wrote: > > Hi Heinrich, > > > > On Sat, 14 Sept 2024 at 18:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 9/5/24 10:27, Sughosh Ganu wrote: > >>> Add an event which would be used for notifying changes in the > >>> LMB modules' memory map. This is to be used for having a > >>> synchronous view of the memory that is currently in use, and that is > >>> available for allocations. > >> > >> The synchronous view problem only exists because we are duplicating > >> data. Store the EFI memory type in LMB and the problem vanishes. > > > > We could, but I wonder if that would be enough. > > Apart from the EFI memory types, we plan on fixing the EFI memory > > attributes. I'll go read the spec again and take look at EDK2 but > > IIRC, you can't correlate memory types to memory attributes (at least > > not for all of them). > > > > If that's true we'll also need the attributes in LMB + perhaps logic > > for 64kb page size OSes and 4KB page firmware, which might be a bit > > too much. In that case, I think the sync is fine, but I really don't > > see the point of making an event out of it. Perhaps we could just > > update the efi memory map directly from LMB. > > Yes, memory attributes should be stored in LMB too if UEFI is enabled. > > I would suggest: > > * GetMemoryMap() calls into LMB to generate the map on the fly. > * The EFI_MEMORY_ATTRIBUTES_TABLE is generated from LMB information. > * The EFI_MEMORY_ATTRIBUTE_PROTOCOL calls into LMB. > > Best regards > > Heinrich That would work. TBH I do have a few doubts on how clean this is going to be. It seems a bit cleaner to me atm to keep the efi memory map as is. Especially if we get rid of the event. Would it make sense to clean this up from the event and merge it close to its current form? Then we could prototype what you suggest and send an RFC to decide it it looks cleaner or not. Thanks /Ilias > > > > > Thanks > > /Ilias > >> > >> The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. > >> > >> Best regards > >> > >> Heinrich > >> > >>> > >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > >>> --- > >>> common/event.c | 2 ++ > >>> include/event.h | 14 ++++++++++++++ > >>> 2 files changed, 16 insertions(+) > >>> > >>> diff --git a/common/event.c b/common/event.c > >>> index dda569d447..fc8002603c 100644 > >>> --- a/common/event.c > >>> +++ b/common/event.c > >>> @@ -48,6 +48,8 @@ const char *const type_name[] = { > >>> > >>> /* main loop events */ > >>> "main_loop", > >>> + > >>> + "lmb_map_update", > >>> }; > >>> > >>> _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); > >>> diff --git a/include/event.h b/include/event.h > >>> index fb353ad623..fce7e96170 100644 > >>> --- a/include/event.h > >>> +++ b/include/event.h > >>> @@ -153,6 +153,14 @@ enum event_t { > >>> */ > >>> EVT_MAIN_LOOP, > >>> > >>> + /** > >>> + * @EVT_LMB_MAP_UPDATE: > >>> + * This event is triggered on an update to the LMB reserved memory > >>> + * region. This can be used to notify about any LMB memory allocation > >>> + * or freeing of memory having occurred. > >>> + */ > >>> + EVT_LMB_MAP_UPDATE, > >>> + > >>> /** > >>> * @EVT_COUNT: > >>> * This constants holds the maximum event number + 1 and is used when > >>> @@ -203,6 +211,12 @@ union event_data { > >>> oftree tree; > >>> struct bootm_headers *images; > >>> } ft_fixup; > >>> + > >>> + struct event_lmb_map_update { > >>> + u64 base; > >>> + u64 size; > >>> + u8 op; > >>> + } lmb_map; > >>> }; > >>> > >>> /** > >> >
On 26.09.24 15:54, Ilias Apalodimas wrote: > Hi Heinrich, > > On Thu, 26 Sept 2024 at 16:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 26.09.24 14:58, Ilias Apalodimas wrote: >>> Hi Heinrich, >>> >>> On Sat, 14 Sept 2024 at 18:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >>>> >>>> On 9/5/24 10:27, Sughosh Ganu wrote: >>>>> Add an event which would be used for notifying changes in the >>>>> LMB modules' memory map. This is to be used for having a >>>>> synchronous view of the memory that is currently in use, and that is >>>>> available for allocations. >>>> >>>> The synchronous view problem only exists because we are duplicating >>>> data. Store the EFI memory type in LMB and the problem vanishes. >>> >>> We could, but I wonder if that would be enough. >>> Apart from the EFI memory types, we plan on fixing the EFI memory >>> attributes. I'll go read the spec again and take look at EDK2 but >>> IIRC, you can't correlate memory types to memory attributes (at least >>> not for all of them). >>> >>> If that's true we'll also need the attributes in LMB + perhaps logic >>> for 64kb page size OSes and 4KB page firmware, which might be a bit >>> too much. In that case, I think the sync is fine, but I really don't >>> see the point of making an event out of it. Perhaps we could just >>> update the efi memory map directly from LMB. >> >> Yes, memory attributes should be stored in LMB too if UEFI is enabled. >> >> I would suggest: >> >> * GetMemoryMap() calls into LMB to generate the map on the fly. >> * The EFI_MEMORY_ATTRIBUTES_TABLE is generated from LMB information. >> * The EFI_MEMORY_ATTRIBUTE_PROTOCOL calls into LMB. >> >> Best regards >> >> Heinrich > > That would work. TBH I do have a few doubts on how clean this is going > to be. It seems a bit cleaner to me atm to keep the efi memory map as > is. Especially if we get rid of the event. > Would it make sense to clean this up from the event and merge it close > to its current form? Then we could prototype what you suggest and send > an RFC to decide it it looks cleaner or not. > Sure, we can split the work into two series. Best regards Heinrich >>>> >>>> The event is only needed to notify EFI_EVENT_GROUP_MEMORY_MAP_CHANGE. >>>> >>>> Best regards >>>> >>>> Heinrich >>>> >>>>> >>>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> >>>>> --- >>>>> common/event.c | 2 ++ >>>>> include/event.h | 14 ++++++++++++++ >>>>> 2 files changed, 16 insertions(+) >>>>> >>>>> diff --git a/common/event.c b/common/event.c >>>>> index dda569d447..fc8002603c 100644 >>>>> --- a/common/event.c >>>>> +++ b/common/event.c >>>>> @@ -48,6 +48,8 @@ const char *const type_name[] = { >>>>> >>>>> /* main loop events */ >>>>> "main_loop", >>>>> + >>>>> + "lmb_map_update", >>>>> }; >>>>> >>>>> _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); >>>>> diff --git a/include/event.h b/include/event.h >>>>> index fb353ad623..fce7e96170 100644 >>>>> --- a/include/event.h >>>>> +++ b/include/event.h >>>>> @@ -153,6 +153,14 @@ enum event_t { >>>>> */ >>>>> EVT_MAIN_LOOP, >>>>> >>>>> + /** >>>>> + * @EVT_LMB_MAP_UPDATE: >>>>> + * This event is triggered on an update to the LMB reserved memory >>>>> + * region. This can be used to notify about any LMB memory allocation >>>>> + * or freeing of memory having occurred. >>>>> + */ >>>>> + EVT_LMB_MAP_UPDATE, >>>>> + >>>>> /** >>>>> * @EVT_COUNT: >>>>> * This constants holds the maximum event number + 1 and is used when >>>>> @@ -203,6 +211,12 @@ union event_data { >>>>> oftree tree; >>>>> struct bootm_headers *images; >>>>> } ft_fixup; >>>>> + >>>>> + struct event_lmb_map_update { >>>>> + u64 base; >>>>> + u64 size; >>>>> + u8 op; >>>>> + } lmb_map; >>>>> }; >>>>> >>>>> /** >>>> >>
Hi Sughosh, On Thu, 26 Sept 2024 at 14:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Thu, 26 Sept 2024 at 16:38, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 26 Sept 2024 at 09:12, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Wed, 25 Sept 2024 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > > > > > Add an event which would be used for notifying changes in the > > > > > > > > > LMB modules' memory map. This is to be used for having a > > > > > > > > > synchronous view of the memory that is currently in use, and that is > > > > > > > > > available for allocations. > > > > > > > > > > > > > > > > The synchronous view problem only exists because we are duplicating > > > > > > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > > > > > > > > > > > The LMB module only concerns itself with RAM memory. If I understand > > > > > > > correctly, you are proposing maintaining the EFI memory map within the > > > > > > > LMB module ? That would mean handling memory types other than > > > > > > > conventional memory in LMB. > > > > > > > > > > > > I am pretty sure I've asked this before, but do these *always* need to > > > > > > be in sync? > > > > > > > > > > > > The efi allocators will call LMB now. So when we allocate something > > > > > > gtom EFI, even if any potential changes from LMB haven't been > > > > > > published to EFI, we won't have any memory corruptions. Can't we just > > > > > > opportunistically update the memory map once someone requests it? > > > > > > > > > > I have given this a thought. Because what you mention, Simon has a > > > > > similar comment. But to achieve this, it would require generating a > > > > > new efi memory map afresh every time such a requirement comes up. This > > > > > would mean, create a new memory map, put in the conventional memory, > > > > > and add other memory types that were part of the existing memory map. > > > > > And then remove the older memory map. This would need to be done every > > > > > time a memory map is needed to be generated. And that would also > > > > > include instances when a user enters a command to get the current > > > > > memory map. I think notifying any changes to the lmb memory map to the > > > > > efi memory module is easier, and less error prone. > > > > > > > > We need to get some agreement on my memory-allocation patches first. I > > > > don't believe we are on the same page on those, despite some weeks of > > > > discussion. We need to resolve that issue first. I did try right from > > > > the start to first agree on the problem to be solved. We skipped that, > > > > so now we are having to do it now... > > > > > > These patches are currently under review. But fwiw, I think you are > > > aware that these patches are not related to what your patch series is > > > attempting. Your patches are related to the efi_allocate_pool() > > > function, whereas this series is trying to use LMB as the backend for > > > allocating pages, as requested from efi_allocate_pages(). So these are > > > not related. But like I said, you are aware of these details :) > > > > The thing is, if we actually tidy up the EFI allocations then there > > will be no use of allocate-pages before the app starts. So we won't > > need to 'sync' the two different sets of memory tables. There will > > just be one. The issue is, again, that we never actually identified the problem to be solved at the beginning. So we are suffering somewhat now from a lack of clarity. But in any case, let me try to respond to the points you raise. > > How do we get rid of using efi_allocate_pages() in U-Boot ? This > function is used, among other things, to allocate memory used for > loading images to memory. This can be a significant amount of memory. > Are you proposing using malloc() even for loading EFI images ? We actually don't use efi_allocate_pages() to load things into memory, or at least I hope we don't. We should be using lmb. No, we cannot use malloc() for loading images as the region is quite small on many boards. Heinrich mentioned it can be as small as 1-2MB on some. Also, it isn't really what malloc() is designed for. > > Moreover, the syncing of the memory maps is currently needed primarily > as we support printing the EFI memory map through a command. And I > mentioned in one of my earlier replies that not syncing the changes in > the LMB memory map with the EFI module would mean that the EFI memory > map would have to be generated afresh whenever there is a need for the > EFI memory map -- this would be either when requested from an EFI app, > or when a user asks through a command. Re-generating the EFI memory > map is not a trivial task, and syncing the changes in the LMB map as > and when they happen is much easier. Yes I have seen you make this point and I am sorry I didn't reply directly. The thing is, we don't care about the EFI memory until we boot the app. After all, it is only the app that actually needs it. I see that 'efi_init_obj_list()' is called by the efidebug command, so yes, it should show the memory map. But in my view of the world the memory map (before an app boots and does what it likes in terms of allocations, etc.) consists solely of: 1. U-Boot's memory (malloc() space, code, bloblist, stack, arch-specific stuff, etc.) 2. Images that were loaded (which are in lmb) Since we need to handle (1) by manually adding records to the EFI map, it isn't a big stretch to handle (2) with a for-loop through the lmb records. > > If I were confident that we could apply your series and then clean it > > up later, I would be fine with it. But given that my two series have > > sat here for a considerable period of time and are still not really > > making progress, I lack confidence. Regards, Simon
On Fri, 27 Sept 2024 at 16:12, Simon Glass <sjg@chromium.org> wrote: > > Hi Sughosh, > > On Thu, 26 Sept 2024 at 14:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Thu, 26 Sept 2024 at 16:38, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Sughosh, > > > > > > On Thu, 26 Sept 2024 at 09:12, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > On Wed, 25 Sept 2024 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Sughosh, > > > > > > > > > > On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > > > > > > Add an event which would be used for notifying changes in the > > > > > > > > > > LMB modules' memory map. This is to be used for having a > > > > > > > > > > synchronous view of the memory that is currently in use, and that is > > > > > > > > > > available for allocations. > > > > > > > > > > > > > > > > > > The synchronous view problem only exists because we are duplicating > > > > > > > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > > > > > > > > > > > > > The LMB module only concerns itself with RAM memory. If I understand > > > > > > > > correctly, you are proposing maintaining the EFI memory map within the > > > > > > > > LMB module ? That would mean handling memory types other than > > > > > > > > conventional memory in LMB. > > > > > > > > > > > > > > I am pretty sure I've asked this before, but do these *always* need to > > > > > > > be in sync? > > > > > > > > > > > > > > The efi allocators will call LMB now. So when we allocate something > > > > > > > gtom EFI, even if any potential changes from LMB haven't been > > > > > > > published to EFI, we won't have any memory corruptions. Can't we just > > > > > > > opportunistically update the memory map once someone requests it? > > > > > > > > > > > > I have given this a thought. Because what you mention, Simon has a > > > > > > similar comment. But to achieve this, it would require generating a > > > > > > new efi memory map afresh every time such a requirement comes up. This > > > > > > would mean, create a new memory map, put in the conventional memory, > > > > > > and add other memory types that were part of the existing memory map. > > > > > > And then remove the older memory map. This would need to be done every > > > > > > time a memory map is needed to be generated. And that would also > > > > > > include instances when a user enters a command to get the current > > > > > > memory map. I think notifying any changes to the lmb memory map to the > > > > > > efi memory module is easier, and less error prone. > > > > > > > > > > We need to get some agreement on my memory-allocation patches first. I > > > > > don't believe we are on the same page on those, despite some weeks of > > > > > discussion. We need to resolve that issue first. I did try right from > > > > > the start to first agree on the problem to be solved. We skipped that, > > > > > so now we are having to do it now... > > > > > > > > These patches are currently under review. But fwiw, I think you are > > > > aware that these patches are not related to what your patch series is > > > > attempting. Your patches are related to the efi_allocate_pool() > > > > function, whereas this series is trying to use LMB as the backend for > > > > allocating pages, as requested from efi_allocate_pages(). So these are > > > > not related. But like I said, you are aware of these details :) > > > > > > The thing is, if we actually tidy up the EFI allocations then there > > > will be no use of allocate-pages before the app starts. So we won't > > > need to 'sync' the two different sets of memory tables. There will > > > just be one. > > The issue is, again, that we never actually identified the problem to > be solved at the beginning. So we are suffering somewhat now from a > lack of clarity. But in any case, let me try to respond to the points > you raise. > > > > > How do we get rid of using efi_allocate_pages() in U-Boot ? This > > function is used, among other things, to allocate memory used for > > loading images to memory. This can be a significant amount of memory. > > Are you proposing using malloc() even for loading EFI images ? > > We actually don't use efi_allocate_pages() to load things into memory, > or at least I hope we don't. We should be using lmb. No, we cannot use > malloc() for loading images as the region is quite small on many > boards. Heinrich mentioned it can be as small as 1-2MB on some. Also, > it isn't really what malloc() is designed for. So you are proposing using lmb for loading images. And that is precisely what this series is doing -- changing the efi_allocate_pages() function so that it calls the lmb API's to get the memory. So then I wonder why are we not on the same page ? Yes, I understand that you have a differing view when it comes to efi_allocate_pool(), but the changes to the efi_allocate_pages() done in this series are exactly what you refer to above. > > > > > Moreover, the syncing of the memory maps is currently needed primarily > > as we support printing the EFI memory map through a command. And I > > mentioned in one of my earlier replies that not syncing the changes in > > the LMB memory map with the EFI module would mean that the EFI memory > > map would have to be generated afresh whenever there is a need for the > > EFI memory map -- this would be either when requested from an EFI app, > > or when a user asks through a command. Re-generating the EFI memory > > map is not a trivial task, and syncing the changes in the LMB map as > > and when they happen is much easier. > > Yes I have seen you make this point and I am sorry I didn't reply > directly. The thing is, we don't care about the EFI memory until we > boot the app. After all, it is only the app that actually needs it. > > I see that 'efi_init_obj_list()' is called by the efidebug command, so > yes, it should show the memory map. > > But in my view of the world the memory map (before an app boots and > does what it likes in terms of allocations, etc.) consists solely of: > > 1. U-Boot's memory (malloc() space, code, bloblist, stack, > arch-specific stuff, etc.) > 2. Images that were loaded (which are in lmb) > > Since we need to handle (1) by manually adding records to the EFI map, > it isn't a big stretch to handle (2) with a for-loop through the lmb > records. > > > > > If I were confident that we could apply your series and then clean it > > > up later, I would be fine with it. But given that my two series have > > > sat here for a considerable period of time and are still not really > > > making progress, I lack confidence. So the difference here is, unlike the way the LMB memory map gets stored and shown, i.e. a separate entry for the total available memory, and then a separate list of currently allocated entries, the EFI memory map storage is designed in a different way. The conventional memory, which is the DRAM memory gets added first, and that is then followed by carving out the different memory regions which are being used, e.g. the U-Boot's memory, and other memory regions that have been requested. So if you try out the 'efidebug memmap' command, you will see a single list of memory regions, which includes the conventional memory, and other reserved memory regions. Although I did not come up with this design, I suspect that it is the way it is because the EFI memory map is not dealing only with DRAM/conventional memory, but other possible memory regions as well. What this means is that syncing the EFI memory map in the manner that you propose would mean that the entire EFI memory map would have to be re-generated, as the carve-out's (reserved memory regions) might have changed. Please also note that, apart from the added complexity, we are also going to be worse off from the size impact point-of-view. Especially if we consider Ilias's review comment of doing away with the event framework, and directly calling the efi_add_memory_map_pg() directly. -sughosh > > Regards, > Simon
Hi Sughosh, On Fri, 27 Sept 2024 at 05:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Fri, 27 Sept 2024 at 16:12, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Thu, 26 Sept 2024 at 14:29, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Thu, 26 Sept 2024 at 16:38, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Thu, 26 Sept 2024 at 09:12, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > On Wed, 25 Sept 2024 at 18:23, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas > > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > > > Hi Sughosh, > > > > > > > > > > > > > > > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > > > > > > > > > > > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > > > > > > > Add an event which would be used for notifying changes in the > > > > > > > > > > > LMB modules' memory map. This is to be used for having a > > > > > > > > > > > synchronous view of the memory that is currently in use, and that is > > > > > > > > > > > available for allocations. > > > > > > > > > > > > > > > > > > > > The synchronous view problem only exists because we are duplicating > > > > > > > > > > data. Store the EFI memory type in LMB and the problem vanishes. > > > > > > > > > > > > > > > > > > The LMB module only concerns itself with RAM memory. If I understand > > > > > > > > > correctly, you are proposing maintaining the EFI memory map within the > > > > > > > > > LMB module ? That would mean handling memory types other than > > > > > > > > > conventional memory in LMB. > > > > > > > > > > > > > > > > I am pretty sure I've asked this before, but do these *always* need to > > > > > > > > be in sync? > > > > > > > > > > > > > > > > The efi allocators will call LMB now. So when we allocate something > > > > > > > > gtom EFI, even if any potential changes from LMB haven't been > > > > > > > > published to EFI, we won't have any memory corruptions. Can't we just > > > > > > > > opportunistically update the memory map once someone requests it? > > > > > > > > > > > > > > I have given this a thought. Because what you mention, Simon has a > > > > > > > similar comment. But to achieve this, it would require generating a > > > > > > > new efi memory map afresh every time such a requirement comes up. This > > > > > > > would mean, create a new memory map, put in the conventional memory, > > > > > > > and add other memory types that were part of the existing memory map. > > > > > > > And then remove the older memory map. This would need to be done every > > > > > > > time a memory map is needed to be generated. And that would also > > > > > > > include instances when a user enters a command to get the current > > > > > > > memory map. I think notifying any changes to the lmb memory map to the > > > > > > > efi memory module is easier, and less error prone. > > > > > > > > > > > > We need to get some agreement on my memory-allocation patches first. I > > > > > > don't believe we are on the same page on those, despite some weeks of > > > > > > discussion. We need to resolve that issue first. I did try right from > > > > > > the start to first agree on the problem to be solved. We skipped that, > > > > > > so now we are having to do it now... > > > > > > > > > > These patches are currently under review. But fwiw, I think you are > > > > > aware that these patches are not related to what your patch series is > > > > > attempting. Your patches are related to the efi_allocate_pool() > > > > > function, whereas this series is trying to use LMB as the backend for > > > > > allocating pages, as requested from efi_allocate_pages(). So these are > > > > > not related. But like I said, you are aware of these details :) > > > > > > > > The thing is, if we actually tidy up the EFI allocations then there > > > > will be no use of allocate-pages before the app starts. So we won't > > > > need to 'sync' the two different sets of memory tables. There will > > > > just be one. > > > > The issue is, again, that we never actually identified the problem to > > be solved at the beginning. So we are suffering somewhat now from a > > lack of clarity. But in any case, let me try to respond to the points > > you raise. > > > > > > > > How do we get rid of using efi_allocate_pages() in U-Boot ? This > > > function is used, among other things, to allocate memory used for > > > loading images to memory. This can be a significant amount of memory. > > > Are you proposing using malloc() even for loading EFI images ? > > > > We actually don't use efi_allocate_pages() to load things into memory, > > or at least I hope we don't. We should be using lmb. No, we cannot use > > malloc() for loading images as the region is quite small on many > > boards. Heinrich mentioned it can be as small as 1-2MB on some. Also, > > it isn't really what malloc() is designed for. > > So you are proposing using lmb for loading images. And that is > precisely what this series is doing -- changing the > efi_allocate_pages() function so that it calls the lmb API's to get > the memory. So then I wonder why are we not on the same page ? Yes, I > understand that you have a differing view when it comes to > efi_allocate_pool(), but the changes to the efi_allocate_pages() done > in this series are exactly what you refer to above. > > > > > > > > > Moreover, the syncing of the memory maps is currently needed primarily > > > as we support printing the EFI memory map through a command. And I > > > mentioned in one of my earlier replies that not syncing the changes in > > > the LMB memory map with the EFI module would mean that the EFI memory > > > map would have to be generated afresh whenever there is a need for the > > > EFI memory map -- this would be either when requested from an EFI app, > > > or when a user asks through a command. Re-generating the EFI memory > > > map is not a trivial task, and syncing the changes in the LMB map as > > > and when they happen is much easier. > > > > Yes I have seen you make this point and I am sorry I didn't reply > > directly. The thing is, we don't care about the EFI memory until we > > boot the app. After all, it is only the app that actually needs it. > > > > I see that 'efi_init_obj_list()' is called by the efidebug command, so > > yes, it should show the memory map. > > > > But in my view of the world the memory map (before an app boots and > > does what it likes in terms of allocations, etc.) consists solely of: > > > > 1. U-Boot's memory (malloc() space, code, bloblist, stack, > > arch-specific stuff, etc.) > > 2. Images that were loaded (which are in lmb) > > > > Since we need to handle (1) by manually adding records to the EFI map, > > it isn't a big stretch to handle (2) with a for-loop through the lmb > > records. > > > > > > > > If I were confident that we could apply your series and then clean it > > > > up later, I would be fine with it. But given that my two series have > > > > sat here for a considerable period of time and are still not really > > > > making progress, I lack confidence. > > So the difference here is, unlike the way the LMB memory map gets > stored and shown, i.e. a separate entry for the total available > memory, and then a separate list of currently allocated entries, the > EFI memory map storage is designed in a different way. The > conventional memory, which is the DRAM memory gets added first, and > that is then followed by carving out the different memory regions > which are being used, e.g. the U-Boot's memory, and other memory > regions that have been requested. So if you try out the 'efidebug > memmap' command, you will see a single list of memory regions, which > includes the conventional memory, and other reserved memory regions. > Although I did not come up with this design, I suspect that it is the > way it is because the EFI memory map is not dealing only with > DRAM/conventional memory, but other possible memory regions as well. > > What this means is that syncing the EFI memory map in the manner that > you propose would mean that the entire EFI memory map would have to be > re-generated, as the carve-out's (reserved memory regions) might have > changed. Please also note that, apart from the added complexity, we > are also going to be worse off from the size impact point-of-view. > Especially if we consider Ilias's review comment of doing away with > the event framework, and directly calling the efi_add_memory_map_pg() > directly. Look, I certainly understand your frustration and I share it. I would like to see progress on the series i sent, which moves pool allocations to use malloc(). I would also like to see the EFI test applied, which has been held up for a long time (perhaps the latest version will be OK) I did discuss this with Tom and was OK with your first series on the basis that we can figure out the next step after that. But I never believed that the problem to be solved was that lmb and EFI were not deeply integrated and I was expecting to have made some progress on the malloc() issue by now. I should also point out that no one else dug into this to understand what was really going on (that EFI was allocating stuff where it shouldn't, i.e. in space that is supposed to be used by lmb). I had an inkling of it but had never spent the time to really understand it. My current view on EFI memory is that, just before launching the app (or firmware update, or whatever), we should be able to call efi_memory_init(), which adds the known memory, the U-Boot regions and the images it knows about. Only the last piece is missing today. As to that, we need to clean up the EFI hack and maintain a list of images and which device they came from, so that the EFI loader can find that out. It isn't just about lmb. So I hope you can understand that, coming from that point of view, all this stuff about keeping things in sync doesn't seem right. Regards, Simon
diff --git a/common/event.c b/common/event.c index dda569d447..fc8002603c 100644 --- a/common/event.c +++ b/common/event.c @@ -48,6 +48,8 @@ const char *const type_name[] = { /* main loop events */ "main_loop", + + "lmb_map_update", }; _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size"); diff --git a/include/event.h b/include/event.h index fb353ad623..fce7e96170 100644 --- a/include/event.h +++ b/include/event.h @@ -153,6 +153,14 @@ enum event_t { */ EVT_MAIN_LOOP, + /** + * @EVT_LMB_MAP_UPDATE: + * This event is triggered on an update to the LMB reserved memory + * region. This can be used to notify about any LMB memory allocation + * or freeing of memory having occurred. + */ + EVT_LMB_MAP_UPDATE, + /** * @EVT_COUNT: * This constants holds the maximum event number + 1 and is used when @@ -203,6 +211,12 @@ union event_data { oftree tree; struct bootm_headers *images; } ft_fixup; + + struct event_lmb_map_update { + u64 base; + u64 size; + u8 op; + } lmb_map; }; /**
Add an event which would be used for notifying changes in the LMB modules' memory map. This is to be used for having a synchronous view of the memory that is currently in use, and that is available for allocations. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- common/event.c | 2 ++ include/event.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+)