Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/ResetVector: send post codes to qemu debug console
On Fri, Jan 26, 2024 at 8:16 AM Tom Lendacky wrote:
>
> On 1/26/24 08:29, Gerd Hoffmann wrote:
> > Neat when doing ResetVector coding.
> > Incompatible with TDX and SEV, therefore not enabled by default.
> >
> > Signed-off-by: Gerd Hoffmann
>
> Acked-by: Tom Lendacky
>
Acked-by: Erdem Aktas
>
> > ---
> > OvmfPkg/ResetVector/QemuDebugCon.asm | 35 +++
> > OvmfPkg/ResetVector/ResetVector.nasmb | 4 +++
> > 2 files changed, 39 insertions(+)
> > create mode 100644 OvmfPkg/ResetVector/QemuDebugCon.asm
> >
> > diff --git a/OvmfPkg/ResetVector/QemuDebugCon.asm
> > b/OvmfPkg/ResetVector/QemuDebugCon.asm
> > new file mode 100644
> > index ..e385ca1be83a
> > --- /dev/null
> > +++ b/OvmfPkg/ResetVector/QemuDebugCon.asm
> > @@ -0,0 +1,35 @@
> > +;--
> > +; @file
> > +; qemu debug console support macros (based on serial port macros)
> > +;
> > +; Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
> > +; SPDX-License-Identifier: BSD-2-Clause-Patent
> > +;
> > +;--
> > +
> > +%macro debugShowCharacter 1
> > +mov dx, 0x402
> > +mov al, %1
> > +out dx, al
> > +%endmacro
> > +
> > +%macro debugShowHexDigit 1
> > + %if (%1 < 0xa)
> > +debugShowCharacter BYTE ('0' + (%1))
> > + %else
> > +debugShowCharacter BYTE ('a' + ((%1) - 0xa))
> > + %endif
> > +%endmacro
> > +
> > +%macro debugShowPostCode 1
> > +debugShowHexDigit (((%1) >> 4) & 0xf)
> > +debugShowHexDigit ((%1) & 0xf)
> > +debugShowCharacter `\r`
> > +debugShowCharacter `\n`
> > +%endmacro
> > +
> > +BITS16
> > +
> > +%macro debugInitialize 0
> > +; not required
> > +%endmacro
> > diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
> > b/OvmfPkg/ResetVector/ResetVector.nasmb
> > index 5832aaa8abf7..69ce43ef6a96 100644
> > --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> > @@ -40,6 +40,10 @@
> > %include "Port80Debug.asm"
> > %elifdef DEBUG_SERIAL
> > %include "SerialDebug.asm"
> > +%elif 0
> > +; Set ^ this to 1 to enable postcodes on the qemu debug console.
> > +; Disabled by default because it is incompatible with SEV and TDX.
> > + %include "QemuDebugCon.asm"
> > %else
> > %include "DebugDisabled.asm"
> > %endif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114646): https://edk2.groups.io/g/devel/message/114646
Mute This Topic: https://groups.io/mt/103976858/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] OvmfPkg/ResetVector: send post codes to qemu debug console
Same for TDX, I did not run it but it should cause failure
as debugShowPostCode is called OvmfPkg/ResetVector/Ia32/IntelTdx.asm before
actually the #VE handlers are installed.
-Erdem
On Wed, Jan 24, 2024 at 11:55 AM Tom Lendacky
wrote:
> On 1/24/24 13:24, Tom Lendacky wrote:
> > On 1/24/24 10:47, Laszlo Ersek wrote:
> >> On 1/24/24 16:31, Gerd Hoffmann wrote:
> >>> Neat when doing ResetVector coding.
> >>>
> >>> Signed-off-by: Gerd Hoffmann
> >>> ---
> >>> OvmfPkg/ResetVector/DebugCon.asm | 43
> +++
> >>> OvmfPkg/ResetVector/ResetVector.nasmb | 2 +-
> >>> 2 files changed, 44 insertions(+), 1 deletion(-)
> >>> create mode 100644 OvmfPkg/ResetVector/DebugCon.asm
> >>>
> >>> diff --git a/OvmfPkg/ResetVector/DebugCon.asm
> >>> b/OvmfPkg/ResetVector/DebugCon.asm
> >>> new file mode 100644
> >>> index ..9c57d1a52c75
> >>> --- /dev/null
> >>> +++ b/OvmfPkg/ResetVector/DebugCon.asm
> >>> @@ -0,0 +1,43 @@
> >>>
> +;--
> >>> +; @file
> >>> +; qemu debug console support macros (based on serial port macros)
> >>> +;
> >>> +; Copyright (c) 2008 - 2018, Intel Corporation. All rights
> reserved.
> >>> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> >>> +;
> >>>
> +;--
> >>> +
> >>> +%macro outToDebugPort 1
> >>> +mov dx, 0x402
> >>> +mov al, %1
> >>> +out dx, al
> >>> +%endmacro
> >>> +
> >>> +%macro debugShowCharacter 1
> >>> +outToDebugPort %1
> >>> +%endmacro
> >>> +
> >>> +%macro debugShowHexDigit 1
> >>> + %if (%1 < 0xa)
> >>> +debugShowCharacter BYTE ('0' + (%1))
> >>> + %else
> >>> +debugShowCharacter BYTE ('a' + ((%1) - 0xa))
> >>> + %endif
> >>> +%endmacro
> >>> +
> >>> +%macro debugNewline 0
> >>> +debugShowCharacter `\r`
> >>> +debugShowCharacter `\n`
> >>> +%endmacro
> >>> +
> >>> +%macro debugShowPostCode 1
> >>> +debugShowHexDigit (((%1) >> 4) & 0xf)
> >>> +debugShowHexDigit ((%1) & 0xf)
> >>> +debugNewline
> >>> +%endmacro
> >>> +
> >>> +BITS16
> >>> +
> >>> +%macro debugInitialize 0
> >>> +; not required
> >>> +%endmacro
> >>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
> >>> b/OvmfPkg/ResetVector/ResetVector.nasmb
> >>> index 5832aaa8abf7..f1655ddfcde3 100644
> >>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> >>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> >>> @@ -41,7 +41,7 @@
> >>> %elifdef DEBUG_SERIAL
> >>> %include "SerialDebug.asm"
> >>> %else
> >>> - %include "DebugDisabled.asm"
> >>> + %include "DebugCon.asm"
> >>> %endif
> >>> %include "Ia32/SearchForBfvBase.asm"
> >>
> >> (1) How much output does this produce? Trapping to QEMU for every single
> >> character written to the debug console is very slow. If it only produces
> >> a few bytes, that should be OK.
> >>
> >> (2) debugInitialize could actually be put to use; the presence of the
> >> QEMU debug console is detectable. We'd need to allocate a single byte
> >> somewhere, detect the console in debugInitialize, save the result in
> >> that byte, then check the byte in debugShowPostCode. Eliminates even
> >> those few (?) traps if there is no debug console configured.
> >>
> >> (3) I'm already hating myself for asking about this, but... is there a
> >> way for only customizing debugInitialize and debugShowCharacter? The
> >> rest is common with "SerialDebug.asm", and I wish I hadn't had to review
> >> those (unchanged) macros, they make my eyes bleed :/
> >>
> >> Anyway, if you think any of these (or all of these!) means too much
> >> work, I'm OK with the patch going-in as is.
> >
> > My concern would be around SEV-ES/SEV-SNP guest usage. The in and out
> > instruction will generate a #VC and would require a #VC handler to be in
> > place.
> >
> > I'm pretty sure this will cause issues for those types of guests, but it
> > might be a few days before I can get to it to verify. @Gerd, were you
> able
> > to test this with those types of guests?
>
> Had a meeting get canceled and so got a chance to test this. As I thought,
> this causes SEV-ES/SEV-SNP guest failures.
>
> Thanks,
> Tom
>
> >
> > Thanks,
> > Tom
> >
> >>
> >> I'm a bit concerned about (1), TBH.
> >>
> >> Laszlo
> >>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114361): https://edk2.groups.io/g/devel/message/114361
Mute This Topic: https://groups.io/mt/103933942/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V1 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA
On Sun, Oct 29, 2023 at 11:42 PM Sun, CepingX wrote:
>
> On Saturday, October 28, 2023 12:45 AM, Erdem Aktas wrote:
> This should be the [PATCH V1 2/2] I assume?
> Yes, the name is same with [PATCH v1 0/2] , may be confusion, I would update
> in next version to avoid the same title name.
>
>
> On Thu, Oct 26, 2023 at 5:58 PM sunceping wrote:
> [Sources]
>VirtualMemory.h
>MemoryEncryption.c
> + X64/TdVmCallMapGPA.nasm
> I do not think we need another TdVmCallMapGPA definition, do we?
> Currently, the TdVmCall always clears the R11 if the return code is not
> successful, which means we need to change TdVmCall if we don't add
> TdVmCallMapGPA.
> Refer the GHCI Spec , if the returns code is not successful, the R11 value is
> not valid for the sub-functions except MapGPA, which means TdVmCall should
> clear the value on
> unsuccessful returns and only save the value if MapGPA returns
> unsuccessfully. If an update is required, the logic in TdVmCall could be
> complex.
It seems like TdVmCallMapGPA function is actually duplicating most of
the code that the TdVmCall function is already doing.
According to the spec, R11 has meaningful data when mapgpa has RETRY,
GPA_IN_USE or ALIGN_ERROR.
I do not think the TdVmCall change logic would be complex (or not more
than what TdVmCallMapGPA is already doing).
I would like to see what others are saying on this.
>
> [LibraryClasses]
>BaseLib
> diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> index b47f56b391a5..1f29f9194c30 100644
> --- a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> +++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> @@ -38,6 +38,10 @@ typedef enum {
>
> STATIC PAGE_TABLE_POOL *mPageTablePool = NULL;
>
> +#define TDVMCALL_STATUS_RETRY 0x1
> +
> +#define MAX_RETRIES_PER_PAGE 3
> +
> /**
>This function is used to help request the host VMM to map a GPA range as
>private or shared-memory mappings.
> @@ -546,6 +550,13 @@ SetOrClearSharedBit (
>EFI_STATUSStatus;
>EDKII_MEMORY_ACCEPT_PROTOCOL *MemoryAcceptProtocol;
>
> + UINT64 MapGpaRetryaddr;
> Should be replaced with MapGpaRetryAddr for consistency in variable name
> casing style ?
> Yes, it would be updated in next version.
>
> + UINT32 RetryCount;
> + UINT64 EndAddress;
> +
> + MapGpaRetryaddr = 0;
> + RetryCount = 0;
> +
>AddressEncMask = GetMemEncryptionAddressMask ();
>
>//
> @@ -559,7 +570,30 @@ SetOrClearSharedBit (
> PhysicalAddress &= ~AddressEncMask;
>}
>
> - TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, NULL);
> + while (RetryCount < MAX_RETRIES_PER_PAGE) {
> +TdStatus = TdVmCallMapGPA (PhysicalAddress, Length, );
> Why not this?
> TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
> );
> The TdVmCall always clears the R11 value when unsuccessful returns as above
> comments, therefor add the TdVmCallMapGPA to handle it.
right, the tdvmcall does not handle the R11 correctly for mapGPA. I
think it should be an easy fix in that function instead of creating a
whole copy of that function.
Is there a reason why we think it is complicated?
>
> +if (TdStatus != TDVMCALL_STATUS_RETRY) {
> + break;
> +}
> +
> +DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry PhysicalAddress is
> %llx, MapGpaRetryaddr is %llx\n", __func__, PhysicalAddress,
> MapGpaRetryaddr));
> +
> +EndAddress = PhysicalAddress + Length;
> +if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >
> EndAddress)) {
> should be?
> if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >= EndAddress))
> Yes, that’s right, it would be updated in next version.
>
> Thanks
> Ceping
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110350): https://edk2.groups.io/g/devel/message/110350
Mute This Topic: https://groups.io/mt/102212640/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V1 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA
Hi,
This should be the [PATCH V1 2/2] I assume?
On Thu, Oct 26, 2023 at 5:58 PM sunceping wrote:
> From: Ceping Sun
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4572
>
> According to section 3.2 of the [GHCI] document, if the result of MapGPA
> is "TDG.VP.VMCALL_RETRY", TDVF must retry mapping for pages in that region,
> starting with the GPA specified in R11.
>
> Reference:
> [GHCI]: TDX Guest-Host-Communication Interface v1.0
> https://cdrdv2.intel.com/v1/dl/getContent/726790
>
> Cc: Erdem Aktas
> Cc: James Bottomley
> Cc: Jiewen Yao
> Cc: Min Xu
> Cc: Tom Lendacky
> Cc: Michael Roth
> Cc: Gerd Hoffmann
> Signed-off-by: Ceping Sun
> ---
> .../BaseMemEncryptTdxLib.inf | 1 +
> .../BaseMemEncryptTdxLib/MemoryEncryption.c | 36 ++-
> 2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> b/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> index 11768825f8ca..742b65a289ce 100644
> --- a/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/BaseMemEncryptTdxLib.inf
> @@ -30,6 +30,7 @@
> [Sources]
>VirtualMemory.h
>MemoryEncryption.c
> + X64/TdVmCallMapGPA.nasm
>
I do not think we need another TdVmCallMapGPA definition, do we?
>
> [LibraryClasses]
>BaseLib
> diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> index b47f56b391a5..1f29f9194c30 100644
> --- a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> +++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
> @@ -38,6 +38,10 @@ typedef enum {
>
> STATIC PAGE_TABLE_POOL *mPageTablePool = NULL;
>
> +#define TDVMCALL_STATUS_RETRY 0x1
> +
> +#define MAX_RETRIES_PER_PAGE 3
> +
> /**
>This function is used to help request the host VMM to map a GPA range as
>private or shared-memory mappings.
> @@ -546,6 +550,13 @@ SetOrClearSharedBit (
>EFI_STATUSStatus;
>EDKII_MEMORY_ACCEPT_PROTOCOL *MemoryAcceptProtocol;
>
> + UINT64 MapGpaRetryaddr;
>
Should be replaced with MapGpaRetryAddr for consistency in variable
name casing style ?
> + UINT32 RetryCount;
> + UINT64 EndAddress;
> +
> + MapGpaRetryaddr = 0;
> + RetryCount = 0;
> +
>AddressEncMask = GetMemEncryptionAddressMask ();
>
>//
> @@ -559,7 +570,30 @@ SetOrClearSharedBit (
> PhysicalAddress &= ~AddressEncMask;
>}
>
> - TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
> NULL);
> + while (RetryCount < MAX_RETRIES_PER_PAGE) {
> +TdStatus = TdVmCallMapGPA (PhysicalAddress, Length, );
>
Why not this?
TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0,
);
> +if (TdStatus != TDVMCALL_STATUS_RETRY) {
> + break;
> +}
> +
> +DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry PhysicalAddress is
> %llx, MapGpaRetryaddr is %llx\n", __func__, PhysicalAddress,
> MapGpaRetryaddr));
> +
> +EndAddress = PhysicalAddress + Length;
> +if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >
> EndAddress)) {
>
should be?
if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >=
EndAddress))
> + DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed Retry
> PhysicalAddress is %llx, MapGpaRetryaddr is %llx\n", __func__,
> PhysicalAddress, MapGpaRetryaddr));
> + break;
> +}
> +
> +if (MapGpaRetryaddr == PhysicalAddress) {
> + RetryCount++;
> + continue;
> +}
> +
> +PhysicalAddress = MapGpaRetryaddr;
> +Length = EndAddress - PhysicalAddress;
> +RetryCount = 0;
> + }
> +
>if (TdStatus != 0) {
> DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed with %llx\n",
> __func__, TdStatus));
> ASSERT (FALSE);
> --
> 2.34.1
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110223): https://edk2.groups.io/g/devel/message/110223
Mute This Topic: https://groups.io/mt/102212640/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V1] OvmfPkg/PeilessStartupLib: Updated with PcdSecureBootSupported
Reviewed-by: Erdem Aktas On Sun, Jul 16, 2023 at 6:55 PM Yao, Jiewen wrote: > > Reviewed-by: Jiewen Yao > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of Gerd > > Hoffmann > > Sent: Monday, July 10, 2023 6:36 PM > > To: devel@edk2.groups.io; Sun, CepingX > > Cc: Aktas, Erdem ; James Bottomley > > ; Yao, Jiewen ; Xu, Min M > > ; Tom Lendacky ; Michael > > Roth > > Subject: Re: [edk2-devel] [PATCH V1] OvmfPkg/PeilessStartupLib: Updated with > > PcdSecureBootSupported > > > > On Mon, Jul 10, 2023 at 06:05:39PM +0800, sunceping wrote: > > > SECURE_BOOT_FEATURE_ENABLED was dropped by the commit(92da8a154f), > > but the > > > PeilessStartupLib was not updated with PcdSecureBootSupported, that made > > > SecureBoot no longer work in IntelTdxX64. > > > > > > Fix this by replacing SECURE_BOOT_FEATURE_ENABLED with > > > PcdSecureBootSupported in PeilessStartupLib. > > > > > > Cc: Erdem Aktas > > > Cc: James Bottomley > > > Cc: Jiewen Yao > > > Cc: Gerd Hoffmann > > > Cc: Min Xu > > > Cc: Tom Lendacky > > > Cc: Michael Roth > > > Signed-off-by: Ceping Sun > > > > Acked-by: Gerd Hoffmann > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106971): https://edk2.groups.io/g/devel/message/106971 Mute This Topic: https://groups.io/mt/100054785/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V3 04/29] OvmfPkg: Extend VmgExitLib to handle #VE exception
Hi Min,
On Mon, Nov 1, 2021 at 6:16 AM Min Xu wrote:
>
> +VmTdExitHandleVe (
> + IN OUT EFI_EXCEPTION_TYPE *ExceptionType,
> + IN OUT EFI_SYSTEM_CONTEXT SystemContext
> + )
> +{
> + UINT64Status;
> + TD_RETURN_DATAReturnData;
> + EFI_SYSTEM_CONTEXT_X64*Regs;
> +
> + Regs = SystemContext.SystemContextX64;
> + Status = TdCall (TDCALL_TDGETVEINFO, 0, 0, 0, );
> + ASSERT (Status == 0);
> + if (Status != 0) {
> +DEBUG ((DEBUG_ERROR, "#VE happened. TDGETVEINFO failed with Status =
> 0x%llx\n", Status));
> +TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0);
> + }
>
What is the difference between TdVmCall (EXIT_REASON_HLT, 0, 0, 0, 0, 0) vs
TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0);
>From the VMM stand point both seems the same?
+
> + switch (ReturnData.VeInfo.ExitReason) {
> +case EXIT_REASON_CPUID:
> +Status = CpuIdExit (Regs, );
> +DEBUG ((DEBUG_VERBOSE ,
> + "CPUID #VE happened, ExitReasion is %d, ExitQualification =
> 0x%x.\n",
> + ReturnData.VeInfo.ExitReason,
> ReturnData.VeInfo.ExitQualification.Val
> + ));
> +break;
> +
> +case EXIT_REASON_HLT:
> +if (FixedPcdGetBool (PcdIgnoreVeHalt) == FALSE) {
> + Status = TdVmCall (EXIT_REASON_HLT, 0, 0, 0, 0, 0);
> +}
> +break;
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83798): https://edk2.groups.io/g/devel/message/83798
Mute This Topic: https://groups.io/mt/86739958/21656
Mute #ve:https://edk2.groups.io/g/devel/mutehashtag/ve
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V3 02/29] MdePkg: Add TdxLib to wrap Tdx operations
Hi Min,
Sorry for the late review. My comments and a few questions are inline.
Thanks
-Erdem
On Mon, Nov 1, 2021 at 6:16 AM Min Xu wrote:
>
> RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
>
> +**/
> +UINT32
> +GetGpaPageLevel (
> + UINT32 PageSize
> + )
> +{
> + UINT32 Index;
> +
> + for (Index = 0; Index < ARRAY_SIZE (mTdxAcceptPageLevelMap); Index++) {
> +if (mTdxAcceptPageLevelMap[Index] == PageSize) {
> + break;
> +}
> + }
> +
> + return Index == ARRAY_SIZE (mTdxAcceptPageLevelMap) ? -1 : Index;
Is this intentional? Returning signed integer but the function returns
unsigned integer.
+/**
+ This function accepts a pending private page, and initialize the page to
+ all-0 using the TD ephemeral private key.
+
+ @param[in] StartAddress Guest physical address of the private page
+ to accept.
+ @param[in] NumberOfPagesNumber of the pages to be accepted.
+ @param[in] PageSize GPA page size. Accept 1G/2M/4K page size.
The comment says that 1G is acceptable but the code only accepts 2M or
4K page sizes.
+
+ @return EFI_SUCCESS
> +EFI_STATUS
> +EFIAPI
> +TdAcceptPages (
> + IN UINT64 StartAddress,
> + IN UINT64 NumberOfPages,
> + IN UINT32 PageSize
> + )
> +{
> + EFI_STATUS Status;
> + UINT64 Address;
> + UINT64 TdxStatus;
> + UINT64 Index;
> + UINT32 GpaPageLevel;
> + UINT32 PageSize2;
> +
> + Address = StartAddress;
> +
> + GpaPageLevel = GetGpaPageLevel (PageSize);
> + if (GpaPageLevel == -1) {
Comparing unsigned int to signed int. I believe this should work with
GCC with warning messages.
Just checking if this is intentional?
> +DEBUG ((DEBUG_ERROR, "Accept page size must be 4K/2M. Invalid page size
> - 0x%llx\n", PageSize));
> +return EFI_INVALID_PARAMETER;
> + }
> +
> + Status = EFI_SUCCESS;
> + for (Index = 0; Index < NumberOfPages; Index++) {
> +TdxStatus = TdCall (TDCALL_TDACCEPTPAGE, Address | GpaPageLevel, 0, 0,
> 0);
Address[11:3] and [63:52] are reserved and must be 0. The code does
not check it, spec is not clear about the behavior but I am assuming
that in that case, TDX_OPERAND_INVALID will be returned as error code
but should we check and log it properly?
> +if (TdxStatus != TDX_EXIT_REASON_SUCCESS) {
> +if ((TdxStatus & ~0xULL) ==
> TDX_EXIT_REASON_PAGE_ALREADY_ACCEPTED) {
> + //
> + // Already accepted
> + //
> + mNumberOfDuplicatedAcceptedPages++;
Is there any legit case that a page should be accepted twice? Looks
like other than debug printing, this information is ignored.
> + DEBUG ((DEBUG_WARN, "Page at Address (0x%llx) has already been
> accepted. - %d\n", Address, mNumberOfDuplicatedAcceptedPages));
> +} else if ((TdxStatus & ~0xULL) ==
> TDX_EXIT_REASON_PAGE_SIZE_MISMATCH) {
> + //
> + // GpaPageLevel is mismatch, fall back to a smaller GpaPageLevel
> if possible
> + //
> + DEBUG ((DEBUG_VERBOSE, "Address %llx cannot be accepted in
> PageLevel of %d\n", Address, GpaPageLevel));
> +
> + if (GpaPageLevel == 0) {
> +//
> +// Cannot fall back to smaller page level
> +//
Could you help me understand this? So if ,for some reason, VMM maps a
2MB private page and a guest wants to accept it in 4KB page chunks,
this will always fail. Is it not a possible use case?
> +DEBUG ((DEBUG_ERROR, "AcceptPage cannot fallback from PageLevel
> %d\n", GpaPageLevel));
> +Status = EFI_INVALID_PARAMETER;
> +break;
> + } else {
> +//
> +// Fall back to a smaller page size
> +//
> +PageSize2 = mTdxAcceptPageLevelMap [GpaPageLevel - 1];
> +Status = TdAcceptPages(Address, 512, PageSize2);
> +if (EFI_ERROR (Status)) {
> + break;
> +}
> + }
> +}else {
> +
> + //
> + // Other errors
> + //
Why not handle the TDX_OPERAND_BUSY? It is not an error and tdaccept
needs to be retired, I guess, handling it like an error might cause
reliability issues.
> + DEBUG ((DEBUG_ERROR, "Address %llx (%d) failed to be accepted.
> Error = 0x%llx\n",
> +Address, Index, TdxStatus));
> + Status = EFI_INVALID_PARAMETER;
> + break;
> +}
> +}
> +Address += PageSize;
> + }
> + return Status;
> +}
.
> +**/
> +EFI_STATUS
> +EFIAPI
> +TdExtendRtmr (
> + IN UINT32 *Data,
> + IN UINT32 DataLen,
> + IN UINT8 Index
> + )
> +{
> + EFI_STATUSStatus;
> + UINT64TdCallStatus;
> + UINT8 *ExtendBuffer;
> +
> + Status = EFI_SUCCESS;
> +
> + ASSERT (Data != NULL);
> + ASSERT (DataLen == SHA384_DIGEST_SIZE);
> + ASSERT (Index >= 0 && Index < RTMR_COUNT);
> +
Because of the Asserts above , the following condition will never run,
Re: [edk2-devel] [PATCH v7 09/31] OvmfPkg/SecMain: register GHCB gpa for the SEV-SNP guest
On Mon, Sep 13, 2021 at 9:20 PM Brijesh Singh wrote:
> +*/
> +STATIC
> +VOID
> +SevSnpGhcbRegister (
> + UINTN Address
> + )
> +{
> + MSR_SEV_ES_GHCB_REGISTER Msr;
> + MSR_SEV_ES_GHCB_REGISTER CurrentMsr;
> + EFI_PHYSICAL_ADDRESS GuestFrameNumber;
> +
> + GuestFrameNumber = Address >> EFI_PAGE_SHIFT;
> +
> + //
> + // Save the current MSR Value
> + //
> + CurrentMsr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
We are backing the current MSR value but when was it initialized
before ? Also is not this function supposed to set the Address as the
GHCB address? If it is, do we care about the old value?
> + // Restore the MSR
> + //
> + AsmWriteMsr64 (MSR_SEV_ES_GHCB, CurrentMsr.GhcbPhysicalAddress);
Why are we restoring the old value? I may have misunderstood but I
thought this function will set Address as the new GHCB address?
Thanks
-Erdem
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80731): https://edk2.groups.io/g/devel/message/80731
Mute This Topic: https://groups.io/mt/85582697/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 03/23] OvmfPkg/ResetVector: Enable Intel TDX in ResetVector of Ovmf
On Mon, Sep 13, 2021 at 9:06 AM Xu, Min M wrote: > > > > +TdxApWait: > > > +cmp byte[TDX_WORK_AREA_PGTBL_READY], 0 > > > +je TdxApWait > > Don't we need memory fence before je TdxApWait. I did not check > > what the compiler generates for this loop. > Below is the code compiler generated for this loop. (VS2017/release) >106 <1> TdxApWait: >107 070B 803D04B080 <1> cmp > byte[TDX_WORK_AREA_PGTBL_READY], 0 >108 0712 74F7 <1> je TdxApWait >109 0714 EB17 <1> jmp > ExitInitTdxWorkarea > > This is the code lfence is added. >106 <1> TdxApWait: >107 070B 803D04B080 <1> cmp > byte[TDX_WORK_AREA_PGTBL_READY], 0 >108 0712 0FAEE8 <1> lfence >109 0715 74F4 <1> je TdxApWait >110 0717 EB17 <1> jmp > ExitInitTdxWorkarea > > I am not sure if lfence is needed. Thanks Min! You are right, I also checked it with GCC5 and saw the same output. Thanks for checking it. -Erdem -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80631): https://edk2.groups.io/g/devel/message/80631 Mute This Topic: https://groups.io/mt/84837891/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V5 2/2] OvmfPkg/ResetVector: Enable Intel TDX in ResetVector of Ovmf
On Mon, Aug 30, 2021 at 5:35 AM Min Xu wrote: > +; > +; Check if it is Intel Tdx > +; > +; Modified: EAX, EBX, ECX, EDX > +; > +; If it is Intel Tdx, EAX is zero > +; If it is not Intel Tdx, EAX is non-zero > +; > +IsTdx: IsTdx returns 0 when TDX is enabled in CPUID but IsTdxEnabled return 1 when TDX is enabled. Is this intentional? here is how IsTdxEnabled defined. ; If TDX is enabled then EAX will be 1 ; If TDX is disabled then EAX will be 0. ; IsTdxEnabled: > +; > +; In Td guest, BSP/AP shares the same entry point > +; BSP builds up the page table, while APs shouldn't do the same task. > +; Instead, APs just leverage the page table which is built by BSP. > +; APs will wait until the page table is ready. > +; > +TdxApWait: > +cmp byte[TDX_WORK_AREA_PGTBL_READY], 0 > +je TdxApWait > +jmp ExitInitTdxWorkarea Don't we need memory fence before je TdxApWait > +; Check TDX features, Non-TDX or TDX-BSP or TDX-APs? > +; > +; By design TDX BSP is reponsible for inintializing the PageTables. s/reponsible/responsible s/inintializing/initializing -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80525): https://edk2.groups.io/g/devel/message/80525 Mute This Topic: https://groups.io/mt/85242569/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 06/23] MdePkg: Add TdxLib to wrap Tdx operations
On Thu, Aug 12, 2021 at 2:57 PM Min Xu wrote: > - TdMaxVCpuNum: Get the maximum number of virutal CPUs. s/virutal/virtual > - TdVCpuNum : Get the number of virtual CPUs. (In some case VMM may > add more vCPU in runtime). s/case/cases How is this possible considering that once the TD is finalized, there should not be any new vcpu added, right? Am I missing something here? > +++ b/MdePkg/Library/TdxLib/X64/Tdcall.nasm > @@ -0,0 +1,120 @@ > +;-- > +%macro tdcall_regs_preamble 2 is this even used in this file? > +; Zero out unused (for standard TDVMCALL) registers to avoid leaking > +; secrets to the VMM. this is for TDCALL right, there is no leaking to the tdx module. > + > +xor ebx, ebx > +xor esi, esi > +xor edi, edi > + > +xor edx, edx > +xor ebp, ebp zeroing only the lower 32bit values? why not the higher 32bit value if leaking is the concern? > +++ b/MdePkg/Library/TdxLib/X64/Tdvmcall.nasm > +%define TDVMCALL_EXPOSE_REGS_MASK 0xffec Should we expose only the minimum number of registers needed for the TDVMCALL? > > +%macro tdcall_regs_preamble 2 > +mov rax, %1 > + > +mov ecx, %2 should not we make sure that the higher 32bit of RCX is 0? RCX [63:32] are reserved and always need to be 0. > +; R10 = 0 (standard TDVMCALL) > + > +xor r10d, r10d > + > +; Zero out unused (for standard TDVMCALL) registers to avoid leaking > +; secrets to the VMM. Is not rcx the bitmap of the registers that will be exposed to VMM? unused registers should be set 0 in the bitmap, why zeroing them? > + > +xor ebx, ebx > +xor esi, esi > +xor edi, edi > +xor edx, edx > +xor ebp, ebp if we are concerned about leaking some data, why xor only the lower 32bits? -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80523): https://edk2.groups.io/g/devel/message/80523 Mute This Topic: https://groups.io/mt/84837895/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 05/23] MdePkg: Add TdxProbeLib to probe Intel Tdx
On Thu, Aug 12, 2021 at 2:57 PM Min Xu wrote: > + > +#include > +#include "InternalTdxProbe.h" > + > +/** > + TDX only works in X64. So allways return -1 to indicate Non-Td. s/allways/always Also, -1 or 1? PROBE_NOT_TD_GUEST is defined as 1. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80522): https://edk2.groups.io/g/devel/message/80522 Mute This Topic: https://groups.io/mt/84837894/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 03/23] OvmfPkg/ResetVector: Enable Intel TDX in ResetVector of Ovmf
On Thu, Aug 12, 2021 at 2:57 PM Min Xu wrote: > > +; > +; Check if it is Intel Tdx > +; > +; Modified: EAX, EBX, ECX, EDX > +; > +; If it is Intel Tdx, EAX is zero > +; If it is not Intel Tdx, EAX is non-zero > +; > +IsTdx: IsTdx returns 0 when TDX is enabled in CPUID but IsTdxEnabled return 1 when TDX is enabled. Is this intentional? here is how IsTdxEnabled defined. ; If TDX is enabled then EAX will be 1 ; If TDX is disabled then EAX will be 0. ; IsTdxEnabled: > +TdxApWait: > +cmp byte[TDX_WORK_AREA_PGTBL_READY], 0 > +je TdxApWait Don't we need memory fence before je TdxApWait. I did not check what the compiler generates for this loop. -Erdem -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80521): https://edk2.groups.io/g/devel/message/80521 Mute This Topic: https://groups.io/mt/84837891/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 02/23] OvmfPkg/Sec: Update the check logic in SevEsIsEnabled
On Thu, Aug 12, 2021 at 2:57 PM Min Xu wrote: > > RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429 > > SevEsIsEnabled return TRUE if SevEsWorkArea->SevEsEnabled is non-zero. s/return/returns > It is correct when SevEsWorkArea is only used by SEV. After Intel TDX > is enabled in Ovmf, the SevEsWorkArea is shared by TDX and SEV. (This > is to avoid the waist of memory region in MEMFD). The value of s/waist/waste -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80520): https://edk2.groups.io/g/devel/message/80520 Mute This Topic: https://groups.io/mt/84837890/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V5 1/2] OvmfPkg: Introduce Tdx BFV/CFV PCDs and PcdOvmfImageSizeInKb
I have few naive questions. Sorry if the answers were obvious. >>TDVF also include a configuration firmware volume (CFV) that is separated >>from the BFV. The reason is because the CFV is measured in RTMR, while >>the BFV is measured in MRTD. If I understand correctly, this means that the BFV is encrypted and measured during TD build time. Since CFV is not included in the MRTD, CFV region is not encrypted with the guest key, is it? >> The measurement value of the CFV (provisioned configuration data) is >> extended to >> RTMR registers (similar to TPM PCRs). At the same time it is recorded in the >> TD Event >> log. Even if it is measured at runtime, the content needs to be copied to somewhere else, otherwise what stops VMM to change the content after it is being measured (assuming that it is not encrypted). >> As to the spare part in varstore, it is not external input, is it? It's >> produced and consumed >> by code itself. From this perspective it should not be measured. If the >> spare part >> is included in the measurement, then the *good* measurement is not known >> anymore. >> Because no one knows about the content of spare part in advance. I am confused about how this memory is initialized. If it is encrypted, then no need to measure it but also it becomes useless as the key will change in the next boot. If it is not encrypted, VMM can always modify the content and might cause unexpected behavior at runtime, right? I might be missing something here but if this region is not encrypted: - CFV content needs to be copied into an encrypted buffer after being measured and should never be used again. - Allowing variables to be stored in SPARE part seems like opening an attack surface as no one knows what will be stored in that region. Is this correct understanding? -Erdem On Wed, Sep 1, 2021 at 10:20 PM Andrew Fish wrote: > > > > On Sep 1, 2021, at 9:53 AM, James Bottomley wrote: > > > > On Wed, 2021-09-01 at 08:59 +, Yao, Jiewen wrote: > >> Hi Min > >> I agree with Gerd and Ard in this case. > >> > >> It is NOT so obvious that the FTW is produced then consumed in the > >> code. What if the attacker prepares some special configuration to > >> trigger the FTW process at the first boot, the code will do *read* > >> before *write*? That is a potential attack surface. > > > > It's not just that: even if you can ensure nothing in the host changed > > the variables, how do you know *your* code inside the guest is updating > > them? In ordinary OVMF we try to ensure that by having the variables > > SMM protected so the only update path available to the kernel is via > > the setVariable interface, but we can't do that in the confidential > > computing case because SMM isn't supported. That means a random kernel > > attacker in the guest can potentially write to the var store too. > > > > At least for the first SEV prototype I had to make the var store part > > of the first firmware volume firstly so it got measured but secondly so > > it couldn't be used as a source of configuration attacks. > > > > I have a nasty feeling that configuration attacks are going to be the > > bane of all confidential computing solutions because they give the > > untrusted VMM a wide attack surface. > > > > James, > > If we take a big step back the requirement for an EFI Runtime Service, like > the variable API, is just exclusive access to hardware at OS runtime. The > variable store needs to be on a hardware device that has a persistent > reliable store. The FTW is really about maintaining the consistency of the > store if the power gets yanked at the wrong moment. So the fact that the UEFI > Variable Store is in NOR FLASH is a historical artifact more than > architecture. Also on physical devices hardware cost money, and you need the > NOR FLASH for the firmware so why change it. Thus conceptually the variable > store could be backed by a virtual hardware device that was designed with > security in mind. Maybe more of message passing interface and the reliability > of updates is maintained by the hardware device not the UEFI code. It would > also be possible for the hardware device to enforce security policy. You > could even have EFI send a one shot message per 1st boot to the hardware to > define a security policy. If you wanted the hardware device could even > implement the UEFI Secure Boot infrastructure so the UEFI Variable Driver > could be untrusted. I guess this hypothetical variable store virtual hardware > device could also have hardware access to other security hardware resources > (like a TPM) and implement security policies based on that. > > FYI on Macs with a T2 (security chip) the UEFI variable store lives on the T2. > > Thanks, > > Andrew Fish > > > > James > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80500): https://edk2.groups.io/g/devel/message/80500 Mute This Topic:
Re: [edk2-devel] [PATCH V4 3/3] OvmfPkg/ResetVector: Enable Intel TDX in ResetVector of Ovmf
I agree with Brijesh on that this patch should be divided into smaller ones.
On Mon, Aug 2, 2021 at 6:18 PM Min Xu wrote:
>
> +;
> +; EBP[6:0] CPU supported GPA width
> +;
> +and ebp, 0x3f
Based on
https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1eas-v0.85.039.pdf
sec 8.1.2
Is not this EBP[5:0] for GPA width ? it should be "and ebp, 0x1f"
-Erdem
On Mon, Aug 2, 2021 at 6:18 PM Min Xu wrote:
>
> RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
>
> Intel's Trust Domain Extensions (Intel TDX) refers to an Intel technology
> that extends Virtual Machines Extensions (VMX) and Multi-Key Total Memory
> Encryption (MKTME) with a new kind of virutal machines guest called a
> Trust Domain (TD). A TD is desinged to run in a CPU mode that protects the
> confidentiality of TD memory contents and the TD's CPU state from other
> software, including the hosting Virtual-Machine Monitor (VMM), unless
> explicitly shared by the TD itself.
>
> Note: Intel TDX only is available on X64, so the Tdx related changes are
> in X64 path. In IA32 path, there may be null stub to make the build
> success.
>
> This patch includes below major changes.
>
> 1. CC_WORK_AREA
> Ovmf is able to run on different types of VM guest with ONE image. These
> VM guest includes the Legacy guest, SEV guest and TDX guest. In
> ResetVector CC_WORK_AREA is such a memory region to record the VM guest
> information. Below is the definition of the work area.
>
> typedef struct {
>UINT8 Type; // 0 legacy, 1 SEV, 2 TDX
>UINT8 SubType; // Depends on Type
>UINT8 Rsvd[2]; // Reserved
>union VM_GUEST {
> TDX_WORK_AREA Tdx;
> SEV_WORK_AREA Sev;
>} Guest;
> } CC_WORK_AREA_HEAD;
>
> typedef struct {
>... ...
> } TDX_WORK_AREA
>
> typedef struct {
>... ...
> } SEV_WORK_AREA
>
> 2. X64/IntelTdxMetadata.asm
> IntelTdxMetadata describes the information about the image for VMM use.
> For example, the base address and length of the TdHob, TdMailbox, etc.
> Its offset is put in a GUID-ed structure which is appended in the GUID-ed
> chain from a fixed GPA (0xffd0). Below are the items in TdxMetadata:
> _Bfv:
> Boot Firmware Volume
> _Cfv:
> Configuration Firmware Volume
> _Stack:
> Initial stack
> _Heap:
> Initial heap
> _MailBox:
> TDVF reserves the memory region so each AP can receive the message
> sent by the guest OS.
> _CcWorkarea:
> Compute Confidential work area which is consumed by CC technologies,
> such as SEV, TDX.
> _TdHob:
> VMM pass the resource information in TdHob to TDVF.
> _TdxPageTable:
> If 5-level page table is supported (GPAW is 52), a top level page
> directory pointers (1 * 256TB entry) is generated in this page.
> _OvmfPageTable:
> Initial page table for standard Ovmf.
>
> TDVF indicate above chunk of temporary initialized memory region (_Stack/
> _Heap/_MailBox/_CcWorkarea/_TdHob/_TdxPageTables/OvmfPageTable) to support
> TDVF code finishing the memory initialization. Because the other
> unaccepted memory cannot be accessed until they're accepted.
>
> Since AMD SEV has already defined some SEV specific memory region in
> MEMFD. TDX re-use the memory regions defined by SEV.
> - MailBox : PcdOvmfSecGhcbBackupBase|PcdOvmfSecGhcbBackupSize
> - TdHob: PcdOvmfSecGhcbBase|PcdOvmfSecGhcbSize
> - TdxPageTable : PcdOvmfSecGhcbPageTableBase|PcdOvmfSecGhcbPageTableSize
> - CcWorkArea : PcdSevEsWorkAreaBase|PcdSevEsWorkAreaSize
>
> 3. Ia32/IntelTdx.asm
> IntelTdx.asm includes below routines used in ResetVector
> - IsTdx
>Check if the running system is Tdx guest.
>
> - InitTdxWorkarea
>It initialize the CC_WORK_AREA. Because it is called by both BSP and
>APs and to avoid the race condition, only BSP can initialize the
>WORK_AREA. AP will wait until the field of TDX_WORK_AREA_PGTBL_READY
>is set.
>
> - ReloadFlat32
>After reset all CPUs in TDX are initialized to 32-bit protected mode.
>But GDT register is not set. So this routine loads the GDT and set the
>CR0, then jump to Flat 32 protected mode. After that CR4 and other
>registers are set.
>
> - InitTdx
>This routine wrap above 3 routines together to do Tdx initialization
>in ResetVector phase.
>
> - PostSetCr3PageTables64Tdx
>It is called after SetCr3PageTables64 in Tdx guest to set CR0/CR4.
>If GPAW is 52, then CR3 is adjusted as well.
>
> - IsTdxEnabled
>It is a OneTimeCall to probe if TDX is enabled by checking the
>CC_WORK_AREA.
>
> 4. Ia32/AmdSev.asm
> AmdSev.asm includes the SEV routines used in ResetVector. This patch
> remove the code of clearing SEV_ES_WORK_AREA in CheckSevFeatures. The
> clearing of SEV_ES_WORK_AREA is called at Main16 in Main.asm.
>
> 5. Main.asm
> Previously OvmfPkg/ResetVector use the Main.asm in UefiCpuPkg. There is
> only Main16 entry point. Main32 entry point is needed in Main.asm because
> of Intel TDX. To
Re: [edk2-devel] [PATCH V4 1/3] OvmfPkg: Add Tdx BFV/CFV PCDs and PcdOvmfImageSizeInKb
Reviewed-by: Erdem Aktas On Mon, Aug 2, 2021 at 6:18 PM Min Xu wrote: > > RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429 > > Tdx Virtual Firmware (TDVF) includes one Firmware Volume (FV) known > as the Boot Firmware Volume (BFV). The FV format is defined in the > UEFI Platform Initialization (PI) spec. BFV includes all TDVF components > required during boot. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78636): https://edk2.groups.io/g/devel/message/78636 Mute This Topic: https://groups.io/mt/84631104/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH v5 07/28] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase
On Wed, Jun 30, 2021 at 5:54 AM Brijesh Singh wrote: > > a) Enhance the OVMF reset vector code to validate the pages as described >above (go through step 2 - 3). > OR > b) Validate the pages during the guest creation time. The SEV firmware >provides a command which can be used by the VMM to validate the pages >without affecting the measurement of the launch. Are you referring to the PAGE_TYPE_UNMEASURED? Does it not affect the measurement , PAGE_INFO will be still measured, right? > Approach #b seems much simpler; it does not require any changes to the > OVMF reset vector code. I am worried about verifying the measurement. I understand the secret page and cpuid page being part of measurement because both of them are mentioned in the AMD SNP SPEC but now we are introducing a new parameters (all the 4KB page addresses between SNP_HV_VALIDATED_START and SNP_HV_VALIDATED_END) that VM owner needs to know to calculate the measurement and verify the attestation. Sorry if I am overthinking or missing something here. -Erdem -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78464): https://edk2.groups.io/g/devel/message/78464 Mute This Topic: https://groups.io/mt/83891520/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF
Hi all, Sorry for the late reply. I like to add some clarification on "one binary". I feel like the way everyone uses the term "one binary" in the email threads is causing some confusion. As I have tried to explain before, we are not looking for everything in a single binary. As Laszlo has mentioned in a lot of places, some additional features are TDX specific and not worth pushing into the OvmfPkgX64.dsc. What we are asking with "one binary" is: Simply enabling OVMF + a guest OS to boot in a TDX domain without breaking any existing functionality. Intel should put everything else (specifically related to remote attestation) in a separate platform configuration. This aligns well with Laszlo's perspective and it is similar to what AMD is doing as far as I can see. Enabling a minimum functionality in OvmfPkgX64.dsc without breaking existing functionality will allow a lot of flexibility for us for integration/testing and productionization. A separate platform configuration file can be used to provide all the security guarantees that TDX provides. Based on the above clarification, Intel has updated their slides accordingly - I really appreciate their hard work and community collaboration spirit. I am hoping that this clears up the confusion. On Thu, Jun 3, 2021 at 9:12 AM Laszlo Ersek wrote: > My point is that the "one binary" that the slide deck refers to (i.e., > OvmfPkgX64.dsc) might prove OK for utilizing the Intel TDX *technology* > in itself. Simply enabling OVMF + a guest OS to boot in a TDX domain. This is what we are asking as a single binary. > But "OvmfPkgX64.dsc" is *not* the "one binary" that is suitable for > remote attestation, which has a much broader scope, involving multiple > computers, networking, deployment, guest-owner/host-owner separation, > whatever. For the latter, we needed a separate platform anyway, even > with only SEV in mind; that's why "OvmfPkg/AmdSev/AmdSevX64.dsc" exists. This is okay for us. > (10) This slide (slide 11) basically describes an intrusive > reorganization of "OvmfPkgX64.fdf". I don't think I can agree to that. > While confidential computing is important, it is not relevant for many > users. Even if we don't cause outright regressions for existent setups, > the maintenance cost of the traditional OVMF platform will skyrocket. > > The big bunch of areas that SEV-ES introduced to MEMFD is already a big > complication. I'd feel much better if we could isolate all that to a > dedicated "remote attested boot" firmware platform, and not risk the > functionality and maintenance of the traditional platform. I think this > ties in with my comment (1). > > For example, seeing a configuration firmware volume (CFV) with secure > boot keys embedded, in the "usual" FDF, will confuse lots of people, me > included. In the traditional OVMF use case, we use a different method: > namely OvmfPkg/EnrollDefaultKeys, for "priming" a variable store > template, in the build environment. Embedding secure keys might be a good idea to remove the VMM from the TCB but I agree with Laszlo, OvmfPkgX64 is not the best place to do this. The existing secure boot flow should not be changed. Any additional improvement to reduce the TCB can go TDX specific platform communication. > (12) My comment is that the GUID-ed structure chain already starts at a > fixed GPA (the "AMD SEV option"). Ordering between GUID-ed structures is > irrelevant, so any TDX-specific structures should be eligible for > appending to, prepending to, or intermixing with, other (possibly > SEV-specific) structures. There need not be a separate entry point, just > different GUIDS. I think there is a confusion in slide #13. I agree that the GUIDed table should solve this issue. And I do not think there is any dependency on SEV patches. I am assuming anyone can easily add an entry to the GUIDed table and based on other responses, this seems like a trivial implementation. > (13) Regarding "4G-0x20[0x10] is OVMF AP reset vector (used in OVMF > implementation)" -- I think this is a typo: this "AP reset vector" is > *not* used in OVMF. To my knowledge, it is a vestige from the UefiCpuPkg > reset vector. In OVMF, APs are booted via MpInitLib (in multiple > firmware phases), using INIT-SIPI-SIPI, and the reset vector for the > APs, posited through those IPIs, is prepared in low RAM. > I am guessing what Intel refers to is "SEV-ES resetblock" which sets the AP reset vector (jump address) address. > (22) EmuVariableFvbRuntimeDxe > > Ouch, this is an unpleasant surprise. > > First, if you know for a fact that pflash is not part of the *board* in > any TDX setup, then pulling Copying configuration variables into configuration volume is "good" to reduce the TCB but not necessary to boot a guest with TDX is enabled. Again it goes under the topic of what is minimum and what is secure and TDX specific platform configuration. So I think it should not be part of OvmfPkgX64.dsc On Thu, Jun 3, 2021 at 4:19 PM Yao, Jiewen
Re: [edk2-devel] [PATCH v3 09/13] OvmfPkg/BaseMemEncryptSevLib: introduce MemEncryptSevClearMmioPageEncMask()
> + @param[in] BaseAddress The physical address that is the start > + address of a MMIO region. Based on the code, what I understand is that the address parameters should be "guest virtual address", not the physical address. But in this patch, all the address parameters are named as PhysicalAddress. Is this intentional? -Erdem On Wed, May 19, 2021 at 11:20 AM Brijesh Singh wrote: > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 > > The MemEncryptSevClearMmioPageEncMask() helper can be used for clearing > the memory encryption mask for the Mmio region. > > The MemEncryptSevClearMmioPageEncMask() is a simplified version of > MemEncryptSevClearPageEncMask() -- it does not flush the caches after > clearing the page encryption mask. > > Cc: James Bottomley > Cc: Min Xu > Cc: Jiewen Yao > Cc: Tom Lendacky > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Laszlo Ersek > Cc: Erdem Aktas > Reviewed-by: Laszlo Ersek > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Include/Library/MemEncryptSevLib.h| 25 ++ > .../BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 + > .../Ia32/MemEncryptSevLib.c | 31 + > .../X64/MemEncryptSevLib.c| 33 +++ > .../X64/PeiDxeVirtualMemory.c | 33 +++ > .../X64/SecVirtualMemory.c| 30 + > 6 files changed, 175 insertions(+) > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h > b/OvmfPkg/Include/Library/MemEncryptSevLib.h > index 99f15a7d1271..b91490d5d44d 100644 > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > @@ -203,4 +203,29 @@ MemEncryptSevGetAddressRangeState ( >IN UINTNLength >); > > +/** > + This function clears memory encryption bit for the MMIO region specified by > + BaseAddress and NumPages. > + > + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use > + current CR3) > + @param[in] BaseAddress The physical address that is the start > + address of a MMIO region. > + @param[in] NumPagesThe number of pages from start memory > + region. > + > + @retval RETURN_SUCCESS The attributes were cleared for the > + memory region. > + @retval RETURN_INVALID_PARAMETERNumber of pages is zero. > + @retval RETURN_UNSUPPORTED Clearing the memory encryption > attribute > + is not supported > +**/ > +RETURN_STATUS > +EFIAPI > +MemEncryptSevClearMmioPageEncMask ( > + IN PHYSICAL_ADDRESS Cr3BaseAddress, > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINTNNumPages > + ); > + > #endif // _MEM_ENCRYPT_SEV_LIB_H_ > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > index fe2a0b2826cd..8dc39e647b90 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > @@ -126,4 +126,27 @@ InternalMemEncryptSevGetAddressRangeState ( >IN UINTNLength >); > > +/** > + This function clears memory encryption bit for the MMIO region specified by > + PhysicalAddress and Length. > + > + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use > + current CR3) > + @param[in] PhysicalAddress The physical address that is the start > + address of a MMIO region. > + @param[in] Length The length of memory region > + > + @retval RETURN_SUCCESS The attributes were cleared for the > + memory region. > + @retval RETURN_INVALID_PARAMETERLength is zero. > + @retval RETURN_UNSUPPORTED Clearing the memory encyrption > attribute > + is not supported > +**/ > +RETURN_STATUS > +EFIAPI > +InternalMemEncryptSevClearMmioPageEncMask ( > + IN PHYSICAL_ADDRESSCr3BaseAddress, > + IN PHYSICAL_ADDRESSPhysicalAddress, > + IN UINTN Length > + ); > #endif > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > index 12a5bf495bd7..169d3118e44f 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > @@ -111,3 +111,34 @@ MemEncryptSevGetAddressRangeState ( >// >return MemEncryptSevAddressRangeEncrypted; > } > + > +/** > + This function clears memory encryption bit for the MMIO region specified by > +
Re: [edk2-devel] [PATCH v3 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction
>> +#define RMPADJUST_VMPL_MAX 3 Why is RMPADJUST_VMPL_MAX defined as 3? Is it not defined in bits [15:12] of CPUID Fn8000_001F[EBX]? -Erdem -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75833): https://edk2.groups.io/g/devel/message/75833 Mute This Topic: https://groups.io/mt/82943408/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 05/13] MdePkg/Register/Amd: define GHCB macro for the Page State Change
I verified that the values align with the GHCB spec publication:
#56421 Revision: 2.00
Reviewed-by: Erdem Aktas
On Wed, May 12, 2021 at 4:46 PM Brijesh Singh wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> The Page State Change NAE exit will be used by the SEV-SNP guest to
> request a page state change using the GHCB protocol. See the GHCB
> spec section 4.1.6 and 2.3.1 for more detail on the structure
> definitions.
>
> Cc: James Bottomley
> Cc: Min Xu
> Cc: Jiewen Yao
> Cc: Tom Lendacky
> Cc: Jordan Justen
> Cc: Ard Biesheuvel
> Cc: Laszlo Ersek
> Cc: Erdem Aktas
> Cc: Michael D Kinney
> Cc: Liming Gao
> Cc: Zhiguang Liu
> Reviewed-by: Laszlo Ersek
> Reviewed-by: Liming Gao
> Signed-off-by: Brijesh Singh
> ---
> MdePkg/Include/Register/Amd/Fam17Msr.h | 15
> MdePkg/Include/Register/Amd/Ghcb.h | 33 ++
> 2 files changed, 48 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h
> b/MdePkg/Include/Register/Amd/Fam17Msr.h
> index 542e4cdf4782..62014854d9b7 100644
> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> @@ -58,6 +58,19 @@ typedef union {
> UINT64 GuestFrameNumber:52;
>} GhcbGpaRegister;
>
> + struct {
> +UINT64 Function:12;
> +UINT64 GuestFrameNumber:40;
> +UINT64 Operation:4;
> +UINT64 Reserved:8;
> + } SnpPageStateChangeRequest;
> +
> + struct {
> +UINT32 Function:12;
> +UINT32 Reserved:20;
> +UINT32 ErrorCode;
> + } SnpPageStateChangeResponse;
> +
>VOID*Ghcb;
>
>UINT64 GhcbPhysicalAddress;
> @@ -69,6 +82,8 @@ typedef union {
> #define GHCB_INFO_CPUID_RESPONSE5
> #define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
> #define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE19
> +#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
> +#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE21
> #define GHCB_HYPERVISOR_FEATURES_REQUEST128
> #define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
> #define GHCB_INFO_TERMINATE_REQUEST 256
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h
> b/MdePkg/Include/Register/Amd/Ghcb.h
> index ec232ebd3807..029904b1c63a 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -54,6 +54,7 @@
> #define SVM_EXIT_NMI_COMPLETE 0x8003ULL
> #define SVM_EXIT_AP_RESET_HOLD 0x8004ULL
> #define SVM_EXIT_AP_JUMP_TABLE 0x8005ULL
> +#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x8010ULL
> #define SVM_EXIT_HYPERVISOR_FEATURES0x8000FFFDULL
> #define SVM_EXIT_UNSUPPORTED0x8000ULL
>
> @@ -162,4 +163,36 @@ typedef union {
> #define GHCB_HV_FEATURES_SNP_AP_CREATE
> (GHCB_HV_FEATURES_SNP | BIT1)
> #define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION
> (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
> #define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER
> (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
> +
> +//
> +// SNP Page State Change.
> +//
> +// Note that the PSMASH and UNSMASH operations are not supported when using
> the MSR protocol.
> +//
> +#define SNP_PAGE_STATE_PRIVATE 1
> +#define SNP_PAGE_STATE_SHARED 2
> +#define SNP_PAGE_STATE_PSMASH 3
> +#define SNP_PAGE_STATE_UNSMASH 4
> +
> +typedef struct {
> + UINT64 CurrentPage:12;
> + UINT64 GuestFrameNumber:40;
> + UINT64 Operation:4;
> + UINT64 PageSize:1;
> + UINT64 Reserved:7;
> +} SNP_PAGE_STATE_ENTRY;
> +
> +typedef struct {
> + UINT16 CurrentEntry;
> + UINT16 EndEntry;
> + UINT32 Reserved;
> +} SNP_PAGE_STATE_HEADER;
> +
> +#define SNP_PAGE_STATE_MAX_ENTRY253
> +
> +typedef struct {
> + SNP_PAGE_STATE_HEADER Header;
> + SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
> +} SNP_PAGE_STATE_CHANGE_INFO;
> +
> #endif
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75227): https://edk2.groups.io/g/devel/message/75227
Mute This Topic: https://groups.io/mt/82787288/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 04/13] MdePkg/Register/Amd: define GHCB macro for Register GPA structure
I verified that the values align with the GHCB spec publication:
#56421 Revision: 2.00
Just one question: is there any reason why GHCB_* defines are decimal
while the SVM_EXIT_* are all in hexadecimal? Does EDK2 have any
preference?
Reviewed-by: Erdem Aktas
-Erdem
On Wed, May 12, 2021 at 4:46 PM Brijesh Singh wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> An SEV-SNP guest is required to perform the GHCB GPA registration. See
> the GHCB specification for further details.
>
> Cc: James Bottomley
> Cc: Min Xu
> Cc: Jiewen Yao
> Cc: Tom Lendacky
> Cc: Jordan Justen
> Cc: Ard Biesheuvel
> Cc: Laszlo Ersek
> Cc: Erdem Aktas
> Cc: Michael D Kinney
> Cc: Liming Gao
> Cc: Zhiguang Liu
> Reviewed-by: Laszlo Ersek
> Reviewed-by: Liming Gao
> Signed-off-by: Brijesh Singh
> ---
> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h
> b/MdePkg/Include/Register/Amd/Fam17Msr.h
> index cdb8f588ccf8..542e4cdf4782 100644
> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> @@ -53,6 +53,11 @@ typedef union {
> UINT64 Features:52;
>} GhcbHypervisorFeatures;
>
> + struct {
> +UINT64 Function:12;
> +UINT64 GuestFrameNumber:52;
> + } GhcbGpaRegister;
> +
>VOID*Ghcb;
>
>UINT64 GhcbPhysicalAddress;
> @@ -62,6 +67,8 @@ typedef union {
> #define GHCB_INFO_SEV_INFO_GET 2
> #define GHCB_INFO_CPUID_REQUEST 4
> #define GHCB_INFO_CPUID_RESPONSE5
> +#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
> +#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE19
> #define GHCB_HYPERVISOR_FEATURES_REQUEST128
> #define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
> #define GHCB_INFO_TERMINATE_REQUEST 256
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75226): https://edk2.groups.io/g/devel/message/75226
Mute This Topic: https://groups.io/mt/82787284/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 03/13] MdePkg/Register/Amd: define GHCB macros for hypervisor feature detection
I verified that the values align with the GHCB spec publication:
#56421 Revision: 2.00
Reviewed-by: Erdem Aktas
-Erdem
On Wed, May 12, 2021 at 4:46 PM Brijesh Singh wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> Version 2 of GHCB introduces advertisement of features that are supported
> by the hypervisor. See the GHCB spec section 2.2 for an additional details.
>
> Cc: James Bottomley
> Cc: Min Xu
> Cc: Jiewen Yao
> Cc: Tom Lendacky
> Cc: Jordan Justen
> Cc: Ard Biesheuvel
> Cc: Laszlo Ersek
> Cc: Erdem Aktas
> Cc: Michael D Kinney
> Cc: Liming Gao
> Cc: Zhiguang Liu
> Reviewed-by: Laszlo Ersek
> Reviewed-by: Liming Gao
> Signed-off-by: Brijesh Singh
> ---
> MdePkg/Include/Register/Amd/Fam17Msr.h | 7 +++
> MdePkg/Include/Register/Amd/Ghcb.h | 8
> 2 files changed, 15 insertions(+)
>
> diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h
> b/MdePkg/Include/Register/Amd/Fam17Msr.h
> index 7368ce7af02a..cdb8f588ccf8 100644
> --- a/MdePkg/Include/Register/Amd/Fam17Msr.h
> +++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
> @@ -48,6 +48,11 @@ typedef union {
> UINT32 Reserved2:32;
>} GhcbTerminate;
>
> + struct {
> +UINT64 Function:12;
> +UINT64 Features:52;
> + } GhcbHypervisorFeatures;
> +
>VOID*Ghcb;
>
>UINT64 GhcbPhysicalAddress;
> @@ -57,6 +62,8 @@ typedef union {
> #define GHCB_INFO_SEV_INFO_GET 2
> #define GHCB_INFO_CPUID_REQUEST 4
> #define GHCB_INFO_CPUID_RESPONSE5
> +#define GHCB_HYPERVISOR_FEATURES_REQUEST128
> +#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
> #define GHCB_INFO_TERMINATE_REQUEST 256
>
> #define GHCB_TERMINATE_GHCB0
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h
> b/MdePkg/Include/Register/Amd/Ghcb.h
> index 712dc8e769c0..ec232ebd3807 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -54,6 +54,7 @@
> #define SVM_EXIT_NMI_COMPLETE 0x8003ULL
> #define SVM_EXIT_AP_RESET_HOLD 0x8004ULL
> #define SVM_EXIT_AP_JUMP_TABLE 0x8005ULL
> +#define SVM_EXIT_HYPERVISOR_FEATURES0x8000FFFDULL
> #define SVM_EXIT_UNSUPPORTED0x8000ULL
>
> //
> @@ -154,4 +155,11 @@ typedef union {
> #define GHCB_EVENT_INJECTION_TYPE_EXCEPTION 3
> #define GHCB_EVENT_INJECTION_TYPE_SOFT_INT 4
>
> +//
> +// Hypervisor features
> +//
> +#define GHCB_HV_FEATURES_SNP BIT0
> +#define GHCB_HV_FEATURES_SNP_AP_CREATE
> (GHCB_HV_FEATURES_SNP | BIT1)
> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION
> (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER
> (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
> #endif
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75225): https://edk2.groups.io/g/devel/message/75225
Mute This Topic: https://groups.io/mt/82787286/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH RFC v2 21/28] OvmfPkg/MemEncryptSevLib: Add support to validate system RAM
> +//
> +// If the request page state change is shared then invalidate the pages
> before
> +// adding the page in the RMP table.
> +//
> +if (State == SevSnpPagePrivate) {
> + PvalidateRange (Info, 0, i, TRUE);
> +}
Looks like some copy-paste mistake in the comment.
Also, it checks the if hypervisor failed to process all the entries
for shared pages, but I do not see that it is checked if for the
private pages. Is there any reason for that?
> +VmgDone (Ghcb, InterruptState);
> + }
> +}
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index f9355172d6..1c1e911bd0 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -267,6 +267,7 @@
> !else
>
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> !endif
> +
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
>
> [LibraryClasses.common.PEI_CORE]
>HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 3f27d7b90d..804f5d62be 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -271,6 +271,7 @@
> !else
>
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> !endif
> +
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
>
> [LibraryClasses.common.PEI_CORE]
>HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> --
> 2.17.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74706): https://edk2.groups.io/g/devel/message/74706
Mute This Topic: https://groups.io/mt/82479072/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH RFC v2 17/28] OvmfPkg/ResetVector: Invalidate the GHCB page
Hi Brijesh, I have few naive questions inlined: On Fri, Apr 30, 2021 at 4:52 AM Brijesh Singh wrote: > +; Use PVALIDATE instruction to invalidate the page > +mov eax, GHCB_BASE > +mov ecx, 0 > +mov edx, 0 > +DB 0xF2, 0x0F, 0x01, 0xFF > +cmp eax, 0 > +jnz TerminateSevGuestLaunch Any reason why the PVALIDATE return value (EFLAGS.CF) is not checked here? IMO, this might lead some page replay attacks. > > +; > +; The page table built above cleared the memory encryption mask from the > +; GHCB_BASE (aka made it shared). When SEV-SNP is enabled, to maintain > +; the security guarantees, the page state transition from private to > +; shared must go through the page invalidation steps. Invalidate the > +; memory range before loading the page table below. > +; > +; NOTE: the invalidation must happen after zeroing the GHCB memory. This > +; is because, in the 32-bit mode all the access are considered > private. > +; The invalidation before the zero'ing will cause a #VC. > +; > +OneTimeCall InvalidateGHCBPage I am not sure if this is a great idea. 1. Zeroing page content before paging is enabled. We are actually writing 0s encrypted with a guest key. 2. invalidating the page and making it shared. Doesn't this reveal a mapping of what 0's look like when a specific page is encrypted? And when the page is marked as shared, from the guest and host perspective, it is not zeroed but filled with some data that looks random. So what is the purpose of zeroing the page before invalidation? Thanks -Erdem -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74702): https://edk2.groups.io/g/devel/message/74702 Mute This Topic: https://groups.io/mt/82479067/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] Maintainers.txt: Add 'Erdem Aktas' to Confidential Computing reviewers
Add 'Erdem Aktas' as a reviewer for OvmfPkg/Confidential Computing. Signed-off-by: Erdem Aktas --- Maintainers.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Maintainers.txt b/Maintainers.txt index fda3df5de2..cafe6b1ab8 100644 --- a/Maintainers.txt +++ b/Maintainers.txt @@ -458,6 +458,7 @@ F: OvmfPkg/PlatformPei/AmdSev.c F: OvmfPkg/ResetVector/ F: OvmfPkg/Sec/ R: Brijesh Singh +R: Erdem Aktas R: James Bottomley R: Jiewen Yao R: Min Xu -- 2.31.1.498.g6c1eba8ee3d-goog -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74372): https://edk2.groups.io/g/devel/message/74372 Mute This Topic: https://groups.io/mt/82287761/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest]
Hi Laszlo,
I am sorry to hear that it sounded like we are dictating a certain
approach. Although I can see why it sounded that way, it certainly was not
my intention.
We want to work with the EDK2 community to have a solution that is
beneficial for everyone and we appreciate the inputs that we got from you
and Paolo. Code quality is always a high priority for us. Therefore, if,
at some point, things get too hacky to impact the
quality/maintainability of the upstream code, we will consider making
adjustments on our side.
With the current discussion, I was just trying to describe our use case and
the importance of having a single binary where there is no absolute need
for architectural differences. As far as I know, the only problematic area
is modifying the reset vector to be compatible with TDX and it seems like
Intel has a solution for it without impacting the overall quality of the
upstream code. I just want to reiterate that we are open for discussion and
what we ask should not be considered "at all cost" and please let us know
that if edk2 community/maintainers are still thinking that what Intel is
proposing is not feasible.
>>Can Google at least propose a designated reviewer ("R") for the
>>"OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a
patch?
Sure I would be happy too.
-Erdem
On Wed, Apr 21, 2021 at 3:44 AM Laszlo Ersek wrote:
> On 04/21/21 02:38, Yao, Jiewen wrote:
> > Hello
> > Do we have some conclusion on this topic?
> >
> > Do we agree the one-binary solution in OVMF or we need more discussion?
>
> Well it's not technically impossible to do, just very ugly and brittle.
> And I'm doubtful that this is a unique problem ("just fix the reset
> vector") the likes of which will supposedly never return during the
> integration of SEV and TDX. Once we make this promise ("one firmware
> binary at all costs"), the hacks we accept for its sake will only
> accumulate over time, and we'll have more and more precedent to justify
> the next hack. Technical debt is not exactly what we don't have enough
> of, in edk2.
>
> I won't make a secret out of the fact that I'm slightly annoyed that
> this approach is being dictated by Google (as far as I understand, at
> this point, anyway). I don't see or recall a lot of Google contributions
> in the edk2 history or the bug tracker. I'm not enthusiastic about
> complexity without explicit commitment / investment on the beneficiary's
> side.
>
> I won't nack the approach personally, but I'm quite unhappy about it.
> Can Google at least propose a designated reviewer ("R") for the
> "OvmfPkg: Confidential Computing" section of "Maintainers.txt", in a patch?
>
> Thanks
> Laszlo
>
> >
> >
> > Thank you
> > Yao Jiewen
> >
> >> -Original Message-
> >> From: Erdem Aktas
> >> Sent: Friday, April 16, 2021 3:43 AM
> >> To: Paolo Bonzini
> >> Cc: devel@edk2.groups.io; j...@linux.ibm.com; Yao, Jiewen
> >> ; dgilb...@redhat.com; Laszlo Ersek
> >> ; Xu, Min M ;
> >> thomas.lenda...@amd.com; Brijesh Singh ; Justen,
> >> Jordan L ; Ard Biesheuvel
> >> ; Nathaniel McCallum
> >> ; Ning Yang
> >> Subject: Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg:
> >> Reserve the Secrets and Cpuid page for the SEV-SNP guest]
> >>
> >> Thanks Paolo.
> >>
> >> On Thu, Apr 15, 2021 at 12:59 AM Paolo Bonzini
> >> wrote:
> >>>
> >>> On 15/04/21 01:34, Erdem Aktas wrote:
> We do not want to generate different binaries for AMD, Intel, Intel
> with TDX, AMD with SEV/SNP etc
> >>>
> >>> My question is why the user would want a single binary for VMs with and
> >>> without TDX/SNP. I know there is attestation, but why would you even
> >>> want the _possibility_ that your guest starts running without TDX or
> SNP
> >>> protection, and only find out later via attestation?
> >>
> >> There might be multiple reasons why customers want it but we need this
> >> requirement for a couple of other reasons too.
> >>
> >> We do not only have hardware based confidential VMs. We might have
> >> some other solutions which measure the initial image before boot.
> >> Ultimately we might want to use a common attestation interface where
> >> customers might be running different kinds of VMs. Using a single
> >> binary will make it easier to manage/verify measurements for both of
> >> us and the customers. I am not a PM so I cannot give more context on
> >> customer use cases.
> >>
> >> Another reason is how we deploy and manage guest firmware. We have a
> >> lot of optimization and customization to speed up firmware loading
> >> time and also reduce the time to deploy new builds on the whole fleet
> >> uniformly. Adding a new firmware binary is a big challenge for us to
> >> enable these features. On the top of integration challenges, it will
> >> create maintainability issues in the long run for us when we provide
> >> tools to verify/reproduce the hashes in the attestation report.
> >>
> >>> want the _possibility_ that your guest starts running without TDX or
Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest]
Thanks Paolo. On Thu, Apr 15, 2021 at 12:59 AM Paolo Bonzini wrote: > > On 15/04/21 01:34, Erdem Aktas wrote: > > We do not want to generate different binaries for AMD, Intel, Intel > > with TDX, AMD with SEV/SNP etc > > My question is why the user would want a single binary for VMs with and > without TDX/SNP. I know there is attestation, but why would you even > want the _possibility_ that your guest starts running without TDX or SNP > protection, and only find out later via attestation? There might be multiple reasons why customers want it but we need this requirement for a couple of other reasons too. We do not only have hardware based confidential VMs. We might have some other solutions which measure the initial image before boot. Ultimately we might want to use a common attestation interface where customers might be running different kinds of VMs. Using a single binary will make it easier to manage/verify measurements for both of us and the customers. I am not a PM so I cannot give more context on customer use cases. Another reason is how we deploy and manage guest firmware. We have a lot of optimization and customization to speed up firmware loading time and also reduce the time to deploy new builds on the whole fleet uniformly. Adding a new firmware binary is a big challenge for us to enable these features. On the top of integration challenges, it will create maintainability issues in the long run for us when we provide tools to verify/reproduce the hashes in the attestation report. > want the _possibility_ that your guest starts running without TDX or SNP > protection, and only find out later via attestation? I am missing the point here. Customers should rely on only the attestation report to establish the trust. -If firmware does not support TDX and TDX is enabled, that firmware will crash at some point. -If firmware is generic firmware that supports TDX and SNP and others, and TDX is enabled or not, still the customer needs to verify the TDX enablement through attestation. -If firmware is a customized binary compiled to support TDX, irrelevant of TDX being enabled or not, still the customer needs to verify the TDX enablement through attestation. > For a similar reason, OVMF already supports shipping a binary that fails > to boot if SMM is not available to the firmware, because then secure > boot would be trivially circumvented. > > I can understand having a single binary for both TDX or SNP. That's not > a problem since you can set up the SEV startup VMSA to 32-bit protected > mode just like TDX wants. I agree that this is doable but I am not sure if we need to also modify the reset vector for AMD SNP in that case. Also it will not solve our problem. If we start to generate a new firmware for every feature , it will not end well for us, I think. Both TDX and SNP are still new features in the same architecture, and it seems to me that they are sharing a lot of common/similar code. AMD has already made some of their patches in (SEV and SEV-ES) which works very nicely for our use case and integration. Looks like Intel just has an issue on how to fix their reset vector problem. Once they solve it and upstream accepts the changes, I do not see any other big blocker. OVMF was doing a great job on abstracting differences and providing a common interface without creating multiple binaries. I do not see why it should not do the same thing here. > > therefore we were expecting the TDX > > changes to be part of the upstream code. > > Having 1 or more binaries should be unrelated to the changes being > upstream (or more likely, I am misunderstanding you). You are right, it is my bad for not clarifying it. What I mean is we want it to be part of the upstream so it can be easier for us to pull the changes and they are compatible with the changes that SNP is doing but we also do not want to use different configuration files to generate different binaries for each use case. > Thanks, > > Paolo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74143): https://edk2.groups.io/g/devel/message/74143 Mute This Topic: https://groups.io/mt/81969494/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] separate OVMF binary for TDX? [was: OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest]
Hi all, >>Can we please pry a little bit at that "one binary" requirement? I think when we call it a "one binary" requirement, it sounds like we are asking something new but what we are asking is pretty much captured by James Bottomley. We do not want to generate different binaries for AMD, Intel, Intel with TDX, AMD with SEV/SNP etc therefore we were expecting the TDX changes to be part of the upstream code. Of course there can be tuning or customization for specific use cases but those are all future and product specific questions. I just do not see the reason why the upstreamed code should have a limitation of not being able to generate a single binary for the same architecture. -Erdem On Mon, Apr 12, 2021 at 7:34 AM James Bottomley wrote: > > On Mon, 2021-04-12 at 11:54 +, Yao, Jiewen wrote: > > I totally agree with you that from security perspective, the best > > idea to isolate AMD SEV/Intel TDX from standard OVMF. > > There's a big difference between building tuned binaries and separating > the subsystems entirely. Ideally we don't want customers running > images to have to build them differently for Intel or AMD (a bit like > how QEMU/KVM hide the VM differences from users), and Confidential > Computing shares a huge amount of interface similarity, so we wouldn't > want that separated. I think the rule should be that if both Intel and > AMD expose a feature in different ways, OVMF tries to expose a uniform > API for that feature over two differing implementations. > > > Do you want to propose move AMD SEV support to another SEC? > > You mean have an entirely separate SEC for AMD, OVMF and Intel? I > really wouldn't do that: much that's in the SEC: page table setup, > memory mapping and decompression is common to all of them. This all > follows for a lot of the components. > > To build separate binaries, we just need separate dsc and fdf files. > Then I think the goal would be to share as much as possible to avoid > duplicating the maintenance and possibly diverging the user API. > > James > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74106): https://edk2.groups.io/g/devel/message/74106 Mute This Topic: https://groups.io/mt/81969494/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
