Date   

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

Laszlo Ersek
 

On 03/30/20 19:44, Ard Biesheuvel wrote:
On Mon, 30 Mar 2020 at 19:11, Sean via Groups.Io
<sean.brogan=microsoft.com@groups.io> wrote:

On Mon, Mar 30, 2020 at 10:04 AM, Ard Biesheuvel wrote:

Is there any way I could contribute ArmVirtQemu to this? Or would it
be easier if I provided comments/instructions?

Either way.
Any instructions you provide would be great. I was going to hack something up for feedback but happy for someone else to do it. Let me know.
OK, so the typical invocation would be

qemu-system-aarch64 -M virt -cpu cortex-a57 -m 1024 -net none
-nographic -bios .../path/to/QEMU_EFI.fd -hda
fat:rw:.../path/to/startup.nsh

The only complication compared to OVMF is that there is no separate
serial port for debug output vs console output, so everything is going
to come out of the same pipe, and grep'ing the console output for
meaningful strings may easily result in false positives. (-pflash
could be used as well, but doesn't really add anything in this case,
and QEMU for ARM has a quirk where pflash images must be exactly 64 MB
in size)
I'm begging you not to introduce instances of "-bios" anywhere near
edk2. Please? :) We need to educate QEMU users, and we need to keep our
sanity when facing bug reports. Let's not set bad examples anywhere, if
we can manage.

I think truncate(1) can do what we need for padding, without actually
allocating those MBs.

Thanks!
Laszlo


