Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Ard Biesheuvel]

On 3 December 2015 at 12:52, Laszlo Ersek  wrote:
> Ard,
>
> On 11/30/15 18:48, Laszlo Ersek wrote:
>> On 11/30/15 12:48, Ard Biesheuvel wrote:
>>> On 30 November 2015 at 12:09, Laszlo Ersek  wrote:
 On 11/30/15 11:03, Ard Biesheuvel wrote:
>>> [...]
>
> Couldn't we simply add EFI_RESOURCE_SYSTEM_MEMORY resource descriptor
> HOBs the first time we enumerate the nodes?

 I didn't suggest it for two reasons.

 (1) I recalled that last time we had had a very long discussion about how 
 the DXE core selects the initial range for memory allocations (for its own 
 purposes, primarily). I had trouble remembering all the details now. So 
 there were three options for me:

 - recommend the HOBs, and hope for the best, i.e., hope whatever the DXE 
 core picks will be good for us

 - recommend the HOBs, and actually review what the DXE core does (it was 
 modified a little bit last time)

 - recommend a single HOB (which implies the DXE core gets initialized 
 exactly the same as before, no thinking or code browsing needed), and 
 delay the installation of the addtional ranges until later.

 I picked the last option for simplicity.

 (2) The other reason is that I don't think that the HOB approach would 
 solve the question of caching attributes. I don't think the DXE core tries 
 to set any attributes on its own when the GCD is primed from the HOBs.

 Remember we have

 InitializeMemory()  
 [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
   ArmPlatformInitializeSystemMemory()   
 [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
 PcdSet64 (PcdSystemMemorySize, ...)
   MemoryPeim()  
 [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
 PcdGet64 (PcdSystemMemorySize)
 BuildResourceDescriptorHob()
 InitMmu()   
 [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
   ArmPlatformGetVirtualMemoryMap()  
 [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
 PcdGet64 (PcdSystemMemorySize)
   ArmConfigureMmu() 
 [ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c]

 In other words, all of the following happens in the memory init PEIM 
 (which comes from ArmPlatformPkg, but it's plugged chock full of our 
 ArmVirtPkg library instances):
 - the in-DRAM DTB is initially parsed for the memory node
 - the size PCD is set
 - the base PCD is verified
 - a memory descriptor HOB is built, dependent on the PCDs
 - a virtual memory map is built, with caching attributes, dependent on the 
 PCDs
 - the MMU is configured

 Therefore we have a nice fast caching setting for the "base" memory node 
 only because you cover it explicitly in ArmPlatformGetVirtualMemoryMap() 
 -- dependent on the PCDs --, not because the DXE core covers it when it 
 processes the HOBs. So if you want to process several memory nodes *for 
 real* in the the memory init PEIM, then not only do you have to create the 
 HOBs there, but also extend the virtual memory map for the MMU similarly. 
 And a fixed count of PCDs won't be enough to carry the information from 
 the DTB to ArmPlatformGetVirtualMemoryMap() -- some loop would have to 
 exist that connects the DTB with the virtual memory map.

 This is what I meant in my original response by "having more flexibility 
 in DXE".

>>>
>>> Yes, that does sound like a lot of work for little gain. But I am not
>>> too crazy about adding more 'A PRIORI' modules, to avoid ending up in
>>> dependency hell later.
>>
>> I agree.
>>
>> Unfortunately, the only other possibility I can see is a separate driver
>> that has a DepEx on the CPU architectural protocol, iterates over the
>> FDT again, and installs the memory ranges (including setting the caching).
>>
>> (I also thought about delaying this logic within VirtFdtDxe: set up a
>> protocol notify callback on the CPU arch protocol, and do the same thing
>> in the callback. Unfortunately, this is exactly what the DXE core does
>> too, and the ordering between protocol notify callbacks is not
>> specified. So if ours ran first, there would be trouble.)
>>
>> Take your pick. If you opt for the separate driver / DepEx approach, I
>> can certainly agree with that as well. It would take a bit more
>> boilerplate (separate directory, separate INF file), but it would be
>> very clean, as far as dispatch order and driver responsibility go.
>
> What would be your preference here?
>
> Personally I'm leaning towards a separate driver, not dissimilar to
> VirtFdtDxe, that goes over the DTB, hunting for the memory nodes only,
> adding them and setting the caching attributes as well. (So it would
> have the depex.)
>
> We can think of 

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

Ard,

On 11/30/15 18:48, Laszlo Ersek wrote:
> On 11/30/15 12:48, Ard Biesheuvel wrote:
>> On 30 November 2015 at 12:09, Laszlo Ersek  wrote:
>>> On 11/30/15 11:03, Ard Biesheuvel wrote:
>> [...]

 Couldn't we simply add EFI_RESOURCE_SYSTEM_MEMORY resource descriptor
 HOBs the first time we enumerate the nodes?
>>>
>>> I didn't suggest it for two reasons.
>>>
>>> (1) I recalled that last time we had had a very long discussion about how 
>>> the DXE core selects the initial range for memory allocations (for its own 
>>> purposes, primarily). I had trouble remembering all the details now. So 
>>> there were three options for me:
>>>
>>> - recommend the HOBs, and hope for the best, i.e., hope whatever the DXE 
>>> core picks will be good for us
>>>
>>> - recommend the HOBs, and actually review what the DXE core does (it was 
>>> modified a little bit last time)
>>>
>>> - recommend a single HOB (which implies the DXE core gets initialized 
>>> exactly the same as before, no thinking or code browsing needed), and delay 
>>> the installation of the addtional ranges until later.
>>>
>>> I picked the last option for simplicity.
>>>
>>> (2) The other reason is that I don't think that the HOB approach would 
>>> solve the question of caching attributes. I don't think the DXE core tries 
>>> to set any attributes on its own when the GCD is primed from the HOBs.
>>>
>>> Remember we have
>>>
>>> InitializeMemory()  
>>> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>>>   ArmPlatformInitializeSystemMemory()   
>>> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>>> PcdSet64 (PcdSystemMemorySize, ...)
>>>   MemoryPeim()  
>>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>> PcdGet64 (PcdSystemMemorySize)
>>> BuildResourceDescriptorHob()
>>> InitMmu()   
>>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>>   ArmPlatformGetVirtualMemoryMap()  
>>> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>>> PcdGet64 (PcdSystemMemorySize)
>>>   ArmConfigureMmu() 
>>> [ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c]
>>>
>>> In other words, all of the following happens in the memory init PEIM (which 
>>> comes from ArmPlatformPkg, but it's plugged chock full of our ArmVirtPkg 
>>> library instances):
>>> - the in-DRAM DTB is initially parsed for the memory node
>>> - the size PCD is set
>>> - the base PCD is verified
>>> - a memory descriptor HOB is built, dependent on the PCDs
>>> - a virtual memory map is built, with caching attributes, dependent on the 
>>> PCDs
>>> - the MMU is configured
>>>
>>> Therefore we have a nice fast caching setting for the "base" memory node 
>>> only because you cover it explicitly in ArmPlatformGetVirtualMemoryMap() -- 
>>> dependent on the PCDs --, not because the DXE core covers it when it 
>>> processes the HOBs. So if you want to process several memory nodes *for 
>>> real* in the the memory init PEIM, then not only do you have to create the 
>>> HOBs there, but also extend the virtual memory map for the MMU similarly. 
>>> And a fixed count of PCDs won't be enough to carry the information from the 
>>> DTB to ArmPlatformGetVirtualMemoryMap() -- some loop would have to exist 
>>> that connects the DTB with the virtual memory map.
>>>
>>> This is what I meant in my original response by "having more flexibility in 
>>> DXE".
>>>
>>
>> Yes, that does sound like a lot of work for little gain. But I am not
>> too crazy about adding more 'A PRIORI' modules, to avoid ending up in
>> dependency hell later.
> 
> I agree.
> 
> Unfortunately, the only other possibility I can see is a separate driver
> that has a DepEx on the CPU architectural protocol, iterates over the
> FDT again, and installs the memory ranges (including setting the caching).
> 
> (I also thought about delaying this logic within VirtFdtDxe: set up a
> protocol notify callback on the CPU arch protocol, and do the same thing
> in the callback. Unfortunately, this is exactly what the DXE core does
> too, and the ordering between protocol notify callbacks is not
> specified. So if ours ran first, there would be trouble.)
> 
> Take your pick. If you opt for the separate driver / DepEx approach, I
> can certainly agree with that as well. It would take a bit more
> boilerplate (separate directory, separate INF file), but it would be
> very clean, as far as dispatch order and driver responsibility go.

What would be your preference here?

Personally I'm leaning towards a separate driver, not dissimilar to
VirtFdtDxe, that goes over the DTB, hunting for the memory nodes only,
adding them and setting the caching attributes as well. (So it would
have the depex.)

We can think of this driver as a "memory controller driver", except the
only thing it'd have to do is look at the DTB.

Thanks
Laszlo
___
edk2-devel 

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Ard Biesheuvel]

On 30 November 2015 at 11: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  wrote:


 On 2015/11/30 16:53, Laszlo Ersek wrote:
> On 11/29/15 07:31, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> 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 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>UINT64 FwCfgDataSize;
>>UINT64 FwCfgDmaAddress;
>>UINT64 FwCfgDmaSize;
>> +  UINT64 CurBase;
>> +  UINT64 CurSize;
>>
>>Hob = GetFirstGuidHob();
>>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 0x4000 0x0 0x800>;
> 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", );
>> +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", );
>> +  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:
   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
   |  |  |  

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 10:28, Ard Biesheuvel wrote:
> On 30 November 2015 at 10:22, Shannon Zhao  wrote:
>>
>>
>> On 2015/11/30 16:53, Laszlo Ersek wrote:
>>> On 11/29/15 07:31, Shannon Zhao wrote:
 From: Shannon Zhao 

 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 
 ---
  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 
  #include 
  #include 
 +#include 

  #include 
  #include 
 @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
UINT64 FwCfgDataSize;
UINT64 FwCfgDmaAddress;
UINT64 FwCfgDmaSize;
 +  UINT64 CurBase;
 +  UINT64 CurSize;

Hob = GetFirstGuidHob();
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 0x4000 0x0 0x800>;
>>> 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", );
 +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", );
 +  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:
>>   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>   |  |  |  |UC]
>>   0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>   |  |  |  |UC]
>>   0x4000-0x4000 [Loader Data|   |  |  |  |  |  |
>>   |WB|WT|WC|UC]
>>   0x4001-0x4007 [Conventional Memory|   |  |  |  |  |  |
>>   |WB|WT|WC|UC]
>>
>> [cut some information here]
>>
>>   

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

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  wrote:
>>>
>>>
>>> On 2015/11/30 16:53, Laszlo Ersek wrote:
 On 11/29/15 07:31, Shannon Zhao wrote:
> From: Shannon Zhao 
>
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>UINT64 FwCfgDataSize;
>UINT64 FwCfgDmaAddress;
>UINT64 FwCfgDmaSize;
> +  UINT64 CurBase;
> +  UINT64 CurSize;
>
>Hob = GetFirstGuidHob();
>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 0x4000 0x0 0x800>;
 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", );
> +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", );
> +  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:
>>>   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>>   |  |  |  |UC]
>>>   0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
>>>   |  |  |  |UC]
>>>   0x4000-0x4000 [Loader Data|   |  |  |  |  |  |
>>>   |WB|WT|WC|UC]

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 12:48, Ard Biesheuvel wrote:
> On 30 November 2015 at 12:09, Laszlo Ersek  wrote:
>> On 11/30/15 11:03, Ard Biesheuvel wrote:
> [...]
>>>
>>> Couldn't we simply add EFI_RESOURCE_SYSTEM_MEMORY resource descriptor
>>> HOBs the first time we enumerate the nodes?
>>
>> I didn't suggest it for two reasons.
>>
>> (1) I recalled that last time we had had a very long discussion about how 
>> the DXE core selects the initial range for memory allocations (for its own 
>> purposes, primarily). I had trouble remembering all the details now. So 
>> there were three options for me:
>>
>> - recommend the HOBs, and hope for the best, i.e., hope whatever the DXE 
>> core picks will be good for us
>>
>> - recommend the HOBs, and actually review what the DXE core does (it was 
>> modified a little bit last time)
>>
>> - recommend a single HOB (which implies the DXE core gets initialized 
>> exactly the same as before, no thinking or code browsing needed), and delay 
>> the installation of the addtional ranges until later.
>>
>> I picked the last option for simplicity.
>>
>> (2) The other reason is that I don't think that the HOB approach would solve 
>> the question of caching attributes. I don't think the DXE core tries to set 
>> any attributes on its own when the GCD is primed from the HOBs.
>>
>> Remember we have
>>
>> InitializeMemory()  
>> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>>   ArmPlatformInitializeSystemMemory()   
>> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>> PcdSet64 (PcdSystemMemorySize, ...)
>>   MemoryPeim()  
>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>> PcdGet64 (PcdSystemMemorySize)
>> BuildResourceDescriptorHob()
>> InitMmu()   
>> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>>   ArmPlatformGetVirtualMemoryMap()  
>> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>> PcdGet64 (PcdSystemMemorySize)
>>   ArmConfigureMmu() 
>> [ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c]
>>
>> In other words, all of the following happens in the memory init PEIM (which 
>> comes from ArmPlatformPkg, but it's plugged chock full of our ArmVirtPkg 
>> library instances):
>> - the in-DRAM DTB is initially parsed for the memory node
>> - the size PCD is set
>> - the base PCD is verified
>> - a memory descriptor HOB is built, dependent on the PCDs
>> - a virtual memory map is built, with caching attributes, dependent on the 
>> PCDs
>> - the MMU is configured
>>
>> Therefore we have a nice fast caching setting for the "base" memory node 
>> only because you cover it explicitly in ArmPlatformGetVirtualMemoryMap() -- 
>> dependent on the PCDs --, not because the DXE core covers it when it 
>> processes the HOBs. So if you want to process several memory nodes *for 
>> real* in the the memory init PEIM, then not only do you have to create the 
>> HOBs there, but also extend the virtual memory map for the MMU similarly. 
>> And a fixed count of PCDs won't be enough to carry the information from the 
>> DTB to ArmPlatformGetVirtualMemoryMap() -- some loop would have to exist 
>> that connects the DTB with the virtual memory map.
>>
>> This is what I meant in my original response by "having more flexibility in 
>> DXE".
>>
> 
> Yes, that does sound like a lot of work for little gain. But I am not
> too crazy about adding more 'A PRIORI' modules, to avoid ending up in
> dependency hell later.

