[edk2-devel] [PATCH v3 1/1] OvmfPkg:Add variable store is full debug info

[edk2-devel] [PATCH v3 1/1] OvmfPkg:Add variable store is full debug info

[Zhenyu Zhang]

From: "Zhenyu Zhang" <zhenyzha@redhat.com>

We observed that EDK2 hits an ASSERT (Out of Resources) when
booting with a full variable store. The message provided in
this case is not helpful for non-experts.
Add debug information to help users understand what caused
the assertion.

 Actual results:
 RecordVarErrorFlag (0xEF) 9A144FE2A47E:937FE521-95AE-4D1A-8929-
 48BCD90AD31A - 0x00000003 - 0x92
 CommonVariableSpace = 0x3FF9C - CommonVariableTotalSize = 0x3FF98

 Synchronous Exception at 0x000000023CA60374
 ......
 ASSERT_EFI_ERROR (Status = Out of Resources)
 ASSERT /builddir/build/BUILD/edk2-ba91d0292e59/OvmfPkg/Library/
 PlatformBootManagerLib/BdsPlatform.c(142): !(((INTN)(RETURN_
 STATUS)(Status)) < 0)

Cc: Oliver Steffen <osteffen@redhat.com>
Cc: Gerd Hoffmann <ghoffman@redhat.com>
Cc: Yao Jiewen <jiewen.yao@intel.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Signed-off-by: Zhenyu Zhang <zhenyzha@redhat.com>
---
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 8dc2bbf97371..fc2d64d69207 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -139,6 +139,9 @@ PlatformRegisterFvBootOption (
 
   if (OptionIndex == -1) {
     Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
+    if (Status == EFI_OUT_OF_RESOURCES) {
+      DEBUG ((DEBUG_ERROR, "ERROR: Variable store is full.\n"));
+    }
     ASSERT_EFI_ERROR (Status);
   }
 
-- 
2.39.3



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108474): https://edk2.groups.io/g/devel/message/108474
Mute This Topic: https://groups.io/mt/101285762/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg:Add variable store is full debug info

[Ard Biesheuvel]

On Mon, 11 Sept 2023 at 04:47, Zhenyu Zhang <zhenyzha@redhat.com> wrote:
>
> From: "Zhenyu Zhang" <zhenyzha@redhat.com>
>
> We observed that EDK2 hits an ASSERT (Out of Resources) when
> booting with a full variable store. The message provided in
> this case is not helpful for non-experts.
> Add debug information to help users understand what caused
> the assertion.
>
>  Actual results:
>  RecordVarErrorFlag (0xEF) 9A144FE2A47E:937FE521-95AE-4D1A-8929-
>  48BCD90AD31A - 0x00000003 - 0x92
>  CommonVariableSpace = 0x3FF9C - CommonVariableTotalSize = 0x3FF98
>
>  Synchronous Exception at 0x000000023CA60374
>  ......
>  ASSERT_EFI_ERROR (Status = Out of Resources)
>  ASSERT /builddir/build/BUILD/edk2-ba91d0292e59/OvmfPkg/Library/
>  PlatformBootManagerLib/BdsPlatform.c(142): !(((INTN)(RETURN_
>  STATUS)(Status)) < 0)
>
> Cc: Oliver Steffen <osteffen@redhat.com>
> Cc: Gerd Hoffmann <ghoffman@redhat.com>
> Cc: Yao Jiewen <jiewen.yao@intel.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Signed-off-by: Zhenyu Zhang <zhenyzha@redhat.com>
> ---
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 8dc2bbf97371..fc2d64d69207 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -139,6 +139,9 @@ PlatformRegisterFvBootOption (
>
>    if (OptionIndex == -1) {
>      Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
> +    if (Status == EFI_OUT_OF_RESOURCES) {
> +      DEBUG ((DEBUG_ERROR, "ERROR: Variable store is full.\n"));

I don't think a full variable store is the only reason we might end up
with EFI_OUT_OF_RESOURCES here, so this message could be misleading.

Given that this is a DEBUG build, I'd expect the developer to be able
to infer from the context that this might be either an out of memory
or an out of flash space condition, and the extra DEBUG message is
kind of redundant.

OTOH, running out of flash space is a serious condition, so I wouldn't
object to adding better diagnostics for this - they just belong
somewhere else (i.e., in the SetVariable() code)


> +    }
>      ASSERT_EFI_ERROR (Status);
>    }
>
> --
> 2.39.3
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108573): https://edk2.groups.io/g/devel/message/108573
Mute This Topic: https://groups.io/mt/101285762/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg:Add variable store is full debug info

