BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Under SEV-ES, a NPF intercept for an NPT entry with a reserved bit set
generates a #VC exception. This condition is assumed to be an MMIO access.
VMGEXIT must be used to allow the hypervisor to handle this intercept.
Add support to construct the required GHCB values to support a NPF NAE
event for MMIO. Parse the instruction that generated the #VC exception,
setting the required register values in the GHCB and creating the proper
SW_EXIT_INFO1, SW_EXITINFO2 and SW_SCRATCH values in the GHCB.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
.../Library/VmgExitLib/X64/VmgExitVcHandler.c | 436 ++++++++++++++++++
1 file changed, 436 insertions(+)
diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
index 1c6b472a47c4..50199845ceef 100644
--- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
@@ -224,6 +224,263 @@ GhcbSetRegValid (
Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
}
+/**
+ Return a pointer to the contents of the specified register.
+
+ Based upon the input register, return a pointer to the registers contents
+ in the x86 processor context.
+
+ @param[in] Regs x64 processor context
+ @param[in] Register Register to obtain pointer for
+
+ @retval Pointer to the contents of the requested register
+
+**/
+STATIC
+INT64 *
+GetRegisterPointer (
+ IN EFI_SYSTEM_CONTEXT_X64 *Regs,
+ IN UINT8 Register
+ )
+{
+ UINT64 *Reg;
+
+ switch (Register) {
+ case 0:
+ Reg = &Regs->Rax;
+ break;
+ case 1:
+ Reg = &Regs->Rcx;
+ break;
+ case 2:
+ Reg = &Regs->Rdx;
+ break;
+ case 3:
+ Reg = &Regs->Rbx;
+ break;
+ case 4:
+ Reg = &Regs->Rsp;
+ break;
+ case 5:
+ Reg = &Regs->Rbp;
+ break;
+ case 6:
+ Reg = &Regs->Rsi;
+ break;
+ case 7:
+ Reg = &Regs->Rdi;
+ break;
+ case 8:
+ Reg = &Regs->R8;
+ break;
+ case 9:
+ Reg = &Regs->R9;
+ break;
+ case 10:
+ Reg = &Regs->R10;
+ break;
+ case 11:
+ Reg = &Regs->R11;
+ break;
+ case 12:
+ Reg = &Regs->R12;
+ break;
+ case 13:
+ Reg = &Regs->R13;
+ break;
+ case 14:
+ Reg = &Regs->R14;
+ break;
+ case 15:
+ Reg = &Regs->R15;
+ break;
+ default:
+ Reg = NULL;
+ }
+ ASSERT (Reg != NULL);
+
+ return (INT64 *) Reg;
+}
+
+/**
+ Update the instruction parsing context for displacement bytes.
+
+ @param[in, out] InstructionData Instruction parsing context
+ @param[in] Size The instruction displacement size
+
+**/
+STATIC
+VOID
+UpdateForDisplacement (
+ IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData,
+ IN UINTN Size
+ )
+{
+ InstructionData->DisplacementSize = Size;
+ InstructionData->Immediate += Size;
+ InstructionData->End += Size;
+}
+
+/**
+ Determine if an instruction address if RIP relative.
+
+ Examine the instruction parsing context to determine if the address offset
+ is relative to the instruction pointer.
+
+ @param[in] InstructionData Instruction parsing context
+
+ @retval TRUE Instruction addressing is RIP relative
+ @retval FALSE Instruction addressing is not RIP relative
+
+**/
+STATIC
+BOOLEAN
+IsRipRelative (
+ IN SEV_ES_INSTRUCTION_DATA *InstructionData
+ )
+{
+ SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
+
+ Ext = &InstructionData->Ext;
+
+ return ((InstructionData->Mode == LongMode64Bit) &&
+ (Ext->ModRm.Mod == 0) &&
+ (Ext->ModRm.Rm == 5) &&
+ (InstructionData->SibPresent == FALSE));
+}
+
+/**
+ Return the effective address of a memory operand.
+
+ Examine the instruction parsing context to obtain the effective memory
+ address of a memory operand.
+
+ @param[in] Regs x64 processor context
+ @param[in] InstructionData Instruction parsing context
+
+ @retval The memory operand effective address
+
+**/
+STATIC
+UINTN
+GetEffectiveMemoryAddress (
+ IN EFI_SYSTEM_CONTEXT_X64 *Regs,
+ IN SEV_ES_INSTRUCTION_DATA *InstructionData
+ )
+{
+ SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
+ INTN EffectiveAddress;
+
+ Ext = &InstructionData->Ext;
+ EffectiveAddress = 0;
+
+ if (IsRipRelative (InstructionData)) {
+ /* RIP-relative displacement is a 32-bit signed value */
+ INT32 RipRelative;
+
+ RipRelative = *(INT32 *) InstructionData->Displacement;
+
+ UpdateForDisplacement (InstructionData, 4);
+ return (UINTN) ((INTN) Regs->Rip + RipRelative);
+ }
+
+ switch (Ext->ModRm.Mod) {
+ case 1:
+ UpdateForDisplacement (InstructionData, 1);
+ EffectiveAddress += (INT8) (*(INT8 *) (InstructionData->Displacement));
+ break;
+ case 2:
+ switch (InstructionData->AddrSize) {
+ case Size16Bits:
+ UpdateForDisplacement (InstructionData, 2);
+ EffectiveAddress += (INT16) (*(INT16 *) (InstructionData->Displacement));
+ break;
+ default:
+ UpdateForDisplacement (InstructionData, 4);
+ EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
+ break;
+ }
+ break;
+ }
+
+ if (InstructionData->SibPresent) {
+ if (Ext->Sib.Index != 4) {
+ EffectiveAddress += (*GetRegisterPointer (Regs, Ext->Sib.Index) << Ext->Sib.Scale);
+ }
+
+ if ((Ext->Sib.Base != 5) || Ext->ModRm.Mod) {
+ EffectiveAddress += *GetRegisterPointer (Regs, Ext->Sib.Base);
+ } else {
+ UpdateForDisplacement (InstructionData, 4);
+ EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
+ }
+ } else {
+ EffectiveAddress += *GetRegisterPointer (Regs, Ext->ModRm.Rm);
+ }
+
+ return (UINTN) EffectiveAddress;
+}
+
+/**
+ Decode a ModRM byte.
+
+ Examine the instruction parsing context to decode a ModRM byte and the SIB
+ byte, if present.
+
+ @param[in] Regs x64 processor context
+ @param[in, out] InstructionData Instruction parsing context
+
+**/
+STATIC
+VOID
+DecodeModRm (
+ IN EFI_SYSTEM_CONTEXT_X64 *Regs,
+ IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
+ )
+{
+ SEV_ES_INSTRUCTION_REX_PREFIX *RexPrefix;
+ SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
+ SEV_ES_INSTRUCTION_MODRM *ModRm;
+ SEV_ES_INSTRUCTION_SIB *Sib;
+
+ RexPrefix = &InstructionData->RexPrefix;
+ Ext = &InstructionData->Ext;
+ ModRm = &InstructionData->ModRm;
+ Sib = &InstructionData->Sib;
+
+ InstructionData->ModRmPresent = TRUE;
+ ModRm->Uint8 = *(InstructionData->End);
+
+ InstructionData->Displacement++;
+ InstructionData->Immediate++;
+ InstructionData->End++;
+
+ Ext->ModRm.Mod = ModRm->Bits.Mod;
+ Ext->ModRm.Reg = (RexPrefix->Bits.BitR << 3) | ModRm->Bits.Reg;
+ Ext->ModRm.Rm = (RexPrefix->Bits.BitB << 3) | ModRm->Bits.Rm;
+
+ Ext->RegData = *GetRegisterPointer (Regs, Ext->ModRm.Reg);
+
+ if (Ext->ModRm.Mod == 3) {
+ Ext->RmData = *GetRegisterPointer (Regs, Ext->ModRm.Rm);
+ } else {
+ if (ModRm->Bits.Rm == 4) {
+ InstructionData->SibPresent = TRUE;
+ Sib->Uint8 = *(InstructionData->End);
+
+ InstructionData->Displacement++;
+ InstructionData->Immediate++;
+ InstructionData->End++;
+
+ Ext->Sib.Scale = Sib->Bits.Scale;
+ Ext->Sib.Index = (RexPrefix->Bits.BitX << 3) | Sib->Bits.Index;
+ Ext->Sib.Base = (RexPrefix->Bits.BitB << 3) | Sib->Bits.Base;
+ }
+
+ Ext->RmData = GetEffectiveMemoryAddress (Regs, InstructionData);
+ }
+}
+
/**
Decode instruction prefixes.
@@ -411,6 +668,181 @@ UnsupportedExit (
return Status;
}
+/**
+ Handle an MMIO event.
+
+ Use the VMGEXIT instruction to handle either an MMIO read or an MMIO write.
+
+ @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
+ Block
+ @param[in, out] Regs x64 processor context
+ @param[in, out] InstructionData Instruction parsing context
+
+ @retval 0 Event handled successfully
+ @retval Others New exception value to propagate
+
+**/
+STATIC
+UINT64
+MmioExit (
+ IN OUT GHCB *Ghcb,
+ IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
+ IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
+ )
+{
+ UINT64 ExitInfo1, ExitInfo2, Status;
+ UINTN Bytes;
+ INTN *Register;
+ UINT8 OpCode, SignByte;
+
+ Bytes = 0;
+
+ OpCode = *(InstructionData->OpCodes);
+ if (OpCode == 0x0F) {
+ OpCode = *(InstructionData->OpCodes + 1);
+ }
+
+ switch (OpCode) {
+ /* MMIO write */
+ case 0x88:
+ Bytes = 1;
+ case 0x89:
+ DecodeModRm (Regs, InstructionData);
+ Bytes = (Bytes) ? Bytes
+ : (InstructionData->DataSize == Size16Bits) ? 2
+ : (InstructionData->DataSize == Size32Bits) ? 4
+ : (InstructionData->DataSize == Size64Bits) ? 8
+ : 0;
+
+ if (InstructionData->Ext.ModRm.Mod == 3) {
+ /* NPF on two register operands??? */
+ return UnsupportedExit (Ghcb, Regs, InstructionData);
+ }
+
+ ExitInfo1 = InstructionData->Ext.RmData;
+ ExitInfo2 = Bytes;
+ CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
+ if (Status) {
+ return Status;
+ }
+ break;
+
+ case 0xC6:
+ Bytes = 1;
+ case 0xC7:
+ DecodeModRm (Regs, InstructionData);
+ Bytes = (Bytes) ? Bytes
+ : (InstructionData->DataSize == Size16Bits) ? 2
+ : (InstructionData->DataSize == Size32Bits) ? 4
+ : 0;
+
+ InstructionData->ImmediateSize = Bytes;
+ InstructionData->End += Bytes;
+
+ ExitInfo1 = InstructionData->Ext.RmData;
+ ExitInfo2 = Bytes;
+ CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
+ if (Status) {
+ return Status;
+ }
+ break;
+
+ /* MMIO read */
+ case 0x8A:
+ Bytes = 1;
+ case 0x8B:
+ DecodeModRm (Regs, InstructionData);
+ Bytes = (Bytes) ? Bytes
+ : (InstructionData->DataSize == Size16Bits) ? 2
+ : (InstructionData->DataSize == Size32Bits) ? 4
+ : (InstructionData->DataSize == Size64Bits) ? 8
+ : 0;
+ if (InstructionData->Ext.ModRm.Mod == 3) {
+ /* NPF on two register operands??? */
+ return UnsupportedExit (Ghcb, Regs, InstructionData);
+ }
+
+ ExitInfo1 = InstructionData->Ext.RmData;
+ ExitInfo2 = Bytes;
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+ if (Status) {
+ return Status;
+ }
+
+ Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
+ if (Bytes == 4) {
+ /* Zero-extend for 32-bit operation */
+ *Register = 0;
+ }
+ CopyMem (Register, Ghcb->SharedBuffer, Bytes);
+ break;
+
+ /* MMIO Read w/ zero-extension */
+ case 0xB6:
+ Bytes = 1;
+ case 0xB7:
+ Bytes = (Bytes) ? Bytes : 2;
+
+ ExitInfo1 = InstructionData->Ext.RmData;
+ ExitInfo2 = Bytes;
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+ if (Status) {
+ return Status;
+ }
+
+ Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
+ SetMem (Register, InstructionData->DataSize, 0);
+ CopyMem (Register, Ghcb->SharedBuffer, Bytes);
+ break;
+
+ /* MMIO Read w/ sign-extension */
+ case 0xBE:
+ Bytes = 1;
+ case 0xBF:
+ Bytes = (Bytes) ? Bytes : 2;
+
+ ExitInfo1 = InstructionData->Ext.RmData;
+ ExitInfo2 = Bytes;
+
+ Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+ Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+ if (Status) {
+ return Status;
+ }
+
+ if (Bytes == 1) {
+ UINT8 *Data = (UINT8 *) Ghcb->SharedBuffer;
+
+ SignByte = (*Data & 0x80) ? 0xFF : 0x00;
+ } else {
+ UINT16 *Data = (UINT16 *) Ghcb->SharedBuffer;
+
+ SignByte = (*Data & 0x8000) ? 0xFF : 0x00;
+ }
+
+ Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
+ SetMem (Register, InstructionData->DataSize, SignByte);
+ CopyMem (Register, Ghcb->SharedBuffer, Bytes);
+ break;
+
+ default:
+ Status = GP_EXCEPTION;
+ ASSERT (FALSE);
+ }
+
+ return Status;
+}
+
/**
Handle an MSR event.
@@ -806,6 +1238,10 @@ VmgExitHandleVc (
NaeExit = MsrExit;
break;
+ case SVM_EXIT_NPF:
+ NaeExit = MmioExit;
+ break;
+
default:
NaeExit = UnsupportedExit;
}
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59873): https://edk2.groups.io/g/devel/message/59873
Mute This Topic: https://groups.io/mt/74336571/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> Under SEV-ES, a NPF intercept for an NPT entry with a reserved bit set
> generates a #VC exception. This condition is assumed to be an MMIO access.
> VMGEXIT must be used to allow the hypervisor to handle this intercept.
>
> Add support to construct the required GHCB values to support a NPF NAE
> event for MMIO. Parse the instruction that generated the #VC exception,
> setting the required register values in the GHCB and creating the proper
> SW_EXIT_INFO1, SW_EXITINFO2 and SW_SCRATCH values in the GHCB.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 436 ++++++++++++++++++
> 1 file changed, 436 insertions(+)
>
> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> index 1c6b472a47c4..50199845ceef 100644
> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> @@ -224,6 +224,263 @@ GhcbSetRegValid (
> Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
> }
>
> +/**
> + Return a pointer to the contents of the specified register.
> +
> + Based upon the input register, return a pointer to the registers contents
> + in the x86 processor context.
> +
> + @param[in] Regs x64 processor context
> + @param[in] Register Register to obtain pointer for
> +
> + @retval Pointer to the contents of the requested register
> +
> +**/
> +STATIC
> +INT64 *
(1) Please change the return type from (INT64*) to (UINT64*).
My request will look more justified once I get to the rest of my points
below.
> +GetRegisterPointer (
> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN UINT8 Register
> + )
> +{
> + UINT64 *Reg;
> +
> + switch (Register) {
> + case 0:
> + Reg = &Regs->Rax;
> + break;
> + case 1:
> + Reg = &Regs->Rcx;
> + break;
> + case 2:
> + Reg = &Regs->Rdx;
> + break;
> + case 3:
> + Reg = &Regs->Rbx;
> + break;
> + case 4:
> + Reg = &Regs->Rsp;
> + break;
> + case 5:
> + Reg = &Regs->Rbp;
> + break;
> + case 6:
> + Reg = &Regs->Rsi;
> + break;
> + case 7:
> + Reg = &Regs->Rdi;
> + break;
> + case 8:
> + Reg = &Regs->R8;
> + break;
> + case 9:
> + Reg = &Regs->R9;
> + break;
> + case 10:
> + Reg = &Regs->R10;
> + break;
> + case 11:
> + Reg = &Regs->R11;
> + break;
> + case 12:
> + Reg = &Regs->R12;
> + break;
> + case 13:
> + Reg = &Regs->R13;
> + break;
> + case 14:
> + Reg = &Regs->R14;
> + break;
> + case 15:
> + Reg = &Regs->R15;
> + break;
> + default:
> + Reg = NULL;
> + }
> + ASSERT (Reg != NULL);
> +
> + return (INT64 *) Reg;
> +}
(2) Please remove the cast in the "return" statement.
> +
> +/**
> + Update the instruction parsing context for displacement bytes.
> +
> + @param[in, out] InstructionData Instruction parsing context
> + @param[in] Size The instruction displacement size
> +
> +**/
> +STATIC
> +VOID
> +UpdateForDisplacement (
> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData,
> + IN UINTN Size
> + )
> +{
> + InstructionData->DisplacementSize = Size;
> + InstructionData->Immediate += Size;
> + InstructionData->End += Size;
> +}
> +
> +/**
> + Determine if an instruction address if RIP relative.
> +
> + Examine the instruction parsing context to determine if the address offset
> + is relative to the instruction pointer.
> +
> + @param[in] InstructionData Instruction parsing context
> +
> + @retval TRUE Instruction addressing is RIP relative
> + @retval FALSE Instruction addressing is not RIP relative
> +
> +**/
> +STATIC
> +BOOLEAN
> +IsRipRelative (
> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> +
> + Ext = &InstructionData->Ext;
> +
> + return ((InstructionData->Mode == LongMode64Bit) &&
> + (Ext->ModRm.Mod == 0) &&
> + (Ext->ModRm.Rm == 5) &&
> + (InstructionData->SibPresent == FALSE));
> +}
> +
> +/**
> + Return the effective address of a memory operand.
> +
> + Examine the instruction parsing context to obtain the effective memory
> + address of a memory operand.
> +
> + @param[in] Regs x64 processor context
> + @param[in] InstructionData Instruction parsing context
> +
> + @retval The memory operand effective address
> +
> +**/
> +STATIC
> +UINTN
(3) Please make the return type UINT64.
It doesn't change behavior at all, as this is X64-only code, but it will
make our reasoning easier.
(The return value of GetEffectiveMemoryAddress() is assigned to
Ext->RmData (SEV_ES_INSTRUCTION_OPCODE_EXT.RmData) later, which has type
UINTN. But this is X64-only code, so that assignment is fine.)
> +GetEffectiveMemoryAddress (
> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> + INTN EffectiveAddress;
(4) Please make this a UINT64 too.
> +
> + Ext = &InstructionData->Ext;
> + EffectiveAddress = 0;
> +
> + if (IsRipRelative (InstructionData)) {
> + /* RIP-relative displacement is a 32-bit signed value */
(5) Please update the comment style.
> + INT32 RipRelative;
> +
> + RipRelative = *(INT32 *) InstructionData->Displacement;
OK.
> +
> + UpdateForDisplacement (InstructionData, 4);
> + return (UINTN) ((INTN) Regs->Rip + RipRelative);
So, casting "Regs->Rip" (of type UINT64) to INTN is where I start
fidgeting :) The C standard says in "6.3.1.3 Signed and unsigned
integers", paragraph 3:
Otherwise, the new type is signed and the value cannot be represented
in it; either the result is implementation-defined or an
implementation-defined signal is raised.
Now I *do* realize that our particular C language implementation(s) *do*
define the behavior here. If Rip is in the upper half of the address
space, we flip to negative (in two's complement representation), perform
the signed addition, then flip back to positive (which is *not*
implementation defined but standard-defined, but will do the right thing
here).
But that's way too hard to follow if you actually want to pay attention
to the signed/unsigned conversions. We can do this without relying on
the implementation-dependent two's complement representation. Here's
what I suggest:
RipRelative is an INT32, and may be negative. Consider the cast
(UINT64)RipRelative
If RipRelative is non-negative, then the value doesn't change (we'll
perform a plain increment).
If RipRelative is negative, we'll get the following value from the cast,
mathematically speaking:
(MAX_UINT64 + 1) - (-RipRelative) [*]
which is just a different way of writing
(MAX_UINT64 + 1) + RipRelative
And the latter comes straight from the C standard, 6.3.1.3p2:
Otherwise, if the new type is unsigned, the value is converted by
repeatedly adding or subtracting one more than the maximum value that
can be represented in the new type until the value is in the range of
the new type.
Now consider what happens when we add [*] to Regs->Rip (which is itself
a UINT64):
Regs->Rip + ((MAX_UINT64 + 1) - (-RipRelative))
Unpack the outer parens:
Regs->Rip + (MAX_UINT64 + 1) - (-RipRelative)
making for
(Regs->Rip + (MAX_UINT64 + 1)) - (-RipRelative)
The middle term falls away, per "6.2.5 Types", paragraph 9:
[...] A computation involving unsigned operands can never overflow,
because a result that cannot be represented by the resulting unsigned
integer type is reduced modulo the number that is one greater than the
largest value that can be represented by the resulting type.
Therefore we get:
Regs->Rip - (-RipRelative)
which is exactly what we want, for a negative RipRelative.
(6) Thus, the return statement should be:
//
// Negative displacement is handled by standard UINT64 wrap-around.
//
return Regs->Rip + (UINT64)RipRelative;
(Technically, we could even drop the explicit (UINT64) cast --
RipRelative would be converted automatically to UINT64 --, but we should
keep the (UINT64) cast for documentation purposes.)
> + }
> +
> + switch (Ext->ModRm.Mod) {
> + case 1:
> + UpdateForDisplacement (InstructionData, 1);
> + EffectiveAddress += (INT8) (*(INT8 *) (InstructionData->Displacement));
Considering the patch as-is, the outer (INT8) cast is redundant. But,
that's not really my point. My point is how we should update this, after
changing the type of EffectiveAddress to UINT64:
(7) Replace the outer (INT8) cast with (UINT64).
EffectiveAddress += (UINT64) (*(INT8 *) (InstructionData->Displacement));
The reasoning is the same as for (6). If the displacement is negative,
the value we get on the right hand side is
(MAX_UINT64 + 1) - (-Displacement)
And when we add that to EffectiveAddress (also of type UINT64), the
(MAX_UINT64 + 1) term falls away, and we get
EffectiveAddress - (-Displacement)
(The UINT64 conversion would happen anyway, per the "usual arithmetic
conversions", given the new UINT64 type of EffectiveAddress; so the cast
is mainly for documentation, again.)
> + break;
> + case 2:
> + switch (InstructionData->AddrSize) {
> + case Size16Bits:
> + UpdateForDisplacement (InstructionData, 2);
> + EffectiveAddress += (INT16) (*(INT16 *) (InstructionData->Displacement));
(8) Same as (7); please change the outer cast to (UINT64).
> + break;
> + default:
> + UpdateForDisplacement (InstructionData, 4);
> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
(9) Same as (7); please change the outer cast to (UINT64).
> + break;
> + }
> + break;
> + }
> +
> + if (InstructionData->SibPresent) {
> + if (Ext->Sib.Index != 4) {
> + EffectiveAddress += (*GetRegisterPointer (Regs, Ext->Sib.Index) << Ext->Sib.Scale);
In the patch, as-is, we're left-shifting an INT64 that may be negative.
That's not defined by the standard; see "6.5.7 Bitwise shift operators",
p4:
[...] If E1 has a signed type and nonnegative value, and E1 * 2^E2 is
representable in the result type, then that is the resulting value;
otherwise, the behavior is undefined.
(10) Therefore we should do:
INT64 Displacement;
CopyMem (&Displacement, GetRegisterPointer (Regs, Ext->Sib.Index),
sizeof Displacement);
Displacement *= (1 << Ext->Sib.Scale);
//
// Negative displacement is handled by standard UINT64 wrap-around.
//
EffectiveAddress += (UINT64)Displacement;
Assuming that the instruction we're decoding isn't malformed in the
first place, this is safe.
(10a) The CopyMem could be replaced with
Displacement = *(INT64 *)GetRegisterPointer (Regs, Ext->Sib.Index);
but the CopyMem() is cleaner. (It is where we *explicitly* rely on two's
complement representation.)
(10b) "Ext->Sib.Scale" is at most 3 (from DecodeModRm() below -- it
comes from a 2-bits wide bitfield), so left-shifting value 1 (of type
INT32) is OK.
(10c) Multiplying a negative INT64 by 1, 2, 4, or 8 is well-defined
(assuming again that the initial Displacement value is small enough,
which depends on the original instruction).
If we wanted to be super-safe, we could replace this open-coded
INT64 multiplication with a call to SafeInt64Mult(), from
<Library/SafeIntLib.h>, and hang here, if the call fails.
Up to you.
(10d) The final addition follows the same argument as above. We could
again drop the UINT64 cast (the INT64 operand would be converted to
UINT64 via the "usual arithmetic conversions"), but we should keep it
for documentation purposes.
> + }
> +
> + if ((Ext->Sib.Base != 5) || Ext->ModRm.Mod) {
> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->Sib.Base);
This will just continue working, with EffectiveAddress and
(*GetRegisterPointer()) being both UINT64's. A negative displacement
will be encoded within the register that (*GetRegisterPointer()) reads
out.
> + } else {
> + UpdateForDisplacement (InstructionData, 4);
> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
(11) Same as (9) -- please change the outer (INT32) cast to (UINT64),
for documentation.
> + }
> + } else {
> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->ModRm.Rm);
Continues working fine.
> + }
> +
> + return (UINTN) EffectiveAddress;
(12) Please drop the cast.
> +}
> +
> +/**
> + Decode a ModRM byte.
> +
> + Examine the instruction parsing context to decode a ModRM byte and the SIB
> + byte, if present.
> +
> + @param[in] Regs x64 processor context
> + @param[in, out] InstructionData Instruction parsing context
> +
> +**/
> +STATIC
> +VOID
> +DecodeModRm (
> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_REX_PREFIX *RexPrefix;
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> + SEV_ES_INSTRUCTION_MODRM *ModRm;
> + SEV_ES_INSTRUCTION_SIB *Sib;
> +
> + RexPrefix = &InstructionData->RexPrefix;
> + Ext = &InstructionData->Ext;
> + ModRm = &InstructionData->ModRm;
> + Sib = &InstructionData->Sib;
> +
> + InstructionData->ModRmPresent = TRUE;
> + ModRm->Uint8 = *(InstructionData->End);
> +
> + InstructionData->Displacement++;
> + InstructionData->Immediate++;
> + InstructionData->End++;
> +
> + Ext->ModRm.Mod = ModRm->Bits.Mod;
> + Ext->ModRm.Reg = (RexPrefix->Bits.BitR << 3) | ModRm->Bits.Reg;
> + Ext->ModRm.Rm = (RexPrefix->Bits.BitB << 3) | ModRm->Bits.Rm;
> +
> + Ext->RegData = *GetRegisterPointer (Regs, Ext->ModRm.Reg);
> +
> + if (Ext->ModRm.Mod == 3) {
> + Ext->RmData = *GetRegisterPointer (Regs, Ext->ModRm.Rm);
Both of these UINTN field assignments will continue working, with
GetRegisterPointer() returning (UINT64*).
> + } else {
> + if (ModRm->Bits.Rm == 4) {
> + InstructionData->SibPresent = TRUE;
> + Sib->Uint8 = *(InstructionData->End);
> +
> + InstructionData->Displacement++;
> + InstructionData->Immediate++;
> + InstructionData->End++;
> +
> + Ext->Sib.Scale = Sib->Bits.Scale;
> + Ext->Sib.Index = (RexPrefix->Bits.BitX << 3) | Sib->Bits.Index;
> + Ext->Sib.Base = (RexPrefix->Bits.BitB << 3) | Sib->Bits.Base;
> + }
> +
> + Ext->RmData = GetEffectiveMemoryAddress (Regs, InstructionData);
> + }
> +}
> +
> /**
> Decode instruction prefixes.
>
> @@ -411,6 +668,181 @@ UnsupportedExit (
> return Status;
> }
>
> +/**
> + Handle an MMIO event.
> +
> + Use the VMGEXIT instruction to handle either an MMIO read or an MMIO write.
> +
> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
> + Block
> + @param[in, out] Regs x64 processor context
> + @param[in, out] InstructionData Instruction parsing context
> +
> + @retval 0 Event handled successfully
> + @retval Others New exception value to propagate
> +
> +**/
> +STATIC
> +UINT64
> +MmioExit (
> + IN OUT GHCB *Ghcb,
> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + UINT64 ExitInfo1, ExitInfo2, Status;
> + UINTN Bytes;
> + INTN *Register;
(13) Please change this to (UINT64 *).
> + UINT8 OpCode, SignByte;
> +
> + Bytes = 0;
> +
> + OpCode = *(InstructionData->OpCodes);
> + if (OpCode == 0x0F) {
> + OpCode = *(InstructionData->OpCodes + 1);
> + }
(14) Can you add a comment regarding the 0x0F constant?
> +
> + switch (OpCode) {
> + /* MMIO write */
(15) Please update the comment style.
Also, can we be more explicit about the opcodes, with comments?
> + case 0x88:
> + Bytes = 1;
(16) Please add a "fall through" comment.
> + case 0x89:
> + DecodeModRm (Regs, InstructionData);
> + Bytes = (Bytes) ? Bytes
(17) Please use an explicit (Bytes > 0) comparison.
> + : (InstructionData->DataSize == Size16Bits) ? 2
> + : (InstructionData->DataSize == Size32Bits) ? 4
> + : (InstructionData->DataSize == Size64Bits) ? 8
> + : 0;
I struggled for a while to figure out what bothered me about this syntax
:)
(18) The colons ":" should be at the ends of the lines.
Bytes = ((Bytes > 0) ? Bytes :
(InstructionData->DataSize == Size16Bits) ? 2 :
(InstructionData->DataSize == Size32Bits) ? 4 :
(InstructionData->DataSize == Size64Bits) ? 8 :
0);
(I recommend the outermost parens only for supporting the indentation.)
> +
> + if (InstructionData->Ext.ModRm.Mod == 3) {
> + /* NPF on two register operands??? */
(19) Please update the comment style.
> + return UnsupportedExit (Ghcb, Regs, InstructionData);
> + }
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> + CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
> + if (Status) {
(20) please write (Status > 0) or (Status != 0)
> + return Status;
> + }
> + break;
> +
> + case 0xC6:
> + Bytes = 1;
(21) Please add a "fall through" comment.
> + case 0xC7:
> + DecodeModRm (Regs, InstructionData);
> + Bytes = (Bytes) ? Bytes
> + : (InstructionData->DataSize == Size16Bits) ? 2
> + : (InstructionData->DataSize == Size32Bits) ? 4
> + : 0;
(22) please see (17) and (18)
> +
> + InstructionData->ImmediateSize = Bytes;
> + InstructionData->End += Bytes;
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> + CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
> + if (Status) {
(23) please write (Status > 0) or (Status != 0)
> + return Status;
> + }
> + break;
> +
> + /* MMIO read */
(24) pls see (15)
> + case 0x8A:
> + Bytes = 1;
(25) Please add a "fall through" comment.
> + case 0x8B:
> + DecodeModRm (Regs, InstructionData);
> + Bytes = (Bytes) ? Bytes
> + : (InstructionData->DataSize == Size16Bits) ? 2
> + : (InstructionData->DataSize == Size32Bits) ? 4
> + : (InstructionData->DataSize == Size64Bits) ? 8
> + : 0;
(26) please see (17) and (18)
> + if (InstructionData->Ext.ModRm.Mod == 3) {
> + /* NPF on two register operands??? */
(27) Please update the comment style.
> + return UnsupportedExit (Ghcb, Regs, InstructionData);
> + }
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> + if (Status) {
(28) please write (Status > 0) or (Status != 0)
> + return Status;
> + }
> +
> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> + if (Bytes == 4) {
> + /* Zero-extend for 32-bit operation */
(29) Please update the comment style.
> + *Register = 0;
Continues working with Register having type (UINT64 *).
> + }
> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
> + break;
> +
> + /* MMIO Read w/ zero-extension */
(30) Please see (15)
> + case 0xB6:
> + Bytes = 1;
(31) Please add a "fall through" comment.
> + case 0xB7:
> + Bytes = (Bytes) ? Bytes : 2;
(32) Please use an explicit (Bytes > 0) comparison.
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> + if (Status) {
(33) please write (Status > 0) or (Status != 0)
> + return Status;
> + }
> +
> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> + SetMem (Register, InstructionData->DataSize, 0);
> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
> + break;
> +
> + /* MMIO Read w/ sign-extension */
(34) Please see (15)
> + case 0xBE:
> + Bytes = 1;
(35) Please add a "fall through" comment.
> + case 0xBF:
> + Bytes = (Bytes) ? Bytes : 2;
(36) Please see (17)
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> + if (Status) {
(37) please write (Status > 0) or (Status != 0)
> + return Status;
> + }
> +
> + if (Bytes == 1) {
> + UINT8 *Data = (UINT8 *) Ghcb->SharedBuffer;
> +
> + SignByte = (*Data & 0x80) ? 0xFF : 0x00;
(38) Please use BIT7 (or if there's an even better dedicated macro, then
that), rather than 0x80.
(39) Usual comment about bitmask used in logical context.
> + } else {
> + UINT16 *Data = (UINT16 *) Ghcb->SharedBuffer;
> +
> + SignByte = (*Data & 0x8000) ? 0xFF : 0x00;
> + }
(40) Same two comments as (38) and (39).
> +
> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> + SetMem (Register, InstructionData->DataSize, SignByte);
> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
> + break;
> +
> + default:
> + Status = GP_EXCEPTION;
> + ASSERT (FALSE);
> + }
> +
> + return Status;
> +}
> +
> /**
> Handle an MSR event.
>
> @@ -806,6 +1238,10 @@ VmgExitHandleVc (
> NaeExit = MsrExit;
> break;
>
> + case SVM_EXIT_NPF:
> + NaeExit = MmioExit;
> + break;
> +
> default:
> NaeExit = UnsupportedExit;
> }
>
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60144): https://edk2.groups.io/g/devel/message/60144
Mute This Topic: https://groups.io/mt/74336571/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/22/20 16:14, Laszlo Ersek wrote: > INT64 Displacement; > Displacement *= (1 << Ext->Sib.Scale); > (10c) Multiplying a negative INT64 by 1, 2, 4, or 8 is well-defined > (assuming again that the initial Displacement value is small enough, > which depends on the original instruction). > > If we wanted to be super-safe, we could replace this open-coded > INT64 multiplication with a call to SafeInt64Mult(), from > <Library/SafeIntLib.h>, and hang here, if the call fails. > > Up to you. Side comment: normally, even if we were 100% sure the result would be in range, we shouldn't use the "*" operator for INT64 multiplication -- because of intrinsics. BaseLib offers MultS64x64() for that problem. But, this is X64-only code. So using "*" for INT64 multiplication should be fine on all toolchains, regarded purely from an intrinsics perspective. So it's a choice between "*" (if we trust the instruction being decoded to be sane) or SafeInt64Mult() (if we don't). MultS64x64() sits in the middle, and doesn't buy us anything here. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#60146): https://edk2.groups.io/g/devel/message/60146 Mute This Topic: https://groups.io/mt/74336571/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 5/22/20 9:14 AM, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7Cfdd2325c2e5341d8ae5408d7fe5a75f5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257536808864483&sdata=RICgoxIHQzIwTS0UWB0gFK39ENqFaoSH%2FeX6DU0h0VI%3D&reserved=0
>>
>> Under SEV-ES, a NPF intercept for an NPT entry with a reserved bit set
>> generates a #VC exception. This condition is assumed to be an MMIO access.
>> VMGEXIT must be used to allow the hypervisor to handle this intercept.
>>
>> Add support to construct the required GHCB values to support a NPF NAE
>> event for MMIO. Parse the instruction that generated the #VC exception,
>> setting the required register values in the GHCB and creating the proper
>> SW_EXIT_INFO1, SW_EXITINFO2 and SW_SCRATCH values in the GHCB.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 436 ++++++++++++++++++
>> 1 file changed, 436 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> index 1c6b472a47c4..50199845ceef 100644
>> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> @@ -224,6 +224,263 @@ GhcbSetRegValid (
>> Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
>> }
>>
>> +/**
>> + Return a pointer to the contents of the specified register.
>> +
>> + Based upon the input register, return a pointer to the registers contents
>> + in the x86 processor context.
>> +
>> + @param[in] Regs x64 processor context
>> + @param[in] Register Register to obtain pointer for
>> +
>> + @retval Pointer to the contents of the requested register
>> +
>> +**/
>> +STATIC
>> +INT64 *
>
> (1) Please change the return type from (INT64*) to (UINT64*).
>
> My request will look more justified once I get to the rest of my points
> below.
>
>> +GetRegisterPointer (
>> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN UINT8 Register
>> + )
>> +{
>> + UINT64 *Reg;
>> +
>> + switch (Register) {
>> + case 0:
>> + Reg = &Regs->Rax;
>> + break;
>> + case 1:
>> + Reg = &Regs->Rcx;
>> + break;
>> + case 2:
>> + Reg = &Regs->Rdx;
>> + break;
>> + case 3:
>> + Reg = &Regs->Rbx;
>> + break;
>> + case 4:
>> + Reg = &Regs->Rsp;
>> + break;
>> + case 5:
>> + Reg = &Regs->Rbp;
>> + break;
>> + case 6:
>> + Reg = &Regs->Rsi;
>> + break;
>> + case 7:
>> + Reg = &Regs->Rdi;
>> + break;
>> + case 8:
>> + Reg = &Regs->R8;
>> + break;
>> + case 9:
>> + Reg = &Regs->R9;
>> + break;
>> + case 10:
>> + Reg = &Regs->R10;
>> + break;
>> + case 11:
>> + Reg = &Regs->R11;
>> + break;
>> + case 12:
>> + Reg = &Regs->R12;
>> + break;
>> + case 13:
>> + Reg = &Regs->R13;
>> + break;
>> + case 14:
>> + Reg = &Regs->R14;
>> + break;
>> + case 15:
>> + Reg = &Regs->R15;
>> + break;
>> + default:
>> + Reg = NULL;
>> + }
>> + ASSERT (Reg != NULL);
>> +
>> + return (INT64 *) Reg;
>> +}
>
> (2) Please remove the cast in the "return" statement.
>
>> +
>> +/**
>> + Update the instruction parsing context for displacement bytes.
>> +
>> + @param[in, out] InstructionData Instruction parsing context
>> + @param[in] Size The instruction displacement size
>> +
>> +**/
>> +STATIC
>> +VOID
>> +UpdateForDisplacement (
>> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData,
>> + IN UINTN Size
>> + )
>> +{
>> + InstructionData->DisplacementSize = Size;
>> + InstructionData->Immediate += Size;
>> + InstructionData->End += Size;
>> +}
>> +
>> +/**
>> + Determine if an instruction address if RIP relative.
>> +
>> + Examine the instruction parsing context to determine if the address offset
>> + is relative to the instruction pointer.
>> +
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @retval TRUE Instruction addressing is RIP relative
>> + @retval FALSE Instruction addressing is not RIP relative
>> +
>> +**/
>> +STATIC
>> +BOOLEAN
>> +IsRipRelative (
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> +
>> + Ext = &InstructionData->Ext;
>> +
>> + return ((InstructionData->Mode == LongMode64Bit) &&
>> + (Ext->ModRm.Mod == 0) &&
>> + (Ext->ModRm.Rm == 5) &&
>> + (InstructionData->SibPresent == FALSE));
>> +}
>> +
>> +/**
>> + Return the effective address of a memory operand.
>> +
>> + Examine the instruction parsing context to obtain the effective memory
>> + address of a memory operand.
>> +
>> + @param[in] Regs x64 processor context
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @retval The memory operand effective address
>> +
>> +**/
>> +STATIC
>> +UINTN
>
> (3) Please make the return type UINT64.
>
> It doesn't change behavior at all, as this is X64-only code, but it will
> make our reasoning easier.
>
> (The return value of GetEffectiveMemoryAddress() is assigned to
> Ext->RmData (SEV_ES_INSTRUCTION_OPCODE_EXT.RmData) later, which has type
> UINTN. But this is X64-only code, so that assignment is fine.)
>
>> +GetEffectiveMemoryAddress (
>> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + INTN EffectiveAddress;
>
> (4) Please make this a UINT64 too.
>
>> +
>> + Ext = &InstructionData->Ext;
>> + EffectiveAddress = 0;
>> +
>> + if (IsRipRelative (InstructionData)) {
>> + /* RIP-relative displacement is a 32-bit signed value */
>
> (5) Please update the comment style.
>
>> + INT32 RipRelative;
>> +
>> + RipRelative = *(INT32 *) InstructionData->Displacement;
>
> OK.
>
>> +
>> + UpdateForDisplacement (InstructionData, 4);
>> + return (UINTN) ((INTN) Regs->Rip + RipRelative);
>
> So, casting "Regs->Rip" (of type UINT64) to INTN is where I start
> fidgeting :) The C standard says in "6.3.1.3 Signed and unsigned
> integers", paragraph 3:
>
> Otherwise, the new type is signed and the value cannot be represented
> in it; either the result is implementation-defined or an
> implementation-defined signal is raised.
>
> Now I *do* realize that our particular C language implementation(s) *do*
> define the behavior here. If Rip is in the upper half of the address
> space, we flip to negative (in two's complement representation), perform
> the signed addition, then flip back to positive (which is *not*
> implementation defined but standard-defined, but will do the right thing
> here).
>
> But that's way too hard to follow if you actually want to pay attention
> to the signed/unsigned conversions. We can do this without relying on
> the implementation-dependent two's complement representation. Here's
> what I suggest:
>
> RipRelative is an INT32, and may be negative. Consider the cast
>
> (UINT64)RipRelative
>
> If RipRelative is non-negative, then the value doesn't change (we'll
> perform a plain increment).
>
> If RipRelative is negative, we'll get the following value from the cast,
> mathematically speaking:
>
> (MAX_UINT64 + 1) - (-RipRelative) [*]
>
> which is just a different way of writing
>
> (MAX_UINT64 + 1) + RipRelative
>
> And the latter comes straight from the C standard, 6.3.1.3p2:
>
> Otherwise, if the new type is unsigned, the value is converted by
> repeatedly adding or subtracting one more than the maximum value that
> can be represented in the new type until the value is in the range of
> the new type.
>
> Now consider what happens when we add [*] to Regs->Rip (which is itself
> a UINT64):
>
> Regs->Rip + ((MAX_UINT64 + 1) - (-RipRelative))
>
> Unpack the outer parens:
>
> Regs->Rip + (MAX_UINT64 + 1) - (-RipRelative)
>
> making for
>
> (Regs->Rip + (MAX_UINT64 + 1)) - (-RipRelative)
>
> The middle term falls away, per "6.2.5 Types", paragraph 9:
>
> [...] A computation involving unsigned operands can never overflow,
> because a result that cannot be represented by the resulting unsigned
> integer type is reduced modulo the number that is one greater than the
> largest value that can be represented by the resulting type.
>
> Therefore we get:
>
> Regs->Rip - (-RipRelative)
>
> which is exactly what we want, for a negative RipRelative.
>
> (6) Thus, the return statement should be:
>
> //
> // Negative displacement is handled by standard UINT64 wrap-around.
> //
> return Regs->Rip + (UINT64)RipRelative;
>
> (Technically, we could even drop the explicit (UINT64) cast --
> RipRelative would be converted automatically to UINT64 --, but we should
> keep the (UINT64) cast for documentation purposes.)
Impressive! I'll make all those changes.
>
>> + }
>> +
>> + switch (Ext->ModRm.Mod) {
>> + case 1:
>> + UpdateForDisplacement (InstructionData, 1);
>> + EffectiveAddress += (INT8) (*(INT8 *) (InstructionData->Displacement));
>
> Considering the patch as-is, the outer (INT8) cast is redundant. But,
> that's not really my point. My point is how we should update this, after
> changing the type of EffectiveAddress to UINT64:
>
> (7) Replace the outer (INT8) cast with (UINT64).
>
> EffectiveAddress += (UINT64) (*(INT8 *) (InstructionData->Displacement));
>
> The reasoning is the same as for (6). If the displacement is negative,
> the value we get on the right hand side is
>
> (MAX_UINT64 + 1) - (-Displacement)
>
> And when we add that to EffectiveAddress (also of type UINT64), the
> (MAX_UINT64 + 1) term falls away, and we get
>
> EffectiveAddress - (-Displacement)
>
> (The UINT64 conversion would happen anyway, per the "usual arithmetic
> conversions", given the new UINT64 type of EffectiveAddress; so the cast
> is mainly for documentation, again.)
>
>> + break;
>> + case 2:
>> + switch (InstructionData->AddrSize) {
>> + case Size16Bits:
>> + UpdateForDisplacement (InstructionData, 2);
>> + EffectiveAddress += (INT16) (*(INT16 *) (InstructionData->Displacement));
>
> (8) Same as (7); please change the outer cast to (UINT64).
>
>> + break;
>> + default:
>> + UpdateForDisplacement (InstructionData, 4);
>> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
>
> (9) Same as (7); please change the outer cast to (UINT64).
>
>> + break;
>> + }
>> + break;
>> + }
>> +
>> + if (InstructionData->SibPresent) {
>> + if (Ext->Sib.Index != 4) {
>> + EffectiveAddress += (*GetRegisterPointer (Regs, Ext->Sib.Index) << Ext->Sib.Scale);
>
> In the patch, as-is, we're left-shifting an INT64 that may be negative.
> That's not defined by the standard; see "6.5.7 Bitwise shift operators",
> p4:
>
> [...] If E1 has a signed type and nonnegative value, and E1 * 2^E2 is
> representable in the result type, then that is the resulting value;
> otherwise, the behavior is undefined.
>
> (10) Therefore we should do:
>
> INT64 Displacement;
>
> CopyMem (&Displacement, GetRegisterPointer (Regs, Ext->Sib.Index),
> sizeof Displacement);
> Displacement *= (1 << Ext->Sib.Scale);
> //
> // Negative displacement is handled by standard UINT64 wrap-around.
> //
> EffectiveAddress += (UINT64)Displacement;
>
> Assuming that the instruction we're decoding isn't malformed in the
> first place, this is safe.
>
> (10a) The CopyMem could be replaced with
>
> Displacement = *(INT64 *)GetRegisterPointer (Regs, Ext->Sib.Index);
>
> but the CopyMem() is cleaner. (It is where we *explicitly* rely on two's
> complement representation.)
>
> (10b) "Ext->Sib.Scale" is at most 3 (from DecodeModRm() below -- it
> comes from a 2-bits wide bitfield), so left-shifting value 1 (of type
> INT32) is OK.
>
> (10c) Multiplying a negative INT64 by 1, 2, 4, or 8 is well-defined
> (assuming again that the initial Displacement value is small enough,
> which depends on the original instruction).
>
> If we wanted to be super-safe, we could replace this open-coded
> INT64 multiplication with a call to SafeInt64Mult(), from
> <Library/SafeIntLib.h>, and hang here, if the call fails.
>
> Up to you.
>
> (10d) The final addition follows the same argument as above. We could
> again drop the UINT64 cast (the INT64 operand would be converted to
> UINT64 via the "usual arithmetic conversions"), but we should keep it
> for documentation purposes.
>
>> + }
>> +
>> + if ((Ext->Sib.Base != 5) || Ext->ModRm.Mod) {
>> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->Sib.Base);
>
> This will just continue working, with EffectiveAddress and
> (*GetRegisterPointer()) being both UINT64's. A negative displacement
> will be encoded within the register that (*GetRegisterPointer()) reads
> out.
>
>> + } else {
>> + UpdateForDisplacement (InstructionData, 4);
>> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement));
>
> (11) Same as (9) -- please change the outer (INT32) cast to (UINT64),
> for documentation.
>
>> + }
>> + } else {
>> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->ModRm.Rm);
>
> Continues working fine.
>
>> + }
>> +
>> + return (UINTN) EffectiveAddress;
>
> (12) Please drop the cast.
Ditto here.
>
>> +}
>> +
>> +/**
>> + Decode a ModRM byte.
>> +
>> + Examine the instruction parsing context to decode a ModRM byte and the SIB
>> + byte, if present.
>> +
>> + @param[in] Regs x64 processor context
>> + @param[in, out] InstructionData Instruction parsing context
>> +
>> +**/
>> +STATIC
>> +VOID
>> +DecodeModRm (
>> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_REX_PREFIX *RexPrefix;
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + SEV_ES_INSTRUCTION_MODRM *ModRm;
>> + SEV_ES_INSTRUCTION_SIB *Sib;
>> +
>> + RexPrefix = &InstructionData->RexPrefix;
>> + Ext = &InstructionData->Ext;
>> + ModRm = &InstructionData->ModRm;
>> + Sib = &InstructionData->Sib;
>> +
>> + InstructionData->ModRmPresent = TRUE;
>> + ModRm->Uint8 = *(InstructionData->End);
>> +
>> + InstructionData->Displacement++;
>> + InstructionData->Immediate++;
>> + InstructionData->End++;
>> +
>> + Ext->ModRm.Mod = ModRm->Bits.Mod;
>> + Ext->ModRm.Reg = (RexPrefix->Bits.BitR << 3) | ModRm->Bits.Reg;
>> + Ext->ModRm.Rm = (RexPrefix->Bits.BitB << 3) | ModRm->Bits.Rm;
>> +
>> + Ext->RegData = *GetRegisterPointer (Regs, Ext->ModRm.Reg);
>> +
>> + if (Ext->ModRm.Mod == 3) {
>> + Ext->RmData = *GetRegisterPointer (Regs, Ext->ModRm.Rm);
>
> Both of these UINTN field assignments will continue working, with
> GetRegisterPointer() returning (UINT64*).
>
>> + } else {
>> + if (ModRm->Bits.Rm == 4) {
>> + InstructionData->SibPresent = TRUE;
>> + Sib->Uint8 = *(InstructionData->End);
>> +
>> + InstructionData->Displacement++;
>> + InstructionData->Immediate++;
>> + InstructionData->End++;
>> +
>> + Ext->Sib.Scale = Sib->Bits.Scale;
>> + Ext->Sib.Index = (RexPrefix->Bits.BitX << 3) | Sib->Bits.Index;
>> + Ext->Sib.Base = (RexPrefix->Bits.BitB << 3) | Sib->Bits.Base;
>> + }
>> +
>> + Ext->RmData = GetEffectiveMemoryAddress (Regs, InstructionData);
>> + }
>> +}
>> +
>> /**
>> Decode instruction prefixes.
>>
>> @@ -411,6 +668,181 @@ UnsupportedExit (
>> return Status;
>> }
>>
>> +/**
>> + Handle an MMIO event.
>> +
>> + Use the VMGEXIT instruction to handle either an MMIO read or an MMIO write.
>> +
>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
>> + Block
>> + @param[in, out] Regs x64 processor context
>> + @param[in, out] InstructionData Instruction parsing context
>> +
>> + @retval 0 Event handled successfully
>> + @retval Others New exception value to propagate
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +MmioExit (
>> + IN OUT GHCB *Ghcb,
>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + UINT64 ExitInfo1, ExitInfo2, Status;
>> + UINTN Bytes;
>> + INTN *Register;
>
> (13) Please change this to (UINT64 *).
>
>> + UINT8 OpCode, SignByte;
>> +
>> + Bytes = 0;
>> +
>> + OpCode = *(InstructionData->OpCodes);
>> + if (OpCode == 0x0F) {
>> + OpCode = *(InstructionData->OpCodes + 1);
>> + }
>
> (14) Can you add a comment regarding the 0x0F constant?
I'll create a #define (TWO_BYTE_OPCODE_ESCAPE) that should (hopefully) be
self commenting.
>
>> +
>> + switch (OpCode) {
>> + /* MMIO write */
>
> (15) Please update the comment style.
>
> Also, can we be more explicit about the opcodes, with comments?
Can do.
>
>> + case 0x88:
>> + Bytes = 1;
>
> (16) Please add a "fall through" comment.
For this and remaining comments: Will do.
Thanks!
Tom
>
>> + case 0x89:
>> + DecodeModRm (Regs, InstructionData);
>> + Bytes = (Bytes) ? Bytes
>
> (17) Please use an explicit (Bytes > 0) comparison.
>
>> + : (InstructionData->DataSize == Size16Bits) ? 2
>> + : (InstructionData->DataSize == Size32Bits) ? 4
>> + : (InstructionData->DataSize == Size64Bits) ? 8
>> + : 0;
>
> I struggled for a while to figure out what bothered me about this syntax
> :)
>
> (18) The colons ":" should be at the ends of the lines.
>
> Bytes = ((Bytes > 0) ? Bytes :
> (InstructionData->DataSize == Size16Bits) ? 2 :
> (InstructionData->DataSize == Size32Bits) ? 4 :
> (InstructionData->DataSize == Size64Bits) ? 8 :
> 0);
>
> (I recommend the outermost parens only for supporting the indentation.)
>
>> +
>> + if (InstructionData->Ext.ModRm.Mod == 3) {
>> + /* NPF on two register operands??? */
>
> (19) Please update the comment style.
>
>> + return UnsupportedExit (Ghcb, Regs, InstructionData);
>> + }
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> + CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (20) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> + break;
>> +
>> + case 0xC6:
>> + Bytes = 1;
>
> (21) Please add a "fall through" comment.
>
>> + case 0xC7:
>> + DecodeModRm (Regs, InstructionData);
>> + Bytes = (Bytes) ? Bytes
>> + : (InstructionData->DataSize == Size16Bits) ? 2
>> + : (InstructionData->DataSize == Size32Bits) ? 4
>> + : 0;
>
> (22) please see (17) and (18)
>
>> +
>> + InstructionData->ImmediateSize = Bytes;
>> + InstructionData->End += Bytes;
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> + CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (23) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> + break;
>> +
>> + /* MMIO read */
>
> (24) pls see (15)
>
>> + case 0x8A:
>> + Bytes = 1;
>
> (25) Please add a "fall through" comment.
>
>> + case 0x8B:
>> + DecodeModRm (Regs, InstructionData);
>> + Bytes = (Bytes) ? Bytes
>> + : (InstructionData->DataSize == Size16Bits) ? 2
>> + : (InstructionData->DataSize == Size32Bits) ? 4
>> + : (InstructionData->DataSize == Size64Bits) ? 8
>> + : 0;
>
> (26) please see (17) and (18)
>
>> + if (InstructionData->Ext.ModRm.Mod == 3) {
>> + /* NPF on two register operands??? */
>
> (27) Please update the comment style.
>
>> + return UnsupportedExit (Ghcb, Regs, InstructionData);
>> + }
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (28) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> +
>> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
>> + if (Bytes == 4) {
>> + /* Zero-extend for 32-bit operation */
>
> (29) Please update the comment style.
>
>> + *Register = 0;
>
> Continues working with Register having type (UINT64 *).
>
>> + }
>> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>> + break;
>> +
>> + /* MMIO Read w/ zero-extension */
>
> (30) Please see (15)
>
>> + case 0xB6:
>> + Bytes = 1;
>
> (31) Please add a "fall through" comment.
>
>> + case 0xB7:
>> + Bytes = (Bytes) ? Bytes : 2;
>
> (32) Please use an explicit (Bytes > 0) comparison.
>
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (33) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> +
>> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
>> + SetMem (Register, InstructionData->DataSize, 0);
>> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>> + break;
>> +
>> + /* MMIO Read w/ sign-extension */
>
> (34) Please see (15)
>
>> + case 0xBE:
>> + Bytes = 1;
>
> (35) Please add a "fall through" comment.
>
>> + case 0xBF:
>> + Bytes = (Bytes) ? Bytes : 2;
>
> (36) Please see (17)
>
>> +
>> + ExitInfo1 = InstructionData->Ext.RmData;
>> + ExitInfo2 = Bytes;
>> +
>> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>> + if (Status) {
>
> (37) please write (Status > 0) or (Status != 0)
>
>> + return Status;
>> + }
>> +
>> + if (Bytes == 1) {
>> + UINT8 *Data = (UINT8 *) Ghcb->SharedBuffer;
>> +
>> + SignByte = (*Data & 0x80) ? 0xFF : 0x00;
>
> (38) Please use BIT7 (or if there's an even better dedicated macro, then
> that), rather than 0x80.
>
> (39) Usual comment about bitmask used in logical context.
>
>> + } else {
>> + UINT16 *Data = (UINT16 *) Ghcb->SharedBuffer;
>> +
>> + SignByte = (*Data & 0x8000) ? 0xFF : 0x00;
>> + }
>
> (40) Same two comments as (38) and (39).
>
>> +
>> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
>> + SetMem (Register, InstructionData->DataSize, SignByte);
>> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>> + break;
>> +
>> + default:
>> + Status = GP_EXCEPTION;
>> + ASSERT (FALSE);
>> + }
>> +
>> + return Status;
>> +}
>> +
>> /**
>> Handle an MSR event.
>>
>> @@ -806,6 +1238,10 @@ VmgExitHandleVc (
>> NaeExit = MsrExit;
>> break;
>>
>> + case SVM_EXIT_NPF:
>> + NaeExit = MmioExit;
>> + break;
>> +
>> default:
>> NaeExit = UnsupportedExit;
>> }
>>
>
> Thanks!
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60172): https://edk2.groups.io/g/devel/message/60172
Mute This Topic: https://groups.io/mt/74336571/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.