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.
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
>>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
