[edk2] Protocol EFI_FORM_BROWSER_EXTENSION_PROTOCOL - included in UDK code but not part of the spec

2016-09-15 Thread Boaz Kahana 
The protocol EFI_FORM_BROWSER_EXTENSION_PROTOCOL is defined in 
MdeModulePkg\Include\Protocol\FormBrowserEx.h with EFI_ prefix although it is 
not part of the UEFI spec.
(It was added by Intel - Liming - in Sep 2011)

IMHO it should be added to the UEFI spec otherwise it should not use the EFI_ 
prefix.

Boaz

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

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific

2016-09-15 Thread Ard Biesheuvel 
On 15 September 2016 at 16:48, Andrew Fish  wrote:
>
>> On Sep 15, 2016, at 8:28 AM, Leif Lindholm  wrote:
>>
>> On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote:
>>> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to
>>> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option
>>> ROM on the same PCI controller.
>>>
>>> This quirk is highly specific to not just the X64 architecture in general,
>>> but to the PC platform in particular, given that only X64 platforms that
>>> require legacy PC BIOS compatibility require it. However, making the
>>> quirk dependent on the presence of the legacy BIOS protocol met with
>>> resistance, due to the fact that it introduces a dependency on the
>>> IntelFrameworkModulePkg package.
>>>
>>> So instead, make the quirk dependent on whether we are compiling for the
>>> X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor
>>> directives around it.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel 
>>
>> I'm happy with the change, but it may be worth putting a comment in
>
> Do we have to use an #ifdef? Seems like it should be a PCD. Is this for 
> legacy BIOS Option ROM support, or for existing EFI ROMs?
>

Strictly for being able to execute a legacy BIOS Option ROM in the
context of a CSM. Hence my assertion that this is specific to the
64-bit variant of the PC platform.

Getting new PCDs introduced is also met with fierce resistance, to the
extent that (as this code testifies), we'd rather keep quirks hidden
in the code like this, and enable them by default rather than
acknowledging their existence by adding explicit controls to
enable/disable them.

Others have suggested a dynamic PCD, which can be flicked when the
legacy BIOS protocol installed. But since X64 is not my primary
interested, I will settle for a feature PCD, and #ifdef or anything
else that results in this quirk to be disabled by default on 64-bit
ARM.

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

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific

2016-09-15 Thread Brian J. Johnson 

On 09/15/2016 10:48 AM, Andrew Fish wrote:



On Sep 15, 2016, at 8:28 AM, Leif Lindholm  wrote:

On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote:

The 'universal' PCI bus driver in MdeModulePkg contains a quirk to
degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option
ROM on the same PCI controller.

This quirk is highly specific to not just the X64 architecture in general,
but to the PC platform in particular, given that only X64 platforms that
require legacy PC BIOS compatibility require it. However, making the
quirk dependent on the presence of the legacy BIOS protocol met with
resistance, due to the fact that it introduces a dependency on the
IntelFrameworkModulePkg package.

So instead, make the quirk dependent on whether we are compiling for the
X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor
directives around it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 


I'm happy with the change, but it may be worth putting a comment in


Do we have to use an #ifdef? Seems like it should be a PCD. Is this for legacy 
BIOS Option ROM support, or for existing EFI ROMs?

Thanks,

Andrew Fish


