Date   
Re: UefiPayloadPkg: assert error in PciHostBridgeDxe

Andrew Fish
 



On Jul 3, 2020, at 11:06 AM, Laszlo Ersek <lersek@...> wrote:

On 07/03/20 01:03, Andrew Fish via groups.io wrote:


On Jul 2, 2020, at 3:54 PM, King Sumo <kingsumos@...> wrote:

Hi,

When booting UefiPayloadPkg in my system (x86 Denverton SoC, coreboot) an assert error is generated in the PciHostBridgeDxe driver.
In the InitializePciHostBridge() function if all ASSERT's are ignored (by commenting out the code) the boot can move further on until it reaches the UEFI Shell.
Any clues?

Loading driver at 0x0007EE61000 EntryPoint=0x0007EE6CF52 PciHostBridgeDxe.efi
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7EF1F698
ProtectUefiImageCommon - 0x7EF1F140
 - 0x000000007EE61000 - 0x0000000000013000
SetUefiImageMemoryAttributes - 0x000000007EE61000 - 0x0000000000001000 (0x0000000000004008)
SetUefiImageMemoryAttributes - 0x000000007EE62000 - 0x0000000000010000 (0x0000000000020008)
SetUefiImageMemoryAttributes - 0x000000007EE72000 - 0x0000000000002000 (0x0000000000004008)
PROGRESS CODE: V03040002 I0
InitRootBridge: populated root bus 0, with room for 7 subordinate bus(es)
RootBridge: PciRoot(0x0)
 Support/Attr: 10003 / 10003
   DmaAbove4G: No
NoExtConfSpace: No
    AllocAttr: 0 ()
          Bus: 0 - 7 Translation=0
           Io: 0 - 4FFF Translation=0
          Mem: D4000000 - FE0FFFFF Translation=0
   MemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0
         PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
  PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0
PciHostBridgeDxe: IntersectMemoryDescriptor: desc [E0000000, F0000000) type 1 cap 870000000002600F conflicts with aperture [D4000000, FE100000) cap 1


It looks like the above request for "D4000000 - FE0FFFFF” overlaps with an existing mapping (E0000000, F0000000). The edk2 has something called GCD (Global Coherency Domain) that is kind of like malloc for MMIO space, and what part of the CPU address has DRAM. So it looks like 2 device think they own (E0000000, F0000000), and there can be only one. You may be getting lucky that your hardware works, but something seems miss configured. 

PciHostBridgeDxe calls the PciHostBridgeGetRootBridges() function from
the platform's "PciHostBridgeLib" instance. The function outputs an
array of PCI_ROOT_BRIDGE structures that describe the PCI root bridges
on the system, including the various resource apertures for each bridge
(expressed as device address ranges).

Then PciHostBridgeDxe tries to allocate the described apertures from the
according resource type GCD spaces (after translating the address ranges
in question from device addresses to host addresses).

The attempt to allocate any one such aperture consists of three steps,
(1) make sure the GCD memory map covers the aperture with compatible
descriptor(s), (2) set the aperture range to uncacheable (for MMIO only;
not defined for IO), (3) actually allocate the aperture.

In step (1), there's a slight trick. We don't call gDS->AddIoSpace() /
gDS->AddMemorySpace() indiscriminately, because the platform may have
populated the GCD memory space / IO space maps before, such that there
could be a (partial or complete) overlap with the aperture being added.

Therefore the internal AddIoSpace() and AddMemoryMappedIoSpace()
functions are idempotent. They contrast every existent GCD space
descriptor with the aperture being added (including gaps, that is,
"nonexistent" type descriptors). For the case when both ranges are fully
distinct, nothing is done. If there is a partial or full overlap, and
the descriptor is "nonexistent", then the intersection is added with
gDS->AddIoSpace() / gDS->AddMemorySpace(). If there is a partial or full
overlap, and the descriptor type is *not* "nonexistent", then the
intersection's *compatibility* is checked, against the requested type
and capabilities of the aperture.

The above procedure ensures that, for any given resource aperture, the
GCD IO or memory space map covers the aperture, with nonzero (that is,
possibly more than one!) adjacent descriptor entries, such that each
descriptor intersecting with the aperture has compatible type and
capabilities with the aperture.

In turn, the error message seen above reports a platform misconfiguration.

It reports that the GCD memory space map already has an entry (a
descriptor) at [E0000000, F0000000), with type 1 (that is,
"EfiGcdMemoryTypeReserved" -- see "MdePkg/Include/Pi/PiDxeCis.h"). The
capabilities of this range are 0x8700_0000_0002_600F

At the same time, the platform's PciHostBridgeLib instance reported a
root bridge with an MMIO aperture at [D4000000, FE100000), with
capabilities 1.

This is a conflict. The capabilities don't even matter (we don't even
check whether the existent GCD descriptor's capabilities are a superset
of the aperture's), because the aperture requires GCD memory type
EfiGcdMemoryTypeMemoryMappedIo, but the GCD descriptor has type
EfiGcdMemoryTypeReserved.

In brief, the failure is due to the platform reporting a PCI root bridge
aperture such that it overlaps an area that is already listed as
"reserved" in the GCD memory space map. So this is a platform bug;
either in the "PciHostBridgeLib" instance, or in the module that
populates the GCD memory space map.


Laszlo,

Thanks for the detailed response. 

Sumo,

In your platforms DSC file you can or in 0x00100000 by hand into gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel to turn on DEBUG_GCD and get more DEBUG prints about GCD configuration. That may help you track down what is happening. 

Thanks,

Andrew Fish

Thanks
Laszlo



Thanks,

Andrew Fish

ASSERT_EFI_ERROR (Status = Invalid Parameter)
ASSERT [PciHostBridgeDxe] /home/lxuser/occ/edk2/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c(488): !EFI_ERROR (Status)

Thanks,
Sumo





Re: UefiPayloadPkg: assert error in PciHostBridgeDxe

Laszlo Ersek
 

On 07/03/20 01:03, Andrew Fish via groups.io wrote:


On Jul 2, 2020, at 3:54 PM, King Sumo <kingsumos@...> wrote:

Hi,

When booting UefiPayloadPkg in my system (x86 Denverton SoC, coreboot) an assert error is generated in the PciHostBridgeDxe driver.
In the InitializePciHostBridge() function if all ASSERT's are ignored (by commenting out the code) the boot can move further on until it reaches the UEFI Shell.
Any clues?

Loading driver at 0x0007EE61000 EntryPoint=0x0007EE6CF52 PciHostBridgeDxe.efi
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7EF1F698
ProtectUefiImageCommon - 0x7EF1F140
- 0x000000007EE61000 - 0x0000000000013000
SetUefiImageMemoryAttributes - 0x000000007EE61000 - 0x0000000000001000 (0x0000000000004008)
SetUefiImageMemoryAttributes - 0x000000007EE62000 - 0x0000000000010000 (0x0000000000020008)
SetUefiImageMemoryAttributes - 0x000000007EE72000 - 0x0000000000002000 (0x0000000000004008)
PROGRESS CODE: V03040002 I0
InitRootBridge: populated root bus 0, with room for 7 subordinate bus(es)
RootBridge: PciRoot(0x0)
Support/Attr: 10003 / 10003
DmaAbove4G: No
NoExtConfSpace: No
AllocAttr: 0 ()
Bus: 0 - 7 Translation=0
Io: 0 - 4FFF Translation=0
Mem: D4000000 - FE0FFFFF Translation=0
MemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0
PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0
PciHostBridgeDxe: IntersectMemoryDescriptor: desc [E0000000, F0000000) type 1 cap 870000000002600F conflicts with aperture [D4000000, FE100000) cap 1
It looks like the above request for "D4000000 - FE0FFFFF” overlaps with an existing mapping (E0000000, F0000000). The edk2 has something called GCD (Global Coherency Domain) that is kind of like malloc for MMIO space, and what part of the CPU address has DRAM. So it looks like 2 device think they own (E0000000, F0000000), and there can be only one. You may be getting lucky that your hardware works, but something seems miss configured.
PciHostBridgeDxe calls the PciHostBridgeGetRootBridges() function from
the platform's "PciHostBridgeLib" instance. The function outputs an
array of PCI_ROOT_BRIDGE structures that describe the PCI root bridges
on the system, including the various resource apertures for each bridge
(expressed as device address ranges).

Then PciHostBridgeDxe tries to allocate the described apertures from the
according resource type GCD spaces (after translating the address ranges
in question from device addresses to host addresses).

