diff mbox series

[RFC,v2,38/48] lib: Kconfig: add a config symbol for getting lmb memory map updates

Message ID 20240704073544.670249-39-sughosh.ganu@linaro.org
State Superseded
Headers show
Series Make U-Boot memory reservations coherent | expand

Commit Message

Sughosh Ganu July 4, 2024, 7:35 a.m. UTC
Add a Kconfig symbol to enable getting updates on any memory map
changes that might be done by the LMB module. This notification
mechanism can then be used to have a synchronous view of allocated and
free memory.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V1:
* Change the description to highlight only LMB notifications.
* Add a separate line for dependencies.

 lib/Kconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Tom Rini July 5, 2024, 7:50 p.m. UTC | #1
On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:

> Add a Kconfig symbol to enable getting updates on any memory map
> changes that might be done by the LMB module. This notification
> mechanism can then be used to have a synchronous view of allocated and
> free memory.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V1:
> * Change the description to highlight only LMB notifications.
> * Add a separate line for dependencies.
> 
>  lib/Kconfig | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 7eea517b3b..b422183a0f 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
>  config LIB_UUID
>  	bool
>  
> +config MEM_MAP_UPDATE_NOTIFY
> +	bool "Get notified of any changes to the LMB memory map"
> +	depends on EVENT && LMB && EFI_LOADER
> +	default y
> +	help
> +	  Enable this option to get notification on any changes to the
> +	  memory that is allocated or freed by the LMB module. This will
> +	  allow different modules that allocate memory or maintain a memory
> +	  map to have a synchronous view of available and allocated memory.

This needs to be select'd when it's going to be used, opting out of
making sure memory reservations are obeyed isn't a good idea.
Ilias Apalodimas July 22, 2024, 12:30 p.m. UTC | #2
On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
>
> > Add a Kconfig symbol to enable getting updates on any memory map
> > changes that might be done by the LMB module. This notification
> > mechanism can then be used to have a synchronous view of allocated and
> > free memory.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V1:
> > * Change the description to highlight only LMB notifications.
> > * Add a separate line for dependencies.
> >
> >  lib/Kconfig | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 7eea517b3b..b422183a0f 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> >  config LIB_UUID
> >       bool
> >
> > +config MEM_MAP_UPDATE_NOTIFY
> > +     bool "Get notified of any changes to the LMB memory map"
> > +     depends on EVENT && LMB && EFI_LOADER
> > +     default y
> > +     help
> > +       Enable this option to get notification on any changes to the
> > +       memory that is allocated or freed by the LMB module. This will
> > +       allow different modules that allocate memory or maintain a memory
> > +       map to have a synchronous view of available and allocated memory.
>
> This needs to be select'd when it's going to be used, opting out of
> making sure memory reservations are obeyed isn't a good idea.

+1 which begs the question, do we need the config option at all ?

Cheers
/Ilias
>
> --
> Tom
Sughosh Ganu July 22, 2024, 12:59 p.m. UTC | #3
On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> >
> > > Add a Kconfig symbol to enable getting updates on any memory map
> > > changes that might be done by the LMB module. This notification
> > > mechanism can then be used to have a synchronous view of allocated and
> > > free memory.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V1:
> > > * Change the description to highlight only LMB notifications.
> > > * Add a separate line for dependencies.
> > >
> > >  lib/Kconfig | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > index 7eea517b3b..b422183a0f 100644
> > > --- a/lib/Kconfig
> > > +++ b/lib/Kconfig
> > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > >  config LIB_UUID
> > >       bool
> > >
> > > +config MEM_MAP_UPDATE_NOTIFY
> > > +     bool "Get notified of any changes to the LMB memory map"
> > > +     depends on EVENT && LMB && EFI_LOADER
> > > +     default y
> > > +     help
> > > +       Enable this option to get notification on any changes to the
> > > +       memory that is allocated or freed by the LMB module. This will
> > > +       allow different modules that allocate memory or maintain a memory
> > > +       map to have a synchronous view of available and allocated memory.
> >
> > This needs to be select'd when it's going to be used, opting out of
> > making sure memory reservations are obeyed isn't a good idea.
>
> +1 which begs the question, do we need the config option at all ?

