On 2015/11/30 18:01, Laszlo Ersek wrote:
> On 11/30/15 10:50, Laszlo Ersek wrote:
>> On 11/30/15 10:28, Ard Biesheuvel wrote:
>>> On 30 November 2015 at 10:22, Shannon Zhao <zhaoshengl...@huawei.com> wrote:
>>>>
>>>>
>>>> On 2015/11/30 16:53, Laszlo Ersek wrote:
>>>>> On 11/29/15 07:31, Shannon Zhao wrote:
>>>>>> From: Shannon Zhao <shannon.z...@linaro.org>
>>>>>>
>>>>>> 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 <shannon.z...@linaro.org>
>>>>>> ---
>>>>>> 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.
>
I don't think it needs to call gDS->SetMemorySpaceAttributes(). The
above log is printed while I only set EFI_MEMORY_WB | EFI_MEMORY_RUNTIME
And the log shows the attributes are RUN and WB.
If I set EFI_MEMORY_WB | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_UC,
then the log shows the attributes are WB|WT|WC|UC as below:
0x000060000000-0x0000bfffffff [Conventional Memory| | | | | | |
|WB|WT|WC|UC]
> Sigh. This could get messy. gDS->SetMemorySpaceAttributes() is
> ultimately implemented by the CPU arch protocol driver, which in this
> case is ArmPkg/Drivers/CpuDxe.
>
> However, that driver is currently dispatched strictly later than VirtFdtDxe.
>
> Should we add ArmPkg/Drivers/CpuDxe to the APRIORI file too, just before
> VirtFdtDxe?
>
> Or else, if we don't call gDS->SetMemorySpaceAttributes(), what do you
> think will be the default caching attribute for the NUMA ranges?
>
> It appears the simplest thing to try is to add CpuDxe to APRIORI DXE.
>
> Thanks
> Laszlo
>
>> - 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
>>>>
>>
>
>
> .
>
--
Shannon
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