The attempt to allocate any one such aperture consists of three steps,
(1) make sure the GCD memory map covers the aperture with compatible
descriptor(s), (2) set the aperture range to uncacheable (for MMIO only;
not defined for IO), (3) actually allocate the aperture.

In step (1), there's a slight trick. We don't call gDS->AddIoSpace() /
gDS->AddMemorySpace() indiscriminately, because the platform may have
populated the GCD memory space / IO space maps before, such that there
could be a (partial or complete) overlap with the aperture being added.

Therefore the internal AddIoSpace() and AddMemoryMappedIoSpace()
functions are idempotent. They contrast every existent GCD space
descriptor with the aperture being added (including gaps, that is,
"nonexistent" type descriptors). For the case when both ranges are fully
distinct, nothing is done. If there is a partial or full overlap, and
the descriptor is "nonexistent", then the intersection is added with
gDS->AddIoSpace() / gDS->AddMemorySpace(). If there is a partial or full
overlap, and the descriptor type is *not* "nonexistent", then the
intersection's *compatibility* is checked, against the requested type
and capabilities of the aperture.

The above procedure ensures that, for any given resource aperture, the
GCD IO or memory space map covers the aperture, with nonzero (that is,
possibly more than one!) adjacent descriptor entries, such that each
descriptor intersecting with the aperture has compatible type and
capabilities with the aperture.

In turn, the error message seen above reports a platform misconfiguration.

It reports that the GCD memory space map already has an entry (a
descriptor) at [E0000000, F0000000), with type 1 (that is,
"EfiGcdMemoryTypeReserved" -- see "MdePkg/Include/Pi/PiDxeCis.h"). The
capabilities of this range are 0x8700_0000_0002_600F

At the same time, the platform's PciHostBridgeLib instance reported a
root bridge with an MMIO aperture at [D4000000, FE100000), with
capabilities 1.

This is a conflict. The capabilities don't even matter (we don't even
check whether the existent GCD descriptor's capabilities are a superset
of the aperture's), because the aperture requires GCD memory type
EfiGcdMemoryTypeMemoryMappedIo, but the GCD descriptor has type
EfiGcdMemoryTypeReserved.

In brief, the failure is due to the platform reporting a PCI root bridge
aperture such that it overlaps an area that is already listed as
"reserved" in the GCD memory space map. So this is a platform bug;
either in the "PciHostBridgeLib" instance, or in the module that
populates the GCD memory space map.

Thanks
Laszlo



Thanks,

Andrew Fish

ASSERT_EFI_ERROR (Status = Invalid Parameter)
ASSERT [PciHostBridgeDxe] /home/lxuser/occ/edk2/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c(488): !EFI_ERROR (Status)

Thanks,
Sumo




2nd OVMF question about why InitRootBridge does not set ResourceAssigned?

Andrew Fish
 

When tacking down the EFI_MEMORY_UC issue I noticed that InitRootBridg() [1] does not set RootBus-> ResourceAssigned = TRUE. If it was TRUE then the entire PCI aptitude would end up in the GCD map. Given it is not set (set by ZeroMem) the EFI_MEMORY_UC only ends up for the actual allocations as far as I can tell. I’m wondering why it was done this way?

I did notice if I set RootBus-> ResourceAssigned to TRUE the serial console did not come up and I did not get a chance to debug that? Maybe there was a resource conflict with the ISA bus driver or some such?

[1] https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c#L112

Thanks,

Andrew Fish

Question about the OVMF and MTRRs?

Andrew Fish
 

I noticed for my version of QEMU that MTRR registers are not supported. Is that expected? Or is it something if you get wrong it does not break in QEMU?

Due to no MTRR support the CPU Protocol SetMemoeryAttributes() call is falling [1] and non of the GCD ranges have EFI_MEMORY_UC Attribute. 

I’m not sure but I think the no MTRR case should attempt to set the attributes via page tables via:

return AssignMemoryPageAttributes (NULL, BaseAddress, Length, MemoryAttributes, NULL);

Vs 

return EFI_UNSUPPORTED;


Thanks,

Andrew Fish

Re: [PATCH 2/2] ReadMe.rst: delete statements about other accepted licenses

Laszlo Ersek
 

On 07/02/20 13:24, Leif Lindholm wrote:
In the past, the TianoCore Contribution Agreement provided equal to what
the (+patent) gives us, but also for all the alternative licenses listed
under "Code Contributions". When the contribution agreement was dropped,
no conversation was had about this aspect.
Please see my wording questions under the blurb.


Until this issue is resolved, only code licensed under edk2+patent should
be accepted into the project.

This addresses, but does not resolve,
https://bugzilla.tianocore.org/show_bug.cgi?id=2834

Cc: Andrew Fish <afish@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Signed-off-by: Leif Lindholm <leif@...>
---
ReadMe.rst | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/ReadMe.rst b/ReadMe.rst
index d597e34efc22..79f07240039e 100644
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@ -111,23 +111,14 @@ To make a contribution to a TianoCore project, follow these steps.
that the project documents on its web page. If the process is
not documented, then submit the code on development email list
for the project.
-#. It is preferred that contributions are submitted using the same
- copyright license as the base project. When that is not possible,
- then contributions using the following licenses can be accepted:
-
-- BSD (2-clause): http://opensource.org/licenses/BSD-2-Clause
-- BSD (3-clause): http://opensource.org/licenses/BSD-3-Clause
-- MIT: http://opensource.org/licenses/MIT
-- Python-2.0: http://opensource.org/licenses/Python-2.0
-- Zlib: http://opensource.org/licenses/Zlib
+#. Contributions must be submitted using the same copyright license
+ as the base project.

For documentation:

- FreeBSD Documentation License
https://www.freebsd.org/copyright/freebsd-doc-license.html

-Contributions of code put into the public domain can also be accepted.
-
Contributions using other licenses might be accepted, but further
review will be required.

*IF* the problem you describe is real (honestly I've never *understood*
the problem statement before), then I think this change reflects that.
So here's a conditional:

Acked-by: Laszlo Ersek <lersek@...>

I defer to Mike on this though, as Mike drove
<https://bugzilla.tianocore.org/show_bug.cgi?id=1373>.

Thanks,
Laszlo

Re: [PATCH 1/2] Revert "BaseTools/PatchCheck.py: Add LicenseCheck"

Laszlo Ersek
 

On 07/02/20 13:24, Leif Lindholm wrote:
This reverts commit a4cfb842fca9693a330cb5435284c1ee8bfbbace.
This commit suggests inclusion of non-edk2+license content without
The expression "non-edk2+license" is a typo.

I think you meant "non-BSD-2-Clause-Patent".

a contribution agreement is something the community has made a
decision on, which is incorrect.
I'm OK with commit a4cfb842fca9 being reverted, as this solves the
practical problem of adding generated files.

Acked-by: Laszlo Ersek <lersek@...>

That said, I *still* don't grasp the more general problem you refer to.

Can we express it as follows (using commits 6b621f958123 and
6f21d772aa6b as reference):

(1) Content under OvmfPkg/Include/IndustryStandard/Xen was introduced
under the MIT license in commit 6b621f958123. That commit was marked with:

License: This patch adds many files under the MIT licence.
Contributed-under: TianoCore Contribution Agreement 1.0
and the header files also contained open-coded instances of the MIT
license. (These would later be replaced with SPDX identifiers in commit
6f21d772aa6b.)

As a result, these files effectively granted use and distribution rights
under the MIT license, *plus* a patent grant (per TCA).

(2) If we did the same today (that is, add new MIT-licensed files, but
no "Contributed-under: TCA" line on the commit message), then that would
grant use and distribution rights under the "MIT license", and *no*
patent grant.

Is this the issue you're thinking of?


So are we basically looking to replace (for example):

SPDX-License-Identifier: MIT

with *something* like:

SPDX-License-Identifier: MIT-Patent

