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 1CE962095DFF5 for ; Wed, 23 Aug 2017 15:52:28 -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 D8F5DC049E31; Wed, 23 Aug 2017 22:55:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D8F5DC049E31 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=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-43.phx2.redhat.com [10.3.116.43]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7480A5C550; Wed, 23 Aug 2017 22:55:00 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1503490967-5559-1-git-send-email-brijesh.singh@amd.com> <1503490967-5559-13-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Thu, 24 Aug 2017 00:54:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1503490967-5559-13-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]); Wed, 23 Aug 2017 22:55:02 +0000 (UTC) Subject: Re: [PATCH v3 12/23] OvmfPkg/VirtioRngDxe: map host address to device address 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: Wed, 23 Aug 2017 22:52:28 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/23/17 14:22, Brijesh Singh wrote: > patch maps the host address to a device address for buffers (including > rings, device specifc request and response pointed by vring descriptor, > and any further memory reference by those request and response). > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Tom Lendacky > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 + > OvmfPkg/VirtioRngDxe/VirtioRng.c | 82 +++++++++++++++++--- > 2 files changed, 74 insertions(+), 9 deletions(-) > > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h > index 998f9fae48c2..389c8ddc8d31 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h > @@ -38,6 +38,7 @@ typedef struct { > EFI_EVENT ExitBoot; // DriverBindingStart 0 > VRING Ring; // VirtioRingInit 2 > EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1 > + VOID *RingMap; // VirtioRingMap 2 > } VIRTIO_RNG_DEV; > > #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c > index 0abca488e6cd..59f32d343179 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c > @@ -140,6 +140,8 @@ VirtioRngGetRNG ( > UINT32 Len; > UINT32 BufferSize; > EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *Mapping; > > if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { > return EFI_INVALID_PARAMETER; > @@ -159,6 +161,20 @@ VirtioRngGetRNG ( > } > > Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > + // > + // Map Buffer's system phyiscal address to device address > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterWrite, > + (VOID *)Buffer, > + RNGValueLength, > + &DeviceAddress, > + &Mapping > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBuffer; > + } (1) Sorry, I missed this in my previous review: According to the interface contract of this protocol member function, we should set Status to EFI_DEVICE_ERROR here, before jumping to the FreeBuffer label. (Both old code, and new code other than this, do it.) I can fix this up. With that: Reviewed-by: Laszlo Ersek Thanks, Laszlo > > // > // The Virtio RNG device may return less data than we asked it to, and can > @@ -170,7 +186,7 @@ VirtioRngGetRNG ( > > VirtioPrepare (&Dev->Ring, &Indices); > VirtioAppendDesc (&Dev->Ring, > - (UINTN)Buffer + Index, > + DeviceAddress + Index, > BufferSize, > VRING_DESC_F_WRITE, > &Indices); > @@ -178,17 +194,35 @@ VirtioRngGetRNG ( > if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != > EFI_SUCCESS) { > Status = EFI_DEVICE_ERROR; > - goto FreeBuffer; > + goto UnmapBuffer; > } > ASSERT (Len > 0); > ASSERT (Len <= BufferSize); > } > > + // > + // Unmap the device buffer before accessing it. > + // > + Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto FreeBuffer; > + } > + > for (Index = 0; Index < RNGValueLength; Index++) { > RNGValue[Index] = Buffer[Index]; > } > Status = EFI_SUCCESS; > > +UnmapBuffer: > + // > + // If we are reached here due to the error then unmap the buffer otherwise > + // the buffer is already unmapped after VirtioFlush(). > + // > + if (EFI_ERROR (Status)) { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + } > + > FreeBuffer: > FreePool ((VOID *)Buffer); > return Status; > @@ -205,6 +239,7 @@ VirtioRngInit ( > EFI_STATUS Status; > UINT16 QueueSize; > UINT64 Features; > + UINT64 RingBaseShift; > > // > // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence. > @@ -282,25 +317,42 @@ VirtioRngInit ( > } > > // > + // If anything fails from here on, we must release the ring resources. > + // > + Status = VirtioRingMap ( > + Dev->VirtIo, > + &Dev->Ring, > + &RingBaseShift, > + &Dev->RingMap > + ); > + if (EFI_ERROR (Status)) { > + goto ReleaseQueue; > + } > + > + // > // Additional steps for MMIO: align the queue appropriately, and set the > - // size. If anything fails from here on, we must release the ring resources. > + // size. If anything fails from here on, we must unmap the ring resources. > // > Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > // step 4c -- Report GPFN (guest-physical frame number) of queue. > // > - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); > + Status = Dev->VirtIo->SetQueueAddress ( > + Dev->VirtIo, > + &Dev->Ring, > + RingBaseShift > + ); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -310,7 +362,7 @@ VirtioRngInit ( > Features &= ~(UINT64)VIRTIO_F_VERSION_1; > Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > } > > @@ -320,7 +372,7 @@ VirtioRngInit ( > NextDevStat |= VSTAT_DRIVER_OK; > Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -331,6 +383,9 @@ VirtioRngInit ( > > return EFI_SUCCESS; > > +UnmapQueue: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > + > ReleaseQueue: > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > @@ -359,6 +414,9 @@ VirtioRngUninit ( > // the old comms area. > // > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > + > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > } > > @@ -385,6 +443,12 @@ VirtioRngExitBoot ( > // > Dev = Context; > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + // > + // Unmap the ring buffer so that hypervisor will not be able to get readable > + // data after device reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > } > > >