[PATCH 04/16] event: add event to notify lmb memory map changes

[Ilias Apalodimas]

Hi Heinrich,

On Thu, 26 Sept 2024 at 16:19, Heinrich Schuchardt <xypron.glpk at 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 at 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 at 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;
> >>>    };
> >>>
> >>>    /**
> >>
>