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.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++--
1 file changed, 152 insertions(+), 16 deletions(-)
diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 5e72b1a24b59..a1ee810919e4 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -409,11 +409,19 @@ VirtioScsiPassThru (
UINT16 TargetValue;
EFI_STATUS Status;
volatile VIRTIO_SCSI_REQ Request;
- volatile VIRTIO_SCSI_RESP Response;
+ volatile VIRTIO_SCSI_RESP *Response;
+ VOID *ResponseBuffer;
DESC_INDICES Indices;
+ VOID *RequestMapping;
+ VOID *ResponseMapping;
+ VOID *InDataMapping;
+ VOID *OutDataMapping;
+ EFI_PHYSICAL_ADDRESS RequestDeviceAddress;
+ EFI_PHYSICAL_ADDRESS ResponseDeviceAddress;
+ EFI_PHYSICAL_ADDRESS InDataDeviceAddress;
+ EFI_PHYSICAL_ADDRESS OutDataDeviceAddress;
ZeroMem ((VOID*) &Request, sizeof (Request));
- ZeroMem ((VOID*) &Response, sizeof (Response));
Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
CopyMem (&TargetValue, Target, sizeof TargetValue);
@@ -423,12 +431,96 @@ VirtioScsiPassThru (
return Status;
}
- VirtioPrepare (&Dev->Ring, &Indices);
+ //
+ // Map the scsi-blk Request header buffer
+ //
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterRead,
+ (VOID *) &Request,
+ sizeof Request,
+ &RequestDeviceAddress,
+ &RequestMapping);
+ if (EFI_ERROR (Status)) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Map the input buffer
+ //
+ if (Packet->InTransferLength > 0) {
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterWrite,
+ Packet->InDataBuffer,
+ Packet->InTransferLength,
+ &InDataDeviceAddress,
+ &InDataMapping
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto UnmapRequestBuffer;
+ }
+ }
+
+ //
+ // Map the output buffer
+ //
+ if (Packet->OutTransferLength > 0) {
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterRead,
+ Packet->OutDataBuffer,
+ Packet->OutTransferLength,
+ &OutDataDeviceAddress,
+ &OutDataMapping
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto UnmapInDataBuffer;
+ }
+ }
+
+ //
+ // Response header is bi-direction (we preset with host status and expect the
+ // device to update it). Allocate a response buffer which can be mapped to
+ // access equally by both processor and device.
+ //
+ Status = Dev->VirtIo->AllocateSharedPages (
+ Dev->VirtIo,
+ EFI_SIZE_TO_PAGES (sizeof *Response),
+ &ResponseBuffer
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto UnmapOutDataBuffer;
+ }
+
+ Response = ResponseBuffer;
//
// preset a host status for ourselves that we do not accept as success
//
- Response.Response = VIRTIO_SCSI_S_FAILURE;
+ Response->Response = VIRTIO_SCSI_S_FAILURE;
+
+ //
+ // Map the response buffer with BusMasterCommonBuffer so that response
+ // buffer can be accessed by both host and device.
+ //
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterCommonBuffer,
+ ResponseBuffer,
+ sizeof (*Response),
+ &ResponseDeviceAddress,
+ &ResponseMapping
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_DEVICE_ERROR;
+ goto FreeResponseBuffer;
+ }
+
+ VirtioPrepare (&Dev->Ring, &Indices);
//
// ensured by VirtioScsiInit() -- this predicate, in combination with the
@@ -439,31 +531,49 @@ VirtioScsiPassThru (
//
// enqueue Request
//
- VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
- VRING_DESC_F_NEXT, &Indices);
+ VirtioAppendDesc (
+ &Dev->Ring,
+ RequestDeviceAddress,
+ sizeof Request,
+ VRING_DESC_F_NEXT,
+ &Indices
+ );
//
// enqueue "dataout" if any
//
if (Packet->OutTransferLength > 0) {
- VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
- Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
+ VirtioAppendDesc (
+ &Dev->Ring,
+ OutDataDeviceAddress,
+ Packet->OutTransferLength,
+ VRING_DESC_F_NEXT,
+ &Indices
+ );
}
//
// enqueue Response, to be written by the host
//
- VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
- VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
- VRING_DESC_F_NEXT : 0),
- &Indices);
+ VirtioAppendDesc (
+ &Dev->Ring,
+ ResponseDeviceAddress,
+ sizeof *Response,
+ VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0),
+ &Indices
+ );
//
// enqueue "datain" if any, to be written by the host
//
if (Packet->InTransferLength > 0) {
- VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
- Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
+ VirtioAppendDesc (
+ &Dev->Ring,
+ InDataDeviceAddress,
+ Packet->InTransferLength,
+ VRING_DESC_F_WRITE,
+ &Indices
+ );
}
// If kicking the host fails, we must fake a host adapter error.
@@ -477,10 +587,36 @@ VirtioScsiPassThru (
Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
Packet->SenseDataLength = 0;
- return EFI_DEVICE_ERROR;
+ Status = EFI_DEVICE_ERROR;
+ goto UnmapResponseBuffer;
}
- return ParseResponse (Packet, &Response);
+ Status = ParseResponse (Packet, Response);
+
+UnmapResponseBuffer:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
+
+FreeResponseBuffer:
+ Dev->VirtIo->FreeSharedPages (
+ Dev->VirtIo,
+ EFI_SIZE_TO_PAGES (sizeof *Response),
+ ResponseBuffer
+ );
+
+UnmapOutDataBuffer:
+ if (Packet->OutTransferLength > 0) {
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
+ }
+
+UnmapInDataBuffer:
+ if (Packet->InTransferLength > 0) {
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
+ }
+
+UnmapRequestBuffer:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
+
+ return Status;
}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Brijesh,
On 08/28/17 13:26, 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.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++--
> 1 file changed, 152 insertions(+), 16 deletions(-)
I have several comments here. I believe that this time my comments are
best kept together, so I'm not going to scatter them all over the patch.
(1) You have a typo below, "scsi-blk". I think you meant "virtio-scsi"
instead.
(2) "ResponseBuffer" is not adequately zeroed before it is mapped. The
Response->Response field is set correctly, but the entire structure
should be zeroed, after it is allocated. (Practically in place of the
ZeroMem() that you remove at the top of the patch.)
For VirtioBlkDxe, this wasn't an issue, because there the entire
response buffer was a single byte, "HostStatus". By setting that byte,
no stale data was left in the area that would be exposed to the host.
(3) By reviewing this patch, I realized that I missed an error in commit
3540f2cef57a ("Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response
buffers", 2017-08-27).
In other words, this point is about the VirtioBlkDxe driver.
Namely, when "RequestIsWrite" is FALSE -- i.e., the CPU wants data from
the device --, we map "Buffer" for VirtioOperationBusMasterWrite. In
this case, checking the return status of
Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
is critical -- we must not do that unmapping on the error path only. If
the unmapping fails, then the device's data don't reach the caller, and
we must fail the request with EFI_DEVICE_ERROR.
I believe the mitigation could be:
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 6abd937f86c6..5a63986b3f39 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -260,6 +260,7 @@ SynchronousRequest (
> EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress;
> EFI_PHYSICAL_ADDRESS RequestDeviceAddress;
> EFI_STATUS Status;
> + EFI_STATUS UnmapStatus;
>
> BlockSize = Dev->BlockIoMedia.BlockSize;
>
> @@ -430,7 +431,13 @@ SynchronousRequest (
>
> UnmapDataBuffer:
> if (BufferSize > 0) {
> - Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> + UnmapStatus = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> + if (EFI_ERROR (UnmapStatus) && !RequestIsWrite && !EFI_ERROR (Status)) {
> + //
> + // Data from the bus master may not reach the caller; fail the request.
> + //
> + Status = EFI_DEVICE_ERROR;
> + }
> }
>
> UnmapRequestBuffer:
I'm very sorry about this. If you agree with the above suggestion, can
you please submit it as a standalone patch?
(4) Back to this patch. The EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru()
member function is required to implement elaborate error reporting, and
some output fields must be set even if we report EFI_DEVICE_ERROR. The
pre-patch code conforms to the UEFI spec's requirements, and we should
keep that up.
Please locate the following code:
// If kicking the host fails, we must fake a host adapter error.
// EFI_NOT_READY would save us the effort, but it would also suggest that the
// caller retry.
//
if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
&Indices, NULL) != EFI_SUCCESS) {
Packet->InTransferLength = 0;
Packet->OutTransferLength = 0;
Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
Packet->SenseDataLength = 0;
return EFI_DEVICE_ERROR;
}
This entire block of code should be factored out to a helper function,
in a separate patch:
STATIC
EFI_STATUS
ReportHostAdapterError (
OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
)
{
Packet->InTransferLength = 0;
Packet->OutTransferLength = 0;
Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
Packet->SenseDataLength = 0;
return EFI_DEVICE_ERROR;
}
Then, in this patch, *whenever* we intend to return EFI_DEVICE_ERROR
from *before* VirtioFlush(), do:
/* attempt shared pages allocation or mapping, as appropriate */
/* ... */
if (EFI_ERROR (Status)) {
Status = ReportHostAdapterError (Packet);
goto ErrorHandlingLabel;
}
(The same should also be performed when VirtioFlush() itself fails, of
course.)
The idea is, if PopulateRequest() succeeds, but we don't reach
VirtioFlush() for another -- new -- error scenario, or VirtioFlush()
itself fails (like before), then we must fake this kind of host adapter
error in *all* cases. The above helper function will simplify that.
(5) The same issue as (3) is present in this patch (i.e., for
VirtioScsiDxe); however, it is more complicated here.
Assume that
- the caller submits a bi-directional request,
- we actually perform the request fine,
- but then we fail to unmap "InDataMapping".
In this case, flipping the return status to EFI_DEVICE_ERROR just
doesn't cut it: we'd have to update the Packet->xxxx fields accordingly,
so that they report the full loss of the incoming transfer. This would
be hellishly difficult; the update would have to be different for a
bidirectional transfer vs. for a simple read, and in general, who wants
to mess with SCSI sense data?
Therefore we have to guarantee *in advance* that this error won't
present itself. Which means, of course, a CommonBuffer mapping for the
input transfer (if any):
(5a) If "Packet->InTransferLength" is positive on input, allocate a
shared buffer, zero it (!), and finally map it for CommonBuffer
operation (--> InDataMapping). Proceed with the rest of the patch.
(5b) If VirtioFlush() fails, then do as before (see point (4) above) --
report a host adapter error.
(5c) if VirtioFlush() succeeds, then call ParseResponse(), storing its
return value in "Status".
(5d) Now, ParseResponse() will *always* update
"Packet->InTransferLength". Therefore, after ParseResponse() returns, if
"Packet->InTransferLength" is positive, then we have to CopyMem()
"Packet->InTransferLength" bytes from the common buffer to
"Packet->InDataBuffer".
This way, we'll only have to unmap BusMasterCommonBuffer and
BusMasterRead mappings on the final path (both on success and error),
and we won't have to care about their return values.
Also, on the final path (on both success and error), we don't have to
touch "Packet":
- we either reached ParseResponse(), and then it set the output fields
appropriately,
- or we jumped over ParseResponse() due to an error, but in all such
cases, we called ReportHostAdapterError(), so the output fields are
set again.
Thank you,
Laszlo
On 08/28/17 13:26, Brijesh Singh wrote:
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e72b1a24b59..a1ee810919e4 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -409,11 +409,19 @@ VirtioScsiPassThru (
> UINT16 TargetValue;
> EFI_STATUS Status;
> volatile VIRTIO_SCSI_REQ Request;
> - volatile VIRTIO_SCSI_RESP Response;
> + volatile VIRTIO_SCSI_RESP *Response;
> + VOID *ResponseBuffer;
> DESC_INDICES Indices;
> + VOID *RequestMapping;
> + VOID *ResponseMapping;
> + VOID *InDataMapping;
> + VOID *OutDataMapping;
> + EFI_PHYSICAL_ADDRESS RequestDeviceAddress;
> + EFI_PHYSICAL_ADDRESS ResponseDeviceAddress;
> + EFI_PHYSICAL_ADDRESS InDataDeviceAddress;
> + EFI_PHYSICAL_ADDRESS OutDataDeviceAddress;
>
> ZeroMem ((VOID*) &Request, sizeof (Request));
> - ZeroMem ((VOID*) &Response, sizeof (Response));
>
> Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
> CopyMem (&TargetValue, Target, sizeof TargetValue);
> @@ -423,12 +431,96 @@ VirtioScsiPassThru (
> return Status;
> }
>
> - VirtioPrepare (&Dev->Ring, &Indices);
> + //
> + // Map the scsi-blk Request header buffer
> + //
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterRead,
> + (VOID *) &Request,
> + sizeof Request,
> + &RequestDeviceAddress,
> + &RequestMapping);
> + if (EFI_ERROR (Status)) {
> + return EFI_DEVICE_ERROR;
> + }
> +
> + //
> + // Map the input buffer
> + //
> + if (Packet->InTransferLength > 0) {
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterWrite,
> + Packet->InDataBuffer,
> + Packet->InTransferLength,
> + &InDataDeviceAddress,
> + &InDataMapping
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_DEVICE_ERROR;
> + goto UnmapRequestBuffer;
> + }
> + }
> +
> + //
> + // Map the output buffer
> + //
> + if (Packet->OutTransferLength > 0) {
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterRead,
> + Packet->OutDataBuffer,
> + Packet->OutTransferLength,
> + &OutDataDeviceAddress,
> + &OutDataMapping
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_DEVICE_ERROR;
> + goto UnmapInDataBuffer;
> + }
> + }
> +
> + //
> + // Response header is bi-direction (we preset with host status and expect the
> + // device to update it). Allocate a response buffer which can be mapped to
> + // access equally by both processor and device.
> + //
> + Status = Dev->VirtIo->AllocateSharedPages (
> + Dev->VirtIo,
> + EFI_SIZE_TO_PAGES (sizeof *Response),
> + &ResponseBuffer
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_DEVICE_ERROR;
> + goto UnmapOutDataBuffer;
> + }
> +
> + Response = ResponseBuffer;
>
> //
> // preset a host status for ourselves that we do not accept as success
> //
> - Response.Response = VIRTIO_SCSI_S_FAILURE;
> + Response->Response = VIRTIO_SCSI_S_FAILURE;
> +
> + //
> + // Map the response buffer with BusMasterCommonBuffer so that response
> + // buffer can be accessed by both host and device.
> + //
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterCommonBuffer,
> + ResponseBuffer,
> + sizeof (*Response),
> + &ResponseDeviceAddress,
> + &ResponseMapping
> + );
> + if (EFI_ERROR (Status)) {
> + Status = EFI_DEVICE_ERROR;
> + goto FreeResponseBuffer;
> + }
> +
> + VirtioPrepare (&Dev->Ring, &Indices);
>
> //
> // ensured by VirtioScsiInit() -- this predicate, in combination with the
> @@ -439,31 +531,49 @@ VirtioScsiPassThru (
> //
> // enqueue Request
> //
> - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> - VRING_DESC_F_NEXT, &Indices);
> + VirtioAppendDesc (
> + &Dev->Ring,
> + RequestDeviceAddress,
> + sizeof Request,
> + VRING_DESC_F_NEXT,
> + &Indices
> + );
>
> //
> // enqueue "dataout" if any
> //
> if (Packet->OutTransferLength > 0) {
> - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
> - Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
> + VirtioAppendDesc (
> + &Dev->Ring,
> + OutDataDeviceAddress,
> + Packet->OutTransferLength,
> + VRING_DESC_F_NEXT,
> + &Indices
> + );
> }
>
> //
> // enqueue Response, to be written by the host
> //
> - VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
> - VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
> - VRING_DESC_F_NEXT : 0),
> - &Indices);
> + VirtioAppendDesc (
> + &Dev->Ring,
> + ResponseDeviceAddress,
> + sizeof *Response,
> + VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0),
> + &Indices
> + );
>
> //
> // enqueue "datain" if any, to be written by the host
> //
> if (Packet->InTransferLength > 0) {
> - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
> - Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
> + VirtioAppendDesc (
> + &Dev->Ring,
> + InDataDeviceAddress,
> + Packet->InTransferLength,
> + VRING_DESC_F_WRITE,
> + &Indices
> + );
> }
>
> // If kicking the host fails, we must fake a host adapter error.
> @@ -477,10 +587,36 @@ VirtioScsiPassThru (
> Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> Packet->SenseDataLength = 0;
> - return EFI_DEVICE_ERROR;
> + Status = EFI_DEVICE_ERROR;
> + goto UnmapResponseBuffer;
> }
>
> - return ParseResponse (Packet, &Response);
> + Status = ParseResponse (Packet, Response);
> +
> +UnmapResponseBuffer:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
> +
> +FreeResponseBuffer:
> + Dev->VirtIo->FreeSharedPages (
> + Dev->VirtIo,
> + EFI_SIZE_TO_PAGES (sizeof *Response),
> + ResponseBuffer
> + );
> +
> +UnmapOutDataBuffer:
> + if (Packet->OutTransferLength > 0) {
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> + }
> +
> +UnmapInDataBuffer:
> + if (Packet->InTransferLength > 0) {
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
> + }
> +
> +UnmapRequestBuffer:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> + return Status;
> }
>
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 8/29/17 11:02 AM, Laszlo Ersek wrote:
> Hi Brijesh,
>
> On 08/28/17 13:26, 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.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++--
>> 1 file changed, 152 insertions(+), 16 deletions(-)
> I have several comments here. I believe that this time my comments are
> best kept together, so I'm not going to scatter them all over the patch.
>
> (1) You have a typo below, "scsi-blk". I think you meant "virtio-scsi"
> instead.
Will fix it, thanks
>
> (2) "ResponseBuffer" is not adequately zeroed before it is mapped. The
> Response->Response field is set correctly, but the entire structure
> should be zeroed, after it is allocated. (Practically in place of the
> ZeroMem() that you remove at the top of the patch.)
>
> For VirtioBlkDxe, this wasn't an issue, because there the entire
> response buffer was a single byte, "HostStatus". By setting that byte,
> no stale data was left in the area that would be exposed to the host.
Yes good catch. will fix in v2
>
> (3) By reviewing this patch, I realized that I missed an error in commit
> 3540f2cef57a ("Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response
> buffers", 2017-08-27).
>
> In other words, this point is about the VirtioBlkDxe driver.
>
> Namely, when "RequestIsWrite" is FALSE -- i.e., the CPU wants data from
> the device --, we map "Buffer" for VirtioOperationBusMasterWrite. In
> this case, checking the return status of
>
> Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
>
> is critical -- we must not do that unmapping on the error path only. If
> the unmapping fails, then the device's data don't reach the caller, and
> we must fail the request with EFI_DEVICE_ERROR.
>
> I believe the mitigation could be:
I will send separate patch to handle this case.
>> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> index 6abd937f86c6..5a63986b3f39 100644
>> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> @@ -260,6 +260,7 @@ SynchronousRequest (
>> EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress;
>> EFI_PHYSICAL_ADDRESS RequestDeviceAddress;
>> EFI_STATUS Status;
>> + EFI_STATUS UnmapStatus;
>>
>> BlockSize = Dev->BlockIoMedia.BlockSize;
>>
>> @@ -430,7 +431,13 @@ SynchronousRequest (
>>
>> UnmapDataBuffer:
>> if (BufferSize > 0) {
>> - Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
>> + UnmapStatus = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
>> + if (EFI_ERROR (UnmapStatus) && !RequestIsWrite && !EFI_ERROR (Status)) {
>> + //
>> + // Data from the bus master may not reach the caller; fail the request.
>> + //
>> + Status = EFI_DEVICE_ERROR;
>> + }
>> }
>>
>> UnmapRequestBuffer:
> I'm very sorry about this. If you agree with the above suggestion, can
> you please submit it as a standalone patch?
>
>
> (4) Back to this patch. The EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru()
> member function is required to implement elaborate error reporting, and
> some output fields must be set even if we report EFI_DEVICE_ERROR. The
> pre-patch code conforms to the UEFI spec's requirements, and we should
> keep that up.
>
> Please locate the following code:
>
> // If kicking the host fails, we must fake a host adapter error.
> // EFI_NOT_READY would save us the effort, but it would also suggest that the
> // caller retry.
> //
> if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
> &Indices, NULL) != EFI_SUCCESS) {
> Packet->InTransferLength = 0;
> Packet->OutTransferLength = 0;
> Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> Packet->SenseDataLength = 0;
> return EFI_DEVICE_ERROR;
> }
>
> This entire block of code should be factored out to a helper function,
> in a separate patch:
>
> STATIC
> EFI_STATUS
> ReportHostAdapterError (
> OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> )
> {
> Packet->InTransferLength = 0;
> Packet->OutTransferLength = 0;
> Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> Packet->SenseDataLength = 0;
> return EFI_DEVICE_ERROR;
> }
>
> Then, in this patch, *whenever* we intend to return EFI_DEVICE_ERROR
> from *before* VirtioFlush(), do:
>
> /* attempt shared pages allocation or mapping, as appropriate */
> /* ... */
> if (EFI_ERROR (Status)) {
> Status = ReportHostAdapterError (Packet);
> goto ErrorHandlingLabel;
> }
>
> (The same should also be performed when VirtioFlush() itself fails, of
> course.)
>
> The idea is, if PopulateRequest() succeeds, but we don't reach
> VirtioFlush() for another -- new -- error scenario, or VirtioFlush()
> itself fails (like before), then we must fake this kind of host adapter
> error in *all* cases. The above helper function will simplify that.
>
>
> (5) The same issue as (3) is present in this patch (i.e., for
> VirtioScsiDxe); however, it is more complicated here.
>
> Assume that
>
> - the caller submits a bi-directional request,
>
> - we actually perform the request fine,
>
> - but then we fail to unmap "InDataMapping".
>
> In this case, flipping the return status to EFI_DEVICE_ERROR just
> doesn't cut it: we'd have to update the Packet->xxxx fields accordingly,
> so that they report the full loss of the incoming transfer. This would
> be hellishly difficult; the update would have to be different for a
> bidirectional transfer vs. for a simple read, and in general, who wants
> to mess with SCSI sense data?
>
> Therefore we have to guarantee *in advance* that this error won't
> present itself. Which means, of course, a CommonBuffer mapping for the
> input transfer (if any):
>
> (5a) If "Packet->InTransferLength" is positive on input, allocate a
> shared buffer, zero it (!), and finally map it for CommonBuffer
> operation (--> InDataMapping). Proceed with the rest of the patch.
>
> (5b) If VirtioFlush() fails, then do as before (see point (4) above) --
> report a host adapter error.
>
> (5c) if VirtioFlush() succeeds, then call ParseResponse(), storing its
> return value in "Status".
>
> (5d) Now, ParseResponse() will *always* update
> "Packet->InTransferLength". Therefore, after ParseResponse() returns, if
> "Packet->InTransferLength" is positive, then we have to CopyMem()
> "Packet->InTransferLength" bytes from the common buffer to
> "Packet->InDataBuffer".
>
> This way, we'll only have to unmap BusMasterCommonBuffer and
> BusMasterRead mappings on the final path (both on success and error),
> and we won't have to care about their return values.
>
> Also, on the final path (on both success and error), we don't have to
> touch "Packet":
>
> - we either reached ParseResponse(), and then it set the output fields
> appropriately,
>
> - or we jumped over ParseResponse() due to an error, but in all such
> cases, we called ReportHostAdapterError(), so the output fields are
> set again.
I will go ahead and implement your feedback in v2 and send it possibly
today. thanks
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.