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.jus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lenda...@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 <ler...@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;
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60109): https://edk2.groups.io/g/devel/message/60109
Mute This Topic: https://groups.io/mt/74336568/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
