From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.10225.1590142468445857152 for ; Fri, 22 May 2020 03:14:28 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RgaspHR2; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590142467; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NbBd0CmzZffN3XQyKSQ9OEHttV/iyGgvWnNE2vZ2x8Y=; b=RgaspHR2cd2a4oPy0t4YQv3LxZA1GQHBudJ26flkD5Q2KEwmbtuV4k5lqFz2lJls0XilBq IXrpxBLiSVaOap7CFpLQKpFl5SiWIg6QfJ4P2cChD+EqjichBcuaJCNpJDf2I7694nzSni wefBR8+Hm0xUPt6j1bzftd9JEdgFl4o= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-147-s8h2I5sJNdWmy_6bHWprjQ-1; Fri, 22 May 2020 06:14:23 -0400 X-MC-Unique: s8h2I5sJNdWmy_6bHWprjQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AE3E4107ACCA; Fri, 22 May 2020 10:14:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-40.ams2.redhat.com [10.36.112.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id 50C75105912D; Fri, 22 May 2020 10:14:19 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 14/46] OvmfPkg/VmgExitLib: Support string IO for IOIO_PROT NAE events To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh , Ard Biesheuvel References: <96260a004ba86ab5e7ad440d40415fda3814ab3c.1589925074.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <34279e72-09c7-17da-8178-9c6da5ffa9a6@redhat.com> Date: Fri, 22 May 2020 12:14:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <96260a004ba86ab5e7ad440d40415fda3814ab3c.1589925074.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > .../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 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; >