[v1] SEV-ES TPM enablement fixes

[edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Posted by Lendacky, Thomas 4 years, 9 months ago

From: Tom Lendacky <thomas.lendacky@amd.com>

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

Enabling TPM support results in guest termination of an SEV-ES guest
because it uses MMIO opcodes that are not currently supported.

Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
use a memory offset directly encoded in the instruction. Also, add a DEBUG
statement to identify an unsupported MMIO opcode being used.

Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 273f36499988..f9660b757d8e 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -678,6 +678,7 @@ MmioExit (
   UINTN   Bytes;
   UINT64  *Register;
   UINT8   OpCode, SignByte;
+  UINTN   Address;
 
   Bytes = 0;
 
@@ -727,6 +728,51 @@ MmioExit (
     }
     break;
 
+  //
+  // MMIO write (MOV moffsetX, aX)
+  //
+  case 0xA2:
+    Bytes = 1;
+    //
+    // fall through
+    //
+  case 0xA3:
+    Bytes = ((Bytes != 0) ? Bytes :
+             (InstructionData->DataSize == Size16Bits) ? 2 :
+             (InstructionData->DataSize == Size32Bits) ? 4 :
+             (InstructionData->DataSize == Size64Bits) ? 8 :
+             0);
+
+    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+    if (InstructionData->AddrSize == Size8Bits) {
+      Address = *(UINT8 *) InstructionData->Immediate;
+    } else if (InstructionData->AddrSize == Size16Bits) {
+      Address = *(UINT16 *) InstructionData->Immediate;
+    } else if (InstructionData->AddrSize == Size32Bits) {
+      Address = *(UINT32 *) InstructionData->Immediate;
+    } else {
+      Address = *(UINTN *) InstructionData->Immediate;
+    }
+
+    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+    if (Status != 0) {
+      return Status;
+    }
+
+    ExitInfo1 = Address;
+    ExitInfo2 = Bytes;
+    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
+
+    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
+    if (Status != 0) {
+      return Status;
+    }
+    break;
+
   //
   // MMIO write (MOV reg/memX, immX)
   //
@@ -809,6 +855,58 @@ MmioExit (
     CopyMem (Register, Ghcb->SharedBuffer, Bytes);
     break;
 
+  //
+  // MMIO read (MOV aX, moffsetX)
+  //
+  case 0xA0:
+    Bytes = 1;
+    //
+    // fall through
+    //
+  case 0xA1:
+    Bytes = ((Bytes != 0) ? Bytes :
+             (InstructionData->DataSize == Size16Bits) ? 2 :
+             (InstructionData->DataSize == Size32Bits) ? 4 :
+             (InstructionData->DataSize == Size64Bits) ? 8 :
+             0);
+
+    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
+    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
+
+    if (InstructionData->AddrSize == Size8Bits) {
+      Address = *(UINT8 *) InstructionData->Immediate;
+    } else if (InstructionData->AddrSize == Size16Bits) {
+      Address = *(UINT16 *) InstructionData->Immediate;
+    } else if (InstructionData->AddrSize == Size32Bits) {
+      Address = *(UINT32 *) InstructionData->Immediate;
+    } else {
+      Address = *(UINTN *) InstructionData->Immediate;
+    }
+
+    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
+    if (Status != 0) {
+      return Status;
+    }
+
+    ExitInfo1 = Address;
+    ExitInfo2 = Bytes;
+
+    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
+    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
+    if (Status != 0) {
+      return Status;
+    }
+
+    if (Bytes == 4) {
+      //
+      // Zero-extend for 32-bit operation
+      //
+      Regs->Rax = 0;
+    }
+    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
+    break;
+
   //
   // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
   //
@@ -886,6 +984,7 @@ MmioExit (
     break;
 
   default:
+    DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
     Status = GP_EXCEPTION;
     ASSERT (FALSE);
   }
-- 
2.31.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74310): https://edk2.groups.io/g/devel/message/74310
Mute This Topic: https://groups.io/mt/82247966/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Posted by Laszlo Ersek 4 years, 9 months ago

On 04/21/21 00:54, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
>
> Enabling TPM support results in guest termination of an SEV-ES guest
> because it uses MMIO opcodes that are not currently supported.
>
> Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
> use a memory offset directly encoded in the instruction. Also, add a DEBUG
> statement to identify an unsupported MMIO opcode being used.
>
> Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 273f36499988..f9660b757d8e 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -678,6 +678,7 @@ MmioExit (
>    UINTN   Bytes;
>    UINT64  *Register;
>    UINT8   OpCode, SignByte;
> +  UINTN   Address;
>
>    Bytes = 0;
>
> @@ -727,6 +728,51 @@ MmioExit (
>      }
>      break;
>
> +  //
> +  // MMIO write (MOV moffsetX, aX)
> +  //
> +  case 0xA2:
> +    Bytes = 1;
> +    //
> +    // fall through
> +    //
> +  case 0xA3:
> +    Bytes = ((Bytes != 0) ? Bytes :
> +             (InstructionData->DataSize == Size16Bits) ? 2 :
> +             (InstructionData->DataSize == Size32Bits) ? 4 :
> +             (InstructionData->DataSize == Size64Bits) ? 8 :
> +             0);
> +
> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
> +
> +    if (InstructionData->AddrSize == Size8Bits) {
> +      Address = *(UINT8 *) InstructionData->Immediate;
> +    } else if (InstructionData->AddrSize == Size16Bits) {
> +      Address = *(UINT16 *) InstructionData->Immediate;
> +    } else if (InstructionData->AddrSize == Size32Bits) {
> +      Address = *(UINT32 *) InstructionData->Immediate;
> +    } else {
> +      Address = *(UINTN *) InstructionData->Immediate;
> +    }

(1) Can we simplify this as follows?

    InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
    InstructionData->End += InstructionData->ImmediateSize;
    Address = 0;
    CopyMem (&Address, InstructionData->Immediate,
      InstructionData->ImmediateSize);

> +
> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +
> +    ExitInfo1 = Address;
> +    ExitInfo2 = Bytes;
> +    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
> +
> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +    break;
> +
>    //
>    // MMIO write (MOV reg/memX, immX)
>    //
> @@ -809,6 +855,58 @@ MmioExit (
>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>      break;
>
> +  //
> +  // MMIO read (MOV aX, moffsetX)
> +  //
> +  case 0xA0:
> +    Bytes = 1;
> +    //
> +    // fall through
> +    //
> +  case 0xA1:
> +    Bytes = ((Bytes != 0) ? Bytes :
> +             (InstructionData->DataSize == Size16Bits) ? 2 :
> +             (InstructionData->DataSize == Size32Bits) ? 4 :
> +             (InstructionData->DataSize == Size64Bits) ? 8 :
> +             0);
> +
> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
> +
> +    if (InstructionData->AddrSize == Size8Bits) {
> +      Address = *(UINT8 *) InstructionData->Immediate;
> +    } else if (InstructionData->AddrSize == Size16Bits) {
> +      Address = *(UINT16 *) InstructionData->Immediate;
> +    } else if (InstructionData->AddrSize == Size32Bits) {
> +      Address = *(UINT32 *) InstructionData->Immediate;
> +    } else {
> +      Address = *(UINTN *) InstructionData->Immediate;
> +    }

(2) Similar question as (1).

> +
> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +
> +    ExitInfo1 = Address;
> +    ExitInfo2 = Bytes;
> +
> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> +    if (Status != 0) {
> +      return Status;
> +    }
> +
> +    if (Bytes == 4) {
> +      //
> +      // Zero-extend for 32-bit operation
> +      //
> +      Regs->Rax = 0;
> +    }

(3) This is also seen with opcode 0x8B, but can you remind me please why
we ignore (Bytes == 1) and (Bytes == 2) for zero extension?

> +    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
> +    break;
> +
>    //
>    // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>    //
> @@ -886,6 +984,7 @@ MmioExit (
>      break;
>
>    default:
> +    DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>      Status = GP_EXCEPTION;
>      ASSERT (FALSE);
>    }
>

(4) We should use the DEBUG_ERROR log mask here.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74345): https://edk2.groups.io/g/devel/message/74345
Mute This Topic: https://groups.io/mt/82247966/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Posted by Lendacky, Thomas 4 years, 9 months ago

On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
> On 04/21/21 00:54, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&amp;reserved=0
>>
>> Enabling TPM support results in guest termination of an SEV-ES guest
>> because it uses MMIO opcodes that are not currently supported.
>>
>> Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
>> use a memory offset directly encoded in the instruction. Also, add a DEBUG
>> statement to identify an unsupported MMIO opcode being used.
>>
>> Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
>>  1 file changed, 99 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> index 273f36499988..f9660b757d8e 100644
>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> @@ -678,6 +678,7 @@ MmioExit (
>>    UINTN   Bytes;
>>    UINT64  *Register;
>>    UINT8   OpCode, SignByte;
>> +  UINTN   Address;
>>
>>    Bytes = 0;
>>
>> @@ -727,6 +728,51 @@ MmioExit (
>>      }
>>      break;
>>
>> +  //
>> +  // MMIO write (MOV moffsetX, aX)
>> +  //
>> +  case 0xA2:
>> +    Bytes = 1;
>> +    //
>> +    // fall through
>> +    //
>> +  case 0xA3:
>> +    Bytes = ((Bytes != 0) ? Bytes :
>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>> +             0);
>> +
>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>> +
>> +    if (InstructionData->AddrSize == Size8Bits) {
>> +      Address = *(UINT8 *) InstructionData->Immediate;
>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>> +      Address = *(UINT16 *) InstructionData->Immediate;
>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>> +      Address = *(UINT32 *) InstructionData->Immediate;
>> +    } else {
>> +      Address = *(UINTN *) InstructionData->Immediate;
>> +    }
> 
> (1) Can we simplify this as follows?
> 
>     InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
>     InstructionData->End += InstructionData->ImmediateSize;
>     Address = 0;
>     CopyMem (&Address, InstructionData->Immediate,
>       InstructionData->ImmediateSize);

Yup, that can be done.

> 
>> +
>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>> +    if (Status != 0) {
>> +      return Status;
>> +    }
>> +
>> +    ExitInfo1 = Address;
>> +    ExitInfo2 = Bytes;
>> +    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
>> +
>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>> +    if (Status != 0) {
>> +      return Status;
>> +    }
>> +    break;
>> +
>>    //
>>    // MMIO write (MOV reg/memX, immX)
>>    //
>> @@ -809,6 +855,58 @@ MmioExit (
>>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>>      break;
>>
>> +  //
>> +  // MMIO read (MOV aX, moffsetX)
>> +  //
>> +  case 0xA0:
>> +    Bytes = 1;
>> +    //
>> +    // fall through
>> +    //
>> +  case 0xA1:
>> +    Bytes = ((Bytes != 0) ? Bytes :
>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>> +             0);
>> +
>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>> +
>> +    if (InstructionData->AddrSize == Size8Bits) {
>> +      Address = *(UINT8 *) InstructionData->Immediate;
>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>> +      Address = *(UINT16 *) InstructionData->Immediate;
>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>> +      Address = *(UINT32 *) InstructionData->Immediate;
>> +    } else {
>> +      Address = *(UINTN *) InstructionData->Immediate;
>> +    }
> 
> (2) Similar question as (1).

Will do.

> 
>> +
>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>> +    if (Status != 0) {
>> +      return Status;
>> +    }
>> +
>> +    ExitInfo1 = Address;
>> +    ExitInfo2 = Bytes;
>> +
>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>> +    if (Status != 0) {
>> +      return Status;
>> +    }
>> +
>> +    if (Bytes == 4) {
>> +      //
>> +      // Zero-extend for 32-bit operation
>> +      //
>> +      Regs->Rax = 0;
>> +    }
> 
> (3) This is also seen with opcode 0x8B, but can you remind me please why
> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?

That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
32-bit operand size the 32-bit register results are zero-extended to 64-bits.

> 
>> +    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>> +    break;
>> +
>>    //
>>    // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>>    //
>> @@ -886,6 +984,7 @@ MmioExit (
>>      break;
>>
>>    default:
>> +    DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>>      Status = GP_EXCEPTION;
>>      ASSERT (FALSE);
>>    }
>>
> 
> (4) We should use the DEBUG_ERROR log mask here.

Will change.

Thanks,
Tom

> 
> Thanks
> Laszlo
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74368): https://edk2.groups.io/g/devel/message/74368
Mute This Topic: https://groups.io/mt/82247966/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Posted by Lendacky, Thomas 4 years, 9 months ago

On 4/22/21 9:15 AM, Tom Lendacky wrote:
> On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
>> On 04/21/21 00:54, Lendacky, Thomas wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&amp;reserved=0
>>>
>>> Enabling TPM support results in guest termination of an SEV-ES guest
>>> because it uses MMIO opcodes that are not currently supported.
>>>
>>> Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
>>> use a memory offset directly encoded in the instruction. Also, add a DEBUG
>>> statement to identify an unsupported MMIO opcode being used.
>>>
>>> Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
>>>  1 file changed, 99 insertions(+)
>>>
>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>> index 273f36499988..f9660b757d8e 100644
>>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>> @@ -678,6 +678,7 @@ MmioExit (
>>>    UINTN   Bytes;
>>>    UINT64  *Register;
>>>    UINT8   OpCode, SignByte;
>>> +  UINTN   Address;
>>>
>>>    Bytes = 0;
>>>
>>> @@ -727,6 +728,51 @@ MmioExit (
>>>      }
>>>      break;
>>>
>>> +  //
>>> +  // MMIO write (MOV moffsetX, aX)
>>> +  //
>>> +  case 0xA2:
>>> +    Bytes = 1;
>>> +    //
>>> +    // fall through
>>> +    //
>>> +  case 0xA3:
>>> +    Bytes = ((Bytes != 0) ? Bytes :
>>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>>> +             0);
>>> +
>>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>>> +
>>> +    if (InstructionData->AddrSize == Size8Bits) {
>>> +      Address = *(UINT8 *) InstructionData->Immediate;
>>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>>> +      Address = *(UINT16 *) InstructionData->Immediate;
>>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>>> +      Address = *(UINT32 *) InstructionData->Immediate;
>>> +    } else {
>>> +      Address = *(UINTN *) InstructionData->Immediate;
>>> +    }
>>
>> (1) Can we simplify this as follows?
>>
>>     InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
>>     InstructionData->End += InstructionData->ImmediateSize;
>>     Address = 0;
>>     CopyMem (&Address, InstructionData->Immediate,
>>       InstructionData->ImmediateSize);
> 
> Yup, that can be done.

"Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
isn't an issue. Should I add a comment about that above the setting of
"Address"? Or should I convert "Address" to a UINT64 - although
ValidateMmioMemory expects a UINTN...  Thoughts?

Thanks,
Tom

> 
>>
>>> +
>>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>>> +    if (Status != 0) {
>>> +      return Status;
>>> +    }
>>> +
>>> +    ExitInfo1 = Address;
>>> +    ExitInfo2 = Bytes;
>>> +    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
>>> +
>>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>>> +    if (Status != 0) {
>>> +      return Status;
>>> +    }
>>> +    break;
>>> +
>>>    //
>>>    // MMIO write (MOV reg/memX, immX)
>>>    //
>>> @@ -809,6 +855,58 @@ MmioExit (
>>>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>>>      break;
>>>
>>> +  //
>>> +  // MMIO read (MOV aX, moffsetX)
>>> +  //
>>> +  case 0xA0:
>>> +    Bytes = 1;
>>> +    //
>>> +    // fall through
>>> +    //
>>> +  case 0xA1:
>>> +    Bytes = ((Bytes != 0) ? Bytes :
>>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>>> +             0);
>>> +
>>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>>> +
>>> +    if (InstructionData->AddrSize == Size8Bits) {
>>> +      Address = *(UINT8 *) InstructionData->Immediate;
>>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>>> +      Address = *(UINT16 *) InstructionData->Immediate;
>>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>>> +      Address = *(UINT32 *) InstructionData->Immediate;
>>> +    } else {
>>> +      Address = *(UINTN *) InstructionData->Immediate;
>>> +    }
>>
>> (2) Similar question as (1).
> 
> Will do.
> 
>>
>>> +
>>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>>> +    if (Status != 0) {
>>> +      return Status;
>>> +    }
>>> +
>>> +    ExitInfo1 = Address;
>>> +    ExitInfo2 = Bytes;
>>> +
>>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>>> +    if (Status != 0) {
>>> +      return Status;
>>> +    }
>>> +
>>> +    if (Bytes == 4) {
>>> +      //
>>> +      // Zero-extend for 32-bit operation
>>> +      //
>>> +      Regs->Rax = 0;
>>> +    }
>>
>> (3) This is also seen with opcode 0x8B, but can you remind me please why
>> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
> 
> That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
> 32-bit operand size the 32-bit register results are zero-extended to 64-bits.
> 
>>
>>> +    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>>> +    break;
>>> +
>>>    //
>>>    // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>>>    //
>>> @@ -886,6 +984,7 @@ MmioExit (
>>>      break;
>>>
>>>    default:
>>> +    DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>>>      Status = GP_EXCEPTION;
>>>      ASSERT (FALSE);
>>>    }
>>>
>>
>> (4) We should use the DEBUG_ERROR log mask here.
> 
> Will change.
> 
> Thanks,
> Tom
> 
>>
>> Thanks
>> Laszlo
>>
>>
>>
>> 
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74374): https://edk2.groups.io/g/devel/message/74374
Mute This Topic: https://groups.io/mt/82247966/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Posted by Laszlo Ersek 4 years, 9 months ago

On 04/22/21 17:42, Tom Lendacky wrote:
> On 4/22/21 9:15 AM, Tom Lendacky wrote:
>> On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
>>> On 04/21/21 00:54, Lendacky, Thomas wrote:
>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C19a7d97e2a7b461830ed08d905528472%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546674232278910%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=znSezOvpnItW7mHAJkr%2FtJtkQNFc2H0dG9STpmOpVqU%3D&amp;reserved=0
>>>>
>>>> Enabling TPM support results in guest termination of an SEV-ES guest
>>>> because it uses MMIO opcodes that are not currently supported.
>>>>
>>>> Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
>>>> use a memory offset directly encoded in the instruction. Also, add a DEBUG
>>>> statement to identify an unsupported MMIO opcode being used.
>>>>
>>>> Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Min Xu <min.m.xu@intel.com>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>> ---
>>>>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
>>>>  1 file changed, 99 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>> index 273f36499988..f9660b757d8e 100644
>>>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>> @@ -678,6 +678,7 @@ MmioExit (
>>>>    UINTN   Bytes;
>>>>    UINT64  *Register;
>>>>    UINT8   OpCode, SignByte;
>>>> +  UINTN   Address;
>>>>
>>>>    Bytes = 0;
>>>>
>>>> @@ -727,6 +728,51 @@ MmioExit (
>>>>      }
>>>>      break;
>>>>
>>>> +  //
>>>> +  // MMIO write (MOV moffsetX, aX)
>>>> +  //
>>>> +  case 0xA2:
>>>> +    Bytes = 1;
>>>> +    //
>>>> +    // fall through
>>>> +    //
>>>> +  case 0xA3:
>>>> +    Bytes = ((Bytes != 0) ? Bytes :
>>>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>>>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>>>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>>>> +             0);
>>>> +
>>>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>>>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>>>> +
>>>> +    if (InstructionData->AddrSize == Size8Bits) {
>>>> +      Address = *(UINT8 *) InstructionData->Immediate;
>>>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>>>> +      Address = *(UINT16 *) InstructionData->Immediate;
>>>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>>>> +      Address = *(UINT32 *) InstructionData->Immediate;
>>>> +    } else {
>>>> +      Address = *(UINTN *) InstructionData->Immediate;
>>>> +    }
>>>
>>> (1) Can we simplify this as follows?
>>>
>>>     InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
>>>     InstructionData->End += InstructionData->ImmediateSize;
>>>     Address = 0;
>>>     CopyMem (&Address, InstructionData->Immediate,
>>>       InstructionData->ImmediateSize);
>>
>> Yup, that can be done.
> 
> "Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
> isn't an issue. Should I add a comment about that above the setting of
> "Address"? Or should I convert "Address" to a UINT64 - although
> ValidateMmioMemory expects a UINTN...  Thoughts?

Yes, I had the exact same thought process :)

The comment looks good, but how about expressing it as a STATIC_ASSERT,
with sizeof (UINTN) and sizeof (UINT64) being equal? (Alternatively,
about MAX_UINT64 being equal to MAX_UINTN.)

If you find that too verbose, a comment is good enough too, of course.

Thanks!
Laszlo

> 
> Thanks,
> Tom
> 
>>
>>>
>>>> +
>>>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>>>> +    if (Status != 0) {
>>>> +      return Status;
>>>> +    }
>>>> +
>>>> +    ExitInfo1 = Address;
>>>> +    ExitInfo2 = Bytes;
>>>> +    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
>>>> +
>>>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>>>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>>>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>>>> +    if (Status != 0) {
>>>> +      return Status;
>>>> +    }
>>>> +    break;
>>>> +
>>>>    //
>>>>    // MMIO write (MOV reg/memX, immX)
>>>>    //
>>>> @@ -809,6 +855,58 @@ MmioExit (
>>>>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>>>>      break;
>>>>
>>>> +  //
>>>> +  // MMIO read (MOV aX, moffsetX)
>>>> +  //
>>>> +  case 0xA0:
>>>> +    Bytes = 1;
>>>> +    //
>>>> +    // fall through
>>>> +    //
>>>> +  case 0xA1:
>>>> +    Bytes = ((Bytes != 0) ? Bytes :
>>>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>>>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>>>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>>>> +             0);
>>>> +
>>>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>>>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>>>> +
>>>> +    if (InstructionData->AddrSize == Size8Bits) {
>>>> +      Address = *(UINT8 *) InstructionData->Immediate;
>>>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>>>> +      Address = *(UINT16 *) InstructionData->Immediate;
>>>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>>>> +      Address = *(UINT32 *) InstructionData->Immediate;
>>>> +    } else {
>>>> +      Address = *(UINTN *) InstructionData->Immediate;
>>>> +    }
>>>
>>> (2) Similar question as (1).
>>
>> Will do.
>>
>>>
>>>> +
>>>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>>>> +    if (Status != 0) {
>>>> +      return Status;
>>>> +    }
>>>> +
>>>> +    ExitInfo1 = Address;
>>>> +    ExitInfo2 = Bytes;
>>>> +
>>>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>>>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>>>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>>>> +    if (Status != 0) {
>>>> +      return Status;
>>>> +    }
>>>> +
>>>> +    if (Bytes == 4) {
>>>> +      //
>>>> +      // Zero-extend for 32-bit operation
>>>> +      //
>>>> +      Regs->Rax = 0;
>>>> +    }
>>>
>>> (3) This is also seen with opcode 0x8B, but can you remind me please why
>>> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
>>
>> That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
>> 32-bit operand size the 32-bit register results are zero-extended to 64-bits.
>>
>>>
>>>> +    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>>>> +    break;
>>>> +
>>>>    //
>>>>    // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>>>>    //
>>>> @@ -886,6 +984,7 @@ MmioExit (
>>>>      break;
>>>>
>>>>    default:
>>>> +    DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>>>>      Status = GP_EXCEPTION;
>>>>      ASSERT (FALSE);
>>>>    }
>>>>
>>>
>>> (4) We should use the DEBUG_ERROR log mask here.
>>
>> Will change.
>>
>> Thanks,
>> Tom
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>>
>>>
>>> 
>>>
>>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74387): https://edk2.groups.io/g/devel/message/74387
Mute This Topic: https://groups.io/mt/82247966/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes

Posted by Lendacky, Thomas 4 years, 9 months ago

On 4/23/21 4:10 AM, Laszlo Ersek wrote:
> On 04/22/21 17:42, Tom Lendacky wrote:
>> On 4/22/21 9:15 AM, Tom Lendacky wrote:
>>> On 4/22/21 12:50 AM, Laszlo Ersek via groups.io wrote:
>>>> On 04/21/21 00:54, Lendacky, Thomas wrote:
>>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>>
>>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Ca0ff778cdbb146a6186508d90637ba60%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547658699539157%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=vMmmGGXXT8MtM4WOB4gkwtkr0cwwkx98LoOXYg6fclQ%3D&amp;reserved=0
>>>>>
>>>>> Enabling TPM support results in guest termination of an SEV-ES guest
>>>>> because it uses MMIO opcodes that are not currently supported.
>>>>>
>>>>> Add support for the new MMIO opcodes (0xA0 - 0xA3), MOV instructions which
>>>>> use a memory offset directly encoded in the instruction. Also, add a DEBUG
>>>>> statement to identify an unsupported MMIO opcode being used.
>>>>>
>>>>> Fixes: c45f678a1ea2080344e125dc55b14e4b9f98483d
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>> Cc: Min Xu <min.m.xu@intel.com>
>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>> ---
>>>>>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 99 +++++++++++++++++++
>>>>>  1 file changed, 99 insertions(+)
>>>>>
>>>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>>> index 273f36499988..f9660b757d8e 100644
>>>>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>>>>> @@ -678,6 +678,7 @@ MmioExit (
>>>>>    UINTN   Bytes;
>>>>>    UINT64  *Register;
>>>>>    UINT8   OpCode, SignByte;
>>>>> +  UINTN   Address;
>>>>>
>>>>>    Bytes = 0;
>>>>>
>>>>> @@ -727,6 +728,51 @@ MmioExit (
>>>>>      }
>>>>>      break;
>>>>>
>>>>> +  //
>>>>> +  // MMIO write (MOV moffsetX, aX)
>>>>> +  //
>>>>> +  case 0xA2:
>>>>> +    Bytes = 1;
>>>>> +    //
>>>>> +    // fall through
>>>>> +    //
>>>>> +  case 0xA3:
>>>>> +    Bytes = ((Bytes != 0) ? Bytes :
>>>>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>>>>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>>>>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>>>>> +             0);
>>>>> +
>>>>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>>>>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>>>>> +
>>>>> +    if (InstructionData->AddrSize == Size8Bits) {
>>>>> +      Address = *(UINT8 *) InstructionData->Immediate;
>>>>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>>>>> +      Address = *(UINT16 *) InstructionData->Immediate;
>>>>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>>>>> +      Address = *(UINT32 *) InstructionData->Immediate;
>>>>> +    } else {
>>>>> +      Address = *(UINTN *) InstructionData->Immediate;
>>>>> +    }
>>>>
>>>> (1) Can we simplify this as follows?
>>>>
>>>>     InstructionData->ImmediateSize = 1 << InstructionData->AddrSize;
>>>>     InstructionData->End += InstructionData->ImmediateSize;
>>>>     Address = 0;
>>>>     CopyMem (&Address, InstructionData->Immediate,
>>>>       InstructionData->ImmediateSize);
>>>
>>> Yup, that can be done.
>>
>> "Address" is a type UINTN, but since this is X64 only code, an 8-byte copy
>> isn't an issue. Should I add a comment about that above the setting of
>> "Address"? Or should I convert "Address" to a UINT64 - although
>> ValidateMmioMemory expects a UINTN...  Thoughts?
> 
> Yes, I had the exact same thought process :)
> 
> The comment looks good, but how about expressing it as a STATIC_ASSERT,
> with sizeof (UINTN) and sizeof (UINT64) being equal? (Alternatively,
> about MAX_UINT64 being equal to MAX_UINTN.)

I like the STATIC_ASSERT idea, I'll do that.

Thanks,
Tom

> 
> If you find that too verbose, a comment is good enough too, of course.
> 
> Thanks!
> Laszlo
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>>>
>>>>> +
>>>>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>>>>> +    if (Status != 0) {
>>>>> +      return Status;
>>>>> +    }
>>>>> +
>>>>> +    ExitInfo1 = Address;
>>>>> +    ExitInfo2 = Bytes;
>>>>> +    CopyMem (Ghcb->SharedBuffer, &Regs->Rax, Bytes);
>>>>> +
>>>>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>>>>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>>>>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>>>>> +    if (Status != 0) {
>>>>> +      return Status;
>>>>> +    }
>>>>> +    break;
>>>>> +
>>>>>    //
>>>>>    // MMIO write (MOV reg/memX, immX)
>>>>>    //
>>>>> @@ -809,6 +855,58 @@ MmioExit (
>>>>>      CopyMem (Register, Ghcb->SharedBuffer, Bytes);
>>>>>      break;
>>>>>
>>>>> +  //
>>>>> +  // MMIO read (MOV aX, moffsetX)
>>>>> +  //
>>>>> +  case 0xA0:
>>>>> +    Bytes = 1;
>>>>> +    //
>>>>> +    // fall through
>>>>> +    //
>>>>> +  case 0xA1:
>>>>> +    Bytes = ((Bytes != 0) ? Bytes :
>>>>> +             (InstructionData->DataSize == Size16Bits) ? 2 :
>>>>> +             (InstructionData->DataSize == Size32Bits) ? 4 :
>>>>> +             (InstructionData->DataSize == Size64Bits) ? 8 :
>>>>> +             0);
>>>>> +
>>>>> +    InstructionData->ImmediateSize = (UINTN) (1 << InstructionData->AddrSize);
>>>>> +    InstructionData->End += (UINTN) (1 << InstructionData->AddrSize);
>>>>> +
>>>>> +    if (InstructionData->AddrSize == Size8Bits) {
>>>>> +      Address = *(UINT8 *) InstructionData->Immediate;
>>>>> +    } else if (InstructionData->AddrSize == Size16Bits) {
>>>>> +      Address = *(UINT16 *) InstructionData->Immediate;
>>>>> +    } else if (InstructionData->AddrSize == Size32Bits) {
>>>>> +      Address = *(UINT32 *) InstructionData->Immediate;
>>>>> +    } else {
>>>>> +      Address = *(UINTN *) InstructionData->Immediate;
>>>>> +    }
>>>>
>>>> (2) Similar question as (1).
>>>
>>> Will do.
>>>
>>>>
>>>>> +
>>>>> +    Status = ValidateMmioMemory (Ghcb, Address, Bytes);
>>>>> +    if (Status != 0) {
>>>>> +      return Status;
>>>>> +    }
>>>>> +
>>>>> +    ExitInfo1 = Address;
>>>>> +    ExitInfo2 = Bytes;
>>>>> +
>>>>> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
>>>>> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>>>>> +    Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>>>>> +    if (Status != 0) {
>>>>> +      return Status;
>>>>> +    }
>>>>> +
>>>>> +    if (Bytes == 4) {
>>>>> +      //
>>>>> +      // Zero-extend for 32-bit operation
>>>>> +      //
>>>>> +      Regs->Rax = 0;
>>>>> +    }
>>>>
>>>> (3) This is also seen with opcode 0x8B, but can you remind me please why
>>>> we ignore (Bytes == 1) and (Bytes == 2) for zero extension?
>>>
>>> That comes from the APM Vol 3, Table B-1, that says, in 64-bit mode, for a
>>> 32-bit operand size the 32-bit register results are zero-extended to 64-bits.
>>>
>>>>
>>>>> +    CopyMem (&Regs->Rax, Ghcb->SharedBuffer, Bytes);
>>>>> +    break;
>>>>> +
>>>>>    //
>>>>>    // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
>>>>>    //
>>>>> @@ -886,6 +984,7 @@ MmioExit (
>>>>>      break;
>>>>>
>>>>>    default:
>>>>> +    DEBUG ((DEBUG_INFO, "Invalid MMIO opcode (%x)\n", OpCode));
>>>>>      Status = GP_EXCEPTION;
>>>>>      ASSERT (FALSE);
>>>>>    }
>>>>>
>>>>
>>>> (4) We should use the DEBUG_ERROR log mask here.
>>>
>>> Will change.
>>>
>>> Thanks,
>>> Tom
>>>
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>>
>>>>
>>>> 
>>>>
>>>>
>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74395): https://edk2.groups.io/g/devel/message/74395
Mute This Topic: https://groups.io/mt/82247966/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/3] OvfmPkg/VmgExitLib: Properly decode MMIO MOVZX and MOVSX opcodes
[edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes
[edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV