Date   

Re: [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.

Maciej Rabeda
 

Much appreciated, I'll submit the patch tomorrow :)

On 31-Mar-20 16:50, Gao, Zhichao wrote:
Acked-by: Zhichao Gao <zhichao.gao@...>

-----Original Message-----
From: Fu, Siyuan <siyuan.fu@...>
Sent: Tuesday, March 31, 2020 7:54 PM
To: devel@edk2.groups.io; lersek@...; Ni, Ray <ray.ni@...>;
Gao, Zhichao <zhichao.gao@...>
Cc: maciej.rabeda@...
Subject: RE: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive
flow.

Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: 2020年3月25日 19:34
To: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Cc: devel@edk2.groups.io; maciej.rabeda@...
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4
receive flow.

Ray, Zhichao,

On 02/27/20 12:02, Maciej Rabeda wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already recycled
packet with EFI_SUCCESS token status and finally dereference invalid
pointers from RxData structure.

Cc: Ray Ni <ray.ni@...>
Cc: Zhichao Gao <zhichao.gao@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
---
 ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
can you please review this ShellPkg patch? It's been on the list for
almost a month now.

Thanks
Laszlo

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

 ON_EXIT:

+  //
+  // Recycle the packet before reusing RxToken  //
+ gBS->SignalEvent (Private->IpChoice ==
PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
+
   if (Private->RxCount < Private->SendNum) {
     //
     // Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
     //
     Private->Status = EFI_SUCCESS;
   }
-  //
-  // Singal to recycle the each rxdata here, not at the end of process.
-  //
-  gBS->SignalEvent (Private->IpChoice ==
PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
 }

 /**







Re: [PATCH v3 1/3] MdeModulePkg Variable: Return GetVariable() attr if EFI_BUFFER_TOO_SMALL

Michael Kubacki
 

That has been spelled incorrectly for about 9 years. The file (like many others) also has other spelling errors such as the following. I suggest this be fixed in a separate commit/series focused on fixing spelling errors.

--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -102,7 +102,7 @@ AUTH_VAR_LIB_CONTEXT_OUT mAuthContextOut;
This function writes data to the FWH at the correct LBA even if the LBAs
are fragmented.

- @param Global Pointer to VARAIBLE_GLOBAL structure.
+ @param Global Pointer to VARIABLE_GLOBAL structure.
@param Volatile Point out the Variable is Volatile or Non-Volatile.
@param SetByIndex TRUE if target pointer is given as index.
FALSE if target pointer is absolute.
@@ -504,7 +504,7 @@ InitializeVariableQuota (

@return EFI_SUCCESS Reclaim operation has finished successfully.
@return EFI_OUT_OF_RESOURCES No enough memory resources or variable space.
- @return Others Unexpect error happened during reclaim operation.
+ @return Others Unexpected error happened during reclaim operation.

**/
EFI_STATUS
@@ -2561,7 +2561,7 @@ VariableServiceSetVariable (
}

//
- // Check for reserverd bit in variable attribute.
+ // Check for reserved bit in variable attribute.
// EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated but we still allow
// the delete operation of common authenticated variable at user physical presence.
// So leave EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS attribute check to AuthVariableLib
@@ -3381,7 +3381,7 @@ ConvertNormalVarStorageToAuthVarStorage (
VARIABLE_HEADER *StartPtr;
UINT8 *NextPtr;
VARIABLE_HEADER *EndPtr;
- UINTN AuthVarStroageSize;
+ UINTN AuthVarStorageSize;
AUTHENTICATED_VARIABLE_HEADER *AuthStartPtr;
VARIABLE_STORE_HEADER *AuthVarStorage;

On 3/31/2020 1:18 AM, Jiang, Guomin wrote:
There is a spell error in the comments of VariableServiceGetVariable() in Variable.c.
- @return EFI_BUFFER_TO_SMALL DataSize is too small for the result.
+ @return EFI_BUFFER_TOO_SMALL DataSize is too small for the result.
Need create new bugs for it or fix in this comment directly?

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Wang, Jian J
Sent: Monday, March 30, 2020 12:16 PM
To: michael.kubacki@...; devel@edk2.groups.io
Cc: Bret Barkelew <Bret.Barkelew@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D <michael.d.kinney@...>;
Wu, Hao A <hao.a.wu@...>
Subject: Re: [edk2-devel] [PATCH v3 1/3] MdeModulePkg Variable: Return
GetVariable() attr if EFI_BUFFER_TOO_SMALL


Reviewed-by: Jian J Wang <jian.j.wang@...>

Regards,
Jian

-----Original Message-----
From: michael.kubacki@... <michael.kubacki@...>
Sent: Saturday, March 28, 2020 5:56 AM
To: devel@edk2.groups.io
Cc: Bret Barkelew <Bret.Barkelew@...>; Gao, Liming
<liming.gao@...>; Kinney, Michael D
<michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>;
Wu, Hao A <hao.a.wu@...>
Subject: [PATCH v3 1/3] MdeModulePkg Variable: Return GetVariable()
attr if EFI_BUFFER_TOO_SMALL

From: Michael Kubacki <michael.kubacki@...>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2062

The UEFI specification v2.8 Errata A Section 8.2 "GetVariable()"
"Attributes" parameter description states:

"If not NULL, a pointer to the memory location to return the
attributes bitmask for the variable. See 'Related Definitions.'
If not NULL, then Attributes is set on output both when EFI_SUCCESS
and when EFI_BUFFER_TOO_SMALL is returned."

The attributes were previously only returned from the implementation
in Variable.c on EFI_SUCCESS. They are now returned on EFI_SUCCESS or
EFI_BUFFER_TOO_SMALL according to spec.

Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Liming Gao <liming.gao@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>
---
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 10
+++++++---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.
c |
10 ++++++----
2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index d23aea4bc712..1e71fc642c76 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -18,6 +18,8 @@

Copyright (c) 2006 - 2020, Intel Corporation. All rights
reserved.<BR>
(C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
+Copyright (c) Microsoft Corporation.<BR>
+
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -2379,9 +2381,6 @@ VariableServiceGetVariable (
}

CopyMem (Data, GetVariableDataPtr (Variable.CurrPtr,
mVariableModuleGlobal->VariableGlobal.AuthFormat), VarDataSize);
- if (Attributes != NULL) {
- *Attributes = Variable.CurrPtr->Attributes;
- }

*DataSize = VarDataSize;
UpdateVariableInfo (VariableName, VendorGuid, Variable.Volatile,
TRUE, FALSE, FALSE, FALSE, &gVariableInfo); @@ -2395,6 +2394,11 @@
VariableServiceGetVariable (
}

Done:
+ if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {
+ if (Attributes != NULL && Variable.CurrPtr != NULL) {
+ *Attributes = Variable.CurrPtr->Attributes;
+ }
+ }
ReleaseLockOnlyAtBootTime (&mVariableModuleGlobal-
VariableGlobal.VariableServicesLock);
return Status;
}
diff --git
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c
index 2cf0ed32ae55..ca833fb0244d 100644
---
a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c
+++
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
e.c
@@ -14,6 +14,7 @@
InitCommunicateBuffer() is really function to check the variable data size.

Copyright (c) 2010 - 2019, Intel Corporation. All rights
reserved.<BR>
+Copyright (c) Microsoft Corporation.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -642,10 +643,6 @@ FindVariableInRuntimeCache (
}

CopyMem (Data, GetVariableDataPtr (RtPtrTrack.CurrPtr,
mVariableAuthFormat), TempDataSize);
- if (Attributes != NULL) {
- *Attributes = RtPtrTrack.CurrPtr->Attributes;
- }
-
*DataSize = TempDataSize;

UpdateVariableInfo (VariableName, VendorGuid,
RtPtrTrack.Volatile, TRUE, FALSE, FALSE, TRUE, &mVariableInfo); @@
-661,6 +658,11 @@ FindVariableInRuntimeCache (
}

Done:
+ if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {
+ if (Attributes != NULL && RtPtrTrack.CurrPtr != NULL) {
+ *Attributes = RtPtrTrack.CurrPtr->Attributes;
+ }
+ }
mVariableRuntimeCacheReadLock = FALSE;

return Status;
--
2.16.3.windows.1


Re: [PATCH] .azurepipelines: Enable CI for OvmfPkg and EmulatorPkg

Ard Biesheuvel
 

On 3/31/20 6:26 PM, Sean via Groups.Io wrote:
On Mon, Mar 30, 2020 at 11:41 PM, Ard Biesheuvel wrote:
Not sure I follow. Which command line are we talking about?
@Ard - In this Platform CI, ArmVirt is building and running AARCH64 but not ARM 32bit.  Would it be valuable to build for ARM too?
Yes, absolutely.

I prototyped it but want to make sure I am calling QEMU with the parameters you would expect to work with ArmVirtPkg
qemu-system-arm -M virt -cpu cortex-a15 -pflash /home/vsts/work/1/s/Build/ArmVirtQemu-ARM/DEBUG_GCC5/FV/QEMU_EFI.fd -m 1024 -net none -serial stdio -drive file=fat:rw:/home/vsts/work/1/s/Build/ArmVirtQemu-ARM/DEBUG_GCC5/VirtualDrive,format=raw,media=disk -display none
The parameters look fine. Note that you can use the qemu-system-aarch64 binary as well, so no need to install both.

PR build results can be seen here: https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=5074&view=results
PR code change: https://github.com/spbrogan/edk2/pull/14
Yep, looking good. The main focus is obviously on X64 and AARCH64, but that only increases the risk that we might regress on IA32 or ARM without anyone noticing. So having both IA32 and ARM is a good thing.


Re: [PATCH] .azurepipelines: Enable CI for OvmfPkg and EmulatorPkg

Sean
 

On Mon, Mar 30, 2020 at 11:41 PM, Ard Biesheuvel wrote:
Not sure I follow. Which command line are we talking about?
@Ard - In this Platform CI, ArmVirt is building and running AARCH64 but not ARM 32bit.  Would it be valuable to build for ARM too?  

I prototyped it but want to make sure I am calling QEMU with the parameters you would expect to work with ArmVirtPkg
 
qemu-system-arm -M virt -cpu cortex-a15 -pflash /home/vsts/work/1/s/Build/ArmVirtQemu-ARM/DEBUG_GCC5/FV/QEMU_EFI.fd -m 1024 -net none -serial stdio -drive file=fat:rw:/home/vsts/work/1/s/Build/ArmVirtQemu-ARM/DEBUG_GCC5/VirtualDrive,format=raw,media=disk -display none


PR build results can be seen here: https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=5074&view=results
PR code change: https://github.com/spbrogan/edk2/pull/14


Re: [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

Sean
 

A couple of thoughts. 
1. I would suggest that ASSERT should not be the only protection for an invalid operation as ASSERT is usually disabled on release builds.  
2. We do have a library to make this more explicit and common.  https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h#L548

Thanks


How to add ignore "IgnoreFiles" for CharEncodingCheck

Zhang, Shenglei
 

Hi Sean,

 

I am introducing third party project oniguruma as submodule into edk2, and want to skip CharEncodingCheck for certain files in oniguruma.

I tried to add changes like below, but CI build failed.

"CharEncodingCheck": {

"IgnoreFiles": [[(MdeModulePkg/Universal/RegularExpressionDxe/oniguruma/test/testc.c),(MdeModulePkg/Universal/RegularExpressionDxe/oniguruma/windows/testc.c)]

}

 

So what should I do in CharEncodingCheck_plug_in.yaml? Or how do you handle this kind of case like openssl?        

 

I see you add this to edk2 and hope you could resolve my query. Thanks in advance.

 

Best Regards,

Shenglei

 


Re: [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.

Gao, Zhichao
 

Acked-by: Zhichao Gao <zhichao.gao@...>

-----Original Message-----
From: Fu, Siyuan <siyuan.fu@...>
Sent: Tuesday, March 31, 2020 7:54 PM
To: devel@edk2.groups.io; lersek@...; Ni, Ray <ray.ni@...>;
Gao, Zhichao <zhichao.gao@...>
Cc: maciej.rabeda@...
Subject: RE: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive
flow.

Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: 2020年3月25日 19:34
To: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Cc: devel@edk2.groups.io; maciej.rabeda@...
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4
receive flow.

Ray, Zhichao,

On 02/27/20 12:02, Maciej Rabeda wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already recycled
packet with EFI_SUCCESS token status and finally dereference invalid
pointers from RxData structure.

Cc: Ray Ni <ray.ni@...>
Cc: Zhichao Gao <zhichao.gao@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
---
ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
can you please review this ShellPkg patch? It's been on the list for
almost a month now.

Thanks
Laszlo

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

ON_EXIT:

+ //
+ // Recycle the packet before reusing RxToken //
+ gBS->SignalEvent (Private->IpChoice ==
PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
+
if (Private->RxCount < Private->SendNum) {
//
// Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
//
Private->Status = EFI_SUCCESS;
}
- //
- // Singal to recycle the each rxdata here, not at the end of process.
- //
- gBS->SignalEvent (Private->IpChoice ==
PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private
- RxToken.Packet.RxData)->RecycleSignal);
}

/**


Re: [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

Michael D Kinney
 

ARM and AARCH64 have a compiler intrinsic lib that is linked against all modules.

[LibraryClasses.ARM, LibraryClasses.AARCH64]
#
# It is not possible to prevent ARM compiler calls to generic intrinsic functions.
# This library provides the instrinsic functions generated by a given compiler.
# [LibraryClasses.ARM] and NULL mean link this library into all ARM images.
#
NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf

Can we use this same technique for IA32/X64 VS builds?

Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On
Behalf Of Laszlo Ersek
Sent: Tuesday, March 31, 2020 5:42 AM
To: Ard Biesheuvel <ard.biesheuvel@...>; edk2-
devel-groups-io <devel@edk2.groups.io>;
macarl@...
Subject: Re: [edk2-devel] [PATCH] CryptoPkg/FltUsedLib:
Add FltUsedLib for float.

On 03/30/20 23:41, Ard Biesheuvel wrote:
On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via
Groups.Io
<macarl@...> wrote:

So it's not required by OpenSSL, it's required by the
compiler whenever floating point is used, which can be in
multiple places. For example, this is used in mu_plus
(the Microsoft UEFI value add to EDK2) by the
OnScreenKeyboard driver as well as the UiToolKit driver.

OK, so this is the part that was not obvious to me. This
is basically
code that exists on top of edk2 (consumes edk2) *AND*
uses floating
point *internally*.

If you happen to use BaseCryptLib or the intrinsic in a
driver that happens to need crypto as well, it can break
due to multiple.

I do agree, this only applies to MSVC and requiring
every platform to add a line in their DSC would be a
situation I would prefer to avoid if possible. Is there a
way to specify a library dependency only if a toolchain
is being used?

Yes, so this either belongs in one of the
IntrinsicsLibs we have, or
in the various Entrypoint libraries we have for PEIMs,
DXEs, etc.

However, given that we are talking about static
libraries here, adding
a source file that *only* defines __fltused (and
nothing else) to any
library should also work, as the resulting object file
will only be
incorporated by the linker if it is needed to satisfy a
symbol
dependency, and so it can never cause a conflict if it
is the only
symbol in the object.
IMO: if edk2 intends to support out-of-tree modules that
themselves
utilize floating point, *with or without* a dependency on
OpensslLib,
then we should add Ard's suggestion:

[Sources]
+ FloatUsed.c | MSFT

to some of the "most core" edk2 library instances, i.e.
those that *all*
modules of the given module type inevitably depend on.
The entry point
libs look like a great idea to me.

That should link the _fltused external definition exactly
once into all
modules built with MSVC, regardless of whether each such
module uses
floating point or not.

Thanks
Laszlo



Re: API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support

Laszlo Ersek
 

On 03/31/20 11:22, Leif Lindholm wrote:
On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote:
Leif,
Please understand that the concern of this change is all the platforms that uses
this serial port lib must be changed otherwise build breaks.
Yes. This is the nature of collaborative development.
This is something we on the ARM side have lived with since we got
involved with EDK2, being the less-deployed user of the codebase, and
that is fine.

TianoCore specifically produced the UDK to make life easier for those
who are unable to track upstream, and we have followed that up with
stable tags. I would highly recommend that any team that feels that
this change would be too much effort to move to edk2-stable202002. Of
course, they would then need to manually cherry-pick any improvements
and security fixes from master. That is their choice.

Nevertheless, breaking existing platforms is a side effect, not a
goal. So if an alternative solution can be found (which is not a hack
that is going to affect the codebase negatively over time and simply
need to be fixed properly at a later date), then clearly that is
preferable.

"This breaks many platforms" is a good argument for seeing if a
solution can be found that does not break (as) many platforms. It is
not an argument for duplicating drivers when the change needed for
those platforms is trivial.

These days, Linux tends to deal with API breakages (and other things)
using a semantic patch tool called Coccinelle[1]. It would not be
unreasonable, and indeed it would be helpful to us on the non-x86 side
if something similar was adopted (and semantic patches mandated) for
API breakages in EDK2.

[1] http://coccinelle.lip6.fr/sp.php
Two comments:

(1) One of the reasons why I would like to keep all platforms in a
single tree is to deal with API changes like this. That way, someone
proposing an API change would at least have the chance to fix up all the
consumer sites. Of course it would require diligent review from the
other pkg maintainers, but it could be implemented without any temprary
breakage in the git history even.


(2) Specifically about this problem. The vendor GUID approach is not a
bad one. How about the following alternative:

(2.1) Patch#1:

in the "MdeModulePkg/Library/BaseSerialPortLib16550" directory,
introduce a header file called "GetClock.h". This header file should
declare:

UINT32
GetClock (
VOID
);

Note: EFIAPI not needed.

The header file should be added to the existent
"BaseSerialPortLib16550.inf" file, in the [Sources] section.

Furthermore, in the same patch, introduce a new source file called
"GetClockPcd.c". (Add this file to the INF file as well.) The source
file should do:

#include <Library/PcdLib.h>
#include "GetClock.h"

UINT32
GetClock (
VOID
)
{
return PcdGet32 (PcdSerialClockRate);
}

Finally, in the same patch#1, modify "BaseSerialPortLib16550.c" to
#include "GetClock.h", and to call GetClock() in place of the current
PcdGet32 (PcdSerialClockRate) calls.

This patch basically splits the PcdGet32 (PcdSerialClockRate) call to a
separate translation unit, hiding it behind the more abstract GetClock()
API.


(2.2) Patch#2:

In the *same* "MdeModulePkg/Library/BaseSerialPortLib16550" directory,
introduce three new files:

- BaseSerialPortLibDynamicFrequency16550.inf
- BaseSerialPortLibDynamicFrequency16550.uni
- GetClockDynamicFrequency.c

I think the contents of these files must be fairly obvious at this
point, but anyway:

(2.2.1) INF file:

- modified copy of the INF file from (2.1)

- Update file-top comment, copyright notice, BASE_NAME, MODULE_UNI_FILE,
FILE_GUID.

- [LibraryClasses]: add dependency on SerialUartClockLib

- [Sources]: replace "GetClockPcd.c" with "GetClockDynamicFrequency.c"

- [Pcd]: drop PcdSerialClockRate

(2.2.2) UNI file: copy and modify the existent UNI file as needed.

(2.2.3) GetClockDynamicFrequency.c:

#include <Library/SerialUartClockLib.h>
#include "GetClock.h"

UINT32
GetClock (
VOID
)
{
return BaseSerialPortGetClock ();
}


This way, platforms currently consuming
"MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf"
will see no change whatsoever.

Platforms needing the dynamic frequency can resolve SerialPortLib to
"MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLibDynamicFrequency16550.inf",
*and* also resolve SerialUartClockLib to an instance that matches their
needs.

All implementation except the GetClock() one will be shared between the
two lib instances "BaseSerialPortLib16550" and
"BaseSerialPortLibDynamicFrequency16550".


This is a frequent pattern in edk2 -- refer to the various module
directories that contain two or more INF files. For example:

$ git ls-files -- \
MdePkg/*inf \
MdeModulePkg/*inf \
NetworkPkg/*inf \
PcAtChipsetPkg/*inf \
SecurityPkg/*inf \
ShellPkg/*inf \
UefiCpuPkg/*inf \
| rev \
| cut -f 2- -d / \
| rev \
| sort \
| uniq -d

This will produce a list of directories where each directory contains at
least two INF files:

MdeModulePkg/Bus/I2c/I2cDxe
MdeModulePkg/Core/PiSmmCore
MdeModulePkg/Library/DxeCapsuleLibFmp
MdeModulePkg/Library/DxeCoreMemoryAllocationLib
MdeModulePkg/Library/DxeResetSystemLib/UnitTest
MdeModulePkg/Library/LzmaCustomDecompressLib
MdeModulePkg/Library/PiSmmCoreMemoryAllocationLib
MdeModulePkg/Library/SmmLockBoxLib
MdeModulePkg/Logo
MdeModulePkg/Universal/CapsulePei
MdeModulePkg/Universal/EbcDxe
MdeModulePkg/Universal/FaultTolerantWriteDxe
MdeModulePkg/Universal/Variable/RuntimeDxe
MdePkg/Library/BaseIoLibIntrinsic
MdePkg/Library/BaseUefiDecompressLib
MdePkg/Library/PciSegmentLibSegmentInfo
MdePkg/Library/UefiDevicePathLib
MdePkg/Test/UnitTest/Library/BaseLib
MdePkg/Test/UnitTest/Library/BaseSafeIntLib
PcAtChipsetPkg/Library/AcpiTimerLib
SecurityPkg/HddPassword
SecurityPkg/Library/HashLibBaseCryptoRouter
SecurityPkg/Library/Tpm2DeviceLibDTpm
SecurityPkg/Library/Tpm2DeviceLibRouter
SecurityPkg/Tcg/Opal/OpalPassword
SecurityPkg/Tcg/Tcg2Config
ShellPkg/DynamicCommand/DpDynamicCommand
ShellPkg/DynamicCommand/TftpDynamicCommand
UefiCpuPkg/CpuFeatures
UefiCpuPkg/Library/CpuExceptionHandlerLib
UefiCpuPkg/Library/CpuTimerLib
UefiCpuPkg/Library/MpInitLib
UefiCpuPkg/Library/RegisterCpuFeaturesLib
UefiCpuPkg/Library/SmmCpuFeaturesLib
UefiCpuPkg/PiSmmCommunication

If you look at INF files inside any given such directory, you'll find
that the [Sources] sections between them have non-empty intersections,
but also differences. That's the exact same pattern we should use here, IMO.

(I intentionally used the core packages in this example.)

Thanks
Laszlo


Re: API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support

Leif Lindholm
 

Hi Ray,

I think it's good to start doing it voluntarily, or for changes
expected to affect many platforms. Over time, as people become more
familiar with the tool, it would make sense to make it first
recommended and then mandatory.

For Linux, the coccinelle diff is frequently included in the commit
message, or (especially when used for changing deprecated interfaces
or poor coding practice) they are copmmitted to the repository (under
scripts/coccinelle).

Regards,

Leif

On Tue, Mar 31, 2020 at 12:11:03 +0000, Ni, Ray wrote:
Leif,
Thanks for introducing such an interesting tool.
I see this too is very useful for code refactoring.
It's a game changing tool😊

To help me understand, do you suggest MAYBE when incompatible changes like this
happen, the change owners propose the semantic patches for all platforms?


Thanks,
Ray


-----Original Message-----
From: Leif Lindholm <leif@...>
Sent: Tuesday, March 31, 2020 5:22 PM
To: Ni, Ray <ray.ni@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>; Jiang, Guomin <guomin.jiang@...>; devel@edk2.groups.io; Andrew
Fish <afish@...>; Laszlo Ersek <lersek@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-devel] API breakages and their implications. Was: [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq
Support

On Tue, Mar 31, 2020 at 01:53:21 +0000, Ni, Ray wrote:
Leif,
Please understand that the concern of this change is all the platforms that uses
this serial port lib must be changed otherwise build breaks.
Yes. This is the nature of collaborative development.
This is something we on the ARM side have lived with since we got
involved with EDK2, being the less-deployed user of the codebase, and
that is fine.

TianoCore specifically produced the UDK to make life easier for those
who are unable to track upstream, and we have followed that up with
stable tags. I would highly recommend that any team that feels that
this change would be too much effort to move to edk2-stable202002. Of
course, they would then need to manually cherry-pick any improvements
and security fixes from master. That is their choice.

Nevertheless, breaking existing platforms is a side effect, not a
goal. So if an alternative solution can be found (which is not a hack
that is going to affect the codebase negatively over time and simply
need to be fixed properly at a later date), then clearly that is
preferable.

"This breaks many platforms" is a good argument for seeing if a
solution can be found that does not break (as) many platforms. It is
not an argument for duplicating drivers when the change needed for
those platforms is trivial.

These days, Linux tends to deal with API breakages (and other things)
using a semantic patch tool called Coccinelle[1]. It would not be
unreasonable, and indeed it would be helpful to us on the non-x86 side
if something similar was adopted (and semantic patches mandated) for
API breakages in EDK2.

[1] http://coccinelle.lip6.fr/sp.php

Regards,

Leif

Ard,
Using Guided HOB sounds a good idea to me: )
The benefits of using HOB is:
Length field in the HOB header can be used for extension if more parameters are needed.
DXE can have the HOB access as well.

EFI_SEC_HOB_DATA_PPI can be used to return the new Guided HOB from SEC phase if needed.

Thanks,
Ray


-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@...>
Sent: Monday, March 30, 2020 3:45 PM
To: Leif Lindholm <leif@...>
Cc: Jiang, Guomin <guomin.jiang@...>; devel@edk2.groups.io; pankaj.bansal@...; Ni, Ray
<ray.ni@...>;
Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Ma, Maurice <maurice.ma@...>; Dong,
Guo <guo.dong@...>; You, Benjamin <benjamin.you@...>; Meenakshi Aggarwal
<meenakshi.aggarwal@...>; Varun Sethi <V.Sethi@...>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@...>
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: UART Dynamic clock freq Support

On Mon, 30 Mar 2020 at 09:35, Leif Lindholm <leif@...> wrote:

Hi Jiang,

It is not a question of effort of copying a driver, it is a question
that copying drivers is something that should be avoided wherever
practically possible. I did not think this topic was still under
debate.

If the existing 16550 SerialPortLib is overspecialised to the point
where it only works on a subset of 16550 implementations, then it
should change. There are going to be more non-PC systems turning up
with 16550 UARTs - should they each copy/modify their drivers?

If there are better ways of solving that problem, please suggest.
But more duplicated drivers is not the answer.
Could we use a GUIDed HOB? If it exists, we use its contents, and if
it doesn't, we use the default set by the FixedPCD.


Re: [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.

Siyuan, Fu
 

Reviewed-by: Siyuan Fu <siyuan.fu@...>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: 2020年3月25日 19:34
To: Ni, Ray <ray.ni@...>; Gao, Zhichao <zhichao.gao@...>
Cc: devel@edk2.groups.io; maciej.rabeda@...
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive
flow.

Ray, Zhichao,

On 02/27/20 12:02, Maciej Rabeda wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already
recycled packet with EFI_SUCCESS token status and finally
dereference invalid pointers from RxData structure.

Cc: Ray Ni <ray.ni@...>
Cc: Zhichao Gao <zhichao.gao@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
---
ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
can you please review this ShellPkg patch? It's been on the list for
almost a month now.

Thanks
Laszlo

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

ON_EXIT:

+ //
+ // Recycle the packet before reusing RxToken
+ //
+ gBS->SignalEvent (Private->IpChoice ==
PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal);
+
if (Private->RxCount < Private->SendNum) {
//
// Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
//
Private->Status = EFI_SUCCESS;
}
- //
- // Singal to recycle the each rxdata here, not at the end of process.
- //
- gBS->SignalEvent (Private->IpChoice ==
PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private-
RxToken.Packet.RxData)->RecycleSignal);
}

/**


Re: [PATCH edk2-platforms 1/1] Maintainers: switch to my @arm.com email address

Ard Biesheuvel
 

On 3/31/20 2:33 PM, Leif Lindholm wrote:
On Tue, Mar 31, 2020 at 13:12:28 +0200, Ard Biesheuvel wrote:
I no longer work for Linaro so switch to my ARM email address.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
Reviewed-by: Leif Lindholm <leif@...>
(Not that I can be trusted on the spelling...)
Thanks

Pushed as 864987702e01433f6c46c9d974a2233f436b8394


Re: [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

Laszlo Ersek
 

On 03/30/20 23:41, Ard Biesheuvel wrote:
On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via Groups.Io
<macarl@...> wrote:

So it's not required by OpenSSL, it's required by the compiler whenever floating point is used, which can be in multiple places. For example, this is used in mu_plus (the Microsoft UEFI value add to EDK2) by the OnScreenKeyboard driver as well as the UiToolKit driver.
OK, so this is the part that was not obvious to me. This is basically
code that exists on top of edk2 (consumes edk2) *AND* uses floating
point *internally*.

If you happen to use BaseCryptLib or the intrinsic in a driver that happens to need crypto as well, it can break due to multiple.

I do agree, this only applies to MSVC and requiring every platform to add a line in their DSC would be a situation I would prefer to avoid if possible. Is there a way to specify a library dependency only if a toolchain is being used?
Yes, so this either belongs in one of the IntrinsicsLibs we have, or
in the various Entrypoint libraries we have for PEIMs, DXEs, etc.

However, given that we are talking about static libraries here, adding
a source file that *only* defines __fltused (and nothing else) to any
library should also work, as the resulting object file will only be
incorporated by the linker if it is needed to satisfy a symbol
dependency, and so it can never cause a conflict if it is the only
symbol in the object.
IMO: if edk2 intends to support out-of-tree modules that themselves
utilize floating point, *with or without* a dependency on OpensslLib,
then we should add Ard's suggestion:

[Sources]
+ FloatUsed.c | MSFT

to some of the "most core" edk2 library instances, i.e. those that *all*
modules of the given module type inevitably depend on. The entry point
libs look like a great idea to me.

That should link the _fltused external definition exactly once into all
modules built with MSVC, regardless of whether each such module uses
floating point or not.

Thanks
Laszlo


Re: [PATCH edk2-platforms 1/1] Maintainers: switch to my @arm.com email address

Leif Lindholm
 

On Tue, Mar 31, 2020 at 13:12:28 +0200, Ard Biesheuvel wrote:
I no longer work for Linaro so switch to my ARM email address.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...>
Reviewed-by: Leif Lindholm <leif@...>
(Not that I can be trusted on the spelling...)

---
Maintainers.txt | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Maintainers.txt b/Maintainers.txt
index 475f7d530b85..0e50b6fdf36a 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -80,7 +80,7 @@ EDK II Platforms Packages:

96Boards
F: Platform/96Boards/
-M: Ard Biesheuvel <ard.biesheuvel@...>
+M: Ard Biesheuvel <ard.biesheuvel@...>
M: Leif Lindholm <leif@...>

AMD Seattle
@@ -88,24 +88,24 @@ F: Platform/AMD/OverdriveBoard/
F: Platform/LeMaker/CelloBoard/
F: Platform/SoftIron/
F: Silicon/AMD/Styx/
-M: Ard Biesheuvel <ard.biesheuvel@...>
+M: Ard Biesheuvel <ard.biesheuvel@...>
M: Leif Lindholm <leif@...>

ARM
F: Platform/ARM/
-R: Ard Biesheuvel <ard.biesheuvel@...>
+R: Ard Biesheuvel <ard.biesheuvel@...>
R: Thomas Abraham <thomas.abraham@...>
M: Leif Lindholm <leif@...>

BeagleBoard:
F: Platform/BeagleBoard/
F: Silicon/TexasInstruments/
-R: Ard Biesheuvel <ard.biesheuvel@...>
+R: Ard Biesheuvel <ard.biesheuvel@...>
M: Leif Lindholm <leif@...>

Comcast
F: Platform/Comcast/
-M: Ard Biesheuvel <ard.biesheuvel@...>
+M: Ard Biesheuvel <ard.biesheuvel@...>
M: Leif Lindholm <leif@...>

OptionRomPkg
@@ -116,14 +116,14 @@ M: Ray Ni <ray.ni@...>
DisplayLink
F: Drivers/DisplayLink/
M: Leif Lindholm <leif@...>
-M: Ard Biesheuvel <ard.biesheuvel@...>
+M: Ard Biesheuvel <ard.biesheuvel@...>
R: Andy Hayes <andy.hayes@...>

HiSilicon
F: Platform/Hisilicon/
F: Silicon/Hisilicon/
M: Leif Lindholm <leif@...>
-R: Ard Biesheuvel <ard.biesheuvel@...>
+R: Ard Biesheuvel <ard.biesheuvel@...>

Features/Intel
F: Features/Intel/
@@ -241,7 +241,7 @@ Miscellaneous drivers
F: Silicon/Atmel/
F: Silicon/Openmoko/
F: Silicon/Synopsys/DesignWare/
-R: Ard Biesheuvel <ard.biesheuvel@...>
+R: Ard Biesheuvel <ard.biesheuvel@...>
M: Leif Lindholm <leif@...>

NXP platforms and silicon
@@ -253,7 +253,7 @@ R: Meenakshi Aggarwal <meenakshi.aggarwal@...>
Raspberry Pi platforms and silicon
F: Platform/RaspberryPi/
F: Silicon/Broadcom/
-M: Ard Biesheuvel <ard.biesheuvel@...>
+M: Ard Biesheuvel <ard.biesheuvel@...>
M: Leif Lindholm <leif@...>
R: Pete Batard <pete@...>

@@ -261,5 +261,5 @@ Socionext platforms and silicon
F: Platform/Socionext/
F: Silicon/NXP/Library/Pcf8563RealTimeClockLib/
F: Silicon/Socionext/
-M: Ard Biesheuvel <ard.biesheuvel@...>
+M: Ard Biesheuvel <ard.biesheuvel@...>
M: Leif Lindholm <leif@...>
--
2.17.1


Re: [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

Laszlo Ersek
 

On 03/31/20 09:13, Ard Biesheuvel wrote:
On Tue, 31 Mar 2020 at 03:40, Jiang, Guomin <guomin.jiang@...> wrote:

Hi Laszlo,

Thanks for you spending time review the changes.

And I just want to present how to reproduce the build error.

When build OvmfPkgX64, you can encounter this issue with your local change. The error as below:
TcgPei.lib(AutoGen.obj) : error LNK2001: unresolved external symbol _fltused
c:\sourcecodes\tiano\Build\OvmfX64\DEBUG_VS2019\X64\SecurityPkg\Tcg\TcgPei\TcgPei\DEBUG\TcgPei.dll : fatal error LNK1120: 1 unresolved externals

The build command: build -p OvmfPkg\OvmfPkgX64.dsc -b DEBUG -t VS2019 -a X64 -D TPM_ENABLE
The code changes for reproducing this symptom:
- int GLOBAL_USED _fltused = 1;
+ //int GLOBAL_USED _fltused = 1;
The machine: WIN10
The branch: edk2 master
Doesn't the build error go away as well if you simply add FloatUsed.c
to all BaseCryptLib flavours if Visual Studio is being used?

Something like

diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 1bbe4f435aec..f40bf18e7f5d 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -27,6 +27,7 @@ [Defines]
#

[Sources]
+ FloatUsed.c | MSFT
InternalCryptLib.h
Hash/CryptMd4.c
Hash/CryptMd5.c
This is *exactly* what I've had in mind!

(With the additional (optional) tweak that IMO "FloatUsed.c" belongs
with the openssl INF files, as those are where the floating point usage
originates from. The cryptlib instances themselves have no floating
point code, AIUI.)

Thanks
Laszlo


Re: [PATCH v2 05/28] Silicon/Maxim: Add comments in Ds1307RtcLib

Leif Lindholm
 

On Fri, Mar 20, 2020 at 20:05:20 +0530, Pankaj Bansal wrote:
From: Pankaj Bansal <pankaj.bansal@...>

Add comments to explain the register read and write operation
on Ds1307. These comments have been referred from data sheet:

https://datasheets.maximintegrated.com/en/ds/DS1307.pdf

Signed-off-by: Pankaj Bansal <pankaj.bansal@...>
Reviewed-by: Leif Lindholm <leif@...>

---
Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
index fd7a8696e405..444e01124811 100644
--- a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
+++ b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
@@ -28,6 +28,11 @@ STATIC EFI_I2C_MASTER_PROTOCOL *mI2cMaster;

/**
Read RTC register.
+ Data Read-Slave Transmitter Mode
+
+ <Slave Address> <Word Address (n)> <Slave Address> <Data(n)> <Data(n+1)> <Data(n+2)> <Data(n+X)>
+
+ The first byte is received and handled as in the slave receiver mode.

@param RtcRegAddr Register offset of RTC to be read.

@@ -69,6 +74,9 @@ RtcRead (

/**
Write RTC register.
+ Data Write-Slave Receiver Mode
+
+ <Slave Address> <Word Address (n)> <Data(n)> <Data(n+1)> <Data(n+X)>

@param RtcRegAddr Register offset of RTC to write.
@param Val Value to be written
--
2.17.1


Re: [PATCH v2 04/28] Silicon/Maxim: Fix bug in RtcWrite in Ds1307RtcLib

Leif Lindholm
 

On Fri, Mar 20, 2020 at 20:05:19 +0530, Pankaj Bansal wrote:
From: Pankaj Bansal <pankaj.bansal@...>

There was a bug in I2C DXE implementation, which caused the Ds1307 RTC
device to issue two operation for register write, while this is a single
operation task. refer page 12 (Slave Receiver Mode (Write Mode)) on

https://datasheets.maximintegrated.com/en/ds/DS1307.pdf

Modify ds1307 RtcWrite code accordingly.

Signed-off-by: Pankaj Bansal <pankaj.bansal@...>
So, I'm OK with this patch, but I'll mention that I prefer the design
in Silicon/NXP/Library/Pcf8563RealTimeClockLib which I think could
also be applied here. I think that might have avoided the confusion
that caused the bug.

Reviewed-by: Leif Lindholm <leif@...>

---
Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
index 88dc198ffec8..fd7a8696e405 100644
--- a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
+++ b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
@@ -5,7 +5,7 @@
EmbeddedPkg/Library/TemplateRealTimeClockLib/RealTimeClockLib.c

Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
- Copyright 2017 NXP
+ Copyright 2017, 2020 NXP

SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -84,16 +84,15 @@ RtcWrite (
{
RTC_I2C_REQUEST Req;
EFI_STATUS Status;
+ UINT8 Buffer[2];

- Req.OperationCount = 2;
+ Req.OperationCount = 1;
+ Buffer[0] = RtcRegAddr;
+ Buffer[1] = Val;

Req.SetAddressOp.Flags = 0;
- Req.SetAddressOp.LengthInBytes = sizeof (RtcRegAddr);
- Req.SetAddressOp.Buffer = &RtcRegAddr;
-
- Req.GetSetDateTimeOp.Flags = 0;
- Req.GetSetDateTimeOp.LengthInBytes = sizeof (Val);
- Req.GetSetDateTimeOp.Buffer = &Val;
+ Req.SetAddressOp.LengthInBytes = sizeof (Buffer);
+ Req.SetAddressOp.Buffer = Buffer;

Status = mI2cMaster->StartRequest (mI2cMaster, FixedPcdGet8 (PcdI2cSlaveAddress),
(VOID *)&Req,
--
2.17.1


Re: [PATCH] .azurepipelines: Enable CI for OvmfPkg and EmulatorPkg

Laszlo Ersek
 

On 03/30/20 23:29, Kinney, Michael D wrote:
Hi Laszlo,

I think using the QEMU feature to map a path from the host as
a FAT formatted driver has value to provision QEMU with a set
of target tests to run. I agree there is no persistent storage
of writes to this type of drive. I believe writes are supported
if file state is needed during the single boot of QEMU.

In order to get test results out of QEMU back to the host,
I was considering the use of consoles mapped as named pipes
and send all test results out the console device and the
host can parse the logs.
Yes, this should be viable.

I have not had good experiences trying to build virtual
disks, provision them, map them into QEMU, and remount
from host to collect test results. Slow operations and
cumbersome to mount/unmount from the host (at least from
Windows environments).
I must agree. It is slow and somewhat cumbersome with "guestfish" too,
on Linux.

Guestfish, on the plus side, does not mount the disk with the host
kernel, and so it doesn't need root rights. Guestfish launches a
separate QEMU instance on top of the virtual disk, booting a dedicated
kernel and agent (the "libguestfs appliance") in the guest. The guest
appliance communicates with the "guestfish" host side command shell over
virtio-serial.

So the host kernel never trusts (or even sees) the filesystem structures
of the guest disk image; files are exchanged between the "libguestfs
appliance" (= guest kernel + agent) and the host-side command shell
using a dedicated wire protocol.

But, I agree it is somewhat cumbersome and slow, and I have no idea
whether it works on Windows hosts.

Thanks,
Laszlo



Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On
Behalf Of Laszlo Ersek
Sent: Monday, March 30, 2020 2:11 PM
To: devel@edk2.groups.io; ard.biesheuvel@...; Sean
Brogan <sean.brogan@...>
Subject: Re: [edk2-devel] [PATCH] .azurepipelines: Enable
CI for OvmfPkg and EmulatorPkg

On 03/30/20 08:07, Ard Biesheuvel wrote:
On Mon, 30 Mar 2020 at 01:16, Sean via Groups.Io
<sean.brogan@...> wrote:

Ard/Laszlo or anyone familiar with QEMU.

I read up on the ovmf readme and the qemu wiki but
still have a few issues i am hoping for quick/easy
answers.

1. How do i programmatically exit the emulator.
Seems like uefi shell > reset just reboots. Other ideas?

'reset' is supposed to do that. Use 'reset -s' to kill
the VM

2. Is there an easy way to map a local file system so
that i can setup a startup.nsh?
As Andrew points out, use '-hda fat:<path>' and it will
be exposed to
the VM as a FAT formatted block device.
Note that it will not offer (functional, or any) write
support.

I prefer "mtools" or "guestfish" for formatting and
populating a virtual
disk image.

None of those are easy ways, admittedly -- I'm not
offering some
examples right now exactly because I have to sit down and
think about
them every time I need them. :) If there's interest in
such commands, I
could hack up something, but then please give me some
specs, and I'll
have to work on these things in the morning (while my
brain is still not
mush).

Thanks
Laszlo



Re: [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation.

Maciej Rabeda
 

Hi Laszlo,

Thanks for trying this out!

The condition in the ASSERTs is reversed, consequently for the ASSERTs added in this function.
I have added them to fire up when Ip6IsNDOptionValid() fails to properly react to invalid packet (return with an error and do not proceed with processing options).
In this case, fire assert when current position within the packet (Offset) moved past the size of the option (Option.Length) * 8 (it's in 8-byte units) is outside the packet (further than first byte outside the packet).
Offset one byte past the packet == last option ended at the end of the packet (packet size is Head->PayloadLength).

Can you try swapping the >= to <= and rerun your test?
This should apply to lines 2114, 2167 and 2337.

I will submit a patch for that.

Thanks,
Maciej

On 31-Mar-20 02:35, Laszlo Ersek wrote:
Hi Maciej,

On 03/30/20 14:23, Maciej Rabeda wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2174

Problem has been identified with Ip6ProcessRouterAdvertise() when
Router Advertise packet contains options with malicious/invalid
'Length' field. This can lead to platform entering infinite loop
when processing options from that packet.

Cc: Jiaxin Wu <jiaxin.wu@...>
Cc: Siyuan Fu <siyuan.fu@...>
Signed-off-by: Maciej Rabeda <maciej.rabeda@...>
Reviewed-by: Siyuan Fu <siyuan.fu@...>
---
NetworkPkg/Ip6Dxe/Ip6Nd.c | 44 +++++++++------
NetworkPkg/Ip6Dxe/Ip6Nd.h | 13 +++++
NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++-----
3 files changed, 83 insertions(+), 31 deletions(-)

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index 4288ef02dd46..fd7f60b2f92c 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -1927,7 +1927,7 @@ Ip6ProcessRouterAdvertise (
UINT32 ReachableTime;
UINT32 RetransTimer;
UINT16 RouterLifetime;
- UINT16 Offset;
+ UINT32 Offset;
UINT8 Type;
UINT8 Length;
IP6_ETHER_ADDR_OPTION LinkLayerOption;
@@ -2094,10 +2094,11 @@ Ip6ProcessRouterAdvertise (
//
// The only defined options that may appear are the Source
// Link-Layer Address, Prefix information and MTU options.
- // All included options have a length that is greater than zero.
+ // All included options have a length that is greater than zero and
+ // fit within the input packet.
//
Offset = 16;
- while (Offset < Head->PayloadLength) {
+ while (Offset < (UINT32) Head->PayloadLength) {
NetbufCopy (Packet, Offset, sizeof (UINT8), &Type);
switch (Type) {
case Ip6OptionEtherSource:
@@ -2105,9 +2106,12 @@ Ip6ProcessRouterAdvertise (
// Update the neighbor cache
//
NetbufCopy (Packet, Offset, sizeof (IP6_ETHER_ADDR_OPTION), (UINT8 *) &LinkLayerOption);
- if (LinkLayerOption.Length <= 0) {
- goto Exit;
- }
+
+ //
+ // Option size validity ensured by Ip6IsNDOptionValid().
+ //
+ ASSERT (LinkLayerOption.Length != 0);
+ ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength);
ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));
CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);
@@ -2151,13 +2155,17 @@ Ip6ProcessRouterAdvertise (
}
}
- Offset = (UINT16) (Offset + (UINT16) LinkLayerOption.Length * 8);
+ Offset += (UINT32) LinkLayerOption.Length * 8;
break;
case Ip6OptionPrefixInfo:
NetbufCopy (Packet, Offset, sizeof (IP6_PREFIX_INFO_OPTION), (UINT8 *) &PrefixOption);
- if (PrefixOption.Length != 4) {
- goto Exit;
- }
+
+ //
+ // Option size validity ensured by Ip6IsNDOptionValid().
+ //
+ ASSERT (PrefixOption.Length == 4);
+ ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength);
I've hit this ASSERT():

ASSERT NetworkPkg/Ip6Dxe/Ip6Nd.c(2167): Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength

Here's the packet that triggers it:

02:30:25.350096 52:54:00:18:64:e7 > 33:33:00:00:00:01, ethertype IPv6 (0x86dd), length 142: (class 0xc0, hlim 255, next-header ICMPv6 (58) payload length: 88) fe80::5054:ff:fe18:64e7 > ff02::1: [icmp6 sum ok] ICMP6, router advertisement, length 88
hop limit 64, Flags [none], pref medium, router lifetime 1800s, reachable time 0ms, retrans time 0ms
prefix info option (3), length 32 (4): fd33:eb1b:9b36::/64, Flags [onlink, auto], valid time 3600s, pref. time 3600s
mtu option (5), length 8 (1): 1500
source link-address option (1), length 8 (1): 52:54:00:18:64:e7
rdnss option (25), length 24 (3): lifetime 3600s, addr: fe80::5054:ff:fe18:64e7
0x0000: 6c00 0000 0058 3aff fe80 0000 0000 0000 l....X:.........
0x0010: 5054 00ff fe18 64e7 ff02 0000 0000 0000 PT....d.........
0x0020: 0000 0000 0000 0001 8600 0008 4000 0708 ............@...
0x0030: 0000 0000 0000 0000 0304 40c0 0000 0e10 ..........@.....
0x0040: 0000 0e10 0000 0000 fd33 eb1b 9b36 0000 .........3...6..
0x0050: 0000 0000 0000 0000 0501 0000 0000 05dc ................
0x0060: 0101 5254 0018 64e7 1903 0000 0000 0e10 ..RT..d.........
0x0070: fe80 0000 0000 0000 5054 00ff fe18 64e7 ........PT....d.

Thanks,
Laszlo

+
PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime);
PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime);
@@ -2321,9 +2329,12 @@ Ip6ProcessRouterAdvertise (
break;
case Ip6OptionMtu:
NetbufCopy (Packet, Offset, sizeof (IP6_MTU_OPTION), (UINT8 *) &MTUOption);
- if (MTUOption.Length != 1) {
- goto Exit;
- }
+
+ //
+ // Option size validity ensured by Ip6IsNDOptionValid().
+ //
+ ASSERT (MTUOption.Length == 1);
+ ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength);
//
// Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order
@@ -2338,11 +2349,10 @@ Ip6ProcessRouterAdvertise (
// Silently ignore unrecognized options
//
NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length);
- if (Length <= 0) {
- goto Exit;
- }
- Offset = (UINT16) (Offset + (UINT16) Length * 8);
+ ASSERT (Length != 0);
+
+ Offset += (UINT32) Length * 8;
break;
}
}
diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.h b/NetworkPkg/Ip6Dxe/Ip6Nd.h
index 560dfa343782..5f1bd6fb922a 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.h
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.h
@@ -56,12 +56,21 @@ VOID
VOID *Context
);
+typedef struct _IP6_OPTION_HEADER {
+ UINT8 Type;
+ UINT8 Length;
+} IP6_OPTION_HEADER;
+
+STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) == 2, "IP6_OPTION_HEADER is expected to be exactly 2 bytes long.");
+
typedef struct _IP6_ETHE_ADDR_OPTION {
UINT8 Type;
UINT8 Length;
UINT8 EtherAddr[6];
} IP6_ETHER_ADDR_OPTION;
+STATIC_ASSERT (sizeof (IP6_ETHER_ADDR_OPTION) == 8, "IP6_ETHER_ADDR_OPTION is expected to be exactly 8 bytes long.");
+
typedef struct _IP6_MTU_OPTION {
UINT8 Type;
UINT8 Length;
@@ -69,6 +78,8 @@ typedef struct _IP6_MTU_OPTION {
UINT32 Mtu;
} IP6_MTU_OPTION;
+STATIC_ASSERT (sizeof (IP6_MTU_OPTION) == 8, "IP6_MTU_OPTION is expected to be exactly 8 bytes long.");
+
typedef struct _IP6_PREFIX_INFO_OPTION {
UINT8 Type;
UINT8 Length;
@@ -80,6 +91,8 @@ typedef struct _IP6_PREFIX_INFO_OPTION {
EFI_IPv6_ADDRESS Prefix;
} IP6_PREFIX_INFO_OPTION;
+STATIC_ASSERT (sizeof (IP6_PREFIX_INFO_OPTION) == 32, "IP6_PREFIX_INFO_OPTION is expected to be exactly 32 bytes long.");
+
typedef
VOID
(*IP6_DAD_CALLBACK) (
diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c
index 4d92a852dc86..6b4b029d1479 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Option.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Option.c
@@ -129,45 +129,74 @@ Ip6IsNDOptionValid (
IN UINT16 OptionLen
)
{
- UINT16 Offset;
- UINT8 OptionType;
+ UINT32 Offset;
UINT16 Length;
+ IP6_OPTION_HEADER *OptionHeader;
+
+ if (Option == NULL) {
+ ASSERT (Option != NULL);
+ return FALSE;
+ }
Offset = 0;
- while (Offset < OptionLen) {
- OptionType = *(Option + Offset);
- Length = (UINT16) (*(Option + Offset + 1) * 8);
+ //
+ // RFC 4861 states that Neighbor Discovery packet can contain zero or more
+ // options. Start processing the options if at least Type + Length fields
+ // fit within the input buffer.
+ //
+ while (Offset + sizeof (IP6_OPTION_HEADER) - 1 < OptionLen) {
+ OptionHeader = (IP6_OPTION_HEADER*) (Option + Offset);
+ Length = (UINT16) OptionHeader->Length * 8;
- switch (OptionType) {
+ switch (OptionHeader->Type) {
case Ip6OptionPrefixInfo:
if (Length != 32) {
return FALSE;
}
-
break;
case Ip6OptionMtu:
if (Length != 8) {
return FALSE;
}
-
break;
default:
- //
- // Check the length of Ip6OptionEtherSource, Ip6OptionEtherTarget, and
- // Ip6OptionRedirected here. For unrecognized options, silently ignore
- // and continue processing the message.
- //
+ // RFC 4861 states that Length field cannot be 0.
if (Length == 0) {
return FALSE;
}
+ break;
+ }
+
+ //
+ // Check whether recognized options are within the input buffer's scope.
+ //
+ switch (OptionHeader->Type) {
+ case Ip6OptionEtherSource:
+ case Ip6OptionEtherTarget:
+ case Ip6OptionPrefixInfo:
+ case Ip6OptionRedirected:
+ case Ip6OptionMtu:
+ if (Offset + Length > (UINT32) OptionLen) {
+ return FALSE;
+ }
+ break;
+ default:
+ //
+ // Unrecognized options can be either valid (but unused) or invalid
+ // (garbage in between or right after valid options). Silently ignore.
+ //
break;
}
- Offset = (UINT16) (Offset + Length);
+ //
+ // Advance to the next option.
+ // Length already considers option header's Type + Length.
+ //
+ Offset += Length;
}
return TRUE;


Re: [PATCH v4 1/1] BaseTools: Enable file dependencies in .inf files

PierreGondois
 

This patch is following the v3 named "BaseTools: Build ASL files before C files ", available at https://edk2.groups.io/g/devel/message/53735
Sorry for messing with the names.

Regards,
Pierre

-----Original Message-----
From: PierreGondois <pierre.gondois@...>
Sent: Monday, March 30, 2020 5:09 PM
To: devel@edk2.groups.io
Cc: Pierre Gondois <Pierre.Gondois@...>; bob.c.feng@...; liming.gao@...; Sami Mujawar <Sami.Mujawar@...>; nd <nd@...>
Subject: [PATCH v4 1/1] BaseTools: Enable file dependencies in .inf files

From: Pierre Gondois <pierre.gondois@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2425

The dependencies for C files are satisfied by the build system.
However, there are use cases where source files with different languages are inter-dependent. The EDKII build framework currently doesn't have options to specify such dependencies.
E.g. It may be necessary to specify the build order between
ASL files and C files. The use case being that the AML
blob generated by compiling the ASL files is loaded and
parsed by some C code.

This patch allows to describe dependencies between files listed in the '[Sources]' section of '.inf' files. The list of source files to build prior starts with a colon (':'), e.g.:
[Sources]
FileName1.X
FileName2.Y : FileName1.X
FileName3.Z : FileName1.X FileName2.Y

In the example above:
* FileName1.X will be built prior to FileName2.Y.
* FileName1.X and FileName2.Y will be built prior to
FileName3.Z.

This does not affect the file specific build options, which are pipe separated, e.g.:
FileName2.Y | GCC : FileName1.X

The list of colon separated files must be part of the module, i.e. they must be present in the [Sources] section.

Their file extension must be described in the BaseTools/Conf/build_rule.template file, and generate an output file, i.e. have a non-empty '<OutputFile>' section.

Declaring a dependency in the [Sources] sections leads to the creation of makefile rules between these files in the build. The makefile rule is between the generated files.
E.g.:
FileName1.X generates FileName1.objx
FileName2.Y generates FileName2.objy
Then
FileName2.Y : FileName1.X
will create the following makefile rule:
FileName2.objy : FileName1.objx

INF Specification updates:
s3.2.1, "Common Definitions":
<SrcDependencySeperator> ::= ":"
<DepS> ::= <TS> <SrcDependencySeperator> <TS>

s2.5, "[Sources] Section":
To describe a build order dependency between source
files, specify the source files to build prior by
listing them, colon separated.
<opt3> ::= <FS> [<FeatureFlagExpress>] [<opt4>]
<opt4> ::= <DepS> [FileNameDependency]*

Signed-off-by: Pierre Gondois <pierre.gondois@...>
---

The changes can be seen at https://github.com/PierreARM/edk2/commits/676_build_asl_first_v4

Notes:
v4:
- Correct typo in commit message, requested from [Bob]
- Create a Bugzilla to update the Inf specification,
and submit a patch to update the Inf specification,
available at:
https://bugzilla.tianocore.org/show_bug.cgi?id=2646

BaseTools/Source/Python/AutoGen/BuildEngine.py | 5 ++++
BaseTools/Source/Python/AutoGen/GenMake.py | 5 ++++
BaseTools/Source/Python/AutoGen/ModuleAutoGen.py | 16 ++++++++++
BaseTools/Source/Python/Common/DataType.py | 3 +-
BaseTools/Source/Python/Common/Misc.py | 2 ++
BaseTools/Source/Python/Workspace/InfBuildData.py | 31 ++++++++++++++++++--
BaseTools/Source/Python/Workspace/MetaFileParser.py | 17 +++++++++--
7 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/BuildEngine.py b/BaseTools/Source/Python/AutoGen/BuildEngine.py
index d602414ca41f37155c9c6d00eec54ea3918840c3..59cc0f8a765835ee459880f66453063af5f5779d 100644
--- a/BaseTools/Source/Python/AutoGen/BuildEngine.py
+++ b/BaseTools/Source/Python/AutoGen/BuildEngine.py
@@ -2,6 +2,7 @@
# The engine for building files
#
# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent #

@@ -53,6 +54,7 @@ class TargetDescBlock(object):
self.Outputs = Outputs
self.Commands = Commands
self.Dependencies = Dependencies
+ self.SourceFileDependencies = None
if self.Outputs:
self.Target = self.Outputs[0]
else:
@@ -277,6 +279,9 @@ class FileBuildRule:
TargetDesc.GenListFile = self.GenListFile
TargetDesc.GenIncListFile = self.GenIncListFile
self.BuildTargets[DstFile[0]] = TargetDesc
+
+ TargetDesc.SourceFileDependencies =
+ SourceFile.SourceFileDependencies
+
return TargetDesc

def _BuildCommand(self, Macros):
diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py b/BaseTools/Source/Python/AutoGen/GenMake.py
index bbb3c29446f53fa7f2cb61a216a5b119f72c3fbc..e257fd2c2eefee2782bc52606e537a2147f6857b 100755
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -1013,6 +1013,11 @@ cleanlib:

Deps = []
CCodeDeps = []
+
+ if T.SourceFileDependencies:
+ for File in T.SourceFileDependencies:
+ Deps.append(self.PlaceMacro(str(File),
+ self.Macros))
+
# Add force-dependencies
for Dep in T.Dependencies:
Deps.append(self.PlaceMacro(str(Dep), self.Macros)) diff --git a/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py b/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
index aad591de65f086043d55aeea5661f59c53792e7c..f5e25d6f0231a393c838eb66922ee5e769a4bdde 100755
--- a/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/ModuleAutoGen.py
@@ -2,6 +2,7 @@
# Create makefile for MS nmake and GNU make # # Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent # from __future__ import absolute_import @@ -876,6 +877,21 @@ class ModuleAutoGen(AutoGen):
if Source != File:
CreateDirectory(Source.Dir)

+ # The Makefile rule created from with source file dependency must
+ # have an object file as prerequisite.
+ # This gets the ouput format of sources.
+ OutPutSourceFileDependencies = []
+ if Source.SourceFileDependencies:
+ for File in Source.SourceFileDependencies:
+ if File.Type in self.BuildRules:
+ DepRuleObject = self.BuildRules[File.Type]
+ DepTarget = DepRuleObject.Apply(File, self.BuildRuleOrder)
+ # Files not producing outputs like '.h files don't create a target.
+ if DepTarget:
+
+ OutPutSourceFileDependencies.extend(DepTarget.Outputs)
+
+ Source.SourceFileDependencies =
+ OutPutSourceFileDependencies
+
if File.IsBinary and File == Source and File in BinaryFileList:
# Skip all files that are not binary libraries
if not self.IsLibrary:
diff --git a/BaseTools/Source/Python/Common/DataType.py b/BaseTools/Source/Python/Common/DataType.py
index 8d80b410892946c8b862e060b0bf5f630b409825..8f21e1060446982f866c653aaea70cd662e307a0 100644
--- a/BaseTools/Source/Python/Common/DataType.py
+++ b/BaseTools/Source/Python/Common/DataType.py
@@ -2,7 +2,7 @@
# This file is used to define common static strings used by INF/DEC/DSC files # # Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR> -# Portions copyright (c) 2011 - 2013, ARM Ltd. All rights reserved.<BR>
+# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent

##
@@ -19,6 +19,7 @@ TAB_VALUE_SPLIT = '|'
TAB_COMMA_SPLIT = ','
TAB_SPACE_SPLIT = ' '
TAB_SEMI_COLON_SPLIT = ';'
+TAB_COLON_SPLIT = ':'
TAB_SECTION_START = '['
TAB_SECTION_END = ']'
TAB_OPTION_START = '<'
diff --git a/BaseTools/Source/Python/Common/Misc.py b/BaseTools/Source/Python/Common/Misc.py
index da5fb380f0354d6e885aa716d2c9b2fead249d63..cb5c73d473954254a1f4cfe794d97b1b8a198185 100755
--- a/BaseTools/Source/Python/Common/Misc.py
+++ b/BaseTools/Source/Python/Common/Misc.py
@@ -2,6 +2,7 @@
# Common routines used by all tools
#
# Copyright (c) 2007 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent #

@@ -1438,6 +1439,7 @@ class PathClass(object):
Arch='COMMON', ToolChainFamily='', Target='', TagName='', ToolCode=''):
self.Arch = Arch
self.File = str(File)
+ self.SourceFileDependencies = []
if os.path.isabs(self.File):
self.Root = ''
self.AlterRoot = ''
diff --git a/BaseTools/Source/Python/Workspace/InfBuildData.py b/BaseTools/Source/Python/Workspace/InfBuildData.py
index 7675b0ea00ebd6a5fc3e823c965e32066f66f650..f5333e01d907f3883f43edd3ba65c2477aaf9b78 100644
--- a/BaseTools/Source/Python/Workspace/InfBuildData.py
+++ b/BaseTools/Source/Python/Workspace/InfBuildData.py
@@ -3,6 +3,7 @@
#
# Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR> # (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
+# Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent #

@@ -536,12 +537,19 @@ class InfBuildData(ModuleBuildClassObject):
return []

RetVal = []
+ PendingSourceFileDependencies = dict()
RecordList = self._RawData[MODEL_EFI_SOURCE_FILE, self._Arch, self._Platform]
Macros = self._Macros
for Record in RecordList:
LineNo = Record[-1]
- ToolChainFamily = Record[1]
- TagName = Record[2]
+
+ BuildOptions = ['', '']
+ SplittedValueList = GetSplitValueList(Record[1], TAB_VALUE_SPLIT, 1)
+ BuildOptions[0:len(SplittedValueList)] = SplittedValueList
+ SourceFileDependencies =
+ list(set(GetSplitValueList(Record[2], TAB_SPACE_SPLIT)))
+
+ ToolChainFamily = BuildOptions[0]
+ TagName = BuildOptions[1]
ToolCode = Record[3]

File = PathClass(NormPath(Record[0], Macros), self._ModuleDir, '', @@ -552,9 +560,28 @@ class InfBuildData(ModuleBuildClassObject):
EdkLogger.error('build', ErrorCode, ExtraData=ErrorInfo, File=self.MetaFile, Line=LineNo)

RetVal.append(File)
+
+ # If there are source file dependencies, resolve them after all
+ # the PathClass instances of the source files have been created.
+ if SourceFileDependencies[0] != '':
+ PendingSourceFileDependencies[File] =
+ SourceFileDependencies
+
# add any previously found dependency files to the source list
if self._DependencyFileList:
RetVal.extend(self._DependencyFileList)
+
+ # Resolve the dependencies between the PathClass instances.
+ for SourceFile, SourceFileDepList in PendingSourceFileDependencies.items():
+ for SourceFileDepFile in SourceFileDepList:
+ Found = False
+ for Val in RetVal:
+ if SourceFileDepFile == Val.File:
+ SourceFile.SourceFileDependencies.append(Val)
+ Found = True
+ break
+ if not Found:
+ EdkLogger.error("build", FILE_NOT_FOUND,
+ ExtraData=SourceFileDepFile)
+
return RetVal

## Retrieve library classes employed by this module diff --git a/BaseTools/Source/Python/Workspace/MetaFileParser.py b/BaseTools/Source/Python/Workspace/MetaFileParser.py
index a3b6edbd15ee5bf79cef0ac1fc5e53db30356c91..9212a7015702d212d6a7e6c57b2c1e5913e546a9 100644
--- a/BaseTools/Source/Python/Workspace/MetaFileParser.py
+++ b/BaseTools/Source/Python/Workspace/MetaFileParser.py
@@ -3,6 +3,7 @@
#
# Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR> # (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
+# Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent #

@@ -735,8 +736,20 @@ class InfParser(MetaFileParser):
#
@ParseMacro
def _SourceFileParser(self):
- TokenList = GetSplitValueList(self._CurrentLine, TAB_VALUE_SPLIT)
- self._ValueList[0:len(TokenList)] = TokenList
+ SourceFileDependencySplit = GetSplitValueList(self._CurrentLine, TAB_COLON_SPLIT, 1)
+ BuildOptionSplit =
+ GetSplitValueList(SourceFileDependencySplit[0], TAB_VALUE_SPLIT, 1)
+
+ # Get the filename.
+ self._ValueList[0] = BuildOptionSplit[0]
+
+ # Get the build options.
+ if len(BuildOptionSplit) > 1:
+ self._ValueList[1] = BuildOptionSplit[1]
+
+ # Get the dependencies.
+ if len(SourceFileDependencySplit) > 1:
+ self._ValueList[2] = SourceFileDependencySplit[1]
+
Macros = self._Macros
# For Acpi tables, remove macro like ' TABLE_NAME=Sata1'
if 'COMPONENT_TYPE' in Macros:
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'