I was going to say the same thing... this sounds like exactly the sort 
of thing PCDs are meant for.  And as Leif's comment says, not all X64 
platforms have legacy support or want BAR degradation (ours don't.)


Brian Johnson




the code to the extent that:
---
In order to reliably support legacy Option ROMs, which may not be
64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use
only the lower 32 bits.
Other architectures, or indeed X64 without legacy option ROM support,
can safely leave these in normal operation mode (and may require so in
order to function).
---

For what it's worth:
Reviewed-by: Leif Lindholm 


---

Since nobody proposed any alternatives to the solution I proposed,
let's just make the quirk disappear on architectures that have no
use for it.

Note that forcing non-X64 architectures to install a driver that
produces the IncompatiblePciDevice protocol only to make the PCI
bus driver behave normally (and which, incidentally, can only be
produced once per platform, making it difficult to provide a default
implementation that can be widely reused) is not acceptable IMO.

If anything, this should require an IncompatiblePlatform protocol
that informs the PCI bus driver that a special quirk is required,
but I am aware that this is difficult to accomplish without breaking
backward compatibility. Hence this compromise.

MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index b0632d53b82b..b4771a822d65 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -1052,6 +1052,7 @@ DegradeResource (
  IN PCI_RESOURCE_NODE *PMem64Node
  )
{
+#ifdef MDE_CPU_X64
  PCI_IO_DEVICE*PciIoDevice;
  LIST_ENTRY   *ChildDeviceLink;
  LIST_ENTRY   *ChildNodeLink;
@@ -1101,6 +1102,7 @@ DegradeResource (
}
ChildDeviceLink = ChildDeviceLink->ForwardLink;
  }
+#endif

  //
  // If firmware is in 32-bit mode,
--
2.7.4


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


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




--

  My statements are my own, are not authorized by SGI, and do not
  necessarily represent SGI’s positions.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific

2016-09-15 Thread Andrew Fish 

> On Sep 15, 2016, at 8:28 AM, Leif Lindholm  wrote:
> 
> On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote:
>> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to
>> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option
>> ROM on the same PCI controller.
>> 
>> This quirk is highly specific to not just the X64 architecture in general,
>> but to the PC platform in particular, given that only X64 platforms that
>> require legacy PC BIOS compatibility require it. However, making the
>> quirk dependent on the presence of the legacy BIOS protocol met with
>> resistance, due to the fact that it introduces a dependency on the
>> IntelFrameworkModulePkg package.
>> 
>> So instead, make the quirk dependent on whether we are compiling for the
>> X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor
>> directives around it.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
> 
> I'm happy with the change, but it may be worth putting a comment in

Do we have to use an #ifdef? Seems like it should be a PCD. Is this for legacy 
BIOS Option ROM support, or for existing EFI ROMs?

Thanks,

Andrew Fish

> the code to the extent that:
> ---
> In order to reliably support legacy Option ROMs, which may not be
> 64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use
> only the lower 32 bits.
> Other architectures, or indeed X64 without legacy option ROM support,
> can safely leave these in normal operation mode (and may require so in
> order to function).
> ---
> 
> For what it's worth:
> Reviewed-by: Leif Lindholm 
> 
>> ---
>> 
>> Since nobody proposed any alternatives to the solution I proposed,
>> let's just make the quirk disappear on architectures that have no
>> use for it.
>> 
>> Note that forcing non-X64 architectures to install a driver that
>> produces the IncompatiblePciDevice protocol only to make the PCI
>> bus driver behave normally (and which, incidentally, can only be
>> produced once per platform, making it difficult to provide a default
>> implementation that can be widely reused) is not acceptable IMO.
>> 
>> If anything, this should require an IncompatiblePlatform protocol
>> that informs the PCI bus driver that a special quirk is required,
>> but I am aware that this is difficult to accomplish without breaking
>> backward compatibility. Hence this compromise.
>> 
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c 
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> index b0632d53b82b..b4771a822d65 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> @@ -1052,6 +1052,7 @@ DegradeResource (
>>   IN PCI_RESOURCE_NODE *PMem64Node
>>   )
>> {
>> +#ifdef MDE_CPU_X64
>>   PCI_IO_DEVICE*PciIoDevice;
>>   LIST_ENTRY   *ChildDeviceLink;
>>   LIST_ENTRY   *ChildNodeLink;
>> @@ -1101,6 +1102,7 @@ DegradeResource (
>> }
>> ChildDeviceLink = ChildDeviceLink->ForwardLink;
>>   }
>> +#endif
>> 
>>   //
>>   // If firmware is in 32-bit mode,
>> -- 
>> 2.7.4
>> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific

2016-09-15 Thread Leif Lindholm 
On Thu, Sep 15, 2016 at 03:54:56PM +0100, Ard Biesheuvel wrote:
> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to
> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option
> ROM on the same PCI controller.
> 
> This quirk is highly specific to not just the X64 architecture in general,
> but to the PC platform in particular, given that only X64 platforms that
> require legacy PC BIOS compatibility require it. However, making the
> quirk dependent on the presence of the legacy BIOS protocol met with
> resistance, due to the fact that it introduces a dependency on the
> IntelFrameworkModulePkg package.
> 
> So instead, make the quirk dependent on whether we are compiling for the
> X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor
> directives around it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 

I'm happy with the change, but it may be worth putting a comment in
the code to the extent that:
---
In order to reliably support legacy Option ROMs, which may not be
64-bit safe, X64 needs to ensure PCI MMIO BARs are forced down to use
only the lower 32 bits.
Other architectures, or indeed X64 without legacy option ROM support,
can safely leave these in normal operation mode (and may require so in
order to function).
---

For what it's worth:
Reviewed-by: Leif Lindholm 

> ---
> 
> Since nobody proposed any alternatives to the solution I proposed,
> let's just make the quirk disappear on architectures that have no
> use for it.
> 
> Note that forcing non-X64 architectures to install a driver that
> produces the IncompatiblePciDevice protocol only to make the PCI
> bus driver behave normally (and which, incidentally, can only be
> produced once per platform, making it difficult to provide a default
> implementation that can be widely reused) is not acceptable IMO.
> 
> If anything, this should require an IncompatiblePlatform protocol
> that informs the PCI bus driver that a special quirk is required,
> but I am aware that this is difficult to accomplish without breaking
> backward compatibility. Hence this compromise.
>  
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c 
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index b0632d53b82b..b4771a822d65 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -1052,6 +1052,7 @@ DegradeResource (
>IN PCI_RESOURCE_NODE *PMem64Node
>)
>  {
> +#ifdef MDE_CPU_X64
>PCI_IO_DEVICE*PciIoDevice;
>LIST_ENTRY   *ChildDeviceLink;
>LIST_ENTRY   *ChildNodeLink;
> @@ -1101,6 +1102,7 @@ DegradeResource (
>  }
>  ChildDeviceLink = ChildDeviceLink->ForwardLink;
>}
> +#endif
>  
>//
>// If firmware is in 32-bit mode,
> -- 
> 2.7.4
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] MdeModulePkg/PciBusDxe: make OPROM BAR degradation X64 specific

