date:20161202

Re: [edk2] [PATCH 0/3] MdePkg, MdeModulePkg, Vlv2TbltDevicePkg: 64-bit LoopTimes in S3 MEM_POLL

2016-12-02 Thread Yao, Jiewen

HI Laszlo
Would you please give me some time, so that I could discuss internal team to 
see if there is any concern on this API update?

The estimation will be 1 work week at most.

Usually adding a new API does not need compatibility check, but updating an 
existing API always need.

Thank you
Yao Jiewen

From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Friday, December 2, 2016 5:47 PM
To: Zeng, Star ; Yao, Jiewen ; 
edk2-devel-01 
Cc: Kinney, Michael D ; Tian, Feng 
; Gao, Liming ; Wei, David 

Subject: Re: [edk2] [PATCH 0/3] MdePkg, MdeModulePkg, Vlv2TbltDevicePkg: 64-bit 
LoopTimes in S3 MEM_POLL

On 12/02/16 05:54, Zeng, Star wrote:
> On 2016/12/2 9:53, Yao, Jiewen wrote:
>> HI Laszlo
>> Thank you to raise this long time existing issue. :-)
>>
>> There is historic reason that we inherit this undocumented API from
>> EDK-I to EDKII.
>>
>> I do not object your update.

Thanks!

>> I am thinking if it is time to create a new API which can match PI
>> specification, and suggest all consumers use the new API.
>>
>> RETURN_STATUS
>> EFIAPI
>> S3BootScriptSavePiMemPoll (
>>   IN  S3_BOOT_SCRIPT_LIB_WIDTH  Width,
>>   IN  UINT64Address,
>>   IN  VOID  *Data,
>>   IN  VOID  *DataMask,
>>   IN  UINT64Delay,
>>   );
>
> If the new API is introduced, the old one could be marked as deprecated
> by DISABLE_NEW_DEPRECATED_INTERFACES.

That's cool, but until then, can we please accept these patches? :)

Thanks!
Laszlo

> Thanks,
> Star
>
>>
>> Thank you
>> Yao Jiewen
>>
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Friday, December 2, 2016 1:56 AM
>>> To: edk2-devel-01 >
>>> Cc: Tian, Feng >; Gao, 
>>> Liming
>>> >;
>>> Kinney, Michael D 
>>> >; Zeng, Star
>>> >; Wei, David 
>>> >
>>> Subject: [edk2] [PATCH 0/3] MdePkg, MdeModulePkg, Vlv2TbltDevicePkg:
>>> 64-bit LoopTimes in S3 MEM_POLL
>>>
>>> While working on S3 boot script related stuff in OVMF, I wanted to see
>>> if infinite blocking was possible in the various POLL opcodes. While
>>> edk2's implementation of IO_POLL, PCI_CONFIG_POLL and
>>> PCI_CONFIG2_POLL
>>> follows the PI spec vol5 closely, even internally (using 100ns delay
>>> units), the MEM_POLL internals differ -- they are microseconds based.
>>>
>>> That's not a problem per se (it's just a different internal opcode
>>> representation, which is fine); the problem is that the current
>>> internals don't conform to the spec: in 32-bit builds, the UINT64 number
>>> of 100ns units that the caller intends to wait for is silently
>>> truncated, for no good reason. This issue is not hard to fix (we can
>>> even keep the microseconds-based internals), so let's fix it.
>>>
>>> Repo:   https://github.com/lersek/edk2/
>>> Branch: mempoll_looptimes_64bit
>>>
>>> Cc: David Wei >
>>> Cc: Feng Tian >
>>> Cc: Liming Gao >
>>> Cc: Mang Guo >
>>> Cc: Michael D Kinney 
>>> >
>>> Cc: Star Zeng >
>>>
>>> Thanks
>>> Laszlo
>>>
>>> Laszlo Ersek (3):
>>>   MdePkg, MdeModulePkg: S3BootScriptSaveMemPoll(): accept 64-bit
>>> LoopTimes
>>>   MdeModulePkg: S3SaveStateDxe, SmmS3SaveState: save 64-bit
>>> LoopTimes
>>>   Vlv2TbltDevicePkg/BootScriptSaveDxe: save 64-bit LoopTimes
>>>
>>>  MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c  | 2 +-
>>>  MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c| 8
>>> 
>>>  MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.c | 8
>>> 
>>>  MdePkg/Include/Library/S3BootScriptLib.h| 2 +-
>>>  MdePkg/Library/BaseS3BootScriptLibNull/BootScriptLib.c  | 2 +-
>>>  Vlv2TbltDevicePkg/BootScriptSaveDxe/ScriptSave.c| 4 ++--
>>>  6 files changed, 13 insertions(+), 13 deletions(-)
>>>
>>> --
>>> 2.9.2
>>>
>>> ___
>>> 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] [shell] Pipe causes pool failure in Shell.c

2016-12-02 Thread Tim Lewis

Jaben --

I'm not sure. In the debugger, it clearly showed the memory as having already 
been freed at the pointer. But I didn't track down who had done it.

Tim

-Original Message-
From: Carsey, Jaben [mailto:jaben.car...@intel.com] 
Sent: Friday, December 02, 2016 8:39 AM
To: Tim Lewis ; edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Carsey, Jaben 
Subject: RE: [shell] Pipe causes pool failure in Shell.c

Does that leak memory?  It looks like that memory was allocated at 1720, then 
moved from Out to In at 1729-1735.

Did the pointer possibly get mangled at 1724 or somewhere in between?

Ray,  Have you seen this?

-Jaben

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Tim Lewis
> Sent: Thursday, December 01, 2016 11:45 PM
> To: Tim Lewis ; edk2-devel@lists.01.org
> Subject: [edk2] [shell] Pipe causes pool failure in Shell.c
> Importance: High
> 
> After looking further, it appears that the FreePool() call on line 
> 1756 is unnecessary, and just causes a breakpoint.
> 
> Removing it allows the functionality to work correctly.
> 
> //FreePool (Split->SplitStdIn);
> 
> 
> Tim
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Tim Lewis
> Sent: Thursday, December 01, 2016 5:48 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Pipe causes pool failure in Shell.c
> 
> Using the latest Shell build, try:
> 
> ls -sfo | parse FileInfo 2
> 
> This ends up with a breakpoint when FreePool is called on Shell.c, line 1756.
> 
> I'm still debugging, but I wondered if anyone else has seen this?
> 
> Also:
> 
> ls -sfo > tmp
> parse FileInfo 2 < tmp
> 
> prints nothing, but
> 
> parse tmp FileInfo 2
> 
> works fine.
> 
> Tim
> ___
> 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] Handling multiple paths to the same EFI system partition?

2016-12-02 Thread Matthew Lazarowitz (mlazarow)

I am trying to deal with a requirement that our firmware show 2 boot options 
when there are multiple paths to a GPT disk with an EFI system partition. The 
specific ask is for both paths to be shown in the boot order. However, the 
short form device path means these would be duplicate entries which would cause 
more issues.
Fibre Channel and iSCSI connected drives are the two cases I need to cope with.
The UEFI specification seems to sound as if I could violate the spec if I do 
something like this.
In section 5.3.3 of the 2.6 spec, there is a statement "If GPT-cognizant 
software encounters two disks or partitions with identical GUIDs, results will 
be indeterminate." Other than section 10.12.2 specifying a iScsiMpioCapability 
field for network boots, I do not see guidance on how to handle a multi path 
scenario to a single disk.

The best I can think of right now is to modify the OS created option with the 
full path of of each path if 2 or more paths to a specific disk are detected. 
However, this would seem to be a potential source of conflict between the OS 
and the firmware since firmware would be modifying data the OS may require in a 
specific format.

Is there anything out there to provide some guidance on either rejecting this 
request and blatantly violating the spec, or being able to successfully fulfill 
it?

Thanks
Mathew

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

[edk2] [PATCH v2 5/6] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to updated lib class header

2016-12-02 Thread Laszlo Ersek

Put the FW_CFG_F_DMA constant, introduced in the last patch, to use.

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---