Re: [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings

Laszlo Ersek
 

On 03/30/20 19:24, Liran Alon wrote:

On 30/03/2020 18:54, Laszlo Ersek wrote:
On 03/28/20 21:00, Liran Alon wrote:
@@ -509,6 +720,14 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Reset device to stop device usage of the rings.
+ // This is required to safely free the rings.
+ //
+ PvScsiResetAdapter (Dev);
If I understand correctly (at the end of the series):

in order to save one of the (ultimately) two reset calls in
PvScsiUninit(), namely
- one for releasing the DMA buf
- and the other (internal to PvScsiFreeRings()) for releasing the
rings, you unnested the latter reset call from PvScsiFreeRings(), and
added it manually to the error path of PvScsiInit().
Right.
To me, that's more brittle and more difficult to reason about than
what I proposed, because now PvScsiFreeRings() does not completely
undo PvScsiInitRings().
Hmm right. Because PvScsiInitRings() also setup the ring to the device
with a command. Haven't thought about it.
I specifically requested that we please not try to save on the two
(seemingly) redundant reset calls in PvScsiUninit() -- see the
paragraph containing "bad idea" here:

[...]

(Note: we could be tempted to somehow "centralize" all of these
Reset operations into a single spot. Bad idea. We are revoking
the device's access rights to different resources, so the
revocation operations will show up in different spots. It's a
mere circumstance that the revocations all happen to be Reset
operations.)

I might be paranoid of course -- I just feel that
maybe-superfluous reset operations on error paths are much
better than silently corrupted guest memory and/or disk
contents.

You reacted to that message of mine, but not this paragraph
specifically (it was snipped from your followup) -- so I thought you
were OK with it. I'm a bit disappointed that you disagreed with my
request *silently*.
Oh now I got what you meant! I misinterpreted it. Not silently ignored
it of course!
I can change this in a v4 patch-series if you would like that. Sorry
for the misunderstanding.

I do agree with your statement that PvScsiFreeRings() is not
completely an undo of PvScsiInitRings().
And maybe for making code a little bit easier to reason about, it's
preferred to just do one additional unnecessary device reset.
To summarize: technically, your solution is correct; from a code
hygiene and resource ownership question, I'm convinced it is wrong.
I agree.
But, I'll live with it.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
I can send a v4 patch-series just changing that if you would like.
Or submit a patch to change it after it will be merged.
Or that you will change it when applying. Or leave it as is.

I don't have any objection to any of the above.
OK. That's a relief to me, thank you! And I apologize for insinuating
that you deliberately & silently ignored said feedback from me. I tend
to be pedantic about email (it's not natural for me to think that
someone simply missed a comment) -- sorry about that!

Given that I've already merged v3 -- can you please post a followup
patch? Something like:

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 0a66c98421a9..3066b83b96fc 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -1093,6 +1093,12 @@ PvScsiFreeRings (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Reset device to stop device usage of the rings.
+ // This is required to safely free the rings.
+ //
+ PvScsiResetAdapter (Dev);
+
PvScsiFreeSharedPages (
Dev,
1,
@@ -1199,12 +1205,6 @@ PvScsiInit (
return EFI_SUCCESS;

FreeRings:
- //
- // Reset device to stop device usage of the rings.
- // This is required to safely free the rings.
- //
- PvScsiResetAdapter (Dev);
-
PvScsiFreeRings (Dev);

RestorePciAttributes:
@@ -1220,12 +1220,8 @@ PvScsiUninit (
)
{
//
- // Reset device to:
- // - Make device stop processing all requests.
- // - Stop device usage of the rings.
- //
- // This is required to safely free the DMA communication buffer
- // and the rings.
+ // Reset device to make device stop processing all requests.
+ // This is required to safely free the DMA communication buffer.
//
PvScsiResetAdapter (Dev);
Thank you!
Laszlo


Re: [PATCH v2 1/1] MdePkg/UefiScsiLib: Set FUA bit for synchronous SCSI Write operations

Zurcher, Christopher J
 

This change has been validated on an internal development platform; I will share the details offline.

Thanks,
Christopher Zurcher

-----Original Message-----
From: Gao, Liming <liming.gao@intel.com>
Sent: Monday, March 30, 2020 06:51
To: Liu, Zhiguang <zhiguang.liu@intel.com>; Zurcher, Christopher J
<christopher.j.zurcher@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>
Subject: RE: [PATCH v2 1/1] MdePkg/UefiScsiLib: Set FUA bit for synchronous
SCSI Write operations

Zurcher:
What functionality test has been done for this test?

Thanks
Liming
-----Original Message-----
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Monday, March 30, 2020 9:52 AM
To: Zurcher, Christopher J <christopher.j.zurcher@intel.com>;
devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH v2 1/1] MdePkg/UefiScsiLib: Set FUA bit for synchronous
SCSI Write operations

Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>

-----Original Message-----
From: Zurcher, Christopher J
Sent: Thursday, March 26, 2020 3:34 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Gao, Liming <liming.gao@intel.com>; Liu,
Zhiguang
<zhiguang.liu@intel.com>
Subject: [PATCH v2 1/1] MdePkg/UefiScsiLib: Set FUA bit for synchronous
SCSI Write operations

The FUA (Force Unit Access) bit forces data to be written directly to
disk instead of the write cache. This prevents data from being lost if a
shutdown or reset is requested immediately after a SCSI write operation.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Christopher J Zurcher <christopher.j.zurcher@intel.com>
---
MdePkg/Include/IndustryStandard/Scsi.h | 8 +++++++-
MdePkg/Library/UefiScsiLib/UefiScsiLib.c | 14 ++++++++------
2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/MdePkg/Include/IndustryStandard/Scsi.h
b/MdePkg/Include/IndustryStandard/Scsi.h
index 3e966520a1..64b9918b82 100644
--- a/MdePkg/Include/IndustryStandard/Scsi.h
+++ b/MdePkg/Include/IndustryStandard/Scsi.h
@@ -1,7 +1,7 @@
/** @file
Support for SCSI-2 standard

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

**/
@@ -175,6 +175,12 @@
#define EFI_SCSI_DATA_IN 0
#define EFI_SCSI_DATA_OUT 1

+//
+// SCSI Block Command Cache Control Parameters
+//
+#define EFI_SCSI_BLOCK_FUA BIT3 ///< Force Unit Access
+#define EFI_SCSI_BLOCK_DPO BIT4 ///< Disable Page Out
+
//
// Peripheral Device Type Definitions
//
diff --git a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
index 13a2a1912c..512bec500c 100644
--- a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
+++ b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c
@@ -1,7 +1,7 @@
/** @file
UEFI SCSI Library implementation

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

**/
@@ -1055,15 +1055,16 @@ ScsiWrite10Command (
ZeroMem (&CommandPacket, sizeof
(EFI_SCSI_IO_SCSI_REQUEST_PACKET));
ZeroMem (Cdb, EFI_SCSI_OP_LENGTH_TEN);

- CommandPacket.Timeout = Timeout;
- CommandPacket.OutDataBuffer = DataBuffer;
- CommandPacket.SenseData = SenseData;
- CommandPacket.OutTransferLength= *DataLength;
- CommandPacket.Cdb = Cdb;
+ CommandPacket.Timeout = Timeout;
+ CommandPacket.OutDataBuffer = DataBuffer;
+ CommandPacket.SenseData = SenseData;
+ CommandPacket.OutTransferLength = *DataLength;
+ CommandPacket.Cdb = Cdb;
//
// Fill Cdb for Write (10) Command
//
Cdb[0] = EFI_SCSI_OP_WRITE10;
+ Cdb[1] = EFI_SCSI_BLOCK_FUA;
WriteUnaligned32 ((UINT32 *)&Cdb[2], SwapBytes32 (StartLba));
WriteUnaligned16 ((UINT16 *)&Cdb[7], SwapBytes16 ((UINT16)
SectorSize));

@@ -1263,6 +1264,7 @@ ScsiWrite16Command (
// Fill Cdb for Write (16) Command
//
Cdb[0] = EFI_SCSI_OP_WRITE16;
+ Cdb[1] = EFI_SCSI_BLOCK_FUA;
WriteUnaligned64 ((UINT64 *)&Cdb[2], SwapBytes64 (StartLba));
WriteUnaligned32 ((UINT32 *)&Cdb[10], SwapBytes32 (SectorSize));

--
2.16.2.windows.1


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

Ard Biesheuvel
 

On Mon, 30 Mar 2020 at 21:07, Sean via Groups.Io
<sean.brogan=microsoft.com@groups.io> wrote:



On Mon, Mar 30, 2020 at 10:44 AM, Ard Biesheuvel wrote:

Thanks. But can I get these actions to trigger from my branch as well?
Or do I need special powers for that?

Right now it requires manually running so it requires "special powers". :)
I am happy to get this into a branch that you can PR against and on each PR it will run. Basically you would just submit PRs against the branch in my edk2 fork.

I have it running successfully on GCC5 toolchain and AARCH64
https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=4978&view=results

I don't see any output from the Qemu boot process even for debug. Am I missing a parameter?
qemu-system-aarch64 -M virt -cpu cortex-a57 -bios /home/vsts/work/1/s/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd -m 1024 -net none -drive file=fat:rw:/home/vsts/work/1/s/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/VirtualDrive,format=raw,media=disk -display none
You need either -nographic or -serial stdio to get the serial output
to go to QEMU's stdout. The latter is probably better, since the
former gives you the serial stdout multiplexed with the QEMU console,
accessible via special control sequences, and the latter only gives
you the serial output stream.


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

Sean
 



On Mon, Mar 30, 2020 at 10:44 AM, Ard Biesheuvel wrote:
Thanks. But can I get these actions to trigger from my branch as well?
Or do I need special powers for that?
Right now it requires manually running so it requires "special powers". :) 
I am happy to get this into a branch that you can PR against and on each PR it will run.  Basically you would just submit PRs against the branch in my edk2 fork. 

I have it running successfully on GCC5 toolchain and AARCH64
https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=4978&view=results

I don't see any output from the Qemu boot process even for debug.  Am I missing a parameter?
qemu-system-aarch64 -M virt -cpu cortex-a57 -bios /home/vsts/work/1/s/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd -m 1024 -net none -drive file=fat:rw:/home/vsts/work/1/s/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/VirtualDrive,format=raw,media=disk -display none


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

Laszlo Ersek
 

On 03/30/20 11:02, Ard Biesheuvel wrote:
On Mon, 30 Mar 2020 at 10:52, Guomin Jiang <guomin.jiang@intel.com>
wrote:

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

OpenSSL requires _fltused to be defined as a constant anywhere
floating point is used.
This is to satisfy the linker, however, it is possible to compile a
module with multiple definitions of fltused. This causes the
MSVC compiler to fail the build.
To solve this problem, the FltUsedLib was created that is one spot
that the global static can exist.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
Doesn't this affect *every* platform? Isn't there a better way to do
this? E.g., using weak linkage?
We already have manually added files under
"CryptoPkg/Library/OpensslLib":

- ossl_store.c
- rand_pool.c
- rand_pool_noise.c
- rand_pool_noise_tsc.c

These files are then referenced in both OpensslLib.inf and
OpensslLibCrypto.inf, outside of the "Autogenerated files list".

In particular "ossl_store.c" looks like a good example -- it does
nothing, just provides a needed symbol.

The comment at <https://bugzilla.tianocore.org/show_bug.cgi?id=2596#c0>
states that _fltused is an OpenSSL requirement -- so why not move
_fltused into the edk2 openssl library instances, and even then, only
when building with MSVC?

BTW I don't think I understand the actual problem, from the bug report.
Matthew wrote, "it is possible to compile a module with multiple
definitions of fltused" -- I don't see how (and no example is provided),
assuming the module in question already uses IntrinsicLib.

In <https://bugzilla.tianocore.org/show_bug.cgi?id=2596#c5>, Sean
writes, "the reason we moved to a library to define this symbol was to
deal with two libraries within the same module. If both libs defined it
then there were problems". -- And I don't understand why *either* of
those libraries defined _fltused at all; I think they should have only
dependend on InstrinsicLib, which already ensures there's exactly one
external definition of _fltused.

I just applied the following patch locally:

diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
index 94fe341bec9d..6ae4c4c82ecf 100644
--- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
+++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
@@ -21,7 +21,9 @@ typedef UINTN size_t;

/* OpenSSL will use floating point support, and C compiler produces the _fltused
symbol by default. Simply define this symbol here to satisfy the linker. */
+#if 0
int GLOBAL_USED _fltused = 1;
+#endif

/* Sets buffers to a specified character */
void * memset (void *dest, int ch, size_t count)
and witnessed no build failures in my environment.

This external definition of "_fltused" comes from historical commit
97f98500c1d4 ("Add CryptoPkg (from UDK2010.UP3)", 2010-11-01), and has
been updated (tweaked) once since, in commit 933681b20844 ("CryptoPkg
IntrinsicLib: Make _fltused always be used", 2019-10-24), for
TianoCore#1603.

To me, even the initial addition (from 2010) seems incorrect.

Summary:

- I don't understand the problem. Please state it clearly, including
build platform, target (firmware) platform, toolchain, modules,
libraries, and so on.

- Assuming the _fltused external definition is needed in fact, I think
it should be moved into a C source file referenced by the OpenSSL INF
files. This will solve problems where some module depends on the
OpensslLib class, but not the IntrinsicLib class.

- And, this reference in the OpensslLib INF files should be toolchain
specific.

Adding a new lib *class* dependency to the CryptLib instances will break
*every* platform out there, which is especially incomprehensible given
that some platforms don't need _fltused *at all*. Please let's not make
this 9+ years old hack worse.

Thanks
Laszlo


Re: [PATCH 0/4] remove generation of EFI properties table

Ard Biesheuvel
 

(adding Jian and Hao)

Thanks for the acks, and apologies for failing to cc the MdeModulePkg
maintainers.

Jian, Hao, do you have any opinion on this series?

On Mon, 30 Mar 2020 at 15:42, Gao, Liming <liming.gao@intel.com> wrote:

Ack-by: Liming Gao <liming.gao@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
Sent: Friday, March 27, 2020 1:01 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
<ray.ni@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
Subject: Re: [edk2-devel] [PATCH 0/4] remove generation of EFI properties table

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

I cannot remember if there is windows OS still using the properties table.
Maybe Microsoft people can comment.

If no, I agree we can remove the old code.



-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Sent: Thursday, March 26, 2020 6:25 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek
<lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D
<michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
Subject: [PATCH 0/4] remove generation of EFI properties table

The EFI properties table is broken by design, deprecated, and seems to be
causing confusion as it is unclear to some how it differs from the memory
attributes table (which supersedes it). So let's get rid of the code that
generates it entirely, along with the GUID definitions, PCDs etc.

Due to how the two implementations are intertwined, patch #2 makes the
minimal changes required to stop producing the table (and to allow patch
#3 to remove the associated definitions from MdePkg). Patch #4 is optional
and merges the code together.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=2633

Ard Biesheuvel (4):
OvmfPkg: remove handling of properties table
MdeModulePkg: disable properties table generation but retain the code
MdePkg: remove PropertiesTable GUID
MdeModulePkg/DxeCore: merge properties table routines into MAT
handling

MdeModulePkg/Core/Dxe/DxeMain.h | 9 -
MdeModulePkg/Core/Dxe/DxeMain.inf | 3 -
MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 1 -
.../Core/Dxe/Misc/MemoryAttributesTable.c | 1226 ++++++++++++++-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 1 -
MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 1373 -----------------
MdeModulePkg/MdeModulePkg.dec | 24 -
MdeModulePkg/MdeModulePkg.uni | 21 -
MdePkg/Include/Guid/PropertiesTable.h | 31 -
MdePkg/MdePkg.dec | 3 -
OvmfPkg/OvmfPkgIa32.dsc | 1 -
OvmfPkg/OvmfPkgIa32X64.dsc | 1 -
OvmfPkg/OvmfPkgX64.dsc | 1 -
OvmfPkg/OvmfXen.dsc | 1 -
OvmfPkg/PlatformPei/Platform.c | 1 -
OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
16 files changed, 1222 insertions(+), 1476 deletions(-)
delete mode 100644 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
delete mode 100644 MdePkg/Include/Guid/PropertiesTable.h

--
2.17.1


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

Ard Biesheuvel
 

On Mon, 30 Mar 2020 at 19:11, Sean via Groups.Io
<sean.brogan=microsoft.com@groups.io> wrote:

On Mon, Mar 30, 2020 at 10:04 AM, Ard Biesheuvel wrote:

Is there any way I could contribute ArmVirtQemu to this? Or would it
be easier if I provided comments/instructions?

Either way.
Any instructions you provide would be great. I was going to hack something up for feedback but happy for someone else to do it. Let me know.
OK, so the typical invocation would be

qemu-system-aarch64 -M virt -cpu cortex-a57 -m 1024 -net none
-nographic -bios .../path/to/QEMU_EFI.fd -hda
fat:rw:.../path/to/startup.nsh

The only complication compared to OVMF is that there is no separate
serial port for debug output vs console output, so everything is going
to come out of the same pipe, and grep'ing the console output for
meaningful strings may easily result in false positives. (-pflash
could be used as well, but doesn't really add anything in this case,
and QEMU for ARM has a quirk where pflash images must be exactly 64 MB
in size)


Basically add the platform and pipeline yml here
https://github.com/spbrogan/edk2/tree/ci-for-ovmf/.azurepipelines/platforms

Then i have been leveraging PyTools to build and script operations.
So something like this. https://github.com/spbrogan/edk2/blob/ci-for-ovmf/OvmfPkg/PlatformBuild.py

A little bit of help in this readme.
https://github.com/spbrogan/edk2/blob/ci-for-ovmf/OvmfPkg/README-pytools.md
Thanks. But can I get these actions to trigger from my branch as well?
Or do I need special powers for that?


Re: [PATCH 0/8] CryptoPkg: Retire the deprecate function

Michael D Kinney
 

Hi,

I would prefer we only extend the protocol interface and
never remove any fields from the Protocol/PPI structures
so no offsets are ever changed.

The main remaining issue is how a platform developer knows
if they are using a deprecated interface. The best method
is to know at build time with a compiler failure and the
other option is to know at runtime with ASSERT()/
REPORT_STATUS_CODE(). If the field in the protocol structure
is renamed, then the library wrapper around the protocol
will not build. But what we really want to know is if another
library or module us using a deprecated lib function.

Laszlo had a suggestion in the review of adding modules
to the CryptoPkg for the binary versions of these modules
to provide libraries that matched the enabled functions
in the binaries so a platform build would fail at compile
time. There is very little difference between disabling
a service and permanently deprecating a services from a
platform detection perspective. Perhaps we should explore
Laszlo's ideas more to see if we can address both use cases
and always maintain the Protocol/PPI structures in a
backwards compatible manner and always keep the same GUID
and only increase the Version value when the Protocol/PPI
structure is extended.

Thanks,

Mike

-----Original Message-----
From: Fu, Siyuan <siyuan.fu@intel.com>
Sent: Sunday, March 29, 2020 8:05 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>; devel@edk2.groups.io; Gao,
Zhichao <zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Maciej Rabeda
<maciej.rabeda@linux.intel.com>; Wu, Jiaxin
<jiaxin.wu@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire
the deprecate function

Jiewen,

Same as you. I prefer update version (#1) for adding API,
and change
protocol GUID (#2)for deprecate unsecure API.

Best Regards
Siyuan

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: 2020年3月30日 10:47
To: Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>; devel@edk2.groups.io;
Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>; Maciej Rabeda
<maciej.rabeda@linux.intel.com>;
Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire
the deprecate
function

Thanks Siyuan.
Good, then I think we are aligned.
I agree with you that it is bad example because the
neither version nor GUID
is updated.

We can have at least two options here:
1) Update version
We can change old API to be "VOID *Reserved" or
"UNSUPPORT_FUNC
Reserved" in the EDKII_CRYPTO_PROTOCOL.

I really do not want to see something like
"EDKII_CRYPTO_MD4_INIT
Md4Init" still existing, because it may let people
think we are still support
MD4 and use it somewhere.

2) Update GUID
Then we can remove the "EDKII_CRYPTO_MD4_INIT Md4Init"
completely.
Of course, we can still update version although it is
optional.


For adding new API, I will definitely prefer #1.

For deprecating old API, if we choose #1, we need add
17 reserved fields in
this protocol for MD4, 3DES and RC4.
If we decide to deprecate HMAC_MD5/HMAC_SHA1 because of
no usage,
then we need have a protocol with 156 fields, and 29 of
them are reserved.
As such, I prefer #2 here, unless we have strong reason
to keep 29 reserved
fields in this protocol.


Thank you
Yao Jiewen


-----Original Message-----
From: Fu, Siyuan <siyuan.fu@intel.com>
Sent: Monday, March 30, 2020 10:17 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Kinney,
Michael D
<michael.d.kinney@intel.com>; devel@edk2.groups.io;
Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
<xiaoyux.lu@intel.com>;
Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu,
Jiaxin
<jiaxin.wu@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg:
Retire the deprecate
function

Hi, Jiewen

I agree with all the strategy you listed for the
Modulization FW update,
and
Mike's
for compatibility maintenance. While I have a little
different
understanding
about
"permanent binary compatibility". Mainly about what
kind of
"compatibility" we
have to provide.

I don't think "compatibility " means we cannot
deprecate any old API.
Instead of
that, I think the goal could be:
- If an old binary is using the deprecated API, it
should be able to fail
gracefully.
- If an old binary is NOT using the deprecated API,
it should not be
impacted and
able to work as before.

So how we deprecate an API from this internal
protocol is important. The
current
patch shows a bad example, it removes member
functions from the
protocol
structure, without changing the protocol GUID or
version number. In such
case,
an old binary consumer has no method to know if it's
working with an old
protocol
or a new one, and may call into incorrect function
even it doesn't use any
of the
deprecated APIs. That's something I want to avoid.

Best Regards
Siyuan

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: 2020年3月28日 7:43
To: Kinney, Michael D <michael.d.kinney@intel.com>;
devel@edk2.groups.io;
Fu, Siyuan <siyuan.fu@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu,
XiaoyuX
<xiaoyux.lu@intel.com>; Maciej Rabeda
<maciej.rabeda@linux.intel.com>;
Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg:
Retire the deprecate
function

Thanks Mike.
I understand the *private protocol* now. I think it
is OK to put
CryptoProtocol into Private dir to sever that
purpose.
And for private API, I don’t have concern on the
data structure, since it is
invisible.

Siyuan provided below usage and concern:
"The goal to provide a driver version of crypto
service is to support
modulization
FW update, which means the crypto driver may NOT be
updated together
with
its consumer. A platform may choose to update the
crypto service driver
to a
new version with this patch, then all the
BaseCryptoLib consumers will
be
messed."

This usage might become a problem, when we want to
deprecate an API
and
keep binary compatibility at same time.
(To be specific, I am not worried about source
compatibility, because we
can
update both producer and consumer.
I am not worried about adding API, because there
will be no issue on
appending a function at the end.)

Take below as an example:
Firmware Version 100 uses Crypto Version 100.
We want to deprecate a private API and change to a
new one. So, we
upgrade Crypto to Version 101 and update Firmware
to Version 101.
Of course, we need change *all other consumers* and
rebuild everything
make sure it works correctly.
However, it is hard to support this in
"modulization FW update", because
we
have no chance to update the binary of firmware
version 100.

If we have to keep *permanent binary
compatibility*, then we cannot
deprecate any old API, just because that will break
old consumer.
That brings much validation burden, because you
have to test every
update
in master with old binaries, besides the latest
binaries.
That also brings maintenance burden for the unused
old API. The only
consumer is in the old binary and invisible.
I don’t believe that is what we want.

Modulization FW update is good feature. And we can
have different
strategy
for that besides keeping permanent binary
compatibility.
1) Modulization FW update can be limited a range of
version. At some
point,
you have to update the whole FW, because there are
too many changes
or
incompatible binary changes. The cadence of full
update can be longer
than
the one of partial update. For example, Linux or
windows are making
incompatible change in major version and only keep
compatibility in
minor
version.
2) A project can branch the production launch
firmware, and only keep
binary compatibility and support the modulization
FW update within this
branch. As such, the big update in master won't
impact this branch. If a
production may choose to resync to master, at that
time a full firmware
update is required. I guess most people are using
this way in a real
production.

Thought?

Thank you
Yao Jiewen


-----Original Message-----
From: Kinney, Michael D
<michael.d.kinney@intel.com>
Sent: Saturday, March 28, 2020 12:38 AM
To: devel@edk2.groups.io; Yao, Jiewen
<jiewen.yao@intel.com>; Fu,
Siyuan
<siyuan.fu@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>; Kinney,
Michael D <michael.d.kinney@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu,
XiaoyuX
<xiaoyux.lu@intel.com>;
Maciej Rabeda <maciej.rabeda@linux.intel.com>;
Wu, Jiaxin
<jiaxin.wu@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg:
Retire the deprecate
function

Jiewen,

The purpose of private includes is to keep
modules/lib
in *other* packages from using interfaces that
are the
package with the private interface does not want
other
packages to use and does not want to have to
coordinate
with other packages if that package owner decides
to
make changes to those private interfaces.

For modules/libs within package that do use
private
includes, the package owner gets to decide how to
maintain the interfaces in the private includes
to
support those modules/libs.

For example, the CryptoPkg has modules that are
intended to be used as pre-built binaries, so the
CryptoPkg owner needs to make sure the
maintenance
of the private includes supports both the source
and
binary use cases.

The private Protocol/PPI interfaces in the
CryptoPkg
were designed to support future expansion. The
first
API in the Protocol/PPI is GetVersion(). The
version
value returned can be used to have different
layouts
of fields in the Protocol/PPI. In order to
support
backwards compatibility, APIs are added to the
end
of the Protocol/PPI structure as the version
value
is increased. You will notice that there is an
X509
service that was added further down than the
initial
grouping. This is just an example of how the
CryptoPkg
is maintaining a private interface for binary use
cases.
Other packages may choose alternate techniques.

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io
<devel@edk2.groups.io> On
Behalf Of Yao, Jiewen
Sent: Thursday, March 26, 2020 9:59 PM
To: Fu, Siyuan <siyuan.fu@intel.com>;
devel@edk2.groups.io; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu,
XiaoyuX
<xiaoyux.lu@intel.com>; Maciej Rabeda
<maciej.rabeda@linux.intel.com>; Wu, Jiaxin
<jiaxin.wu@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/8]
CryptoPkg: Retire
the deprecate function

Thanks Siyun.
I think probably we need discuss this more.

1) About private v.s. public.

The benefit for private include is to isolate
external
interface and internal interface.
A package can keep updating its private
interface without
impact any other packages.
However, in this case, a private interface
update will
bring binary compatibility issue with other
package.
I am not sure it is acceptable or not.

Mike
Do you have any comment? Is that the design
goal of
private interface - just keep source code
compatibility,
but not binary compatiblity?

2) About the protocol itself.

One concern I have is that we *hardcode* the
algorithm in
protocol.

We keeps adding new algorithm and removing old
one. That
means this protocol interface is unstable.

Today, we have defined SHA2 set, and
deprecating SHA1 and
older one. Tomorrow we may need add SHA3 set.
Today, we have RSAPKCS1_15. Tomorrow we will
have RSAPSS.
Some other new set of algorithms might be added
later,
such as AEAD, GMAC.

For a protocol definition, I think we need
*abstract the
action*, but not *algorithm*.
One good example is the UEFI HASH2 Protocol.
https://github.com/tianocore/edk2/blob/master/MdePkg/Incl
ude/Protocol/Hash2.h
It just tells you do the hash. You may add new
algorithm
GUID.

Another good example is inside of openssl. Now
it is
using EVP style cipher algo.
For example,
https://www.openssl.org/docs/man1.1.1/man3/EVP_EncryptIni
t_ex.html
https://www.openssl.org/docs/man1.1.0/man3/EVP_CIPHER_CTX
_ctrl.html
The cipher itself is input as parameter.

The benefit is that, if we want to deprecate an
algorithm, the interface can be unchanged.
Just the internal implementation can be
changed.
The current PCD mechanism can still be applied
to
internal implementation.

Can we get a chance to revisit/redesign the
protocol API,
when we move to public include?

Thank you
Yao Jiewen

-----Original Message-----
From: Fu, Siyuan <siyuan.fu@intel.com>
Sent: Friday, March 27, 2020 11:07 AM
To: Yao, Jiewen <jiewen.yao@intel.com>;
devel@edk2.groups.io; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu,
XiaoyuX
<xiaoyux.lu@intel.com>;
Maciej Rabeda
<maciej.rabeda@linux.intel.com>; Wu,
Jiaxin
<jiaxin.wu@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/8]
CryptoPkg: Retire
the deprecate function

