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

[Laszlo Ersek <lersek@redhat.com>]

On 08/04/17 10:50, Jordan Justen wrote:
> 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.

OK, I'll do that.

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

I agree that we don't need it now.

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

OK, thanks. I think it's out of scope for now.

Thanks
Laszlo