The config symbol can be used for removing the code for platforms
which do not support EFI ?

-sughosh

>
> Cheers
> /Ilias
> >
> > --
> > Tom
Ilias Apalodimas July 23, 2024, 7:09 a.m. UTC | #4
Hi Sughosh,


On Mon, 22 Jul 2024 at 15:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > >
> > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > changes that might be done by the LMB module. This notification
> > > > mechanism can then be used to have a synchronous view of allocated and
> > > > free memory.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > > Changes since V1:
> > > > * Change the description to highlight only LMB notifications.
> > > > * Add a separate line for dependencies.
> > > >
> > > >  lib/Kconfig | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > index 7eea517b3b..b422183a0f 100644
> > > > --- a/lib/Kconfig
> > > > +++ b/lib/Kconfig
> > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > >  config LIB_UUID
> > > >       bool
> > > >
> > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > +     default y
> > > > +     help
> > > > +       Enable this option to get notification on any changes to the
> > > > +       memory that is allocated or freed by the LMB module. This will
> > > > +       allow different modules that allocate memory or maintain a memory
> > > > +       map to have a synchronous view of available and allocated memory.
> > >
> > > This needs to be select'd when it's going to be used, opting out of
> > > making sure memory reservations are obeyed isn't a good idea.
> >
> > +1 which begs the question, do we need the config option at all ?
>
> The config symbol can be used for removing the code for platforms
> which do not support EFI ?

In that case, it shouldn't be a user config option. It should be a
Kconfig symbol that is automatically set if EFI_LOADER is enabled.

Regards
/Ilias
>
> -sughosh
>
> >
> > Cheers
> > /Ilias
> > >
> > > --
> > > Tom
Simon Glass July 23, 2024, 12:42 p.m. UTC | #5
Hi Sughosh,

On Mon, 22 Jul 2024 at 13:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > >
> > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > changes that might be done by the LMB module. This notification
> > > > mechanism can then be used to have a synchronous view of allocated and
> > > > free memory.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > > Changes since V1:
> > > > * Change the description to highlight only LMB notifications.
> > > > * Add a separate line for dependencies.
> > > >
> > > >  lib/Kconfig | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > index 7eea517b3b..b422183a0f 100644
> > > > --- a/lib/Kconfig
> > > > +++ b/lib/Kconfig
> > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > >  config LIB_UUID
> > > >       bool
> > > >
> > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > +     default y
> > > > +     help
> > > > +       Enable this option to get notification on any changes to the
> > > > +       memory that is allocated or freed by the LMB module. This will
> > > > +       allow different modules that allocate memory or maintain a memory
> > > > +       map to have a synchronous view of available and allocated memory.
> > >
> > > This needs to be select'd when it's going to be used, opting out of
> > > making sure memory reservations are obeyed isn't a good idea.
> >
> > +1 which begs the question, do we need the config option at all ?
>
> The config symbol can be used for removing the code for platforms
> which do not support EFI ?

I am still of the so-far firm opinion that this can be done once,
before booting, rather than maintaining two separate tables as we go.

Regards,
Simon
Tom Rini July 23, 2024, 2:20 p.m. UTC | #6
On Tue, Jul 23, 2024 at 01:42:59PM +0100, Simon Glass wrote:
> Hi Sughosh,
> 
> On Mon, 22 Jul 2024 at 13:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > > >
> > > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > > changes that might be done by the LMB module. This notification
> > > > > mechanism can then be used to have a synchronous view of allocated and
> > > > > free memory.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > > Changes since V1:
> > > > > * Change the description to highlight only LMB notifications.
> > > > > * Add a separate line for dependencies.
> > > > >
> > > > >  lib/Kconfig | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > >
> > > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > index 7eea517b3b..b422183a0f 100644
> > > > > --- a/lib/Kconfig
> > > > > +++ b/lib/Kconfig
> > > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > > >  config LIB_UUID
> > > > >       bool
> > > > >
> > > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > > +     default y
> > > > > +     help
> > > > > +       Enable this option to get notification on any changes to the
> > > > > +       memory that is allocated or freed by the LMB module. This will
> > > > > +       allow different modules that allocate memory or maintain a memory
> > > > > +       map to have a synchronous view of available and allocated memory.
> > > >
> > > > This needs to be select'd when it's going to be used, opting out of
> > > > making sure memory reservations are obeyed isn't a good idea.
> > >
> > > +1 which begs the question, do we need the config option at all ?
> >
> > The config symbol can be used for removing the code for platforms
> > which do not support EFI ?
> 
> I am still of the so-far firm opinion that this can be done once,
> before booting, rather than maintaining two separate tables as we go.

