Re: [edk2-devel] [edk2-platforms] [PATCH V2] KabylakeSiliconPkg: Logic Error in EISS bit ASSERT

2019-10-04 Thread Chaganty, Rangasai V 
Reviewed-by: Sai Chaganty 

-Original Message-
From: Desimone, Nathaniel L 
Sent: Friday, October 04, 2019 1:14 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel ; Kubacki, Michael A 
; Chaganty, Rangasai V 

Subject: [edk2-platforms] [PATCH V2] KabylakeSiliconPkg: Logic Error in EISS 
bit ASSERT

Current ASSERT logic checks that the EISS bit is still set after we clear it. 
This is incorrect, it should be checking that that the EISS bit is clear after 
we clear it.

Cc: Chasel Chiu 
Cc: Michael Kubacki 
Cc: Sai Chaganty 
Signed-off-by: Nate DeSimone 
---
 .../Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c 
b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
index aadc367a9f..c34c378de2 100644
--- a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
+++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
@@ -184,7 +184,7 @@ DisableBiosWriteProtect (
 B_PCH_SPI_BC_WPD ); -  ASSERT ((PciSegmentRead8 (SpiBaseAddress + 
R_PCH_SPI_BC) & B_PCH_SPI_BC_EISS) != 0);+  ASSERT ((PciSegmentRead8 
(SpiBaseAddress + R_PCH_SPI_BC) & B_PCH_SPI_BC_EISS) == 0);return 
EFI_SUCCESS; }-- 
2.23.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48494): https://edk2.groups.io/g/devel/message/48494
Mute This Topic: https://groups.io/mt/34398391/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Ice Lake FSP Released

2019-10-04 Thread Nate DeSimone 
Hi All,

Intel is pleased to announce that Ice Lake FSP is now available on 
https://github.com/IntelFsp/FSP.

With Best Regards,

Nate

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48493): https://edk2.groups.io/g/devel/message/48493
Mute This Topic: https://groups.io/mt/34399481/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms] [PATCH V2] KabylakeOpenBoardPkg: Resize FSP-T to accommodate debug FSP builds.

2019-10-04 Thread Kubacki, Michael A 
Reviewed-by: Michael Kubacki 

> -Original Message-
> From: Desimone, Nathaniel L 
> Sent: Friday, October 4, 2019 1:45 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Kubacki, Michael A
> ; Jeremy Soller 
> Subject: [edk2-platforms] [PATCH V2] KabylakeOpenBoardPkg: Resize FSP-T
> to accommodate debug FSP builds.
> 
> Resize the flash map for the GalagoPro3 platform to provide enough space to
> accommodate a debug build of FSP-T.
> 
> Cc: Chasel Chiu 
> Cc: Michael Kubacki 
> Cc: Jeremy Soller 
> Signed-off-by: Nate DeSimone 
> ---
>  .../GalagoPro3/Include/Fdf/FlashMapInclude.fdf  | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git
> a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMa
> pInclude.fdf
> b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMa
> pInclude.fdf
> index b33b1e09a9..e024dd127c 100644
> ---
> a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMa
> pInclude.fdf
> +++
> b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMa
> +++ pInclude.fdf
> @@ -41,8 +41,8 @@ SET gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize
> = 0x000A
>  SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspSOffset  =
> 0x0044  # Flash addr (0xFFE6) SET
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspSSize= 0x0006
> # SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspMOffset  =
> 0x004A  # Flash addr (0xFFEC)-SET
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspMSize= 0x000BC000
> #-SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTOffset  =
> 0x0055C000  # Flash addr (0xFFF7C000)-SET
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTSize= 0x4000
> #+SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspMSize=
> 0x000BA000  #+SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTOffset
> = 0x0055A000  # Flash addr (0xFFF7A000)+SET
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTSize= 0x6000
> # SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvPreMemoryOffset =
> 0x0056  # Flash addr (0xFFF8) SET
> gMinPlatformPkgTokenSpaceGuid.PcdFlashFvPreMemorySize   =
> 0x0008  #--
> 2.23.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48492): https://edk2.groups.io/g/devel/message/48492
Mute This Topic: https://groups.io/mt/34398766/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms] [PATCH V2] KabylakeSiliconPkg: Logic Error in EISS bit ASSERT

2019-10-04 Thread Kubacki, Michael A 
Reviewed-by: Michael Kubacki 

> -Original Message-
> From: Desimone, Nathaniel L 
> Sent: Friday, October 4, 2019 1:14 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Kubacki, Michael A
> ; Chaganty, Rangasai V
> 
> Subject: [edk2-platforms] [PATCH V2] KabylakeSiliconPkg: Logic Error in EISS
> bit ASSERT
> 
> Current ASSERT logic checks that the EISS bit is still set after we clear it. 
> This is
> incorrect, it should be checking that that the EISS bit is clear after we 
> clear it.
> 
> Cc: Chasel Chiu 
> Cc: Michael Kubacki 
> Cc: Sai Chaganty 
> Signed-off-by: Nate DeSimone 
> ---
>  .../Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c  | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> index aadc367a9f..c34c378de2 100644
> --- a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> +++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
> @@ -184,7 +184,7 @@ DisableBiosWriteProtect (
>  B_PCH_SPI_BC_WPD ); -  ASSERT ((PciSegmentRead8 (SpiBaseAddress +
> R_PCH_SPI_BC) & B_PCH_SPI_BC_EISS) != 0);+  ASSERT ((PciSegmentRead8
> (SpiBaseAddress + R_PCH_SPI_BC) & B_PCH_SPI_BC_EISS) == 0);return
> EFI_SUCCESS; }--
> 2.23.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48491): https://edk2.groups.io/g/devel/message/48491
Mute This Topic: https://groups.io/mt/34398391/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms] [PATCH V2] KabylakeOpenBoardPkg: Resize FSP-T to accommodate debug FSP builds.

2019-10-04 Thread Nate DeSimone 
Resize the flash map for the GalagoPro3 platform to provide
enough space to accommodate a debug build of FSP-T.

Cc: Chasel Chiu 
Cc: Michael Kubacki 
Cc: Jeremy Soller 
Signed-off-by: Nate DeSimone 
---
 .../GalagoPro3/Include/Fdf/FlashMapInclude.fdf  | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git 
a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMapInclude.fdf
 
b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMapInclude.fdf
index b33b1e09a9..e024dd127c 100644
--- 
a/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMapInclude.fdf
+++ 
b/Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Include/Fdf/FlashMapInclude.fdf
@@ -41,8 +41,8 @@ SET gSiPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize  
  = 0x000A
 SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspSOffset  = 
0x0044  # Flash addr (0xFFE6)
 SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspSSize= 
0x0006  #
 SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspMOffset  = 
0x004A  # Flash addr (0xFFEC)
-SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspMSize= 
0x000BC000  #
-SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTOffset  = 
0x0055C000  # Flash addr (0xFFF7C000)
-SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTSize= 
0x4000  #
+SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspMSize= 
0x000BA000  #
+SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTOffset  = 
0x0055A000  # Flash addr (0xFFF7A000)
+SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvFspTSize= 
0x6000  #
 SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvPreMemoryOffset = 
0x0056  # Flash addr (0xFFF8)
 SET gMinPlatformPkgTokenSpaceGuid.PcdFlashFvPreMemorySize   = 
0x0008  #
-- 
2.23.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48490): https://edk2.groups.io/g/devel/message/48490
Mute This Topic: https://groups.io/mt/34398766/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms] [PATCH V2] KabylakeSiliconPkg: Logic Error in EISS bit ASSERT

2019-10-04 Thread Nate DeSimone 
Current ASSERT logic checks that the EISS bit is still
set after we clear it. This is incorrect, it should be
checking that that the EISS bit is clear after we clear it.

Cc: Chasel Chiu 
Cc: Michael Kubacki 
Cc: Sai Chaganty 
Signed-off-by: Nate DeSimone 
---
 .../Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c 
b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
index aadc367a9f..c34c378de2 100644
--- a/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
+++ b/Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c
@@ -184,7 +184,7 @@ DisableBiosWriteProtect (
 B_PCH_SPI_BC_WPD
 );
 
-  ASSERT ((PciSegmentRead8 (SpiBaseAddress + R_PCH_SPI_BC) & 
B_PCH_SPI_BC_EISS) != 0);
+  ASSERT ((PciSegmentRead8 (SpiBaseAddress + R_PCH_SPI_BC) & 
B_PCH_SPI_BC_EISS) == 0);
 
   return EFI_SUCCESS;
 }
-- 
2.23.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48489): https://edk2.groups.io/g/devel/message/48489
Mute This Topic: https://groups.io/mt/34398391/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 25/35] OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() call

2019-10-04 Thread Laszlo Ersek 
On 09/26/19 14:43, Laszlo Ersek wrote:
> On 09/23/19 17:59, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> On 9/17/19 9:49 PM, Laszlo Ersek wrote:
>>> According to the UEFI spec -- and to the edk2 header
>>> "MdePkg/Include/Protocol/EdidOverride.h" too --,
>>> EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID takes an (EFI_HANDLE*), and not an
>>> EFI_HANDLE, as second parameter ("ChildHandle").
>>>
>>> This is probably [*] a bug in the UEFI spec. Given that this CSM module
>>> (VideoDxe) had been used for a long time on physical platforms before it
>>> was moved to OvmfPkg, keep the current "ChildHandle" argument, just cast
>>> it explicitly.
>>>
>>> [*] The edk2 tree contains no other GetEdid() call, and also no GetEdid()
>>> implementation.
>>>
>>> The edk2-platforms tree contains two GetEdid() calls, at commit
>>> 022c212167e0, in files
>>> - "Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c",
>>> - "Drivers/OptionRomPkg/CirrusLogic5430Dxe/Edid.c".
>>>
>>> From these, the first passes an (EFI_HANDLE*) as "ChildHandle", the
>>> second passes an EFI_HANDLE. It's difficult to draw a conclusion. :/
>>>
>>> No functional changes.
>>>
>>> (I've also requested a non-normative (informative) clarification for the
>>> UEFI spec: , in the
>>> direction that matches this patch.)
>>
>> (EFI_HANDLE*) makes sense to me, but I'd rather wait for the spec
>> clarification before Acking this patch, I don't want we silent a bug
>> with this cast.
> 
> Right, there's been some movement in Mantis#2018.
> 
> It looks like the spec is wrong, but all [*] consumers and producers of
> GetEdid(), investigated thus far, have simply ignored the mistake in the
> spec, and done the right thing in practice.
> 
> So there seems to be a chance for the spec to be fixed. That would be
> followed by fixing the GetEdid() prototype in edk2. And then this patch
> would be dropped.
> 
> [*] the only exception found thus far is the call site in
> edk2-platform's "DisplayLinkPkg", mentioned above in the commit message.
> However, that is a very recent code addition (commit 9df63499ea01,
> 2019-09-09), and it might not reflect "historical" usage, but an attempt
> to write brand new code, conforming to the *letter* of the spec. So in
> case the spec gets fixed, the DisplayLinkPkg code could be fixed in
> tandem, perhaps.

See new thread started here:

985de369-7880-b6cc-46e7-5a2edca6582b@redhat.com">http://mid.mail-archive.com/985de369-7880-b6cc-46e7-5a2edca6582b@redhat.com
https://edk2.groups.io/g/devel/message/48487

Thanks!
Laszlo

>>> Cc: Ard Biesheuvel 
>>> Cc: David Woodhouse 
>>> Cc: Jordan Justen 
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>
>>> Notes:
>>> build-tested only
>>>
>>>  OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c | 6 +-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c 
>>> b/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c
>>> index 0640656dba14..995136adee27 100644
>>> --- a/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c
>>> +++ b/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c
>>> @@ -1402,9 +1402,13 @@ BiosVideoCheckForVbe (
>>>goto Done;
>>>  }
>>>
>>> +//
>>> +// Cast "ChildHandle" to (EFI_HANDLE*) in order to work around the 
>>> spec bug
>>> +// in UEFI v2.8, reported as Mantis#2018.
>>> +//
>>>  Status = EdidOverride->GetEdid (
>>>   EdidOverride,
>>> - BiosVideoPrivate->Handle,
>>> + (EFI_HANDLE *) BiosVideoPrivate->Handle,
>>>   ,
>>>   ,
>>>   (UINT8 **) 
>>>
>>
>>
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48488): https://edk2.groups.io/g/devel/message/48488
Mute This Topic: https://groups.io/mt/34180226/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms: PATCH 1/1] DisplayLinkPkg: DisplayLinkGop: Added GOP driver for USB docking stations based on DisplayLink chips

