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

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

> 
> On Thu, May 07, 2020 at 07:28:30 +0000, Pankaj Bansal (OSS) wrote:
> > Hi Leif,
> >
> > > > +  ARM_SMC_ARGS  ArmSmcArgs;
> > > >
> > > > -Routine Description:
> > > > +  ArmSmcArgs.Arg0 = SMC_DRAM_BANK_INFO;
> > > > +  ArmSmcArgs.Arg1 = -1;
> > >
> > > Should this be SMC_UNK?
> >
> > No. SMC_OK / SMC_UNK is returned values.
> > While x0, x1 are arguments.
> > I have explained this in the MemoryInitPeiLib.h
> 
> OK, then that -1 needs a separate #define.

OK. That I will take care.

> 
> > // This SMC call works in this way:
> > // x1 = -1 : return x0: SMC_OK, x1: total DDR Ram size
> > // x1 >= number of DRAM regions to which DDR RAM is mapped : return x0:
> SMC_UNK
> > // 0 <= x1 < number of DRAM regions to which DDR RAM is mapped : return
> > //           x0: SMC_OK, x1: Base address of DRAM region,
> > //           x2: Size of DRAM region
> >
> > >
> > > >
> > > > +  ArmCallSmc (&ArmSmcArgs);
> > > >
> > > > +  if (ArmSmcArgs.Arg0 == SMC_OK) {
> > > > +    return ArmSmcArgs.Arg1;
> > > > +  }
> > > >
> >
> > ....
> >
> > > > -      {
> > > > -        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.

> 
> > > 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.
I thought ASSERT means we will halt the program execution regardless of DEBUG or RELEASE build.
Can you point me to a reference platform which handles these errors using ACPI BERT methods ?
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?

> 
> /
>     Leif