Did you see the part in the thread where he explained the multiple entry
points that would need to be kept in sync?
Simon Glass July 24, 2024, 2:37 p.m. UTC | #7
Hi Tom,

On Tue, 23 Jul 2024 at 08:20, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jul 23, 2024 at 01:42:59PM +0100, Simon Glass wrote:
> > Hi Sughosh,
> >
> > On Mon, 22 Jul 2024 at 13:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > > > >
> > > > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > > > changes that might be done by the LMB module. This notification
> > > > > > mechanism can then be used to have a synchronous view of allocated and
> > > > > > free memory.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > > Changes since V1:
> > > > > > * Change the description to highlight only LMB notifications.
> > > > > > * Add a separate line for dependencies.
> > > > > >
> > > > > >  lib/Kconfig | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > index 7eea517b3b..b422183a0f 100644
> > > > > > --- a/lib/Kconfig
> > > > > > +++ b/lib/Kconfig
> > > > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > > > >  config LIB_UUID
> > > > > >       bool
> > > > > >
> > > > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > > > +     default y
> > > > > > +     help
> > > > > > +       Enable this option to get notification on any changes to the
> > > > > > +       memory that is allocated or freed by the LMB module. This will
> > > > > > +       allow different modules that allocate memory or maintain a memory
> > > > > > +       map to have a synchronous view of available and allocated memory.
> > > > >
> > > > > This needs to be select'd when it's going to be used, opting out of
> > > > > making sure memory reservations are obeyed isn't a good idea.
> > > >
> > > > +1 which begs the question, do we need the config option at all ?
> > >
> > > The config symbol can be used for removing the code for platforms
> > > which do not support EFI ?
> >
> > I am still of the so-far firm opinion that this can be done once,
> > before booting, rather than maintaining two separate tables as we go.
>
> Did you see the part in the thread where he explained the multiple entry
> points that would need to be kept in sync?

Yes, but I'm not sure what they are, nor why a shared function cannot
be called twice from two different places.

Regards,
Simon
Tom Rini July 24, 2024, 2:52 p.m. UTC | #8
On Wed, Jul 24, 2024 at 08:37:14AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 23 Jul 2024 at 08:20, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Jul 23, 2024 at 01:42:59PM +0100, Simon Glass wrote:
> > > Hi Sughosh,
> > >
> > > On Mon, 22 Jul 2024 at 13:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > > > > >
> > > > > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > > > > changes that might be done by the LMB module. This notification
> > > > > > > mechanism can then be used to have a synchronous view of allocated and
> > > > > > > free memory.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > ---
> > > > > > > Changes since V1:
> > > > > > > * Change the description to highlight only LMB notifications.
> > > > > > > * Add a separate line for dependencies.
> > > > > > >
> > > > > > >  lib/Kconfig | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > >
> > > > > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > index 7eea517b3b..b422183a0f 100644
> > > > > > > --- a/lib/Kconfig
> > > > > > > +++ b/lib/Kconfig
> > > > > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > > > > >  config LIB_UUID
> > > > > > >       bool
> > > > > > >
> > > > > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > > > > +     default y
> > > > > > > +     help
> > > > > > > +       Enable this option to get notification on any changes to the
> > > > > > > +       memory that is allocated or freed by the LMB module. This will
> > > > > > > +       allow different modules that allocate memory or maintain a memory
> > > > > > > +       map to have a synchronous view of available and allocated memory.
> > > > > >
> > > > > > This needs to be select'd when it's going to be used, opting out of
> > > > > > making sure memory reservations are obeyed isn't a good idea.
> > > > >
> > > > > +1 which begs the question, do we need the config option at all ?
> > > >
> > > > The config symbol can be used for removing the code for platforms
> > > > which do not support EFI ?
> > >
> > > I am still of the so-far firm opinion that this can be done once,
> > > before booting, rather than maintaining two separate tables as we go.
> >
> > Did you see the part in the thread where he explained the multiple entry
> > points that would need to be kept in sync?
> 
> Yes, but I'm not sure what they are, nor why a shared function cannot
> be called twice from two different places.