2019-10-04 Thread Laszlo Ersek 
Hello Andy,

I've got a question about your edk2-platforms commit 9df63499ea01 (i.e.,
this patch):

On 08/30/19 17:27, Leif Lindholm wrote:
> On Mon, Aug 19, 2019 at 02:32:00PM +0100, Andy Hayes wrote:

[...]

>> diff --git a/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c 
>> b/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c

[...]

>> +EFI_STATUS
>> +DlReadEdid (
>> +IN USB_DISPLAYLINK_DEV* UsbDisplayLinkDev
>> +)
>> +{

[...]

>> +Status = EdidOverride->GetEdid (
>> +  EdidOverride,
>> +  >Handle,
>> +  ,
>> +  ,
>> +  );

In UEFI v2.8, EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID's "ChildHandle"
parameter is specified as follows, in *natural language*:

  ChildHandle -- A child handle that represents a possible video output
 device.

Accordingly, the function *prototype* should specify the parameter as:

  IN EFI_HANDLE ChildHandle

However, the spec in fact mandates:

  IN EFI_HANDLE *ChildHandle

there.

This is most likely a typo in the prototype, and should be fixed (so
that the prototype match the natural language description). I've
reported the following spec ticket about it:

  https://mantis.uefi.org/mantis/view.php?id=2018

Here's the problem.

It looks like most GetEdid() calls in existence conform to the *spirit*
of the spec, and not to the *letter* thereof; in other words, they pass
an EFI_HANDLE for ChildHandle. Furthermore, the
EFI_EDID_OVERRIDE_PROTOCOL implementations underneath also conform to
the spirit, and not the letter of the spec, and they consume ChildHandle
as an EFI_HANDLE. (They do not de-reference ChildHandle for getting the
actual handle.)

In other words, fixing the typo in the spec would simply adopt existing
practice, and no code would have to be changed for spec conformance.

Except... the code quoted above.

The code above conforms to the *letter* of the spec, and passes an
(EFI_HANDLE*), as ChildHandle. If the typo is fixed in the spec, the
call site in DlReadEdid() would have to be modified.

Can you please confirm wheter:

- the EdidOverride->GetEdid() call is reached in practice (= it is not
dead code),

- the underlying EFI_EDID_OVERRIDE_PROTOCOL actually consumes
ChildHandle (for which it first has to de-reference it)?

My hope is that either the above call site is dead code en-bloc, or the
underlying protocol implementation ignores the ChildHandle parameter.
Then we can fix both the spec and the DlReadEdid() function.

Thanks!
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48487): https://edk2.groups.io/g/devel/message/48487
Mute This Topic: https://groups.io/mt/33080721/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 18/35] NetworkPkg/DxeNetLib: fix type typo in NetLibGetMacAddress()

2019-10-04 Thread Laszlo Ersek 
Ping

On 10/03/19 13:05, Laszlo Ersek wrote:
> Pinging NetworkPkg maintainers again. Please?
> 
> Thanks
> Laszlo
> 
> On 09/26/19 14:14, Laszlo Ersek wrote:
>> Jiaxin, Siyuan,
>>
>> can you please review this patch?
>>
>> Thanks
>> Laszlo
>>
>> On 09/17/19 21:49, Laszlo Ersek wrote:
>>> NetLibGetSnpHandle() returns an EFI_HANDLE, not an (EFI_HANDLE*).
>>> NetLibGetMacAddress() only uses the return value ("SnpHandle") for a
>>> NULL-check. Fix the type of "SnpHandle".
>>>
>>> This patch is a no-op.
>>>
>>> Cc: Jiaxin Wu 
>>> Cc: Siyuan Fu 
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>
>>> Notes:
>>> lightly tested: MAC strings are displayed in UiApp
>>>
>>>  NetworkPkg/Library/DxeNetLib/DxeNetLib.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/NetworkPkg/Library/DxeNetLib/DxeNetLib.c 
>>> b/NetworkPkg/Library/DxeNetLib/DxeNetLib.c
>>> index 8e2f720666ea..a39c20be3d34 100644
>>> --- a/NetworkPkg/Library/DxeNetLib/DxeNetLib.c
>>> +++ b/NetworkPkg/Library/DxeNetLib/DxeNetLib.c
>>> @@ -2182,7 +2182,7 @@ NetLibGetMacAddress (
>>>EFI_SIMPLE_NETWORK_MODE  SnpModeData;
>>>EFI_MANAGED_NETWORK_PROTOCOL *Mnp;
>>>EFI_SERVICE_BINDING_PROTOCOL *MnpSb;
>>> -  EFI_HANDLE   *SnpHandle;
>>> +  EFI_HANDLE   SnpHandle;
>>>EFI_HANDLE   MnpChildHandle;
>>>  
>>>ASSERT (MacAddress != NULL);
>>>
>>
>>
>>
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48486): https://edk2.groups.io/g/devel/message/48486
Mute This Topic: https://groups.io/mt/34180219/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 26/35] SecurityPkg: fix UninstallMultipleProtocolInterfaces() calls

2019-10-04 Thread Laszlo Ersek 
On 10/04/19 15:14, Zhang, Chao B wrote:
> Hi Laszlo:
>Sorry for late response. The fix is good to me.

Thanks!

Can you give Reviewed-by or Acked-by?

> I am also interested in how you find this issue, can you share it?

Sure, please see the explanation in patches #00 and #01 (I CC'd you on
them originally):

* 20190917194935.24322-1-lersek@redhat.com">http://mid.mail-archive.com/20190917194935.24322-1-lersek@redhat.com
  https://edk2.groups.io/g/devel/message/47387

* 20190917194935.24322-2-lersek@redhat.com">http://mid.mail-archive.com/20190917194935.24322-2-lersek@redhat.com
  https://edk2.groups.io/g/devel/message/47388

Thanks,
Laszlo

