* [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
2016-09-15 13:30 [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
@ 2016-09-15 13:30 ` Ard Biesheuvel
2016-09-15 13:38 ` Laszlo Ersek
2016-09-15 13:30 ` [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 13:30 UTC (permalink / raw)
To: edk2-devel, lersek; +Cc: zhaoshenglong, sakar.arora, Ard Biesheuvel
Currently, the code in FdtClientDxe assumes #address-cells and
of <base, size> tuples, this means the size of the entire property
should always be a multiple of 16 bytes (i.e, 4 * sizeof(UINT32),
not 8. So fix this.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index c336e2410033..2063a597323b 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -178,7 +178,7 @@ FindCompatibleNodeReg (
return Status;
}
- if ((*RegSize % 8) != 0) {
+ if ((*RegSize % 16) != 0) {
DEBUG ((EFI_D_ERROR,
"%a: '%a' compatible node has invalid 'reg' property (size == 0x%x)\n",
__FUNCTION__, CompatibleString, *RegSize));
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
2016-09-15 13:30 ` [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties Ard Biesheuvel
@ 2016-09-15 13:38 ` Laszlo Ersek
2016-09-15 13:40 ` Ard Biesheuvel
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2016-09-15 13:38 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
On 09/15/16 15:30, Ard Biesheuvel wrote:
> Currently, the code in FdtClientDxe assumes #address-cells and
s/and/are/?
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> of <base, size> tuples, this means the size of the entire property
> should always be a multiple of 16 bytes (i.e, 4 * sizeof(UINT32),
> not 8. So fix this.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index c336e2410033..2063a597323b 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -178,7 +178,7 @@ FindCompatibleNodeReg (
> return Status;
> }
>
> - if ((*RegSize % 8) != 0) {
> + if ((*RegSize % 16) != 0) {
> DEBUG ((EFI_D_ERROR,
> "%a: '%a' compatible node has invalid 'reg' property (size == 0x%x)\n",
> __FUNCTION__, CompatibleString, *RegSize));
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
2016-09-15 13:38 ` Laszlo Ersek
@ 2016-09-15 13:40 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 13:40 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Shannon Zhao, Sakar Arora
On 15 September 2016 at 14:38, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/15/16 15:30, Ard Biesheuvel wrote:
>> Currently, the code in FdtClientDxe assumes #address-cells and
>
> s/and/are/?
>
No, amusingly, the second sentence started with #size-cells, and was
dropped completely by git commit due to the leading #. I will try to
remember what I put there, and fix it up when committing.
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
Thanks,
Ard.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly
2016-09-15 13:30 [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
2016-09-15 13:30 ` [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties Ard Biesheuvel
@ 2016-09-15 13:30 ` Ard Biesheuvel
2016-09-15 13:42 ` Laszlo Ersek
2016-09-15 13:30 ` [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 13:30 UTC (permalink / raw)
To: edk2-devel, lersek; +Cc: zhaoshenglong, sakar.arora, Ard Biesheuvel
The FDT client protocol methods dealing with "reg" properties return
the size of a 'reg' element. Currently, we have hardcoded this as '8',
since #address-cells == #size-cells == 2 in most cases. However, for
different values, have a single 'reg' element size is not unambiguous,
since - however unlikely - if #address-cells != #size-cells, we do not
know which is which.
So before adding more methods to the protocol, fix up this oversight.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 6 ++++--
ArmVirtPkg/Include/Protocol/FdtClient.h | 3 ++-
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 9 ++++++---
ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 9 ++++++---
ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c | 8 ++++++--
ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c | 10 +++++++---
6 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 2063a597323b..382e9af6bf84 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -159,7 +159,8 @@ FindCompatibleNodeReg (
IN FDT_CLIENT_PROTOCOL *This,
IN CONST CHAR8 *CompatibleString,
OUT CONST VOID **Reg,
- OUT UINT32 *RegElemSize,
+ OUT UINTN *AddressCells,
+ OUT UINTN *SizeCells,
OUT UINT32 *RegSize
)
{
@@ -185,7 +186,8 @@ FindCompatibleNodeReg (
return EFI_NOT_FOUND;
}
- *RegElemSize = 8;
+ *AddressCells = 2;
+ *SizeCells = 2;
return EFI_SUCCESS;
}
diff --git a/ArmVirtPkg/Include/Protocol/FdtClient.h b/ArmVirtPkg/Include/Protocol/FdtClient.h
index 79a8f3ce1b5d..b593c7441426 100644
--- a/ArmVirtPkg/Include/Protocol/FdtClient.h
+++ b/ArmVirtPkg/Include/Protocol/FdtClient.h
@@ -80,7 +80,8 @@ EFI_STATUS
IN FDT_CLIENT_PROTOCOL *This,
IN CONST CHAR8 *CompatibleString,
OUT CONST VOID **Reg,
- OUT UINT32 *RegElemSize,
+ OUT UINTN *AddressCells,
+ OUT UINTN *SizeCells,
OUT UINT32 *RegSize
);
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
index a1cd2da2d43a..64afc4de6b4d 100644
--- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -36,7 +36,8 @@ ArmVirtGicArchLibConstructor (
UINT32 IccSre;
FDT_CLIENT_PROTOCOL *FdtClient;
CONST UINT64 *Reg;
- UINT32 RegElemSize, RegSize;
+ UINT32 RegSize;
+ UINTN AddressCells, SizeCells;
UINTN GicRevision;
EFI_STATUS Status;
UINT64 DistBase, CpuBase, RedistBase;
@@ -47,11 +48,13 @@ ArmVirtGicArchLibConstructor (
GicRevision = 2;
Status = FdtClient->FindCompatibleNodeReg (FdtClient, "arm,cortex-a15-gic",
- (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+ (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+ &RegSize);
if (Status == EFI_NOT_FOUND) {
GicRevision = 3;
Status = FdtClient->FindCompatibleNodeReg (FdtClient, "arm,gic-v3",
- (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+ (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+ &RegSize);
}
if (EFI_ERROR (Status)) {
return Status;
diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 377262563e3e..8ecbe3fb5fe6 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -122,7 +122,8 @@ QemuFwCfgInitialize (
EFI_STATUS Status;
FDT_CLIENT_PROTOCOL *FdtClient;
CONST UINT64 *Reg;
- UINT32 RegElemSize, RegSize;
+ UINT32 RegSize;
+ UINTN AddressCells, SizeCells;
UINT64 FwCfgSelectorAddress;
UINT64 FwCfgSelectorSize;
UINT64 FwCfgDataAddress;
@@ -135,7 +136,8 @@ QemuFwCfgInitialize (
ASSERT_EFI_ERROR (Status);
Status = FdtClient->FindCompatibleNodeReg (FdtClient, "qemu,fw-cfg-mmio",
- (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+ (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+ &RegSize);
if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_WARN,
"%a: No 'qemu,fw-cfg-mmio' compatible DT node found (Status == %r)\n",
@@ -143,7 +145,8 @@ QemuFwCfgInitialize (
return EFI_SUCCESS;
}
- ASSERT (RegElemSize == sizeof (UINT64));
+ ASSERT (AddressCells == 2);
+ ASSERT (SizeCells == 2);
ASSERT (RegSize == 2 * sizeof (UINT64));
FwCfgDataAddress = SwapBytes64 (Reg[0]);
diff --git a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
index 37aea806283e..203946f97bf8 100644
--- a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
+++ b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
@@ -45,7 +45,8 @@ GetXenArmAcpiRsdp (
EFI_STATUS Status;
FDT_CLIENT_PROTOCOL *FdtClient;
CONST UINT64 *Reg;
- UINT32 RegElemSize, RegSize;
+ UINT32 RegSize;
+ UINTN AddressCells, SizeCells;
UINT64 RegBase;
UINT8 Sum;
@@ -59,13 +60,16 @@ GetXenArmAcpiRsdp (
ASSERT_EFI_ERROR (Status);
Status = FdtClient->FindCompatibleNodeReg (FdtClient, "xen,guest-acpi",
- (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+ (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+ &RegSize);
if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_WARN, "%a: No 'xen,guest-acpi' compatible DT node found\n",
__FUNCTION__));
return EFI_NOT_FOUND;
}
+ ASSERT (AddressCells == 2);
+ ASSERT (SizeCells == 2);
ASSERT (RegSize == 2 * sizeof (UINT64));
RegBase = SwapBytes64(Reg[0]);
diff --git a/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c b/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c
index 4a88db32170c..ae012a76f5e0 100644
--- a/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c
+++ b/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c
@@ -31,7 +31,8 @@ InitializeXenioFdtDxe (
EFI_STATUS Status;
FDT_CLIENT_PROTOCOL *FdtClient;
CONST UINT64 *Reg;
- UINT32 RegElemSize, RegSize;
+ UINT32 RegSize;
+ UINTN AddressCells, SizeCells;
EFI_HANDLE Handle;
UINT64 RegBase;
@@ -40,14 +41,17 @@ InitializeXenioFdtDxe (
ASSERT_EFI_ERROR (Status);
Status = FdtClient->FindCompatibleNodeReg (FdtClient, "xen,xen",
- (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+ (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+ &RegSize);
if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_WARN, "%a: No 'xen,xen' compatible DT node found\n",
__FUNCTION__));
return EFI_UNSUPPORTED;
}
- ASSERT (RegSize == 16);
+ ASSERT (AddressCells == 2);
+ ASSERT (SizeCells == 2);
+ ASSERT (RegSize == 2 * sizeof (UINT64));
//
// Retrieve the reg base from this node and wire it up to the
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly
2016-09-15 13:30 ` [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly Ard Biesheuvel
@ 2016-09-15 13:42 ` Laszlo Ersek
0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2016-09-15 13:42 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
On 09/15/16 15:30, Ard Biesheuvel wrote:
> The FDT client protocol methods dealing with "reg" properties return
> the size of a 'reg' element. Currently, we have hardcoded this as '8',
> since #address-cells == #size-cells == 2 in most cases. However, for
> different values, have a single 'reg' element size is not unambiguous,
> since - however unlikely - if #address-cells != #size-cells, we do not
> know which is which.
>
> So before adding more methods to the protocol, fix up this oversight.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 6 ++++--
> ArmVirtPkg/Include/Protocol/FdtClient.h | 3 ++-
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 9 ++++++---
> ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 9 ++++++---
> ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c | 8 ++++++--
> ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c | 10 +++++++---
> 6 files changed, 31 insertions(+), 14 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes
2016-09-15 13:30 [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
2016-09-15 13:30 ` [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties Ard Biesheuvel
2016-09-15 13:30 ` [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly Ard Biesheuvel
@ 2016-09-15 13:30 ` Ard Biesheuvel
2016-09-15 13:57 ` Laszlo Ersek
2016-09-15 13:30 ` [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol Ard Biesheuvel
2016-09-15 13:34 ` [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 13:30 UTC (permalink / raw)
To: edk2-devel, lersek; +Cc: zhaoshenglong, sakar.arora, Ard Biesheuvel
Add high level methods to iterate over all 'reg' properties of all DT
nodes whose device_type properties have the value "memory". Since we are
modifying the FdtClient protocol, update the protocol and the only existing
implementation at the same time.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 76 ++++++++++++++++++++
ArmVirtPkg/Include/Protocol/FdtClient.h | 26 +++++++
2 files changed, 102 insertions(+)
diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 382e9af6bf84..099cce7821d6 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -194,6 +194,80 @@ FindCompatibleNodeReg (
STATIC
EFI_STATUS
+EFIAPI
+FindNextMemoryNodeReg (
+ IN FDT_CLIENT_PROTOCOL *This,
+ IN INT32 PrevNode,
+ OUT INT32 *Node,
+ OUT CONST VOID **Reg,
+ OUT UINTN *AddressCells,
+ OUT UINTN *SizeCells,
+ OUT UINT32 *RegSize
+ )
+{
+ INT32 Prev, Next;
+ CONST CHAR8 *DeviceType;
+ INT32 Len;
+ EFI_STATUS Status;
+
+ ASSERT (mDeviceTreeBase != NULL);
+ ASSERT (Node != NULL);
+
+ for (Prev = PrevNode;; Prev = Next) {
+ Next = fdt_next_node (mDeviceTreeBase, Prev, NULL);
+ if (Next < 0) {
+ break;
+ }
+
+ DeviceType = fdt_getprop (mDeviceTreeBase, Next, "device_type", &Len);
+ if (DeviceType != NULL && AsciiStrCmp (DeviceType, "memory") == 0) {
+
+ //
+ // Get the 'reg' property of this memory node. For now, we will assume
+ // 8 byte quantities for base and size, respectively.
+ // TODO use #cells root properties instead
+ //
+ Status = GetNodeProperty (This, Next, "reg", Reg, RegSize);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((EFI_D_WARN,
+ "%a: ignoring memory node with no 'reg' property\n",
+ __FUNCTION__));
+ continue;
+ }
+ if ((*RegSize % 16) != 0) {
+ DEBUG ((EFI_D_WARN,
+ "%a: ignoring memory node with invalid 'reg' property (size == 0x%x)\n",
+ __FUNCTION__, RegSize));
+ continue;
+ }
+
+ *Node = Next;
+ *AddressCells = 2;
+ *SizeCells = 2;
+ return EFI_SUCCESS;
+ }
+ }
+ return EFI_NOT_FOUND;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+FindMemoryNodeReg (
+ IN FDT_CLIENT_PROTOCOL *This,
+ OUT INT32 *Node,
+ OUT CONST VOID **Reg,
+ OUT UINTN *AddressCells,
+ OUT UINTN *SizeCells,
+ OUT UINT32 *RegSize
+ )
+{
+ return FindNextMemoryNodeReg (This, 0, Node, Reg, AddressCells, SizeCells,
+ RegSize);
+}
+
+STATIC
+EFI_STATUS
GetOrInsertChosenNode (
IN FDT_CLIENT_PROTOCOL *This,
OUT INT32 *Node
@@ -225,6 +299,8 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
FindNextCompatibleNode,
FindCompatibleNodeProperty,
FindCompatibleNodeReg,
+ FindMemoryNodeReg,
+ FindNextMemoryNodeReg,
GetOrInsertChosenNode,
};
diff --git a/ArmVirtPkg/Include/Protocol/FdtClient.h b/ArmVirtPkg/Include/Protocol/FdtClient.h
index b593c7441426..aad76db388be 100644
--- a/ArmVirtPkg/Include/Protocol/FdtClient.h
+++ b/ArmVirtPkg/Include/Protocol/FdtClient.h
@@ -87,6 +87,29 @@ EFI_STATUS
typedef
EFI_STATUS
+(EFIAPI *FDT_CLIENT_FIND_NEXT_MEMORY_NODE_REG) (
+ IN FDT_CLIENT_PROTOCOL *This,
+ IN INT32 PrevNode,
+ OUT INT32 *Node,
+ OUT CONST VOID **Reg,
+ OUT UINTN *AddressCells,
+ OUT UINTN *SizeCells,
+ OUT UINT32 *RegSize
+ );
+
+typedef
+EFI_STATUS
+(EFIAPI *FDT_CLIENT_FIND_MEMORY_NODE_REG) (
+ IN FDT_CLIENT_PROTOCOL *This,
+ OUT INT32 *Node,
+ OUT CONST VOID **Reg,
+ OUT UINTN *AddressCells,
+ OUT UINTN *SizeCells,
+ OUT UINT32 *RegSize
+ );
+
+typedef
+EFI_STATUS
(EFIAPI *FDT_CLIENT_GET_OR_INSERT_CHOSEN_NODE) (
IN FDT_CLIENT_PROTOCOL *This,
OUT INT32 *Node
@@ -101,6 +124,9 @@ struct _FDT_CLIENT_PROTOCOL {
FDT_CLIENT_FIND_COMPATIBLE_NODE_PROPERTY FindCompatibleNodeProperty;
FDT_CLIENT_FIND_COMPATIBLE_NODE_REG FindCompatibleNodeReg;
+ FDT_CLIENT_FIND_MEMORY_NODE_REG FindMemoryNodeReg;
+ FDT_CLIENT_FIND_NEXT_MEMORY_NODE_REG FindNextMemoryNodeReg;
+
FDT_CLIENT_GET_OR_INSERT_CHOSEN_NODE GetOrInsertChosenNode;
};
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes
2016-09-15 13:30 ` [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes Ard Biesheuvel
@ 2016-09-15 13:57 ` Laszlo Ersek
2016-09-15 14:04 ` Ard Biesheuvel
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2016-09-15 13:57 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
On 09/15/16 15:30, Ard Biesheuvel wrote:
> Add high level methods to iterate over all 'reg' properties of all DT
> nodes whose device_type properties have the value "memory". Since we are
> modifying the FdtClient protocol, update the protocol and the only existing
> implementation at the same time.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 76 ++++++++++++++++++++
> ArmVirtPkg/Include/Protocol/FdtClient.h | 26 +++++++
> 2 files changed, 102 insertions(+)
>
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 382e9af6bf84..099cce7821d6 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -194,6 +194,80 @@ FindCompatibleNodeReg (
>
> STATIC
> EFI_STATUS
> +EFIAPI
> +FindNextMemoryNodeReg (
> + IN FDT_CLIENT_PROTOCOL *This,
> + IN INT32 PrevNode,
> + OUT INT32 *Node,
> + OUT CONST VOID **Reg,
> + OUT UINTN *AddressCells,
> + OUT UINTN *SizeCells,
> + OUT UINT32 *RegSize
> + )
> +{
> + INT32 Prev, Next;
> + CONST CHAR8 *DeviceType;
> + INT32 Len;
> + EFI_STATUS Status;
> +
> + ASSERT (mDeviceTreeBase != NULL);
> + ASSERT (Node != NULL);
> +
> + for (Prev = PrevNode;; Prev = Next) {
> + Next = fdt_next_node (mDeviceTreeBase, Prev, NULL);
> + if (Next < 0) {
> + break;
> + }
> +
> + DeviceType = fdt_getprop (mDeviceTreeBase, Next, "device_type", &Len);
> + if (DeviceType != NULL && AsciiStrCmp (DeviceType, "memory") == 0) {
HighMemDxe uses AsciiStrnCmp (Type, "memory", Len) here.
If we're sure that we're not looking "memory*" types here, then the
change is fine. Are we sure?
> +
The empty line is likely unintended.
> + //
> + // Get the 'reg' property of this memory node. For now, we will assume
> + // 8 byte quantities for base and size, respectively.
> + // TODO use #cells root properties instead
> + //
> + Status = GetNodeProperty (This, Next, "reg", Reg, RegSize);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_WARN,
> + "%a: ignoring memory node with no 'reg' property\n",
> + __FUNCTION__));
> + continue;
> + }
> + if ((*RegSize % 16) != 0) {
> + DEBUG ((EFI_D_WARN,
> + "%a: ignoring memory node with invalid 'reg' property (size == 0x%x)\n",
> + __FUNCTION__, RegSize));
This should be *RegSize.
With the above confirmed / fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes
2016-09-15 13:57 ` Laszlo Ersek
@ 2016-09-15 14:04 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 14:04 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Shannon Zhao, Sakar Arora
On 15 September 2016 at 14:57, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/15/16 15:30, Ard Biesheuvel wrote:
>> Add high level methods to iterate over all 'reg' properties of all DT
>> nodes whose device_type properties have the value "memory". Since we are
>> modifying the FdtClient protocol, update the protocol and the only existing
>> implementation at the same time.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 76 ++++++++++++++++++++
>> ArmVirtPkg/Include/Protocol/FdtClient.h | 26 +++++++
>> 2 files changed, 102 insertions(+)
>>
>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> index 382e9af6bf84..099cce7821d6 100644
>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> @@ -194,6 +194,80 @@ FindCompatibleNodeReg (
>>
>> STATIC
>> EFI_STATUS
>> +EFIAPI
>> +FindNextMemoryNodeReg (
>> + IN FDT_CLIENT_PROTOCOL *This,
>> + IN INT32 PrevNode,
>> + OUT INT32 *Node,
>> + OUT CONST VOID **Reg,
>> + OUT UINTN *AddressCells,
>> + OUT UINTN *SizeCells,
>> + OUT UINT32 *RegSize
>> + )
>> +{
>> + INT32 Prev, Next;
>> + CONST CHAR8 *DeviceType;
>> + INT32 Len;
>> + EFI_STATUS Status;
>> +
>> + ASSERT (mDeviceTreeBase != NULL);
>> + ASSERT (Node != NULL);
>> +
>> + for (Prev = PrevNode;; Prev = Next) {
>> + Next = fdt_next_node (mDeviceTreeBase, Prev, NULL);
>> + if (Next < 0) {
>> + break;
>> + }
>> +
>> + DeviceType = fdt_getprop (mDeviceTreeBase, Next, "device_type", &Len);
>> + if (DeviceType != NULL && AsciiStrCmp (DeviceType, "memory") == 0) {
>
> HighMemDxe uses AsciiStrnCmp (Type, "memory", Len) here.
>
> If we're sure that we're not looking "memory*" types here, then the
> change is fine. Are we sure?
>
Actually, that is a bug. AsciiStrCmp () is guaranteed to check the NUL
terminator, and so it will only match on "memory\0". Using
AsciiStrnCmp() with the length of the property value is guaranteed
*not* to check the NUL terminator, so it may match on prefixes of
"memory\0"
>> +
>
> The empty line is likely unintended.
>
indeed.
>> + //
>> + // Get the 'reg' property of this memory node. For now, we will assume
>> + // 8 byte quantities for base and size, respectively.
>> + // TODO use #cells root properties instead
>> + //
>> + Status = GetNodeProperty (This, Next, "reg", Reg, RegSize);
>> + if (EFI_ERROR (Status)) {
>> + DEBUG ((EFI_D_WARN,
>> + "%a: ignoring memory node with no 'reg' property\n",
>> + __FUNCTION__));
>> + continue;
>> + }
>> + if ((*RegSize % 16) != 0) {
>> + DEBUG ((EFI_D_WARN,
>> + "%a: ignoring memory node with invalid 'reg' property (size == 0x%x)\n",
>> + __FUNCTION__, RegSize));
>
> This should be *RegSize.
>
OK
> With the above confirmed / fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol
2016-09-15 13:30 [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
` (2 preceding siblings ...)
2016-09-15 13:30 ` [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes Ard Biesheuvel
@ 2016-09-15 13:30 ` Ard Biesheuvel
2016-09-15 14:15 ` Laszlo Ersek
2016-09-15 13:34 ` [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 13:30 UTC (permalink / raw)
To: edk2-devel, lersek; +Cc: zhaoshenglong, sakar.arora, Ard Biesheuvel
Use the FDT client protocol rather than parsing the DT directly using
fdtlib. While we're at it, update the code so it deals correctly with
memory nodes that describe multiple disjoint regions in their "reg"
properties, and make the code work with #address-cells/#size-cells
properties of <1> as well as <2>.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/HighMemDxe/HighMemDxe.c | 120 +++++++++-----------
ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 16 ++-
2 files changed, 62 insertions(+), 74 deletions(-)
diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
index 4d56e6236b54..08de3cbb7e9c 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
@@ -1,7 +1,7 @@
/** @file
* High memory node enumeration DXE driver for ARM Virtual Machines
*
-* Copyright (c) 2015, Linaro Ltd. All rights reserved.
+* Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
*
* This program and the accompanying materials are licensed and made available
* under the terms and conditions of the BSD License which accompanies this
@@ -15,12 +15,12 @@
**/
#include <Library/BaseLib.h>
-#include <Library/UefiDriverEntryPoint.h>
#include <Library/DebugLib.h>
-#include <Library/PcdLib.h>
-#include <Library/HobLib.h>
-#include <libfdt.h>
#include <Library/DxeServicesTableLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/FdtClient.h>
EFI_STATUS
EFIAPI
@@ -29,76 +29,66 @@ InitializeHighMemDxe (
IN EFI_SYSTEM_TABLE *SystemTable
)
{
- VOID *Hob;
- VOID *DeviceTreeBase;
- INT32 Node, Prev;
- EFI_STATUS Status;
- CONST CHAR8 *Type;
- INT32 Len;
- CONST VOID *RegProp;
- UINT64 CurBase;
- UINT64 CurSize;
-
- Hob = GetFirstGuidHob(&gFdtHobGuid);
- if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
- return EFI_NOT_FOUND;
- }
- DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob);
-
- if (fdt_check_header (DeviceTreeBase) != 0) {
- DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__,
- DeviceTreeBase));
- return EFI_NOT_FOUND;
- }
+ FDT_CLIENT_PROTOCOL *FdtClient;
+ EFI_STATUS Status, FindNodeStatus;
+ INT32 Node;
+ CONST UINT32 *Reg;
+ UINT32 RegSize;
+ UINTN AddressCells, SizeCells;
+ UINT64 CurBase;
+ UINT64 CurSize;
- DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
+ Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
+ (VOID **)&FdtClient);
+ ASSERT_EFI_ERROR (Status);
//
- // Check for memory node and add the memory spaces expect the lowest one
+ // Check for memory node and add the memory spaces except the lowest one
//
- for (Prev = 0;; Prev = Node) {
- Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
- if (Node < 0) {
- break;
- }
+ for (FindNodeStatus = FdtClient->FindMemoryNodeReg (FdtClient, &Node,
+ (CONST VOID **) &Reg, &AddressCells,
+ &SizeCells, &RegSize);
+ !EFI_ERROR (FindNodeStatus);
+ FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
+ &Node, (CONST VOID **) &Reg, &AddressCells,
+ &SizeCells, &RegSize)) {
+
+ ASSERT (AddressCells <= 2);
+ ASSERT (SizeCells <= 2);
- 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 != NULL && Len == (2 * sizeof (UINT64))) {
+ while (RegSize > 0) {
- CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
- CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
+ CurBase = SwapBytes32 (*Reg++);
+ if (AddressCells > 1) {
+ CurBase |= (CurBase << 32) | SwapBytes32 (*Reg++);
+ }
+ CurSize = SwapBytes32 (*Reg++);
+ if (SizeCells > 1) {
+ CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++);
+ }
+ RegSize -= (AddressCells + SizeCells) * sizeof (UINT32);
- if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {
- Status = gDS->AddMemorySpace (
- EfiGcdMemoryTypeSystemMemory,
- CurBase, CurSize,
- EFI_MEMORY_WB);
+ if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {
+ Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, CurBase,
+ CurSize, EFI_MEMORY_WB);
- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR,
- "%a: Failed to add System RAM @ 0x%lx - 0x%lx (%r)\n",
- __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
- continue;
- }
+ if (EFI_ERROR (Status)) {
+ DEBUG ((EFI_D_ERROR,
+ "%a: Failed to add System RAM @ 0x%lx - 0x%lx (%r)\n",
+ __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
+ continue;
+ }
- Status = gDS->SetMemorySpaceAttributes (
- CurBase, CurSize,
- EFI_MEMORY_WB);
+ Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,
+ EFI_MEMORY_WB);
- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR,
- "%a: Failed to set System RAM @ 0x%lx - 0x%lx attribute (%r)\n",
- __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
- } else {
- DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
- __FUNCTION__, CurBase, CurBase + CurSize - 1));
- }
+ if (EFI_ERROR (Status)) {
+ DEBUG ((EFI_D_ERROR,
+ "%a: Failed to set System RAM @ 0x%lx - 0x%lx attribute (%r)\n",
+ __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
+ } else {
+ DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
+ __FUNCTION__, CurBase, CurBase + CurSize - 1));
}
}
}
diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
index ae632a8bee93..3661cfd8c80c 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
@@ -1,7 +1,7 @@
## @file
# High memory node enumeration DXE driver for ARM Virtual Machines
#
-# Copyright (c) 2015, Linaro Ltd. All rights reserved.
+# Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
#
# This program and the accompanying materials are licensed and made available
# under the terms and conditions of the BSD License which accompanies this
@@ -30,23 +30,21 @@ [Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
ArmPkg/ArmPkg.dec
- ArmPlatformPkg/ArmPlatformPkg.dec
ArmVirtPkg/ArmVirtPkg.dec
- EmbeddedPkg/EmbeddedPkg.dec
[LibraryClasses]
BaseLib
+ DebugLib
+ DxeServicesTableLib
PcdLib
+ UefiBootServicesTableLib
UefiDriverEntryPoint
- FdtLib
- HobLib
- DxeServicesTableLib
-[Guids]
- gFdtHobGuid
+[Protocols]
+ gFdtClientProtocolGuid ## CONSUMES
[Pcd]
gArmTokenSpaceGuid.PcdSystemMemoryBase
[Depex]
- gEfiCpuArchProtocolGuid
+ gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol
2016-09-15 13:30 ` [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol Ard Biesheuvel
@ 2016-09-15 14:15 ` Laszlo Ersek
2016-09-15 14:18 ` Ard Biesheuvel
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2016-09-15 14:15 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
On 09/15/16 15:30, Ard Biesheuvel wrote:
> Use the FDT client protocol rather than parsing the DT directly using
> fdtlib. While we're at it, update the code so it deals correctly with
> memory nodes that describe multiple disjoint regions in their "reg"
> properties, and make the code work with #address-cells/#size-cells
> properties of <1> as well as <2>.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/HighMemDxe/HighMemDxe.c | 120 +++++++++-----------
> ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 16 ++-
> 2 files changed, 62 insertions(+), 74 deletions(-)
>
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> index 4d56e6236b54..08de3cbb7e9c 100644
> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> @@ -1,7 +1,7 @@
> /** @file
> * High memory node enumeration DXE driver for ARM Virtual Machines
> *
> -* Copyright (c) 2015, Linaro Ltd. All rights reserved.
> +* Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
> *
> * This program and the accompanying materials are licensed and made available
> * under the terms and conditions of the BSD License which accompanies this
> @@ -15,12 +15,12 @@
> **/
>
> #include <Library/BaseLib.h>
> -#include <Library/UefiDriverEntryPoint.h>
> #include <Library/DebugLib.h>
> -#include <Library/PcdLib.h>
> -#include <Library/HobLib.h>
> -#include <libfdt.h>
> #include <Library/DxeServicesTableLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include <Protocol/FdtClient.h>
>
> EFI_STATUS
> EFIAPI
> @@ -29,76 +29,66 @@ InitializeHighMemDxe (
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> - VOID *Hob;
> - VOID *DeviceTreeBase;
> - INT32 Node, Prev;
> - EFI_STATUS Status;
> - CONST CHAR8 *Type;
> - INT32 Len;
> - CONST VOID *RegProp;
> - UINT64 CurBase;
> - UINT64 CurSize;
> -
> - Hob = GetFirstGuidHob(&gFdtHobGuid);
> - if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
> - return EFI_NOT_FOUND;
> - }
> - DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob);
> -
> - if (fdt_check_header (DeviceTreeBase) != 0) {
> - DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__,
> - DeviceTreeBase));
> - return EFI_NOT_FOUND;
> - }
> + FDT_CLIENT_PROTOCOL *FdtClient;
> + EFI_STATUS Status, FindNodeStatus;
> + INT32 Node;
> + CONST UINT32 *Reg;
> + UINT32 RegSize;
> + UINTN AddressCells, SizeCells;
> + UINT64 CurBase;
> + UINT64 CurSize;
>
> - DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
> + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> + (VOID **)&FdtClient);
> + ASSERT_EFI_ERROR (Status);
>
> //
> - // Check for memory node and add the memory spaces expect the lowest one
> + // Check for memory node and add the memory spaces except the lowest one
> //
> - for (Prev = 0;; Prev = Node) {
> - Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
> - if (Node < 0) {
> - break;
> - }
> + for (FindNodeStatus = FdtClient->FindMemoryNodeReg (FdtClient, &Node,
> + (CONST VOID **) &Reg, &AddressCells,
> + &SizeCells, &RegSize);
> + !EFI_ERROR (FindNodeStatus);
> + FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
> + &Node, (CONST VOID **) &Reg, &AddressCells,
> + &SizeCells, &RegSize)) {
> +
I think we can remove this empty line. Not too important of course.
> + ASSERT (AddressCells <= 2);
> + ASSERT (SizeCells <= 2);
>
> - 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 != NULL && Len == (2 * sizeof (UINT64))) {
> + while (RegSize > 0) {
>
This one too. (Also just a matter of taste.)
> - CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> - CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
> + CurBase = SwapBytes32 (*Reg++);
> + if (AddressCells > 1) {
> + CurBase |= (CurBase << 32) | SwapBytes32 (*Reg++);
The operator |= seems wrong; it should be just =.
(You could do { CurBase <<= 32; CurBase |= SwapBytes32 (*Reg++); }. The
above is more compact, but the operator should be fixed.)
> + }
> + CurSize = SwapBytes32 (*Reg++);
> + if (SizeCells > 1) {
> + CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++);
Same here.
With these fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol
2016-09-15 14:15 ` Laszlo Ersek
@ 2016-09-15 14:18 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 14:18 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Shannon Zhao, Sakar Arora
On 15 September 2016 at 15:15, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/15/16 15:30, Ard Biesheuvel wrote:
>> Use the FDT client protocol rather than parsing the DT directly using
>> fdtlib. While we're at it, update the code so it deals correctly with
>> memory nodes that describe multiple disjoint regions in their "reg"
>> properties, and make the code work with #address-cells/#size-cells
>> properties of <1> as well as <2>.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmVirtPkg/HighMemDxe/HighMemDxe.c | 120 +++++++++-----------
>> ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 16 ++-
>> 2 files changed, 62 insertions(+), 74 deletions(-)
>>
>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> index 4d56e6236b54..08de3cbb7e9c 100644
>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> @@ -1,7 +1,7 @@
>> /** @file
>> * High memory node enumeration DXE driver for ARM Virtual Machines
>> *
>> -* Copyright (c) 2015, Linaro Ltd. All rights reserved.
>> +* Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
>> *
>> * This program and the accompanying materials are licensed and made available
>> * under the terms and conditions of the BSD License which accompanies this
>> @@ -15,12 +15,12 @@
>> **/
>>
>> #include <Library/BaseLib.h>
>> -#include <Library/UefiDriverEntryPoint.h>
>> #include <Library/DebugLib.h>
>> -#include <Library/PcdLib.h>
>> -#include <Library/HobLib.h>
>> -#include <libfdt.h>
>> #include <Library/DxeServicesTableLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#include <Protocol/FdtClient.h>
>>
>> EFI_STATUS
>> EFIAPI
>> @@ -29,76 +29,66 @@ InitializeHighMemDxe (
>> IN EFI_SYSTEM_TABLE *SystemTable
>> )
>> {
>> - VOID *Hob;
>> - VOID *DeviceTreeBase;
>> - INT32 Node, Prev;
>> - EFI_STATUS Status;
>> - CONST CHAR8 *Type;
>> - INT32 Len;
>> - CONST VOID *RegProp;
>> - UINT64 CurBase;
>> - UINT64 CurSize;
>> -
>> - Hob = GetFirstGuidHob(&gFdtHobGuid);
>> - if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>> - return EFI_NOT_FOUND;
>> - }
>> - DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob);
>> -
>> - if (fdt_check_header (DeviceTreeBase) != 0) {
>> - DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__,
>> - DeviceTreeBase));
>> - return EFI_NOT_FOUND;
>> - }
>> + FDT_CLIENT_PROTOCOL *FdtClient;
>> + EFI_STATUS Status, FindNodeStatus;
>> + INT32 Node;
>> + CONST UINT32 *Reg;
>> + UINT32 RegSize;
>> + UINTN AddressCells, SizeCells;
>> + UINT64 CurBase;
>> + UINT64 CurSize;
>>
>> - DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
>> + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
>> + (VOID **)&FdtClient);
>> + ASSERT_EFI_ERROR (Status);
>>
>> //
>> - // Check for memory node and add the memory spaces expect the lowest one
>> + // Check for memory node and add the memory spaces except the lowest one
>> //
>> - for (Prev = 0;; Prev = Node) {
>> - Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
>> - if (Node < 0) {
>> - break;
>> - }
>> + for (FindNodeStatus = FdtClient->FindMemoryNodeReg (FdtClient, &Node,
>> + (CONST VOID **) &Reg, &AddressCells,
>> + &SizeCells, &RegSize);
>> + !EFI_ERROR (FindNodeStatus);
>> + FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
>> + &Node, (CONST VOID **) &Reg, &AddressCells,
>> + &SizeCells, &RegSize)) {
>> +
>
> I think we can remove this empty line. Not too important of course.
>
Sure
>> + ASSERT (AddressCells <= 2);
>> + ASSERT (SizeCells <= 2);
>>
>> - 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 != NULL && Len == (2 * sizeof (UINT64))) {
>> + while (RegSize > 0) {
>>
>
> This one too. (Also just a matter of taste.)
>
If you prefer, I don't care either way.
>> - CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> - CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>> + CurBase = SwapBytes32 (*Reg++);
>> + if (AddressCells > 1) {
>> + CurBase |= (CurBase << 32) | SwapBytes32 (*Reg++);
>
> The operator |= seems wrong; it should be just =.
>
> (You could do { CurBase <<= 32; CurBase |= SwapBytes32 (*Reg++); }. The
> above is more compact, but the operator should be fixed.)
>
You are right. I got away with it due to the fact that I tested with
base/size pairs <= 4GB, and so the leading cell == 0 in all cases.
Thanks for spotting that!
>> + }
>> + CurSize = SwapBytes32 (*Reg++);
>> + if (SizeCells > 1) {
>> + CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++);
>
> Same here.
>
> With these fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
OK, thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates
2016-09-15 13:30 [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
` (3 preceding siblings ...)
2016-09-15 13:30 ` [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol Ard Biesheuvel
@ 2016-09-15 13:34 ` Ard Biesheuvel
2016-09-15 14:40 ` Ard Biesheuvel
4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 13:34 UTC (permalink / raw)
To: edk2-devel-01, Laszlo Ersek; +Cc: Shannon Zhao, Sakar Arora, Ard Biesheuvel
On 15 September 2016 at 14:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Patch #4 moves HighMemDxe to the FDT client protocol, and updates it to
> handle #address-cells/#size-cells values of <1> as well as <2>, and lets
> it deal with memory nodes whose 'reg' properties describe multiple disjoint
> regions.
>
> Patches #1 to #3 are somewhat preparatory in nature:
> - Patch #1 is a small thinko fix
> - Patch #2 drops the 'regelemsize' output parameter for 'reg' related methods
> of the FDT client protocol in favour of addresscells/sizecells.
> - Patch #3 adds methods to iterate over all 'reg' properties of all memory nodes
>
Branch here
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/virt-highmem-fdt
> Ard Biesheuvel (4):
> ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
> ArmVirtPkg/FdtClientDxe: report address and size cell count directly
> ArmVirtPkg/FdtClient: add methods to iterate over memory nodes
> ArmVirtPkg/HighMemDxe: move to FDT client protocol
>
> ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 84 +++++++++++++-
> ArmVirtPkg/HighMemDxe/HighMemDxe.c | 120 +++++++++-----------
> ArmVirtPkg/HighMemDxe/HighMemDxe.inf | 16 ++-
> ArmVirtPkg/Include/Protocol/FdtClient.h | 29 ++++-
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 9 +-
> ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 9 +-
> ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c | 8 +-
> ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c | 10 +-
> 8 files changed, 196 insertions(+), 89 deletions(-)
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates
2016-09-15 13:34 ` [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates Ard Biesheuvel
@ 2016-09-15 14:40 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2016-09-15 14:40 UTC (permalink / raw)
To: edk2-devel-01, Laszlo Ersek; +Cc: Shannon Zhao, Sakar Arora, Ard Biesheuvel
On 15 September 2016 at 14:34, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 15 September 2016 at 14:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Patch #4 moves HighMemDxe to the FDT client protocol, and updates it to
>> handle #address-cells/#size-cells values of <1> as well as <2>, and lets
>> it deal with memory nodes whose 'reg' properties describe multiple disjoint
>> regions.
>>
>> Patches #1 to #3 are somewhat preparatory in nature:
>> - Patch #1 is a small thinko fix
>> - Patch #2 drops the 'regelemsize' output parameter for 'reg' related methods
>> of the FDT client protocol in favour of addresscells/sizecells.
>> - Patch #3 adds methods to iterate over all 'reg' properties of all memory nodes
>>
Committed as
38ed4a9e3a65 ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
cfc8d51c0cbf ArmVirtPkg/FdtClientDxe: report address and size cell
count directly
969d2eb3875a ArmVirtPkg/FdtClientDxe: add methods to iterate over memory nodes
490acf8908f7 ArmVirtPkg/HighMemDxe: move to FDT client protocol
with your comments addressed.
Thanks,
Ard,
^ permalink raw reply [flat|nested] 14+ messages in thread