? (Assuming the latter exists -- which it doesn't, at the moment?)

Thanks
Laszlo


Cc: Shenglei Zhang <shenglei.zhang@...>
Cc: Yuwei Chen <yuwei.chen@...>
Cc: Liming Gao <liming.gao@...>
Signed-off-by: Leif Lindholm <leif@...>
---
BaseTools/Scripts/PatchCheck.py | 50 ---------------------------------
1 file changed, 50 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index e38cf61f93da..6372f71592d3 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -305,49 +305,12 @@ class GitDiffCheck:
self.line_num = 0
self.state = START
self.new_bin = []
- self.LicenseCheck(self.lines, self.count)
while self.line_num < self.count and self.format_ok:
line_num = self.line_num
self.run()
assert(self.line_num > line_num)
self.report_message_result()

- def LicenseCheck(self, lines, count):
- self.ok = True
- self.startcheck = False
- self.license = True
- line_index = 0
- for line in lines:
- if line.startswith('--- /dev/null'):
- nextline = lines[line_index + 1]
- added_file = self.Readdedfileformat.search(nextline).group(1)
- added_file_extension = os.path.splitext(added_file)[1]
- if added_file_extension in self.file_extension_list:
- self.startcheck = True
- self.license = False
- if self.startcheck and self.license_format_preflix in line:
- if self.bsd2_patent in line or self.bsd3_patent in line:
- self.license = True
- else:
- for optional_license in self.license_optional_list:
- if optional_license in line:
- self.license = True
- self.warning(added_file)
- if line_index + 1 == count or lines[line_index + 1].startswith('diff --') and self.startcheck:
- if not self.license:
- error_message = "Invalid License in: " + added_file
- self.error(error_message)
- self.startcheck = False
- self.license = True
- line_index = line_index + 1
-
- def warning(self, *err):
- count = 0
- for line in err:
- warning_format = 'Warning: License accepted but not BSD plus patent license in'
- print(warning_format, line)
- count += 1
-
def report_message_result(self):
if Verbose.level < Verbose.NORMAL:
return
@@ -534,19 +497,6 @@ class GitDiffCheck:
print(prefix, line)
count += 1

- license_format_preflix = 'SPDX-License-Identifier'
-
- bsd2_patent = 'BSD-2-Clause-Patent'
-
- bsd3_patent = 'BSD-3-Clause-Patent'
-
- license_optional_list = ['BSD-2-Clause', 'BSD-3-Clause', 'MIT', 'Python-2.0', 'Zlib']
-
- Readdedfileformat = re.compile(r'\+\+\+ b\/(.*)\n')
-
- file_extension_list = [".c", ".h", ".inf", ".dsc", ".dec", ".py", ".bat", ".sh", ".uni", ".yaml", ".fdf", ".inc", "yml", ".asm", \
- ".asm16", ".asl", ".vfr", ".s", ".S", ".aslc", ".nasm", ".nasmb", ".idf", ".Vfr", ".H"]
-
class CheckOnePatch:
"""Checks the contents of a git email formatted patch.

Re: [PATCH 0/2] Drop suggestions of alternative acceptable licenses

Laszlo Ersek
 

On 07/02/20 13:24, Leif Lindholm wrote:
(Echoing https://bugzilla.tianocore.org/show_bug.cgi?id=2834:)
In the past, the TianoCore Contribution Agreement provided equal to what
the (+patent) gives us, but also for all the alternative licenses listed
under "Code Contributions". When the contribution agreement was dropped,
no conversation was had about this aspect.
Some crucial words seem to be missing from the above paragraph:

- "the TianoCore Contribution Agreement provided [???] equal to" -- do
you mean "rights"?

- "what the [???] (+patent) gives us" -- do you mean "2-clause BSDL"?

Thanks
Laszlo


Until this issue is resolved, only code licensed under edk2+patent should
be accepted into the project.

It is not obvious whether modifications to non-bsd+patent code that *was*
contributed under the TCA can still be accepted.

This set deletes the current version of license checking in PatchCheck.py
that was added in a4cfb842fca9. It also deletes all references to other
acceptable licenses (and public domain).

This gives us a clean slate to make informed decisions on what is and is
not acceptable for this project.

The statement that "Contributions using other licenses might be accepted,
but further review will be required." is intentionally retained.

Leif Lindholm (2):
Revert "BaseTools/PatchCheck.py: Add LicenseCheck"
ReadMe.rst: delete statements about other accepted licenses

Cc: Shenglei Zhang <shenglei.zhang@...>
Cc: Yuwei Chen <yuwei.chen@...>
Cc: Liming Gao <liming.gao@...>

BaseTools/Scripts/PatchCheck.py | 50 ---------------------------------
ReadMe.rst | 13 ++-------
2 files changed, 2 insertions(+), 61 deletions(-)

Re: [PATCH v6 00/16] Add a plugin to check Ecc issues for edk2 on open ci

Laszlo Ersek
 

Hi Liming,

On 07/03/20 17:13, Gao, Liming wrote:
Include more people and collect the comments.

ECC is the source file coding style checker. Here is its wiki page https://github.com/tianocore/tianocore.github.io/wiki/ECC-tool.
If the changed code doesn't follow edk2 coding style, ECC will report the error.

This patch set enables ECC checker in open CI for each patch coming into edk2 repo. That means new changes need to follow edk2 coding style.
Otherwise, new changes can't be merged.

If you have some comments for ECC checker in open CI, please reply this mail.

(1) The ArmVirtPkg (v6 04/16) and OvmfPkg (v6 11/16) patches already
carry my ACKs; from here:

https://edk2.groups.io/g/devel/message/61154
https://edk2.groups.io/g/devel/message/61155


(2) The UefiCpuPkg patch (v6 15/16) *should* also carry my ACK, from here:

https://edk2.groups.io/g/devel/message/61156

Shenglei picked up my ACK for v4 and v5:

https://edk2.groups.io/g/devel/message/61283
https://edk2.groups.io/g/devel/message/61852

but then dropped it for v6:

https://edk2.groups.io/g/devel/message/61894

Shenglei, can you please explain why you dropped my ACK from the
UefiCpuPkg patch, in v6?


(3) The initial discussion between Shenglei and myself are under the v2
posting:

https://edk2.groups.io/g/devel/message/60665
https://edk2.groups.io/g/devel/message/60711
https://edk2.groups.io/g/devel/message/60961

I'm happy with this work because it lets package maintainers tailor ECC
as they see appropriate.

Thanks,
Laszlo

Re: OvmfPkg: Adding support for bhyve as OvmfPkg/Bhyve

Laszlo Ersek
 

On 07/03/20 05:13, Rebecca Cran wrote:
On 7/2/20 3:27 AM, Laszlo Ersek wrote:

I'll look at the patches on the list. (I don't believe in reviewing
before reviewing. In case I need to make comments for what I find in
your repo, I could only make them without any context to quote.)
Thanks, I'll wait for the changes to PatchCheck.py to land, then send
the bhyve patch to the list. I think at this point I've updated all the
licenses to be BSD+Patent with the exception of the generated file.
Sounds great!
Laszlo

Re: License Check - was OvmfPkg: Adding support for bhyve as OvmfPkg/Bhyve

Laszlo Ersek
 

On 07/03/20 03:40, Gao, Liming wrote:

Besides, there is another new checker of ECC to check coding style for each patch. Can you give your comment?
https://edk2.groups.io/g/devel/message/61966
I've seen that feature.

Importantly, it gives package maintainers package-level exception lists.
So if ECC rejects something, and the package maintainers disagree with
the rejection, they can create an exception inside the package, and
suppress the ECC report.

We don't have the same option with PatchCheck.

(For generated files anyway, that's what I'm suggesting in fact: if
PatchCheck sees the string "@file: generated" in a source file, it
should omit the license check altogether, for that file.)

Thanks!
Laszlo

Re: [PATCH v6 00/16] Add a plugin to check Ecc issues for edk2 on open ci

Liming Gao
 

Include more people and collect the comments.

ECC is the source file coding style checker. Here is its wiki page https://github.com/tianocore/tianocore.github.io/wiki/ECC-tool.
If the changed code doesn't follow edk2 coding style, ECC will report the error.

This patch set enables ECC checker in open CI for each patch coming into edk2 repo. That means new changes need to follow edk2 coding style.
Otherwise, new changes can't be merged.

If you have some comments for ECC checker in open CI, please reply this mail.

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Zhang, Shenglei
Sent: Wednesday, July 1, 2020 9:55 AM
To: devel@edk2.groups.io
Cc: Feng, Bob C <bob.c.feng@...>; Bret Barkelew <Bret.Barkelew@...>; Kinney, Michael D
<michael.d.kinney@...>; Gao, Liming <liming.gao@...>; Sean Brogan <sean.brogan@...>
Subject: [edk2-devel] [PATCH v6 00/16] Add a plugin to check Ecc issues for edk2 on open ci

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2606
As planed we will enable Ecc check for edk2 on open ci. And they are
ready now. I appreciate receiving feedback and comments if someone
find errors or false positive issues.

I created a pipline of EccCheck for my forked edk2. Welcome everyone to
create pull request to test the quality of this plugin.
My forked tree: https://github.com/shenglei10/edk2

And I also created some test cases for ECC plugin. Below are test cases.
https://github.com/shenglei10/edk2/tree/ECC
Results can be view in below azure server.
https://dev.azure.com/shengleizhang/shengleizhang/_build?definitionId=12&_a=summary

Patches
1/16: It's a lib necessary for py3 to run Ecc on azure servers.

2/16: EccCheck.py is a plugin to report Ecc issues for commits. It can be run
on azure servers for open ci, or a local virtual environment.

3/16~16/16: We consider some cases that will report out Ecc issues but they won't
be fixed, like submodule and industry standard related things. So we
add two configuration fields "Exception" and "IgnoreFiles" for people
to use. These patches add configuration in yaml files for Ecc check.

Cc: Bob Feng <bob.c.feng@...>
Cc: Bret Barkelew <Bret.Barkelew@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <liming.gao@...>
Cc: Sean Brogan <sean.brogan@...>

v2: Update 1/17, fix the bug that the script can't hanlde multiple commits.

v3: Update 1/17, set the only workalbe workspace is edk2 root directory.
Update 2/17, designate the version of antlr4 is 4.7.1.
Add 4/17~17/17.

v4. Update 1/17, remove the function EdksetupRebuild(), instead add
function SetupEnvironment(). Update variables' format and type hints
to pass flake8 and mypy.

v5. Conver the former method to plugin solution, to align with
other check points on open ci.

v6. The 1/16 patch is missed in v5 series. Now add it in v6.

Shenglei Zhang (16):
pip-requirements.txt: Add Ecc required lib
.pytool/Plugin: Add a plugin EccCheck
MdeModulePkg/MdeModulePkg.ci.yaml: Add configuration for Ecc check
ArmVirtPkg/ArmVirtPkg.ci.yaml: Add configuration for Ecc check
CryptoPkg/CryptoPkg.ci.yaml: Add configuration for Ecc check
EmulatorPkg/EmulatorPkg.ci.yaml: Add configuration for Ecc check
FatPkg/FatPkg.ci.yaml: Add configuration for Ecc check
FmpDevicePkg/FmpDevicePkg.ci.yaml: Add configuration for Ecc check
MdePkg/MdePkg.ci.yaml: Add configuration for Ecc check
NetworkPkg/NetworkPkg.ci.yaml: Add configuration for Ecc check
OvmfPkg/OvmfPkg.ci.yaml: Add configuration for Ecc check
PcAtChipsetPkg/PcAtChipsetPkg.ci.yaml: Add configuration for Ecc check
SecurityPkg/SecurityPkg.ci.yaml: Add configuration for Ecc check
ShellPkg/ShellPkg.ci.yaml: Add configuration for Ecc check
UefiCpuPkg/UefiCpuPkg.ci.yaml: Add configuration for Ecc check
UnitTestFrameworkPkg: Add configuration for Ecc check in yaml file

.pytool/Plugin/EccCheck/EccCheck.py | 268 ++++++++++++++++++
.pytool/Plugin/EccCheck/EccCheck_plug_in.yaml | 11 +
.pytool/Plugin/EccCheck/Readme.md | 15 +
ArmVirtPkg/ArmVirtPkg.ci.yaml | 11 +
CryptoPkg/CryptoPkg.ci.yaml | 11 +
EmulatorPkg/EmulatorPkg.ci.yaml | 11 +
FatPkg/FatPkg.ci.yaml | 11 +
FmpDevicePkg/FmpDevicePkg.ci.yaml | 11 +
MdeModulePkg/MdeModulePkg.ci.yaml | 11 +
MdePkg/MdePkg.ci.yaml | 11 +
NetworkPkg/NetworkPkg.ci.yaml | 11 +
OvmfPkg/OvmfPkg.ci.yaml | 11 +
PcAtChipsetPkg/PcAtChipsetPkg.ci.yaml | 11 +
SecurityPkg/SecurityPkg.ci.yaml | 11 +
ShellPkg/ShellPkg.ci.yaml | 11 +
UefiCpuPkg/UefiCpuPkg.ci.yaml | 11 +
.../UnitTestFrameworkPkg.ci.yaml | 10 +
pip-requirements.txt | 1 +
18 files changed, 448 insertions(+)
create mode 100644 .pytool/Plugin/EccCheck/EccCheck.py
create mode 100644 .pytool/Plugin/EccCheck/EccCheck_plug_in.yaml
create mode 100644 .pytool/Plugin/EccCheck/Readme.md

--
2.18.0.windows.1


Re: License Check - was OvmfPkg: Adding support for bhyve as OvmfPkg/Bhyve

Liming Gao
 

-----Original Message-----
From: Leif Lindholm <leif@...>
Sent: Friday, July 3, 2020 6:38 PM
To: Gao, Liming <liming.gao@...>
Cc: devel@edk2.groups.io; ard.biesheuvel@...; Laszlo Ersek <lersek@...>; Rebecca Cran <@bcran>;
Andrew Fish <afish@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-devel] License Check - was OvmfPkg: Adding support for bhyve as OvmfPkg/Bhyve

On Fri, Jul 03, 2020 at 01:40:26 +0000, Gao, Liming wrote:
*Reads patch*
*Figuratively spits coffee all over keyboard*

No, this is not OK.

We *STILL* have no agreed process for accepting non bsd+patent content
since we dropped the contribution agreement. I have tried to raise
this issue several times in the past, and there has never been any
outcome from resulting discussions.

So now I'm going to send out a two-patch set consisting of:
- Reverting a4cfb842fca9. (Doing nothing is better than implying that
anything !bsd+patent can currently be added to the tree.)
- Deleting the statement in ReadmMe.rst erroneously claiming that the
includion of these other licenses are acceptable until such a point
an active decision has been taken, approved by the community, that
this is permitted.
If only bsd+patent is allowed, the checker can be enhanced to check this license only.
I don't understand why remove this checker.
Mainly because that was the easiest thing to do :)
People may miss it. So, the checker is helpful to detect the issue.
The feature is useful, but enabling it by default is not the correct
decision for all TianoCore repos, and the situation for non-bsd+patent
contributions is less than ideal.
It can be added in open CI for Edk2 project now.



But also because:
- The thread that spawned this also raised the problem of
machine-generated files.
This is a gap. We have no rule for the generated file.
Laszlo gives one proposal to use the specific tag in file header if this file is auto generated.
Then, the license is not required.


- I am somewhat unhappy the checker got merged in the first place
without wider community feedback. BaseTools and its contents are
used for many repositories (even within TianoCore), and this added
unconditional check breaks the use for some of those.
The patch to add the license checker is reviewed in edk2 mail list
for several weeks.
I don't get other comments. Can you give the suggestion on how to
improve the communication in edk2 community?
I think that for something as fundamental as this, we need to actively
chase feedback. I know that I will never manage to always read all
emails to the lists, so there is always a risk I will miss something
I'm not cc:d on.
For something with as big an impact as a tightening of requirements in
PatchCheck.py, if sufficient feedback (like at least 2-3 maintainers
outside of BaseTools) has not been received, then it would make sense
to ping *all* maintainers, alternatively ping the stewards and ask us
to go gather feedback.
Good suggestion to include more maintainers and stewards.


Besides, there is another new checker of ECC to check coding style
for each patch. Can you give your comment?
https://edk2.groups.io/g/devel/message/61966
I have never managed to get ECC running in any of my setups.
Perhaps I should start trying to track down why, or at least raise a
bugzilla for someone else to investigate.
For ECC checker, I will include more people in ECC checker mail list.

Thanks
Liming
Regards,

Leif

I think the fundamental problem is that contributing code under a
contribution agreement that includes a patent grant is not the same as
contributing it under a patent grant license, given that the latter can
only be done by the author of the code, while the former could be done
by anyone.

This means our current licensing policy is actually more restrictive
that the old one, making it more difficult to incorporate 'second hand'
code.

I don't think we can fix this with a patch though :-(
Yes. This checker is for current allowed license. It doesn't resolve this issue.

Thanks
Liming

Re: [PATCH] MdeModulePkg: Upon BootOption failure, Destroy RamDisk memory before RSC.

Liming Gao
 

BZ to enhance PatchCheck is submitted. https://bugzilla.tianocore.org/show_bug.cgi?id=2836

Thanks
Liming

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm
Sent: Friday, July 3, 2020 6:43 PM
To: Gao, Liming <liming.gao@...>
Cc: devel@edk2.groups.io; Veliyathuparambil Prakashan, KrishnadasX <krishnadasx.veliyathuparambil.prakashan@...>; Laszlo
Ersek <lersek@...>; afish@...; Kinney, Michael D <michael.d.kinney@...>; Gao, Zhichao
<zhichao.gao@...>; Ni, Ray <ray.ni@...>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg: Upon BootOption failure, Destroy RamDisk memory before RSC.

On Fri, Jul 03, 2020 at 06:02:12 +0000, Gao, Liming wrote:
Signed-off-by line is too long and exceeds 80 characters requirement. But, it is valid.

So, I suggest to enhance PatchCheck.py and skip the check for the lines with Signed-off-by, Ack-by:, Reviewed-by:, and Tested-By:.
Acked-by, not Ack-by, but yes, I completely agree. Restricting the
lenght of these tag lines is not correct.
We may want to consider adding "Cc:" to the list also.

Regards,

Leif

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of KrishnadasX Veliyathuparambil Prakashan
Sent: Friday, June 19, 2020 10:40 AM
To: devel@edk2.groups.io
Cc: Gao, Zhichao <zhichao.gao@...>; Ni, Ray <ray.ni@...>
Subject: [edk2-devel] [PATCH] MdeModulePkg: Upon BootOption failure, Destroy RamDisk memory before RSC.

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

For better memory management, re-ordered the DestroyRamDisk and
ReportStatusCode calls inside the EfiBootManagerBoot() function.

This will help to clean the unused memory before reporting the
failure status, so that OEMs can use RSC Listener to launch
custom boot option or application for recovering the failed
hard drive.

This change will help to ensure that the allocated pool of memory
for the failed boot option is freed before executing OEM's RSC
listener callback to handle every boot option failure.

Signed-off-by: KrishnadasX Veliyathuparambil Prakashan <krishnadasx.veliyathuparambil.prakashan@...>
Cc: "Gao, Zhichao" <zhichao.gao@...>
Cc: "Ni, Ray" <ray.ni@...>
---
.../Library/UefiBootManagerLib/BmBoot.c | 28 ++++++++++---------
1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 540d169ec1..aff620ad52 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -2,7 +2,7 @@
Library functions which relates with booting.



Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.

-Copyright (c) 2011 - 2019, Intel Corporation. All rights reserved.<BR>

+Copyright (c) 2011 - 2020, Intel Corporation. All rights reserved.<BR>

(C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>

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



@@ -1903,17 +1903,17 @@ EfiBootManagerBoot (
gBS->UnloadImage (ImageHandle);

}

//

- // Report Status Code with the failure status to indicate that the failure to load boot option

- //

- BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);

- BootOption->Status = Status;

- //

// Destroy the RAM disk

//

if (RamDiskDevicePath != NULL) {

BmDestroyRamDisk (RamDiskDevicePath);

FreePool (RamDiskDevicePath);

}

+ //

+ // Report Status Code with the failure status to indicate that the failure to load boot option

+ //

+ BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);

+ BootOption->Status = Status;

return;

}

}

@@ -1982,13 +1982,6 @@ EfiBootManagerBoot (
Status = gBS->StartImage (ImageHandle, &BootOption->ExitDataSize, &BootOption->ExitData);

DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Image Return Status = %r\n", Status));

BootOption->Status = Status;

- if (EFI_ERROR (Status)) {

- //

- // Report Status Code with the failure status to indicate that boot failure

- //

- BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);

- }

- PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);



//

// Destroy the RAM disk

@@ -1998,6 +1991,15 @@ EfiBootManagerBoot (
FreePool (RamDiskDevicePath);

}



+ if (EFI_ERROR (Status)) {

+ //

+ // Report Status Code with the failure status to indicate that boot failure

+ //

+ BmReportLoadFailure (EFI_SW_DXE_BS_EC_BOOT_OPTION_FAILED, Status);

+ }

+ PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);

+

+

//

// Clear the Watchdog Timer after the image returns

//

--
2.27.0.windows.1


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

View/Reply Online (#61517): https://edk2.groups.io/g/devel/message/61517
Mute This Topic: https://groups.io/mt/74978785/1759384
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming.gao@...]
-=-=-=-=-=-=

Re: [PATCH edk2-platforms v2 0/7] NXP: Add PCIe Support for LX2160aRdbPkg

Leif Lindholm
 

On Mon, Jun 29, 2020 at 12:14:59 +0530, Wasim Khan wrote:
From: Wasim Khan <wasim.khan@...>

LX2160-Rev1 and LX2160-Rev2 has different PCIe controller.
This patch series adds PCIe support for LX2160aRdbPkg which includes
- Add PCIe space in VirtualMemoryMap
- Platform driver to check SoC version and sets PCDs for PCIe controller, which
are used by PciHostBridgeLib and PciSegmentLib.
PciHostBridgeLib and PciSegmentLib already has support for both PCIe controllers.
- Enable NetworkPkg for LX2160aRdbPkg Platform.


Changes in V2:
- Addressed review comments on V1
Thanks!
For the series:
Reviewed-by: Leif Lindholm <leif@...>
Pushed as b716f363e752..b9fba411178f.

V1 series can be referred here:
https://edk2.groups.io/g/devel/message/61062

Wasim Khan (7):
Silicon/NXP: LX2160A: Define PCIe related PCDs
Platform/NXP: LX2160aRdbPkg: Add PCIe space in VirtualMemoryMap
Platform/NXP: LX2160aRdbPkg: Add PlatformDxe driver
Platform/NXP: LX2160aRdbPkg: Enable PlatformDxe driver
Platform/NXP: LX2160aRdbPkg: Hide Root Port for LX2160A-Rev2
Platform/NXP: LX2160aRdbPkg: Enable NetworkPkg
Platform/NXP: LX2160aRdbPkg: Enable PCIE support

Silicon/NXP/NxpQoriqLs.dec | 1 +
Silicon/NXP/LX2160A/LX2160A.dsc.inc | 5 +
Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.dsc | 22 +++++
Platform/NXP/LX2160aRdbPkg/LX2160aRdbPkg.fdf | 15 +++
Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf | 36 ++++++++
Platform/NXP/LX2160aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf | 2 +
Silicon/NXP/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 1 +
Silicon/NXP/Include/Pcie.h | 1 +
Silicon/NXP/LX2160A/Include/Soc.h | 3 +
Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c | 97 ++++++++++++++++++++
Platform/NXP/LX2160aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c | 11 ++-
Silicon/NXP/Library/PciHostBridgeLib/PciHostBridgeLib.c | 6 +-
12 files changed, 198 insertions(+), 2 deletions(-)
create mode 100644 Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf
create mode 100644 Platform/NXP/LX2160aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c

--
2.7.4

Re: [PATCH v2 2/9] UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support (CVE-2019-11098)

Laszlo Ersek
 

On 07/03/20 15:57, Laszlo Ersek wrote:
On 07/02/20 07:15, Guomin Jiang wrote:
From: Michael Kubacki <michael.a.kubacki@...>

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

Moves the GDT and IDT to permanent memory in a memory discovered
callback. This is done to ensure the GDT and IDT authenticated in
pre-memory is not fetched from outside a verified location after
the permanent memory transition.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Signed-off-by: Michael Kubacki <michael.a.kubacki@...>
---
UefiCpuPkg/CpuMpPei/CpuMpPei.c | 40 ++++++++++++++++++-
UefiCpuPkg/CpuMpPei/CpuMpPei.h | 13 ++++++
UefiCpuPkg/CpuMpPei/CpuPaging.c | 14 +++++--
.../Ia32/ArchExceptionHandler.c | 4 +-
.../SecPeiCpuException.c | 2 +-
5 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index 07ccbe7c6a91..2d6f1bc98851 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -429,6 +429,44 @@ GetGdtr (
AsmReadGdtr ((IA32_DESCRIPTOR *)Buffer);
}

+/**
+ Migrates the Global Descriptor Table (GDT) to permanent memory.
+
+ @retval EFI_SUCCESS The GDT was migrated successfully.
+ @retval EFI_OUT_OF_RESOURCES The GDT could not be migrated due to lack of available memory.
+
+**/
+EFI_STATUS
+EFIAPI
+MigrateGdt (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ UINTN GdtBufferSize;
+ IA32_DESCRIPTOR Gdtr;
+ UINT8 *GdtBuffer;
+
+ AsmReadGdtr ((IA32_DESCRIPTOR *) &Gdtr);
+ GdtBufferSize = sizeof (IA32_TSS_DESCRIPTOR) + Gdtr.Limit + 1;
+
+ Status = PeiServicesAllocatePool (
+ GdtBufferSize,
+ (VOID **) &GdtBuffer
+ );
+ ASSERT (GdtBuffer != NULL);
+ if (EFI_ERROR (Status)) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ GdtBuffer = ALIGN_POINTER (GdtBuffer, sizeof (IA32_TSS_DESCRIPTOR));
+ CopyMem ((VOID *) (UINTN) GdtBuffer, (VOID *) Gdtr.Base, Gdtr.Limit + 1);
+ Gdtr.Base = (UINT32)(UINTN) GdtBuffer;
+ AsmWriteGdtr (&Gdtr);
+
+ return EFI_SUCCESS;
+}
+
/**
Initializes CPU exceptions handlers for the sake of stack switch requirement.

@@ -644,7 +682,7 @@ InitializeCpuMpWorker (
&gEfiVectorHandoffInfoPpiGuid,
0,
NULL,
- (VOID **)&VectorHandoffInfoPpi
+ (VOID **) &VectorHandoffInfoPpi
);
if (Status == EFI_SUCCESS) {
VectorInfo = VectorHandoffInfoPpi->Info;
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
index 7d5c527d6006..5dc956409594 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
@@ -397,6 +397,19 @@ SecPlatformInformation2 (
OUT EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2
);

+/**
+ Migrates the Global Descriptor Table (GDT) to permanent memory.
+
+ @retval EFI_SUCCESS The GDT was migrated successfully.
+ @retval EFI_OUT_OF_RESOURCES The GDT could not be migrated due to lack of available memory.
+
+**/
+EFI_STATUS
+EFIAPI
+MigrateGdt (
+ VOID
+ );
+
/**
Initializes MP and exceptions handlers.

diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
index a462e7ee1e38..d0cbebf70bbf 100644
--- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
+++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
@@ -152,7 +152,7 @@ GetPhysicalAddressWidth (
Get the type of top level page table.

@retval Page512G PML4 paging.
- @retval Page1G PAE paing.
+ @retval Page1G PAE paging.

**/
PAGE_ATTRIBUTE
@@ -582,7 +582,7 @@ SetupStackGuardPage (
}

/**
- Enabl/setup stack guard for each processor if PcdCpuStackGuard is set to TRUE.
+ Enable/setup stack guard for each processor if PcdCpuStackGuard is set to TRUE.

Doing this in the memory-discovered callback is to make sure the Stack Guard
feature to cover as most PEI code as possible.
@@ -602,8 +602,14 @@ MemoryDiscoveredPpiNotifyCallback (
IN VOID *Ppi
)
{
- EFI_STATUS Status;
- BOOLEAN InitStackGuard;
+ EFI_STATUS Status;
+ BOOLEAN InitStackGuard;
+ BOOLEAN InterruptState;
+
+ InterruptState = SaveAndDisableInterrupts ();
+ Status = MigrateGdt ();
+ ASSERT_EFI_ERROR (Status);
+ SetInterruptState (InterruptState);

//
// Paging must be setup first. Otherwise the exception TSS setup during MP
(12) The GDT migration should be made dependent on
"PcdMigrateTemporaryRamFirmwareVolumes", shouldn't it?
... Or is this *another* preexistent bug that we should fix regardless
of the "temporary RAM evacuation" feature?

I mean, considering current master, once we switch to permanent PEI RAM,
do we still rely on a GDT that lives in temp RAM?

If that's the case, then we should even split this series into two
series. The first series should fix the other issues first -- typos,
IN/OUT mistakes, this GDT problem, and the S3 shadowing bug in the DXE
IPL PEIM.

Once all that is done, we can introduce
"PcdMigrateTemporaryRamFirmwareVolumes", and the dependent new feature.

Thanks
Laszlo


diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 1aafb7dac139..903449e0daa9 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -18,8 +18,8 @@
**/
VOID
ArchUpdateIdtEntry (
- IN IA32_IDT_GATE_DESCRIPTOR *IdtEntry,
- IN UINTN InterruptHandler
+ OUT IA32_IDT_GATE_DESCRIPTOR *IdtEntry,
+ IN UINTN InterruptHandler
)
{
IdtEntry->Bits.OffsetLow = (UINT16)(UINTN)InterruptHandler;
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c
index 20148db74cf8..d4ae153c5742 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c
@@ -87,7 +87,7 @@ InitializeCpuExceptionHandlers (
IdtEntryCount = (IdtDescriptor.Limit + 1) / sizeof (IA32_IDT_GATE_DESCRIPTOR);
if (IdtEntryCount > CPU_EXCEPTION_NUM) {
//
- // CPU exeption library only setup CPU_EXCEPTION_NUM exception handler at most
+ // CPU exception library only setup CPU_EXCEPTION_NUM exception handler at most
//
IdtEntryCount = CPU_EXCEPTION_NUM;
}

Re: [PATCH v2 4/9] MdeModulePkg/DxeIplPeim: Register for shadow on S3 shadowed boot (CVE-2019-11098)

Laszlo Ersek
 

On 07/03/20 16:00, Laszlo Ersek wrote:
On 07/02/20 07:15, Guomin Jiang wrote:
From: Jian J Wang <jian.j.wang@...>

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

Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Liming Gao <liming.gao@...>
Signed-off-by: Jian J Wang <jian.j.wang@...>
---
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 3 +++
MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
(1) The commit message is empty, and therefore useless. Please explain
why this change is being made.

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 3f1702854660..4ab54594ed66 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -121,6 +121,9 @@ [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## SOMETIMES_CONSUMES

+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot ## CONSUMES
+
[Depex]
gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index d48028cea0dd..9e1831c69819 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -77,7 +77,7 @@ PeimInitializeDxeIpl (

BootMode = GetBootModeHob ();

- if (BootMode != BOOT_ON_S3_RESUME) {
+ if (BootMode != BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot)) {
Status = PeiServicesRegisterForShadow (FileHandle);
if (Status == EFI_SUCCESS) {
//
(2) The above check does not seem complete. I think it should consider
"PcdMigrateTemporaryRamFirmwareVolumes".

I don't exactly understand the impact of the change, but it seems to
potentially affect even such platforms that set
"PcdMigrateTemporaryRamFirmwareVolumes" to FALSE; and that seems wrong.
... On further consideration, this patch seems to be fixing a
preexistent bug that is not related to the CVE at all. I think this
issue was simply exposed when testing the new feature. Is that right?

If that's correct, then please explain this very clearly in the commit
message.

Thanks,
Laszlo

Re: [PATCH 00/11] Introduce LsiScsi driver to OvmfPkg

Laszlo Ersek
 

Hi Gary,

On 07/01/20 06:04, Gary Lin wrote:
This patch series implement the driver for LSI 53C895A SCSI controller
for OVMF so that the user can access the storage devices connected to
QEMU "lsi" controller. The driver is disabled by default since LSI
53C895A is considered as a legacy device. To enable the driver, please
add "-D LSI_SCSI_ENABLE" when building OvmfPkg.

The patch series is also available in my git branch:
https://github.com/lcp/edk2/tree/ovmf-lsi-v1

Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
I've queued your series for review; please give me some time to get to
it. I hope I can review the patches next week.

Thanks
Laszlo


Gary Lin (11):
OvmfPkg/LsiScsiDxe: Create the empty driver
OvmfPkg/LsiScsiDxe: Install the skeleton of driver binding
OvmfPkg/LsiScsiDxe: Report the name of the driver
OvmfPkg/LsiScsiDxe: Probe PCI devices and look for LsiScsi
OvmfPkg/LsiScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
OvmfPkg/LsiScsiDxe: Report Targets and LUNs
OvmfPkg/LsiScsiDxe: Open PciIo protocol and initialize the device
OvmfPkg/LsiScsiDxe: Map DMA buffer
OvmfPkg/LsiScsiDxe: Examine the incoming SCSI Request Packet
OvmfPkg/LsiScsiDxe: Process the SCSI Request Packet
Maintainers.txt: Add myself as the reviewer for LsiScsi driver

Maintainers.txt | 4 +
OvmfPkg/Include/IndustryStandard/LsiScsi.h | 80 ++
OvmfPkg/LsiScsiDxe/LsiScsi.c | 1130 ++++++++++++++++++++
OvmfPkg/LsiScsiDxe/LsiScsi.h | 199 ++++
OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf | 46 +
OvmfPkg/OvmfPkg.dec | 8 +
OvmfPkg/OvmfPkgIa32.dsc | 4 +
OvmfPkg/OvmfPkgIa32.fdf | 3 +
OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
OvmfPkg/OvmfPkgIa32X64.fdf | 3 +
OvmfPkg/OvmfPkgX64.dsc | 4 +
OvmfPkg/OvmfPkgX64.fdf | 3 +
12 files changed, 1488 insertions(+)
create mode 100644 OvmfPkg/Include/IndustryStandard/LsiScsi.h
create mode 100644 OvmfPkg/LsiScsiDxe/LsiScsi.c
create mode 100644 OvmfPkg/LsiScsiDxe/LsiScsi.h
create mode 100644 OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf

Re: [PATCH v2 0/9] Migrate Pointer from flash to permanent memory (CVE-2019-11098)

Laszlo Ersek
 

Hi,

I'm adding Michael Kubacki's new email address to the "To:" list, as
Michael has authored a significant portion of this code, plus he seems
to have contributed a large part of the design, in
<https://bugzilla.tianocore.org/show_bug.cgi?id=1614>.

Also CC'ing Ard.

Comments below.

On 07/02/20 07:15, Guomin Jiang wrote:
The TOCTOU vulnerability allow that the physical present person to replace the code with the normal BootGuard check and PCR0 value.
The issue occur when BootGuard measure IBB and access flash code after NEM disable.
the reason why we access the flash code is that we have some pointer to flash.
To avoid this vulnerability, we need to convert those pointers, the patch series do this work and make sure that no code will access flash address.
I've now read through the comments in
<https://bugzilla.tianocore.org/show_bug.cgi?id=1614>, and I've also
checked the slides at:

[1]
https://conference.hitb.org/hitbsecconf2019ams/sessions/now-you-see-it-toctou-attacks-against-secure-boot-and-bootguard/

https://conference.hitb.org/hitbsecconf2019ams/materials/D1T1%20-%20Toctou%20Attacks%20Against%20Secure%20Boot%20-%20Trammell%20Hudson%20&%20Peter%20Bosch.pdf

My understanding is that this vulnerability (and fix) do not apply to
virtualization, or even most other emulation platforms (such as
EmulatorPkg).

Therefore I'm requesting that the approach seen in this patch series be
reversed, as follows.


* The patch v3 #1 should introduce the new PCD called
"PcdMigrateTemporaryRamFirmwareVolumes". (Currently: patch v2 #7.)

The comments in the DEC file (and in the UNI file) should *very* clearly
explain what the PCD controls. It should provide a *concise* description
of the entire feature. The default value of the PCD should be TRUE.


* The next patches in the series (v3 patches #2, #3, #4) should set the
PCD to FALSE in at least the following platform DSC files:

- ArmVirtPkg [v3 #2]
- EmulatorPkg [v3 #3],
- OvmfPkg [v3 #4].

The commit messages on these patches should explain that the
vulnerability simply doesn't exist on those platforms, as BootGuard is
undefined on them in the first place. "CAR" (Cache-As-RAM) is also
undefined on them.

In particular, slide #8 in the presentation, titled "Chain of Trust
(simplified)", presents two "cascades" (sub-chains of trust). My
understanding is that the attack targets the left hand side cascade
("Fused OEM key", "Signed by Intel"). And that cascade doesn't seem to
exist at all on virtualized platforms.

Furthermore, the abstract at [1] writes,

"These protections are supposed to be secure against physical attacks on
the SPI flash"

foreshadowing that the attack is indeed a physical machine attack.
Doesn't apply to virtualization.

Therefore both the attack and the mitigation appear moot, on virtual
platforms. This should be stated in the commit messages of v3 patches
#2, #3, #4.


* Patch v3 #5 should be the current (v2) patch #1, namely:

MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore

This patch should *immediately* depend on
"PcdMigrateTemporaryRamFirmwareVolumes". That is, please let's not
introduce the feature first as unconditionally active, and then gate it
on "PcdMigrateTemporaryRamFirmwareVolumes" separately. The feature
should honor "PcdMigrateTemporaryRamFirmwareVolumes" right off the bat.

Technically, this more or less means squashing part of patch v2 #7 into
v2 #1.

The commit message should point out that EvacuateTempRam() is never
called if "PcdMigrateTemporaryRamFirmwareVolumes" is FALSE.


* I've made the rest of my comments under the individual patches.

Thanks,
Laszlo


Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Liming Gao <liming.gao@...>
Cc: Debkumar De <debkumar.de@...>
Cc: Harry Han <harry.han@...>
Cc: Catharine West <catharine.west@...>
Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Chao Zhang <chao.b.zhang@...>
Cc: Qi Zhang <qi1.zhang@...>

Guomin Jiang (5):
MdeModulePkg/Core: Create Migrated FV Info Hob for calculating hash
(CVE-2019-11098)
SecurityPkg/Tcg2Pei: Use Migrated FV Info Hob for calculating hash
(CVE-2019-11098)
MdeModulePkg/Core: Add switch to enable or disable TOCTOU feature
(CVE-2019-11098)
UefiCpuPkg/SecMigrationPei: Add switch to control if produce PPI
(CVE-2019-11098)
UefiCpuPkg/CpuMpPei: Enable paging and set NP flag to avoid TOCTOU
(CVE-2019-11098)

Jian J Wang (1):
MdeModulePkg/DxeIplPeim: Register for shadow on S3 shadowed boot
(CVE-2019-11098)

Michael Kubacki (3):
MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore
(CVE-2019-11098)
UefiCpuPkg/CpuMpPei: Add GDT and IDT migration support
(CVE-2019-11098)
UefiCpuPkg/SecMigrationPei: Add initial PEIM (CVE-2019-11098)

MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 3 +
MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 2 +-
MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 417 ++++++++++++++++++
MdeModulePkg/Core/Pei/Image/Image.c | 115 +++++
MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 82 ++++
MdeModulePkg/Core/Pei/PeiMain.h | 169 +++++++
MdeModulePkg/Core/Pei/PeiMain.inf | 3 +
MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 17 +
MdeModulePkg/Core/Pei/Ppi/Ppi.c | 287 ++++++++++++
MdeModulePkg/Include/Guid/MigratedFvInfo.h | 22 +
MdeModulePkg/MdeModulePkg.dec | 8 +
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 31 +-
SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 +
UefiCpuPkg/CpuMpPei/CpuMpPei.c | 40 +-
UefiCpuPkg/CpuMpPei/CpuMpPei.h | 13 +
UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 3 +
UefiCpuPkg/CpuMpPei/CpuPaging.c | 31 +-
UefiCpuPkg/Include/Ppi/RepublishSecPpi.h | 54 +++
.../Ia32/ArchExceptionHandler.c | 4 +-
.../SecPeiCpuException.c | 2 +-
UefiCpuPkg/SecCore/SecCore.inf | 2 +
UefiCpuPkg/SecCore/SecMain.c | 26 +-
UefiCpuPkg/SecCore/SecMain.h | 1 +
UefiCpuPkg/SecMigrationPei/SecMigrationPei.c | 374 ++++++++++++++++
UefiCpuPkg/SecMigrationPei/SecMigrationPei.h | 170 +++++++
.../SecMigrationPei/SecMigrationPei.inf | 68 +++
.../SecMigrationPei/SecMigrationPei.uni | 13 +
UefiCpuPkg/UefiCpuPkg.dec | 4 +
UefiCpuPkg/UefiCpuPkg.dsc | 1 +
29 files changed, 1947 insertions(+), 16 deletions(-)
create mode 100644 MdeModulePkg/Include/Guid/MigratedFvInfo.h
create mode 100644 UefiCpuPkg/Include/Ppi/RepublishSecPpi.h
create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.c
create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.h
create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.inf
create mode 100644 UefiCpuPkg/SecMigrationPei/SecMigrationPei.uni

Re: [PATCH v2 8/9] UefiCpuPkg/SecMigrationPei: Add switch to control if produce PPI (CVE-2019-11098)

Laszlo Ersek
 

On 07/02/20 07:15, Guomin Jiang wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614

SecMigrationPei create RepublishSecPpi, if the TOCTOU switch is off,
the Ppi is meaningless, so relate it with TOCTOU switch to avoid
producing useless PPI.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
UefiCpuPkg/SecMigrationPei/SecMigrationPei.c | 8 +++++---
UefiCpuPkg/SecMigrationPei/SecMigrationPei.inf | 4 ++++
2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/SecMigrationPei/SecMigrationPei.c b/UefiCpuPkg/SecMigrationPei/SecMigrationPei.c
index f96013b09b21..ab8066e8e0de 100644
--- a/UefiCpuPkg/SecMigrationPei/SecMigrationPei.c
+++ b/UefiCpuPkg/SecMigrationPei/SecMigrationPei.c
@@ -363,10 +363,12 @@ SecMigrationPeiInitialize (
IN CONST EFI_PEI_SERVICES **PeiServices
)
{
- EFI_STATUS Status;
+ EFI_STATUS Status = EFI_SUCCESS;

- Status = PeiServicesInstallPpi (&mEdkiiRepublishSecPpiDescriptor);
- ASSERT_EFI_ERROR (Status);
+ if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) {
+ Status = PeiServicesInstallPpi (&mEdkiiRepublishSecPpiDescriptor);
+ ASSERT_EFI_ERROR (Status);
+ }

return Status;
}
diff --git a/UefiCpuPkg/SecMigrationPei/SecMigrationPei.inf b/UefiCpuPkg/SecMigrationPei/SecMigrationPei.inf
index e29c04710941..8edbd3aa23a9 100644
--- a/UefiCpuPkg/SecMigrationPei/SecMigrationPei.inf
+++ b/UefiCpuPkg/SecMigrationPei/SecMigrationPei.inf
@@ -60,5 +60,9 @@ [Ppis]
## SOMETIMES_PRODUCES
gEfiSecPlatformInformation2PpiGuid

+[Pcd]
+ ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMigrateTemporaryRamFirmwareVolumes
+
[Depex]
TRUE
(1) This patch should be squashed into:

"UefiCpuPkg/SecMigrationPei: Add initial PEIM"

Thanks.
Laszlo

Re: [PATCH v2 5/9] MdeModulePkg/Core: Create Migrated FV Info Hob for calculating hash (CVE-2019-11098)

Laszlo Ersek
 

On 07/02/20 07:15, Guomin Jiang wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614

When we allocate pool to save rebased the PEIMs, the address will change
randomly, therefore the hash will change and result PCR0 change as well.
To avoid this, we save the raw PEIMs and use it to calculate hash.
(1) Please extend the commit message. We should state that
"gEdkiiMigratedFvInfoGuid" HOBs are *never* produced if
"PcdMigrateTemporaryRamFirmwareVolumes" is FALSE.


Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Liming Gao <liming.gao@...>
Cc: Debkumar De <debkumar.de@...>
Cc: Harry Han <harry.han@...>
Cc: Catharine West <catharine.west@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 15 +++++++++++++
MdeModulePkg/Core/Pei/PeiMain.h | 1 +
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
MdeModulePkg/Include/Guid/MigratedFvInfo.h | 22 +++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 +++
5 files changed, 42 insertions(+)
create mode 100644 MdeModulePkg/Include/Guid/MigratedFvInfo.h

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index ef88b3423376..7e1ac38f35c8 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -1223,10 +1223,12 @@ EvacuateTempRam (
EFI_FIRMWARE_VOLUME_HEADER *FvHeader;
EFI_FIRMWARE_VOLUME_HEADER *ChildFvHeader;
EFI_FIRMWARE_VOLUME_HEADER *MigratedFvHeader;
+ EFI_FIRMWARE_VOLUME_HEADER *RawDataFvHeader;
EFI_FIRMWARE_VOLUME_HEADER *MigratedChildFvHeader;

PEI_CORE_FV_HANDLE PeiCoreFvHandle;
EFI_PEI_CORE_FV_LOCATION_PPI *PeiCoreFvLocationPpi;
+ EDKII_MIGRATED_FV_INFO MigratedFvInfo;

ASSERT (Private->PeiMemoryInstalled);

@@ -1270,6 +1272,13 @@ EvacuateTempRam (
);
ASSERT_EFI_ERROR (Status);

+ Status = PeiServicesAllocatePages (
+ EfiBootServicesCode,
+ EFI_SIZE_TO_PAGES ((UINTN) FvHeader->FvLength),
+ (EFI_PHYSICAL_ADDRESS *) &RawDataFvHeader
+ );
+ ASSERT_EFI_ERROR (Status);
+
DEBUG ((
DEBUG_VERBOSE,
" Migrating FV[%d] from 0x%08X to 0x%08X\n",
@@ -1279,6 +1288,12 @@ EvacuateTempRam (
));

CopyMem (MigratedFvHeader, FvHeader, (UINTN) FvHeader->FvLength);
+ CopyMem (RawDataFvHeader, MigratedFvHeader, (UINTN) FvHeader->FvLength);
+ MigratedFvInfo.FvOrgBase = (UINT32) (UINTN) FvHeader;
+ MigratedFvInfo.FvNewBase = (UINT32) (UINTN) MigratedFvHeader;
+ MigratedFvInfo.FvDataBase = (UINT32) (UINTN) RawDataFvHeader;
+ MigratedFvInfo.FvLength = (UINT32) (UINTN) FvHeader->FvLength;
+ BuildGuidDataHob (&gEdkiiMigratedFvInfoGuid, &MigratedFvInfo, sizeof (MigratedFvInfo));

//
// Migrate any children for this FV now
(2) Please repeat the same statement here (from the commit message).

That statement is very important for understanding the control flow of
this feature. Some of the other modules do not consult
"PcdMigrateTemporaryRamFirmwareVolumes"; instead, they consume
"gEdkiiMigratedFvInfoGuid" HOBs.

We should explain (both in the commit message and in the code) that
"PcdMigrateTemporaryRamFirmwareVolumes" controls the *latter* kind of
modules too, via the production of "gEdkiiMigratedFvInfoGuid" HOBs. (In
other words, if the PCD is FALSE, such HOBs are never produced.)

Thanks
Laszlo

diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
index b0101dba5e30..cbf74d5b9d9a 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.h
+++ b/MdeModulePkg/Core/Pei/PeiMain.h
@@ -44,6 +44,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/FirmwareFileSystem2.h>
#include <Guid/FirmwareFileSystem3.h>
#include <Guid/AprioriFileName.h>
+#include <Guid/MigratedFvInfo.h>

///
/// It is an FFS type extension used for PeiFindFileEx. It indicates current
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 5ff14100a65f..c80d16b4efa6 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -77,6 +77,7 @@ [Guids]
## CONSUMES ## GUID # Used to compare with FV's file system GUID and get the FV's file system format
gEfiFirmwareFileSystem3Guid
gStatusCodeCallbackGuid
+ gEdkiiMigratedFvInfoGuid ## SOMETIMES_PRODUCES ## HOB

[Ppis]
gEfiPeiStatusCodePpiGuid ## SOMETIMES_CONSUMES # PeiReportStatusService is not ready if this PPI doesn't exist
diff --git a/MdeModulePkg/Include/Guid/MigratedFvInfo.h b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
new file mode 100644
index 000000000000..061c17ed0e48
--- /dev/null
+++ b/MdeModulePkg/Include/Guid/MigratedFvInfo.h
@@ -0,0 +1,22 @@
+/** @file
+ Migrated FV information
+
+Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
+#define __EDKII_MIGRATED_FV_INFO_GUID_H__
+
+typedef struct {
+ UINT32 FvOrgBase; // original FV address
+ UINT32 FvNewBase; // new FV address
+ UINT32 FvDataBase; // original FV data
+ UINT32 FvLength; // Fv Length
+} EDKII_MIGRATED_FV_INFO;
+
+extern EFI_GUID gEdkiiMigratedFvInfoGuid;
+
+#endif // #ifndef __EDKII_MIGRATED_FV_INFO_GUID_H__
+
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 843e963ad34b..5e25cbe98ada 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -389,6 +389,9 @@ [Guids]
## GUID indicates the capsule is to store Capsule On Disk file names.
gEdkiiCapsuleOnDiskNameGuid = { 0x98c80a4f, 0xe16b, 0x4d11, { 0x93, 0x9a, 0xab, 0xe5, 0x61, 0x26, 0x3, 0x30 } }

+ ## Include/Guid/MigratedFvInfo.h
+ gEdkiiMigratedFvInfoGuid = { 0xc1ab12f7, 0x74aa, 0x408d, { 0xa2, 0xf4, 0xc6, 0xce, 0xfd, 0x17, 0x98, 0x71 } }
+
[Ppis]
## Include/Ppi/AtaController.h
gPeiAtaControllerPpiGuid = { 0xa45e60d1, 0xc719, 0x44aa, { 0xb0, 0x7a, 0xaa, 0x77, 0x7f, 0x85, 0x90, 0x6d }}