From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C7BD421A1348B for ; Thu, 11 May 2017 10:44:24 -0700 (PDT) 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 mx1.redhat.com (Postfix) with ESMTPS id B89CDC04B94E; Thu, 11 May 2017 17:44:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B89CDC04B94E Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B89CDC04B94E Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-15.phx2.redhat.com [10.3.116.15]) by smtp.corp.redhat.com (Postfix) with ESMTP id 14DC871C40; Thu, 11 May 2017 17:44:21 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org References: <1494454162-9940-1-git-send-email-brijesh.singh@amd.com> <1494454162-9940-14-git-send-email-brijesh.singh@amd.com> Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, Jordan Justen From: Laszlo Ersek Message-ID: Date: Thu, 11 May 2017 19:44:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1494454162-9940-14-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 11 May 2017 17:44:24 +0000 (UTC) Subject: Re: [RFC v4 13/13] OvmfPkg/QemuFwCfgLib: Add SEV support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 May 2017 17:44:25 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit comments below: On 05/11/17 00:09, Brijesh Singh wrote: > When SEV is enabled, use a bounce buffer to perform the DMA operation. > > > Cc: Jordan Justen > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 54 +++++++++++++++++++- > 1 file changed, 52 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > index 73a19772bee1..86d8bf880e71 100644 > --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > @@ -72,6 +72,8 @@ InternalQemuFwCfgDmaBytes ( > volatile FW_CFG_DMA_ACCESS *Access; > UINT32 AccessHigh, AccessLow; > UINT32 Status; > + UINT32 NumPages; > + VOID *DmaBuffer, *BounceBuffer; > > ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || > Control == FW_CFG_DMA_CTL_SKIP); > @@ -80,11 +82,44 @@ InternalQemuFwCfgDmaBytes ( > return; > } > > - Access = &LocalAccess; > + // > + // When SEV is enabled then allocate DMA bounce buffer > + // > + if (InternalQemuFwCfgSevIsEnabled ()) { > + UINT32 TotalSize; (1) Please make TotalSize a UINTN. > + > + TotalSize = sizeof (*Access); > + // > + // Control operation does not need buffer (2) The comment should say "skip operation". > + // > + if (Control != FW_CFG_DMA_CTL_SKIP) { > + TotalSize += Size; > + } > + > + // > + // Allocate SEV DMA bounce buffer > + // > + NumPages = EFI_SIZE_TO_PAGES (TotalSize); (3) Please write NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize) otherwise Visual Studio will likely yell at us. > + InternalQemuFwCfgSevDmaAllocateBuffer (NumPages, &BounceBuffer); > + > + Access = BounceBuffer; > + DmaBuffer = BounceBuffer + sizeof (*Access); (4) Please cast BounceBuffer to (UINT8*) before the addition; we shouldn't do arithmetic on (VOID*). > + > + // > + // Copy data from Host buffer into DMA buffer > + // > + if (Buffer && Control == FW_CFG_DMA_CTL_WRITE) { (5) The Control check suffices. If FW_CFG_DMA_CTL_WRITE is passed in, then Buffer can only be NULL if Size is also 0, and a zero size is handled transparently by CopyMem(). > + CopyMem (DmaBuffer, Buffer, Size); (Side remark: it's funny how this innocent-looking CopyMem() actually implements decryption :)) > + } > + } else { > + Access = &LocalAccess; > + DmaBuffer = Buffer; > + BounceBuffer = NULL; > + } > > Access->Control = SwapBytes32 (Control); > Access->Length = SwapBytes32 (Size); > - Access->Address = SwapBytes64 ((UINTN)Buffer); > + Access->Address = SwapBytes64 ((UINTN)DmaBuffer); > > // > // Delimit the transfer from (a) modifications to Access, (b) in case of a > @@ -117,6 +152,21 @@ InternalQemuFwCfgDmaBytes ( > // After a read, the caller will want to use Buffer. > // > MemoryFence (); > + > + // > + // If Bounce buffer was allocated then copy the data into host buffer and > + // free the bounce buffer > + // > + if (BounceBuffer) { (6) The edk2 coding style wants us to write this as if (BounceBuffer != NULL) { > + // > + // Copy data from DMA buffer into host buffer > + // > + if (Buffer && Control == FW_CFG_DMA_CTL_READ) { (7) Again, checking only (Control == FW_CFG_DMA_CTL_READ) suffices. > + CopyMem (Buffer, DmaBuffer, Size); (Side note: funny how this innocent-looking CopyMem() implements encryption :)) > + } > + > + InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages); > + } > } > > > (8) In several comments above, you wrote "host buffer". Shouldn't those say "guest buffer"? I agree it is somewhat confusing, because in DMA parlance, "host buffer" is likely the right term. Unfortunately, in virtualization, the "device" that performs the DMA is actually the virtualization host, so "host buffer" ends up meaning the exact opposite of what we want. Can you replace the expression "host buffer" with "encrypted guest buffer" everywhere? Accordingly, can you replace the word "copy" with "encrypt" vs. "decrypt" everywhere, as appropriate? For example, we should end up with something like: // // Copy data from Host buffer into DMA buffer // --> // // Decrypt data from encrypted guest buffer into DMA buffer // Otherwise, the logic of the patch looks good to me. Thanks! Laszlo