Hi, Jiewen

Although the protocol is private, a
corresponding
BaseCryptoLib instance is
not private, like PeiCryptLib.inf,
RuntimeCryptLib,
etc. These library instances
will be static linked to the consumer driver,
for
example an iSCSI network driver.
So actually it's not a "private" change
inside
CryptoPkg.

The goal to provide a driver version of
crypto service
is to support modulization
FW update, which means the crypto driver may
NOT be
updated together with
its consumer. A platform may choose to update
the
crypto service driver to a
new version with this patch, then all the
BaseCryptoLib
consumers will be
messed.

Best Regards
Siyuan

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: 2020年3月27日 10:58
To: devel@edk2.groups.io; Fu, Siyuan
<siyuan.fu@intel.com>; Gao, Zhichao
<zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>;
Lu, XiaoyuX
<xiaoyux.lu@intel.com>;
Maciej Rabeda
<maciej.rabeda@linux.intel.com>; Wu,
Jiaxin
<jiaxin.wu@intel.com>
Subject: RE: [edk2-devel] [PATCH 0/8]
CryptoPkg:
Retire the deprecate
function

EDKII_CRYPTO_PROTOCOL is *private*.
https://github.com/tianocore/edk2/blob/master/CryptoPkg/P
rivate/Protocol/C
rypto.h

Why we cannot change?



-----Original Message-----
From: devel@edk2.groups.io
<devel@edk2.groups.io>
On Behalf Of Siyuan,
Fu
Sent: Friday, March 27, 2020 10:47 AM
To: Gao, Zhichao <zhichao.gao@intel.com>;
devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>;
Lu,
XiaoyuX
<xiaoyux.lu@intel.com>;
Maciej Rabeda
<maciej.rabeda@linux.intel.com>; Wu,
Jiaxin
<jiaxin.wu@intel.com>
Subject: Re: [edk2-devel] [PATCH 0/8]
CryptoPkg:
Retire the deprecate
function

Hi, Zhichao

We should never move/delete a member
field of a
previous defined protocol
Interface. Instead, these protocol APIs
shall be
kept and return an error code
If the function is retired. Otherwise the
consumer
driver may call into an
Incorrect function if it's build with
different
codebase/PCD settings with the
Crypto PEI/DXE/SMM driver.
This comment applies to all the
EDKII_CRYPTO_PROTOCOL related changes
in
your patch set.

Best Regards
Siyuan

-----Original Message-----
From: Gao, Zhichao
<zhichao.gao@intel.com>
Sent: 2020年3月27日 9:56
To: devel@edk2.groups.io
Cc: Wang, Jian J
<jian.j.wang@intel.com>; Lu,
XiaoyuX
<xiaoyux.lu@intel.com>;
Maciej Rabeda
<maciej.rabeda@linux.intel.com>;
Wu, Jiaxin
<jiaxin.wu@intel.com>; Fu, Siyuan
<siyuan.fu@intel.com>
Subject: [PATCH 0/8] CryptoPkg: Retire
the
deprecate function

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

MD4, AR4, Tdes, Aes Ecb mode, MD5 and
SHA1 is not
secure any longer.
They are all deprecated. Edk2 would not
support
them any longer.
So remove them.
But uefi spec want to keep MD5 and SHA1
for
backwards compatibility.
So add two pcds to control the MD5 and
SHA1
enablement. Set the pcds
default value to false to indicate they
are
deprecated.

NetWorkPkg's iSCSI driver would consume
the MD5
function, so change
the md5 pcd to TURE when iSCSI is
enabled.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Maciej Rabeda
<maciej.rabeda@linux.intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Zhichao Gao
<zhichao.gao@intel.com>

Zhichao Gao (8):
CryptoPkg/BaseCrpytLib: Retire MD4
algorithm
CryptoPkg/BaseCryptLib: Retire ARC4
algorithm
CryptoPkg/BaseCryptLib: Retire the
Tdes
algorithm
CryptoPkg/BaseCryptLib: Retire Aes
Ecb mode
algorithm
CryptoPkg/dec: Add pcds to avoid
building the
deprecated function
NetWorkPkg/Pcd.inc: Enable the MD5
for iSCSI
Crypto/BaseCryptLib: Using pcd to
control MD5
enablement
CryptoPkg/BaseCryptLib: Use Pcd to
control the
SHA1 enablement

CryptoPkg/CryptoPkg.dec
|
11 +
CryptoPkg/CryptoPkg.uni
|
11 +
CryptoPkg/Driver/Crypto.c
|
634 +-----------------
CryptoPkg/Include/Library/BaseCryptLib.h |
548 ---------------
.../Library/BaseCryptLib/BaseCryptLib.inf |
9 +-
.../Library/BaseCryptLib/Cipher/CryptAes.c |
114 ----
.../BaseCryptLib/Cipher/CryptAesNull.c
|
52 --
.../Library/BaseCryptLib/Cipher/CryptArc4.c |
205 ------
.../BaseCryptLib/Cipher/CryptArc4Null.c |
124 ----
.../Library/BaseCryptLib/Cipher/CryptTdes.c |
364 ----------
.../BaseCryptLib/Cipher/CryptTdesNull.c |
160 -----
.../Library/BaseCryptLib/Hash/CryptMd4.c |
223 ------
.../Library/BaseCryptLib/Hash/CryptMd4Null.c |
143 ----
.../Library/BaseCryptLib/Hash/CryptMd5.c |
5 +-
.../Library/BaseCryptLib/Hmac/CryptHmacMd5.c |
3 +
.../BaseCryptLib/Hmac/CryptHmacMd5Null.c |
3 +
.../Library/BaseCryptLib/Hmac/CryptHmacSha1.c |
3 +
.../BaseCryptLib/Hmac/CryptHmacSha1Null.c |
3 +
.../Library/BaseCryptLib/PeiCryptLib.inf |
13 +-
.../BaseCryptLib/Pk/CryptPkcs5Pbkdf2.c
|
3 +
.../Library/BaseCryptLib/Pk/CryptRsaBasic.c |
5 +
.../Library/BaseCryptLib/Pk/CryptRsaExt.c |
5 +
.../Library/BaseCryptLib/RuntimeCryptLib.inf |
13 +-
.../Library/BaseCryptLib/SmmCryptLib.inf |
13 +-
.../BaseCryptLibNull/BaseCryptLibNull.inf |
3 -
.../BaseCryptLibNull/Cipher/CryptAesNull.c |
54 +-
.../BaseCryptLibNull/Cipher/CryptArc4Null.c |
124 ----
.../BaseCryptLibNull/Cipher/CryptTdesNull.c |
160 -----
.../BaseCryptLibNull/Hash/CryptMd4Null.c |
143 ----
.../BaseCryptLibNull/Hash/CryptMd5Null.c |
3 +
.../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c |
3 +
.../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c |
4 +-
.../BaseCryptLibOnProtocolPpi/CryptLib.c |
604 +----------------
.../Library/BaseHashApiLib/BaseHashApiLib.c |
12 +
.../Library/BaseHashApiLib/BaseHashApiLib.inf |
1 +
CryptoPkg/Private/Protocol/Crypto.h
|
583 +---------------
NetworkPkg/NetworkPcds.dsc.inc
|
5 +-
37 files changed, 145 insertions(+),
4221
deletions(-)
delete mode 100644
CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4.c
delete mode 100644
CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4Null.c
delete mode 100644
CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c
delete mode 100644
CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdesNull.c
delete mode 100644
CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4.c
delete mode 100644
CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4Null.c
delete mode 100644
CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptArc4Null.c
delete mode 100644
CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptTdesNull.c
delete mode 100644
CryptoPkg/Library/BaseCryptLibNull/Hash/CryptMd4Null.c

--
2.21.0.windows.1



Re: [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings

Liran Alon
 

On 30/03/2020 18:54, Laszlo Ersek wrote:
On 03/28/20 21:00, Liran Alon wrote:
STATIC
EFI_STATUS
PvScsiInit (
@@ -466,6 +669,14 @@ PvScsiInit (
goto RestorePciAttributes;
}
+ //
+ // Init PVSCSI rings
+ //
+ Status = PvScsiInitRings (Dev);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -509,6 +720,14 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Reset device to stop device usage of the rings.
+ // This is required to safely free the rings.
+ //
+ PvScsiResetAdapter (Dev);
If I understand correctly (at the end of the series):

in order to save one of the (ultimately) two reset calls in
PvScsiUninit(), namely
- one for releasing the DMA buf
- and the other (internal to PvScsiFreeRings()) for releasing the rings,
you unnested the latter reset call from PvScsiFreeRings(), and added it
manually to the error path of PvScsiInit().
Right.

To me, that's more brittle and more difficult to reason about than what
I proposed, because now PvScsiFreeRings() does not completely undo
PvScsiInitRings().
Hmm right. Because PvScsiInitRings() also setup the ring to the device with a command. Haven't thought about it.
I specifically requested that we please not try to
save on the two (seemingly) redundant reset calls in PvScsiUninit() --
see the paragraph containing "bad idea" here:

https://urldefense.com/v3/__http://mid.mail-archive.com/45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-Fn-EyPU$
https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56425__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-4bfLR8U$

(Note: we could be tempted to somehow "centralize" all of these
Reset operations into a single spot. Bad idea. We are revoking the
device's access rights to different resources, so the revocation
operations will show up in different spots. It's a mere circumstance
that the revocations all happen to be Reset operations.)

I might be paranoid of course -- I just feel that maybe-superfluous
reset operations on error paths are much better than silently
corrupted guest memory and/or disk contents.

You reacted to that message of mine, but not this paragraph specifically
(it was snipped from your followup) -- so I thought you were OK with it.
I'm a bit disappointed that you disagreed with my request *silently*.
Oh now I got what you meant! I misinterpreted it. Not silently ignored it of course!
I can change this in a v4 patch-series if you would like that. Sorry for the misunderstanding.

I do agree with your statement that PvScsiFreeRings() is not completely an undo of PvScsiInitRings().
And maybe for making code a little bit easier to reason about, it's preferred to just do one additional unnecessary device reset.


To summarize: technically, your solution is correct; from a code hygiene
and resource ownership question, I'm convinced it is wrong.
I agree.

But, I'll live with it.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
I can send a v4 patch-series just changing that if you would like.
Or submit a patch to change it after it will be merged.
Or that you will change it when applying. Or leave it as is.

I don't have any objection to any of the above.


(Also, you didn't set up the "diff.orderFile" knob the way I requested
in point (8):
I actually did set it up in my ~/.gitconfig. It seems that it doesn't work from some reason...

https://urldefense.com/v3/__http://mid.mail-archive.com/0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-bs34O4s$
https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56420__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-msgEmGo$

and so the header file changes are again at the end of the patch...
Another thing I'll have to live with, I guess.)

Laszlo
Thanks for the understanding,
-Liran


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

Sean
 

On Mon, Mar 30, 2020 at 10:04 AM, Ard Biesheuvel wrote:
Is there any way I could contribute ArmVirtQemu to this? Or would it
be easier if I provided comments/instructions?
Either way.  
Any instructions you provide would be great.  I was going to hack something up for feedback but happy for someone else to do it.   Let me know.  

Basically add the platform and pipeline yml here
https://github.com/spbrogan/edk2/tree/ci-for-ovmf/.azurepipelines/platforms

Then i have been leveraging PyTools to build and script operations. 
So something like this.  https://github.com/spbrogan/edk2/blob/ci-for-ovmf/OvmfPkg/PlatformBuild.py

A little bit of help in this readme. 
https://github.com/spbrogan/edk2/blob/ci-for-ovmf/OvmfPkg/README-pytools.md


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

Ard Biesheuvel
 

On Mon, 30 Mar 2020 at 19:01, Sean via Groups.Io
<sean.brogan=microsoft.com@groups.io> wrote:

On Mon, Mar 30, 2020 at 02:36 AM, Ard Biesheuvel wrote:

It should be trivial to extend this to ARM, using TCG emulation.

One question though: what happens if the boot does not make it to the
shell, and crashes for some reason? The QEMU process will hang, so I'd
assume some kind of timeout should be applied?

It has a 1 minute timeout and then the build will kill the process.

The bootlog is uploaded in all cases so you can then look at the log.
Here is an example where i didn't configure QEMU right: https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=4942&view=logs&j=6fb09fdb-58e7-5b12-0698-873f788bd2e9&t=e63402bd-99b5-5ddd-38f9-868e9402ecc0

And the last lines in the bootlog.txt show

MaxCpuCountInitialization: BootCpuCount=1 mMaxCpuCount=1
Q35BoardVerification: no TSEG (SMRAM) on host bridge DID=0x1237; only DID=0x29C0 (Q35) is supported
ASSERT /home/vsts/work/1/s/OvmfPkg/PlatformPei/Platform.c(564): ((BOOLEAN)(0==1))
OK, great. So we're all set :-)

Is there any way I could contribute ArmVirtQemu to this? Or would it
be easier if I provided comments/instructions?


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

Sean
 

On Mon, Mar 30, 2020 at 02:36 AM, Ard Biesheuvel wrote:
It should be trivial to extend this to ARM, using TCG emulation.

One question though: what happens if the boot does not make it to the
shell, and crashes for some reason? The QEMU process will hang, so I'd
assume some kind of timeout should be applied?
It has a 1 minute timeout and then the build will kill the process.  

The bootlog is uploaded in all cases so you can then look at the log.  
Here is an example where i didn't configure QEMU right: https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=4942&view=logs&j=6fb09fdb-58e7-5b12-0698-873f788bd2e9&t=e63402bd-99b5-5ddd-38f9-868e9402ecc0

And the last lines in the bootlog.txt show

MaxCpuCountInitialization: BootCpuCount=1 mMaxCpuCount=1
Q35BoardVerification: no TSEG (SMRAM) on host bridge DID=0x1237; only DID=0x29C0 (Q35) is supported
ASSERT /home/vsts/work/1/s/OvmfPkg/PlatformPei/Platform.c(564): ((BOOLEAN)(0==1))





Re: [PATCH v1 1/2] DynamicTablesPkg: SRAT: Fix entry points

Sami Mujawar
 

Hi Ard,

Please see my reply inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Sent: 30 March 2020 09:08 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Laura Moretta <Laura.Moretta@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 1/2] DynamicTablesPkg: SRAT: Fix entry points

On Sun, 29 Mar 2020 at 19:38, Sami Mujawar <sami.mujawar@arm.com> wrote:

VS2017 reports 'warning C4028: formal parameter 2 different from
declaration' for the library constructor and destructor interfaces for
the SRAT Generator modules.

Remove the CONST qualifier for the SystemTable pointer (the second
parameter to the constructor/destructor/DXE Entry
point) to make it compatible with the formal declaration.
You are removing the CONST qualifier from two places, no?
[SAMI] Yes, CONST is removed from 2 places. I will update the commit message. If you think this must be changed, then we need to do that as part of a separate patch series.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c | 8
++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git
a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
index
5d56af66608d862e6eca81da812d719f110867d2..74cb7d92a5d8cddd3df8334f3ab5
5e6fa3e7267a 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/SratGenerator.c
@@ -800,8 +800,8 @@ ACPI_TABLE_GENERATOR SratGenerator = { EFI_STATUS
EFIAPI AcpiSratLibConstructor (
- IN CONST EFI_HANDLE ImageHandle,
- IN EFI_SYSTEM_TABLE * CONST SystemTable
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE * SystemTable
)
{
EFI_STATUS Status;
@@ -823,8 +823,8 @@ AcpiSratLibConstructor ( EFI_STATUS EFIAPI
AcpiSratLibDestructor (
- IN CONST EFI_HANDLE ImageHandle,
- IN EFI_SYSTEM_TABLE * CONST SystemTable
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE * SystemTable
Can we keep the * with the argument name, please?
[SAMI] We have used the above convention throughout the package.

)
{
EFI_STATUS Status;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'




Re: [EXTERNAL] [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.

Bret Barkelew
 

Reviewed-by: Bret Barkelew <bret.barkelew@...>

 

- Bret

 


From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Guomin Jiang via Groups.Io <guomin.jiang@...>
Sent: Monday, March 30, 2020 1:52:47 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Jian J Wang <jian.j.wang@...>; Xiaoyu Lu <xiaoyux.lu@...>
Subject: [EXTERNAL] [edk2-devel] [PATCH] CryptoPkg/FltUsedLib: Add FltUsedLib for float.
 
REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2596&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=LCgdHl1VW1DIGzMCei5PDRP7UQtD%2FERzkumrbgpYuJs%3D&amp;reserved=0

OpenSSL requires _fltused to be defined as a constant anywhere floating
point is used.
This is to satisfy the linker, however, it is possible to compile a
module with multiple definitions of fltused. This causes the
MSVC compiler to fail the build.
To solve this problem, the FltUsedLib was created that is one spot
that the global static can exist.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Xiaoyu Lu <xiaoyux.lu@...>
Signed-off-by: Guomin Jiang <guomin.jiang@...>
---
 CryptoPkg/CryptoPkg.dsc                       |  2 ++
 .../Library/BaseCryptLib/BaseCryptLib.inf     |  1 +
 .../Library/BaseCryptLib/PeiCryptLib.inf      |  1 +
 .../Library/BaseCryptLib/RuntimeCryptLib.inf  |  1 +
 .../Library/BaseCryptLib/SmmCryptLib.inf      |  1 +
 CryptoPkg/Library/FltUsedLib/FltUsedLib.c     | 14 ++++++++
 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf   | 33 +++++++++++++++++++
 CryptoPkg/Library/TlsLib/TlsLib.inf           |  1 +
 8 files changed, 54 insertions(+)
 create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.c
 create mode 100644 CryptoPkg/Library/FltUsedLib/FltUsedLib.inf

diff --git a/CryptoPkg/CryptoPkg.dsc b/CryptoPkg/CryptoPkg.dsc
index 4cb37b1349..e9854087b5 100644
--- a/CryptoPkg/CryptoPkg.dsc
+++ b/CryptoPkg/CryptoPkg.dsc
@@ -97,6 +97,7 @@
   IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf                                          #???

   OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf

   IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  FltUsedLib|CryptoPkg/Library/FltUsedLib/FltUsedLib.inf

   SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf

 

 [LibraryClasses.ARM]

@@ -232,6 +233,7 @@
   CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

   CryptoPkg/Library/BaseCryptLibNull/BaseCryptLibNull.inf

   CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  CryptoPkg/Library/FltUsedLib/FltUsedLib.inf

   CryptoPkg/Library/TlsLib/TlsLib.inf

   CryptoPkg/Library/TlsLibNull/TlsLibNull.inf

   CryptoPkg/Library/OpensslLib/OpensslLib.inf

diff --git a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
index 1bbe4f435a..c82e703aa0 100644
--- a/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -84,6 +84,7 @@
   DebugLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib

   PrintLib

 

 #

diff --git a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
index c836c257f8..342aad008c 100644
--- a/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
@@ -78,6 +78,7 @@
   DebugLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib

 

 #

 # Remove these [BuildOptions] after this library is cleaned up

diff --git a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
index bff308a4f5..dcf209d7f5 100644
--- a/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
@@ -89,6 +89,7 @@
   DebugLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib

   PrintLib

 

 #

diff --git a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
index cc0b65fd25..7fdaaa48a6 100644
--- a/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
+++ b/CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf
@@ -88,6 +88,7 @@
   MemoryAllocationLib

   OpensslLib

   IntrinsicLib

+  FltUsedLib

   PrintLib

 

 #

diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.c b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
new file mode 100644
index 0000000000..8f2487ea13
--- /dev/null
+++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.c
@@ -0,0 +1,14 @@
+/** @file

+  Need include this when use floats.

+

+  Copyright (C) Microsoft Corporation. All rights reserved.<BR>

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

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

+

+**/

+

+//

+// You need to include this to let the compiler know we are going to use

+// floating point

+//

+int _fltused = 0x9875;

diff --git a/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
new file mode 100644
index 0000000000..8d67eadfa5
--- /dev/null
+++ b/CryptoPkg/Library/FltUsedLib/FltUsedLib.inf
@@ -0,0 +1,33 @@
+## @file

+# It is depent on when using floats

+#

+# Copyright (C) Microsoft Corporation. All rights reserved.<BR>

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

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

+#

+##

+

+[Defines]

+  INF_VERSION                    = 0x00010005

+  BASE_NAME                      = FltUsedLib

+  FILE_GUID                      = C004F180-9FE2-4D2B-8318-BADC2A231774

+  MODULE_TYPE                    = BASE

+  VERSION_STRING                 = 1.0

+  LIBRARY_CLASS                  = FltUsedLib

+

+#

+# The following information is for reference only and not required by the build tools.

+#

+#  VALID_ARCHITECTURES           = IA32 X64 AARCH64

+#

+

+[Sources]

+  FltUsedLib.c

+

+[Packages]

+  MdePkg/MdePkg.dec

+

+[BuildOptions]

+  # Disable GL due to linker error LNK1237

+  # https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fcpp%2Ferror-messages%2Ftool-errors%2Flinker-tools-error-lnk1237%3Fview%3Dvs-2017&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=vULhLfRubt3e7xbVLa%2BHC0E34JTcgOSbdYndn%2FxNEps%3D&amp;reserved=0

+  MSFT:*_*_*_CC_FLAGS = /GL-

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index 2f3ce695c3..febbdf5149 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -37,6 +37,7 @@
   BaseMemoryLib

   DebugLib

   IntrinsicLib

+  FltUsedLib

   MemoryAllocationLib

   OpensslLib

   SafeIntLib

--
2.25.1.windows.1


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

View/Reply Online (#56613): https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F56613&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=DHVFRPBfEkbBLIgrIdwQxnGH0XjtN1oxsZYVyLDIqf4%3D&amp;reserved=0
Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F72648022%2F1822150&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=KyjsUA%2BJ4U6YdFn22%2BHhr%2F0JF5AojkVTU52vmE1zh2Q%3D&amp;reserved=0
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Cdcda1866343d4d6dab3908d7d487ba8e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637211551721384527&amp;sdata=04dDAgBNEz8Ra5JuAfOVK3dvos1vhfQDma5Y2Qv4eao%3D&amp;reserved=0  [brbarkel@...]
-=-=-=-=-=-=


Re: [PATCH v6 00/42] SEV-ES guest support

Lendacky, Thomas
 

I've gotten some nice feedback from Laszlo, especially on the OvmfPkg side of this patchset, but haven't seen much response from the other maintainers. Is there any feedback on the MdePkg, MdeModulePkg and UefiCpuPkg changes that needs to be addressed in order to merge this?

I do have some minor changes on ensuring the per-CPU variable page stays encrypted, but not much beyond that. Those changes can be submitted afterwards or as a new version before inclusion.

Thanks,
Tom

On 3/24/20 12:40 PM, Tom Lendacky wrote:
This patch series provides support for running EDK2/OVMF under SEV-ES.
Secure Encrypted Virtualization - Encrypted State (SEV-ES) expands on the
SEV support to protect the guest register state from the hypervisor. See
"AMD64 Architecture Programmer's Manual Volume 2: System Programming",
section "15.35 Encrypted State (SEV-ES)" [1].
In order to allow a hypervisor to perform functions on behalf of a guest,
there is architectural support for notifying a guest's operating system
when certain types of VMEXITs are about to occur. This allows the guest to
selectively share information with the hypervisor to satisfy the requested
function. The notification is performed using a new exception, the VMM
Communication exception (#VC). The information is shared through the
Guest-Hypervisor Communication Block (GHCB) using the VMGEXIT instruction.
The GHCB format and the protocol for using it is documented in "SEV-ES
Guest-Hypervisor Communication Block Standardization" [2].
The main areas of the EDK2 code that are updated to support SEV-ES are
around the exception handling support and the AP boot support.
Exception support is required starting in Sec, continuing through Pei
and into Dxe in order to handle #VC exceptions that are generated. Each
AP requires it's own GHCB page as well as a page to hold values specific
to that AP.
AP booting poses some interesting challenges. The INIT-SIPI-SIPI sequence
is typically used to boot the APs. However, the hypervisor is not allowed
to update the guest registers. The GHCB document [2] talks about how SMP
booting under SEV-ES is performed.
Since the GHCB page must be a shared (unencrypted) page, the processor
must be running in long mode in order for the guest and hypervisor to
communicate with each other. As a result, SEV-ES is only supported under
the X64 architecture.
[1] https://www.amd.com/system/files/TechDocs/24593.pdf
[2] https://developer.amd.com/wp-content/resources/56421.pdf
---
These patches are based on commit:
2f524a745e23 ("BaseTools:Fix build tools print traceback info issue")
Proper execution of SEV-ES relies on Bugzilla 2340 being fixed.
A version of the tree (with an extra patch to workaround Bugzilla 2340) can
be found at:
https://github.com/AMDESE/ovmf/tree/sev-es-v13
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Changes since v5:
- Remove extraneous VmgExitLib usage
- Miscellaneous changes to address feedback (coding style, etc.)
Changes since v4:
- Move the SEV-ES protocol negotiation out of the SEC exception handler
and into the SecMain.c file. As a result:
- Move the SecGhcb related PCDs out of UefiCpuPkg and into OvmfPkg
- Combine SecAMDSevVcHandler.c and PeiDxeAMDSevVcHandler.c into a
single AMDSevVcHandler.c
- Consolidate VmgExitLib usage into common LibraryClasses sections
- Add documentation comments to the VmgExitLib functions
Changes since v3:
- Remove the need for the MP library finalization routine. The AP
jump table address will be held by the hypervisor rather than
communicated via the GHCB MSR. This removes some fragility around
the UEFI to OS transition.
- Rename the SEV-ES RIP reset area to SEV-ES workarea and use it to
communicate the SEV-ES status, so that SEC CPU exception handling is
only established for an SEV-ES guest.
- Fix SMM build breakageAdd around QemuFlashPtrWrite().
- Fix SMM build breakage by adding VC exception support the SMM CPU
exception handling.
- Add memory fencing around the invocation of AsmVmgExit().
- Clarify comments around the SEV-ES AP reset RIP values and usage.
- Move some PCD definitions from MdeModulePkg to UefiCpuPkg.
- Remove the 16-bit code selector definition from MdeModulePkg
Changes since v2:
- Added a way to locate the SEV-ES fixed AP RIP address for starting
AP's to avoid updating the actual flash image (build time location
that is identified with a GUID value).
- Create a VmgExit library to replace static inline functions.
- Move some PCDs to the appropriate packages
- Add support for writing to QEMU flash under SEV-ES
- Add additional MMIO opcode support
- Cleaned up the GHCB MSR CPUID protocol support
Changes since v1:
- Patches reworked to be more specific to the component/area being updated
and order of definition/usage
- Created a library for VMGEXIT-related functions to replace use of inline
functions
- Allocation method for GDT changed from AllocatePool to AllocatePages
- Early caching only enabled for SEV-ES guests
- Ensure AP loop mode set to halt loop mode for SEV-ES guests
- Reserved SEC GHCB-related memory areas when S3 is enabled
Tom Lendacky (42):
MdePkg: Create PCDs to be used in support of SEV-ES
MdePkg: Add the MSR definition for the GHCB register
MdePkg: Add a structure definition for the GHCB
MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page tables
MdePkg/BaseLib: Add support for the XGETBV instruction
MdePkg/BaseLib: Add support for the VMGEXIT instruction
UefiCpuPkg: Implement library support for VMGEXIT
OvmfPkg: Prepare OvmfPkg to use the VmgExitLib library
UefiPayloadPkg: Prepare UefiPayloadPkg to use the VmgExitLib library
UefiCpuPkg/CpuExceptionHandler: Add base support for the #VC exception
UefiCpuPkg/CpuExceptionHandler: Add support for IOIO_PROT NAE events
UefiCpuPkg/CpuExceptionHandler: Support string IO for IOIO_PROT NAE
events
UefiCpuPkg/CpuExceptionHandler: Add support for CPUID NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for MSR_PROT NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for NPF NAE events (MMIO)
UefiCpuPkg/CpuExceptionHandler: Add support for WBINVD NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for RDTSC NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for RDPMC NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for INVD NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for VMMCALL NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for RDTSCP NAE events
UefiCpuPkg/CpuExceptionHandler: Add support for MONITOR/MONITORX NAE
events
UefiCpuPkg/CpuExceptionHandler: Add support for MWAIT/MWAITX NAE
events
UefiCpuPkg/CpuExceptionHandler: Add support for DR7 Read/Write NAE
events
OvmfPkg/MemEncryptSevLib: Add an SEV-ES guest indicator function
OvmfPkg: Add support to perform SEV-ES initialization
OvmfPkg: Create a GHCB page for use during Sec phase
OvmfPkg/PlatformPei: Reserve GHCB-related areas if S3 is supported
OvmfPkg: Create GHCB pages for use during Pei and Dxe phase
OvmfPkg/PlatformPei: Move early GDT into ram when SEV-ES is enabled
UefiCpuPkg: Create an SEV-ES workarea PCD
OvmfPkg: Reserve a page in memory for the SEV-ES usage
OvmfPkg/ResetVector: Add support for a 32-bit SEV check
OvmfPkg/Sec: Add #VC exception handling for Sec phase
OvmfPkg/Sec: Enable cache early to speed up booting
OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with
SEV-ES is enabled
UefiCpuPkg: Add a 16-bit protected mode code segment descriptor
UefiCpuPkg/MpInitLib: Add CPU MP data flag to indicate if SEV-ES is
enabled
UefiCpuPkg: Allow AP booting under SEV-ES
OvmfPkg: Use the SEV-ES work area for the SEV-ES AP reset vector
OvmfPkg: Move the GHCB allocations into reserved memory
UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
MdeModulePkg/MdeModulePkg.dec | 9 +
OvmfPkg/OvmfPkg.dec | 9 +
UefiCpuPkg/UefiCpuPkg.dec | 17 +
OvmfPkg/OvmfPkgIa32.dsc | 6 +
OvmfPkg/OvmfPkgIa32X64.dsc | 6 +
OvmfPkg/OvmfPkgX64.dsc | 6 +
OvmfPkg/OvmfXen.dsc | 1 +
UefiCpuPkg/UefiCpuPkg.dsc | 2 +
UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 2 +
UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc | 2 +
OvmfPkg/OvmfPkgX64.fdf | 9 +
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 2 +
MdePkg/Library/BaseLib/BaseLib.inf | 4 +
OvmfPkg/PlatformPei/PlatformPei.inf | 7 +
.../FvbServicesRuntimeDxe.inf | 2 +
OvmfPkg/ResetVector/ResetVector.inf | 8 +
OvmfPkg/Sec/SecMain.inf | 4 +
.../DxeCpuExceptionHandlerLib.inf | 5 +
.../PeiCpuExceptionHandlerLib.inf | 5 +
.../SecPeiCpuExceptionHandlerLib.inf | 5 +
.../SmmCpuExceptionHandlerLib.inf | 5 +
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 4 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 4 +
UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf | 33 +
.../Core/DxeIplPeim/X64/VirtualMemory.h | 12 +-
MdePkg/Include/Library/BaseLib.h | 31 +
MdePkg/Include/Register/Amd/Fam17Msr.h | 42 +
MdePkg/Include/Register/Amd/Ghcb.h | 136 ++
OvmfPkg/Include/Library/MemEncryptSevLib.h | 12 +
.../QemuFlash.h | 6 +
UefiCpuPkg/CpuDxe/CpuGdt.h | 4 +-
UefiCpuPkg/Include/Library/VmgExitLib.h | 111 ++
.../CpuExceptionHandlerLib/AMDSevVcCommon.h | 26 +
.../CpuExceptionCommon.h | 2 +
UefiCpuPkg/Library/MpInitLib/MpLib.h | 68 +-
.../Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 4 +-
.../Core/DxeIplPeim/X64/DxeLoadFunc.c | 11 +-
.../Core/DxeIplPeim/X64/VirtualMemory.c | 49 +-
MdePkg/Library/BaseLib/Ia32/GccInline.c | 45 +
MdePkg/Library/BaseLib/X64/GccInline.c | 47 +
.../MemEncryptSevLibInternal.c | 75 +-
OvmfPkg/PlatformPei/AmdSev.c | 82 ++
OvmfPkg/PlatformPei/MemDetect.c | 23 +
.../QemuFlash.c | 23 +-
.../QemuFlashDxe.c | 15 +
.../QemuFlashSmm.c | 9 +
OvmfPkg/Sec/SecMain.c | 160 ++-
UefiCpuPkg/CpuDxe/CpuGdt.c | 8 +-
.../CpuExceptionHandlerLib/AMDSevVcHandler.c | 29 +
.../CpuExceptionCommon.c | 2 +-
.../Ia32/ArchAMDSevVcHandler.c | 24 +
.../PeiDxeSmmCpuException.c | 16 +
.../SecPeiCpuException.c | 16 +
.../X64/ArchAMDSevVcHandler.c | 1237 +++++++++++++++++
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 114 +-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 257 +++-
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +
UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c | 249 ++++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 2 +-
MdePkg/Library/BaseLib/Ia32/VmgExit.nasm | 37 +
MdePkg/Library/BaseLib/Ia32/XGetBv.nasm | 31 +
MdePkg/Library/BaseLib/X64/VmgExit.nasm | 32 +
MdePkg/Library/BaseLib/X64/XGetBv.nasm | 34 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 100 ++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 351 ++++-
OvmfPkg/ResetVector/ResetVector.nasmb | 20 +
.../X64/ExceptionHandlerAsm.nasm | 17 +
UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 2 +-
.../Library/MpInitLib/Ia32/MpFuncs.nasm | 15 +
UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 4 +-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 370 ++++-
UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni | 15 +
.../ResetVector/Vtf0/Ia16/Real16ToFlat32.asm | 9 +
73 files changed, 4061 insertions(+), 99 deletions(-)
create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.inf
create mode 100644 MdePkg/Include/Register/Amd/Ghcb.h
create mode 100644 UefiCpuPkg/Include/Library/VmgExitLib.h
create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/AMDSevVcCommon.h
create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/AMDSevVcHandler.c
create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchAMDSevVcHandler.c
create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchAMDSevVcHandler.c
create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.c
create mode 100644 MdePkg/Library/BaseLib/Ia32/VmgExit.nasm
create mode 100644 MdePkg/Library/BaseLib/Ia32/XGetBv.nasm
create mode 100644 MdePkg/Library/BaseLib/X64/VmgExit.nasm
create mode 100644 MdePkg/Library/BaseLib/X64/XGetBv.nasm
create mode 100644 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
create mode 100644 UefiCpuPkg/Library/VmgExitLib/VmgExitLib.uni


Re: [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller

Laszlo Ersek
 

On 03/28/20 21:00, Liran Alon wrote:
Hi,

This series adds driver support for VMware PVSCSI controller.

This controller is supported by VMware and QEMU. This work is part of
the more general agenda of enhancing OVMF boot device support to have
feature parity with SeaBIOS (Which supports booting from VMware PVSCSI).

I pushed a copy of these (v3) patches to https://github.com/nikital/edk2/tree/pvscsi6
The v2 patches can be found at https://github.com/nikital/edk2/tree/pvscsi5
The v1 patches can be found at https://github.com/nikital/edk2/tree/pvscsi4
Merged in commit range 6c1fb56802d5..f34c7645bd87, via
<https://github.com/tianocore/edk2/pull/474>.

Thanks for the contribution!
Laszlo

v2->v3:
* Add function documentation to PvScsiWriteCmdDesc with “@param” to explain DescWords alignment requirement. [Laszlo]
* Also set Packet’s HostAdapterStatus and TargetStatus when returning EFI_BAD_BUFFER_SIZE from PassThru() method. [Liran]
* Add comments explaining why DMA communication buffer fields sizes are defined as they are. [Laszlo]
* Remove unnecessary (UINT64) casts from EFI_PHYSICAL_ADDRESS expressions. [Laszlo]
* Changed HandleResponse() to always copy SenseData based on Response->SenseLen. Not only if ScsiStatus equal to CHECK. [Liran]
* Changed HandleResponse() to update Packet TransferLength only on underrun. [Liran]
* Changed PassThru() to return EFI_STATUS_SUCCESS on device return overrun/underrun. [Laszlo & Liran]
* Removed unnecessary PvScsiAllocatePages(), PvScsiFreePages(), PvScsiMapBuffer() and PvScsiUnmapBuffer() utility functions. [Laszlo]
* Changed PvScsiInitRings() to align PVSCSI_CMD_DESC_SETUP_RINGS command to UINT32 before using it with EfiPciIoWidthFifoUint32. [Laszlo]
* Removed PciIoOperation parameter from PvScsiAllocateSharedPages(). [Laszlo
* Added STATIC_ASSERT() to verify PVSCSI_CMD_DESC_SETUP_RINGS is of size multiple of UINT32 words [Laszlo].
* Added reset device before either freeing PVSCSI rings or DMA communication buffer. [Laszlo]
* Removed unnecessary cast to (VOID **) in call to PvScsiFreeSharedPages() in PvScsiUninit(). [Laszlo]
* Added #include <Library/BaseLib.h> and BaseLib to PvScsiDxe.inf because of RShiftU64() usage. [Laszlo]

v1->v2:
* Removed Nikita’s Reviewed-By tags. [Laszlo]
* Renamed PvScsi.inf to PvScsiDxe.inf and fixed references from all DSC files. [Laszlo]
* Changed “!ifdef $(PVSCSI_ENABLE)” in DSC files to “!if $(PVSCSI_ENABLE) == TRUE”. [Laszlo]
* Fix Identation in various places. [Laszlo]
* Added “#include <Uefi/UefiSpec.h>” for EFI_SYSTEM_TABLE. [Laszlo]
* Fix various typos. [Laszlo]
* Made “STATIC” on same line of object delcerations. [Laszlo]
* Added Laszlo’s Reviewed-by tags on some patches. [Laszlo]
* Added missing spaces before “(“ on various function calls. [Laszlo]
* Added PvScsi.h header file to INF [Sources] section. [Laszlo]
* Changed [Protocols] section in INF file to be lexicographically sorted. [Laszlo]
* Changed [PCDs] section in INF file to be lexigraphically sorted. [Laszlo]
* Fixed function comments blocks to be “/** **/” instead of “//” style. [Laszlo]
* Changed PvScsiGetTargetLun() to ZeroMem() all target bytes except first one. [Laszlo]
* Replaced “IOSpace” with “MMIO-Space” in comments. [Laszlo]
* Changed enums to match EDK2 coding convention. [Laszlo]
* Use PCI_BAR_IDX0 instead of hard-coded 0. [Laszlo]
* Use EFI_PAGES_TO_SIZE() instead of manually multiplying with EFI_PAGE_SIZE. [Laszlo]
* Use RShiftU64() to shift UINT64 vars. [Laszlo]
* Changed ReqNumEntries var to UINT32 and shift to use “1U <<” instead of “1 <<”. [Laszlo]
* Changed condition on flag (In PvScsiWaitForRequestCompletion()) to be a boolean expression. [Laszlo]
* Replaced “FakeHostAdapterError” label with a utility function. [Laszlo]
* Added debug message to PvScsiExitBoot() to assist debugging. [Laszlo]
* Fixed resource management to make each function either completely succeed or completely fail and free all resources. [Laszlo]
* Changed PvScsiWriteCmdDesc() to use EfiPciIoWidthFifoUint32. [Laszlo]
* Changed PvScsiWriteCmdDesc() prototype to make clear it descriptor must be an array of words. [Laszlo]




Re: [PATCH v1 1/1] INF Spec: Add file dependency to [Sources] syntax

PierreGondois
 

The Bugzilla ticket number was erroneous in the v1. The v2 is available at: https://edk2.groups.io/g/devel/message/56663
Regards,
Pierre

-----Original Message-----
From: PierreGondois <pierre.gondois@arm.com>
Sent: 30 March 2020 16:43
To: devel@edk2.groups.io
Cc: Pierre Gondois <Pierre.Gondois@arm.com>; liming.gao@intel.com; sami.mujawar@arm.org; nd <nd@arm.com>
Subject: [PATCH v1 1/1] INF Spec: Add file dependency to [Sources] syntax

From: Pierre Gondois <Pierre.Gondois@arm.com>

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

When building an edk2 module, a C file was including a .hex file generated by the compilation of an ASL file.
To describe this dependency between an ASL file and a C file, the edk2 patch,
- named "BaseTools: Build ASL files before C files",
- discussed at: https://edk2.groups.io/g/devel/message/52550
has been created.
This patch allows to establish build dependencies in the [Sources] section, between files that are not of the same language.
E.g.:
[Sources]
FileName1.X
FileName2.Y : FileName1.X
FileName3.Z : FileName1.X FileName2.Y

Here:
* FileName1.X will be built prior to FileName2.Y.
* FileName1.X and FileName2.Y will be built prior to
FileName3.Z.

This patch updates the Inf specification accordingly.

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

The changes can be seen at https://github.com/PierreARM/edk2-InfSpecification/tree/Bugzilla_2464_Enable_Build_Dependencies_v1

Notes:
v1:
- Enable build dependencies in the [Sources] section

2_inf_overview/25_[sources]_section.md | 12 ++++++++++++
3_edk_ii_inf_file_format/32_component_inf_definition.md | 3 +++
3_edk_ii_inf_file_format/39_[sources]_sections.md | 6 ++++--
README.md | 1 +
4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/2_inf_overview/25_[sources]_section.md b/2_inf_overview/25_[sources]_section.md
index 54757e61e37268eed293a5288e607cf2c7cfacf6..5b9f0a8395ef2be4497d99197dc695625d841830 100644
--- a/2_inf_overview/25_[sources]_section.md
+++ b/2_inf_overview/25_[sources]_section.md
@@ -2,6 +2,7 @@
2.5 [Sources] Section

Copyright (c) 2007-2019, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2020, ARM Limited. All rights reserved.<BR>

Redistribution and use in source (original document form) and 'compiled'
forms (converted to PDF, epub, HTML and other formats) with or without @@ -94,6 +95,17 @@ The following is an example for sources sections.

```

+The following example depicts the syntax to establish dependencies
+between files of different source types. As shown in the example below,
+Dsdt.asl will be compiled before DadtHandler.c:
+
+```ini
+[Sources.common]
+ DsdtHandler.c : Dsdt.asl
+ DsdtHandler.h
+ Dsdt.asl
+```
+
All Unicode files must be listed in the source section. If a Unicode file, `A.uni`, has the statement: `#include B.uni`, and `B.uni` has a statement:
`#include C.uni`, both `B.uni` and `C.uni` files must be listed in the INF diff --git a/3_edk_ii_inf_file_format/32_component_inf_definition.md b/3_edk_ii_inf_file_format/32_component_inf_definition.md
index 164771cb4cfff6e81fbf762a67ff741c190cecde..d776714c24c0baf2348f53dc2576c9feb6f3cb5e 100644
--- a/3_edk_ii_inf_file_format/32_component_inf_definition.md
+++ b/3_edk_ii_inf_file_format/32_component_inf_definition.md
@@ -2,6 +2,7 @@
3.2 Component INF Definition

Copyright (c) 2007-2019, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2020, ARM Limited. All rights reserved.<BR>

Redistribution and use in source (original document form) and 'compiled'
forms (converted to PDF, epub, HTML and other formats) with or without @@ -133,6 +134,8 @@ The following are common definitions used by multiple section types.
<Eq> ::= <TS> "=" <TS>
<FieldSeparator> ::= "|"
<FS> ::= <TS> <FieldSeparator> <TS>
+<SrcDepSeperator> ::= ":"
+<DepS> ::= <TS> <SrcDepSeperator> <TS>
<Wildcard> ::= "*"
<CommaSpace> ::= "," <Space>*
<Cs> ::= "," <Space>*
diff --git a/3_edk_ii_inf_file_format/39_[sources]_sections.md b/3_edk_ii_inf_file_format/39_[sources]_sections.md
index 810995df26ba409ca2cf3ebe6238aa5d55cf81f1..ac966425101fd44a57b09d36f95a0f732eab1c59 100644
--- a/3_edk_ii_inf_file_format/39_[sources]_sections.md
+++ b/3_edk_ii_inf_file_format/39_[sources]_sections.md
@@ -2,6 +2,7 @@
3.9 [Sources] Sections

Copyright (c) 2007-2019, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2020, ARM Limited. All rights reserved.<BR>

Redistribution and use in source (original document form) and 'compiled'
forms (converted to PDF, epub, HTML and other formats) with or without @@ -74,7 +75,8 @@ This section is not valid for a generated "As Built" binary INF file.
<Options> ::= <FS> [<Family>] [<opt1>]
<opt1> ::= <FS> [<TagName>] [<opt2>]
<opt2> ::= <FS> [<ToolCode>] [<opt3>]
-<opt3> ::= <FS> [<FeatureFlagExpress>]
+<opt3> ::= <FS> [<FeatureFlagExpress>] [<opt4>]
+<opt4> ::= <DepS> <FileNameDependency>+
<Family> ::= {"MSFT"} {"GCC"} {"INTEL"} {<Wildcard>}
<TagName> ::= {<ToolWord>} {"*"}
<ToolCode> ::= _CommandCode_
@@ -83,7 +85,7 @@ This section is not valid for a generated "As Built" binary INF file.

#### Parameters

-**_Filename_**
+**_Filename_, _FileNameDependency_**

Paths listed in the filename elements of the `[Sources]` section must be relative to the directory the INF file resides in. Use of "..", "." and "../"
diff --git a/README.md b/README.md
index 60fba19fd67fd8d3dd33199de23f9bfe20aea7c9..4f771fc0f6e4ff516be95b1879d58329ab3bbecc 100644
--- a/README.md
+++ b/README.md
@@ -203,3 +203,4 @@ Copyright (c) 2007-2017, Intel Corporation. All rights reserved.
| | [#1162](https://bugzilla.tianocore.org/show_bug.cgi?id=1162) Correct the item in Table 1 to align with 3.4 section | |
| 1.28 | [#1453](https://bugzilla.tianocore.org/show_bug.cgi?id=1453) Update INF spec to remove EDK related contents | Mar 2019 |
| 1.29 | [#1952](https://bugzilla.tianocore.org/show_bug.cgi?id=1952) Add new MODULE_TYPE HOST_APPLICATION | July 2019 |
+| | [#2646](https://bugzilla.tianocore.org/show_bug.cgi?id=2646) Add file dependency to [Sources] syntax | |
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


Re: [PATCH v3 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices()

Laszlo Ersek
 

On 03/28/20 21:00, Liran Alon wrote:
This causes the device to forget about the request/completion rings.
We allocated said rings in EfiBootServicesData type memory, and code
executing after ExitBootServices() is permitted to overwrite it.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 43 +++++++++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index da3535c75220..d7f0d3c8790c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -1221,6 +1221,31 @@ PvScsiUninit (
PvScsiRestorePciAttributes (Dev);
}

+/**
+ Event notification called by ExitBootServices()
+**/
+STATIC
+VOID
+EFIAPI
+PvScsiExitBoot (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ PVSCSI_DEV *Dev;
+
+ Dev = Context;
+ DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
+
+ //
+ // Reset the device to stop device usage of the rings.
+ //
+ // We allocated said rings in EfiBootServicesData type memory, and code
+ // executing after ExitBootServices() is permitted to overwrite it.
+ //
+ PvScsiResetAdapter (Dev);
+}
+
My R-b stands.

Thanks
Laszlo

//
// Driver Binding
//
@@ -1314,6 +1339,17 @@ PvScsiDriverBindingStart (
goto ClosePciIo;
}

+ Status = gBS->CreateEvent (
+ EVT_SIGNAL_EXIT_BOOT_SERVICES,
+ TPL_CALLBACK,
+ &PvScsiExitBoot,
+ Dev,
+ &Dev->ExitBoot
+ );
+ if (EFI_ERROR (Status)) {
+ goto UninitDev;
+ }
+
//
// Setup complete, attempt to export the driver instance's PassThru interface
//
@@ -1325,11 +1361,14 @@ PvScsiDriverBindingStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto UninitDev;
+ goto CloseExitBoot;
}

return EFI_SUCCESS;

+CloseExitBoot:
+ gBS->CloseEvent (Dev->ExitBoot);
+
UninitDev:
PvScsiUninit (Dev);

@@ -1384,6 +1423,8 @@ PvScsiDriverBindingStop (
return Status;
}

+ gBS->CloseEvent (Dev->ExitBoot);
+
PvScsiUninit (Dev);

gBS->CloseProtocol (
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 02feac734743..544359ebc05c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -51,6 +51,7 @@ typedef struct {
typedef struct {
UINT32 Signature;
EFI_PCI_IO_PROTOCOL *PciIo;
+ EFI_EVENT ExitBoot;
UINT64 OriginalPciAttributes;
PVSCSI_RING_DESC RingDesc;
PVSCSI_DMA_BUFFER *DmaBuf;


Re: [PATCH v3 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

Laszlo Ersek
 

On 03/28/20 21:00, Liran Alon wrote:
Implement EXT_SCSI_PASS_THRU.PassThru().

Machines should be able to boot after this commit.
Tested with Ubuntu 16.04 guest.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/OvmfPkg.dec | 6 +
OvmfPkg/PvScsiDxe/PvScsi.c | 456 +++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 +-
4 files changed, 465 insertions(+), 3 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>