Notes:
v2:
- no need to include another header [Leif]
- scope is smaller now (only FW_CFG_F_DMA)

 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 6033a2a14c42..1b19893709fc 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -163,7 +163,7 @@ QemuFwCfgInitialize (
 
 QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
 Features = QemuFwCfgRead32 ();
-if ((Features & BIT1) != 0) {
+if ((Features & FW_CFG_F_DMA) != 0) {
   mFwCfgDmaAddress = FwCfgDmaAddress;
   InternalQemuFwCfgReadBytes = DmaReadBytes;
 }
-- 
2.9.2


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

[edk2] [PATCH v2 6/6] OvmfPkg/QemuFwCfgLib: support QEMU's DMA-like fw_cfg access method

2016-12-02 Thread Laszlo Ersek

The benefits of the DMA-like access method are (a) speed, (b) write
support in QEMU 2.9+.

(IOPort-based write support was discontinued in QEMU 2.4, and the
DMA-based one is being added to QEMU 2.9. Write support needs no separate
feature detection because writeability is governed on the level of
individual fw_cfg files -- if a file meant to be written by the firmware
exists in the directory, then it is writeable with the DMA method.)

We don't enable this feature for the SEC library instance, because:
- the SEC instance remains without clients (I've checked that it builds
  though),
- in SEC, any possible fw_cfg use is expected to be small and read-only.

Cc: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
Reviewed-by: Jordan Justen 
---

Notes:
v2:
- no need to include IndustryStandard/QemuFwCfgDma.h any longer (no such
  file)
- kept Jordan's R-b

 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 13 
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 72 
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c  | 25 ++-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 15 
 4 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
index 5b162bf98739..6e87c625102e 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
@@ -30,4 +30,17 @@ InternalQemuFwCfgIsAvailable (
   VOID
   );
 
+
+/**
+  Returns a boolean indicating whether QEMU provides the DMA-like access method
+  for fw_cfg.
+
+  @retvalTRUE   The DMA-like access method is available.
+  @retvalFALSE  The DMA-like access method is unavailable.
+**/
+BOOLEAN
+InternalQemuFwCfgDmaIsAvailable (
+  VOID
+  );
+
 #endif
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 804d5b0e42be..0bbf121de432 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -99,6 +99,70 @@ QemuFwCfgSelectItem (
 
 
 /**
+  Transfer an array of bytes using the DMA interface.
+
+  @param[in] SizeSize in bytes to transfer.
+  @param[in,out] Buffer  Buffer to read data into or write data from. May be
+ NULL if Size is zero.
+  @param[in] Write   TRUE if writing to fw_cfg from Buffer, FALSE if
+ reading from fw_cfg into Buffer.
+**/
+VOID
+InternalQemuFwCfgDmaBytes (
+  IN UINT32   Size,
+  IN OUT VOID *Buffer OPTIONAL,
+  IN BOOLEAN  Write
+  )
+{
+  volatile FW_CFG_DMA_ACCESS Access;
+  UINT32 AccessHigh, AccessLow;
+  UINT32 Status;
+
+  if (Size == 0) {
+return;
+  }
+
+  Access.Control = SwapBytes32 (
+Write ? FW_CFG_DMA_CTL_WRITE : FW_CFG_DMA_CTL_READ
+);
+  Access.Length  = SwapBytes32 (Size);
+  Access.Address = SwapBytes64 ((UINTN)Buffer);
+
+  //
+  // Delimit the transfer from (a) modifications to Access, (b) in case of a
+  // write, from writes to Buffer by the caller.
+  //
+  MemoryFence ();
+
+  //
+  // Start the transfer.
+  //
+  AccessHigh = (UINT32)RShiftU64 ((UINTN), 32);
+  AccessLow  = (UINT32)(UINTN)
+  IoWrite32 (0x514, SwapBytes32 (AccessHigh));
+  IoWrite32 (0x518, SwapBytes32 (AccessLow));
+
+  //
+  // Don't look at Access.Control before starting the transfer.
+  //
+  MemoryFence ();
+
+  //
+  // Wait for the transfer to complete.
+  //
+  do {
+Status = SwapBytes32 (Access.Control);
+ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0);
+  } while (Status != 0);
+
+  //
+  // After a read, the caller will want to use Buffer.
+  //
+  MemoryFence ();
+}
+
+
+/**
   Reads firmware configuration bytes into a buffer
 
   @param[in] Size - Size in bytes to read
@@ -112,6 +176,10 @@ InternalQemuFwCfgReadBytes (
   IN VOID   *Buffer  OPTIONAL
   )
 {
+  if (InternalQemuFwCfgDmaIsAvailable () && Size <= MAX_UINT32) {
+InternalQemuFwCfgDmaBytes ((UINT32)Size, Buffer, FALSE);
+return;
+  }
   IoReadFifo8 (0x511, Size, Buffer);
 }
 
@@ -160,6 +228,10 @@ QemuFwCfgWriteBytes (
   )
 {
   if (InternalQemuFwCfgIsAvailable ()) {
+if (InternalQemuFwCfgDmaIsAvailable () && Size <= MAX_UINT32) {
+  InternalQemuFwCfgDmaBytes ((UINT32)Size, Buffer, TRUE);
+  return;
+}
 IoWriteFifo8 (0x511, Size, Buffer);
   }
 }
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
index 88d88c0edf69..ac05f4c347f3 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
@@ -20,6 +20,7 @@
 #include "QemuFwCfgLibInternal.h"
 
 STATIC BOOLEAN mQemuFwCfgSupported = FALSE;
+STATIC BOOLEAN

[edk2] [PATCH v2 1/6] ArmVirtPkg/QemuFwCfgLib: remove superfluous InternalQemuFwCfgIsAvailable()

2016-12-02 Thread Laszlo Ersek

InternalQemuFwCfgIsAvailable() is an API that is incorrectly exposed by
the "OvmfPkg/Include/Library/QemuFwCfgLib.h" library class header; the API
is meant to be used internally to library instances (if it's needed at
all). ArmVirtPkg's instance has no use for it actually, so simplify the
code and remove the function definition.

Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
Reviewed-by: Jordan Justen 
Reviewed-by: Leif Lindholm 
---
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 31 
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 8ecbe3fb5fe6..2fd8d9050566 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -75,25 +75,6 @@ typedef struct {
 
 
 /**
-  Returns a boolean indicating if the firmware configuration interface is
-  available for library-internal purposes.
-
-  This function never changes fw_cfg state.
-
-  @retval TRUE   The interface is available internally.
-  @retval FALSE  The interface is not available internally.
-**/
-BOOLEAN
-EFIAPI
-InternalQemuFwCfgIsAvailable (
-  VOID
-  )
-{
-  return (BOOLEAN)(mFwCfgSelectorAddress != 0 && mFwCfgDataAddress != 0);
-}
-
-
-/**
   Returns a boolean indicating if the firmware configuration interface
   is available or not.
 
@@ -109,7 +90,7 @@ QemuFwCfgIsAvailable (
   VOID
   )
 {
-  return InternalQemuFwCfgIsAvailable ();
+  return (BOOLEAN)(mFwCfgSelectorAddress != 0 && mFwCfgDataAddress != 0);
 }
 
 
@@ -187,7 +168,7 @@ QemuFwCfgInitialize (
 FwCfgDmaAddress = 0;
   }
 
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
 UINT32 Signature;
 
 QemuFwCfgSelectItem (QemuFwCfgItemSignature);
@@ -231,7 +212,7 @@ QemuFwCfgSelectItem (
   IN FIRMWARE_CONFIG_ITEM QemuFwCfgItem
   )
 {
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
 MmioWrite16 (mFwCfgSelectorAddress, SwapBytes16 ((UINT16)QemuFwCfgItem));
   }
 }
@@ -360,7 +341,7 @@ QemuFwCfgReadBytes (
   IN VOID  *Buffer
   )
 {
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
 InternalQemuFwCfgReadBytes (Size, Buffer);
   } else {
 ZeroMem (Buffer, Size);
@@ -384,7 +365,7 @@ QemuFwCfgWriteBytes (
   IN VOID   *Buffer
   )
 {
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
 UINTN Idx;
 
 for (Idx = 0; Idx < Size; ++Idx) {
@@ -494,7 +475,7 @@ QemuFwCfgFindFile (
   UINT32 Count;
   UINT32 Idx;
 
-  if (!InternalQemuFwCfgIsAvailable ()) {
+  if (!QemuFwCfgIsAvailable ()) {
 return RETURN_UNSUPPORTED;
   }
 
-- 
2.9.2


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

[edk2] [PATCH v2 4/6] OvmfPkg/QemuFwCfgLib: extend lib class header with more definitions

2016-12-02 Thread Laszlo Ersek

The last patch consists purely of code movement; going forward, we should
use a few more symbolic constants.

Cc: Ard Biesheuvel 
Cc: Jordan Justen 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---

Notes:
v2:
- split these out to a separate patch

 OvmfPkg/Include/Library/QemuFwCfgLib.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h 
b/OvmfPkg/Include/Library/QemuFwCfgLib.h
index 40a07456c530..3e017d53a97e 100644
--- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
+++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
@@ -24,12 +24,20 @@
 #define QEMU_FW_CFG_FNAME_SIZE 56
 
 //
+// If the following bit is set in the UINT32 fw_cfg revision / feature bitmap
+// -- read from key 0x0001 with the basic IO Port or MMIO method --, then the
+// DMA interface is available.
+//
+#define FW_CFG_F_DMA BIT1
+
+//
 // Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
 //
 #define FW_CFG_DMA_CTL_ERROR  BIT0
 #define FW_CFG_DMA_CTL_READ   BIT1
 #define FW_CFG_DMA_CTL_SKIP   BIT2
 #define FW_CFG_DMA_CTL_SELECT BIT3
+#define FW_CFG_DMA_CTL_WRITE  BIT4
 
 typedef enum {
   QemuFwCfgItemSignature= 0x,
-- 
2.9.2


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

[edk2] [PATCH v2 3/6] ArmVirtPkg, OvmfPkg: QemuFwCfgLib: move DMA-related defs to lib class

2016-12-02 Thread Laszlo Ersek

Move the type and macro definitions related to QEMU's DMA-like fw_cfg
access method to the library class header.

Cc: Ard Biesheuvel 
Cc: Jordan Justen 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---

Notes:
v2:
- move definitions to lib class header rather than to a new
  IndustryStandard include [Jordan]
- because ArmVirtPkg's library instance already includes the lib class
  header, the code movement must be atomic and straddle both packages
- restrict patch to code movement solely; FW_CFG_F_DMA and
  FW_CFG_DMA_CTL_WRITE will be added separately

 OvmfPkg/Include/Library/QemuFwCfgLib.h | 19 +++
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 20 
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h 
b/OvmfPkg/Include/Library/QemuFwCfgLib.h
index 7c29422fbd72..40a07456c530 100644
--- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
+++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
@@ -23,6 +23,14 @@
 //
 #define QEMU_FW_CFG_FNAME_SIZE 56
 
+//
+// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
+//
+#define FW_CFG_DMA_CTL_ERROR  BIT0
+#define FW_CFG_DMA_CTL_READ   BIT1
+#define FW_CFG_DMA_CTL_SKIP   BIT2
+#define FW_CFG_DMA_CTL_SELECT BIT3
+
 typedef enum {
   QemuFwCfgItemSignature= 0x,
   QemuFwCfgItemInterfaceVersion = 0x0001,
@@ -59,6 +67,17 @@ typedef enum {
 
 } FIRMWARE_CONFIG_ITEM;
 
+//
+// Communication structure for the DMA access method. All fields are encoded in
+// big endian.
+//
+#pragma pack (1)
+typedef struct {
+  UINT32 Control;
+  UINT32 Length;
+  UINT64 Address;
+} FW_CFG_DMA_ACCESS;
+#pragma pack ()
 
 /**
   Returns a boolean indicating if the firmware configuration interface
diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 2fd8d9050566..6033a2a14c42 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -53,26 +53,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
 //
 STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
 
-//
-// Communication structure for DmaReadBytes(). All fields are encoded in big
-// endian.
-//
-#pragma pack (1)
-typedef struct {
-  UINT32 Control;
-  UINT32 Length;
-  UINT64 Address;
-} FW_CFG_DMA_ACCESS;
-#pragma pack ()
-
-//
-// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
-//
-#define FW_CFG_DMA_CTL_ERROR  BIT0
-#define FW_CFG_DMA_CTL_READ   BIT1
-#define FW_CFG_DMA_CTL_SKIP   BIT2
-#define FW_CFG_DMA_CTL_SELECT BIT3
-
 
 /**
   Returns a boolean indicating if the firmware configuration interface
-- 
2.9.2


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

[edk2] [PATCH v2 2/6] OvmfPkg/QemuFwCfgLib: move InternalQemuFwCfgIsAvailable() to lib instances

2016-12-02 Thread Laszlo Ersek

InternalQemuFwCfgIsAvailable() is an API that is incorrectly exposed by
the "OvmfPkg/Include/Library/QemuFwCfgLib.h" library class header; the API
is meant to be used internally to library instances (if it's needed at
all).

In OvmfPkg, we have two lib instances (for SEC and PEI/DXE); they provide
different implementations of InternalQemuFwCfgIsAvailable(), for the
shared file "OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c". Move the API
declaration to a new internal header called "QemuFwCfgLibInternal.h", and
drop EFIAPI in the process.

Cc: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
Reviewed-by: Jordan Justen 
---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf   |  1 +
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf|  1 +
 OvmfPkg/Include/Library/QemuFwCfgLib.h  | 16 --
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 33 
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c |  2 ++
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c  |  3 +-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c |  2 +-
 7 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
index a95e1e730c2c..66ac77850915 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
@@ -32,6 +32,7 @@ [Defines]
 #
 
 [Sources]
+  QemuFwCfgLibInternal.h
   QemuFwCfgLib.c
   QemuFwCfgPeiDxe.c
 
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
index 03a659c9b082..c1d6a54b1a39 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
@@ -30,6 +30,7 @@ [Defines]
 #
 
 [Sources]
+  QemuFwCfgLibInternal.h
   QemuFwCfgLib.c
   QemuFwCfgSec.c
 
diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h 
b/OvmfPkg/Include/Library/QemuFwCfgLib.h
index baaa257d6188..7c29422fbd72 100644
--- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
+++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
@@ -206,22 +206,6 @@ QemuFwCfgFindFile (
 
 
 /**
-  Returns a boolean indicating if the firmware configuration interface is
-  available for library-internal purposes.
-
-  This function never changes fw_cfg state.
-
-  @retvalTRUE   The interface is available internally.
-  @retvalFALSE  The interface is not available internally.
-**/
-BOOLEAN
-EFIAPI
-InternalQemuFwCfgIsAvailable (
-  VOID
-  );
-
-
-/**
   Determine if S3 support is explicitly enabled.
 
   @retval  TRUE   if S3 support is explicitly enabled.
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
new file mode 100644
index ..5b162bf98739
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
@@ -0,0 +1,33 @@
+/** @file
+  Internal interfaces specific to the QemuFwCfgLib instances in OvmfPkg.
+
+  Copyright (C) 2016, Red Hat, Inc.
+
+  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.
+
+**/
+
+#ifndef __QEMU_FW_CFG_LIB_INTERNAL_H__
+#define __QEMU_FW_CFG_LIB_INTERNAL_H__
+
+/**
+  Returns a boolean indicating if the firmware configuration interface is
+  available for library-internal purposes.
+
+  This function never changes fw_cfg state.
+
+  @retvalTRUE   The interface is available internally.
+  @retvalFALSE  The interface is not available internally.
+**/
+BOOLEAN
+InternalQemuFwCfgIsAvailable (
+  VOID
+  );
+
+#endif
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 5c96d2af2532..804d5b0e42be 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -22,6 +22,8 @@
 #include 
 #include 
 
+#include "QemuFwCfgLibInternal.h"
+
 
 /**
   Reads an 8-bit I/O port fifo into a block of memory.
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c 
b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
index f693cff29e01..88d88c0edf69 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
@@ -17,6 +17,8 @@
 #include 
 #include 
 
+#include "QemuFwCfgLibInternal.h"
+
 STATIC BOOLEAN mQemuFwCfgSupported = FALSE;
 
 
@@ -83,7 +85,6 @@ QemuFwCfgInitialize (
   @retvalFALSE  The interface is not available internally.
 **/
 BOOLEAN
-EFIAPI
 InternalQemuFwCfgIsAvailable (
   VOID
   )
diff --git

[edk2] [PATCH v2 0/6] OvmfPkg/QemuFwCfgLib: support the DMA-like interface

2016-12-02 Thread Laszlo Ersek

Version 2 of
.

The following patches

  [PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h
  [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to
  OvmfPkg/IndustryStandard

have been replaced with

  [PATCH v2 3/6] ArmVirtPkg, OvmfPkg: QemuFwCfgLib: move DMA-related
 defs to lib class
  [PATCH v2 4/6] OvmfPkg/QemuFwCfgLib: extend lib class header with more
 definitions
  [PATCH v2 5/6] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to updated
 lib class header

The last patch

  [PATCH v2 6/6] OvmfPkg/QemuFwCfgLib: support QEMU's DMA-like fw_cfg
 access method

needed a trivial update too; for that I preserved Jordan's R-b. Please
see the individual patches for v1->v2 details.

Repo:   https://github.com/lersek/edk2/
Branch: ovmf_fwcfg_dma_v2

Cc: Ard Biesheuvel 
Cc: Jordan Justen 
Cc: Leif Lindholm 

Thanks
Laszlo

Laszlo Ersek (6):
  ArmVirtPkg/QemuFwCfgLib: remove superfluous
InternalQemuFwCfgIsAvailable()
  OvmfPkg/QemuFwCfgLib: move InternalQemuFwCfgIsAvailable() to lib
instances
  ArmVirtPkg, OvmfPkg: QemuFwCfgLib: move DMA-related defs to lib class
  OvmfPkg/QemuFwCfgLib: extend lib class header with more definitions
  ArmVirtPkg/QemuFwCfgLib: rebase lib instance to updated lib class
header
  OvmfPkg/QemuFwCfgLib: support QEMU's DMA-like fw_cfg access method

 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c  | 53 ++
 OvmfPkg/Include/Library/QemuFwCfgLib.h  | 43 +++-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 74 
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf   |  1 +
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 46 
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c  | 28 +++-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 17 -
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf|  1 +
 8 files changed, 198 insertions(+), 65 deletions(-)
 create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h

-- 
2.9.2

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

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

2016-12-02 Thread Laszlo Ersek

On 12/02/16 17:02, Anthony PERARD wrote:
> On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
>> On 12/01/16 16:28, Anthony PERARD wrote:
>>> Hi,
>>>
>>> That might be only with the Xen part of OVMF but now that the GCC5
>>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
>>> to boot in Xen guests.
>>>
> [...]
>>>
>>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF
>>> boot.
>>>
>>> While trying to debug that, I've added some debug prints (in this module
>>> and in XenPvBlkDxe), and the exception could change and become a "page
>>> fault" instead, or even an assert failure in the PrintLib, that was the
>>> ASSERT(Buffer != NULL) at I think
>>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>>
>>> Adding EFIAPI to internal functions in XenBusDxe makes things work
>>> again.  My guest is that gcc would bypass (optimise) an exported
>>> functions and call directly an internal one but without reordering the
>>> arguments (EFIAPI vs nothing).
>>>
>>> Does that make sense?
>>
>> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
>> (until the root cause is found and addressed) to the XenBusDxe patches.
> 
> That works, using GCC49 (with gcc 6.2.1) works as well.
> 
>> Hrpmf, wait a second, I do see something interesting: in this series you
>> *are* modifying APIs declared in a library class header (namely
>> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
>> libraries) *are* required to specify EFIAPI.
>>
>> What happens if you apply patch #1 only?
> 
> With only XenHypercallLib changes, the error is the same.
> 
> But I did find the minimum change needed, it envolve a function with a
> VA_LIST as argument.
> 
> With only the following patch, OVMF works again.
> 
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 1666c4b..85b0956 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
>  }
>  
>  XENSTORE_STATUS
> +EFIAPI
>  XenStoreVSPrint (
>IN CONST XENSTORE_TRANSACTION *Transaction,
>IN CONST CHAR8   *DirectoryPath,
> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> index c9d4c65..33bb647 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.h
> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> @@ -209,6 +209,7 @@ XenStoreSPrint (
> indicating the type of write failure.
>  **/
>  XENSTORE_STATUS
> +EFIAPI
>  XenStoreVSPrint (
>IN CONST XENSTORE_TRANSACTION *Transaction,
>IN CONST CHAR8   *DirectoryPath,
>IN CONST CHAR8   *Node,
>IN CONST CHAR8   *FormatString,
>IN VA_LIST   Marker
>);
> 
> 
> I think the exception happen when this function is called via
> XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
> OvmfPkg/XenPvBlkDxe/BlockFront.c
> 

It used to be a known requirement / limitation that all functions with
variable argument lists had to be EFIAPI, regardless of cross-module
use. However, commit 48d5f9a551a93acb45f272dda879b0ab5a504e36 changed
that, and varargs should "just work" now. I suspect this is a
__builtin_ms_va_* regression in gcc-6. Thank you for narrowing it down.
It might make sense to report a bug in the upstream gcc tracker.

... Oh wow, this is a known gcc bug! See:

https://lists.01.org/pipermail/edk2-devel/2016-August/001018.html

Upstream gcc BZ  was
apparently solved for "Target Milestone: 6.3" (your version is 6.2.1).
So we'll either need a GCC6 toolchain in BaseTools that drops -flto, in
order to work around this gcc issue, or we'll have to ask gcc-6 users to
use at least gcc-6.3.

Oh wait, gcc-6.3 hasn't been released yet. We need the BaseTools
workaround then.

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

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

2016-12-02 Thread Ard Biesheuvel

On 2 December 2016 at 15:56, Leif Lindholm  wrote:
> On Fri, Dec 02, 2016 at 03:07:39PM +, Ard Biesheuvel wrote:
>>
>> > On 2 Dec 2016, at 14:40, Leif Lindholm  wrote:
>> >
>> > On Fri, Dec 02, 2016 at 12:54:45PM +, Ni, Ruiyu wrote:
>> > +Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin;
>> > +Dev->BarCount++;
>> > +
>> > +if (Desc->AddrSpaceGranularity == 64) {
>> > +  Dev->ConfigSpace.Device.Bar[Idx] |= 0x4;
>> > +  Dev->ConfigSpace.Device.Bar[++Idx] = 
>> > (UINT32)(Desc->AddrRangeMin >> 32);
>> 
>>  1. You need to use RShiftU64 otherwise the above code will break the
>>  build process in 32bit.
>> >>>
>> >>> I don't understand how that could cause breakage (and experiments with
>> >>> IA32 and ARM fail to reproduce it with GCC).
>> >>>
>> >>> -  Bar[x] is an array of UINT32
>> >>> -  Desc->AddrRangeMin is UINT64
>> >>> -  The result of (Desc->AddrRangeMin >> 32) is explicitly cast to
>> >>>  UINT32 (the type of Bar[x]).
>> >>>
>> >>> Is the problem related to the shift value being signed?
>> >>> Or does some other compiler refuse to cast a UINT64 to a UINT32?
>> >>
>> >> Desc->AddrRangeMin is UINT64. My experience tells me that
>> >> LShiftU64 or RShiftU64 should be used for shifting operation
>> >> on UINT64.
>> >
>> > This is the bit that confuses me. There is no such automatic
>> > limitation in the C language (if long long is supported at all). So if
>> > we're working around some bug in a specific compiler - I am happy to
>> > do so. I just prefer knowing why rather than cargo-culting.
>> >
>> >> I guess the GCC might cleverly recognizes the ">>32" actually
>> >> picks up the high-32 bits so in 32bit environment, it just uses
>> >> value of the register which stores the high-32 bits.
>> >> Can you check the assembly code GCC generates?
>> >> or simply can you change ">>32" to ">>31" or ">>33" to see
>> >> what happens?
>> >
>> > There is no cleverness required, it's just a defined operation on a
>> > base type. But sure, for example:
>> >
>> > unsigned int shift(long long val)
>> > {
>> >  return (val >> 33);
>> > }
>> >
>> > decodes as
>> > ---
>> >  :
>> >   0: 48 89 f8mov%rdi,%rax
>> >   3: 48 c1 f8 21  sar$0x21,%rax
>> >   7: c3   retq
>> > ---
>> > (in x86_64, using gcc -O2 to get rid of some redundant stack
>> > operations)
>> >
>> > The only thing that changes if the shift value is modified is that the
>> > 0x21 is changed accordingly.
>> >
>> > Change it to an inline constant, not much changes:
>> >
>> > unsigned int shiftconst(void)
>> > {
>> >  unsigned long long var = 0xULL;
>> >  return (var >> 31);
>> > }
>> >
>> > decodes as
>> > ---
>> >  :
>> >   0:   55push   %rbp
>> >   1:   48 89 e5 mov%rsp,%rbp
>> >   4:   48 b8 dd dd cc cc bbmovabs $0x,%rax
>> >   b:   bb aa aa
>> >   e:   48 89 45 f8mov%rax,-0x8(%rbp)
>> >  12:   48 8b 45 f8  mov-0x8(%rbp),%rax
>> >  16:   48 c1 e8 1f  shr$0x1f,%rax
>> >  1a:   5d   pop%rbp
>> >  1b:   c3retq
>> > ---
>> > (with -O0, to prevent the compiler from just doing the arithmetic
>> > compile time)
>> >
>> > Both also compile correctly for IA32 with -m32.
>> >
>> > The point is that there is nothing clever here for the compiler to do:
>> > casting a 64-bit int to a 32-bit int is an operation that discards the
>> > top 32 bits - but the behaviour is entirely defined.
>> >
>> > In fact, all architectures other than IA32/ARM use the Math64.c
>> > implementation of RShiftU64, which simply does the shift.
>> > And ARM does it only for GCC. I'm not sure why, I can't imagine a GCC
>> > version that would do anything more complicated than
>> > ---
>> >  :
>> >   0:e1a000c1asrr0, r1, #1
>> >   4:e12fff1ebxlr
>> > ---
>> > (and that's with -march=armv4t onwards)
>>
>> That's a 32-bit operand. Shifting a 64-bit value will result in a
>> call to a compiler intrinsic, which is kind of the point of Riuyu's
>> remark
>
> No, it's not.
> As stated, that is the output from compiling:
>
> unsigned int shift(long long val)
> {
>   return (val >> 33);
> }
>
> Unless you are claiming long long is 32-bit on ARM.
>
> Although I did mess up and make it signed :)
> When fixed, the code generation is identical but with an lsr instead.
>
> The compiler just operates directly on the top 32-bits (in r1), since
> the ones in r0 are irrelevant to the result.
>

OK, but that does mean it is performing optimization up to some point.
So right shifts of 64-bit quantities by 32 bits or more will be
'optimized' by GCC into something that does not rely on the
intrinsics. But this is not universally true for all toolchains (and
variable shifts are probably treated differently as well)

>

Re: [edk2] [shell] Pipe causes pool failure in Shell.c

2016-12-02 Thread Carsey, Jaben

Does that leak memory?  It looks like that memory was allocated at 1720, then 
moved from Out to In at 1729-1735.

Did the pointer possibly get mangled at 1724 or somewhere in between?

Ray,  Have you seen this?

-Jaben

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Tim Lewis
> Sent: Thursday, December 01, 2016 11:45 PM
> To: Tim Lewis ; edk2-devel@lists.01.org
> Subject: [edk2] [shell] Pipe causes pool failure in Shell.c
> Importance: High
> 
> After looking further, it appears that the FreePool() call on line 1756 is
> unnecessary, and just causes a breakpoint.
> 
> Removing it allows the functionality to work correctly.
> 
> //FreePool (Split->SplitStdIn);
> 
> 
> Tim
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Tim Lewis
> Sent: Thursday, December 01, 2016 5:48 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Pipe causes pool failure in Shell.c
> 
> Using the latest Shell build, try:
> 
> ls -sfo | parse FileInfo 2
> 
> This ends up with a breakpoint when FreePool is called on Shell.c, line 1756.
> 
> I'm still debugging, but I wondered if anyone else has seen this?
> 
> Also:
> 
> ls -sfo > tmp
> parse FileInfo 2 < tmp
> 
> prints nothing, but
> 
> parse tmp FileInfo 2
> 
> works fine.
> 
> Tim
> ___
> 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

2016-12-02 Thread Leif Lindholm

On Thu, Dec 01, 2016 at 03:53:19PM +, Leif Lindholm wrote:
> LzmaCustomDecompressLib and PeiDxeDebugLibReportStatusCode were copied
> from IntelFrameworkModulePkg to MdeModulePkg, but the originals were
> kept for compatibility.
> 
> Nevertheless, new code should be using the MdeModulePkg versions, so
> change all references in in-tree platforms.
> 
> Since the patches are individually independent, I plan to push them
> myself as Reviewed-by:s appear. Laszlo/Mike - Are you OK with me pusing
> 1 and 2 myself before the whole series is reviewed?
> 
> Changes from RFC:
> - Broken down into per-package patches.
> - Received Reviewed-by:s added.
> 
> Leif Lindholm (7):

I have now pushed:

>   OvmfPkg: Remove use of IntelFrameworkModulePkg legacy libs
>   QuarkSocPkg: Remove use of IntelFrameworkModulePkg legacy libs
>   DuetPkg: Remove use of IntelFrameworkModulePkg legacy libs
>   EmulatorPkg: Remove use of IntelFrameworkModulePkg legacy libs

Which have Reviewed-by: (thank you all for those).

The following remain without feedback (and Ard will be back next week,
so expect at least two of those can be resolved then):

>   BeagleBoardPkg: Remove use of IntelFrameworkModulePkg legacy libs
>   EmbeddedPkg: Remove use of IntelFrameworkModulePkg legacy libs
>   Vlv2TbltDevicePkg: Remove use of IntelFrameworkModulePkg legacy libs

Regards,

Leif

>  BeagleBoardPkg/BeagleBoardPkg.dsc   | 4 ++--
>  DuetPkg/DuetPkgIa32.dsc | 4 ++--
>  DuetPkg/DuetPkgX64.dsc  | 4 ++--
>  EmbeddedPkg/EmbeddedPkg.dsc | 2 +-
>  EmulatorPkg/EmulatorPkg.dsc | 4 ++--
>  OvmfPkg/OvmfPkgIa32.dsc | 4 ++--
>  OvmfPkg/OvmfPkgIa32X64.dsc  | 4 ++--
>  OvmfPkg/OvmfPkgX64.dsc  | 4 ++--
>  QuarkSocPkg/QuarkSocPkg.dsc | 2 +-
>  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc | 4 ++--
>  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc   | 6 +++---
>  Vlv2TbltDevicePkg/PlatformPkgX64.dsc| 6 +++---
>  12 files changed, 24 insertions(+), 24 deletions(-)
> 
> -- 
> 2.10.2
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

2016-12-02 Thread Anthony PERARD

On Thu, Dec 01, 2016 at 07:43:24PM +0100, Laszlo Ersek wrote:
> On 12/01/16 16:28, Anthony PERARD wrote:
> > Hi,
> > 
> > That might be only with the Xen part of OVMF but now that the GCC5
> > toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail
> > to boot in Xen guests.
> > 
[...]
> > 
> > Removing the gcc option -flto in only the XenBusDxe module makes OVMF
> > boot.
> > 
> > While trying to debug that, I've added some debug prints (in this module
> > and in XenPvBlkDxe), and the exception could change and become a "page
> > fault" instead, or even an assert failure in the PrintLib, that was the
> > ASSERT(Buffer != NULL) at I think
> > MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
> > 
> > Adding EFIAPI to internal functions in XenBusDxe makes things work
> > again.  My guest is that gcc would bypass (optimise) an exported
> > functions and call directly an internal one but without reordering the
> > arguments (EFIAPI vs nothing).
> > 
> > Does that make sense?
> 
> If "-b NOOPT" works for you, I'd prefer that as a temporary solution
> (until the root cause is found and addressed) to the XenBusDxe patches.

That works, using GCC49 (with gcc 6.2.1) works as well.

> Hrpmf, wait a second, I do see something interesting: in this series you
> *are* modifying APIs declared in a library class header (namely
> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> libraries) *are* required to specify EFIAPI.
> 
> What happens if you apply patch #1 only?

With only XenHypercallLib changes, the error is the same.

But I did find the minimum change needed, it envolve a function with a
VA_LIST as argument.

With only the following patch, OVMF works again.

diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 1666c4b..85b0956 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1307,6 +1307,7 @@ XenStoreTransactionEnd (
 }
 
 XENSTORE_STATUS
+EFIAPI
 XenStoreVSPrint (
   IN CONST XENSTORE_TRANSACTION *Transaction,
   IN CONST CHAR8   *DirectoryPath,
diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
index c9d4c65..33bb647 100644
--- a/OvmfPkg/XenBusDxe/XenStore.h
+++ b/OvmfPkg/XenBusDxe/XenStore.h
@@ -209,6 +209,7 @@ XenStoreSPrint (
indicating the type of write failure.
 **/
 XENSTORE_STATUS
+EFIAPI
 XenStoreVSPrint (
   IN CONST XENSTORE_TRANSACTION *Transaction,
   IN CONST CHAR8   *DirectoryPath,
   IN CONST CHAR8   *Node,
   IN CONST CHAR8   *FormatString,
   IN VA_LIST   Marker
   );


I think the exception happen when this function is called via
XENBUS_PROTOCOL->XsPrintf() from XenPvBlockFrontInitialization() in
OvmfPkg/XenPvBlkDxe/BlockFront.c

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

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

2016-12-02 Thread Leif Lindholm

On Fri, Dec 02, 2016 at 03:07:39PM +, Ard Biesheuvel wrote:
> 
> > On 2 Dec 2016, at 14:40, Leif Lindholm  wrote:
> > 
> > On Fri, Dec 02, 2016 at 12:54:45PM +, Ni, Ruiyu wrote:
> > +Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin;
> > +Dev->BarCount++;
> > +
> > +if (Desc->AddrSpaceGranularity == 64) {
> > +  Dev->ConfigSpace.Device.Bar[Idx] |= 0x4;
> > +  Dev->ConfigSpace.Device.Bar[++Idx] = (UINT32)(Desc->AddrRangeMin 
> > >> 32);
>  
>  1. You need to use RShiftU64 otherwise the above code will break the
>  build process in 32bit.
> >>> 
> >>> I don't understand how that could cause breakage (and experiments with
> >>> IA32 and ARM fail to reproduce it with GCC).
> >>> 
> >>> -  Bar[x] is an array of UINT32
> >>> -  Desc->AddrRangeMin is UINT64
> >>> -  The result of (Desc->AddrRangeMin >> 32) is explicitly cast to
> >>>  UINT32 (the type of Bar[x]).
> >>> 
> >>> Is the problem related to the shift value being signed?
> >>> Or does some other compiler refuse to cast a UINT64 to a UINT32?
> >> 
> >> Desc->AddrRangeMin is UINT64. My experience tells me that
> >> LShiftU64 or RShiftU64 should be used for shifting operation
> >> on UINT64.
> > 
> > This is the bit that confuses me. There is no such automatic
> > limitation in the C language (if long long is supported at all). So if
> > we're working around some bug in a specific compiler - I am happy to
> > do so. I just prefer knowing why rather than cargo-culting.
> > 
> >> I guess the GCC might cleverly recognizes the ">>32" actually
> >> picks up the high-32 bits so in 32bit environment, it just uses
> >> value of the register which stores the high-32 bits.
> >> Can you check the assembly code GCC generates?
> >> or simply can you change ">>32" to ">>31" or ">>33" to see
> >> what happens?
> > 
> > There is no cleverness required, it's just a defined operation on a
> > base type. But sure, for example:
> > 
> > unsigned int shift(long long val)
> > {
> >  return (val >> 33);
> > }
> > 
> > decodes as
> > ---
> >  :
> >   0: 48 89 f8mov%rdi,%rax
> >   3: 48 c1 f8 21  sar$0x21,%rax
> >   7: c3   retq
> > ---
> > (in x86_64, using gcc -O2 to get rid of some redundant stack
> > operations)
> > 
> > The only thing that changes if the shift value is modified is that the
> > 0x21 is changed accordingly.
> > 
> > Change it to an inline constant, not much changes:
> > 
> > unsigned int shiftconst(void)
> > {
> >  unsigned long long var = 0xULL;
> >  return (var >> 31);
> > }
> > 
> > decodes as
> > ---
> >  :
> >   0:   55push   %rbp
> >   1:   48 89 e5 mov%rsp,%rbp
> >   4:   48 b8 dd dd cc cc bbmovabs $0x,%rax
> >   b:   bb aa aa
> >   e:   48 89 45 f8mov%rax,-0x8(%rbp)
> >  12:   48 8b 45 f8  mov-0x8(%rbp),%rax
> >  16:   48 c1 e8 1f  shr$0x1f,%rax
> >  1a:   5d   pop%rbp
> >  1b:   c3retq
> > ---
> > (with -O0, to prevent the compiler from just doing the arithmetic
> > compile time)
> > 
> > Both also compile correctly for IA32 with -m32.
> > 
> > The point is that there is nothing clever here for the compiler to do:
> > casting a 64-bit int to a 32-bit int is an operation that discards the
> > top 32 bits - but the behaviour is entirely defined.
> > 
> > In fact, all architectures other than IA32/ARM use the Math64.c
> > implementation of RShiftU64, which simply does the shift.
> > And ARM does it only for GCC. I'm not sure why, I can't imagine a GCC
> > version that would do anything more complicated than
> > ---
> >  :
> >   0:e1a000c1asrr0, r1, #1
> >   4:e12fff1ebxlr
> > ---
> > (and that's with -march=armv4t onwards)
> 
> That's a 32-bit operand. Shifting a 64-bit value will result in a
> call to a compiler intrinsic, which is kind of the point of Riuyu's
> remark

No, it's not.
As stated, that is the output from compiling:

unsigned int shift(long long val)
{
  return (val >> 33);
}

Unless you are claiming long long is 32-bit on ARM.

Although I did mess up and make it signed :)
When fixed, the code generation is identical but with an lsr instead.

The compiler just operates directly on the top 32-bits (in r1), since
the ones in r0 are irrelevant to the result.

Change the shift to 31 and the compiler doesn't do anything cute, so
the code is slightly more obvious:
---
 :
   0:e1a00fa0   lsr r0, r0, #31
   4:e1800081   orr r0, r0, r1, lsl #1
   8:e12fff1e   bx  lr
---

But even if there were intrinsics generated, shouldn't we support the
intrinsic instead of massaging the C code and hope we can prevent it
from being generated.

/
Leif

> 
> 
> > So if we are saying that there are certain specific compilers that do
> > not support

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

2016-12-02 Thread Ard Biesheuvel


> On 2 Dec 2016, at 14:40, Leif Lindholm  wrote:
> 
> On Fri, Dec 02, 2016 at 12:54:45PM +, Ni, Ruiyu wrote:
> +Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin;
> +Dev->BarCount++;
> +
> +if (Desc->AddrSpaceGranularity == 64) {
> +  Dev->ConfigSpace.Device.Bar[Idx] |= 0x4;
> +  Dev->ConfigSpace.Device.Bar[++Idx] = (UINT32)(Desc->AddrRangeMin 
> >> 32);
 
 1. You need to use RShiftU64 otherwise the above code will break the
 build process in 32bit.
>>> 
>>> I don't understand how that could cause breakage (and experiments with
>>> IA32 and ARM fail to reproduce it with GCC).
>>> 
>>> -  Bar[x] is an array of UINT32
>>> -  Desc->AddrRangeMin is UINT64
>>> -  The result of (Desc->AddrRangeMin >> 32) is explicitly cast to
>>>  UINT32 (the type of Bar[x]).
>>> 
>>> Is the problem related to the shift value being signed?
>>> Or does some other compiler refuse to cast a UINT64 to a UINT32?
>> 
>> Desc->AddrRangeMin is UINT64. My experience tells me that
>> LShiftU64 or RShiftU64 should be used for shifting operation
>> on UINT64.
> 
> This is the bit that confuses me. There is no such automatic
> limitation in the C language (if long long is supported at all). So if
> we're working around some bug in a specific compiler - I am happy to
> do so. I just prefer knowing why rather than cargo-culting.
> 
>> I guess the GCC might cleverly recognizes the ">>32" actually
>> picks up the high-32 bits so in 32bit environment, it just uses
>> value of the register which stores the high-32 bits.
>> Can you check the assembly code GCC generates?
>> or simply can you change ">>32" to ">>31" or ">>33" to see
>> what happens?
> 
> There is no cleverness required, it's just a defined operation on a
> base type. But sure, for example:
> 
> unsigned int shift(long long val)
> {
>  return (val >> 33);
> }
> 
> decodes as
> ---
>  :
>   0: 48 89 f8mov%rdi,%rax
>   3: 48 c1 f8 21  sar$0x21,%rax
>   7: c3   retq
> ---
> (in x86_64, using gcc -O2 to get rid of some redundant stack
> operations)
> 
> The only thing that changes if the shift value is modified is that the
> 0x21 is changed accordingly.
> 
> Change it to an inline constant, not much changes:
> 
> unsigned int shiftconst(void)
> {
>  unsigned long long var = 0xULL;
>  return (var >> 31);
> }
> 
> decodes as
> ---
>  :
>   0:   55push   %rbp
>   1:   48 89 e5 mov%rsp,%rbp
>   4:   48 b8 dd dd cc cc bbmovabs $0x,%rax
>   b:   bb aa aa
>   e:   48 89 45 f8mov%rax,-0x8(%rbp)
>  12:   48 8b 45 f8  mov-0x8(%rbp),%rax
>  16:   48 c1 e8 1f  shr$0x1f,%rax
>  1a:   5d   pop%rbp
>  1b:   c3retq
> ---
> (with -O0, to prevent the compiler from just doing the arithmetic
> compile time)
> 
> Both also compile correctly for IA32 with -m32.
> 
> The point is that there is nothing clever here for the compiler to do:
> casting a 64-bit int to a 32-bit int is an operation that discards the
> top 32 bits - but the behaviour is entirely defined.
> 
> In fact, all architectures other than IA32/ARM use the Math64.c
> implementation of RShiftU64, which simply does the shift.
> And ARM does it only for GCC. I'm not sure why, I can't imagine a GCC
> version that would do anything more complicated than
> ---
>  :
>   0:e1a000c1asrr0, r1, #1
>   4:e12fff1ebxlr
> ---
> (and that's with -march=armv4t onwards)

That's a 32-bit operand. Shifting a 64-bit value will result in a call to a 
compiler intrinsic, which is kind of the point of Riuyu's remark


> So if we are saying that there are certain specific compilers that do
> not support 64-bit arithmetic for 32-bit architectures, and that those
> compilers are still supported by BaseTools - then I'm fine with that.
> 
> But I don't want to replace functional C code with abstraction
> functions without a proper understanding of where it would be
> problematic.
> 
> Regards,
> 
> Leif
> 
>> 
>>> 
>>> Regards,
>>> 
>>> Leif
>>> 
> +}
> +  }
> +}
> diff --git
> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.h
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> PciDeviceIo.h
> new file mode 100644
> index ..bc0a3d3258f9
> --- /dev/null
> +++
> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> Pc
> +++ iDeviceIo.h
> @@ -0,0 +1,84 @@
> +/** @file
> +
> +  Copyright (C) 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  distribution. The full text of the license

[edk2] [shell] shift generates errors with no command-line options

2016-12-02 Thread Tim Lewis

In a script that looks like this:

:start
if not %1 == "" then
  echo %1
  shift
  goto start
endif

'shift' will generate an error message. The spec doesn't describe any behavior 
like this and it is annoying to write scripts that must avoid it.

Of course you can work around it...

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

Re: [edk2] [Patch v2 0/4] Fix SOURCE_DEBUG_ENABLE

2016-12-02 Thread Yao, Jiewen

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Kinney, Michael D
> Sent: Friday, December 2, 2016 6:08 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Wei, David
> ; Guo, Mang 
> Subject: [Patch v2 0/4] Fix SOURCE_DEBUG_ENABLE
> 
> Changes in V2
> =
> * Fix build failure when SOURCE_DEBUG_ENABLE is FALSE
> 
> Fix SOURCE_DEBUG_ENABLE feature that includes updates to library
> mappings, PCD settings, and adding DebugAgent UART Console as one
> of the possible console devices in the PlatformBdsLib.
> 
> The series also cleans up a few inconsistencies in the DSC files
> and updates PlatformPkgGccX64.dsc to match PlatformPkgX64.dsc.
> 
> Cc: Jiewen Yao 
> Cc: David Wei 
> Cc: Mang Guo 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 
> 
> Michael Kinney (4):
>   Vlv2TbltDevicePkg: Make !if statement usage consistent
>   Vlv2TbltDevicePkg: Fix SOURCE_DEBUG_ENABLE feature
>   Vlv2TbltDevicePkg/PlatformBdsLib: Add DebugAgent Console
>   Vlv2TbltDevicePkg: Update PlatformPkgGccX64.dsc
> 
>  .../Library/PlatformBdsLib/BdsPlatform.h   | 13 +++-
>  .../Library/PlatformBdsLib/PlatformBdsLib.inf  |  1 +
>  .../Library/PlatformBdsLib/PlatformData.c  | 48
> +-
>  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc| 77
> ++
>  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc  | 15 +++--
>  Vlv2TbltDevicePkg/PlatformPkgX64.dsc   | 26 
>  6 files changed, 119 insertions(+), 61 deletions(-)
> 
> --
> 2.6.3.windows.1

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

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

2016-12-02 Thread Leif Lindholm

On Fri, Dec 02, 2016 at 12:54:45PM +, Ni, Ruiyu wrote:
> >> > +Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin;
> >> > +Dev->BarCount++;
> >> > +
> >> > +if (Desc->AddrSpaceGranularity == 64) {
> >> > +  Dev->ConfigSpace.Device.Bar[Idx] |= 0x4;
> >> > +  Dev->ConfigSpace.Device.Bar[++Idx] = (UINT32)(Desc->AddrRangeMin 
> >> > >> 32);
> >>
> >> 1. You need to use RShiftU64 otherwise the above code will break the
> >> build process in 32bit.
> >
> >I don't understand how that could cause breakage (and experiments with
> >IA32 and ARM fail to reproduce it with GCC).
> >
> >-  Bar[x] is an array of UINT32
> >-  Desc->AddrRangeMin is UINT64
> >-  The result of (Desc->AddrRangeMin >> 32) is explicitly cast to
> >   UINT32 (the type of Bar[x]).
> >
> >Is the problem related to the shift value being signed?
> >Or does some other compiler refuse to cast a UINT64 to a UINT32?
> 
> Desc->AddrRangeMin is UINT64. My experience tells me that
> LShiftU64 or RShiftU64 should be used for shifting operation
> on UINT64.

This is the bit that confuses me. There is no such automatic
limitation in the C language (if long long is supported at all). So if
we're working around some bug in a specific compiler - I am happy to
do so. I just prefer knowing why rather than cargo-culting.

> I guess the GCC might cleverly recognizes the ">>32" actually
> picks up the high-32 bits so in 32bit environment, it just uses
> value of the register which stores the high-32 bits.
> Can you check the assembly code GCC generates?
> or simply can you change ">>32" to ">>31" or ">>33" to see
> what happens?

There is no cleverness required, it's just a defined operation on a
base type. But sure, for example:

unsigned int shift(long long val)
{
  return (val >> 33);
}

decodes as
---
 :
   0:48 89 f8   mov%rdi,%rax
   3:48 c1 f8 21sar$0x21,%rax
   7:c3 retq
---
(in x86_64, using gcc -O2 to get rid of some redundant stack
operations)

The only thing that changes if the shift value is modified is that the
0x21 is changed accordingly.

Change it to an inline constant, not much changes:

unsigned int shiftconst(void)
{
  unsigned long long var = 0xULL;
  return (var >> 31);
}

decodes as
---
 :
   0:   55  push   %rbp
   1:   48 89 e5mov%rsp,%rbp
   4:   48 b8 dd dd cc cc bbmovabs $0x,%rax
   b:   bb aa aa
   e:   48 89 45 f8 mov%rax,-0x8(%rbp)
  12:   48 8b 45 f8 mov-0x8(%rbp),%rax
  16:   48 c1 e8 1f shr$0x1f,%rax
  1a:   5d  pop%rbp
  1b:   c3  retq
---
(with -O0, to prevent the compiler from just doing the arithmetic
compile time)

Both also compile correctly for IA32 with -m32.

The point is that there is nothing clever here for the compiler to do:
casting a 64-bit int to a 32-bit int is an operation that discards the
top 32 bits - but the behaviour is entirely defined.

In fact, all architectures other than IA32/ARM use the Math64.c
implementation of RShiftU64, which simply does the shift.
And ARM does it only for GCC. I'm not sure why, I can't imagine a GCC
version that would do anything more complicated than
---
 :
   0:   e1a000c1asr r0, r1, #1
   4:   e12fff1ebx  lr
---
(and that's with -march=armv4t onwards).

So if we are saying that there are certain specific compilers that do
not support 64-bit arithmetic for 32-bit architectures, and that those
compilers are still supported by BaseTools - then I'm fine with that.

But I don't want to replace functional C code with abstraction
functions without a proper understanding of where it would be
problematic.

Regards,

Leif

> 
> >
> >Regards,
> >
> >Leif
> >
> >> > +}
> >> > +  }
> >> > +}
> >> > diff --git
> >> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> >> > PciDeviceIo.h
> >> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> >> > PciDeviceIo.h
> >> > new file mode 100644
> >> > index ..bc0a3d3258f9
> >> > --- /dev/null
> >> > +++
> >> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> >> > Pc
> >> > +++ iDeviceIo.h
> >> > @@ -0,0 +1,84 @@
> >> > +/** @file
> >> > +
> >> > +  Copyright (C) 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  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.
> >> > +
> >> > +**/
> >> > +
> >> > +#ifndef

[edk2] [shell] @ does not work before if, endif, for and many other command.

2016-12-02 Thread Tim Lewis

Try this:

:start
@if %1 == ""  then
  @goto Done
@endif
  @echo Parameter: %1
  @shift
  goto start
:Done

Per Chapter 4

Also, additional '@' before a command in a script file can prevent the current 
command from being echoed.


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

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

2016-12-02 Thread Ni, Ruiyu



Regards,
Ray

>-Original Message-
>From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>Sent: Wednesday, November 30, 2016 10:07 PM
>To: Ni, Ruiyu 
>Cc: Ard Biesheuvel ; edk2-devel@lists.01.org; Gao, 
>Liming ;
>af...@apple.com; Kinney, Michael D ; 
>m...@semihalf.com; Tian, Feng 
>Subject: Re: [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for 
>non-discoverable devices
>
>Hi Ray,
>
>Ard is on holiday this week, and mostly managing to stay off the
>email...
>
>Anyway, I could really use this support in order to bring the Marvell
>Armada 70x0 support (worked on by the Semihalf guys) into
>OpenPlatformPkg, so I will pick this up while he is away - at least
>parts 1-3.
>
>4/5 is required for 5/5, so I will not be looking to push the final
>one myself.
>
>Would you OK with pushing just 1-3 once we get this one resolved?
>
>On Mon, Nov 28, 2016 at 01:56:21AM +, Ni, Ruiyu wrote:
>> > diff --git
>> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > PciDeviceIo.c
>> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > PciDeviceIo.c
>> > new file mode 100644
>> > index ..fd59267a8d66
>> > --- /dev/null
>> > +++
>> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > Pc
>> > +++ iDeviceIo.c
>> > @@ -0,0 +1,797 @@
>...
>> > +VOID
>> > +InitializePciIoProtocol (
>> > +  NON_DISCOVERABLE_PCI_DEVICE *Dev
>> > +  )
>> > +{
>> > +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *Desc;
>> > +  INTNIdx;
>> > +
>> > +  InitializeListHead (>UncachedAllocationList);
>
>The above line has crept in here, but belongs to 4/5 (causes
>compilation error). I will fix that and resend - is it OK if I resend
>this patch only?
>
>> > +  // Iterate over the resources to populate the virtual BARs
>> > +  //
>> > +  Idx = Dev->BarOffset;
>> > +  for (Desc = Dev->Device->Resources, Dev->BarCount = 0;
>> > +   Desc->Desc != ACPI_END_TAG_DESCRIPTOR;
>> > +   Desc = (VOID *)((UINT8 *)Desc + Desc->Len + 3)) {
>> > +
>> > +ASSERT (Desc->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR);
>> > +ASSERT (Desc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM);
>> > +
>> > +if (Idx >= PCI_MAX_BARS ||
>> > +(Idx == PCI_MAX_BARS - 1 && Desc->AddrSpaceGranularity == 64)) {
>> > +  DEBUG ((DEBUG_ERROR,
>> > +"%a: resource count exceeds number of emulated BARs\n",
>> > +__FUNCTION__));
>> > +  ASSERT (FALSE);
>> > +  break;
>> > +}
>> > +
>> > +Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin;
>> > +Dev->BarCount++;
>> > +
>> > +if (Desc->AddrSpaceGranularity == 64) {
>> > +  Dev->ConfigSpace.Device.Bar[Idx] |= 0x4;
>> > +  Dev->ConfigSpace.Device.Bar[++Idx] = (UINT32)(Desc->AddrRangeMin >> 
>> > 32);
>>
>> 1. You need to use RShiftU64 otherwise the above code will break the
>> build process in 32bit.
>
>I don't understand how that could cause breakage (and experiments with
>IA32 and ARM fail to reproduce it with GCC).
>
>-  Bar[x] is an array of UINT32
>-  Desc->AddrRangeMin is UINT64
>-  The result of (Desc->AddrRangeMin >> 32) is explicitly cast to
>   UINT32 (the type of Bar[x]).
>
>Is the problem related to the shift value being signed?
>Or does some other compiler refuse to cast a UINT64 to a UINT32?

Desc->AddrRangeMin is UINT64. My experience tells me that
LShiftU64 or RShiftU64 should be used for shifting operation
on UINT64.
I guess the GCC might cleverly recognizes the ">>32" actually
picks up the high-32 bits so in 32bit environment, it just uses
value of the register which stores the high-32 bits.
Can you check the assembly code GCC generates?
or simply can you change ">>32" to ">>31" or ">>33" to see
what happens?

>
>Regards,
>
>Leif
>
>> > +}
>> > +  }
>> > +}
>> > diff --git
>> > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > PciDeviceIo.h
>> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > PciDeviceIo.h
>> > new file mode 100644
>> > index ..bc0a3d3258f9
>> > --- /dev/null
>> > +++
>> > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
>> > Pc
>> > +++ iDeviceIo.h
>> > @@ -0,0 +1,84 @@
>> > +/** @file
>> > +
>> > +  Copyright (C) 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  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.
>> > +
>> > +**/
>> > +
>> > +#ifndef __NON_DISCOVERABLE_PCI_DEVICE_IO_H__
>> > +#define

Re: [edk2] [PATCH 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

2016-12-02 Thread Leif Lindholm

On Fri, Dec 02, 2016 at 12:53:45PM +0100, Laszlo Ersek wrote:
> On 12/02/16 12:27, Leif Lindholm wrote:
> > On Thu, Dec 01, 2016 at 11:38:42AM -0800, Jordan Justen wrote:
> >> For the future, I'd recommend adding Cc's for package owners to the
> >> associated patch commit message.
> > 
> > I guess what you're actually asking for is to have only the parts of
> > series I want your review on sent to you?
> > 
> > That's a fair point, but having Cc: tags of package maintainers in the
> > commit log is just noise.
> 
> Strongly disagree, I fully *want* the commit log to capture the fact
> that I CC'd X, Y and Z, and if the committed patch doesn't have feedback
> tags from them, that's not my fault.

And if it did capture that, I would agree - but it doesn't.
It's in-band text which may or may not have had any effect on the
transmission of email - depending on client version, configuration and
developer workflow.

Nevertheless, this is exactly the discussion I wanted on a different
thread than this as stated below.

> Independently, technically speaking, there's also no other way for
> assigning different sets of CC's to different patches in a series, to my
> knowledge. There are hacks around it (for example, you can mail out each
> patch individually, setting the CC's per mailing, or else edit the CC
> headers (not commit message tags) in the formatted patch email), but
> none of those stick around when you rebase the work for v2 or v3, and
> format and send out the series again.
> 
> > And we are indeed seeing a lot of those at
> > the moment.
> > 
> > This seems like an excellent time to bring back the idea of unifying
> > the */Contributions.txt and codifying some of this behaviour
> > better. (None of them mention Cc: - and they are in fact all
> > bit-identical.)
> 
> Well, my personal guide mentions Cc:
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-18

And that's an extremely useful resource, but entirely
unofficial. Let's change that.

Regards,

Leif

> Thanks
> Laszlo
> 
> > Rather than derailing this thread, how about I send out a patch that
> > does just that, and then we can discuss modifications to the document?
> > 
> >> Series Reviewed-by: Jordan Justen 
> > 
> > Many thanks - I'll apply for your packages.
> > 
> > Regards,
> > 
> > Leif
> > 
> >> On 2016-12-01 07:53:19, Leif Lindholm wrote:
> >>> LzmaCustomDecompressLib and PeiDxeDebugLibReportStatusCode were copied
> >>> from IntelFrameworkModulePkg to MdeModulePkg, but the originals were
> >>> kept for compatibility.
> >>>
> >>> Nevertheless, new code should be using the MdeModulePkg versions, so
> >>> change all references in in-tree platforms.
> >>>
> >>> Since the patches are individually independent, I plan to push them
> >>> myself as Reviewed-by:s appear. Laszlo/Mike - Are you OK with me pusing
> >>> 1 and 2 myself before the whole series is reviewed?
> >>>
> >>> Changes from RFC:
> >>> - Broken down into per-package patches.
> >>> - Received Reviewed-by:s added.
> >>>
> >>> Leif Lindholm (7):
> >>>   OvmfPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >>>   QuarkSocPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >>>   BeagleBoardPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >>>   EmbeddedPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >>>   DuetPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >>>   EmulatorPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >>>   Vlv2TbltDevicePkg: Remove use of IntelFrameworkModulePkg legacy libs
> >>>
> >>>  BeagleBoardPkg/BeagleBoardPkg.dsc   | 4 ++--
> >>>  DuetPkg/DuetPkgIa32.dsc | 4 ++--
> >>>  DuetPkg/DuetPkgX64.dsc  | 4 ++--
> >>>  EmbeddedPkg/EmbeddedPkg.dsc | 2 +-
> >>>  EmulatorPkg/EmulatorPkg.dsc | 4 ++--
> >>>  OvmfPkg/OvmfPkgIa32.dsc | 4 ++--
> >>>  OvmfPkg/OvmfPkgIa32X64.dsc  | 4 ++--
> >>>  OvmfPkg/OvmfPkgX64.dsc  | 4 ++--
> >>>  QuarkSocPkg/QuarkSocPkg.dsc | 2 +-
> >>>  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc | 4 ++--
> >>>  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc   | 6 +++---
> >>>  Vlv2TbltDevicePkg/PlatformPkgX64.dsc| 6 +++---
> >>>  12 files changed, 24 insertions(+), 24 deletions(-)
> >>>
> >>> -- 
> >>> 2.10.2
> >>>
> >>> ___
> >>> 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 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

2016-12-02 Thread Laszlo Ersek

On 12/02/16 12:27, Leif Lindholm wrote:
> On Thu, Dec 01, 2016 at 11:38:42AM -0800, Jordan Justen wrote:
>> For the future, I'd recommend adding Cc's for package owners to the
>> associated patch commit message.
> 
> I guess what you're actually asking for is to have only the parts of
> series I want your review on sent to you?
> 
> That's a fair point, but having Cc: tags of package maintainers in the
> commit log is just noise.

Strongly disagree, I fully *want* the commit log to capture the fact
that I CC'd X, Y and Z, and if the committed patch doesn't have feedback
tags from them, that's not my fault.

Independently, technically speaking, there's also no other way for
assigning different sets of CC's to different patches in a series, to my
knowledge. There are hacks around it (for example, you can mail out each
patch individually, setting the CC's per mailing, or else edit the CC
headers (not commit message tags) in the formatted patch email), but
none of those stick around when you rebase the work for v2 or v3, and
format and send out the series again.

> And we are indeed seeing a lot of those at
> the moment.
> 
> This seems like an excellent time to bring back the idea of unifying
> the */Contributions.txt and codifying some of this behaviour
> better. (None of them mention Cc: - and they are in fact all
> bit-identical.)

Well, my personal guide mentions Cc:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-18

Thanks
Laszlo

> Rather than derailing this thread, how about I send out a patch that
> does just that, and then we can discuss modifications to the document?
> 
>> Series Reviewed-by: Jordan Justen 
> 
> Many thanks - I'll apply for your packages.
> 
> Regards,
> 
> Leif
> 
>> On 2016-12-01 07:53:19, Leif Lindholm wrote:
>>> LzmaCustomDecompressLib and PeiDxeDebugLibReportStatusCode were copied
>>> from IntelFrameworkModulePkg to MdeModulePkg, but the originals were
>>> kept for compatibility.
>>>
>>> Nevertheless, new code should be using the MdeModulePkg versions, so
>>> change all references in in-tree platforms.
>>>
>>> Since the patches are individually independent, I plan to push them
>>> myself as Reviewed-by:s appear. Laszlo/Mike - Are you OK with me pusing
>>> 1 and 2 myself before the whole series is reviewed?
>>>
>>> Changes from RFC:
>>> - Broken down into per-package patches.
>>> - Received Reviewed-by:s added.
>>>
>>> Leif Lindholm (7):
>>>   OvmfPkg: Remove use of IntelFrameworkModulePkg legacy libs
>>>   QuarkSocPkg: Remove use of IntelFrameworkModulePkg legacy libs
>>>   BeagleBoardPkg: Remove use of IntelFrameworkModulePkg legacy libs
>>>   EmbeddedPkg: Remove use of IntelFrameworkModulePkg legacy libs
>>>   DuetPkg: Remove use of IntelFrameworkModulePkg legacy libs
>>>   EmulatorPkg: Remove use of IntelFrameworkModulePkg legacy libs
>>>   Vlv2TbltDevicePkg: Remove use of IntelFrameworkModulePkg legacy libs
>>>
>>>  BeagleBoardPkg/BeagleBoardPkg.dsc   | 4 ++--
>>>  DuetPkg/DuetPkgIa32.dsc | 4 ++--
>>>  DuetPkg/DuetPkgX64.dsc  | 4 ++--
>>>  EmbeddedPkg/EmbeddedPkg.dsc | 2 +-
>>>  EmulatorPkg/EmulatorPkg.dsc | 4 ++--
>>>  OvmfPkg/OvmfPkgIa32.dsc | 4 ++--
>>>  OvmfPkg/OvmfPkgIa32X64.dsc  | 4 ++--
>>>  OvmfPkg/OvmfPkgX64.dsc  | 4 ++--
>>>  QuarkSocPkg/QuarkSocPkg.dsc | 2 +-
>>>  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc | 4 ++--
>>>  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc   | 6 +++---
>>>  Vlv2TbltDevicePkg/PlatformPkgX64.dsc| 6 +++---
>>>  12 files changed, 24 insertions(+), 24 deletions(-)
>>>
>>> -- 
>>> 2.10.2
>>>
>>> ___
>>> 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 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

2016-12-02 Thread Laszlo Ersek

On 12/02/16 12:03, Leif Lindholm wrote:
> On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:
>> where "QemuFwCfgDma.h" was added in the previous patch.
>>
>> Cc: Ard Biesheuvel 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
> 
> This is nice cleanup.
> One bit of bikeshedding below, address or ignore - regardless:
> Reviewed-by: Leif Lindholm 
> 
>> ---
>>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-
>>  1 file changed, 3 insertions(+), 21 deletions(-)
>>
>> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
>> b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> index 2fd8d9050566..62a85dff328e 100644
>> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> @@ -25,6 +25,8 @@
>>  
>>  #include 
>>  
>> +#include 
>> +
> 
> So, I forget if we have official guidelines on this, but instinctively
> I would put IndustryStandard before Library (alphabetically).

Good point. But, in the next version, this #include directive will go
away anyway, because this file already includes
, and that header file will get the new macros
in v2 (based on Jordan's feedback).

Thanks!
Laszlo

> 
> Regards,
> 
> Leif
> 
>>  STATIC UINTN mFwCfgSelectorAddress;
>>  STATIC UINTN mFwCfgDataAddress;
>>  STATIC UINTN mFwCfgDmaAddress;
>> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
>>  //
>>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
>>  
>> -//
>> -// Communication structure for DmaReadBytes(). All fields are encoded in big
>> -// endian.
>> -//
>> -#pragma pack (1)
>> -typedef struct {
>> -  UINT32 Control;
>> -  UINT32 Length;
>> -  UINT64 Address;
>> -} FW_CFG_DMA_ACCESS;
>> -#pragma pack ()
>> -
>> -//
>> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
>> -//
>> -#define FW_CFG_DMA_CTL_ERROR  BIT0
>> -#define FW_CFG_DMA_CTL_READ   BIT1
>> -#define FW_CFG_DMA_CTL_SKIP   BIT2
>> -#define FW_CFG_DMA_CTL_SELECT BIT3
>> -
>>  
>>  /**
>>Returns a boolean indicating if the firmware configuration interface
>> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (
>>  
>>  QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
>>  Features = QemuFwCfgRead32 ();
>> -if ((Features & BIT1) != 0) {
>> +if ((Features & FW_CFG_F_DMA) != 0) {
>>mFwCfgDmaAddress = FwCfgDmaAddress;
>>InternalQemuFwCfgReadBytes = DmaReadBytes;
>>  }
>> -- 
>> 2.9.2
>>
>>
>> ___
>> 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
> 

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

Re: [edk2] [PATCH 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

2016-12-02 Thread Leif Lindholm

On Thu, Dec 01, 2016 at 11:38:42AM -0800, Jordan Justen wrote:
> For the future, I'd recommend adding Cc's for package owners to the
> associated patch commit message.

I guess what you're actually asking for is to have only the parts of
series I want your review on sent to you?

That's a fair point, but having Cc: tags of package maintainers in the
commit log is just noise. And we are indeed seeing a lot of those at
the moment.

This seems like an excellent time to bring back the idea of unifying
the */Contributions.txt and codifying some of this behaviour
better. (None of them mention Cc: - and they are in fact all
bit-identical.)

Rather than derailing this thread, how about I send out a patch that
does just that, and then we can discuss modifications to the document?

> Series Reviewed-by: Jordan Justen 

Many thanks - I'll apply for your packages.

Regards,

Leif

> On 2016-12-01 07:53:19, Leif Lindholm wrote:
> > LzmaCustomDecompressLib and PeiDxeDebugLibReportStatusCode were copied
> > from IntelFrameworkModulePkg to MdeModulePkg, but the originals were
> > kept for compatibility.
> > 
> > Nevertheless, new code should be using the MdeModulePkg versions, so
> > change all references in in-tree platforms.
> > 
> > Since the patches are individually independent, I plan to push them
> > myself as Reviewed-by:s appear. Laszlo/Mike - Are you OK with me pusing
> > 1 and 2 myself before the whole series is reviewed?
> > 
> > Changes from RFC:
> > - Broken down into per-package patches.
> > - Received Reviewed-by:s added.
> > 
> > Leif Lindholm (7):
> >   OvmfPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >   QuarkSocPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >   BeagleBoardPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >   EmbeddedPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >   DuetPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >   EmulatorPkg: Remove use of IntelFrameworkModulePkg legacy libs
> >   Vlv2TbltDevicePkg: Remove use of IntelFrameworkModulePkg legacy libs
> > 
> >  BeagleBoardPkg/BeagleBoardPkg.dsc   | 4 ++--
> >  DuetPkg/DuetPkgIa32.dsc | 4 ++--
> >  DuetPkg/DuetPkgX64.dsc  | 4 ++--
> >  EmbeddedPkg/EmbeddedPkg.dsc | 2 +-
> >  EmulatorPkg/EmulatorPkg.dsc | 4 ++--
> >  OvmfPkg/OvmfPkgIa32.dsc | 4 ++--
> >  OvmfPkg/OvmfPkgIa32X64.dsc  | 4 ++--
> >  OvmfPkg/OvmfPkgX64.dsc  | 4 ++--
> >  QuarkSocPkg/QuarkSocPkg.dsc | 2 +-
> >  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc | 4 ++--
> >  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc   | 6 +++---
> >  Vlv2TbltDevicePkg/PlatformPkgX64.dsc| 6 +++---
> >  12 files changed, 24 insertions(+), 24 deletions(-)
> > 
> > -- 
> > 2.10.2
> > 
> > ___
> > 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 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

2016-12-02 Thread Leif Lindholm

On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:
> where "QemuFwCfgDma.h" was added in the previous patch.
> 
> Cc: Ard Biesheuvel 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 

This is nice cleanup.
One bit of bikeshedding below, address or ignore - regardless:
Reviewed-by: Leif Lindholm 

> ---
>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c 
> b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 2fd8d9050566..62a85dff328e 100644
> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -25,6 +25,8 @@
>  
>  #include 
>  
> +#include 
> +

So, I forget if we have official guidelines on this, but instinctively
I would put IndustryStandard before Library (alphabetically).

Regards,

Leif

>  STATIC UINTN mFwCfgSelectorAddress;
>  STATIC UINTN mFwCfgDataAddress;
>  STATIC UINTN mFwCfgDmaAddress;
> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
>  //
>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
>  
> -//
> -// Communication structure for DmaReadBytes(). All fields are encoded in big
> -// endian.
> -//
> -#pragma pack (1)
> -typedef struct {
> -  UINT32 Control;
> -  UINT32 Length;
> -  UINT64 Address;
> -} FW_CFG_DMA_ACCESS;
> -#pragma pack ()
> -
> -//
> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
> -//
> -#define FW_CFG_DMA_CTL_ERROR  BIT0
> -#define FW_CFG_DMA_CTL_READ   BIT1
> -#define FW_CFG_DMA_CTL_SKIP   BIT2
> -#define FW_CFG_DMA_CTL_SELECT BIT3
> -
>  
>  /**
>Returns a boolean indicating if the firmware configuration interface
> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (
>  
>  QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
>  Features = QemuFwCfgRead32 ();
> -if ((Features & BIT1) != 0) {
> +if ((Features & FW_CFG_F_DMA) != 0) {
>mFwCfgDmaAddress = FwCfgDmaAddress;
>InternalQemuFwCfgReadBytes = DmaReadBytes;
>  }
> -- 
> 2.9.2
> 
> 
> ___
> 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

[edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script

2016-12-02 Thread Laszlo Ersek

EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE expects the PCI address to
access in UEFI encoding, not in edk2/PciLib encoding. Convert the
ICH9_GEN_PMCON_1 register's address to UEFI representation before storing
it in the boot script.

Cc: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c 
b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
index c5e5ed02f5ad..3694282c82ad 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -307,6 +308,33 @@ FatalError:
 }
 
 /**
+  Convert a PCI address originally composed with PCI_LIB_ADDRESS() to
+  EFI_PCI_ADDRESS() representation (see Table 111. "PCI Configuration Address"
+  in UEFI-2.6).
+
+  @param[in] PciLibAddress  A PCI address originally composed with
+PCI_LIB_ADDRESS().
+
+  @return  The converted address suitable for consumers that expect
+   EFI_PCI_ADDRESS() representation.
+**/
+STATIC
+UINT64
+ConvertPciLibToEfiPciAddress (
+  IN UINT32 PciLibAddress
+  )
+{
+  UINT32 Bus, Device, Function, Register;
+
+  Register = BitFieldRead32 (PciLibAddress,  0, 11);
+  Function = BitFieldRead32 (PciLibAddress, 12, 14);
+  Device   = BitFieldRead32 (PciLibAddress, 15, 19);
+  Bus  = BitFieldRead32 (PciLibAddress, 20, 27);
+
+  return EFI_PCI_ADDRESS (Bus, Device, Function, Register);
+}
+
+/**
   Notification callback for S3SaveState installation.
 
   @param[in] EventEvent whose notification function is being invoked.
@@ -362,7 +390,9 @@ OnS3SaveStateInstalled (
   S3SaveState,
   EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE,
   EfiBootScriptWidthUint16,
-  (UINT64)POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1),
+  ConvertPciLibToEfiPciAddress (
+POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1)
+),
   ,
   
   );
-- 
2.9.2

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

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

2016-12-02 Thread Leif Lindholm

Ray, any comments on the below?

Regards,

Leif

On Wed, Nov 30, 2016 at 02:06:55PM +, Leif Lindholm wrote:
> Hi Ray,
> 
> Ard is on holiday this week, and mostly managing to stay off the
> email...
> 
> Anyway, I could really use this support in order to bring the Marvell
> Armada 70x0 support (worked on by the Semihalf guys) into
> OpenPlatformPkg, so I will pick this up while he is away - at least
> parts 1-3.
> 
> 4/5 is required for 5/5, so I will not be looking to push the final
> one myself.
> 
> Would you OK with pushing just 1-3 once we get this one resolved?
> 
> On Mon, Nov 28, 2016 at 01:56:21AM +, Ni, Ruiyu wrote:
> > > diff --git
> > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> > > PciDeviceIo.c
> > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> > > PciDeviceIo.c
> > > new file mode 100644
> > > index ..fd59267a8d66
> > > --- /dev/null
> > > +++
> > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> > > Pc
> > > +++ iDeviceIo.c
> > > @@ -0,0 +1,797 @@
> ...
> > > +VOID
> > > +InitializePciIoProtocol (
> > > +  NON_DISCOVERABLE_PCI_DEVICE *Dev
> > > +  )
> > > +{
> > > +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *Desc;
> > > +  INTNIdx;
> > > +
> > > +  InitializeListHead (>UncachedAllocationList);
> 
> The above line has crept in here, but belongs to 4/5 (causes
> compilation error). I will fix that and resend - is it OK if I resend
> this patch only?
> 
> > > +  // Iterate over the resources to populate the virtual BARs
> > > +  //
> > > +  Idx = Dev->BarOffset;
> > > +  for (Desc = Dev->Device->Resources, Dev->BarCount = 0;
> > > +   Desc->Desc != ACPI_END_TAG_DESCRIPTOR;
> > > +   Desc = (VOID *)((UINT8 *)Desc + Desc->Len + 3)) {
> > > +
> > > +ASSERT (Desc->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR);
> > > +ASSERT (Desc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM);
> > > +
> > > +if (Idx >= PCI_MAX_BARS ||
> > > +(Idx == PCI_MAX_BARS - 1 && Desc->AddrSpaceGranularity == 64)) {
> > > +  DEBUG ((DEBUG_ERROR,
> > > +"%a: resource count exceeds number of emulated BARs\n",
> > > +__FUNCTION__));
> > > +  ASSERT (FALSE);
> > > +  break;
> > > +}
> > > +
> > > +Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin;
> > > +Dev->BarCount++;
> > > +
> > > +if (Desc->AddrSpaceGranularity == 64) {
> > > +  Dev->ConfigSpace.Device.Bar[Idx] |= 0x4;
> > > +  Dev->ConfigSpace.Device.Bar[++Idx] = (UINT32)(Desc->AddrRangeMin 
> > > >> 32);
> > 
> > 1. You need to use RShiftU64 otherwise the above code will break the
> > build process in 32bit.
> 
> I don't understand how that could cause breakage (and experiments with
> IA32 and ARM fail to reproduce it with GCC).
> 
> -  Bar[x] is an array of UINT32
> -  Desc->AddrRangeMin is UINT64
> -  The result of (Desc->AddrRangeMin >> 32) is explicitly cast to
>UINT32 (the type of Bar[x]).
> 
> Is the problem related to the shift value being signed?
> Or does some other compiler refuse to cast a UINT64 to a UINT32?
> 
> Regards,
> 
> Leif
> 
> > > +}
> > > +  }
> > > +}
> > > diff --git
> > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> > > PciDeviceIo.h
> > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> > > PciDeviceIo.h
> > > new file mode 100644
> > > index ..bc0a3d3258f9
> > > --- /dev/null
> > > +++
> > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable
> > > Pc
> > > +++ iDeviceIo.h
> > > @@ -0,0 +1,84 @@
> > > +/** @file
> > > +
> > > +  Copyright (C) 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  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.
> > > +
> > > +**/
> > > +
> > > +#ifndef __NON_DISCOVERABLE_PCI_DEVICE_IO_H__
> > > +#define __NON_DISCOVERABLE_PCI_DEVICE_IO_H__
> > > +
> > > +#include 
> > > +#include 
> > > +#include  #include
> > > +
> > > +#include 
> > > +
> > > +#include 
> > > +
> > > +#include 
> > > +#include 
> > > +#include 
> > > +
> > > +#define NON_DISCOVERABLE_PCI_DEVICE_SIG SIGNATURE_32 ('P', 'P', 'I',
> > > +'D')
> > > +
> > > +#define NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(PciIoPointer) \
> > > +CR (PciIoPointer, NON_DISCOVERABLE_PCI_DEVICE, PciIo, \
> > > +NON_DISCOVERABLE_PCI_DEVICE_SIG)
> > > +
> > > +#define PCI_ID_VENDOR_UNKNOWN 0x
> > > +#define PCI_ID_DEVICE_DONTCARE0x
> > > +
> > > +#define PCI_MAX_BARS  6
> > > +
> > > +typedef struct {
> > > +  UINT32

Re: [edk2] [PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h

2016-12-02 Thread Laszlo Ersek

On 12/02/16 02:01, Jordan Justen wrote:
> On 2016-12-01 12:48:51, Laszlo Ersek wrote:
>> On 12/01/16 20:34, Jordan Justen wrote:
>>> On 2016-12-01 09:56:31, Laszlo Ersek wrote:
 Add the type and macro definitions related to QEMU's DMA-like fw_cfg
 access method in a dedicated header file.

 Cc: Ard Biesheuvel 
 Cc: Jordan Justen 
 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Laszlo Ersek 
 ---
  OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h | 50 
>>>
>>> What do you think about just
>>> OvmfPkg/Include/IndustryStandard/QemuFwCfg.h?
>>>
>>> Arguably, the FIRMWARE_CONFIG_ITEM enums could be moved there as
>>> well...
>>>
>>> Then again, I think we could also just put this content into
>>> OvmfPkg/Include/Library/QemuFwCfgLib.h.
>>
>> Adding this stuff to "OvmfPkg/Include/Library/QemuFwCfgLib.h" sounds
>> good to me; I'll do that in v2.
>>
> 
> Ok. By the way, I looked over the series, and with that change you can
> add Reviewed-by: Jordan Justen 

Thank you!

> I'll let you decide if you want to send out a v2.

Yes, I will; first I'd like you to at least skim v2, second, I need
Ard's R-b for the ArmVirtPkg patches.

Hm, Ard seems to be on vacation (good choice :)), so maybe I'll CC Leif
as well. Then I'll use your R-b and hopefully Leif's in Ard's review's
stead.

Thanks!
Laszlo

> -Jordan
> 
>>
>>> -Jordan
>>>
  1 file changed, 50 insertions(+)

 diff --git a/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h 
 b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
 new file mode 100644
 index ..37a5804adb05
 --- /dev/null
 +++ b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
 @@ -0,0 +1,50 @@
 +/** @file
 +  Macro and type definitions related to QEMU's DMA-like fw_cfg access 
 method,
 +  based on "docs/specs/fw_cfg.txt" in the QEMU tree.
 +
 +  Copyright (C) 2016, Red Hat, Inc.
 +
 +  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.
 +**/
 +
 +
 +#ifndef __FW_CFG_DMA__
 +#define __FW_CFG_DMA__
 +
 +#include 
 +
 +//
 +// If the following bit is set in the UINT32 fw_cfg revision / feature 
 bitmap
 +// -- read from key 0x0001 with the basic IO Port or MMIO method --, then 
 the
 +// DMA interface is available.
 +//
 +#define FW_CFG_F_DMA BIT1
 +
 +//
 +// Communication structure for the DMA access method. All fields are 
 encoded in
 +// big endian.
 +//
 +#pragma pack (1)
 +typedef struct {
 +  UINT32 Control;
 +  UINT32 Length;
 +  UINT64 Address;
 +} FW_CFG_DMA_ACCESS;
 +#pragma pack ()
 +
 +//
 +// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
 +//
 +#define FW_CFG_DMA_CTL_ERROR  BIT0
 +#define FW_CFG_DMA_CTL_READ   BIT1
 +#define FW_CFG_DMA_CTL_SKIP   BIT2
 +#define FW_CFG_DMA_CTL_SELECT BIT3
 +#define FW_CFG_DMA_CTL_WRITE  BIT4
 +
 +#endif
 -- 
 2.9.2


>>

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

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

2016-12-02 Thread Laszlo Ersek

On 12/02/16 05:36, Gao, Liming wrote:
> I agree to root cause this issue with GCC6. Could you help submit this issue 
> in bugzilia? Then, we will investigate it further. 

Thanks, Liming, I've now filed
.

Cheers
Laszlo

> Thanks
> Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Friday, December 02, 2016 2:43 AM
> To: Anthony PERARD ; Justen, Jordan L 
> ; Gao, Liming ; Zhu, 
> Yonghong ; Ard Biesheuvel 
> Cc: edk2-de...@ml01.01.org
> Subject: Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled 
> with GCC5
> 
> On 12/01/16 16:28, Anthony PERARD wrote:
>> Hi,
>>
>> That might be only with the Xen part of OVMF but now that the GCC5 
>> toolchains is used with my gcc (6.2.1 20160830, Arch Linux), OVMF fail 
>> to boot in Xen guests.
>>
>> Here is the result:
>>  X64 Exception Type - 06(#UD - Invalid Opcode)  CPU Apic ID -  
>> 
>> RIP  - 1F26AF6B, CS  - 0038, RFLAGS - 
>> 00010202 RAX  - 0001, RCX - 1F26AF51, RDX 
>> - 0004 RBX  - , RSP - 1F43C510, 
>> RBP - 1E583D18 RSI  - 0003, RDI - 0001
>> R8   - , R9  - , R10 - 1E58DB98
>> R11  - 0002, R12 - 1E58D898, R13 - 
>> 
>> R14  - 1E58D8A0, R15 - 1F26D001
>> DS   - 0030, ES  - 0030, FS  - 0030
>> GS   - 0030, SS  - 0030
>> CR0  - 8033, CR2 - , CR3 - 
>> 1F3DB000
>> CR4  - 0668, CR8 - 
>> DR0  - , DR1 - , DR2 - 
>> 
>> DR3  - , DR6 - 0FF0, DR7 - 
>> 0400 GDTR - 1F3C9A98 0047, LDTR - 
>> 
>> IDTR - 1EB0A018 0FFF,   TR - 
>> FXSAVE_STATE - 1F43C170
>>  Find PE image 
>> ./Build/OvmfX64/DEBUG_GCC5/X64/OvmfPkg/XenBusDxe/XenBusDxe/DEBUG/XenBusDxe.dll
>>  (ImageBase=1F266000, EntryPoint=1F2669D5) 
>>
>> Removing the gcc option -flto in only the XenBusDxe module makes OVMF 
>> boot.
>>
>> While trying to debug that, I've added some debug prints (in this 
>> module and in XenPvBlkDxe), and the exception could change and become 
>> a "page fault" instead, or even an assert failure in the PrintLib, 
>> that was the ASSERT(Buffer != NULL) at I think
>> MdePkg/Library/BasePrintLib/PrintLibInternal.c:366
>>
>> Adding EFIAPI to internal functions in XenBusDxe makes things work 
>> again.  My guest is that gcc would bypass (optimise) an exported 
>> functions and call directly an internal one but without reordering the 
>> arguments (EFIAPI vs nothing).
>>
>> Does that make sense?
> 
> Thank you for the investigation. It is strange that only the Xen modules are 
> affected, I'm unsure what that's the case.
> 
> Either way, it seems to be a gcc-6 bug, or an edk2 toolchain bug. You should 
> *not* need EFIAPI for functions with external linkage if the calls to them 
> straddle only files in the same module. I'm suspecting
> gcc-6 (we've received no such reports with gcc-5). Maybe we need a GCC6 
> toolchain as well, for turning off some new features in gcc-6?
> 
> Jordan, Liming, Yonghong, Ard -- any ideas?
> 
> Anthony: while we all figure this out, please consider building OVMF with the 
> "-b NOOPT" switch. Support for the NOOPT build target has recently been added 
> to the GCC Ia32/X64 toolchains in BaseTools, and to the OVMF DSC files as 
> well. The build targets correspond to:
> 
> RELEASE -- compiler optimization enabled; DEBUG, ASSERT, and similar
>DebugLib features compiled out
> DEBUG   -- compiler optimization enabled; DebugLib features preserved
> NOOPT   -- compiler optimization disabled; DebugLib features preserved
> 
> (Note that for ArmVirtPkg and the GCC AARCH64 toolchains in BaseTools, there 
> is no NOOPT, and DEBUG means actually NOOPT -- if I remember correctly. Ard 
> will correct me if I'm wrong :))
> 
> If "-b NOOPT" works for you, I'd prefer that as a temporary solution (until 
> the root cause is found and addressed) to the XenBusDxe patches.
> 
> Hrpmf, wait a second, I do see something interesting: in this series you
> *are* modifying APIs declared in a library class header (namely 
> "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
> libraries) *are* required to specify EFIAPI.
> 
> What happens if you apply patch #1 only?
> 
> Thanks!
> Laszlo
> 
>> Anthony PERARD (4):
>>   OvmfPkg/XenHypercallLib: Add EFIAPI
>>   OvmfPkg/XenBusDxe: Add EFIAPI to XenEventChannelNotify
>>   OvmfPkg/XenBusDxe: Add EFIAPI to

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

2016-12-02 Thread Laszlo Ersek

On 12/02/16 01:58, Jordan Justen wrote:
> On 2016-12-01 12:54:34, Laszlo Ersek wrote:
>> On 12/01/16 21:06, Jordan Justen wrote:
>>> On 2016-12-01 10:43:24, Laszlo Ersek wrote:
 Hrpmf, wait a second, I do see something interesting: in this series you
 *are* modifying APIs declared in a library class header (namely
 "OvmfPkg/Include/Library/XenHypercallLib.h"). Such functions (public
 libraries) *are* required to specify EFIAPI.

 What happens if you apply patch #1 only?
>>>
>>> I agree that this should be fixed.
>>>
>>> But, if it works, I'm concerned that it would just be hiding a bug. My
>>> understanding was that the EFIAPI on libraries was needed so that a
>>> library implementation could be assembly based if desired. In this
>>> case C is used for the implementation, so the calling conventions
>>> should align.
>>
>> Never tried the following, so I'm unsure if edk2 intends to support it
>> explicitly, but what about binary-only library instances (the 2-clause
>> BSDL allows that)?
> 
> Good point. Once again, I agree that it is a bug that needs to be
> fixed. The functions in the library interface should have EFIAPI.
> 
> I was just pointing out that I don't think it *ought* be the issue
> here since both side are C, and being built by the same compiler.

Got it now. :)

Thanks!
Laszlo

> Like you mentioned ... maybe it is a compiler bug, or a bug with our
> build flags.
> 
> -Jordan
> 
>> If the library class header doesn't state EFIAPI on the functions, the
>> library vendor builds the library instance with Visual Studio, and the
>> library user builds the client module with gcc (against the same library
>> class header), calls will fail.
>>
>> (The way I imagine using binary-only library instances is that the
>> library comes as a binary object with a matching INF file, in a separate
>> subdirectory, and the user resolves the library class in his/her
>> platform DSC to that INF file. Not sure about the exact [section names]
>> in the library instance's INF file though.)
>>
>> Thanks!
>> Laszlo

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

Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: AllocatePool should use MemoryType.

2016-12-02 Thread Laszlo Ersek

On 12/02/16 03:03, Yao, Jiewen wrote:
> Thank you Laszlo.
> 
>  
> 
> This time, I did not use number purposely.
> 
>  
> 
> My thinking is that each patch resolves a specific problem. Although I
> fixed all of them at same time, they can be check in independently.

Ah! I looked briefly at the patches and was wondering about the
connection between them. And, I applied them all together and tested
them together.

When I applied them with "git am", I was slightly annoyed that I had to
rename the patches myself, because the file names didn't have numbers
(from the subject lines) and so "git am *" wouldn't keep the original order.

So apparently this was all intentional :) In the future I'd still
recommend to use numbers, also a cover letter, and in the cover letter
just state that the patches are independent. The subject on the cover
letter can be "various fixes" or some such.

Thanks!
Laszlo

> 
>  
> 
> Thank you
> 
> Yao Jiewen
> 
>  
> 
> *From:*Laszlo Ersek [mailto:ler...@redhat.com]
> *Sent:* Friday, December 2, 2016 5:51 AM
> *To:* Yao, Jiewen ; edk2-de...@ml01.01.org
> *Cc:* Kinney, Michael D ; Fan, Jeff
> 
> *Subject:* Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: AllocatePool
> should use MemoryType.
> 
>  
> 
> On 12/01/16 09:23, Jiewen Yao wrote:
>> PiSmmCore supports page level protection based upon the Memory Type
>> (EfiRuntimeServicesCode/EfiRuntimeServicesData) and PE image.
>> 
>> However, the Memory Type information is ignored in AllocatePool().
>> If a caller calls AllocatePool with EfiRuntimeServicesCode,
>> the final memory is still allocated as EfiRuntimeServicesData.
>> 
>> This patch supports AllocatePool with EfiRuntimeServicesCode.
>> 
>> Cc: Jeff Fan >
>> Cc: Michael D Kinney > >
>> Cc: Laszlo Ersek >
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jiewen Yao > >
>> ---
>>  MdeModulePkg/Core/PiSmmCore/PiSmmCore.h  |  13 ++-
>>  MdeModulePkg/Core/PiSmmCore/Pool.c   |  66 +---
>>  MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 114 +++-
>>  3 files changed, 124 insertions(+), 69 deletions(-)
> 
> series
> Regression-tested-by: Laszlo Ersek  >
> 
> (Please make sure to number the patches in the series next time.)
> 
> Thanks
> Laszlo
> 

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

[edk2] [shell] Redirected Environment Variables Are Not Saved Correctly

2016-12-02 Thread Tim Lewis

Test by doing this, immediately after booting:

Notice that echo has created a new environment variable _key to the value 4, 
But this variable does not show up when running "set"

Why? It is because the FileHandleWrappers does not use SetEnvironmentVariable. 
Instead, it tries to read and write the UEFI variable directly. But this method 
does not update the internal list of environment variables maintained by the 
Shell Protocol API.

This causes a lot of issues, because the results of any console redirection to 
an environment variable is not immediately visible.

Tim

[cid:image001.png@01D24C37.475327F0]
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Vlv2TbltDevicePkg:Add Intel I210 lan UNDI driver on Turbot2.

2016-12-02 Thread Wu, Mike

Looks good. Reviewed-by: Mike Wu 


Best Regards
Mike Wu


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of lushifex
Sent: Friday, December 02, 2016 4:58 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH] Vlv2TbltDevicePkg:Add Intel I210 lan UNDI driver on 
Turbot2.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: lushifex 
---
 Vlv2TbltDevicePkg/PlatformPkg.fdf| 7 +++
 Vlv2TbltDevicePkg/PlatformPkgGcc.fdf | 7 +++
 2 files changed, 14 insertions(+)

diff --git a/Vlv2TbltDevicePkg/PlatformPkg.fdf 
b/Vlv2TbltDevicePkg/PlatformPkg.fdf
index 907f88a..ae4ee2d 100644
--- a/Vlv2TbltDevicePkg/PlatformPkg.fdf
+++ b/Vlv2TbltDevicePkg/PlatformPkg.fdf
@@ -696,6 +696,13 @@ FILE FREEFORM = 878AC2CC-5343-46F2-B563-51F89DAF56BA {
 SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/RtkUndiDxe/$(DXE_ARCHITECTURE)/RtkUndiDxe.efi
 SECTION UI = "UNDI"
   }
+  # 32-bit E7006X3.EFI UNDI driver is not available.
+  !if $(DXE_ARCHITECTURE) == X64
+  FILE DRIVER = 0270D660-E7E2-4C6B-94B1-16B7FCD49351 {
+SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/I211PcieUndiDxe/$(DXE_ARCHITECTURE)/E7006X3.EFI
+SECTION UI = "UNDI"
+  }
+  !endif
   INF  MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
   INF  MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
   INF  MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
diff --git a/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf 
b/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf
index af2b6cb..43d20ee 100644
--- a/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf
+++ b/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf
@@ -649,6 +649,13 @@ FILE FREEFORM = 878AC2CC-5343-46F2-B563-51F89DAF56BA {
 SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/RtkUndiDxe/$(DXE_ARCHITECTURE)/RtkUndiDxe.efi
 SECTION UI = "UNDI"
   }
+  # 32-bit E7006X3.EFI UNDI driver is not available.
+  !if $(DXE_ARCHITECTURE) == X64
+  FILE DRIVER = 0270D660-E7E2-4C6B-94B1-16B7FCD49351 {
+SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/I211PcieUndiDxe/$(DXE_ARCHITECTURE)/E7006X3.EFI
+SECTION UI = "UNDI"
+  }
+  !endif
   INF  MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
   INF  MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
   INF  MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
-- 
2.7.0.windows.1


___
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

[edk2] [PATCH] Vlv2TbltDevicePkg:Add Intel I210 lan UNDI driver on Turbot2.

2016-12-02 Thread lushifex

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: lushifex 
---
 Vlv2TbltDevicePkg/PlatformPkg.fdf| 7 +++
 Vlv2TbltDevicePkg/PlatformPkgGcc.fdf | 7 +++
 2 files changed, 14 insertions(+)

diff --git a/Vlv2TbltDevicePkg/PlatformPkg.fdf 
b/Vlv2TbltDevicePkg/PlatformPkg.fdf
index 907f88a..ae4ee2d 100644
--- a/Vlv2TbltDevicePkg/PlatformPkg.fdf
+++ b/Vlv2TbltDevicePkg/PlatformPkg.fdf
@@ -696,6 +696,13 @@ FILE FREEFORM = 878AC2CC-5343-46F2-B563-51F89DAF56BA {
 SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/RtkUndiDxe/$(DXE_ARCHITECTURE)/RtkUndiDxe.efi
 SECTION UI = "UNDI"
   }
+  # 32-bit E7006X3.EFI UNDI driver is not available.
+  !if $(DXE_ARCHITECTURE) == X64
+  FILE DRIVER = 0270D660-E7E2-4C6B-94B1-16B7FCD49351 {
+SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/I211PcieUndiDxe/$(DXE_ARCHITECTURE)/E7006X3.EFI
+SECTION UI = "UNDI"
+  }
+  !endif
   INF  MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
   INF  MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
   INF  MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
diff --git a/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf 
b/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf
index af2b6cb..43d20ee 100644
--- a/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf
+++ b/Vlv2TbltDevicePkg/PlatformPkgGcc.fdf
@@ -649,6 +649,13 @@ FILE FREEFORM = 878AC2CC-5343-46F2-B563-51F89DAF56BA {
 SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/RtkUndiDxe/$(DXE_ARCHITECTURE)/RtkUndiDxe.efi
 SECTION UI = "UNDI"
   }
+  # 32-bit E7006X3.EFI UNDI driver is not available.
+  !if $(DXE_ARCHITECTURE) == X64
+  FILE DRIVER = 0270D660-E7E2-4C6B-94B1-16B7FCD49351 {
+SECTION PE32 = 
Vlv2MiscBinariesPkg/UNDI/I211PcieUndiDxe/$(DXE_ARCHITECTURE)/E7006X3.EFI
+SECTION UI = "UNDI"
+  }
+  !endif
   INF  MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
   INF  MdeModulePkg/Universal/Network/DpcDxe/DpcDxe.inf
   INF  MdeModulePkg/Universal/Network/MnpDxe/MnpDxe.inf
-- 
2.7.0.windows.1


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

Re: [edk2] [PATCH 0/3] MdePkg, MdeModulePkg, Vlv2TbltDevicePkg: 64-bit LoopTimes in S3 MEM_POLL

Re: [edk2] [shell] Pipe causes pool failure in Shell.c

[edk2] Handling multiple paths to the same EFI system partition?

[edk2] [PATCH v2 5/6] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to updated lib class header

[edk2] [PATCH v2 6/6] OvmfPkg/QemuFwCfgLib: support QEMU's DMA-like fw_cfg access method

[edk2] [PATCH v2 1/6] ArmVirtPkg/QemuFwCfgLib: remove superfluous InternalQemuFwCfgIsAvailable()

[edk2] [PATCH v2 4/6] OvmfPkg/QemuFwCfgLib: extend lib class header with more definitions

[edk2] [PATCH v2 3/6] ArmVirtPkg, OvmfPkg: QemuFwCfgLib: move DMA-related defs to lib class

[edk2] [PATCH v2 2/6] OvmfPkg/QemuFwCfgLib: move InternalQemuFwCfgIsAvailable() to lib instances

[edk2] [PATCH v2 0/6] OvmfPkg/QemuFwCfgLib: support the DMA-like interface

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

Re: [edk2] [shell] Pipe causes pool failure in Shell.c

Re: [edk2] [PATCH 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

[edk2] [shell] shift generates errors with no command-line options

Re: [edk2] [Patch v2 0/4] Fix SOURCE_DEBUG_ENABLE

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

[edk2] [shell] @ does not work before if, endif, for and many other command.

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

Re: [edk2] [PATCH 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

Re: [edk2] [PATCH 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

Re: [edk2] [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

Re: [edk2] [PATCH 0/7] Various: Remove EDK2 use of IntelFrameworkModulePkg legacy libs

Re: [edk2] [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

[edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script

Re: [edk2] [PATCH v4 3/5] MdeModulePkg: implement generic PCI I/O driver for non-discoverable devices

Re: [edk2] [PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

Re: [edk2] [PATCH 0/4] Fix runtime issue in XenBusDxe when compiled with GCC5

Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: AllocatePool should use MemoryType.

[edk2] [shell] Redirected Environment Variables Are Not Saved Correctly

Re: [edk2] [PATCH] Vlv2TbltDevicePkg:Add Intel I210 lan UNDI driver on Turbot2.

[edk2] [PATCH] Vlv2TbltDevicePkg:Add Intel I210 lan UNDI driver on Turbot2.

36 matches

Site Navigation

Mail list logo

Footer information