Because we don't want to miss the third or fourth entry point down the
road. That's why going the other direction makes more sense I believe,
we won't have a future problem here because we designed with that in
mind.
Simon Glass July 24, 2024, 3:40 p.m. UTC | #9
Hi Tom,

On Wed, 24 Jul 2024 at 08:52, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jul 24, 2024 at 08:37:14AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 23 Jul 2024 at 08:20, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Tue, Jul 23, 2024 at 01:42:59PM +0100, Simon Glass wrote:
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 22 Jul 2024 at 13:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > > > > > >
> > > > > > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > > > > > changes that might be done by the LMB module. This notification
> > > > > > > > mechanism can then be used to have a synchronous view of allocated and
> > > > > > > > free memory.
> > > > > > > >
> > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > > ---
> > > > > > > > Changes since V1:
> > > > > > > > * Change the description to highlight only LMB notifications.
> > > > > > > > * Add a separate line for dependencies.
> > > > > > > >
> > > > > > > >  lib/Kconfig | 10 ++++++++++
> > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > index 7eea517b3b..b422183a0f 100644
> > > > > > > > --- a/lib/Kconfig
> > > > > > > > +++ b/lib/Kconfig
> > > > > > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > > > > > >  config LIB_UUID
> > > > > > > >       bool
> > > > > > > >
> > > > > > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > > > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > > > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > > > > > +     default y
> > > > > > > > +     help
> > > > > > > > +       Enable this option to get notification on any changes to the
> > > > > > > > +       memory that is allocated or freed by the LMB module. This will
> > > > > > > > +       allow different modules that allocate memory or maintain a memory
> > > > > > > > +       map to have a synchronous view of available and allocated memory.
> > > > > > >
> > > > > > > This needs to be select'd when it's going to be used, opting out of
> > > > > > > making sure memory reservations are obeyed isn't a good idea.
> > > > > >
> > > > > > +1 which begs the question, do we need the config option at all ?
> > > > >
> > > > > The config symbol can be used for removing the code for platforms
> > > > > which do not support EFI ?
> > > >
> > > > I am still of the so-far firm opinion that this can be done once,
> > > > before booting, rather than maintaining two separate tables as we go.
> > >
> > > Did you see the part in the thread where he explained the multiple entry
> > > points that would need to be kept in sync?
> >
> > Yes, but I'm not sure what they are, nor why a shared function cannot
> > be called twice from two different places.
>
> Because we don't want to miss the third or fourth entry point down the
> road. That's why going the other direction makes more sense I believe,
> we won't have a future problem here because we designed with that in
> mind.

You might be right, but I don't even know what we are referring to
here...what are the two 'entry points'?

My current belief is that we can set up the EFI memory table before
booting, with a single pass through the table. Maintaining two
independent tables as we go doesn't seem very useful. It is harder to
test too.

