On 11/30/15 12:06, Shannon Zhao wrote:
>
>
> 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 <[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.
>>
>
> 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]
And that's exactly what we want. The attributes WB / WT / WC / UC on the
UEFI memory map descriptor that corresponds to the NUMA range don't say
anything about the current caching settings of said memory; only about
the capabilities / possibilities of that range. If you don't set UC, the
OS will think it is not possible to set that memory range to UC. Which
is not correct.
The capabilities should look exactly the same as in any free memory (=
"Conventional Memory") entry in the "normal" range.
Thanks
Laszlo
>
>> 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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
