Re: [edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
On September 11, 2021 9:16 AM, Erdem Aktas wrote:
>
> On Thu, Aug 12, 2021 at 2:57 PM Min Xu wrote:
> > +UINT8
> > +EFIAPI
> > +TdMmioRead8 (
> > + IN UINTN Address
> > + )
> > +{
> > + UINT64 Value;
> > + UINT64 Status;
> > +
> > + Address |= TdSharedPageMask ();
>
> Why is the SharedBit set? VMM does not care if the sharedbit is set.
> Actually it should not even be aware of it.
>
That is because GPA for MMIO region that VMM emulates must be shared region.
i.e. shared bit must be set.
See Section 12.3 in below link.
https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
>
> > + MemoryFence ();
> > + Status = TdVmCall (TDVMCALL_MMIO, TDVMCALL_ACCESS_SIZE_1,
> > + TDVMCALL_ACCESS_READ, Address, 0, ); if (Status != 0) {
> So for some reason, MMIO read fails, we are doing a memory read. Why
> should an MMIO read fail? Could you elaborate which use case this covers?
If invalid operands provided by TD, e.g., MMIO address,
TDG.VP.VMCALL_INVALID_OPERAND is returned.
See Section 3.7 Table 3-21 in below link:
https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
>
Thanks!
Min
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81207): https://edk2.groups.io/g/devel/message/81207
Mute This Topic: https://groups.io/mt/84837896/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
On Thu, Aug 12, 2021 at 2:57 PM Min Xu wrote:
> +UINT8
> +EFIAPI
> +TdMmioRead8 (
> + IN UINTN Address
> + )
> +{
> + UINT64 Value;
> + UINT64 Status;
> +
> + Address |= TdSharedPageMask ();
Why is the SharedBit set? VMM does not care if the sharedbit is set.
Actually it should not even be aware of it.
> + MemoryFence ();
> + Status = TdVmCall (TDVMCALL_MMIO, TDVMCALL_ACCESS_SIZE_1,
> TDVMCALL_ACCESS_READ, Address, 0, );
> + if (Status != 0) {
So for some reason, MMIO read fails, we are doing a memory read. Why
should an MMIO read fail? Could you elaborate which use case this
covers?
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80524): https://edk2.groups.io/g/devel/message/80524
Mute This Topic: https://groups.io/mt/84837896/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
Hi, > > One more question: How does this align with the WorkArea changes posted > > yesterday? The WorkArea gets a mode field for SEV / TDX / normal, so I > > think > > you should be able to use that instead of invoking cpuid each time you need > > to know whenever tdx is active or not. > We don't want to make the TdxProbeLib depend on the WorkArea. Why? > CPUID(0x21) makes TdxProbeLib less dependency. For Intel TDX the > WorkArea is designed to be used in ResetVector phase. I don't see why the dependency is a problem. TDX-enabled builds need both TdxProbeLib / TdxLib and WorkArea anyway. Each cpuid instruction is a vmexit, and the code invokes cpuid twice for each io / mmio access. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#79625): https://edk2.groups.io/g/devel/message/79625 Mute This Topic: https://groups.io/mt/84837896/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
On August 19, 2021 2:31 PM, Gerd Hoffmann wrote: > > > IIRC some of the TDX features require a separate firmware binary. > > > So, if we need a separate binary anyway at some point in the future, > > > isn't it simpler then to use a separate firmware binary right from the > > > start? > > > > > > You can simply add a Tdx-specific variant of the library > > > (BaseIoLibIntrinsicTdx.inf) and switch at compile time instead of > > > having runtime switches all over the place. > > > > > TDVF has 2 Config for upstream. See > > https://edk2.groups.io/g/devel/message/76367 > > Config-A merge the *basic* TDVF features to existing OvmfX64Pkg.dsc. > (Align with existing SEV). > > Hmm, so we'll have two variants with two feature sets. > > One more question: How does this align with the WorkArea changes posted > yesterday? The WorkArea gets a mode field for SEV / TDX / normal, so I think > you should be able to use that instead of invoking cpuid each time you need > to know whenever tdx is active or not. We don't want to make the TdxProbeLib depend on the WorkArea. CPUID(0x21) makes TdxProbeLib less dependency. For Intel TDX the WorkArea is designed to be used in ResetVector phase. > Thanks! Min -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#79571): https://edk2.groups.io/g/devel/message/79571 Mute This Topic: https://groups.io/mt/84837896/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
Hi, > > IIRC some of the TDX features require a separate firmware binary. So, if we > > need a separate binary anyway at some point in the future, isn't it simpler > > then > > to use a separate firmware binary right from the start? > > > > You can simply add a Tdx-specific variant of the library > > (BaseIoLibIntrinsicTdx.inf) and switch at compile time instead of having > > runtime > > switches all over the place. > > > TDVF has 2 Config for upstream. See > https://edk2.groups.io/g/devel/message/76367 > Config-A merge the *basic* TDVF features to existing OvmfX64Pkg.dsc. (Align > with existing SEV). Hmm, so we'll have two variants with two feature sets. One more question: How does this align with the WorkArea changes posted yesterday? The WorkArea gets a mode field for SEV / TDX / normal, so I think you should be able to use that instead of invoking cpuid each time you need to know whenever tdx is active or not. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#79550): https://edk2.groups.io/g/devel/message/79550 Mute This Topic: https://groups.io/mt/84837896/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
On August 17, 2021 4:38 PM, Gerd Hoffmann wrote: > > Hi, > > > In the I/O functions of above files, if IsTdxGuest() returns TRUE, > > then Td I/O routine is called, otherwise the legacy I/O routine is called. > > Td I/O routines are declared in IoLibTdx.h and implemented in > > IoLibInternalTdx.c. > > Sorry, I'm a bit late to the party, but what is the overall long plan here? > Yes there are discussions about the TDVF (Trust Domain Virtual Firmware). https://edk2.groups.io/g/devel/topic/83283616#76022 The design slides and recorded meeting are in below link: https://edk2.groups.io/g/devel/files/Designs/2021/0611 > > IIRC some of the TDX features require a separate firmware binary. So, if we > need a separate binary anyway at some point in the future, isn't it simpler > then > to use a separate firmware binary right from the start? > > You can simply add a Tdx-specific variant of the library > (BaseIoLibIntrinsicTdx.inf) and switch at compile time instead of having > runtime > switches all over the place. > TDVF has 2 Config for upstream. See https://edk2.groups.io/g/devel/message/76367 Config-A merge the *basic* TDVF features to existing OvmfX64Pkg.dsc. (Align with existing SEV). OvmfX64Pkg.dsc includes SEV/TDX/normal OVMF basic boot capability. The final binary can run on SEV/TDX/normal OVMF So we have to probe the Td guest in run-time and switch to the corresponding I/O routine. The solution of using a separate firmware binary is not feasible in this situation. Thanks. Min -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#79469): https://edk2.groups.io/g/devel/message/79469 Mute This Topic: https://groups.io/mt/84837896/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
Hi, > In the I/O functions of above files, if IsTdxGuest() returns TRUE, then > Td I/O routine is called, otherwise the legacy I/O routine is called. > Td I/O routines are declared in IoLibTdx.h and implemented in > IoLibInternalTdx.c. Sorry, I'm a bit late to the party, but what is the overall long plan here? IIRC some of the TDX features require a separate firmware binary. So, if we need a separate binary anyway at some point in the future, isn't it simpler then to use a separate firmware binary right from the start? You can simply add a Tdx-specific variant of the library (BaseIoLibIntrinsicTdx.inf) and switch at compile time instead of having runtime switches all over the place. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#79414): https://edk2.groups.io/g/devel/message/79414 Mute This Topic: https://groups.io/mt/84837896/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH 07/23] MdePkg: Update BaseIoLibIntrinsicSev to support Tdx
RFC： https://bugzilla.tianocore.org/show_bug.cgi?id=3429
Intel TDX architecture does not prescribe a specific software convention
to perform I/O from the guest TD. Guest TD providers have many choices to
provide I/O to the guest. The common I/O models are emulated devices,
para-virtualized devices, SRIOV devices and Direct Device assignments.
TDVF chooses para-virtualized I/O (Choice-A) which use the TDG.VP.VMCALL
function to invoke the funtions provided by the host VMM to perform I/O.
Another choice (Choice-B) is the emulation performed by the #VE handler.
There are 2 benefits of para-virtualized I/O:
1. Performance.
VMEXIT/VMENTRY is skipped so that the performance is better than #VE
handler.
2. De-couple with #VE handler.
Choice-B depends on the #VE handler which means I/O is not available
until #VE handler is installed. For example, in PEI phase #VE handler
is installed in CpuMpPei, while communication with Qemu (via I/O port)
happen earlier than it.
BaseIoLibIntrinsicSev.inf is the IoLib used by OvmfPkg. TDVF updates
BaseIoLibIntrinsicSev to support I/O in Td guest. Below files are updated
to support I/O in Td guest.
- IoLib.c
- IoLibGcc.c
- IoLibMsc.c
- X64/IoFifoSev.nasm
In the I/O functions of above files, if IsTdxGuest() returns TRUE, then
Td I/O routine is called, otherwise the legacy I/O routine is called.
Td I/O routines are declared in IoLibTdx.h and implemented in
IoLibInternalTdx.c.
BaseIoLibIntrinsic.inf is the IoLib used by other packages. It will not
support I/O in Td guest. But some files are shared between
BaseIoLibIntrinsic and BaseIoLibIntrinsicSev (IoLib.c is the example). So
IoLibInternalTdxNull.c is created which holds the null stub of the Td I/O
routines. IoLibInternalTdxNull.c is included in BaseIoLibIntrinsic.inf.
BaseIoLibIntrinsic.inf doesn't import TdxProbeLib/TdxLib so that the Pkgs
which include BaseIoLibIntrinsic.inf need not include TdxProbeLib/TdxLib.
BaseIoLibIntrinsicArmVirt.inf is not touched because it shares no files
with BaseIoLibIntrinsicSev.
Cc: Michael D Kinney
Cc: Liming Gao
Cc: Zhiguang Liu
Cc: Brijesh Singh
Cc: Erdem Aktas
Cc: James Bottomley
Cc: Jiewen Yao
Cc: Tom Lendacky
Signed-off-by: Min Xu
---
.../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf | 2 +
.../BaseIoLibIntrinsicSev.inf | 6 +-
MdePkg/Library/BaseIoLibIntrinsic/IoLib.c | 97 ++-
MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c | 49 +-
.../BaseIoLibIntrinsic/IoLibInternalTdx.c | 690 ++
.../BaseIoLibIntrinsic/IoLibInternalTdxNull.c | 499 +
MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c | 73 +-
MdePkg/Library/BaseIoLibIntrinsic/IoLibTdx.h | 411 +++
.../BaseIoLibIntrinsic/X64/IoFifoSev.nasm | 133
9 files changed, 1911 insertions(+), 49 deletions(-)
create mode 100644 MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c
create mode 100644 MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdxNull.c
create mode 100644 MdePkg/Library/BaseIoLibIntrinsic/IoLibTdx.h
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
index 97eeada0656e..27b15d9ae256 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+++ b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
@@ -34,6 +34,8 @@
IoLibMmioBuffer.c
BaseIoLibIntrinsicInternal.h
IoHighLevel.c
+ IoLibInternalTdxNull.c
+ IoLibTdx.h
[Sources.IA32]
IoLibGcc.c| GCC
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
index 34f9d1d1062f..53ff8d6fd37d 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
+++ b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
@@ -30,17 +30,20 @@
IoLibMmioBuffer.c
BaseIoLibIntrinsicInternal.h
IoHighLevel.c
+ IoLibTdx.h
[Sources.IA32]
IoLibGcc.c| GCC
IoLibMsc.c| MSFT
IoLib.c
+ IoLibInternalTdxNull.c
Ia32/IoFifoSev.nasm
[Sources.X64]
IoLibGcc.c| GCC
IoLibMsc.c| MSFT
IoLib.c
+ IoLibInternalTdx.c
X64/IoFifoSev.nasm
[Packages]
@@ -50,4 +53,5 @@
DebugLib
BaseLib
RegisterFilterLib
-
+ TdxLib
+ TdxProbeLib
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
b/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
index d0d7044f0901..68298749ee56 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
+++ b/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
@@ -7,6 +7,7 @@
**/
#include "BaseIoLibIntrinsicInternal.h"
+#include "IoLibTdx.h"
/**
Reads a 64-bit I/O port.
@@ -70,6 +71,8 @@ IoWrite64 (
If 8-bit MMIO register operations are not supported, then ASSERT().
+ For Td guest TDVMCALL_MMIO is invoked to read MMIO registers.
+
@param Address The MMIO register to read.
@return The value read.
@@ -86,9 +89,13 @@ MmioRead8 (
Flag = FilterBeforeMmIoRead (FilterWidth8, Address, );
if (Flag) {
-MemoryFence ();
-