Regards,
Simon
Tom Rini July 24, 2024, 10:47 p.m. UTC | #10
On Wed, Jul 24, 2024 at 09:40:35AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 24 Jul 2024 at 08:52, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jul 24, 2024 at 08:37:14AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Tue, 23 Jul 2024 at 08:20, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Tue, Jul 23, 2024 at 01:42:59PM +0100, Simon Glass wrote:
> > > > > Hi Sughosh,
> > > > >
> > > > > On Mon, 22 Jul 2024 at 13:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > > > > > > >
> > > > > > > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > > > > > > changes that might be done by the LMB module. This notification
> > > > > > > > > mechanism can then be used to have a synchronous view of allocated and
> > > > > > > > > free memory.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > > > ---
> > > > > > > > > Changes since V1:
> > > > > > > > > * Change the description to highlight only LMB notifications.
> > > > > > > > > * Add a separate line for dependencies.
> > > > > > > > >
> > > > > > > > >  lib/Kconfig | 10 ++++++++++
> > > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > > index 7eea517b3b..b422183a0f 100644
> > > > > > > > > --- a/lib/Kconfig
> > > > > > > > > +++ b/lib/Kconfig
> > > > > > > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > > > > > > >  config LIB_UUID
> > > > > > > > >       bool
> > > > > > > > >
> > > > > > > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > > > > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > > > > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > > > > > > +     default y
> > > > > > > > > +     help
> > > > > > > > > +       Enable this option to get notification on any changes to the
> > > > > > > > > +       memory that is allocated or freed by the LMB module. This will
> > > > > > > > > +       allow different modules that allocate memory or maintain a memory
> > > > > > > > > +       map to have a synchronous view of available and allocated memory.
> > > > > > > >
> > > > > > > > This needs to be select'd when it's going to be used, opting out of
> > > > > > > > making sure memory reservations are obeyed isn't a good idea.
> > > > > > >
> > > > > > > +1 which begs the question, do we need the config option at all ?
> > > > > >
> > > > > > The config symbol can be used for removing the code for platforms
> > > > > > which do not support EFI ?
> > > > >
> > > > > I am still of the so-far firm opinion that this can be done once,
> > > > > before booting, rather than maintaining two separate tables as we go.
> > > >
> > > > Did you see the part in the thread where he explained the multiple entry
> > > > points that would need to be kept in sync?
> > >
> > > Yes, but I'm not sure what they are, nor why a shared function cannot
> > > be called twice from two different places.
> >
> > Because we don't want to miss the third or fourth entry point down the
> > road. That's why going the other direction makes more sense I believe,
> > we won't have a future problem here because we designed with that in
> > mind.
> 
> You might be right, but I don't even know what we are referring to
> here...what are the two 'entry points'?

I would have to refer back to Sughosh's explanation to repeat it, sorry.

> My current belief is that we can set up the EFI memory table before
> booting, with a single pass through the table. Maintaining two
> independent tables as we go doesn't seem very useful. It is harder to
> test too.

You seem to be fixated on "booting" when at least part of the issue is
re-entering the EFI_LOADER. And since EFI_LOADER needs X/Y/Z as well, we
had much earlier rejected "make EFI_LOADER's memory model what
everything else uses".
Simon Glass July 25, 2024, 11:32 p.m. UTC | #11
Hi Tom,

