Re: [PATCH edk2-platforms v4 12/24] Silicon/NXP: Move RAM retrieval from SocLib

["Leif Lindholm" <leif@nuviainc.com>]

Hi Pankaj,

On Fri, May 08, 2020 at 05:31:53 +0000, Pankaj Bansal (OSS) wrote:
> > > > > -      {
> > > > > -        Found = TRUE;
> > > > > -        break;
> > > > > -      }
> > > > > -      NextHob.Raw = GET_NEXT_HOB (NextHob);
> > > > > +  Status = GetDramRegionsInfo (DramRegions, ARRAY_SIZE
> > (DramRegions));
> > > > > +  ASSERT_EFI_ERROR (Status);
> > > >
> > > > Slightly concerned here because we end up with a variable Status that
> > > > is only *used* in DEBUG builds. That could lead to toolchain warnings.
> > >
> > > I don't think this would cause build warnings in RELEASE builds. I have tested it.
> > > Also the Status is frequently being handled in this way in other places in edk2:
> > >
> > >
> > https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/PlatformPei/
> > PlatformPeim.c#L90
> > 
> > The example you point to sets Status, overwrites it (once or twice
> > depending on conditionals), then returns it.
> > This patch in its current form sets Status, accesses it only in DEBUG
> > builds and does not return it.
> 
> Ok. Then I can return status at the end of this function (MemoryPeim).
> I still can't bring myself to just ignore the critical error status from a function.

If it is considered critical, it needs to be handled. If it is not
critical (but would, for example, be useful to log and tell someone
about) it can be explicitly cast away until such a time as the full
error reporting can be iplemented. That should not be confused for
ignoring it.

> > > > And I was completely serious last time around, I am OK with the return
> > > > value being cast away explicitly. What I meant with that is:
> > > >   (VOID)GetDramRegionsInfo (DramRegions, ARRAY_SIZE (DramRegions));
> > >
> > > I agreed with your suggestion to return EFI_BUFFER_TOO_SMALL from
> > GetDramRegionsInfo,
> > > But If we discard the return value it means we are OK with some RAM not
> > being reported to UEFI firmware
> > > and subsequently to OS. Isn't this a critical error ?
> > 
> > ASSERTs are only triggered in DEBUG builds, and send the processor
> > into WFI.
> > 
> > If it is a critical error (is it critical if you have found some RAM,
> > but been unable to fully reconcile all of the RAM in the system?), it
> > should do more than that.
> > 
> > I am all for properly handling that situation, but this patch has
> > never done that. Feel free to rework before submitting v5, or leave it
> > until (if) adding ACPI support and report the condition properly
> > through BERT.
> 
> I have referred reference platform lib https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c#L83
> As per this lib, I have added ASSERTS in all the scenarios, which
> are critical.

Adding the ASSERT is good hygiene, I have no objection to that.

> I thought ASSERT means we will halt the program execution regardless
> of DEBUG or RELEASE build.

Dying without attempted recovery is not an appropriate response for a
RELEASE build. Especially in the case where we have successfully
located memory to use, but have not fully managed to reconcile all
entries.

> Can you point me to a reference platform which handles these errors
> using ACPI BERT methods ?

Sadly, no. The Hisilicon 1620 platform implements BERT support as part
of its APEI driver, but it does not appear to actually log anything.

> I see that in DebugLib.h : 
> 
>   Note that a reserved macro named MDEPKG_NDEBUG is introduced for the intention
>   of size reduction when compiler optimization is disabled. If MDEPKG_NDEBUG is
>   defined, then debug and assert related macros wrapped by it are the NULL implementations.
> 
> We have NOT defined MDEPKG_NDEBUG in our platforms for RELEASE or DEBUG builds.
> Up until we have not implemented the ACPI BERT methods, can we keep
> it this way to avoid masking the errors?

It would be unfortunate if the NXP platforms behaved differently from
the majority of other EDK2 platforms. It would also mean that if
someone was to create a derivative platform (which I expect is what
NXP wants), then various drivers in this reference implementation
would start failing to build if *they* added MDEPKG_NDEBUG to *their*
.dsc.

So it would be much preferred if you could add the MDEPKG_NDEBUG
stanzas to RELEASE CC_FLAGS.

But thank you for explaining why I wasn't seeing the "set but not
used" warning :)

Best Regards,

Leif