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

[Laszlo Ersek <lersek@redhat.com>]

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.

> 
> 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 :)

Thanks
Laszlo