[Zhenyu Zhang]

> I don't think a full variable store is the only reason we might end up
> with EFI_OUT_OF_RESOURCES here, so this message could be misleading.
>
> Given that this is a DEBUG build, I'd expect the developer to be able
> to infer from the context that this might be either an out of memory
> or an out of flash space condition, and the extra DEBUG message is
> kind of redundant.

Sorry for the late reply, you are right.
Yes, there may be different situations here.
It will be clearer if we can distinguish between out of memory
or out of flash space.

> OTOH, running out of flash space is a serious condition, so I wouldn't
> object to adding better diagnostics for this - they just belong
> somewhere else (i.e., in the SetVariable() code)

Sorry in SetVariable(), how can we tell that there is insufficient
flash storage?
I noticed there is a similar method in NorFlashKvmtool.c,
but I'm not sure it's what you're talking about.

  if (FlashRegion > (FlashDevice->DeviceBaseAddress + FlashDevice->Size)) {
    DEBUG ((DEBUG_ERROR, "Insufficient flash storage size\n"));
    return EFI_OUT_OF_RESOURCES;
  }


On Wed, Sep 13, 2023 at 4:54 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 11 Sept 2023 at 04:47, Zhenyu Zhang <zhenyzha@redhat.com> wrote:
> >
> > From: "Zhenyu Zhang" <zhenyzha@redhat.com>
> >
> > We observed that EDK2 hits an ASSERT (Out of Resources) when
> > booting with a full variable store. The message provided in
> > this case is not helpful for non-experts.
> > Add debug information to help users understand what caused
> > the assertion.
> >
> >  Actual results:
> >  RecordVarErrorFlag (0xEF) 9A144FE2A47E:937FE521-95AE-4D1A-8929-
> >  48BCD90AD31A - 0x00000003 - 0x92
> >  CommonVariableSpace = 0x3FF9C - CommonVariableTotalSize = 0x3FF98
> >
> >  Synchronous Exception at 0x000000023CA60374
> >  ......
> >  ASSERT_EFI_ERROR (Status = Out of Resources)
> >  ASSERT /builddir/build/BUILD/edk2-ba91d0292e59/OvmfPkg/Library/
> >  PlatformBootManagerLib/BdsPlatform.c(142): !(((INTN)(RETURN_
> >  STATUS)(Status)) < 0)
> >
> > Cc: Oliver Steffen <osteffen@redhat.com>
> > Cc: Gerd Hoffmann <ghoffman@redhat.com>
> > Cc: Yao Jiewen <jiewen.yao@intel.com>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Julien Grall <julien@xen.org>
> > Signed-off-by: Zhenyu Zhang <zhenyzha@redhat.com>
> > ---
> >  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> > index 8dc2bbf97371..fc2d64d69207 100644
> > --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> > +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> > @@ -139,6 +139,9 @@ PlatformRegisterFvBootOption (
> >
> >    if (OptionIndex == -1) {
> >      Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
> > +    if (Status == EFI_OUT_OF_RESOURCES) {
> > +      DEBUG ((DEBUG_ERROR, "ERROR: Variable store is full.\n"));
>
> I don't think a full variable store is the only reason we might end up
> with EFI_OUT_OF_RESOURCES here, so this message could be misleading.
>
> Given that this is a DEBUG build, I'd expect the developer to be able
> to infer from the context that this might be either an out of memory
> or an out of flash space condition, and the extra DEBUG message is
> kind of redundant.
>
> OTOH, running out of flash space is a serious condition, so I wouldn't
> object to adding better diagnostics for this - they just belong
> somewhere else (i.e., in the SetVariable() code)
>
>
> > +    }
> >      ASSERT_EFI_ERROR (Status);
> >    }
> >
> > --
> > 2.39.3
> >
>
>
> 
>
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108903): https://edk2.groups.io/g/devel/message/108903
Mute This Topic: https://groups.io/mt/101285762/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-