2016-09-15 Thread Ard Biesheuvel 
The 'universal' PCI bus driver in MdeModulePkg contains a quirk to
degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option
ROM on the same PCI controller.

This quirk is highly specific to not just the X64 architecture in general,
but to the PC platform in particular, given that only X64 platforms that
require legacy PC BIOS compatibility require it. However, making the
quirk dependent on the presence of the legacy BIOS protocol met with
resistance, due to the fact that it introduces a dependency on the
IntelFrameworkModulePkg package.

So instead, make the quirk dependent on whether we are compiling for the
X64 architecture, by putting the #ifdef MDE_CPU_X64/#endif preprocessor
directives around it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---

Since nobody proposed any alternatives to the solution I proposed,
let's just make the quirk disappear on architectures that have no
use for it.

Note that forcing non-X64 architectures to install a driver that
produces the IncompatiblePciDevice protocol only to make the PCI
bus driver behave normally (and which, incidentally, can only be
produced once per platform, making it difficult to provide a default
implementation that can be widely reused) is not acceptable IMO.

If anything, this should require an IncompatiblePlatform protocol
that informs the PCI bus driver that a special quirk is required,
but I am aware that this is difficult to accomplish without breaking
backward compatibility. Hence this compromise.
 
 MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index b0632d53b82b..b4771a822d65 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -1052,6 +1052,7 @@ DegradeResource (
   IN PCI_RESOURCE_NODE *PMem64Node
   )
 {
+#ifdef MDE_CPU_X64
   PCI_IO_DEVICE*PciIoDevice;
   LIST_ENTRY   *ChildDeviceLink;
   LIST_ENTRY   *ChildNodeLink;
@@ -1101,6 +1102,7 @@ DegradeResource (
 }
 ChildDeviceLink = ChildDeviceLink->ForwardLink;
   }
