Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-07-26 09:23:26, Laszlo Ersek wrote:
> On 07/26/17 02:13, Jordan Justen wrote:
> > On 2017-07-10 20:22:31, Laszlo Ersek wrote:
> >> +STATIC
> >> +EFI_STATUS
> >> +E820HighRamIterate (
> >> +  IN     E820_HIGH_RAM_ENTRY_CALLBACK Callback,
> >> +  IN OUT VOID                         *Context
> >> +  )
> > 
> > I think a simpler option would be:
> > 
> > STATIC
> > EFI_STATUS
> > ScanOrAddE820HighRam (
> >   IN     E820_HIGH_RAM_ENTRY_CALLBACK Callback,
> >   OUT    UINT64                       *MaxAddress  OPTIONAL
> >   )
> >
> > If MaxAddress != NULL, then scan for it, otherwise add HOBs.
> > 
> > Do you anticipate future needs where the iterate callback could be
> > helpful?
> 
> Not at the moment.
> 
> Originally I started with two open-coded loops, but the code duplication
> in this case was really ugly. Using callbacks simplified the call sites
> very nicely. And, I was a bit concerned that you wouldn't like a
> solution that wasn't generic enough :)
> 
> I can rework the function like suggested if you prefer that.

I moved the code of the 2 callbacks into the the function and dropped
the callbacks to see how it looked. It seemed a bit clearer and was
less code.

The callback almost seems like something to consider for fw-cfg lib,
except I don't think we really have the need today.

> > 
> > You might also consider ScanOrAdd64BitE820Ram to somewhat clarify that
> > the 'HighRam' is addresses that don't fit in 32 bits.
> 
> OK.
> 
> > 
> >> +  QemuFwCfgSelectItem (FwCfgItem);
> >> +  for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) {
> >> +    QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry);
> >> +    DEBUG ((
> >> +      DEBUG_VERBOSE,
> >> +      "%a: Base=0x%Lx Length=0x%Lx Type=%u\n",
> >> +      __FUNCTION__,
> >> +      E820Entry.BaseAddr,
> >> +      E820Entry.Length,
> >> +      E820Entry.Type
> >> +      ));
> >> +    if (E820Entry.Type == EfiAcpiAddressRangeMemory &&
> >> +        E820Entry.BaseAddr >= BASE_4GB) {
> > 
> > I guess at least for IA32/X64, today, and for the foreseeable future
> > the firmware device will cause a break at 4GB.
> 
> Sorry, I don't understand. What do you mean by "firmware device" and
> "break at 4GB"?
> 
> > It seems like we could just check for the end of the range to be above
> > 4GB to easily remove this assumption, right?
> 
> I wanted to ensure that no RAM range would be included that (for any
> unexpected reason) straddled the 4GB mark.
> 
> Do you mean that the pflash address range is guaranteed to end at 4GB
> (exclusive), so no RAM range can straddle the mark?
> 
> Even so, what expression do you have in mind exactly? As far as I can
> imagine, checking the end vs. the base of the E820 entry would only
> complicate the above expression, not simplify it.
> 
> Please elaborate :)

Yeah, I meant is there any easy way to handle a situation (unlike qemu
piix4/q35 systems) where a RAM region went from below 4GB to above
4GB. If you don't think there is a simple was to handle it, then don't
worry about it.

-Jordan