On Wed, 24 Jul 2024 at 16:47, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jul 24, 2024 at 09:40:35AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 24 Jul 2024 at 08:52, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Jul 24, 2024 at 08:37:14AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 23 Jul 2024 at 08:20, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Tue, Jul 23, 2024 at 01:42:59PM +0100, Simon Glass wrote:
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Mon, 22 Jul 2024 at 13:59, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > On Mon, 22 Jul 2024 at 18:00, Ilias Apalodimas
> > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, 5 Jul 2024 at 22:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jul 04, 2024 at 01:05:34PM +0530, Sughosh Ganu wrote:
> > > > > > > > >
> > > > > > > > > > Add a Kconfig symbol to enable getting updates on any memory map
> > > > > > > > > > changes that might be done by the LMB module. This notification
> > > > > > > > > > mechanism can then be used to have a synchronous view of allocated and
> > > > > > > > > > free memory.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > > Changes since V1:
> > > > > > > > > > * Change the description to highlight only LMB notifications.
> > > > > > > > > > * Add a separate line for dependencies.
> > > > > > > > > >
> > > > > > > > > >  lib/Kconfig | 10 ++++++++++
> > > > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/lib/Kconfig b/lib/Kconfig
> > > > > > > > > > index 7eea517b3b..b422183a0f 100644
> > > > > > > > > > --- a/lib/Kconfig
> > > > > > > > > > +++ b/lib/Kconfig
> > > > > > > > > > @@ -74,6 +74,16 @@ config HAVE_PRIVATE_LIBGCC
> > > > > > > > > >  config LIB_UUID
> > > > > > > > > >       bool
> > > > > > > > > >
> > > > > > > > > > +config MEM_MAP_UPDATE_NOTIFY
> > > > > > > > > > +     bool "Get notified of any changes to the LMB memory map"
> > > > > > > > > > +     depends on EVENT && LMB && EFI_LOADER
> > > > > > > > > > +     default y
> > > > > > > > > > +     help
> > > > > > > > > > +       Enable this option to get notification on any changes to the
> > > > > > > > > > +       memory that is allocated or freed by the LMB module. This will
> > > > > > > > > > +       allow different modules that allocate memory or maintain a memory
> > > > > > > > > > +       map to have a synchronous view of available and allocated memory.
> > > > > > > > >
> > > > > > > > > This needs to be select'd when it's going to be used, opting out of
> > > > > > > > > making sure memory reservations are obeyed isn't a good idea.
> > > > > > > >
> > > > > > > > +1 which begs the question, do we need the config option at all ?
> > > > > > >
> > > > > > > The config symbol can be used for removing the code for platforms
> > > > > > > which do not support EFI ?
> > > > > >
> > > > > > I am still of the so-far firm opinion that this can be done once,
> > > > > > before booting, rather than maintaining two separate tables as we go.
> > > > >
> > > > > Did you see the part in the thread where he explained the multiple entry
> > > > > points that would need to be kept in sync?
> > > >
> > > > Yes, but I'm not sure what they are, nor why a shared function cannot
> > > > be called twice from two different places.
> > >
> > > Because we don't want to miss the third or fourth entry point down the
> > > road. That's why going the other direction makes more sense I believe,
> > > we won't have a future problem here because we designed with that in
> > > mind.
> >
> > You might be right, but I don't even know what we are referring to
> > here...what are the two 'entry points'?
>
> I would have to refer back to Sughosh's explanation to repeat it, sorry.
>
> > My current belief is that we can set up the EFI memory table before
> > booting, with a single pass through the table. Maintaining two
> > independent tables as we go doesn't seem very useful. It is harder to
> > test too.
>
> You seem to be fixated on "booting" when at least part of the issue is
> re-entering the EFI_LOADER. And since EFI_LOADER needs X/Y/Z as well, we
> had much earlier rejected "make EFI_LOADER's memory model what
> everything else uses".

I wouldn't say I am fixated. EFI_LOADER's purpose is to boot an OS.

What do you mean by re-entering the EFI_LOADER? Do you mean running
one app, then coming back to U-Boot, then running another? I am not
suggesting we reinit EFI in that case, if that is what you are worried
about.

I sent a separate series to show some of the issues I've been
concerned about for a few years now. Perhaps we should schedule a
discussion to talk all this through one day?

Regards,
Simon
diff mbox series

Patch

diff --git a/lib/Kconfig b/lib/Kconfig
index 7eea517b3b..b422183a0f 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -74,6 +74,16 @@  config HAVE_PRIVATE_LIBGCC
 config LIB_UUID
 	bool
 
+config MEM_MAP_UPDATE_NOTIFY
+	bool "Get notified of any changes to the LMB memory map"
+	depends on EVENT && LMB && EFI_LOADER
+	default y
+	help
+	  Enable this option to get notification on any changes to the
+	  memory that is allocated or freed by the LMB module. This will
+	  allow different modules that allocate memory or maintain a memory
+	  map to have a synchronous view of available and allocated memory.
+
 config RANDOM_UUID
 	bool "GPT Random UUID generation"
 	select LIB_UUID