On 11/30/15 10:28, Ard Biesheuvel wrote:
> On 30 November 2015 at 10:22, Shannon Zhao <[email protected]> wrote:
>>
>>
>> On 2015/11/30 16:53, Laszlo Ersek wrote:
>>> On 11/29/15 07:31, Shannon Zhao wrote:
>>>> From: Shannon Zhao <[email protected]>
>>>>
>>>> Here we add the memory space for the memory nodes except the lowest one
>>>> in FDT. So these spaces will show up in the UEFI memory map.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Shannon Zhao <[email protected]>
>>>> ---
>>>> ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c | 34
>>>> ++++++++++++++++++++++++++++++++++
>>>> ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf | 4 ++++
>>>> 2 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>> index 74f80d1..2c8d785 100644
>>>> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>> +++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c
>>>> @@ -27,6 +27,7 @@
>>>> #include <Library/HobLib.h>
>>>> #include <libfdt.h>
>>>> #include <Library/XenIoMmioLib.h>
>>>> +#include <Library/DxeServicesTableLib.h>
>>>>
>>>> #include <Guid/Fdt.h>
>>>> #include <Guid/VirtioMmioTransport.h>
>>>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>>> UINT64 FwCfgDataSize;
>>>> UINT64 FwCfgDmaAddress;
>>>> UINT64 FwCfgDmaSize;
>>>> + UINT64 CurBase;
>>>> + UINT64 CurSize;
>>>>
>>>> Hob = GetFirstGuidHob(&gFdtHobGuid);
>>>> if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>>>> @@ -332,6 +335,37 @@ InitializeVirtFdtDxe (
>>>> break;
>>>> }
>>>>
>>>> + //
>>>> + // Check for memory node and add the memory spaces expect the lowest
>>>> one
>>>> + //
>>>
>>> (1) Based on the DTB node, which looks like:
>>>
>>> memory {
>>> reg = <0x0 0x40000000 0x0 0x8000000>;
>>> device_type = "memory";
>>> };
>>>
>>> I agree that checking it here, before we look at "compatible", is a good
>>> thing.
>>>
>>> However, can you please factor this out into a separate function? (Just
>>> within this file, as a STATIC function.)
>>>
>>> I propose that the function return a BOOLEAN:
>>> - TRUE if the node was a match (either successfully or unsuccessfully);
>>> in which case you can "continue;" with the next iteration directly
>>> - FALSE, if there is no match (i.e., the current iteration must
>>> proceed, in order to look for different device types)
>>>
>>
>> Sure.
>>
>>>> + Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>>>> + if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>>>> + //
>>>> + // Get the 'reg' property of this node. For now, we will assume
>>>> + // two 8 byte quantities for base and size, respectively.
>>>> + //
>>>> + RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>>>> + if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
>>>> +
>>>> + CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>>>> + CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>>>> +
>>>> + if (FixedPcdGet64 (PcdSystemMemoryBase) != CurBase) {
>>>> + Status = gDS->AddMemorySpace(EfiGcdMemoryTypeSystemMemory,
>>>> + CurBase, CurSize,
>>>> + EFI_MEMORY_WB |
>>>> EFI_MEMORY_RUNTIME);
>>>
>>> (1) The edk2 coding style requires a space between the function name (or
>>> function pointer) and the opening paren.
>>>
>>> (2) Regarding the indentation of function arguments, they are aligned
>>> with the first or (more usually:) second character of the function or
>>> *member* name. In this case, they should be aligned with the second "d"
>>> in "AddMemorySpace".
>>>
>> Thanks for explain the edk2 coding style. Sorry, I'm not familiar with this.
>>
>>> (3) The last argument is a bitmask of capabilities. As far as I
>>> remember, all system memory regions I've seen dumped by Linux from the
>>> UEFI memmap at startup had all of: UC | WC | WT | WB.
>>>
>>> I agree that RUNTIME and WB should be listed here, but do you have any
>>> particular reason for not allowing UC|WC|WT?
>>>
>>> (4) Related question -- when you boot the guest kernel with this patch
>>> in the firmware, and pass "efi=debug" to Linux, do the memory attributes
>>> of the additional NUMA nodes look identically to the "base" memory? (I'm
>>> asking this to see if we need additional gDS->SetMemorySpaceAttributes()
>>> calls.)
>>>
>>> Can you paste a sample output from "efi=debug"?
>>>
>>
>> Processing EFI memory map:
>> 0x000004000000-0x000007ffffff [Memory Mapped I/O |RUN| |XP| | | |
>> | | | |UC]
>> 0x000009010000-0x000009010fff [Memory Mapped I/O |RUN| |XP| | | |
>> | | | |UC]
>> 0x000040000000-0x00004000ffff [Loader Data | | | | | | |
>> |WB|WT|WC|UC]
>> 0x000040010000-0x00004007ffff [Conventional Memory| | | | | | |
>> |WB|WT|WC|UC]
>>
>> [cut some information here]
>>
>> 0x000060000000-0x0000bfffffff [Conventional Memory|RUN| | | | | |
>> |WB| | | ]
>>
>> Looks like the region is showed with the attributes we set. While
>> comparing with other Conventional Memory, it doesn't have RUNTIME. So
>> drop it?
>>
>
> You should not set the runtime bit, but you should set the UC, WC and
> WT bits, as Laszlo mentioned.
After reading the specs a bit more, I think the following should be done:
- Call AddMemorySpace() with Capabilities=WB|WT|WC|UC (and nothing
else) -- as you say. That is, in the existing call, clear RUNTIME, and
add WT|WC|UC.
- Then call SetMemorySpaceAttributes too, for the same range (each
range), with Attributes=WB only. The default attributes (after the
AddMemorySpace() call) don't seem to be specified by the PI spec, so
we had better specify explicitly what caching we'd like. (The UEFI
memmap dump also shows the capabilities only, not the current
setting.)
So the point here is to set WB|WT|WC|UC as capabilities, and WB as
actual attribute.
- If either of these calls fails, it should be logged on error level
(naming the range and also which call of the above two failed), but
we should continue processing the DTB either way.
Thanks
Laszlo
>
>>>> + if (EFI_ERROR(Status)) {
>>>> + ASSERT_EFI_ERROR(Status);
>>>> + }
>>>
>>> (5) "ASSERT_EFI_ERROR (Status)" would suffice.
>>>
>>> However, I think I'd prefer if we just logged an error here (with
>>> EFI_D_ERROR level), and continued (i.e., returned TRUE).
>>>
>>>> + DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
>>>> + __FUNCTION__, CurBase, CurBase + CurSize - 1));
>>>> + }
>>>
>>> (6) This isn't indented correctly (not just the arguments).
>>>
>>>> + } else {
>>>> + DEBUG ((EFI_D_ERROR, "%a: Failed to parse FDT memory node\n",
>>>> + __FUNCTION__));
>>>> + }
>>>
>>> (7) I think this error log branch is not necessary here. The same will
>>> have been logged in the code that is modified by patch #1.
>>>
>>
>> Will fix these and re-send a new version.
>>
>> Thanks,
>> --
>> Shannon
>>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