+#endif
 
   //
   // If firmware is in 32-bit mode,
-- 
2.7.4

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

Re: [edk2] [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates

2016-09-15 Thread Ard Biesheuvel 
On 15 September 2016 at 14:34, Ard Biesheuvel  wrote:
> On 15 September 2016 at 14:30, Ard Biesheuvel  
> 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,
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol

2016-09-15 Thread Ard Biesheuvel 
On 15 September 2016 at 15:15, Laszlo Ersek  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 
>> ---
>>  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 
>> -#include 
>>  #include 
>> -#include 
>> -#include 
>> -#include 
>>  #include 
>> +#include 
>> +#include 
>> +
>> +#include 
>>
>>  EFI_STATUS
>>  EFIAPI
>> @@ -29,76 +29,66 @@ InitializeHighMemDxe (
>>IN EFI_SYSTEM_TABLE *SystemTable
>>)
>>  {
>> -  VOID *Hob;
>> -  VOID *DeviceTreeBase;
>> -  INT32Node, Prev;
>> -  EFI_STATUS   Status;
>> -  CONST CHAR8  *Type;
>> -  INT32Len;
>> -  CONST VOID   *RegProp;
>> -  UINT64   CurBase;
>> -  UINT64   CurSize;
>> -
>> -  Hob = GetFirstGuidHob();
>> -  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_STATUSStatus, FindNodeStatus;
>> +  INT32 Node;
>> +  CONST UINT32  *Reg;
>> +  UINT32RegSize;
>> +  UINTN AddressCells, SizeCells;
>> +  UINT64CurBase;
>> +  UINT64CurSize;
>>
>> -  DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
>> +  Status = gBS->LocateProtocol (, NULL,
>> +  (VOID **));
>> +  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, ,
>> + (CONST VOID **) , ,
>> + , );
>> +   !EFI_ERROR (FindNodeStatus);
>> +   FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
>> + , (CONST VOID **) , 
>> ,
>> + , )) {
>> +
>
> 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", );
>> -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 != 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!

>> +  }
>> +

