From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web09.8346.1619070619083566823 for ; Wed, 21 Apr 2021 22:50:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YEHRQbDK; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1619070617; 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=rUn3Ul6Obo3PxQdKjzIxDLt+aqwrHE/rjImKQl6PAcw=; b=YEHRQbDKobOX0ujeFzZzQoOO2uWVCbycqdFbCpN890j7iXmF+2EsZOsrN/Z4Koal7mNj81 P2SBxY16XG6M2FTapalTC9FOglI8RbWHYpVO0GIJ/tbxCe9Cjo5sNP4MUHo1mSeuiGAMha kI/WGlPflMeRO8va7Kw3ctGJgOgFMno= 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-420-WYcXEhL8NO6kQ5xR0mw2_w-1; Thu, 22 Apr 2021 01:50:15 -0400 X-MC-Unique: WYcXEhL8NO6kQ5xR0mw2_w-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C5B1B10054F6; Thu, 22 Apr 2021 05:50:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-151.ams2.redhat.com [10.36.112.151]) by smtp.corp.redhat.com (Postfix) with ESMTP id 860955C1D5; Thu, 22 Apr 2021 05:50:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] OvmfPkg/VmgExitLib: Add support for new MMIO MOV opcodes To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Joerg Roedel , Borislav Petkov , Ard Biesheuvel , Jordan Justen , Brijesh Singh , James Bottomley , Jiewen Yao , Min Xu References: <79ed645c089ffab10716cdb8813f191f6e0afcfb.1618959281.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 22 Apr 2021 07:50:10 +0200 MIME-Version: 1.0 In-Reply-To: <79ed645c089ffab10716cdb8813f191f6e0afcfb.1618959281.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/21/21 00:54, Lendacky, Thomas wrote: > From: Tom Lendacky > > 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 > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Brijesh Singh > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Min Xu > Signed-off-by: Tom Lendacky > --- > 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