I agree.

Unfortunately, the only other possibility I can see is a separate driver
that has a DepEx on the CPU architectural protocol, iterates over the
FDT again, and installs the memory ranges (including setting the caching).

(I also thought about delaying this logic within VirtFdtDxe: set up a
protocol notify callback on the CPU arch protocol, and do the same thing
in the callback. Unfortunately, this is exactly what the DXE core does
too, and the ordering between protocol notify callbacks is not
specified. So if ours ran first, there would be trouble.)

Take your pick. If you opt for the separate driver / DepEx approach, I
can certainly agree with that as well. It would take a bit more
boilerplate (separate directory, separate INF file), but it would be
very clean, as far as dispatch order and driver responsibility go.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Shannon Zhao]

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  wrote:


 On 2015/11/30 16:53, Laszlo Ersek wrote:
> On 11/29/15 07:31, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> 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 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>UINT64 FwCfgDataSize;
>>UINT64 FwCfgDmaAddress;
>>UINT64 FwCfgDmaSize;
>> +  UINT64 CurBase;
>> +  UINT64 CurSize;
>>
>>Hob = GetFirstGuidHob();
>>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 0x4000 0x0 0x800>;
> 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", );
>> +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", );
>> +  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:
   0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
   |  |  |  |UC]
   

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/30/15 11:03, Ard Biesheuvel wrote:
> On 30 November 2015 at 11: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  
 wrote:
