Re: [edk2-devel] [PATCH v8 14/46] OvmfPkg/VmgExitLib: Support string IO for IOIO_PROT NAE events

["Laszlo Ersek" <lersek@redhat.com>]

On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> Add support to the #VC exception handler to handle string IO. This
> requires expanding the IO instruction parsing to recognize string based
> IO instructions as well as preparing an un-encrypted buffer to be used
> to transfer (either to or from the guest) the string contents for the IO
> operation. The SW_EXITINFO2 and SW_SCRATCH fields of the GHCB are set
> appropriately for the operation. Multiple VMGEXIT invocations may be
> needed to complete the string IO operation.
> 
> 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 | 86 ++++++++++++++++---
>  1 file changed, 72 insertions(+), 14 deletions(-)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> index b4578ae922c1..906b32e93d53 100644
> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> @@ -453,6 +453,22 @@ IoioExitInfo (
>    ExitInfo = 0;
>  
>    switch (*(InstructionData->OpCodes)) {
> +  // INS opcodes
> +  case 0x6C:
> +  case 0x6D:
> +    ExitInfo |= IOIO_TYPE_INS;
> +    ExitInfo |= IOIO_SEG_ES;
> +    ExitInfo |= ((Regs->Rdx & 0xffff) << 16);
> +    break;
> +
> +  // OUTS opcodes
> +  case 0x6E:
> +  case 0x6F:
> +    ExitInfo |= IOIO_TYPE_OUTS;
> +    ExitInfo |= IOIO_SEG_DS;
> +    ExitInfo |= ((Regs->Rdx & 0xffff) << 16);
> +    break;
> +
>    // IN immediate opcodes
>    case 0xE4:
>    case 0xE5:

(1) Same request about comment style as before.

> @@ -490,6 +506,8 @@ IoioExitInfo (
>    }
>  
>    switch (*(InstructionData->OpCodes)) {
> +  case 0x6C:
> +  case 0x6E:
>    case 0xE4:
>    case 0xE6:
>    case 0xEC:
> @@ -550,30 +568,70 @@ IoioExit (
>    IN     SEV_ES_INSTRUCTION_DATA  *InstructionData
>    )
>  {
> -  UINT64  ExitInfo1, Status;
> +  UINT64   ExitInfo1, ExitInfo2, Status;
> +  BOOLEAN  String;

(2) Please call this "IsString" or "IsStringOp" or something that
suggests it's a boolean, not an actual string.

>  
>    ExitInfo1 = IoioExitInfo (Regs, InstructionData);
>    if (!ExitInfo1) {
>      return UnsupportedExit (Ghcb, Regs, InstructionData);
>    }
>  
> -  if (ExitInfo1 & IOIO_TYPE_IN) {
> -    Ghcb->SaveArea.Rax = 0;
> +  String = (ExitInfo1 & IOIO_TYPE_STR) ? TRUE : FALSE;

(3) Please use the more idiomatic ((ExitInfo1 & IOIO_TYPE_STR) != 0) syntax.

> +  if (String) {
> +    UINTN  IoBytes, VmgExitBytes;
> +    UINTN  GhcbCount, OpCount;
> +
> +    Status = 0;
> +
> +    IoBytes = (ExitInfo1 >> 4) & 0x7;
> +    GhcbCount = sizeof (Ghcb->SharedBuffer) / IoBytes;
> +
> +    OpCount = (ExitInfo1 & IOIO_REP) ? Regs->Rcx : 1;

(4) Please write

  ((ExitInfo1 & IOIO_REP) != 0) ? Regs->Rcx : 1

> +    while (OpCount) {
> +      ExitInfo2 = MIN (OpCount, GhcbCount);
> +      VmgExitBytes = ExitInfo2 * IoBytes;
> +
> +      if (!(ExitInfo1 & IOIO_TYPE_IN)) {

(5) Please write

  if ((ExitInfo1 & IOIO_TYPE_IN) == 0) {

> +        CopyMem (Ghcb->SharedBuffer, (VOID *) Regs->Rsi, VmgExitBytes);
> +        Regs->Rsi += VmgExitBytes;
> +      }
> +
> +      Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +      Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, ExitInfo2);
> +      if (Status) {

(6) Status is not a BOOLEAN but a UINT64. Please check (Status != 0) or
(Status > 0). Please revise the rest of the patches for this.

> +        return Status;
> +      }
> +
> +      if (ExitInfo1 & IOIO_TYPE_IN) {

(7) Similar to (4) and (5)

There are some more instances of this in the patch.

With the style nits fixed

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +        CopyMem ((VOID *) Regs->Rdi, Ghcb->SharedBuffer, VmgExitBytes);
> +        Regs->Rdi += VmgExitBytes;
> +      }
> +
> +      if (ExitInfo1 & IOIO_REP) {
> +        Regs->Rcx -= ExitInfo2;
> +      }
> +
> +      OpCount -= ExitInfo2;
> +    }
>    } else {
> -    CopyMem (&Ghcb->SaveArea.Rax, &Regs->Rax, IOIO_DATA_BYTES (ExitInfo1));
> -  }
> -  GhcbSetRegValid (Ghcb, GhcbRax);
> +    if (ExitInfo1 & IOIO_TYPE_IN) {
> +      Ghcb->SaveArea.Rax = 0;
> +    } else {
> +      CopyMem (&Ghcb->SaveArea.Rax, &Regs->Rax, IOIO_DATA_BYTES (ExitInfo1));
> +    }
> +    GhcbSetRegValid (Ghcb, GhcbRax);
>  
> -  Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, 0);
> -  if (Status) {
> -    return Status;
> -  }
> +    Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, 0);
> +    if (Status) {
> +      return Status;
> +    }
>  
> -  if (ExitInfo1 & IOIO_TYPE_IN) {
> -    if (!GhcbIsRegValid (Ghcb, GhcbRax)) {
> -      return UnsupportedExit (Ghcb, Regs, InstructionData);
> +    if (ExitInfo1 & IOIO_TYPE_IN) {
> +      if (!GhcbIsRegValid (Ghcb, GhcbRax)) {
> +        return UnsupportedExit (Ghcb, Regs, InstructionData);
> +      }
> +      CopyMem (&Regs->Rax, &Ghcb->SaveArea.Rax, IOIO_DATA_BYTES (ExitInfo1));
>      }
> -    CopyMem (&Regs->Rax, &Ghcb->SaveArea.Rax, IOIO_DATA_BYTES (ExitInfo1));
>    }
>  
>    return 0;
>