On 08/27/17 22:40, Laszlo Ersek wrote: > This patch is functionally correct. I'd like to request three stylistic > improvements: > > On 08/25/17 23:43, 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 <ard.biesheu...@linaro.org> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Tom Lendacky <thomas.lenda...@amd.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com> >> --- >> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 157 ++++++++++++++++++-- >> 1 file changed, 143 insertions(+), 14 deletions(-) >> >> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> index 663ba281ab73..ab69cb08a625 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, Ret; > > (1) Please rename the "Ret" variable to "UnmapStatus", and define it > separately. > >> >> BlockSize = Dev->BlockIoMedia.BlockSize; >> >> @@ -278,9 +287,89 @@ SynchronousRequest ( >> 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; >> + } >> + >> + // >> + // 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) { >> + if (RequestIsWrite) { >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterRead, >> + (VOID *) Buffer, >> + BufferSize, >> + &BufferDeviceAddress, >> + &BufferMapping >> + ); >> + } else { >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterWrite, >> + (VOID *) Buffer, >> + BufferSize, >> + &BufferDeviceAddress, >> + &BufferMapping >> + ); >> + } > > (2) Please squash these two branches into one, and determine the > Operation argument like this: > > (RequestIsWrite ? > VirtioOperationBusMasterRead : > VirtioOperationBusMasterWrite), > > (The conditional operator (? :) should be used sparingly, but when it > improves readability, it should be used.) > >> + >> + if (EFI_ERROR (Status)) { >> + Status = EFI_DEVICE_ERROR; >> + goto UnmapRequestBuffer; >> + } >> + } >> + >> + // >> + // 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; >> + } >> + >> + HostStatus = HostStatusBuffer; >> + >> + // >> // preset a host status for ourselves that we do not accept as success >> // >> - HostStatus = VIRTIO_BLK_S_IOERR; >> + *HostStatus = VIRTIO_BLK_S_IOERR;
(4) I think we should be careful with BusMasterCommonBuffer operations similarly to BusMasterWrite operations -- populate first, map second. This is to avoid exposing any stale data, even for a short window, to the device. Speaking in SEV terms, IoMmuMap() will decrypt NumberOfBytes in-place -- which is by-design, but then it should decrypt what we just put there, not whatever used to be there (until we overwrite it). IOW, please move the mapping just under the *HostStatus assignment. ... I've now checked all the VirtioOperationBusMasterCommonBuffer mappings in the tree, and the rest is fine -- in fact there is only one (outside of this patch) at the moment: in VirtioRingMap(). And that is fine, because VirtioRingInit() zeroes out the entire ring, after allocating it. Probably a good idea to attend to this in the other drivers (wherever they use BusMasterCommonBuffer). Thanks! Laszlo >> >> // >> // ensured by VirtioBlkInit() -- this predicate, in combination with the >> @@ -291,8 +380,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 +405,62 @@ 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; >> + Status = VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL); >> + >> + // >> + // Unmap the HostStatus buffer before accessing it >> + // >> + Ret = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping); > > (3) Okay, so here you will assign UnmapStatus. And then, the rest: > >> + if (EFI_ERROR (Ret)) { >> + Status = EFI_DEVICE_ERROR; >> + } >> + >> + if (!EFI_ERROR (Status) && >> + *HostStatus == VIRTIO_BLK_S_OK) { >> + Status = EFI_SUCCESS; >> + } else { >> + Status = EFI_DEVICE_ERROR; >> } > > should be written like this: > > if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) || > *HostStatus != VIRTIO_BLK_S_OK) { > Status = EFI_DEVICE_ERROR; > } > > Namely, > > - this block will ensure that we only look at *HostStatus when we're > allowed to (i.e., after both VirtioFlush() and Unmap succeeded), > > - If VirtioFlush() fails and sets Status to some error code, then we > coerce Status to EFI_DEVICE_ERROR, > > - If the entire condition evaluates to FALSE, then Status is already > EFI_SUCCESS, so no need to touch it. > > > Regarding the VirtioScsiDxe driver... in this patch, we get away with > the above shortcut (without making a mess of the code), but in the > VirtioScsiDxe driver, we won't. In that driver, the bus master device > can produce *two* output buffers, so you will have to unmap at least one > of those under an error-handling label -- when you mapped the first > successfully, and failed to map the second. > > (In this patch you managed to unmap StatusMapping in shared code, but > that only worked because you had only one output buffer, and you could > put its mapping last.) > > And once you unmap an output buffer on both the success path and the > failure path, things get more complex. I think that's OK, we should just > be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I > laid out in > <f7736294-0368-cddc-3fcd-de76cae5650e@redhat.com">http://mid.mail-archive.com/f7736294-0368-cddc-3fcd-de76cae5650e@redhat.com>. > > Thank you, > Laszlo > >> >> - return EFI_DEVICE_ERROR; >> +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; >> } >> >> >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel