Date   

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

Ard Biesheuvel
 

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

@Rebecca - Agree. I need to package up a newer version. I have treated this as lower priority. Is there a feature you need or just best practices on keeping current?
@Ard - Padding in python is easy. I just need to understand the requirements.
OK, in that case, please switch to a single -pflash argument from
-bios, but pad the QEMU_EFI.fd file to exactly 64 MiB first. I also
noticed that you are logging to a file for OVMF - you might want to do
the same for ArmVirtQemu, by using "-serial file:" + QemuLogFile
instead of 'stdio'


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

Sean
 

@Rebecca - Agree.  I need to package up a newer version.  I have treated this as lower priority.  Is there a feature you need or just best practices on keeping current? 
@Ard - Padding in python is easy.  I just need to understand the requirements. 
@Mike - I would like to get to read/write filesystem for Target based tests so that we don't have to reinvent yet another way to collect results (especially one as error prone as sniffing logs).  But lets get basic CI with read-only functionality working and checked in first.  As it looks like in the last 8 hours another breaking change has been checked in.  After rebase to top of edk2 master. 


NFO - "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.24.28314\bin\Hostx86\x64\link.exe" /OUT:d:\a\1\s\Build\Ovmf3264\RELEASE_VS2019\X64\OvmfPkg\QemuKernelLoaderFsDxe\QemuKernelLoaderFsDxe\DEBUG\QemuKernelLoaderFsDxe.dll /NOLOGO /NODEFAULTLIB /IGNORE:4001 /IGNORE:4281 /IGNORE:4254 /OPT:REF /OPT:ICF=10 /MAP /ALIGN:32 /SECTION:.xdata,D /SECTION:.pdata,D /Machine:X64 /LTCG /DLL /ENTRY:_ModuleEntryPoint /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /BASE:0 /DRIVER /MERGE:.rdata=.data  @d:\a\1\s\Build\Ovmf3264\RELEASE_VS2019\X64\OvmfPkg\QemuKernelLoaderFsDxe\QemuKernelLoaderFsDxe\OUTPUT\static_library_files.lst
INFO - PvScsi.c
INFO - Generating code
INFO - d:\a\1\s\OvmfPkg\PvScsiDxe\PvScsi.c(459): error C2220: the following warning is treated as an error
INFO - d:\a\1\s\OvmfPkg\PvScsiDxe\PvScsi.c(459): warning C4244: '=': conversion from 'const UINT16' to 'UINT8', possible loss of data
INFO - Finished generating code
INFO - NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.24.28314\bin\Hostx86\x64\cl.exe"' : return code '0x2'
INFO - Stop.
INFO - "GenFw" -e DXE_DRIVER -o d:\a\1\s\Build\Ovmf3264\RELEASE_VS2019\X64\OvmfPkg\QemuKernelLoaderFsDxe\QemuKernelLoaderFsDxe\OUTPUT\QemuKernelLoaderFsDxe.efi d:\a\1\s\Build\Ovmf3264\RELEASE_VS2019\X64\OvmfPkg\QemuKernelLoaderFsDxe\QemuKernelLoaderFsDxe\DEBUG\QemuKernelLoaderFsDxe.dll
INFO - 
INFO - 
INFO - build.py...
INFO -  : error 7000: Failed to execute command
INFO - C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.24.28314\bin\Hostx86\x86\nmake.exe /nologo tbuild [d:\a\1\s\Build\Ovmf3264\RELEASE_VS2019\X64\OvmfPkg\PvScsiDxe\PvScsiDxe]
INFO - 
INFO - 
INFO - build.py...
INFO -  : error F002: Failed to build module
INFO - d:\a\1\s\OvmfPkg\PvScsiDxe\PvScsiDxe.inf [X64, VS2019, RELEASE]
INFO - 
INFO - - Failed -


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

Ard Biesheuvel
 

On Mon, 30 Mar 2020 at 23:29, Matthew Carlson via Groups.Io
<macarl=microsoft.com@groups.io> wrote:

So it's not required by OpenSSL, it's required by the compiler whenever floating point is used, which can be in multiple places. For example, this is used in mu_plus (the Microsoft UEFI value add to EDK2) by the OnScreenKeyboard driver as well as the UiToolKit driver. If you happen to use BaseCryptLib or the intrinsic in a driver that happens to need crypto as well, it can break due to multiple.

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

However, given that we are talking about static libraries here, adding
a source file that *only* defines __fltused (and nothing else) to any
library should also work, as the resulting object file will only be
incorporated by the linker if it is needed to satisfy a symbol
dependency, and so it can never cause a conflict if it is the only
symbol in the object.


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

Michael D Kinney
 

Hi Laszlo,

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

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

I have not had good experiences trying to build virtual
disks, provision them, map them into QEMU, and remount
from host to collect test results. Slow operations and
cumbersome to mount/unmount from the host (at least from
Windows environments).

Mike

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

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

Ard/Laszlo or anyone familiar with QEMU.

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

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

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

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

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

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

Thanks
Laszlo



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

Matthew Carlson
 

So it's not required by OpenSSL, it's required by the compiler whenever floating point is used, which can be in multiple places. For example, this is used in mu_plus (the Microsoft UEFI value add to EDK2) by the OnScreenKeyboard driver as well as the UiToolKit driver. If you happen to use BaseCryptLib or the intrinsic in a driver that happens to need crypto as well, it can break due to multiple.

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


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

Ard Biesheuvel
 

On Mon, 30 Mar 2020 at 22:56, Laszlo Ersek <lersek@redhat.com> wrote:

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.
truncate should work on Linux, but I have no idea how to pad an image
to a certain size on Windows, and I think the idea was to enable both?

In any case, I don't feel as strongly about this as Laszlo does: even
though -bios really shouldn't be used when you are actually installing
an OS into the VM, I don't think it is inappropriate for booting into
a shell and nothing else. Perhaps we could annotate the scripts in a
way that discourages people from adopting it?

Or if there is an easy way to make this work on both Windows and Linux
using -pflash, I obviously wouldn't mind. I just don't feel it is
essential.


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

Rebecca Cran
 

On 3/30/20 3:03 PM, Sean via Groups.Io wrote:

Laszlo/Ard -

I have cleaned up git history and pushed to https://github.com/spbrogan/edk2 master.

If you want you can create a PR against that branch and it should kick OVMF, Emulator, and ArmVirt to build and run.

Looking for feedback.
The 20190215 release of iasl/acpica-tools is pretty old (https://acpica.org/downloads). Should it be bumped to something more recent?


--
Rebecca Cran


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

Laszlo Ersek
 

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

Ard/Laszlo or anyone familiar with QEMU.

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

1. How do i programmatically exit the emulator. Seems like uefi shell > reset just reboots. Other ideas?
'reset' is supposed to do that. Use 'reset -s' to kill the VM

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

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

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

Thanks
Laszlo


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

Sean
 

Laszlo/Ard - 

I have cleaned up git history and pushed to https://github.com/spbrogan/edk2 master.  

If you want you can create a PR against that branch and it should kick OVMF, Emulator, and ArmVirt to build and run.  

Looking for feedback.  

@Laszlo - I followed Ard's parameters but feel free to make a PR that changes it to not use "-bios".  :) 


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