Re: [PATCH v2 2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

[Laszlo Ersek <lersek@redhat.com>]

On 11/27/18 22:18, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 21:25, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 11/27/18 18:52, Ard Biesheuvel wrote:
>>> On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>
>>>> On 11/27/18 15:54, Ard Biesheuvel wrote:
>>>>> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
>>>>> entire virtual address space is mapped with EFI_MEMORY_UC attributes,
>>>>> regardless of whether any devices actually reside there.
>>>>>
>>>>> Now that we are relaxing the address space limit to more than 40 bits,
>>>>> mapping all that address space actually takes up more space in page
>>>>> tables than we have so far made available as temporary RAM. So let's
>>>>> get rid of the mapping rather than increasing the available RAM, given
>>>>> that the mapping is not particularly useful anyway.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> ---
>>>>>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
>>>>>  1 file changed, 5 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>>>> index 815ca145b644..70863abb2e7b 100644
>>>>> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>>>> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
>>>>> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (
>>>>>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
>>>>>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>>>>
>>>>> -  // Peripheral space after DRAM
>>>>> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
>>>>> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>>>>> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
>>>>> -                                       VirtualMemoryTable[2].PhysicalBase;
>>>>> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>>>> -
>>>>>    // Remap the FD region as normal executable memory
>>>>> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
>>>>> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
>>>>> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
>>>>> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>>>>> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
>>>>> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>>>>> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
>>>>> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>>>>>
>>>>>    // End of Table
>>>>> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>>>> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
>>>>>
>>>>>    *VirtualMemoryMap = VirtualMemoryTable;
>>>>>  }
>>>>>
>>>>
>>>> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?
>>>>
>>>
>>> Not quite. It complements it, in the sense that is should fix the
>>> issue reported by Eric when mapping the entire address 48-bit address
>>> space.
>>
>> Oh, you meant this one *on top* of that? In particular, on top of:
>>
>> [edk2] [PATCH v2 11/13] ArmVirtPkg/QemuVirtMemInfoLib: ignore
>>                         PcdPrePiCpuMemorySize
>>
>> That wasn't clear to me, sorry.
>>
> 
> No, the other way around actually :-)

How so? Patch v2 11/13 removes:

> -  VirtualMemoryTable[2].Length       = TopOfMemory -

and adds:

> +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

and in the current patch, you remove

> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

So the current patch wouldn't apply before v2 11/13.

Anyway, this is not so important :)

> Apologies, I managed to confuse myself a bit as well, so I understand
> this may be slightly difficult to follow.

Yeah :)

>> If this one comes on top of the v2 13-part series, do you ultimately
>> need v2 11/13 as a separate patch -- in that form anyway? It seems that
>> you could squash this patch into v2 11/13, and eliminate the dependency
>> on PcdPrePiCpuMemorySize *by* killing the entry that maps the Peripheral
>> space after DRAM.
>>
> 
> Indeed. So after applying these two patches, I will need to respin
> that series once more, and now that I think of it, it might make sense
> to simplify those changes signficantly, given that only the Xen code
> needs to access the CPU's capability registers in the platform MMU
> setup code.

Thank you for explaining. I'll wait for the one and only v3 then.

Thanks!
Laszlo