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

["Leif Lindholm" <leif@nuviainc.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.

> // 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.

> > 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.

/
    Leif