>
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: 2019年10月3日 19:07
> To: Zhang, Chao B ; Wang, Jian J 
> ; Yao, Jiewen 
> Cc: edk2-devel-groups-io 
> Subject: Re: [edk2-devel] [PATCH 26/35] SecurityPkg: fix 
> UninstallMultipleProtocolInterfaces() calls
> 
> Pinging SecurityPkg maintainers again, for reviewing this patch.
> 
> Thanks
> Laszlo
> 
> On 09/26/19 14:45, Laszlo Ersek wrote:
>> Chao, Jian, Jiewen,
>>
>> can you please review this patch?
>>
>> Thanks,
>> Laszlo
>>
>> On 09/17/19 21:49, Laszlo Ersek wrote:
>>> Unlike the InstallMultipleProtocolInterfaces() boot service, which 
>>> takes an (EFI_HANDLE*) as first parameter, the
>>> UninstallMultipleProtocolInterfaces() boot service takes an 
>>> EFI_HANDLE as first parameter.
>>>
>>> These are actual bugs. They must have remained hidden until now 
>>> because they are all in Unload() functions, which are probably 
>>> exercised infrequently. Fix the UninstallMultipleProtocolInterfaces() calls.
>>>
>>> Cc: Chao Zhang 
>>> Cc: Jian Wang 
>>> Cc: Jiewen Yao 
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>
>>> Notes:
>>> build-tested only
>>>
>>>  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c  
>>> | 2 +-
>>>  SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c 
>>> | 2 +-
>>>
>>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi
>>> gDriver.c | 2 +-
>>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c 
>>> b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>>> index 54155a338100..9052eced757d 100644
>>> --- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>>> +++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>>> @@ -443,7 +443,7 @@ Tcg2ConfigDriverUnload (
>>>ASSERT (PrivateData->Signature == 
>>> TCG2_CONFIG_PRIVATE_DATA_SIGNATURE);
>>>
>>>gBS->UninstallMultipleProtocolInterfaces (
>>> - ,
>>> + ImageHandle,
>>>   ,
>>>   PrivateData,
>>>   NULL
>>> diff --git a/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c 
>>> b/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>>> index 341879e4c4ba..fb06624fdb8f 100644
>>> --- a/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>>> +++ b/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>>> @@ -138,7 +138,7 @@ TcgConfigDriverUnload (
>>>ASSERT (PrivateData->Signature == 
>>> TCG_CONFIG_PRIVATE_DATA_SIGNATURE);
>>>
>>>gBS->UninstallMultipleProtocolInterfaces (
>>> - ,
>>> + ImageHandle,
>>>   ,
>>>   PrivateData,
>>>   NULL
>>> diff --git 
>>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
>>> figDriver.c 
>>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
>>> figDriver.c index 798ef9cfbc01..6c0294151e6c 100644
>>> --- 
>>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
>>> figDriver.c
>>> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoo
>>> +++ tConfigDriver.c
>>> @@ -115,7 +115,7 @@ SecureBootConfigDriverUnload (
>>>ASSERT (PrivateData->Signature == 
>>> SECUREBOOT_CONFIG_PRIVATE_DATA_SIGNATURE);
>>>
>>>gBS->UninstallMultipleProtocolInterfaces (
>>> - ,
>>> + ImageHandle,
>>>   ,
>>>   PrivateData,
>>>   NULL
>>>
>>
>>
>> 
>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48485): https://edk2.groups.io/g/devel/message/48485
Mute This Topic: https://groups.io/mt/34180228/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH V2 2/3] BoardModulePkg/FirmwareBootMediaInfoLib: Add library

2019-10-04 Thread Kubacki, Michael A 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2229

Introduces a new library class FirmwareBootMediaInfoLib that is
used to report the firmware boot media device. A default library
instance is provided that always returns the firmware boot media
is SPI flash. For platforms with other firmware boot media
options, a board-specific instance of this library should be
used instead to provide the correct firmware boot media device
information.

Cc: Eric Dong 
Cc: Liming Gao 
Signed-off-by: Michael Kubacki 
---
 Platform/Intel/BoardModulePkg/BoardModulePkg.dec   
   |  3 ++
 Platform/Intel/BoardModulePkg/BoardModulePkg.dsc   
   |  3 ++
 
Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
 | 35 
 Platform/Intel/BoardModulePkg/Include/Library/FirmwareBootMediaInfoLib.h   
   | 26 +++
 
Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.c
   | 24 ++
 5 files changed, 91 insertions(+)

diff --git a/Platform/Intel/BoardModulePkg/BoardModulePkg.dec 
b/Platform/Intel/BoardModulePkg/BoardModulePkg.dec
index 6f13945ca8..f96fb09aa1 100644
--- a/Platform/Intel/BoardModulePkg/BoardModulePkg.dec
+++ b/Platform/Intel/BoardModulePkg/BoardModulePkg.dec
@@ -33,6 +33,9 @@
   ##  @libraryclassProvide services to get BIOS ID information.
   BiosIdLib|Include/Library/BiosIdLib.h
 
+  ## @libraryclass Provides a service to determine the firmware boot media 
device.
+  FirmwareBootMediaInfoLib|Include/Library/FirmwareBootMediaInfoLib.h
+
 [Guids]
   ## Include Include/Guid/BiosId.h
   gBiosIdGuid = { 0xC3E36D09, 0x8294, 0x4b97, { 0xA8, 0x57, 0xD5, 0x28, 0x8F, 
0xE3, 0x3E, 0x28 } }
diff --git a/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc 
b/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc
index 734ead9be8..3d605cf876 100644
--- a/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc
+++ b/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc
@@ -33,6 +33,7 @@
   DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
 
 [LibraryClasses.common.PEIM]
+  
FirmwareBootMediaLib|IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
 
@@ -40,6 +41,7 @@
   
PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
 
 [LibraryClasses.common.DXE_DRIVER]
+  
FirmwareBootMediaLib|IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
 
@@ -77,3 +79,4 @@
   BoardModulePkg/Library/BiosIdLib/DxeBiosIdLib.inf
   BoardModulePkg/Library/BiosIdLib/PeiBiosIdLib.inf
 
+  
BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
diff --git 
a/Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
 
b/Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
new file mode 100644
index 00..637aeca2af
--- /dev/null
+++ 
b/Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
@@ -0,0 +1,35 @@
+## @file
+# Firmware Boot Media Info Library
+#
+# This library identifies firmware boot media device information used in the 
boot flow for system initialization
+# decisions dependent upon the firmware boot media.
+#
+# This library instance provides a default implementation of the 
FirmwareBootMediaInfoLib library class that always
+# returns SPI flash as the boot media device. For any system firmware in which 
this is not the case, an instance
+# of this library class should be provided that returns the correct boot media 
for the platform.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION  = 0x00010005
+  BASE_NAME= PeiFirmwareBootMediaInfoLib
+  FILE_GUID= 91CC29F5-AEAD-4108-9E91-C8DECDC1C654
+  MODULE_TYPE  = PEIM
+  VERSION_STRING   = 1.0
+  LIBRARY_CLASS= FirmwareBootMediaInfoLib
+
+[Sources]
+  PeiFirmwareBootMediaInfoLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  IntelSiliconPkg/IntelSiliconPkg.dec
+  BoardModulePkg/BoardModulePkg.dec
+
+[LibraryClasses]
+  BaseLib
+  FirmwareBootMediaLib
diff --git 
a/Platform/Intel/BoardModulePkg/Include/Library/FirmwareBootMediaInfoLib.h 
b/Platform/Intel/BoardModulePkg/Include/Library/FirmwareBootMediaInfoLib.h
new file mode 100644
index 00..b08f21ac74
--- /dev/null
+++ b/Platform/Intel/BoardModulePkg/Include/Library/FirmwareBootMediaInfoLib.h
@@ -0,0 +1,26 @@
+/** @file
+  This library

[edk2-devel] [edk2-platforms][PATCH V2 0/3] Add FW Boot Media Device Indicator

2019-10-04 Thread Kubacki, Michael A 
V2 Changes:
 1. Add the new libraries in BoardModulePkg and IntelSiliconPkg to the
package DSC files so they are included in the package build.

This patch series introduces a mechanism for determining the firmware
boot media device. This allows the firmware boot media to be discovered
through a standardized API.

Traditionally, most systems have only supported firmware storage on SPI
flash. Presently, several other storage technologies are being used to
store boot system firmware such as eMMC, UFS, and NVMe.

The API for all board, platform, and silicon code to consume the
firmware boot media device is provided by the FirmwareBootMediaLib
in IntelSiliconPkg.

A driver (FirmwareBootMediaInfoPei) is added to BoardModulePkg to
serve as a consistent location for reporting the firmware boot device
information. In order to abstract the potentially hardware-specific
details to determine the boot media (for platforms that support
multiple firmware boot media devices), the driver retrieves the
boot media information using a new library class introduced called
FirmwareBootMediaInfoLib. A default instance of this library class
is provided in BoardModulePkg that always returns SPI flash. This
is intended to serve as a default implementation of the library for
the most common scenario and to easily allow a board package to
substitute the logic required to determine the boot media in more
complex scenarios. Ultimately, FirmwareBootMediaInfoPei produces a
HOB containing the firmware boot media device information so it can
be used in the HOB consumer phase.

Cc: Sai Chaganty 
Cc: Eric Dong 
Cc: Liming Gao 
Cc: Ray Ni 
Signed-off-by: Michael Kubacki 

Michael Kubacki (3):
  IntelSiliconPkg/FirmwareBootMediaLib: Add library
  BoardModulePkg/FirmwareBootMediaInfoLib: Add library
  BoardModulePkg/FirmwareBootMediaInfoPei: Add module

 Platform/Intel/BoardModulePkg/BoardModulePkg.dec   
   |   3 +
 Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec  
   |   8 +-
 Platform/Intel/BoardModulePkg/BoardModulePkg.dsc   
   |   6 ++
 Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc  
   |   4 +-
 
Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.inf
  |  46 +
 
Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
 |  35 +++
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
|  43 
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
   |  38 +++
 Platform/Intel/BoardModulePkg/Include/Library/FirmwareBootMediaInfoLib.h   
   |  26 +
 Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h   
   | 106 +++
 Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.c 
   |  76 ++
 
Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.c
   |  24 +
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.c
  | 107 +++
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c
| 109 
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.c
 |  82 +++
 15 files changed, 711 insertions(+), 2 deletions(-)
 create mode 100644 
Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.inf
 create mode 100644 
Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
 create mode 100644 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
 create mode 100644 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
 create mode 100644 
Platform/Intel/BoardModulePkg/Include/Library/FirmwareBootMediaInfoLib.h
 create mode 100644 
Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h
 create mode 100644 
Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.c
 create mode 100644 
Platform/Intel/BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.c
 create mode 100644 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.c
 create mode 100644 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c
 create mode 100644 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.c

-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48481):

[edk2-devel] [edk2-platforms][PATCH V2 3/3] BoardModulePkg/FirmwareBootMediaInfoPei: Add module

2019-10-04 Thread Kubacki, Michael A 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2229

Adds a new module that is responsible for publishing the boot
media device information that will be used in the boot flow for
system initialization decisions dependent upon firmware boot
media.

The module depends on a library class to implement the board-
specific details to get the firmware boot media device. This
allows the module to remain consistent in its responsibility
across implementations and the details to be easily substituted.

Cc: Eric Dong 
Cc: Liming Gao 
Signed-off-by: Michael Kubacki 
---
 Platform/Intel/BoardModulePkg/BoardModulePkg.dsc   
  |  3 +
 
Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.inf
 | 46 
 Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.c 
  | 76 
 3 files changed, 125 insertions(+)

diff --git a/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc 
b/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc
index 3d605cf876..5ec68ceebf 100644
--- a/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc
+++ b/Platform/Intel/BoardModulePkg/BoardModulePkg.dsc
@@ -31,9 +31,11 @@
   DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
+  PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
 
 [LibraryClasses.common.PEIM]
   
FirmwareBootMediaLib|IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
+  
FirmwareBootMediaInfoLib|BoardModulePkg/Library/PeiFirmwareBootMediaInfoLib/PeiFirmwareBootMediaInfoLib.inf
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
 
@@ -73,6 +75,7 @@
 
###
 
 [Components]
+  BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.inf
   BoardModulePkg/Library/CmosAccessLib/CmosAccessLib.inf
   
BoardModulePkg/Library/PlatformCmosAccessLibNull/PlatformCmosAccessLibNull.inf
 
diff --git 
a/Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.inf
 
b/Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.inf
new file mode 100644
index 00..a81932f2a1
--- /dev/null
+++ 
b/Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.inf
@@ -0,0 +1,46 @@
+## @file
+# Firmware Boot Media Info Module
+#
+# This module publishes firmware boot media device information used in the 
boot flow for system initialization
+# decisions dependent upon the firmware boot media.
+#
+# This module depends upon a library instance to actually perform firmware 
boot media device detection since the
+# detection mechanism will vary across systems. In many cases, the media type 
may simply be set to a single firmware
+# boot media device with no run-time logic required. In any case, this module 
should dispatch as early as possible in
+# the system boot flow so the firmware boot media information is available for 
other modules. If any dependencies are
+# required to dynamically determine the firmware boot media device, those 
should be in the DEPEX section of the active
+# FirmwareBootMediaInfoLib such that this module will dispatch once those 
dependencies are satisfied.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION   = 0x00010017
+  BASE_NAME = FirmwareBootMediaInfoPei
+  FILE_GUID = A8F14FA9-FC88-45F4-A622-F06E6C56E632
+  VERSION_STRING= 1.0
+  MODULE_TYPE   = PEIM
+  ENTRY_POINT   = FirmwareBootMediaInfoPeiEntry
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  FirmwareBootMediaLib
+  FirmwareBootMediaInfoLib
+  HobLib
+  PeimEntryPoint
+  PeiServicesLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  IntelSiliconPkg/IntelSiliconPkg.dec
+  BoardModulePkg/BoardModulePkg.dec
+
+[Sources]
+  FirmwareBootMediaInfoPei.c
+
+[Depex]
+  TRUE
diff --git 
a/Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.c
 
b/Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.c
new file mode 100644
index 00..7a71071053
--- /dev/null
+++ 
b/Platform/Intel/BoardModulePkg/FirmwareBootMediaInfo/FirmwareBootMediaInfoPei.c
@@ -0,0 +1,76 @@
+/** @file
+  Firmware Boot Media Info Module
+
+  This module publishes firmware boot media device information used in the 
boot flow for system initialization
+  decisions dependent upon the firmware boot media.
+
+  This module depends upon a library instance to actually perform firmware 
boot media device detection since the
+  detection mechanism will vary across systems. In many cases, the media type 
may simply be set to a single firmware
+  boot media device with no run-time logic required. In any

[edk2-devel] [edk2-platforms][PATCH V2 1/3] IntelSiliconPkg/FirmwareBootMediaLib: Add library

2019-10-04 Thread Kubacki, Michael A 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2229

Adds a new library that is used to make system initialization
decisions in the boot flow dependent upon firmware boot media.
Note that the firmware boot media is the storage media that
the boot firmware is stored on. It is not the OS storage media
which may be stored upon a different non-volatile storage device.

Any Intel board, platform, or silicon code that must take action
conditionally based on the firmware boot media must use this
library API.

Cc: Sai Chaganty 
Cc: Ray Ni 
Signed-off-by: Michael Kubacki 
---
 Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec  
|   8 +-
 Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc  
|   4 +-
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
 |  43 
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
|  38 +++
 Silicon/Intel/IntelSiliconPkg/Include/Library/FirmwareBootMediaLib.h   
| 106 +++
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.c
   | 107 +++
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/FirmwareBootMediaLib.c
 | 109 
 
Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.c
  |  82 +++
 8 files changed, 495 insertions(+), 2 deletions(-)

diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec 
b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
index fe5bfa0dc6..f70e3b977d 100644
--- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
+++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
@@ -3,7 +3,7 @@
 #
 # This package provides common open source Intel silicon modules.
 #
-# Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
+# Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -22,6 +22,10 @@
   #
   MicrocodeFlashAccessLib|Include/Library/MicrocodeFlashAccessLib.h
 
+  ## @libraryclass  Provides services to identify the firmware boot media 
device.
+  #
+  FirmwareBootMediaLib|Include/Library/FirmwareBootMediaLib.h
+
 [Guids]
   ## GUID for Package token space
   # {A9F8D54E-1107-4F0A-ADD0-4587E7A4A735}
@@ -35,6 +39,8 @@
   ## Include/Guid/MicrocodeFmp.h
   gMicrocodeFmpImageTypeIdGuid  = { 0x96d4fdcd, 0x1502, 0x424d, { 0x9d, 
0x4c, 0x9b, 0x12, 0xd2, 0xdc, 0xae, 0x5c } }
 
+  gFirmwareBootMediaHobGuid = { 0x8c7340ea, 0xde8b, 0x4e06, {0xa4, 0x78, 0xec, 
0x8b, 0x62, 0xd7, 0xa, 0x8b } }
+
 [Ppis]
   gEdkiiVTdInfoPpiGuid = { 0x8a59fcb3, 0xf191, 0x400c, { 0x97, 0x67, 0x67, 
0xaf, 0x2b, 0x25, 0x68, 0x4a } }
 
diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc 
b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
index 58b5b656ef..3fb8a08b50 100644
--- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
+++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
@@ -1,7 +1,7 @@
 ## @file
 # This package provides common open source Intel silicon modules.
 #
-# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.
 #
 #SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -80,6 +80,8 @@
   
IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf
   IntelSiliconPkg/Feature/Capsule/MicrocodeUpdateDxe/MicrocodeUpdateDxe.inf
   
IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashAccessLibNull/MicrocodeFlashAccessLibNull.inf
+  IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwareBootMediaLib.inf
+  IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
 
 [BuildOptions]
   *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
diff --git 
a/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
 
b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
new file mode 100644
index 00..83ed5f04af
--- /dev/null
+++ 
b/Silicon/Intel/IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFirmwareBootMediaLib.inf
@@ -0,0 +1,43 @@
+## @file
+# Firmware Boot Media Library
+#
+# The firmware boot media device is used to make system initialization 
decisions in the boot flow dependent
+# upon firmware boot media. Note that the firmware boot media is the storage 
media that the boot firmware is stored on.
+# It is not the OS storage media which may be stored upon a different 
non-volatile storage device.
+#
+# This library contains an implementation for the DXE and SMM boot phases.
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION= 0x00010005
+  BASE_NAME  = DxeSmmFirmwareBootMediaLib
+  FILE_GUID

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

2019-10-04 Thread Kubacki, Michael A 
> On 10/03/19 23:53, Kubacki, Michael A wrote:
> > #1 - The plan is to remove the polling entirely in V3.
> >
> > #2 - I'd prefer to take a definitive direction and reduce validation and
> maintenance
> > effort but you and Laszlo both requested this so I'll add a 
> > FeaturePCD
> to control
> > activation of the runtime cache in this patch series. Perhaps this 
> > can be
> removed
> > in the future.
> 
> Thanks!
> 
> (I'm also happy with the lock / timeout resolution. I had known about the
> reentrancy restriction in the UEFI spec (I happened to look at something in
> the kernel just the other day that reminded me of that part of the spec), but
> it wasn't clear to me that the lock + timeout in the patch series were
> intended to protect against just that. Kudos to Andrew for the comment!)
> 
> (
> 
> Meanwhile, I've received help from my colleagues wrt.
> QueryVariableInfo(), but right now it's too early to tell if we'll be able to
> settle on that in the long term:
> 
> [PATCH] efi/efi_test: require CAP_SYS_ADMIN to open the chardev
> http://mid.mail-archive.com/20191003100712.31045-1-javierm@redhat.com
> 
> )
> 

Thanks for the QueryVariableInfo () update.

> For the next version of this edk2 patch set (where you plan to include the
> new FeaturePCD, if I understand correctly), I'd like to request the
> following: either set the DEC default to FALSE please, or please include a
> patch for OvmfPkg where you set the PCD to FALSE in all the OvmfPkg DSC
> files.
> 
> I think the next stable release should be made like that. Then, for the stable
> release following that, we can re-evaluate the question, and might decide to
> invert the PCD in OVMF (enabling the feature), assuming
> QueryVariableInfo() proves usable in Fedora, by then.
> 
> 

I'd like to propose the DEC default be set to TRUE and I make the changes in
all the OvmfPkg DSC files to set the PCD to FALSE. 

> Two independent questions:
> 
> - Has this work been regression-tested on ARM / AARCH64? (For example,
> ArmVirtPkg platforms use the unified runtime DXE driver, not the split
> runtime/SMM drivers. So no change in behavior is expected; we should test
> that.)
> 
> In the "Testing Performed" section of your blurb, item#3 suggests something
> similar ("Boot from S5 to EFI shell with DXE variables enabled"), but I 
> figured
> I'd raise AARCH64 specifically.
> 
> 

I have not tested on ARM / AARCH64. I will add this test for V3.

> - Can you please confirm that the handling of *volatile* variables is not
> affected? ArmVirtPkg and OvmfPkg use quite different sizes for volatile and
> non-volatile variables; see:
> 
>   - 9c7d0d499296 ("OvmfPkg/TlsAuthConfigLib: configure trusted CA certs
>   for HTTPS boot", 2018-03-30)
>   - ffe048a0807b ("ArmVirtPkg: handle NETWORK_TLS_ENABLE in
>   ArmVirtQemu*", 2019-06-28)
> 

The handling of volatile variables will not be affected.

> Thank you!
> Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48479): https://edk2.groups.io/g/devel/message/48479
Mute This Topic: https://groups.io/mt/34318592/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

2019-10-04 Thread Kubacki, Michael A 
> On 10/04/19 01:31, Kubacki, Michael A wrote:
> > I agree, I will make the default to enable the runtime cache.
> 
> I've just made a request for the opposite :) , before reading this part
> of the thread.
> 
> Let me revise my request then, seeing the above preference. From the
> following three options:
> 
> (1) set DEC default to FALSE,
> 
> (2) set the DEC default to TRUE, but set the PCD in OvmfPkg DSC files to
> FALSE,
> 
> (3) set the DEC default to TRUE, and inherit it in OvmfPkg,
> 
> I'd ask for (2) in the short or mid term, and entertain (3) in the long
> term (dependent on the upstream kernel patch I linked elsewhere in the
> thread:
>  javi...@redhat.com>).
> 
> 
> With the PCD available in the DEC file, I agree that downstream OVMF
> packages could easily be built with the PCD set to FALSE, e.g. on the
> "build" command line, regardless of the upstream OvmfPkg DSC setting.
> Given that, option (2) might appear superfluous.
> 
> However, I'd like upstream OvmfPkg's DSC setting to reflect that as yet,
> there is no alternative to GetVariable(), for exercising the SMM stack
> built into OVMF without side effects to the variable store. Once the
> kernel patch is merged and QueryVariableInfo() becomes *recommendable*
> as a practical substitute for GetVariable(), we can switch upstream
> OvmfPkg from option (2) to option (3).
> 
> Does that sound acceptable?
> 

I have no problem setting the PCD to FALSE in all the OvmfPkg DSC files. 

> Thanks!
> Laszlo
> 
> 
> >
> >> -Original Message-
> >> From: Kinney, Michael D 
> >> Sent: Thursday, October 3, 2019 3:01 PM
> >> To: Kubacki, Michael A ; Wu, Hao A
> >> ; devel@edk2.groups.io; Kinney, Michael D
> >> 
> >> Cc: Bi, Dandan ; Ard Biesheuvel
> >> ; Dong, Eric ; Laszlo
> Ersek
> >> ; Gao, Liming ; Ni, Ray
> >> ; Wang, Jian J ; Yao, Jiewen
> >> 
> >> Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT
> GetVariable()
> >> cache support
> >>
> >> Michael,
> >>
> >> Perhaps the FeaturePCD for #2 should be enabled by default
> >> so the platform DSC only needs to set this PCD for some
> >> validation tests.
> >>
> >> Mike
> >>
> >>
> >>> -Original Message-
> >>> From: Kubacki, Michael A 
> >>> Sent: Thursday, October 3, 2019 2:54 PM
> >>> To: Wu, Hao A ; devel@edk2.groups.io
> >>> Cc: Bi, Dandan ; Ard Biesheuvel
> >>> ; Dong, Eric
> >>> ; Laszlo Ersek ;
> >>> Gao, Liming ; Kinney, Michael D
> >>> ; Ni, Ray
> >>> ; Wang, Jian J
> >>> ; Yao, Jiewen
> >>> 
> >>> Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> >>> RT GetVariable() cache support
> >>>
> >>> #1 - The plan is to remove the polling entirely in V3.
> >>>
> >>> #2 - I'd prefer to take a definitive direction and
> >>> reduce validation and maintenance
> >>> effort but you and Laszlo both requested this so
> >>> I'll add a FeaturePCD to control
> >>> activation of the runtime cache in this patch
> >>> series. Perhaps this can be removed
> >>> in the future.
> >>>
> >>> #3 - Will be done in V3.
> >>>
> >>> Other replies are inline.
> >>>
>  -Original Message-
>  From: Wu, Hao A 
>  Sent: Thursday, October 3, 2019 1:05 AM
>  To: Kubacki, Michael A ;
>  devel@edk2.groups.io
>  Cc: Bi, Dandan ; Ard Biesheuvel
>  ; Dong, Eric
> >>> ; Laszlo Ersek
>  ; Gao, Liming
> >>> ; Kinney, Michael
>  D ; Ni, Ray
> >>> ; Wang, Jian J
>  ; Yao, Jiewen
> >>> 
>  Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add
> >>> RT GetVariable()
>  cache support
> 
>  Before any comment on the patch, since I am not
> >>> experienced in the
>  Variable
>  driver, I would like to ask for help from other
> >>> reviewers to look into this
>  patch and provide feedbacks as well. Thanks in
> >>> advance.
> 
>  With the above fact, some comments provided below
> >>> maybe wrong. So
>  please help
>  to kindly correct me.
> 
> 
>  Some general comments:
>  1. I am not sure if bringing the TimerLib dependency
> >>> (delay in acquiring the
> runtime cache read lock) to variable driver (a
> >>> software driver for the most
> part) is a good idea.
> 
> Hope other reviewers can provide some feedbacks for
> >>> this. Thanks in
>  advance.
> 
>  2. In my opinion, I prefer a switch can be provided
> >>> for platform owners to
> choose between using the runtime cache and going
> >>> through SMM for
>  GetVariable
> (and for GetNextVariableName in the next patch as
> >>> well).
> 
> If platform owners feel uncomfortable with using
> >>> the runtime cache with
> regard to the security perspective, they can switch
> >>> to the origin solution.
> 
>  3. Please help to remove the 'EFIAPI' keyword for new
> >>> driver internal
>  functions;
> 
> 
>  Inline comments below:
> 
> 
> > -Original Message-
> >

Re: [edk2-devel] [PATCH 33/35] StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader tracking

2019-10-04 Thread Achin Gupta 
Makes sense!

Reviewed-by: Achin Gupta 

On Thu, Oct 03, 2019 at 01:10:53PM +0200, Laszlo Ersek wrote:
> Pinging StandaloneMmPkg maintainers again, for reviewing this patch.
>
> Thanks
> Laszlo
>
> On 09/26/19 14:48, Laszlo Ersek wrote:
> > Achin, Jiewen, Supreeth,
> >
> > can one of you guys please review this patch?
> >
> > Thanks
> > Laszlo
> >
> > On 09/17/19 21:49, Laszlo Ersek wrote:
> >> The FvHasBeenProcessed() and FvIsBeingProcesssed() functions make sure
> >> that every firmware volume is processed only once (every driver in every
> >> firmware volume should be discovered only once). For this, the functions
> >> use a linked list.
> >>
> >> In MdeModulePkg's DXE Core and SMM Core, the key used for identifying
> >> those firmware volumes that have been processed is the EFI_HANDLE on which
> >> the DXE or SMM firmware volume protocol is installed. In the
> >> StandaloneMmPkg core however, the key is the address of the firmware
> >> volume header; that is, it has type (EFI_FIRMWARE_VOLUME_HEADER*).
> >>
> >> (EFI_FIRMWARE_VOLUME_HEADER*) has nothing to do with EFI_HANDLE.
> >> EFI_HANDLE just happens to be specified as (VOID*), and therefore the
> >> conversion between (EFI_FIRMWARE_VOLUME_HEADER*) and EFI_HANDLE is silent.
> >>
> >> (The FvHasBeenProcessed() and FvIsBeingProcesssed() functions were likely
> >> copied verbatim from MdeModulePkg's DXE Core and/or the SMM Core, and not
> >> flagged by the compiler in StandaloneMmPkg due to UEFI regrettably
> >> specifying EFI_HANDLE as (VOID*), thereby enabling the above implicit
> >> conversion.)
> >>
> >> We should not exploit this circumstance. Represent the key type faithfully
> >> instead.
> >>
> >> This is a semantic fix; there is no change in operation.
> >>
> >> Cc: Achin Gupta 
> >> Cc: Jiewen Yao 
> >> Cc: Supreeth Venkatesh 
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>
> >> Notes:
> >> build-tested only
> >>
> >>  StandaloneMmPkg/Core/StandaloneMmCore.h |  2 +-
> >>  StandaloneMmPkg/Core/Dispatcher.c   | 80 +++-
> >>  StandaloneMmPkg/Core/FwVol.c| 16 ++--
> >>  3 files changed, 52 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h 
> >> b/StandaloneMmPkg/Core/StandaloneMmCore.h
> >> index dcf91bc5e916..23ddbe169faf 100644
> >> --- a/StandaloneMmPkg/Core/StandaloneMmCore.h
> >> +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h
> >> @@ -67,7 +67,7 @@ typedef struct {
> >>
> >>LIST_ENTRY  ScheduledLink;// mScheduledQueue
> >>
> >> -  EFI_HANDLE  FvHandle;
> >> +  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
> >>EFI_GUIDFileName;
> >>VOID*Pe32Data;
> >>UINTN   Pe32DataSize;
> >> diff --git a/StandaloneMmPkg/Core/Dispatcher.c 
> >> b/StandaloneMmPkg/Core/Dispatcher.c
> >> index 3788389f95ed..9853445a64a1 100644
> >> --- a/StandaloneMmPkg/Core/Dispatcher.c
> >> +++ b/StandaloneMmPkg/Core/Dispatcher.c
> >> @@ -5,7 +5,7 @@
> >>  is added to the mDiscoveredList. The Before, and After Depex 
> >> are
> >>  pre-processed as drivers are added to the mDiscoveredList. If 
> >> an Apriori
> >>  file exists in the FV those drivers are addeded to the
> >> -mScheduledQueue. The mFvHandleList is used to make sure a
> >> +mScheduledQueue. The mFwVolList is used to make sure a
> >>  FV is only processed once.
> >>
> >>Step #2 - Dispatch. Remove driver from the mScheduledQueue and load and
> >> @@ -40,13 +40,13 @@
> >>  //
> >>  // MM Dispatcher Data structures
> >>  //
> >> -#define KNOWN_HANDLE_SIGNATURE  SIGNATURE_32('k','n','o','w')
> >> +#define KNOWN_FWVOL_SIGNATURE  SIGNATURE_32('k','n','o','w')
> >>
> >>  typedef struct {
> >> -  UINTN   Signature;
> >> -  LIST_ENTRY  Link; // mFvHandleList
> >> -  EFI_HANDLE  Handle;
> >> -} KNOWN_HANDLE;
> >> +  UINTN  Signature;
> >> +  LIST_ENTRY Link; // mFwVolList
> >> +  EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
> >> +} KNOWN_FWVOL;
> >>
> >>  //
> >>  // Function Prototypes
> >> @@ -86,9 +86,10 @@ LIST_ENTRY  mDiscoveredList = 
> >> INITIALIZE_LIST_HEAD_VARIABLE (mDiscoveredList);
> >>  LIST_ENTRY  mScheduledQueue = INITIALIZE_LIST_HEAD_VARIABLE 
> >> (mScheduledQueue);
> >>
> >>  //
> >> -// List of handles who's Fv's have been parsed and added to the 
> >> mFwDriverList.
> >> +// List of firmware volume headers whose containing firmware volumes have 
> >> been
> >> +// parsed and added to the mFwDriverList.
> >>  //
> >> -LIST_ENTRY  mFvHandleList = INITIALIZE_LIST_HEAD_VARIABLE (mFvHandleList);
> >> +LIST_ENTRY  mFwVolList = INITIALIZE_LIST_HEAD_VARIABLE (mFwVolList);
> >>
> >>  //
> >>  // Flag for the MM Dispacher.  TRUE if dispatcher is execuing.
> >> @@ -769,26 +770,30 @@ 
> >> MmInsertOnScheduledQueueWhileProcessingBeforeAndAfter (
> >>  }
> >>
> >>

Re: [edk2-devel] [PATCH v2 3/7] BaseTools: strip trailing whitespace

2019-10-04 Thread Michael D Kinney 
Hi Leif,

Do you want to run the same script on the edk2-platforms
repo?

Thanks,

Mike

> -Original Message-
> From: devel@edk2.groups.io  On
> Behalf Of Leif Lindholm
> Sent: Friday, October 4, 2019 3:20 AM
> To: Kinney, Michael D 
> Cc: devel@edk2.groups.io; Feng, Bob C
> ; Gao, Liming
> 
> Subject: Re: [edk2-devel] [PATCH v2 3/7] BaseTools:
> strip trailing whitespace
> 
> Mike, others,
> 
> Many thanks - series pushed as
> 61af5f249495..d19040804afb
> 
> /
> Leif
> 
> On Thu, Oct 03, 2019 at 09:05:05PM +, Kinney,
> Michael D wrote:
> > Leif,
> >
> > Reviewed-by: Michael D Kinney
> 
> >
> > I am covering for Liming and Bob this week, so you do
> not have to wait
> > for a review from them to push these changes.
> >
> > Mike
> >
> > > -Original Message-
> > > From: devel@edk2.groups.io  On
> Behalf Of Leif
> > > Lindholm
> > > Sent: Tuesday, October 1, 2019 5:49 AM
> > > To: devel@edk2.groups.io
> > > Cc: Feng, Bob C ; Gao, Liming
> > > 
> > > Subject: [edk2-devel] [PATCH v2 3/7] BaseTools:
> strip trailing
> > > whitespace
> > >
> > > Cc: Bob Feng 
> > > Cc: Liming Gao 
> > > Signed-off-by: Leif Lindholm
> 
> > > ---
> > >
> > > Resubmitting a v2 with changes to external projects:
> > > - BrotliCompress
> > > - LzmaCompress
> > > - Pccts
> > > backed out for now.
> > >
> > > BaseTools/Source/C/GNUmakefile
> |
> > > 2 +-
> > >  BaseTools/Source/C/Makefiles/app.makefile
> > > | 4 ++--
> > >  BaseTools/Source/C/Makefiles/footer.makefile
> > > | 4 ++--
> > >  BaseTools/Source/C/Makefiles/header.makefile
> > > | 8 
> > >  BaseTools/Source/C/Makefiles/lib.makefile
> > > | 2 +-
> > >  BaseTools/Source/C/Makefiles/ms.common
> > > | 4 ++--
> > >  BaseTools/Source/C/VfrCompile/GNUmakefile
> > > | 6 +++---
> > >  BaseTools/Source/Python/Ecc/Check.py
> > > | 2 +-
> > >
> BaseTools/Source/Python/GenFds/GenFdsGlobalVariable.py
> > > | 2 +-
> > >  BaseTools/Source/Python/Makefile
> > > | 2 +-
> > >  10 files changed, 18 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/BaseTools/Source/C/GNUmakefile
> > > b/BaseTools/Source/C/GNUmakefile index
> 37bcce519c7e..df4eb64ea95e
> > > 100644
> > > --- a/BaseTools/Source/C/GNUmakefile
> > > +++ b/BaseTools/Source/C/GNUmakefile
> > > @@ -77,7 +77,7 @@ $(SUBDIRS):
> > >  $(patsubst %,%-clean,$(sort $(SUBDIRS))):
> > >   -$(MAKE) -C $(@:-clean=) clean
> > >
> > > -$(VFRAUTOGEN): VfrCompile/VfrSyntax.g
> > > +$(VFRAUTOGEN): VfrCompile/VfrSyntax.g
> > >   $(MAKE) -C VfrCompile VfrLexer.h
> > >
> > >  clean:  $(patsubst %,%-clean,$(sort $(SUBDIRS)))
> diff - -git
> > > a/BaseTools/Source/C/Makefiles/app.makefile
> > > b/BaseTools/Source/C/Makefiles/app.makefile
> > > index fcadb4ed2194..6a2a8f5e8a0e 100644
> > > --- a/BaseTools/Source/C/Makefiles/app.makefile
> > > +++ b/BaseTools/Source/C/Makefiles/app.makefile
> > > @@ -12,9 +12,9 @@ include
> > > $(MAKEROOT)/Makefiles/header.makefile
> > >  APPLICATION = $(MAKEROOT)/bin/$(APPNAME)
> > >
> > >  .PHONY:all
> > > -all: $(MAKEROOT)/bin $(APPLICATION)
> > > +all: $(MAKEROOT)/bin $(APPLICATION)
> > >
> > > -$(APPLICATION): $(OBJECTS)
> > > +$(APPLICATION): $(OBJECTS)
> > >   $(LINKER) -o $(APPLICATION) $(BUILD_LFLAGS)
> > > $(OBJECTS) -L$(MAKEROOT)/libs $(LIBS)
> > >
> > >  $(OBJECTS):
> $(MAKEROOT)/Include/Common/BuildVersion.h
> > > diff --git
> > > a/BaseTools/Source/C/Makefiles/footer.makefile
> > > b/BaseTools/Source/C/Makefiles/footer.makefile
> > > index e823246cfbb4..85c3374224f2 100644
> > > --- a/BaseTools/Source/C/Makefiles/footer.makefile
> > > +++ b/BaseTools/Source/C/Makefiles/footer.makefile
> > > @@ -14,10 +14,10 @@ $(MAKEROOT)/libs-$(HOST_ARCH):
> > >  install: $(MAKEROOT)/libs-$(HOST_ARCH) $(LIBRARY)
> > >   cp $(LIBRARY) $(MAKEROOT)/libs-$(HOST_ARCH)
> > >
> > > -$(LIBRARY): $(OBJECTS)
> > > +$(LIBRARY): $(OBJECTS)
> > >   $(BUILD_AR) crs $@ $^
> > >
> > > -%.o : %.c
> > > +%.o : %.c
> > >   $(BUILD_CC)  -c $(BUILD_CPPFLAGS) $(BUILD_CFLAGS)
> $< -o $@
> > >
> > >  %.o : %.cpp
> > > diff --git
> > > a/BaseTools/Source/C/Makefiles/header.makefile
> > > b/BaseTools/Source/C/Makefiles/header.makefile
> > > index 52cbffcb4423..4e9b36d98bdb 100644
> > > --- a/BaseTools/Source/C/Makefiles/header.makefile
> > > +++ b/BaseTools/Source/C/Makefiles/header.makefile
> > > @@ -61,7 +61,7 @@ else
> > >  $(error Bad HOST_ARCH)
> > >  endif
> > >
> > > -INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I
> > > $(MAKEROOT)/Include/Common -I $(MAKEROOT)/Include/ -
> I
> > > $(MAKEROOT)/Include/IndustryStandard -I
> $(MAKEROOT)/Common/ -I .. -I
> > > . $(ARCH_INCLUDE)
> > > +INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I
> > > $(MAKEROOT)/Include/Common
> > > +-I $(MAKEROOT)/Include/ -I
> > > $(MAKEROOT)/Include/IndustryStandard -I
> > > +$(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE)
> > >  BUILD_CPPFLAGS = $(INCLUDE)
> > >
> > >  # keep EXTRA_OPTFLAGS last
> > > @@ -82,7 +82,7 @@ BUILD_CXXFLAGS = -Wno-unused-
> result
> > >
> > >  ifeq ($(HOST_ARCH), IA32)
> > >  #
> > > -# Snow Leopard  is a 32-bit and

Re: [edk2-devel] [PATCH v2] MdePkg:Include: Update SmBios header file

2019-10-04 Thread Abner Chang 
Just aware that SMBIOS 3.3.0 is published on DMTF,
https://www.dmtf.org/standards/smbios

The latest version of patch set is PATCH v4, please review it and help to push 
to mainstream if no further comments. Thanks
Abner

> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Thursday, September 19, 2019 1:23 PM
> To: Chang, Abner (HPS SW/FW Technologist) ;
> devel@edk2.groups.io
> Cc: Kinney, Michael D ; Leif Lindholm
> ; Chen, Gilbert 
> Subject: RE: [PATCH v2] MdePkg:Include: Update SmBios header file
> 
> Abner:
>   Please add BZ URL in the commit message, and separate this patch to three
> changes. Each one is for each package of MdePkg, MdeModulePkg and
> ShellPkg.
> 
>   As Leif say, SmBios 3.3 spec is not published. This patch will not be pushed
> until SmBios 3.3 is published.
> 
> >-Original Message-
> >From: Abner Chang [mailto:abner.ch...@hpe.com]
> >Sent: Thursday, September 19, 2019 11:05 AM
> >To: devel@edk2.groups.io
> >Cc: abner.ch...@hpe.com; Kinney, Michael D
> >; Gao, Liming ; Leif
> >Lindholm ; Gilbert Chen
> >
> >Subject: [PATCH v2] MdePkg:Include: Update SmBios header file
> >
> >Update SmBios header file to conform with SMBIOS v3.3.0.
> >The major update is to add definitions of SMBIOS Type 44h record.
> >
> >Signed-off-by: Abner Chang 
> >
> >Cc: Michael D Kinney 
> >Cc: Liming Gao 
> >Cc: Leif Lindholm 
> >Cc: Gilbert Chen 
> >---
> > MdeModulePkg/MdeModulePkg.dec  |  6 +-
> > MdePkg/Include/IndustryStandard/SmBios.h   | 76
> >+-
> > .../SmbiosView/PrintInfo.c | 23 ++-
> > .../SmbiosView/PrintInfo.h | 13 +++-
> > .../SmbiosView/QueryTable.c| 63 +-
> > .../UefiShellDebug1CommandsLib.uni |  3 +-
> > 6 files changed, 174 insertions(+), 10 deletions(-)
> >
> >diff --git a/MdeModulePkg/MdeModulePkg.dec
> >b/MdeModulePkg/MdeModulePkg.dec index 19935c8..e3a65ab 100644
> >--- a/MdeModulePkg/MdeModulePkg.dec
> >+++ b/MdeModulePkg/MdeModulePkg.dec
> >@@ -1792,10 +1792,10 @@
> >
> >   ## SMBIOS version.
> >   # @Prompt SMBIOS version.
> >-
> >gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0302|UINT16|0
> x0
> >0010055
> >+
> >gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion|0x0303|UINT16|0
> x0
> >0010055
> >
> >-  ## SMBIOS Docrev field in SMBIOS 3.0 (64-bit) Entry Point Structure.
> >-  # @Prompt SMBIOS Docrev field in SMBIOS 3.0 (64-bit) Entry Point
> Structure.
> >+  ## SMBIOS Docrev field in SMBIOS 3.3 (64-bit) Entry Point Structure.
> >+  # @Prompt SMBIOS Docrev field in SMBIOS 3.3 (64-bit) Entry Point
> >Structure.
> >
> >gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0|UINT8|0x0001
> 00
> >6A
> >
> >   ## SMBIOS produce method.
> >diff --git a/MdePkg/Include/IndustryStandard/SmBios.h
> >b/MdePkg/Include/IndustryStandard/SmBios.h
> >index f3b6f18..f504cc8 100644
> >--- a/MdePkg/Include/IndustryStandard/SmBios.h
> >+++ b/MdePkg/Include/IndustryStandard/SmBios.h
> >@@ -1,8 +1,9 @@
> > /** @file
> >-  Industry Standard Definitions of SMBIOS Table Specification v3.2.0.
> >+  Industry Standard Definitions of SMBIOS Table Specification v3.3.0.
> >
> > Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> > (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP
> >+(C) Copyright 2015 - 2019 Hewlett Packard Enterprise Development
> >+LP
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> >@@ -46,7 +47,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  #define
> >SMBIOS_3_0_TABLE_MAX_LENGTH 0x
> >
> > //
> >-// SMBIOS type macros which is according to SMBIOS 2.7 specification.
> >+// SMBIOS type macros which is according to SMBIOS 3.3.0 specification.
> > //
> > #define SMBIOS_TYPE_BIOS_INFORMATION 0
> > #define SMBIOS_TYPE_SYSTEM_INFORMATION   1
> >@@ -92,6 +93,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  #define
> >SMBIOS_TYPE_ONBOARD_DEVICES_EXTENDED_INFORMATION 41  #define
> >SMBIOS_TYPE_MANAGEMENT_CONTROLLER_HOST_INTERFACE 42
> > #define SMBIOS_TYPE_TPM_DEVICE   43
> >+#define SMBIOS_TYPE_PROCESSOR_ADDITIONAL_INFORMATION 44
> >
> > ///
> > /// Inactive type is added from SMBIOS 2.2. Reference SMBIOS 2.6,
> >chapter 3.3.43.
> >@@ -727,7 +729,10 @@ typedef enum {
> >   ProcessorFamilyMII   = 0x012E,
> >   ProcessorFamilyWinChip   = 0x0140,
> >   ProcessorFamilyDSP   = 0x015E,
> >-  ProcessorFamilyVideoProcessor= 0x01F4
> >+  ProcessorFamilyVideoProcessor= 0x01F4,
> >+  ProcessorFamilyRiscvRV32 = 0x0200,
> >+  ProcessorFamilyRiscVRV64 = 0x0201,
> >+  ProcessorFamilyRiscVRV128= 0x0202
> > } PROCESSOR_FAMILY2_DATA;
> >
> > ///
> >@@ -857,6 +862,19 @@ typedef struct {
> > } PROCESSOR_FEATURE_FLAGS;
> >
> > typedef struct {
> >+  UINT32  ProcessorReserved1 :1;
> >+  UINT32

Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature

2019-10-04 Thread Igor Mammedov 
On Tue, 1 Oct 2019 17:31:17 +0200
"Laszlo Ersek"  wrote:

> On 09/27/19 13:35, Igor Mammedov wrote:
> > On Tue, 24 Sep 2019 13:34:55 +0200
> > "Laszlo Ersek"  wrote:  
> 
> >> Going forward, if I understand correctly, the plan is to populate the
> >> new SMRAM with the hotplug SMI handler. (This would likely be put in
> >> place by OvmfPkg/PlatformPei, from NASM source code. The
> >> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
> >> original contents before, and restores them after, the initial SMBASE
> >> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
> >> survive the initial relocation of the cold-plugged VCPUs.)  
> 
> > that's the tricky part, can we safely detect (in SmmRelocateBases)
> > race condition in case of several hotplugged CPUs are executing
> > SMI relocation handler at the same time? (crashing system in case
> > that happens is good enough)  
> 
> Wait, there are *two* initial SMI handlers here, one for cold-plugged
> CPUs, and another for hot-plugged CPUs. Let's just call them coldplug
> and hotplug handlers, for simplicity.
> 
> In chronological order, during boot:
> 
> (1) OvmfPkg/PlatformPei would put the hotplug handler in place, at the
> default SMBASE. This code would lie dormant for a long time.
> 
> (2) UefiCpuPkg/PiSmmCpuDxeSmm backs up the current contents of the
> default SMBASE area, and installs the coldplug handler. This handler is
> then actually used, for relocating SMBASE for the present (cold-plugged)
> CPUs. Finally, UefiCpuPkg/PiSmmCpuDxeSmm restores the original contents
> of the default SMBASE area. This would in effect re-establish the
> hotplug handler from (1).
> 
> (3) At OS runtime, a CPU is hotplugged, and then the hotplug handler
> runs, at the default SMBASE.
> 
> The function SmmRelocateBases() is strictly limited to step (2), and has
> nothing to do with hotplug. Therefore it need not deal with any race
> conditions related to multi-hotplug.
> 
> This is just to clarify that your question applies to the initial SMI
> handler (the hotplug one) that is put in place in step (1), and then
> used in step (3). The question does not apply to SmmRelocateBases().
> 
> 
> With that out of the way, I do think we should be able to detect this,
> with a simple -- haha, famous last words! -- compare-and-exchange (LOCK
> CMPXCHG); a kind of semaphore.
> 
> The initial value of an integer variable in SMRAM should be 1. At the
> start of the hotplug initial SMI handler, the CPU should try to swap
> that with 0. If it succeeds, proceed. If it fails (the variable's value
> is not 1), force a platform reset. Finally, right before the RSM,
> restore 1 (swap it back).
> 
> (It does not matter if another hotplug CPU starts the relocation in SMM
> while the earlier one is left with *only* the RSM instruction in SMM,
> immediately after the swap-back.)
I don't see why it doesn't matter, let me illustrate the case
I'm talking about.

assuming there are 2 hotplugged CPUs with pending SMI and
stray INIT-SIPI-SIPI in flight to CPU2.

HP CPU1HP CPU2
save state at 0x3-
start executing hotplug SMI handler  -
set new SMBASE   -
- save state at 0x3
 (overwrites CPU1 set SMBASE to 
default one)
-  .
 ---zzz--   
  
RSM
restores CPU1 with CPU2 saved state
 incl. CPU2 SMBASE = either default or
 already new CPU2 specific one

semaphore is irrelevant in this case as we will loose CPU1 relocation
at best (assuming hotplug handler does nothing else beside relocation)
and if hotplug handler does something else this case will probably
leave FW in inconsistent state.


Safest way to deal with it would be:

  1. wait till all host CPUs are brought into SMM (with hotplug SMI handler = 
check me in)

  2. wait till all hotplugged CPUs are put in well know state
(a host cpu should send INIT-SIPI-SIPI with NOP vector to wake up)

  3. a host CPU will serialize hotplugged CPUs relocation by sending
 uincast SMI + INIT-SIPI-SIPI NOP vector to wake up the second time
 (with hotplug SMI handler = relocate_me)

alternatively we could skip step 3 and do following:

 hotplug_SMI_handler()   
 hp_cpu_check_in_map[apic_id] = 1;
 /* wait till another cpu tells me to continue */
 while(hp_cpu_check_in_map[apic_id]) ;;
 ...

 host_cpu_hotplug_all_cpus()
 wait till all hotpluged CPUs are in hp_cpu_check_in_map;
 for(i=0;;) {
if (hp_cpu_check_in_map[i]) {
/* relocate this CPU */
hp_cpu_check_in_map[i] = 0;
}

tell QEMU that FW pulled the CPU in;
/* we can add a flag to QEMU's cpu hotplug MMIO interface to FW do

Re: [edk2-devel] [PATCH 27/35] SecurityPkg: stop abusing EFI_EVENT for protocol notify registration

2019-10-04 Thread Zhang, Chao B 
Reviewed-by : Chao Zhang 

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: 2019年10月3日 19:07
To: Zhang, Chao B ; Wang, Jian J 
; Yao, Jiewen 
Cc: edk2-devel-groups-io 
Subject: Re: [edk2-devel] [PATCH 27/35] SecurityPkg: stop abusing EFI_EVENT for 
protocol notify registration

Pinging SecurityPkg maintainers again, for reviewing this patch.

Thanks
Laszlo

On 09/26/19 14:46, Laszlo Ersek wrote:
> Chao, Jian, Jiewen,
> 
> can you please review this patch?
> 
> Thanks,
> Laszlo
> 
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
>> EfiCreateProtocolNotifyEvent() takes a (VOID**) for "Registration", 
>> similarly to gBS->RegisterProtocolNotify(). We should pass the 
>> address of an actual pointer-to-VOID, and not the address of an 
>> EFI_EVENT. EFI_EVENT just happens to be specified as (VOID*), and has 
>> nothing to do with the registration.
>>
>> This change is a no-op in practice; it's a semantic improvement.
>>
>> Cc: Chao Zhang 
>> Cc: Jian Wang 
>> Cc: Jiewen Yao 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> build-tested only
>>
>>  SecurityPkg/HddPassword/HddPasswordDxe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/SecurityPkg/HddPassword/HddPasswordDxe.c 
>> b/SecurityPkg/HddPassword/HddPasswordDxe.c
>> index b0d795b6597f..051e64091d7f 100644
>> --- a/SecurityPkg/HddPassword/HddPasswordDxe.c
>> +++ b/SecurityPkg/HddPassword/HddPasswordDxe.c
>> @@ -2770,7 +2770,7 @@ HddPasswordDxeInit (  {
>>EFI_STATUS Status;
>>HDD_PASSWORD_DXE_PRIVATE_DATA  *Private;
>> -  EFI_EVENT  Registration;
>> +  VOID   *Registration;
>>EFI_EVENT  EndOfDxeEvent;
>>EDKII_VARIABLE_LOCK_PROTOCOL   *VariableLock;
>>  
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48474): https://edk2.groups.io/g/devel/message/48474
Mute This Topic: https://groups.io/mt/34180229/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 26/35] SecurityPkg: fix UninstallMultipleProtocolInterfaces() calls

2019-10-04 Thread Zhang, Chao B 
Hi Laszlo:
   Sorry for late response. The fix is good to me. I am also interested in how 
you find this issue, can you share it?
 
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: 2019年10月3日 19:07
To: Zhang, Chao B ; Wang, Jian J 
; Yao, Jiewen 
Cc: edk2-devel-groups-io 
Subject: Re: [edk2-devel] [PATCH 26/35] SecurityPkg: fix 
UninstallMultipleProtocolInterfaces() calls

Pinging SecurityPkg maintainers again, for reviewing this patch.

Thanks
Laszlo

On 09/26/19 14:45, Laszlo Ersek wrote:
> Chao, Jian, Jiewen,
> 
> can you please review this patch?
> 
> Thanks,
> Laszlo
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
>> Unlike the InstallMultipleProtocolInterfaces() boot service, which 
>> takes an (EFI_HANDLE*) as first parameter, the
>> UninstallMultipleProtocolInterfaces() boot service takes an 
>> EFI_HANDLE as first parameter.
>>
>> These are actual bugs. They must have remained hidden until now 
>> because they are all in Unload() functions, which are probably 
>> exercised infrequently. Fix the UninstallMultipleProtocolInterfaces() calls.
>>
>> Cc: Chao Zhang 
>> Cc: Jian Wang 
>> Cc: Jiewen Yao 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> Notes:
>> build-tested only
>>
>>  SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c   
>>| 2 +-
>>  SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c  
>>| 2 +-
>>  
>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfi
>> gDriver.c | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c 
>> b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>> index 54155a338100..9052eced757d 100644
>> --- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>> +++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c
>> @@ -443,7 +443,7 @@ Tcg2ConfigDriverUnload (
>>ASSERT (PrivateData->Signature == 
>> TCG2_CONFIG_PRIVATE_DATA_SIGNATURE);
>>  
>>gBS->UninstallMultipleProtocolInterfaces (
>> - ,
>> + ImageHandle,
>>   ,
>>   PrivateData,
>>   NULL
>> diff --git a/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c 
>> b/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>> index 341879e4c4ba..fb06624fdb8f 100644
>> --- a/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>> +++ b/SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c
>> @@ -138,7 +138,7 @@ TcgConfigDriverUnload (
>>ASSERT (PrivateData->Signature == 
>> TCG_CONFIG_PRIVATE_DATA_SIGNATURE);
>>  
>>gBS->UninstallMultipleProtocolInterfaces (
>> - ,
>> + ImageHandle,
>>   ,
>>   PrivateData,
>>   NULL
>> diff --git 
>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
>> figDriver.c 
>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
>> figDriver.c index 798ef9cfbc01..6c0294151e6c 100644
>> --- 
>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
>> figDriver.c
>> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoo
>> +++ tConfigDriver.c
>> @@ -115,7 +115,7 @@ SecureBootConfigDriverUnload (
>>ASSERT (PrivateData->Signature == 
>> SECUREBOOT_CONFIG_PRIVATE_DATA_SIGNATURE);
>>  
>>gBS->UninstallMultipleProtocolInterfaces (
>> - ,
>> + ImageHandle,
>>   ,
>>   PrivateData,
>>   NULL
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48473): https://edk2.groups.io/g/devel/message/48473
Mute This Topic: https://groups.io/mt/34180228/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Qemu-devel] [PATCH 1/2] q35: implement 128K SMRAM at default SMBASE address

2019-10-04 Thread Igor Mammedov 
On Tue, 1 Oct 2019 20:03:20 +0200
"Laszlo Ersek"  wrote:

> On 09/30/19 16:22, Yao, Jiewen wrote:
> >   
> >> -Original Message-
> >> From: devel@edk2.groups.io  On Behalf Of Igor
> >> Mammedov
> >> Sent: Monday, September 30, 2019 8:37 PM
> >> To: Laszlo Ersek   
> 
> >>> To me it looks like we need to figure out how QEMU can make the OS call
> >>> into SMM (in the GPE cpu hotplug handler), passing in parameters and
> >>> such. This would be step (03).
> >>>
> >>> Do you agree?
> >>>
> >>> If so, I'll ask Jiewen about such OS->SMM calls separately, because I
> >>> seem to remember that there used to be an "SMM communcation table" of
> >>> sorts, for flexible OS->SMM calls. However, it appears to be deprecated
> >>> lately.  
> >> we can try to resurrect and put over it some kind of protocol
> >> to describe which CPUs to where hotplugged.
> >>
> >> or we could put a parameter into SMI status register (IO port 0xb3)
> >> and the trigger SMI from GPE handler to tell SMI handler that cpu
> >> hotplug happened and then use QEMU's cpu hotplug interface
> >> to enumerate hotplugged CPUs for SMI handler.
> >>
> >> The later is probably simpler as we won't need to reinvent the wheel
> >> (just reuse the interface that's already in use by GPE handler).  
> 
> Based on "docs/specs/acpi_cpu_hotplug.txt", this seems to boil down to a
> bunch of IO port accesses at base 0x0cd8.
> 
> Is that correct?

yep, you can use it to iterate over hotplugged CPUs.
hw side (QEMU) uses cpu_hotplug_ops as IO write/read handlers
and firmware side (ACPI) scannig for hotplugged CPUs is implemented
in CPU_SCAN_METHOD.

What we can do on QEMU side is to write agreed upon value to command port (0xB2)
from CPU_SCAN_METHOD after taking ctrl_lock but before starting scan loop.
That way firmware will first bring up (from fw pov) all hotplugged CPUs
and then return control to OS to do the same from OS pov.


> 
> > [Jiewen] The PI specification Volume 4 - SMM defines 
> > EFI_MM_COMMUNICATION_PROTOCOL.Communicate() - It can be used to communicate 
> > between OS and SMM handler. But it requires the runtime protocol call. I am 
> > not sure how OS loader passes this information to OS kernel.
> > 
> > As such, I think using ACPI SCI/GPE -> software SMI handler is an easier 
> > way to achieve this. I also recommend this way.
> > For parameter passing, we can use 1) Port B2 (1 byte), 2) Port B3 (1 byte), 
> > 3) chipset scratch register (4 bytes or 8 bytes, based upon scratch 
> > register size), 4) ACPI NVS OPREGION, if the data structure is complicated. 
> >  
> 
> I'm confused about the details. In two categories:
> (1) what values to use,
> (2) how those values are passed.
> 
> Assume a CPU is hotpluged, QEMU injects an SCI, and the ACPI GPE handler
> in the OS -- which also originates from QEMU -- writes a particular byte
> to the Data port (0xB3), and then to the Control port (0xB2),
> broadcasting an SMI.
> 
> (1) What values to use.
> 
> Note that values ICH9_APM_ACPI_ENABLE (2) and ICH9_APM_ACPI_DISABLE (3)
> are already reserved in QEMU for IO port 0xB2, for different purposes.
> So we can't use those in the GPE handler.

SeaBIOS writes 0x00 into command port, but it seems that's taken by
EFI_SMM_COMMUNICATION_PROTOCOL. So we can use the next unused value
(lets say 0x4). We probably don't have to use status port or 
EFI_SMM_COMMUNICATION_PROTOCOL, since the value of written into 0xB2
is sufficient to distinguish hotplug event.

> Furthermote, OVMF's EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation
> (in "OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c") writes 0 to both ports,
> as long as the caller does not specify them.
> 
>   IoWrite8 (ICH9_APM_STS, DataPort== NULL ? 0 : *DataPort);
>   IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort);
> 
> And, there is only one Trigger() call site in edk2: namely in
> "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c", in the
> SmmCommunicationCommunicate() function.
> 
> (That function implements EFI_SMM_COMMUNICATION_PROTOCOL.Communicate().)
> This call site passes NULL for both DataPort and CommandPort.
> 
> Yet further, EFI_SMM_COMMUNICATION_PROTOCOL.Communicate() is used for
> example by the UEFI variable driver, for talking between the
> unprivileged (runtime DXE) and privileged (SMM) half.
> 
> As a result, all of the software SMIs currently in use in OVMF, related
> to actual firmware services, write 0 to both ports.
> 
> I guess we can choose new values, as long as we avoid 2 and 3 for the
> control port (0xB2), because those are reserved in QEMU -- see
> ich9_apm_ctrl_changed() in "hw/isa/lpc_ich9.c".
> 
> 
> (2) How the parameters are passed.
> 
> 
> (2a) For the new CPU, the SMI remains pending, until it gets an
> INIT-SIPI-SIPI from one of the previously plugged CPUs (most likely, the
> BSP). At that point, the new CPU will execute the "initial SMI handler
> for hotplugged CPUs", at the default SMBASE.
> 
> That's a routine we'll have to write in assembly, from zero. In

Re: [edk2-devel] [PATCH v2 3/7] BaseTools: strip trailing whitespace

2019-10-04 Thread Leif Lindholm 
Mike, others,

Many thanks - series pushed as
61af5f249495..d19040804afb

/
Leif

On Thu, Oct 03, 2019 at 09:05:05PM +, Kinney, Michael D wrote:
> Leif,
> 
> Reviewed-by: Michael D Kinney 
> 
> I am covering for Liming and Bob this week, so you do not
> have to wait for a review from them to push these changes.
> 
> Mike
> 
> > -Original Message-
> > From: devel@edk2.groups.io  On
> > Behalf Of Leif Lindholm
> > Sent: Tuesday, October 1, 2019 5:49 AM
> > To: devel@edk2.groups.io
> > Cc: Feng, Bob C ; Gao, Liming
> > 
> > Subject: [edk2-devel] [PATCH v2 3/7] BaseTools: strip
> > trailing whitespace
> > 
> > Cc: Bob Feng 
> > Cc: Liming Gao 
> > Signed-off-by: Leif Lindholm 
> > ---
> > 
> > Resubmitting a v2 with changes to external projects:
> > - BrotliCompress
> > - LzmaCompress
> > - Pccts
> > backed out for now.
> > 
> > BaseTools/Source/C/GNUmakefile |
> > 2 +-
> >  BaseTools/Source/C/Makefiles/app.makefile
> > | 4 ++--
> >  BaseTools/Source/C/Makefiles/footer.makefile
> > | 4 ++--
> >  BaseTools/Source/C/Makefiles/header.makefile
> > | 8 
> >  BaseTools/Source/C/Makefiles/lib.makefile
> > | 2 +-
> >  BaseTools/Source/C/Makefiles/ms.common
> > | 4 ++--
> >  BaseTools/Source/C/VfrCompile/GNUmakefile
> > | 6 +++---
> >  BaseTools/Source/Python/Ecc/Check.py
> > | 2 +-
> >  BaseTools/Source/Python/GenFds/GenFdsGlobalVariable.py
> > | 2 +-
> >  BaseTools/Source/Python/Makefile
> > | 2 +-
> >  10 files changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/BaseTools/Source/C/GNUmakefile
> > b/BaseTools/Source/C/GNUmakefile index
> > 37bcce519c7e..df4eb64ea95e 100644
> > --- a/BaseTools/Source/C/GNUmakefile
> > +++ b/BaseTools/Source/C/GNUmakefile
> > @@ -77,7 +77,7 @@ $(SUBDIRS):
> >  $(patsubst %,%-clean,$(sort $(SUBDIRS))):
> > -$(MAKE) -C $(@:-clean=) clean
> > 
> > -$(VFRAUTOGEN): VfrCompile/VfrSyntax.g
> > +$(VFRAUTOGEN): VfrCompile/VfrSyntax.g
> > $(MAKE) -C VfrCompile VfrLexer.h
> > 
> >  clean:  $(patsubst %,%-clean,$(sort $(SUBDIRS))) diff -
> > -git a/BaseTools/Source/C/Makefiles/app.makefile
> > b/BaseTools/Source/C/Makefiles/app.makefile
> > index fcadb4ed2194..6a2a8f5e8a0e 100644
> > --- a/BaseTools/Source/C/Makefiles/app.makefile
> > +++ b/BaseTools/Source/C/Makefiles/app.makefile
> > @@ -12,9 +12,9 @@ include
> > $(MAKEROOT)/Makefiles/header.makefile
> >  APPLICATION = $(MAKEROOT)/bin/$(APPNAME)
> > 
> >  .PHONY:all
> > -all: $(MAKEROOT)/bin $(APPLICATION)
> > +all: $(MAKEROOT)/bin $(APPLICATION)
> > 
> > -$(APPLICATION): $(OBJECTS)
> > +$(APPLICATION): $(OBJECTS)
> > $(LINKER) -o $(APPLICATION) $(BUILD_LFLAGS)
> > $(OBJECTS) -L$(MAKEROOT)/libs $(LIBS)
> > 
> >  $(OBJECTS): $(MAKEROOT)/Include/Common/BuildVersion.h
> > diff --git
> > a/BaseTools/Source/C/Makefiles/footer.makefile
> > b/BaseTools/Source/C/Makefiles/footer.makefile
> > index e823246cfbb4..85c3374224f2 100644
> > --- a/BaseTools/Source/C/Makefiles/footer.makefile
> > +++ b/BaseTools/Source/C/Makefiles/footer.makefile
> > @@ -14,10 +14,10 @@ $(MAKEROOT)/libs-$(HOST_ARCH):
> >  install: $(MAKEROOT)/libs-$(HOST_ARCH) $(LIBRARY)
> > cp $(LIBRARY) $(MAKEROOT)/libs-$(HOST_ARCH)
> > 
> > -$(LIBRARY): $(OBJECTS)
> > +$(LIBRARY): $(OBJECTS)
> > $(BUILD_AR) crs $@ $^
> > 
> > -%.o : %.c
> > +%.o : %.c
> > $(BUILD_CC)  -c $(BUILD_CPPFLAGS) $(BUILD_CFLAGS) $<
> > -o $@
> > 
> >  %.o : %.cpp
> > diff --git
> > a/BaseTools/Source/C/Makefiles/header.makefile
> > b/BaseTools/Source/C/Makefiles/header.makefile
> > index 52cbffcb4423..4e9b36d98bdb 100644
> > --- a/BaseTools/Source/C/Makefiles/header.makefile
> > +++ b/BaseTools/Source/C/Makefiles/header.makefile
> > @@ -61,7 +61,7 @@ else
> >  $(error Bad HOST_ARCH)
> >  endif
> > 
> > -INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I
> > $(MAKEROOT)/Include/Common -I $(MAKEROOT)/Include/ -I
> > $(MAKEROOT)/Include/IndustryStandard -I
> > $(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE)
> > +INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I
> > $(MAKEROOT)/Include/Common
> > +-I $(MAKEROOT)/Include/ -I
> > $(MAKEROOT)/Include/IndustryStandard -I
> > +$(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE)
> >  BUILD_CPPFLAGS = $(INCLUDE)
> > 
> >  # keep EXTRA_OPTFLAGS last
> > @@ -82,7 +82,7 @@ BUILD_CXXFLAGS = -Wno-unused-result
> > 
> >  ifeq ($(HOST_ARCH), IA32)
> >  #
> > -# Snow Leopard  is a 32-bit and 64-bit environment.
> > uname -m returns i386, but gcc defaults
> > +# Snow Leopard  is a 32-bit and 64-bit environment.
> > uname -m returns
> > +i386, but gcc defaults
> >  #  to x86_64. So make sure tools match uname -m. You
> > can manual have a 64-bit kernal on Snow Leopard  #  so
> > only do this is uname -m returns i386.
> >  #
> > @@ -96,7 +96,7 @@ endif
> >  # keep BUILD_OPTFLAGS last
> >  BUILD_CFLAGS   += $(BUILD_OPTFLAGS)
> >  BUILD_CXXFLAGS += $(BUILD_OPTFLAGS)
> > -
> > +
> >  # keep EXTRA_LDFLAGS last
> >  BUILD_LFLAGS += $(EXTRA_LDFLAGS)
> > 
> > @@ -107,7 +107,7 @@ BUILD_LFLAGS += $(EXTRA_LDFLAGS)
> >  all:
> > 
>

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

2019-10-04 Thread Laszlo Ersek 
On 10/04/19 01:31, Kubacki, Michael A wrote:
> I agree, I will make the default to enable the runtime cache.

I've just made a request for the opposite :) , before reading this part
of the thread.

Let me revise my request then, seeing the above preference. From the
following three options:

(1) set DEC default to FALSE,

(2) set the DEC default to TRUE, but set the PCD in OvmfPkg DSC files to
FALSE,

(3) set the DEC default to TRUE, and inherit it in OvmfPkg,

I'd ask for (2) in the short or mid term, and entertain (3) in the long
term (dependent on the upstream kernel patch I linked elsewhere in the
thread:
).


With the PCD available in the DEC file, I agree that downstream OVMF
packages could easily be built with the PCD set to FALSE, e.g. on the
"build" command line, regardless of the upstream OvmfPkg DSC setting.
Given that, option (2) might appear superfluous.

However, I'd like upstream OvmfPkg's DSC setting to reflect that as yet,
there is no alternative to GetVariable(), for exercising the SMM stack
built into OVMF without side effects to the variable store. Once the
kernel patch is merged and QueryVariableInfo() becomes *recommendable*
as a practical substitute for GetVariable(), we can switch upstream
OvmfPkg from option (2) to option (3).

Does that sound acceptable?

Thanks!
Laszlo


> 
>> -Original Message-
>> From: Kinney, Michael D 
>> Sent: Thursday, October 3, 2019 3:01 PM
>> To: Kubacki, Michael A ; Wu, Hao A
>> ; devel@edk2.groups.io; Kinney, Michael D
>> 
>> Cc: Bi, Dandan ; Ard Biesheuvel
>> ; Dong, Eric ; Laszlo Ersek
>> ; Gao, Liming ; Ni, Ray
>> ; Wang, Jian J ; Yao, Jiewen
>> 
>> Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable()
>> cache support
>>
>> Michael,
>>
>> Perhaps the FeaturePCD for #2 should be enabled by default
>> so the platform DSC only needs to set this PCD for some
>> validation tests.
>>
>> Mike
>>
>>
>>> -Original Message-
>>> From: Kubacki, Michael A 
>>> Sent: Thursday, October 3, 2019 2:54 PM
>>> To: Wu, Hao A ; devel@edk2.groups.io
>>> Cc: Bi, Dandan ; Ard Biesheuvel
>>> ; Dong, Eric
>>> ; Laszlo Ersek ;
>>> Gao, Liming ; Kinney, Michael D
>>> ; Ni, Ray
>>> ; Wang, Jian J
>>> ; Yao, Jiewen
>>> 
>>> Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add
>>> RT GetVariable() cache support
>>>
>>> #1 - The plan is to remove the polling entirely in V3.
>>>
>>> #2 - I'd prefer to take a definitive direction and
>>> reduce validation and maintenance
>>> effort but you and Laszlo both requested this so
>>> I'll add a FeaturePCD to control
>>> activation of the runtime cache in this patch
>>> series. Perhaps this can be removed
>>> in the future.
>>>
>>> #3 - Will be done in V3.
>>>
>>> Other replies are inline.
>>>
 -Original Message-
 From: Wu, Hao A 
 Sent: Thursday, October 3, 2019 1:05 AM
 To: Kubacki, Michael A ;
 devel@edk2.groups.io
 Cc: Bi, Dandan ; Ard Biesheuvel
 ; Dong, Eric
>>> ; Laszlo Ersek
 ; Gao, Liming
>>> ; Kinney, Michael
 D ; Ni, Ray
>>> ; Wang, Jian J
 ; Yao, Jiewen
>>> 
 Subject: RE: [PATCH V2 7/9] MdeModulePkg/Variable: Add
>>> RT GetVariable()
 cache support

 Before any comment on the patch, since I am not
>>> experienced in the
 Variable
 driver, I would like to ask for help from other
>>> reviewers to look into this
 patch and provide feedbacks as well. Thanks in
>>> advance.

 With the above fact, some comments provided below
>>> maybe wrong. So
 please help
 to kindly correct me.


 Some general comments:
 1. I am not sure if bringing the TimerLib dependency
>>> (delay in acquiring the
runtime cache read lock) to variable driver (a
>>> software driver for the most
part) is a good idea.

Hope other reviewers can provide some feedbacks for
>>> this. Thanks in
 advance.

 2. In my opinion, I prefer a switch can be provided
>>> for platform owners to
choose between using the runtime cache and going
>>> through SMM for
 GetVariable
(and for GetNextVariableName in the next patch as
>>> well).

If platform owners feel uncomfortable with using
>>> the runtime cache with
regard to the security perspective, they can switch
>>> to the origin solution.

 3. Please help to remove the 'EFIAPI' keyword for new
>>> driver internal
 functions;


 Inline comments below:


> -Original Message-
> From: Kubacki, Michael A
> Sent: Saturday, September 28, 2019 9:47 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo
>>> Ersek; Gao, Liming;
 Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao,
>>> Jiewen
> Subject: [PATCH V2 7/9] MdeModulePkg/Variable: Add
>>> RT GetVariable()
> cache support
>
>
>>>

Re: [edk2-devel] [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support

2019-10-04 Thread Laszlo Ersek 
On 10/03/19 23:53, Kubacki, Michael A wrote:
> #1 - The plan is to remove the polling entirely in V3.
> 
> #2 - I'd prefer to take a definitive direction and reduce validation and 
> maintenance
> effort but you and Laszlo both requested this so I'll add a 
> FeaturePCD to control
> activation of the runtime cache in this patch series. Perhaps this 
> can be removed
> in the future.

Thanks!

(I'm also happy with the lock / timeout resolution. I had known about
the reentrancy restriction in the UEFI spec (I happened to look at
something in the kernel just the other day that reminded me of that part
of the spec), but it wasn't clear to me that the lock + timeout in the
patch series were intended to protect against just that. Kudos to Andrew
for the comment!)

(

Meanwhile, I've received help from my colleagues wrt.
QueryVariableInfo(), but right now it's too early to tell if we'll be
able to settle on that in the long term:

[PATCH] efi/efi_test: require CAP_SYS_ADMIN to open the chardev
http://mid.mail-archive.com/20191003100712.31045-1-javierm@redhat.com

)

For the next version of this edk2 patch set (where you plan to include
the new FeaturePCD, if I understand correctly), I'd like to request the
following: either set the DEC default to FALSE please, or please include
a patch for OvmfPkg where you set the PCD to FALSE in all the OvmfPkg
DSC files.

I think the next stable release should be made like that. Then, for the
stable release following that, we can re-evaluate the question, and
might decide to invert the PCD in OVMF (enabling the feature), assuming
QueryVariableInfo() proves usable in Fedora, by then.


Two independent questions:

- Has this work been regression-tested on ARM / AARCH64? (For example,
ArmVirtPkg platforms use the unified runtime DXE driver, not the split
runtime/SMM drivers. So no change in behavior is expected; we should
test that.)

In the "Testing Performed" section of your blurb, item#3 suggests
something similar ("Boot from S5 to EFI shell with DXE variables
enabled"), but I figured I'd raise AARCH64 specifically.


- Can you please confirm that the handling of *volatile* variables is
not affected? ArmVirtPkg and OvmfPkg use quite different sizes for
volatile and non-volatile variables; see:

  - 9c7d0d499296 ("OvmfPkg/TlsAuthConfigLib: configure trusted CA certs
  for HTTPS boot", 2018-03-30)
  - ffe048a0807b ("ArmVirtPkg: handle NETWORK_TLS_ENABLE in
  ArmVirtQemu*", 2019-06-28)

Thank you!
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48469): https://edk2.groups.io/g/devel/message/48469
Mute This Topic: https://groups.io/mt/34318592/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-