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