Re: [edk2] [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol

2016-09-15 Thread Laszlo Ersek 
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 
> ---
>  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 
> -#include 
>  #include 
> -#include 
> -#include 
> -#include 
>  #include 
> +#include 
> +#include 
> +
> +#include 
>  
>  EFI_STATUS
>  EFIAPI
> @@ -29,76 +29,66 @@ InitializeHighMemDxe (
>IN EFI_SYSTEM_TABLE *SystemTable
>)
>  {
> -  VOID *Hob;
> -  VOID *DeviceTreeBase;
> -  INT32Node, Prev;
> -  EFI_STATUS   Status;
> -  CONST CHAR8  *Type;
> -  INT32Len;
> -  CONST VOID   *RegProp;
> -  UINT64   CurBase;
> -  UINT64   CurSize;
> -
> -  Hob = GetFirstGuidHob();
> -  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_STATUSStatus, FindNodeStatus;
> +  INT32 Node;
> +  CONST UINT32  *Reg;
> +  UINT32RegSize;
> +  UINTN AddressCells, SizeCells;
> +  UINT64CurBase;
> +  UINT64CurSize;
>  
> -  DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
> +  Status = gBS->LocateProtocol (, NULL,
> +  (VOID **));
> +  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, ,
> + (CONST VOID **) , ,
> + , );
> +   !EFI_ERROR (FindNodeStatus);
> +   FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
> + , (CONST VOID **) , 
> ,
> + , )) {
> +

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", );
> -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 != 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 

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

Re: [edk2] [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes

2016-09-15 Thread Ard Biesheuvel 
On 15 September 2016 at 14:57, Laszlo Ersek  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 
>> ---
>>  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", );
>> +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 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes

2016-09-15 Thread Laszlo Ersek 
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 
> ---
>  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", );
> +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 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly

2016-09-15 Thread Laszlo Ersek 
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 
> ---
>  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 

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

Re: [edk2] [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties

2016-09-15 Thread Ard Biesheuvel 
On 15 September 2016 at 14:38, Laszlo Ersek  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 
>

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

Re: [edk2] [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties

2016-09-15 Thread Laszlo Ersek 
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 

> of  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 
> ---
>  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));
> 

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

Re: [edk2] [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates

2016-09-15 Thread Ard Biesheuvel 
On 15 September 2016 at 14:30, Ard Biesheuvel  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
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly

2016-09-15 Thread 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 
---
 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 (
   UINT32IccSre;
   FDT_CLIENT_PROTOCOL   *FdtClient;
   CONST UINT64  *Reg;
-  UINT32RegElemSize, RegSize;
+  UINT32RegSize;
+  UINTN AddressCells, SizeCells;
   UINTN GicRevision;
   EFI_STATUSStatus;
   UINT64DistBase, CpuBase, RedistBase;
@@ -47,11 +48,13 @@ ArmVirtGicArchLibConstructor (
 
   GicRevision = 2;
   Status = FdtClient->FindCompatibleNodeReg (FdtClient, "arm,cortex-a15-gic",
-(CONST VOID **), , );
+(CONST VOID **), , ,
+);
   if (Status == EFI_NOT_FOUND) {
 GicRevision = 3;
 Status = FdtClient->FindCompatibleNodeReg (FdtClient, "arm,gic-v3",
-  (CONST VOID **), , );
+  (CONST VOID **), , ,
+  );
   }
   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_STATUSStatus;
   FDT_CLIENT_PROTOCOL   *FdtClient;
   CONST UINT64  *Reg;
-  UINT32RegElemSize, RegSize;
+  UINT32RegSize;
+  UINTN AddressCells, SizeCells;
   UINT64FwCfgSelectorAddress;
   UINT64FwCfgSelectorSize;
   UINT64FwCfgDataAddress;
@@ -135,7 +136,8 @@ QemuFwCfgInitialize (
   ASSERT_EFI_ERROR (Status);
 
   Status = FdtClient->FindCompatibleNodeReg (FdtClient, "qemu,fw-cfg-mmio",
- (CONST VOID **), , );
+ (CONST VOID **), , ,
+ );
   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

[edk2] [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties

2016-09-15 Thread Ard Biesheuvel 
Currently, the code in FdtClientDxe assumes #address-cells and
of  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 
---
 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

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

[edk2] [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates

2016-09-15 Thread Ard Biesheuvel 
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

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

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

Re: [edk2] SMM PciExpressLib Instance

2016-09-15 Thread Felix Poludov 
Mike,

I updated the library to be based on DxeRuntimePciExpressLib.
Here is the new patch.

Thanks,
Felix

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Felix Polyudov >
==
diff --git a/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c 
b/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c
new file mode 100644
index 000..1b88770
--- /dev/null
+++ b/MdePkg/Library/SmmPciExpressLib/PciExpressLib.c
@@ -0,0 +1,1431 @@
+/** @file
+  Functions in this library instance make use of MMIO functions in IoLib to
+  access memory mapped PCI configuration space.
+
+  All assertions for I/O operations are handled in MMIO functions in the IoLib
+  Library.
+
+  Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.
+  Portions copyright (c) 2016, American Megatrends, Inc. 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 distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php.
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+///
+/// Module global that contains the base physical address of the PCI Express 
MMIO range.
+///
+UINTN mSmmPciExpressLibPciExpressBaseAddress = 0;
+
+/**
+  The constructor function caches the PCI Express Base Address 
+  
+  @param  ImageHandle   The firmware allocated handle for the EFI image.
+  @param  SystemTable   A pointer to the EFI System Table.
+  
+  @retval EFI_SUCCESS   The constructor completed successfully.
+**/
+EFI_STATUS
+EFIAPI
+SmmPciExpressLibConstructor (
+  IN EFI_HANDLEImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  //
+  // Cache the physical address of the PCI Express MMIO range into a module 
global variable
+  //
+  mSmmPciExpressLibPciExpressBaseAddress = (UINTN) PcdGet64 
(PcdPciExpressBaseAddress);
+  
+  return EFI_SUCCESS;
+}
+
+/**
+  Assert the validity of a PCI address. A valid PCI address should contain 1's
+  only in the low 28 bits.
+
+  @param  A The address to validate.
+
+**/
+#define ASSERT_INVALID_PCI_ADDRESS(A) \
+  ASSERT (((A) & ~0xfff) == 0)
+
+/**
+  Registers a PCI device so PCI configuration registers may be accessed after 
+  SetVirtualAddressMap().
+  
+  Registers the PCI device specified by Address so all the PCI configuration 
+  registers associated with that PCI device may be accessed after 
SetVirtualAddressMap() 
+  is called.
+  
+  If Address > 0x0FFF, then ASSERT().
+
+  @param  Address The address that encodes the PCI Bus, Device, Function and
+  Register.
+  
+  @retval RETURN_SUCCESS   The PCI device was registered for runtime 
access.
+  @retval RETURN_UNSUPPORTED   An attempt was made to call this function 
+   after ExitBootServices().
+  @retval RETURN_UNSUPPORTED   The resources required to access the PCI 
device
+   at runtime could not be mapped.
+  @retval RETURN_OUT_OF_RESOURCES  There are not enough resources available to
+   complete the registration.
+
+**/
+RETURN_STATUS
+EFIAPI
+PciExpressRegisterForRuntimeAccess (
+  IN UINTN  Address
+  )
+{
+  ASSERT_INVALID_PCI_ADDRESS (Address);
+  return RETURN_UNSUPPORTED;
+}
+
+/**
+  Gets MMIO address that can be used to access PCI Express location defined by 
Address.
+  
+  This internal functions converts PCI Express address to a CPU MMIO address 
by adding
+  PCI Express Base Address stored in a global variable 
mSmmPciExpressLibPciExpressBaseAddress.
+  mSmmPciExpressLibPciExpressBaseAddress is initialized in the library 
constructor from PCD entry
+  PcdPciExpressBaseAddress.
+  
+  @param  Address  The address that encodes the PCI Bus, Device, Function and 
Register.
+  @return  MMIO address corresponding to Address.
+
+**/
+UINTN
+GetPciExpressAddress (
+  IN UINTN  Address
+  )
+{
+  //
+  // Make sure Address is valid
+  //
+  ASSERT_INVALID_PCI_ADDRESS (Address);
+  return mSmmPciExpressLibPciExpressBaseAddress + Address;
+}
+
+/**
+  Reads an 8-bit PCI configuration register.
+
+  Reads and returns the 8-bit PCI configuration register specified by Address.
+  This function must guarantee that all PCI read and write operations are
+  serialized.
+
+  If Address > 0x0FFF, then ASSERT().
+
+  @param  Address The address that encodes the PCI Bus, Device, Function and
+  Register.
+
+  @return The read value from the PCI configuration register.
+
+**/
+UINT8
+EFIAPI
+PciExpressRead8 (
+  IN  UINTN Address
+  )
+{
+  return MmioRead8 (GetPciExpressAddress (Address));
+}
+
+/**
+  Writes an 8-bit PCI configuration register.
+

Re: [edk2] Exporting discontiguous System Memory to EFI STUB

2016-09-15 Thread Ard Biesheuvel 
On 15 September 2016 at 10:01, Laszlo Ersek  wrote:
> On 09/15/16 10:45, Sakar Arora wrote:
>> Hi
>>
>> This is in aarch64 UEFI context.
>>
>> The efi stub code ignores any memory nodes in the device tree. It
>> only relies on the UEFI memory map for memory info.
>>
>> In such a scenario, how can one export discontiguous regions of
>> system memory to the efi stub? There seems to be only one way to
>> inform UEFI about system memory, via PcdSystemMemoryBase.
>>
>> Looking at the latest Arm Juno code, it seems like building a memory
>> resource descriptor hob, for the extra memory region, does the trick.
>> Would that be the best way to go?
>>
>> Suggestions please.
>
> There are two ways.
>
> First, in the PEI phase, you can produce memory resource descriptor HOBs
> that will describe system memory areas. When the DXE core starts, it
> will convert the suitable HOBs to EfiGcdMemoryTypeSystemMemory memory
> space. During DXE and BDS, boot/runtime code/data allocations will be
> satisfied from these. Then the UEFI memmap will reflect those
> allocations, and the system memory left unused, to the EFI stub.
>
> Second, you can add EfiGcdMemoryTypeSystemMemory memory space during and
> after the DXE phase, explicitly, using the DXE services. (IIRC, the PI
> spec says that memory space added this way may be picked by the UEFI
> memory allocation system immediately; IOW, it may immediately become
> available to the pool and page allocation boot serivces, to allocate
> from. IIRC, in edk2 this actually happens.) The rest is the same as
> above, wrt. the UEFI memmap.
>
> You can see an example for the second method under
> "ArmVirtPkg/HighMemDxe". I think it might be particularly close to your
> use case, as it practically translates the memory nodes found in QEMU's
> (copied) DTB to EfiGcdMemoryTypeSystemMemory memory space.
>
> (Ard, when do you plan to port this driver to FDT_CLIENT_PROTOCOL? :))
>

Any day now :-)

But seriously, I think this is orthogonal to the question, since I
don't expect that this platform uses DTB to describe the platform *to
UEFI*, and it would not run any of the runtime DT probing code.

So in this particular case, it is simply a matter of adding the
additional memory at any point during the execution of UEFI that is
convenient, by using one of the two methods you describe.

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

Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

2016-09-15 Thread Leif Lindholm 
On Wed, Sep 14, 2016 at 05:14:19PM +, Kinney, Michael D wrote:
> MdePkg/Include/Library/UefiLib.h does have some helper macros for
> setting timer events periods that are in 100 nS units:
> 
>   #define EFI_TIMER_PERIOD_MICROSECONDS(Microseconds) 
> MultU64x32((UINT64)(Microseconds), 10)
>   #define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds) 
> MultU64x32((UINT64)(Milliseconds), 1)
>   #define EFI_TIMER_PERIOD_SECONDS(Seconds)   
> MultU64x32((UINT64)(Seconds), 1000)
> 
> I believe the examples you show are for use with the gBS->Stall()
> service which is in 1 uS units.

Correct.

> Maybe we should consider some additional macros in UefiLib.h
> 
>   #define EFI_STALL_PERIOD_MICROSECONDS(Microseconds) (Microseconds)
>   #define EFI_STALL_PERIOD_MILLISECONDS(Milliseconds) ((Milliseconds) * 1000)
>   #define EFI_STALL_PERIOD_SECONDS(Seconds)   ((Seconds) * 100)
> 
> Or maybe some macros that actually do the call to gBS->Stall() too.
> 
>   #define EFI_STALL_MICROSECONDS(Microseconds) gBS->Stall (Microseconds)
>   #define EFI_STALL_MILLISECONDS(Milliseconds) gBS->Stall ((Milliseconds) * 
> 1000)
>   #define EFI_STALL_SECONDS(Seconds)   gBS->Stall ((Seconds) * 
> 100)
> 

Either (or both) of those two look good to me. The latter has the
benefit of a smaller call site, at the cost of perhaps obscuring the
dependency on UefiRuntimeServicesTableLib.

> The other method of generating timed delays for PEI/DXE/SMM modules
> is using the Services in MdePkg/Include/Library/TimerLib.h:
> 
>   UINTN
>   EFIAPI
>   NanoSecondDelay (
> IN  UINTN NanoSeconds
> );
> 
>   UINTN
>   EFIAPI
>   MicroSecondDelay (
> IN  UINTN MicroSeconds
> );
> 
> If we wanted macros helper to use these services to match UEFI ones, maybe 
> add the following to TimerLib.h:
> 
>   #define DELAY_NANOSECONDS(Nanoseconds)   NanoSecondDelay (Nanoseconds)
>   #define DELAY_MICROSECONDS(Microseconds) MicroSecondDelay (Microseconds)
>   #define DELAY_MILLISECONDS(Milliseconds) MicroSecondDelay ((Microseconds) * 
> 1000)
>   #define DELAY_SECONDS(Seconds)   MicroSecondDelay ((Microseconds) * 
> 100) 

I'm less concerned about those, but it could make sense for
completeness.

> Do you think it would improve the maintenance of the code if macros
> like these were used consistently?

It would certainly be good to reduce duplication, and consistency
would help with the readability of the code. (Which is good for
reviewing.)

Regards,

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

Re: [edk2] CLANG cross-compilation issue

2016-09-15 Thread Leif Lindholm 
Hi Andrew, list,

Just a follow-up.

On Wed, Sep 07, 2016 at 08:34:47AM -0700, Andrew Fish wrote:
> > The symptom is a failed link step with the message:
> > /usr/bin/ld: unrecognised emulation mode: aarch64linux
> > 
> > As a workaround, I was able to get my build functional again by
> > creating a symbolic link /usr/bin/aarch64-linux-gnueabi-ld pointing at
> > the actual location of my cross-ld.
> 
> I'm not sure it works the same as Xcode, but one of the search paths
> is often relative to the current location of the tools. This could
> be macOS specific as the tools don't install in /usr/bin anymore,
> and those locations just contain a reflector to the active
> toolchain.

I had a chat with our toolchain guys, and apparently this bug was
fixed over 5 years ago for the BSD platforms (including OS X), but for
some reason didn't get carried across to GNU:
https://llvm.org/bugs/show_bug.cgi?id=9777

They've now put on their schedule to fix this behaviour for GNU also.

Regards,

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

Re: [edk2] Exporting discontiguous System Memory to EFI STUB

2016-09-15 Thread Laszlo Ersek 
On 09/15/16 10:45, Sakar Arora wrote:
> Hi
> 
> This is in aarch64 UEFI context.
> 
> The efi stub code ignores any memory nodes in the device tree. It
> only relies on the UEFI memory map for memory info.
> 
> In such a scenario, how can one export discontiguous regions of
> system memory to the efi stub? There seems to be only one way to
> inform UEFI about system memory, via PcdSystemMemoryBase.
> 
> Looking at the latest Arm Juno code, it seems like building a memory
> resource descriptor hob, for the extra memory region, does the trick.
> Would that be the best way to go?
> 
> Suggestions please.

There are two ways.

First, in the PEI phase, you can produce memory resource descriptor HOBs
that will describe system memory areas. When the DXE core starts, it
will convert the suitable HOBs to EfiGcdMemoryTypeSystemMemory memory
space. During DXE and BDS, boot/runtime code/data allocations will be
satisfied from these. Then the UEFI memmap will reflect those
allocations, and the system memory left unused, to the EFI stub.

Second, you can add EfiGcdMemoryTypeSystemMemory memory space during and
after the DXE phase, explicitly, using the DXE services. (IIRC, the PI
spec says that memory space added this way may be picked by the UEFI
memory allocation system immediately; IOW, it may immediately become
available to the pool and page allocation boot serivces, to allocate
from. IIRC, in edk2 this actually happens.) The rest is the same as
above, wrt. the UEFI memmap.

You can see an example for the second method under
"ArmVirtPkg/HighMemDxe". I think it might be particularly close to your
use case, as it practically translates the memory nodes found in QEMU's
(copied) DTB to EfiGcdMemoryTypeSystemMemory memory space.

(Ard, when do you plan to port this driver to FDT_CLIENT_PROTOCOL? :))

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

[edk2] Exporting discontiguous System Memory to EFI STUB

2016-09-15 Thread Sakar Arora 
Hi

This is in aarch64 UEFI context.

The efi stub code ignores any memory nodes in the device tree. It only relies 
on the UEFI memory map for memory info.

In such a scenario, how can one export discontiguous regions of system memory 
to the efi stub? There seems to be only one way to inform UEFI about system 
memory, via PcdSystemMemoryBase.

Looking at the latest Arm Juno code, it seems like building a memory resource 
descriptor hob, for the extra memory region, does the trick. Would that be the 
best way to go?

Suggestions please.

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