>
>
> On 2015/11/30 16:53, Laszlo Ersek wrote:
>> On 11/29/15 07:31, Shannon Zhao wrote:
>>> From: Shannon Zhao 
>>>
>>> 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 
>>> ---
>>>  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 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>>UINT64 FwCfgDataSize;
>>>UINT64 FwCfgDmaAddress;
>>>UINT64 FwCfgDmaSize;
>>> +  UINT64 CurBase;
>>> +  UINT64 CurSize;
>>>
>>>Hob = GetFirstGuidHob();
>>>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 0x4000 0x0 0x800>;
>> 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", );
>>> +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", );
>>> +  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 

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Ard Biesheuvel]

On 30 November 2015 at 12:09, Laszlo Ersek  wrote:
> On 11/30/15 11:03, Ard Biesheuvel wrote:
[...]
>>
>> Couldn't we simply add EFI_RESOURCE_SYSTEM_MEMORY resource descriptor
>> HOBs the first time we enumerate the nodes?
>
> I didn't suggest it for two reasons.
>
> (1) I recalled that last time we had had a very long discussion about how the 
> DXE core selects the initial range for memory allocations (for its own 
> purposes, primarily). I had trouble remembering all the details now. So there 
> were three options for me:
>
> - recommend the HOBs, and hope for the best, i.e., hope whatever the DXE core 
> picks will be good for us
>
> - recommend the HOBs, and actually review what the DXE core does (it was 
> modified a little bit last time)
>
> - recommend a single HOB (which implies the DXE core gets initialized exactly 
> the same as before, no thinking or code browsing needed), and delay the 
> installation of the addtional ranges until later.
>
> I picked the last option for simplicity.
>
> (2) The other reason is that I don't think that the HOB approach would solve 
> the question of caching attributes. I don't think the DXE core tries to set 
> any attributes on its own when the GCD is primed from the HOBs.
>
> Remember we have
>
> InitializeMemory()  
> [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>   ArmPlatformInitializeSystemMemory()   
> [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
> PcdSet64 (PcdSystemMemorySize, ...)
>   MemoryPeim()  
> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
> PcdGet64 (PcdSystemMemorySize)
> BuildResourceDescriptorHob()
> InitMmu()   
> [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>   ArmPlatformGetVirtualMemoryMap()  
> [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
> PcdGet64 (PcdSystemMemorySize)
>   ArmConfigureMmu() 
> [ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c]
>
> In other words, all of the following happens in the memory init PEIM (which 
> comes from ArmPlatformPkg, but it's plugged chock full of our ArmVirtPkg 
> library instances):
> - the in-DRAM DTB is initially parsed for the memory node
> - the size PCD is set
> - the base PCD is verified
> - a memory descriptor HOB is built, dependent on the PCDs
> - a virtual memory map is built, with caching attributes, dependent on the 
> PCDs
> - the MMU is configured
>
> Therefore we have a nice fast caching setting for the "base" memory node only 
> because you cover it explicitly in ArmPlatformGetVirtualMemoryMap() -- 
> dependent on the PCDs --, not because the DXE core covers it when it 
> processes the HOBs. So if you want to process several memory nodes *for real* 
> in the the memory init PEIM, then not only do you have to create the HOBs 
> there, but also extend the virtual memory map for the MMU similarly. And a 
> fixed count of PCDs won't be enough to carry the information from the DTB to 
> ArmPlatformGetVirtualMemoryMap() -- some loop would have to exist that 
> connects the DTB with the virtual memory map.
>
> This is what I meant in my original response by "having more flexibility in 
> DXE".
>

Yes, that does sound like a lot of work for little gain. But I am not
too crazy about adding more 'A PRIORI' modules, to avoid ending up in
dependency hell later.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Laszlo Ersek]

On 11/29/15 07:31, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>UINT64 FwCfgDataSize;
>UINT64 FwCfgDmaAddress;
>UINT64 FwCfgDmaSize;
> +  UINT64 CurBase;
> +  UINT64 CurSize;
>  
>Hob = GetFirstGuidHob();
>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 0x4000 0x0 0x800>;
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)

> +Type = fdt_getprop (DeviceTreeBase, Node, "device_type", );
> +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", );
> +  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".

(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"?

> +  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.

Thanks
Laszlo

> +}
> +
>  Type = fdt_getprop (DeviceTreeBase, Node, "compatible", );
>  if (Type == NULL) {
>continue;
> diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf 
> b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> index ee2503a..0e6927b 100644
> --- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
> +++ 

Re: [edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Shannon Zhao]

On 2015/11/30 16:53, Laszlo Ersek wrote:
> On 11/29/15 07:31, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> 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 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  #include 
>> @@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
>>UINT64 FwCfgDataSize;
>>UINT64 FwCfgDmaAddress;
>>UINT64 FwCfgDmaSize;
>> +  UINT64 CurBase;
>> +  UINT64 CurSize;
>>  
>>Hob = GetFirstGuidHob();
>>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 0x4000 0x0 0x800>;
> 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", );
>> +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", );
>> +  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:
  0x0400-0x07ff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
  |  |  |  |UC]
  0x0901-0x09010fff [Memory Mapped I/O  |RUN|  |XP|  |  |  |
  |  |  |  |UC]
  0x4000-0x4000 [Loader Data|   |  |  |  |  |  |
  |WB|WT|WC|UC]
  0x4001-0x4007 [Conventional Memory|   |  |  |  |  |  |
  |WB|WT|WC|UC]

[cut some information here]

  0x6000-0xbfff [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?

>> +  if (EFI_ERROR(Status)) {
>> +ASSERT_EFI_ERROR(Status);
>> +  }
> 
> (5) "ASSERT_EFI_ERROR (Status)" would suffice.
> 
> However, I think 

[edk2] [PATCH 2/2] ArmVirtPkg: Add memory space for the memory nodes except the lowest one

[Shannon Zhao]

From: Shannon Zhao 

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 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -304,6 +305,8 @@ InitializeVirtFdtDxe (
   UINT64 FwCfgDataSize;
   UINT64 FwCfgDmaAddress;
   UINT64 FwCfgDmaSize;
+  UINT64 CurBase;
+  UINT64 CurSize;
 
   Hob = GetFirstGuidHob();
   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
+//
+Type = fdt_getprop (DeviceTreeBase, Node, "device_type", );
+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", );
+  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);
+  if (EFI_ERROR(Status)) {
+ASSERT_EFI_ERROR(Status);
+  }
+ DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
+ __FUNCTION__, CurBase, CurBase + CurSize - 1));
+}
+  } else {
+DEBUG ((EFI_D_ERROR, "%a: Failed to parse FDT memory node\n",
+   __FUNCTION__));
+  }
+}
+
 Type = fdt_getprop (DeviceTreeBase, Node, "compatible", );
 if (Type == NULL) {
   continue;
diff --git a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf 
b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
index ee2503a..0e6927b 100644
--- a/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
+++ b/ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.inf
@@ -43,12 +43,16 @@
   VirtioMmioDeviceLib
   HobLib
   XenIoMmioLib
+  DxeServicesTableLib
 
 [Guids]
   gFdtTableGuid
   gVirtioMmioTransportGuid
   gFdtHobGuid
 
+[FixedPcd]
+  gArmTokenSpaceGuid.PcdSystemMemoryBase
+
 [Pcd]
   gArmVirtTokenSpaceGuid.PcdArmPsciMethod
   gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress
-- 
2.0.4


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel