| Message ID | 20240905082811.1585467-3-sughosh.ganu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make EFI memory allocations synchronous with LMB | expand |
On 9/5/24 10:27, Sughosh Ganu wrote: > Add a flag LMB_NONOTIFY that can be passed to the LMB API's for > reserving memory. This will then result in no notification being sent > from the LMB module for the changes to the LMB's memory map. You seem to be using this in patch 3 and 7. Please, describe in this patch why you want to be able to suppress notification. In the EFI context we should use LMB notification to notify the EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event. See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI specification. Best regards Heinrich > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > include/lmb.h | 1 + > lib/lmb.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/lmb.h b/include/lmb.h > index 45a06c3b99..ffba7e2889 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -23,6 +23,7 @@ enum lmb_flags { > LMB_NONE = 0, > LMB_NOMAP = BIT(1), > LMB_NOOVERWRITE = BIT(2), > + LMB_NONOTIFY = BIT(3), > }; > > /** > diff --git a/lib/lmb.c b/lib/lmb.c > index da6a1595cc..419b31a651 100644 > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -30,7 +30,7 @@ static struct lmb lmb; > static void lmb_print_region_flags(enum lmb_flags flags) > { > u64 bitpos; > - const char *flag_str[] = { "none", "no-map", "no-overwrite" }; > + const char *flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" }; > > do { > bitpos = flags ? fls(flags) - 1 : 0;
On Sat, 14 Sept 2024 at 20:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 9/5/24 10:27, Sughosh Ganu wrote: > > Add a flag LMB_NONOTIFY that can be passed to the LMB API's for > > reserving memory. This will then result in no notification being sent > > from the LMB module for the changes to the LMB's memory map. > > You seem to be using this in patch 3 and 7. > > Please, describe in this patch why you want to be able to suppress > notification. Will add the reasoning behind this flag in the commit message. > > In the EFI context we should use LMB notification to notify the > EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event. > > See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI > specification. So, do you want me to use the EFI event signaling mechanism for this purpose ? Is my understanding correct ? If so, this will mean that we have an event notification specifically for EFI, and there might be one needed for any other consumers of this event. Currently there aren't any other consumers of the LMB memory map change event other than EFI, but using the U-Boot's event notification mechanism means that the same notification mechanism can be used if there were any additional consumers of this event in the future. In that case, we would have two separate event notifications, one for EFI, and one for non-EFI consumers. -sughosh > > Best regards > > Heinrich > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > include/lmb.h | 1 + > > lib/lmb.c | 2 +- > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/lmb.h b/include/lmb.h > > index 45a06c3b99..ffba7e2889 100644 > > --- a/include/lmb.h > > +++ b/include/lmb.h > > @@ -23,6 +23,7 @@ enum lmb_flags { > > LMB_NONE = 0, > > LMB_NOMAP = BIT(1), > > LMB_NOOVERWRITE = BIT(2), > > + LMB_NONOTIFY = BIT(3), > > }; > > > > /** > > diff --git a/lib/lmb.c b/lib/lmb.c > > index da6a1595cc..419b31a651 100644 > > --- a/lib/lmb.c > > +++ b/lib/lmb.c > > @@ -30,7 +30,7 @@ static struct lmb lmb; > > static void lmb_print_region_flags(enum lmb_flags flags) > > { > > u64 bitpos; > > - const char *flag_str[] = { "none", "no-map", "no-overwrite" }; > > + const char *flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" }; > > > > do { > > bitpos = flags ? fls(flags) - 1 : 0; >
Hi, On Tue, 17 Sept 2024 at 13:55, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Sat, 14 Sept 2024 at 20:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > Add a flag LMB_NONOTIFY that can be passed to the LMB API's for > > > reserving memory. This will then result in no notification being sent > > > from the LMB module for the changes to the LMB's memory map. > > > > You seem to be using this in patch 3 and 7. > > > > Please, describe in this patch why you want to be able to suppress > > notification. > > Will add the reasoning behind this flag in the commit message. > > > > > In the EFI context we should use LMB notification to notify the > > EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event. > > > > See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI > > specification. > > So, do you want me to use the EFI event signaling mechanism for this > purpose ? Is my understanding correct ? If so, this will mean that we > have an event notification specifically for EFI, and there might be > one needed for any other consumers of this event. Currently there > aren't any other consumers of the LMB memory map change event other > than EFI, but using the U-Boot's event notification mechanism means > that the same notification mechanism can be used if there were any > additional consumers of this event in the future. In that case, we > would have two separate event notifications, one for EFI, and one for > non-EFI consumers. As I have previously said, none of this is necessary. Essentially all of the EFI setup that is done in U-Boot can be delayed until we are actually starting an EFI app. The current approach of keeping parallel EFI tables everywhere is causing much confusion. For EFI, it should be enough to read the lmb tables at the end and add whatever parallel tables are needed to boot the app. We should not need to keep things in sync through the life of U-Boot, since: 1. EFI pool-allocations should use malloc() until the app starts 2. EFI page allocations should not be allowed until the app starts This whole area needs a healthy dose of 'keep it simple'. Regards, Simon
On Thu, 19 Sept 2024 at 19:42, Simon Glass <sjg@chromium.org> wrote: > > Hi, > > On Tue, 17 Sept 2024 at 13:55, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Sat, 14 Sept 2024 at 20:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > Add a flag LMB_NONOTIFY that can be passed to the LMB API's for > > > > reserving memory. This will then result in no notification being sent > > > > from the LMB module for the changes to the LMB's memory map. > > > > > > You seem to be using this in patch 3 and 7. > > > > > > Please, describe in this patch why you want to be able to suppress > > > notification. > > > > Will add the reasoning behind this flag in the commit message. > > > > > > > > In the EFI context we should use LMB notification to notify the > > > EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event. > > > > > > See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI > > > specification. > > > > So, do you want me to use the EFI event signaling mechanism for this > > purpose ? Is my understanding correct ? If so, this will mean that we > > have an event notification specifically for EFI, and there might be > > one needed for any other consumers of this event. Currently there > > aren't any other consumers of the LMB memory map change event other > > than EFI, but using the U-Boot's event notification mechanism means > > that the same notification mechanism can be used if there were any > > additional consumers of this event in the future. In that case, we > > would have two separate event notifications, one for EFI, and one for > > non-EFI consumers. > > As I have previously said, none of this is necessary. > > Essentially all of the EFI setup that is done in U-Boot can be delayed > until we are actually starting an EFI app. The current approach of > keeping parallel EFI tables everywhere is causing much confusion. > > For EFI, it should be enough to read the lmb tables at the end and add > whatever parallel tables are needed to boot the app. We should not > need to keep things in sync through the life of U-Boot, since: > > 1. EFI pool-allocations should use malloc() until the app starts > 2. EFI page allocations should not be allowed until the app starts > > This whole area needs a healthy dose of 'keep it simple'. Except, the current implementation proposed by the patch *is* actually much more simpler than syncing the maps when the app starts. Because there is another scenario when the map needs to be synced, when the user issues a 'efidebug memmap' command, which dumps the efi memory map. Syncing the map from lmb means that the efi memory map first has to be generated afresh. And that is much more complicated than the method incorporated in the patch series. Apart from the ram/conventional memory type, we also need to include other memory types that might have been added to the memory map. So, this would mean, create a new memory map, add entries for conventional memory as have been obtained from lmb followed by other entries, and then discard the old memory map. And this has to be carried out every time a need for a memory map update is needed. -sughosh > > Regards, > Simon
Hi Sughosh, On Fri, 20 Sept 2024 at 06:33, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Thu, 19 Sept 2024 at 19:42, Simon Glass <sjg@chromium.org> wrote: > > > > Hi, > > > > On Tue, 17 Sept 2024 at 13:55, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Sat, 14 Sept 2024 at 20:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > Add a flag LMB_NONOTIFY that can be passed to the LMB API's for > > > > > reserving memory. This will then result in no notification being sent > > > > > from the LMB module for the changes to the LMB's memory map. > > > > > > > > You seem to be using this in patch 3 and 7. > > > > > > > > Please, describe in this patch why you want to be able to suppress > > > > notification. > > > > > > Will add the reasoning behind this flag in the commit message. > > > > > > > > > > > In the EFI context we should use LMB notification to notify the > > > > EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event. > > > > > > > > See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI > > > > specification. > > > > > > So, do you want me to use the EFI event signaling mechanism for this > > > purpose ? Is my understanding correct ? If so, this will mean that we > > > have an event notification specifically for EFI, and there might be > > > one needed for any other consumers of this event. Currently there > > > aren't any other consumers of the LMB memory map change event other > > > than EFI, but using the U-Boot's event notification mechanism means > > > that the same notification mechanism can be used if there were any > > > additional consumers of this event in the future. In that case, we > > > would have two separate event notifications, one for EFI, and one for > > > non-EFI consumers. > > > > As I have previously said, none of this is necessary. > > > > Essentially all of the EFI setup that is done in U-Boot can be delayed > > until we are actually starting an EFI app. The current approach of > > keeping parallel EFI tables everywhere is causing much confusion. > > > > For EFI, it should be enough to read the lmb tables at the end and add > > whatever parallel tables are needed to boot the app. We should not > > need to keep things in sync through the life of U-Boot, since: > > > > 1. EFI pool-allocations should use malloc() until the app starts > > 2. EFI page allocations should not be allowed until the app starts > > > > This whole area needs a healthy dose of 'keep it simple'. > > Except, the current implementation proposed by the patch *is* actually > much more simpler than syncing the maps when the app starts. Because > there is another scenario when the map needs to be synced, when the > user issues a 'efidebug memmap' command, which dumps the efi memory > map. > > Syncing the map from lmb means that the efi memory map first has to be > generated afresh. And that is much more complicated than the method > incorporated in the patch series. Apart from the ram/conventional > memory type, we also need to include other memory types that might > have been added to the memory map. So, this would mean, create a new > memory map, add entries for conventional memory as have been obtained > from lmb followed by other entries, and then discard the old memory > map. And this has to be carried out every time a need for a memory map > update is needed. We are just not on the same page here. But I've replied to this point on your other email. Also please understand that my objective is actually to not do EFI allocations until just before the EFI app actually starts (when we set up the EFI memory map). Until then, I believe malloc() suffices. Regards, Simon
Hi Simon, On Thu, 19 Sept 2024 at 17:12, Simon Glass <sjg@chromium.org> wrote: > > Hi, > > On Tue, 17 Sept 2024 at 13:55, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Sat, 14 Sept 2024 at 20:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > Add a flag LMB_NONOTIFY that can be passed to the LMB API's for > > > > reserving memory. This will then result in no notification being sent > > > > from the LMB module for the changes to the LMB's memory map. > > > > > > You seem to be using this in patch 3 and 7. > > > > > > Please, describe in this patch why you want to be able to suppress > > > notification. > > > > Will add the reasoning behind this flag in the commit message. > > > > > > > > In the EFI context we should use LMB notification to notify the > > > EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event. > > > > > > See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI > > > specification. > > > > So, do you want me to use the EFI event signaling mechanism for this > > purpose ? Is my understanding correct ? If so, this will mean that we > > have an event notification specifically for EFI, and there might be > > one needed for any other consumers of this event. Currently there > > aren't any other consumers of the LMB memory map change event other > > than EFI, but using the U-Boot's event notification mechanism means > > that the same notification mechanism can be used if there were any > > additional consumers of this event in the future. In that case, we > > would have two separate event notifications, one for EFI, and one for > > non-EFI consumers. > > As I have previously said, none of this is necessary. > > Essentially all of the EFI setup that is done in U-Boot can be delayed > until we are actually starting an EFI app. Can you explain how you plan to deal with EFI variables, the TPM eventlog, measuring events when tables are added, capsules updates etc etc, which expect certain EFI services to be up and running? > The current approach of > keeping parallel EFI tables everywhere is causing much confusion. Apart from all of the above the EFI app can return. Which makes all of the above just a burden > > For EFI, it should be enough to read the lmb tables at the end and add > whatever parallel tables are needed to boot the app. Yes apart from the fact that LMB has no idea about memory types or permissions. > We should not > need to keep things in sync through the life of U-Boot, since: > > 1. EFI pool-allocations should use malloc() until the app starts > 2. EFI page allocations should not be allowed until the app starts > > This whole area needs a healthy dose of 'keep it simple'. > > Regards, > Simon Thanks /Ilias
Hi Ilias, On Fri, 27 Sept 2024 at 05:08, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Thu, 19 Sept 2024 at 17:12, Simon Glass <sjg@chromium.org> wrote: > > > > Hi, > > > > On Tue, 17 Sept 2024 at 13:55, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Sat, 14 Sept 2024 at 20:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > On 9/5/24 10:27, Sughosh Ganu wrote: > > > > > Add a flag LMB_NONOTIFY that can be passed to the LMB API's for > > > > > reserving memory. This will then result in no notification being sent > > > > > from the LMB module for the changes to the LMB's memory map. > > > > > > > > You seem to be using this in patch 3 and 7. > > > > > > > > Please, describe in this patch why you want to be able to suppress > > > > notification. > > > > > > Will add the reasoning behind this flag in the commit message. > > > > > > > > > > > In the EFI context we should use LMB notification to notify the > > > > EFI_EVENT_GROUP_MEMORY_MAP_CHANGE event. > > > > > > > > See chapter 7.1.2 EFI_BOOT_SERVICES.CreateEventEx() in the UEFI > > > > specification. > > > > > > So, do you want me to use the EFI event signaling mechanism for this > > > purpose ? Is my understanding correct ? If so, this will mean that we > > > have an event notification specifically for EFI, and there might be > > > one needed for any other consumers of this event. Currently there > > > aren't any other consumers of the LMB memory map change event other > > > than EFI, but using the U-Boot's event notification mechanism means > > > that the same notification mechanism can be used if there were any > > > additional consumers of this event in the future. In that case, we > > > would have two separate event notifications, one for EFI, and one for > > > non-EFI consumers. > > > > As I have previously said, none of this is necessary. > > > > Essentially all of the EFI setup that is done in U-Boot can be delayed > > until we are actually starting an EFI app. > > Can you explain how you plan to deal with EFI variables, the TPM > eventlog, measuring events when tables are added, capsules updates etc > etc, which expect certain EFI services to be up and running? Yes, sure. Accessing EFI variables is going to require EFI to be inited, I assume. For capsule updates, I believe that is triggered on boot, so again it would need EFI services. It's fine to use EFI when it is needed. But this memory-map thing has really got out of hand. BTW we need TPM eventlog and measuring for any type of boot so it needs to work without EFI. > > > The current approach of > > keeping parallel EFI tables everywhere is causing much confusion. > > Apart from all of the above the EFI app can return. Which makes all of > the above just a burden It's fine if it returns, isn't it? What is the burden? > > > > > For EFI, it should be enough to read the lmb tables at the end and add > > whatever parallel tables are needed to boot the app. > > Yes apart from the fact that LMB has no idea about memory types or permissions. Neither should it. It is for laying out images in memory. We need to maintain some separation of concerns here, or we'll end up with a complex mess. Memory types are really an EFI construct which should be easy enough to add statically - e.g. U-Boot code is EFI_BOOT_SERVICES_CODE. Memory permissions are known based on the memory areas, so I don't think lmb needs to know about that. > > > We should not > > need to keep things in sync through the life of U-Boot, since: > > > > 1. EFI pool-allocations should use malloc() until the app starts > > 2. EFI page allocations should not be allowed until the app starts > > > > This whole area needs a healthy dose of 'keep it simple'. Regards, Simon
diff --git a/include/lmb.h b/include/lmb.h index 45a06c3b99..ffba7e2889 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -23,6 +23,7 @@ enum lmb_flags { LMB_NONE = 0, LMB_NOMAP = BIT(1), LMB_NOOVERWRITE = BIT(2), + LMB_NONOTIFY = BIT(3), }; /** diff --git a/lib/lmb.c b/lib/lmb.c index da6a1595cc..419b31a651 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -30,7 +30,7 @@ static struct lmb lmb; static void lmb_print_region_flags(enum lmb_flags flags) { u64 bitpos; - const char *flag_str[] = { "none", "no-map", "no-overwrite" }; + const char *flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" }; do { bitpos = flags ? fls(flags) - 1 : 0;
Add a flag LMB_NONOTIFY that can be passed to the LMB API's for reserving memory. This will then result in no notification being sent from the LMB module for the changes to the LMB's memory map. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- include/lmb.h | 1 + lib/lmb.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)