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

[Heinrich Schuchardt]

On 26.09.24 15:54, Ilias Apalodimas wrote:
> 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.
>

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 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;
>>>>>     };
>>>>>
>>>>>     /**
>>>>
>>