On Wed, 25 Sept 2024 at 18:23, Simon Glass <s...@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 20 Sept 2024 at 13:38, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
> >
> > On Fri, 20 Sept 2024 at 14:51, Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Tue, 17 Sept 2024 at 15:33, Sughosh Ganu <sughosh.g...@linaro.org>
> > > wrote:
> > > >
> > > > On Sat, 14 Sept 2024 at 20:38, Heinrich Schuchardt <xypron.g...@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.g...@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;
> > > > > > };
> > > > > >
> > > > > > /**
> > > > >
