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 5427F21DF806B for ; Mon, 28 Aug 2017 01:08:21 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8F767C0587FE; Mon, 28 Aug 2017 08:10:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8F767C0587FE Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-67.phx2.redhat.com [10.3.116.67]) by smtp.corp.redhat.com (Postfix) with ESMTP id 263C57A421; Mon, 28 Aug 2017 08:10:57 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Ard Biesheuvel , Jordan Justen , Tom Lendacky References: <1503880469-15999-1-git-send-email-brijesh.singh@amd.com> <1503880469-15999-3-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <8a87ced5-45f4-3d06-06a8-a4b579be16d0@redhat.com> Date: Mon, 28 Aug 2017 10:10:56 +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: <1503880469-15999-3-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 28 Aug 2017 08:11:00 +0000 (UTC) Subject: Re: [PATCH v2 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers 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, 28 Aug 2017 08:08:21 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/28/17 02:34, Brijesh Singh wrote: > When device is behind the IOMMU, driver is require to pass the device > address of virtio request, response and any memory referenced by those > request/response to the bus master. > > The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to > map request and response buffers system physical address to the device > address. > > - If the buffer need to be accessed by both the processor and a bus > master then map with BusMasterCommonBuffer. > > - If the buffer need to be accessed for a write operation by a bus master > then map with BusMasterWrite. > > - If the buffer need to be accessed for a read operation by a bus master > then map with BusMasterRead. > > 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.c | 138 ++++++++++++++++++-- > 1 file changed, 125 insertions(+), 13 deletions(-) Reviewed-by: Laszlo Ersek Thanks! Laszlo > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > index 663ba281ab73..c9c42aa41243 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > @@ -232,7 +232,8 @@ VerifyReadWriteRequest ( > > @retval EFI_DEVICE_ERROR Failed to notify host side via VirtIo write, or > unable to parse host response, or host response > - is not VIRTIO_BLK_S_OK. > + is not VIRTIO_BLK_S_OK or failed to map Buffer > + for a bus master operation. > > **/ > > @@ -249,8 +250,16 @@ SynchronousRequest ( > { > UINT32 BlockSize; > volatile VIRTIO_BLK_REQ Request; > - volatile UINT8 HostStatus; > + volatile UINT8 *HostStatus; > + VOID *HostStatusBuffer; > DESC_INDICES Indices; > + VOID *RequestMapping; > + VOID *StatusMapping; > + VOID *BufferMapping; > + EFI_PHYSICAL_ADDRESS BufferDeviceAddress; > + EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress; > + EFI_PHYSICAL_ADDRESS RequestDeviceAddress; > + EFI_STATUS Status; > > BlockSize = Dev->BlockIoMedia.BlockSize; > > @@ -275,12 +284,82 @@ SynchronousRequest ( > Request.IoPrio = 0; > Request.Sector = MultU64x32(Lba, BlockSize / 512); > > - VirtioPrepare (&Dev->Ring, &Indices); > + // > + // Host status is bi-directional (we preset with a value and expect the > + // device to update it). Allocate a host status buffer which can be mapped > + // to access equally by both processor and the device. > + // > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *HostStatus), > + &HostStatusBuffer > + ); > + if (EFI_ERROR (Status)) { > + return EFI_DEVICE_ERROR; > + } > + > + HostStatus = HostStatusBuffer; > + > + // > + // Map virtio-blk request header (must be done after request header is > + // populated) > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + (VOID *) &Request, > + sizeof Request, > + &RequestDeviceAddress, > + &RequestMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto FreeHostStatusBuffer; > + } > + > + // > + // Map data buffer > + // > + if (BufferSize > 0) { > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + (RequestIsWrite ? > + VirtioOperationBusMasterRead : > + VirtioOperationBusMasterWrite), > + (VOID *) Buffer, > + BufferSize, > + &BufferDeviceAddress, > + &BufferMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto UnmapRequestBuffer; > + } > + } > > // > // preset a host status for ourselves that we do not accept as success > // > - HostStatus = VIRTIO_BLK_S_IOERR; > + *HostStatus = VIRTIO_BLK_S_IOERR; > + > + // > + // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that > + // both processor and device can access it. > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + HostStatusBuffer, > + sizeof *HostStatus, > + &HostStatusDeviceAddress, > + &StatusMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto UnmapDataBuffer; > + } > + > + VirtioPrepare (&Dev->Ring, &Indices); > > // > // ensured by VirtioBlkInit() -- this predicate, in combination with the > @@ -291,8 +370,13 @@ SynchronousRequest ( > // > // virtio-blk header in first desc > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, > - VRING_DESC_F_NEXT, &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + RequestDeviceAddress, > + sizeof Request, > + VRING_DESC_F_NEXT, > + &Indices > + ); > > // > // data buffer for read/write in second desc > @@ -311,27 +395,55 @@ SynchronousRequest ( > // > // VRING_DESC_F_WRITE is interpreted from the host's point of view. > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, > + VirtioAppendDesc ( > + &Dev->Ring, > + BufferDeviceAddress, > + (UINT32) BufferSize, > VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), > - &Indices); > + &Indices > + ); > } > > // > // host status in last (second or third) desc > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, > - VRING_DESC_F_WRITE, &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + HostStatusDeviceAddress, > + sizeof *HostStatus, > + VRING_DESC_F_WRITE, > + &Indices > + ); > > // > // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D). > // > if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, > NULL) == EFI_SUCCESS && > - HostStatus == VIRTIO_BLK_S_OK) { > - return EFI_SUCCESS; > + *HostStatus == VIRTIO_BLK_S_OK) { > + Status = EFI_SUCCESS; > + } else { > + Status = EFI_DEVICE_ERROR; > } > > - return EFI_DEVICE_ERROR; > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping); > + > +UnmapDataBuffer: > + if (BufferSize > 0) { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); > + } > + > +UnmapRequestBuffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); > + > +FreeHostStatusBuffer: > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *HostStatus), > + HostStatusBuffer > + ); > + > + return Status; > } > > >