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 57D6821D2E62D for ; Mon, 21 Aug 2017 08:22:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 07AB77E436; Mon, 21 Aug 2017 15:24:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 07AB77E436 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-165.phx2.redhat.com [10.3.116.165]) by smtp.corp.redhat.com (Postfix) with ESMTP id 597FC704CE; Mon, 21 Aug 2017 15:24:30 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502710605-8058-1-git-send-email-brijesh.singh@amd.com> <1502710605-8058-18-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <8392c590-62db-fea9-5cca-aa0e76183c20@redhat.com> Date: Mon, 21 Aug 2017 17:24:29 +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: <1502710605-8058-18-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 21 Aug 2017 15:24:32 +0000 (UTC) Subject: Re: [PATCH v2 17/23] OvmfPkg/VirtioBlkDxe: 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: Mon, 21 Aug 2017 15:22:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/14/17 13:36, Brijesh Singh wrote: > The SynchronousRequest(), programs the vring descriptor with the buffers (1) you likely meant "The SynchronousRequest() function" > pointed-by virtio-blk requests, status and memory that is referenced > inside the request header. > > The patch uses VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() function to map > host address to device address and programs the vring descriptor with > device addresses. > > 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/VirtioBlkDxe/VirtioBlk.h | 1 + > OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 121 +++++++++++++++++--- > 2 files changed, 109 insertions(+), 13 deletions(-) > > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h > index 6c402ca88ea4..612994d261bc 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h > @@ -41,6 +41,7 @@ typedef struct { > VRING Ring; // VirtioRingInit 2 > EFI_BLOCK_IO_PROTOCOL BlockIo; // VirtioBlkInit 1 > EFI_BLOCK_IO_MEDIA BlockIoMedia; // VirtioBlkInit 1 > + VOID *RingMapping; // VirtioBlkInit 1 > } VBLK_DEV; (2) If possible, please use consistent field names between the drivers. So this should be "RingMap" as well (to follow the pattern set in the virtio-rng driver). (3) My earlier comments apply -- depth should be 2, and "VirtioBlkInit" should be replaced with "VirtioRingMap". > > #define VIRTIO_BLK_FROM_BLOCK_IO(BlockIoPointer) \ > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > index bff15fe3add1..57baceb20a19 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > @@ -251,6 +251,11 @@ SynchronousRequest ( > volatile VIRTIO_BLK_REQ Request; > volatile UINT8 HostStatus; > DESC_INDICES Indices; > + VOID *RequestMapping; > + VOID *StatusMapping; > + VOID *BufferMapping; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + EFI_STATUS Status; > > BlockSize = Dev->BlockIoMedia.BlockSize; > > @@ -289,14 +294,30 @@ SynchronousRequest ( > ASSERT (Dev->Ring.QueueSize >= 3); > > // > + // Map virtio-blk request header > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + (VOID *) &Request, > + sizeof Request, > + &DeviceAddress, > + &RequestMapping > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } (4) You should return EFI_DEVICE_ERROR here, regardless of the Status received from VirtioMapAllBytesInSharedBuffer(). Please consult the leading comment block on return values. (5) In fact, please extend the documentation for the EFI_DEVICE_ERROR retval -- it now includes any case when a mapping for a bus master operation fails. > + > + // > // virtio-blk header in first desc > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, > + VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, sizeof Request, > VRING_DESC_F_NEXT, &Indices); (6) Please break every argument to a separate line. (7) As mentioned earlier, please remove the (UINTN) cast from "DeviceAddress". (8) Please do not mix the mapping operations with the virtio ring manipulation: (8a) Rather than reusing DeviceAddress, please introduce three local address variables. (8b) Concentrate the mapping operations, for all three areas, to one sequence, before touching the virtio ring. For this, you might want to move the VirtioPrepare() call as well just before the first VirtioAppendDesc() call. (8c) If possible, the virtio ring manipulation code should *only* be updated with the device addresses; control flow should not be changed. Basically, the VirtioPrepare() -> VirtioAppendDesc() -> VirtioFlush() sequence should never be interrupted (due to error conditions or anything else). All preparations should be done earlier and all errors should be caught earlier. This will require quite a bit of reorganization for this patch, so my remaining comments, for this function anyway, will be somewhat superficial: > > // > // data buffer for read/write in second desc > // > + BufferMapping = NULL; (9) This is not useful. As discussed earlier, a NULL mapping can be valid, dependent on VIRTIO_DEVICE_PROTOCOL implementation, so it shouldn't be used for control flow. Instead, please use the (BufferSize > 0) condition to tell a flush request apart from read/write requests. (See the leading comment block on the SynchronousRequest() function.) > if (BufferSize > 0) { > // > // From virtio-0.9.5, 2.3.2 Descriptor Table: > @@ -309,29 +330,86 @@ SynchronousRequest ( > ASSERT (BufferSize <= SIZE_1GB); > > // > + // Map data buffer > + // > + if (RequestIsWrite) { > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + (VOID *) Buffer, > + BufferSize, > + &DeviceAddress, > + &BufferMapping > + ); > + } else { > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterWrite, > + (VOID *) Buffer, > + BufferSize, > + &DeviceAddress, > + &BufferMapping > + ); > + } > + > + if (EFI_ERROR (Status)) { > + goto Unmap_Request_Buffer; > + } > + > + // > // VRING_DESC_F_WRITE is interpreted from the host's point of view. > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, > + VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, (UINT32) BufferSize, > VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), > &Indices); > } (10) Please break every argument to a separate line. (11) Please remove the (UINTN) cast from the device address. > > // > + // Map virtio-blk status header > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterWrite, > + (VOID *) &HostStatus, > + sizeof HostStatus, > + &DeviceAddress, > + &StatusMapping > + ); > + if (EFI_ERROR (Status)) { > + goto Unmap_Data_Buffer; > + } > + > + // > // host status in last (second or third) desc > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, > + VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, sizeof HostStatus, > VRING_DESC_F_WRITE, &Indices); (12) Please break every argument to a separate line. (13) Please remove the (UINTN) cast from the device address. > > // > // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D). > // > - if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, > - NULL) == EFI_SUCCESS && > + if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL) != EFI_SUCCESS) { > + Status = EFI_DEVICE_ERROR; > + } > + (14) The original code isn't very idiomatic here, please use the EFI_ERROR() macro instead. (This was the first virtio driver I wrote.) > + // > + // Unmap the HostStatus buffer before accessing it > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping); (15) Please check the return status. (16) This is where all three (or all two) mappings should be undone (with return statuses checked), in reverse order of mapping. If any one of them fails, you can jump to the corresponding error handling label (which will continue to unmap stuff, but without checking error statuses). > + > + if (Status != EFI_DEVICE_ERROR && > HostStatus == VIRTIO_BLK_S_OK) { > - return EFI_SUCCESS; > + Status = EFI_SUCCESS; > + } else { > + Status = EFI_DEVICE_ERROR; > } > > - return EFI_DEVICE_ERROR; > +Unmap_Data_Buffer: (17) Please don't use underscores whenever the coding style requires CamelCase -- the label should be "UnmapDataBuffer". > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); (18) incorrect indentation > +Unmap_Request_Buffer: (19) should be "UnmapRequestBuffer". > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); > + > + return Status; > } > > > @@ -601,6 +679,7 @@ VirtioBlkInit ( > UINT8 AlignmentOffset; > UINT32 OptIoSize; > UINT16 QueueSize; > + UINT64 RingBaseShift; > > PhysicalBlockExp = 0; > AlignmentOffset = 0; > @@ -728,26 +807,33 @@ VirtioBlkInit ( > goto Failed; > } > (20) Please add a comment here: // // If anything fails from here on, we must release the ring resources. // > + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift, > + &Dev->RingMapping); (21) Please break each argument to a separate line. > + 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. > // (22) Please replace the word "release" with "unmap", in the above comment. > 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); (23) Please break each argument to a separate line. > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > > @@ -758,7 +844,7 @@ VirtioBlkInit ( > Features &= ~(UINT64)VIRTIO_F_VERSION_1; > Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > } > > @@ -768,7 +854,7 @@ VirtioBlkInit ( > NextDevStat |= VSTAT_DRIVER_OK; > Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -811,7 +897,10 @@ VirtioBlkInit ( > } > return EFI_SUCCESS; > > +UnmapQueue: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMapping); (24) incorrect indentation > ReleaseQueue: > + (25) I think you intended to add the empty line above the "ReleaseQueue" label, not below it. > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > Failed: (26) You forgot to call UnmapSharedBuffer() in the VirtioBlkUninit() function. Each ring must be unmapped in three places: - in the device init function, if there is an error and we're past mapping the ring, - in the device uninit function, - in the ExitBootServices notify function. > @@ -885,6 +974,12 @@ VirtioBlkExitBoot ( > // > Dev = Context; > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + // > + // Unmap the ring buffer so that hypervisor can not get a readable data > + // after device is reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMapping); > } > > /** > I think I'm skipping the rest of the series for now (except the last patch, I have comments for that). Please rework the rest of the transport-independent virtio drivers (scsi and net) based on my comments for this (blk) and the previous (rng) patch. Thanks! Laszlo