* [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-09 14:27 ` Laszlo Ersek
2017-08-07 11:58 ` [PATCH v1 02/14] OvmfPkg/Virtio10Dxe: Implement new member functions Brijesh Singh
` (14 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The patch extends VIRTIO_DEVICE_PROTOCOL to provide the following new
member functions:
- AllocateSharedPages : allocate a memory region suitable for sharing
between guest and hypervisor (e.g ring buffer).
- FreeSharedPages: free the memory allocated using AllocateSharedPages ().
- MapSharedBuffer: map a host address to device address suitable to share
with device for bus master operations.
- UnmapSharedBuffer: unmap the device address obtained through the
MapSharedBuffer().
Suggested-by: Laszlo Ersek <lersek@redhat.com>
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/Include/Protocol/VirtioDevice.h | 121 ++++++++++++++++++++
1 file changed, 121 insertions(+)
diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h
index 910a4866e7ac..ea5272165389 100644
--- a/OvmfPkg/Include/Protocol/VirtioDevice.h
+++ b/OvmfPkg/Include/Protocol/VirtioDevice.h
@@ -5,6 +5,7 @@
and should not be used outside of the EDK II tree.
Copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -31,6 +32,26 @@
typedef struct _VIRTIO_DEVICE_PROTOCOL VIRTIO_DEVICE_PROTOCOL;
+//
+// VIRTIO Operation for Map
+//
+typedef enum {
+ //
+ // A read operation from system memory by a bus master
+ //
+ EfiVirtIoOperationBusMasterRead,
+ //
+ // A write operation from system memory by a bus master
+ //
+ EfiVirtIoOperationBusMasterWrite,
+ //
+ // Provides both read and write access to system memory by both the
+ // processor and a bus master
+ //
+ EfiVirtIoOperationBusMasterCommonBuffer,
+ EfiVirtIoOperationMaximum
+} VIRTIO_MAP_OPERATION;
+
/**
Read a word from the device-specific I/O region of the Virtio Header.
@@ -319,6 +340,100 @@ EFI_STATUS
IN UINT8 DeviceStatus
);
+/**
+
+ Allocates pages that are suitable for sharing between guest and hypervisor.
+
+ @param This The protocol instance pointer.
+ @param Pages The number of pages to allocate.
+ @param HostAddress A pointer to store the base system memory
+ address of the allocated range.
+
+ @retval EFI_SUCCESS The requested memory pages were allocated.
+ @retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *VIRTIO_ALLOCATE_SHARED)(
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN UINTN Pages,
+ IN OUT VOID **HostAddress
+ );
+
+/**
+ Frees memory that was allocated with SharedAllocateBuffer().
+
+ @param This The protocol instance pointer.
+ @param Pages The number of pages to free.
+ @param HostAddress The base system memory address of the allocated range.
+
+**/
+typedef
+VOID
+(EFIAPI *VIRTIO_FREE_SHARED)(
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN UINTN Pages,
+ IN VOID *HostAddress
+ );
+
+/**
+ Provides the shared addresses required to access system memory from a
+ DMA bus master.
+
+ @param This The protocol instance pointer.
+ @param Operation Indicates if the bus master is going to
+ read or write to system memory.
+ @param HostAddress The system memory address to map to shared
+ buffer address.
+ @param NumberOfBytes On input the number of bytes to map.
+ On output the number of bytes that were
+ mapped.
+ @param DeviceAddress The resulting shared map address for the
+ bus master to access the hosts HostAddress.
+ @param Mapping A resulting value to pass to Unmap().
+
+
+ @retval EFI_SUCCESS The range was mapped for the returned
+ NumberOfBytes.
+ @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a
+ common buffer.
+ @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+ @retval EFI_OUT_OF_RESOURCES The request could not be completed due to
+ a lack of resources.
+ @retval EFI_DEVICE_ERROR The system hardware could not map the
+ requested address.
+**/
+
+typedef
+EFI_STATUS
+(EFIAPI *VIRTIO_MAP_SHARED) (
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN VIRTIO_MAP_OPERATION Operation,
+ IN VOID *HostAddress,
+ IN OUT UINTN *NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ OUT VOID **Mapping
+ );
+
+/**
+ Completes the Map() operation and releases any corresponding resources.
+
+ @param This The protocol instance pointer.
+ @param Mapping The mapping value returned from Map().
+
+ @retval EFI_SUCCESS The range was unmapped.
+ @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
+ Map().
+ @retval EFI_DEVICE_ERROR The data was not committed to the target
+ system memory.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *VIRTIO_UNMAP_SHARED)(
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN VOID *Mapping
+ );
///
/// This protocol provides an abstraction over the VirtIo transport layer
@@ -353,6 +468,12 @@ struct _VIRTIO_DEVICE_PROTOCOL {
// Functions to read/write Device Specific headers
VIRTIO_DEVICE_WRITE WriteDevice;
VIRTIO_DEVICE_READ ReadDevice;
+
+ // Function to allocate, free, map and unmap shared buffer
+ VIRTIO_ALLOCATE_SHARED AllocateSharedPages;
+ VIRTIO_FREE_SHARED FreeSharedPages;
+ VIRTIO_MAP_SHARED MapSharedBuffer;
+ VIRTIO_UNMAP_SHARED UnmapSharedBuffer;
};
extern EFI_GUID gVirtioDeviceProtocolGuid;
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL
2017-08-07 11:58 ` [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL Brijesh Singh
@ 2017-08-09 14:27 ` Laszlo Ersek
2017-08-09 18:23 ` Brijesh Singh
0 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 14:27 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/07/17 13:58, Brijesh Singh wrote:
> The patch extends VIRTIO_DEVICE_PROTOCOL to provide the following new
> member functions:
>
> - AllocateSharedPages : allocate a memory region suitable for sharing
> between guest and hypervisor (e.g ring buffer).
>
> - FreeSharedPages: free the memory allocated using AllocateSharedPages ().
>
> - MapSharedBuffer: map a host address to device address suitable to share
> with device for bus master operations.
>
> - UnmapSharedBuffer: unmap the device address obtained through the
> MapSharedBuffer().
>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> 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/Include/Protocol/VirtioDevice.h | 121 ++++++++++++++++++++
> 1 file changed, 121 insertions(+)
(1) I suggest the following subject:
OvmfPkg: introduce IOMMU-like member functions to VIRTIO_DEVICE_PROTOCOL
The current wording "new member functions" is a bit too generic IMO.
>
> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h
> index 910a4866e7ac..ea5272165389 100644
> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h
> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h
> @@ -5,6 +5,7 @@
> and should not be used outside of the EDK II tree.
>
> Copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -31,6 +32,26 @@
>
> typedef struct _VIRTIO_DEVICE_PROTOCOL VIRTIO_DEVICE_PROTOCOL;
>
> +//
> +// VIRTIO Operation for Map
> +//
(2) "Map" can be made a bit more precise; please say either
"VIRTIO_MAP_SHARED" or MapSharedBuffer().
> +typedef enum {
> + //
> + // A read operation from system memory by a bus master
> + //
> + EfiVirtIoOperationBusMasterRead,
> + //
> + // A write operation from system memory by a bus master
> + //
> + EfiVirtIoOperationBusMasterWrite,
(3) s/from/to/
> + //
> + // Provides both read and write access to system memory by both the
> + // processor and a bus master
> + //
> + EfiVirtIoOperationBusMasterCommonBuffer,
> + EfiVirtIoOperationMaximum
> +} VIRTIO_MAP_OPERATION;
> +
(4) Please drop the "Efi" prefix.
(5) Please replace the remaining "VirtIo" prefix with "Virtio". Although
you can find both spellings in the source code, they are used in
different contexts. In function names and enumeration constants, we've
used Virtio* thus far, as far as I can see.
I hope this won't cause too much churn for the series!
> /**
>
> Read a word from the device-specific I/O region of the Virtio Header.
> @@ -319,6 +340,100 @@ EFI_STATUS
> IN UINT8 DeviceStatus
> );
>
> +/**
> +
> + Allocates pages that are suitable for sharing between guest and hypervisor.
> +
> + @param This The protocol instance pointer.
> + @param Pages The number of pages to allocate.
> + @param HostAddress A pointer to store the base system memory
> + address of the allocated range.
> +
> + @retval EFI_SUCCESS The requested memory pages were allocated.
> + @retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_ALLOCATE_SHARED)(
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN Pages,
> + IN OUT VOID **HostAddress
> + );
The typedef / function prototype looks good. Some comments for the
comment block:
(6) s/base system memory address/system memory base address/, I'd think;
but I could be convinced otherwise
(7) Please annotate each @param with: [in], [out], or [in,out], as
appropriate (see other examples in the same header file).
(This affects the rest of the members added in this patch.
It also likely affects all three implementations of this set of members
(via copy-pasted comment blocks).)
> +
> +/**
> + Frees memory that was allocated with SharedAllocateBuffer().
(8) s/SharedAllocateBuffer()/VIRTIO_ALLOCATE_SHARED/
> +
> + @param This The protocol instance pointer.
> + @param Pages The number of pages to free.
> + @param HostAddress The base system memory address of the allocated range.
(9) Whatever you do for comment (6), please apply it here as well.
> +
> +**/
> +typedef
> +VOID
> +(EFIAPI *VIRTIO_FREE_SHARED)(
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN Pages,
> + IN VOID *HostAddress
> + );
> +
> +/**
> + Provides the shared addresses required to access system memory from a
> + DMA bus master.
(10) what do you think of s/shared addresses/device address/, singular?
(Also, replace "shared" with "virtio device" or just "device" below?)
Just an idea.
> +
> + @param This The protocol instance pointer.
> + @param Operation Indicates if the bus master is going to
> + read or write to system memory.
> + @param HostAddress The system memory address to map to shared
> + buffer address.
(11) Please point out the connection between
VirtioOperationBusMasterCommonBuffer, VIRTIO_ALLOCATE_SHARED, and
HostAddress.
It doesn't have to be extremely verbose (feel free to steal and adapt
the woring of the PciIo chapter in the UEFI spec), but since this is a
protocol header file, we should say something about that connection.
It's also fine if you just add a pointer to the UEFI spec (spec version,
chapter/section, interface), and say "this interface follows the same
usage pattern".
> + @param NumberOfBytes On input the number of bytes to map.
> + On output the number of bytes that were
> + mapped.
> + @param DeviceAddress The resulting shared map address for the
> + bus master to access the hosts HostAddress.
> + @param Mapping A resulting value to pass to Unmap().
(12) What do you think of: "a resulting token to pass to
VIRTIO_UNMAP_SHARED"?
> +
> +
> + @retval EFI_SUCCESS The range was mapped for the returned
> + NumberOfBytes.
> + @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a
> + common buffer.
> + @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to
> + a lack of resources.
> + @retval EFI_DEVICE_ERROR The system hardware could not map the
> + requested address.
> +**/
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_MAP_SHARED) (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN VIRTIO_MAP_OPERATION Operation,
> + IN VOID *HostAddress,
> + IN OUT UINTN *NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + );
> +
Looks OK.
> +/**
> + Completes the Map() operation and releases any corresponding resources.
(13) What do you think of s/Map()/VIRTIO_MAP_SHARED/?
> +
> + @param This The protocol instance pointer.
> + @param Mapping The mapping value returned from Map().
(14) I suggest "the Mapping token output by VIRTIO_MAP_SHARED".
> +
> + @retval EFI_SUCCESS The range was unmapped.
> + @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
> + Map().
(15) s/value/token/
(16) Please remind the reader that the implementation is not required to
recognize and report all such cases. Invalid Mapping tokens can cause
undefined behavior.
> + @retval EFI_DEVICE_ERROR The data was not committed to the target
> + system memory.
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *VIRTIO_UNMAP_SHARED)(
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN VOID *Mapping
> + );
OK.
(In general I'm not sure if I like EFI_STATUS as return type here. I
would prefer VOID, I think -- my guess is that the rest of the patch set
will ignore the return value anywayy:
- EFI_SUCCESS fits VOID obviously.
- EFI_INVALID_PARAMETER is fully preventable if the caller sticks with
the valid use pattern (and if they don't they can get undefined
behavior anyway).
The only reason I think we should still keep the EFI_STATUS return type
on the interface level is that Unmap() might do actual data movement
after BusMasterWrite. Before client code consumes such data, the client
code should really check whether Unmap() succeeded (i.e., see the
EFI_DEVICE_ERROR case).
So, I guess let's keep the EFI_STATUS return type. (And check the
returns status everywhere in this series, wherever appropriate.)
>
> ///
> /// This protocol provides an abstraction over the VirtIo transport layer
> @@ -353,6 +468,12 @@ struct _VIRTIO_DEVICE_PROTOCOL {
> // Functions to read/write Device Specific headers
> VIRTIO_DEVICE_WRITE WriteDevice;
> VIRTIO_DEVICE_READ ReadDevice;
> +
> + // Function to allocate, free, map and unmap shared buffer
(17) s/Function/Functions/, plural
(18) I realize the current comments in this structure don't conform to
the coding style. Can you perhaps prepend a patch that fixes the comment
style for the following:
// VirtIo Specification Revision: ...
/// VirtIo Specification Revision encoded with VIRTIO_SPEC_REVISION()
/// From the Virtio Spec
// Functions to read/write Device Specific headers
They should all be spelled
//
// comment
//
(There is no general need to prefix or suffix such comments with
additional empty lines -- use your judgement. In actual code, the empty
lines are helpful; between fields of a structure, not necessarily.)
(19) and then please adapt the new comment in this patch.
(20) This is actually for the commit message, but I'm too lazy to
renumber my points :) In the commit message, please state that:
We're free to extend the protocol structure without changing the
protocol GUID, or bumping any protocol version fields (of which we
currently have none), because VIRTIO_DEVICE_PROTOCOL is internal to edk2
by design -- see the disclaimers in "VirtioDevice.h".
Thanks!
Laszlo
> + VIRTIO_ALLOCATE_SHARED AllocateSharedPages;
> + VIRTIO_FREE_SHARED FreeSharedPages;
> + VIRTIO_MAP_SHARED MapSharedBuffer;
> + VIRTIO_UNMAP_SHARED UnmapSharedBuffer;
> };
>
> extern EFI_GUID gVirtioDeviceProtocolGuid;
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL
2017-08-09 14:27 ` Laszlo Ersek
@ 2017-08-09 18:23 ` Brijesh Singh
0 siblings, 0 replies; 42+ messages in thread
From: Brijesh Singh @ 2017-08-09 18:23 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/09/2017 09:27 AM, Laszlo Ersek wrote:
> On 08/07/17 13:58, Brijesh Singh wrote:
>> The patch extends VIRTIO_DEVICE_PROTOCOL to provide the following new
>> member functions:
>>
>> - AllocateSharedPages : allocate a memory region suitable for sharing
>> between guest and hypervisor (e.g ring buffer).
>>
>> - FreeSharedPages: free the memory allocated using AllocateSharedPages ().
>>
>> - MapSharedBuffer: map a host address to device address suitable to share
>> with device for bus master operations.
>>
>> - UnmapSharedBuffer: unmap the device address obtained through the
>> MapSharedBuffer().
>>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> 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/Include/Protocol/VirtioDevice.h | 121 ++++++++++++++++++++
>> 1 file changed, 121 insertions(+)
>
> (1) I suggest the following subject:
>
> OvmfPkg: introduce IOMMU-like member functions to VIRTIO_DEVICE_PROTOCOL
>
> The current wording "new member functions" is a bit too generic IMO.
>
Will do.
>>
>> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h
>> index 910a4866e7ac..ea5272165389 100644
>> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h
>> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h
>> @@ -5,6 +5,7 @@
>> and should not be used outside of the EDK II tree.
>>
>> Copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
>> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>>
>> This program and the accompanying materials are licensed and made available
>> under the terms and conditions of the BSD License which accompanies this
>> @@ -31,6 +32,26 @@
>>
>> typedef struct _VIRTIO_DEVICE_PROTOCOL VIRTIO_DEVICE_PROTOCOL;
>>
>> +//
>> +// VIRTIO Operation for Map
>> +//
>
> (2) "Map" can be made a bit more precise; please say either
> "VIRTIO_MAP_SHARED" or MapSharedBuffer().
>
Will do
>> +typedef enum {
>> + //
>> + // A read operation from system memory by a bus master
>> + //
>> + EfiVirtIoOperationBusMasterRead,
>> + //
>> + // A write operation from system memory by a bus master
>> + //
>> + EfiVirtIoOperationBusMasterWrite,
>
> (3) s/from/to/
>
Will update it. thanks
>> + //
>> + // Provides both read and write access to system memory by both the
>> + // processor and a bus master
>> + //
>> + EfiVirtIoOperationBusMasterCommonBuffer,
>> + EfiVirtIoOperationMaximum
>> +} VIRTIO_MAP_OPERATION;
>> +
>
> (4) Please drop the "Efi" prefix.
>
> (5) Please replace the remaining "VirtIo" prefix with "Virtio". Although
> you can find both spellings in the source code, they are used in
> different contexts. In function names and enumeration constants, we've
> used Virtio* thus far, as far as I can see.
>
> I hope this won't cause too much churn for the series!
>
I was actually trying to follow the IOMMU protocol header file, but I will
drop the Efi Prefix and also fix the "Virtio" prefix.
Drivers don't use these macro's directly, I have a helper function in Patch 4
to map the shared buffers. Depending on your feedback on Patch #4, it may not
be bad change at all. If we don't want those helper functions from patch 4 then
I will anyway have to update the drivers. In summary, I will make the suggested
changes in v2.
>> /**
>>
>> Read a word from the device-specific I/O region of the Virtio Header.
>> @@ -319,6 +340,100 @@ EFI_STATUS
>> IN UINT8 DeviceStatus
>> );
>>
>> +/**
>> +
>> + Allocates pages that are suitable for sharing between guest and hypervisor.
>> +
>> + @param This The protocol instance pointer.
>> + @param Pages The number of pages to allocate.
>> + @param HostAddress A pointer to store the base system memory
>> + address of the allocated range.
>> +
>> + @retval EFI_SUCCESS The requested memory pages were allocated.
>> + @retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated.
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *VIRTIO_ALLOCATE_SHARED)(
>> + IN VIRTIO_DEVICE_PROTOCOL *This,
>> + IN UINTN Pages,
>> + IN OUT VOID **HostAddress
>> + );
>
> The typedef / function prototype looks good. Some comments for the
> comment block:
>
> (6) s/base system memory address/system memory base address/, I'd think;
> but I could be convinced otherwise
>
> (7) Please annotate each @param with: [in], [out], or [in,out], as
> appropriate (see other examples in the same header file).
>
> (This affects the rest of the members added in this patch.
>
> It also likely affects all three implementations of this set of members
> (via copy-pasted comment blocks).)
>
I will fix those, I was doing copy/paste of these functions from
MdeModules/Include/Protocol/IoMmu.h hence missed updating the annotate of
each param.
>> +
>> +/**
>> + Frees memory that was allocated with SharedAllocateBuffer().
>
> (8) s/SharedAllocateBuffer()/VIRTIO_ALLOCATE_SHARED/
>
Will do thanks
>> +
>> + @param This The protocol instance pointer.
>> + @param Pages The number of pages to free.
>> + @param HostAddress The base system memory address of the allocated range.
>
> (9) Whatever you do for comment (6), please apply it here as well.
>
Will do
>> +
>> +**/
>> +typedef
>> +VOID
>> +(EFIAPI *VIRTIO_FREE_SHARED)(
>> + IN VIRTIO_DEVICE_PROTOCOL *This,
>> + IN UINTN Pages,
>> + IN VOID *HostAddress
>> + );
>> +
>> +/**
>> + Provides the shared addresses required to access system memory from a
>> + DMA bus master.
>
> (10) what do you think of s/shared addresses/device address/, singular?
> (Also, replace "shared" with "virtio device" or just "device" below?)
> Just an idea.
>
device address works for me, I will update it.
>> +
>> + @param This The protocol instance pointer.
>> + @param Operation Indicates if the bus master is going to
>> + read or write to system memory.
>> + @param HostAddress The system memory address to map to shared
>> + buffer address.
>
> (11) Please point out the connection between
> VirtioOperationBusMasterCommonBuffer, VIRTIO_ALLOCATE_SHARED, and
> HostAddress.
>
> It doesn't have to be extremely verbose (feel free to steal and adapt
> the woring of the PciIo chapter in the UEFI spec), but since this is a
> protocol header file, we should say something about that connection.
>
> It's also fine if you just add a pointer to the UEFI spec (spec version,
> chapter/section, interface), and say "this interface follows the same
> usage pattern".
>
Again this was basically copy/paste from IoMmu.h hence I did not put anything
regarding the VirtioOperationBusMasterCommonBuffer, I will mention the UEFI
spec version and section.
>> + @param NumberOfBytes On input the number of bytes to map.
>> + On output the number of bytes that were
>> + mapped.
>> + @param DeviceAddress The resulting shared map address for the
>> + bus master to access the hosts HostAddress.
>> + @param Mapping A resulting value to pass to Unmap().
>
> (12) What do you think of: "a resulting token to pass to
> VIRTIO_UNMAP_SHARED"?
>
Works for me. thanks
>> +
>> +
>> + @retval EFI_SUCCESS The range was mapped for the returned
>> + NumberOfBytes.
>> + @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a
>> + common buffer.
>> + @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
>> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to
>> + a lack of resources.
>> + @retval EFI_DEVICE_ERROR The system hardware could not map the
>> + requested address.
>> +**/
>> +
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *VIRTIO_MAP_SHARED) (
>> + IN VIRTIO_DEVICE_PROTOCOL *This,
>> + IN VIRTIO_MAP_OPERATION Operation,
>> + IN VOID *HostAddress,
>> + IN OUT UINTN *NumberOfBytes,
>> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
>> + OUT VOID **Mapping
>> + );
>> +
>
> Looks OK.
>
>> +/**
>> + Completes the Map() operation and releases any corresponding resources.
>
> (13) What do you think of s/Map()/VIRTIO_MAP_SHARED/?
>
Works for me
>> +
>> + @param This The protocol instance pointer.
>> + @param Mapping The mapping value returned from Map().
>
> (14) I suggest "the Mapping token output by VIRTIO_MAP_SHARED".
>
Will do
>> +
>> + @retval EFI_SUCCESS The range was unmapped.
>> + @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
>> + Map().
>
> (15) s/value/token/
>
Will do
> (16) Please remind the reader that the implementation is not required to
> recognize and report all such cases. Invalid Mapping tokens can cause
> undefined behavior.
>
Will do
>> + @retval EFI_DEVICE_ERROR The data was not committed to the target
>> + system memory.
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *VIRTIO_UNMAP_SHARED)(
>> + IN VIRTIO_DEVICE_PROTOCOL *This,
>> + IN VOID *Mapping
>> + );
>
> OK.
>
> (In general I'm not sure if I like EFI_STATUS as return type here. I
> would prefer VOID, I think -- my guess is that the rest of the patch set
> will ignore the return value anywayy:
> - EFI_SUCCESS fits VOID obviously.
> - EFI_INVALID_PARAMETER is fully preventable if the caller sticks with
> the valid use pattern (and if they don't they can get undefined
> behavior anyway).
>
> The only reason I think we should still keep the EFI_STATUS return type
> on the interface level is that Unmap() might do actual data movement
> after BusMasterWrite. Before client code consumes such data, the client
> code should really check whether Unmap() succeeded (i.e., see the
> EFI_DEVICE_ERROR case).
>
> So, I guess let's keep the EFI_STATUS return type. (And check the
> returns status everywhere in this series, wherever appropriate.)
>
Okay lets make sure we check the status when required
>>
>> ///
>> /// This protocol provides an abstraction over the VirtIo transport layer
>> @@ -353,6 +468,12 @@ struct _VIRTIO_DEVICE_PROTOCOL {
>> // Functions to read/write Device Specific headers
>> VIRTIO_DEVICE_WRITE WriteDevice;
>> VIRTIO_DEVICE_READ ReadDevice;
>> +
>> + // Function to allocate, free, map and unmap shared buffer
>
> (17) s/Function/Functions/, plural
>
Will fix it
> (18) I realize the current comments in this structure don't conform to
> the coding style. Can you perhaps prepend a patch that fixes the comment
> style for the following:
>
Sure will do
> // VirtIo Specification Revision: ...
>
> /// VirtIo Specification Revision encoded with VIRTIO_SPEC_REVISION()
>
> /// From the Virtio Spec
>
> // Functions to read/write Device Specific headers
>
> They should all be spelled
>
> //
> // comment
> //
>
> (There is no general need to prefix or suffix such comments with
> additional empty lines -- use your judgement. In actual code, the empty
> lines are helpful; between fields of a structure, not necessarily.)
>
> (19) and then please adapt the new comment in this patch.
>
> (20) This is actually for the commit message, but I'm too lazy to
> renumber my points :) In the commit message, please state that:
>
> We're free to extend the protocol structure without changing the
> protocol GUID, or bumping any protocol version fields (of which we
> currently have none), because VIRTIO_DEVICE_PROTOCOL is internal to edk2
> by design -- see the disclaimers in "VirtioDevice.h".
>
Sure, I will use the message in commit.
Thanks
Brijesh
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 02/14] OvmfPkg/Virtio10Dxe: Implement new member functions
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-09 16:50 ` Laszlo Ersek
2017-08-07 11:58 ` [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: " Brijesh Singh
` (13 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The patch implements the newly added member functions by respectively
delegating the job to:
- EFI_PCI_IO_PROTOCOL.AllocateBuffer() -- with BootServicesData
- EFI_PCI_IO_PROTOCOL.FreeBuffer()
- EFI_PCI_IO_PROTOCOL.Map()
- EFI_PCI_IO_PROTOCOL.Unmap()
Suggested-by: Laszlo Ersek <lersek@redhat.com>
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/Virtio10Dxe/Virtio10.c | 114 +++++++++++++++++++-
1 file changed, 113 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
index a8a6a58c3f1d..5bc8f1c7ee27 100644
--- a/OvmfPkg/Virtio10Dxe/Virtio10.c
+++ b/OvmfPkg/Virtio10Dxe/Virtio10.c
@@ -2,6 +2,7 @@
A non-transitional driver for VirtIo 1.0 PCI devices.
Copyright (C) 2016, Red Hat, Inc.
+ Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -772,6 +773,113 @@ Virtio10ReadDevice (
return Status;
}
+STATIC
+EFI_STATUS
+EFIAPI
+Virtio10AllocateSharedPages (
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN UINTN Pages,
+ IN OUT VOID **HostAddress
+ )
+{
+ VIRTIO_1_0_DEV *Dev;
+ EFI_STATUS Status;
+
+ Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
+
+ Status = Dev->PciIo->AllocateBuffer (
+ Dev->PciIo,
+ AllocateAnyPages,
+ EfiBootServicesData,
+ Pages,
+ HostAddress,
+ 0
+ );
+ return Status;
+}
+
+STATIC
+VOID
+EFIAPI
+Virtio10FreeSharedPages (
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN UINTN Pages,
+ IN VOID *HostAddress
+ )
+{
+ VIRTIO_1_0_DEV *Dev;
+
+ Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
+
+ Dev->PciIo->FreeBuffer (
+ Dev->PciIo,
+ Pages,
+ HostAddress
+ );
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+Virtio10MapSharedBuffer (
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN VIRTIO_MAP_OPERATION Operation,
+ IN VOID *HostAddress,
+ IN OUT UINTN *NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ OUT VOID **Mapping
+ )
+{
+ EFI_STATUS Status;
+ VIRTIO_1_0_DEV *Dev;
+ EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation;
+
+ Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
+
+ //
+ // Map VIRTIO_MAP_OPERATION to EFI_PCI_IO_PROTOCOL_OPERATION
+ //
+ if (Operation == EfiVirtIoOperationBusMasterRead) {
+ PciIoOperation = EfiPciIoOperationBusMasterRead;
+ } else if (Operation == EfiVirtIoOperationBusMasterWrite) {
+ PciIoOperation = EfiPciIoOperationBusMasterWrite;
+ } else if (Operation == EfiVirtIoOperationBusMasterCommonBuffer) {
+ PciIoOperation = EfiPciIoOperationBusMasterCommonBuffer;
+ } else {
+ return EFI_UNSUPPORTED;
+ }
+
+ Status = Dev->PciIo->Map (
+ Dev->PciIo,
+ PciIoOperation,
+ HostAddress,
+ NumberOfBytes,
+ DeviceAddress,
+ Mapping
+ );
+ return Status;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+Virtio10UnmapSharedBuffer (
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN VOID *Mapping
+ )
+{
+ EFI_STATUS Status;
+ VIRTIO_1_0_DEV *Dev;
+
+ Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
+
+ Status = Dev->PciIo->Unmap (
+ Dev->PciIo,
+ Mapping
+ );
+
+ return Status;
+}
STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
VIRTIO_SPEC_REVISION (1, 0, 0),
@@ -788,7 +896,11 @@ STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
Virtio10GetDeviceStatus,
Virtio10SetDeviceStatus,
Virtio10WriteDevice,
- Virtio10ReadDevice
+ Virtio10ReadDevice,
+ Virtio10AllocateSharedPages,
+ Virtio10FreeSharedPages,
+ Virtio10MapSharedBuffer,
+ Virtio10UnmapSharedBuffer
};
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 02/14] OvmfPkg/Virtio10Dxe: Implement new member functions
2017-08-07 11:58 ` [PATCH v1 02/14] OvmfPkg/Virtio10Dxe: Implement new member functions Brijesh Singh
@ 2017-08-09 16:50 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 16:50 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
(1) For the subject, I suggest again
OvmfPkg/Virtio10Dxe: Implement IOMMU-like member functions
On 08/07/17 13:58, Brijesh Singh wrote:
> The patch implements the newly added member functions by respectively
> delegating the job to:
>
> - EFI_PCI_IO_PROTOCOL.AllocateBuffer() -- with BootServicesData
> - EFI_PCI_IO_PROTOCOL.FreeBuffer()
> - EFI_PCI_IO_PROTOCOL.Map()
> - EFI_PCI_IO_PROTOCOL.Unmap()
(2) looks good, but please spell out the new virtio protocol member
names as well. (This will become more important in the commit
message of the next patch, where the reader has to guess what member
functions exactly get the "no-op" implementation.)
And then we should be consistent in the commit messages.
>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> 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/Virtio10Dxe/Virtio10.c | 114 +++++++++++++++++++-
> 1 file changed, 113 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
> index a8a6a58c3f1d..5bc8f1c7ee27 100644
> --- a/OvmfPkg/Virtio10Dxe/Virtio10.c
> +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c
> @@ -2,6 +2,7 @@
> A non-transitional driver for VirtIo 1.0 PCI devices.
>
> Copyright (C) 2016, Red Hat, Inc.
> + Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -772,6 +773,113 @@ Virtio10ReadDevice (
> return Status;
> }
>
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10AllocateSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN Pages,
> + IN OUT VOID **HostAddress
> + )
> +{
> + VIRTIO_1_0_DEV *Dev;
> + EFI_STATUS Status;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + Status = Dev->PciIo->AllocateBuffer (
> + Dev->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + Pages,
> + HostAddress,
> + 0
(3) We should pass Attributes=EFI_PCI_ATTRIBUTE_MEMORY_CACHED, even if
only for cosmetic purposes (at the moment).
(4) Independently, this makes me think about
EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE.
* PciIoAllocateBuffer() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c]
sets the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute
internally, before calling out to the PCI Root Bridge IO Protocol,
*if* "PciIoDevice->Attributes" has
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE set.
In turn:
- If there is an IOMMU protocol in the system, then
RootBridgeIoAllocateBuffer()
[MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c] passes
EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE through to the IOMMU
protocol's AllocateBuffer() member *if* "RootBridge->DmaAbove4G"
is set. Otherwise the DUAL_ADDRESS_CYCLE hint is cleared.
(EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE has the same numeric
value, 0x8000, as EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE.)
- If there is no IOMMU protocol in the system, then
RootBridgeIoAllocateBuffer() itself allocates memory, again
obeying EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE and
"RootBridge->DmaAbove4G".
So regardless of whether there is an IOMMU protocol in the system
or not, on the PCI Root Bridge IO level, 64-bit AllocateBuffer()
is be permitted according to device-level DUAL_ADDRESS_CYCLE *AND*
root-bridge-level DmaAbove4G.
* Let's consider what's going to happen if we convert the virtio
drivers to the new virtio proto member functions, and (in
addition) Virtio10Dxe is converted to explicit
PciIo.AllocateBuffer():
- In OvmfPkg, allocations that used to be plain AllocatePool() or
AllocatePages(), will be restricted to 32-bit, due to two
reasons (either of which is sufficient in this regard):
- In "OvmfPkg/Library/PciHostBridgeLib", we clear DmaAbove4G,
for hysterical raisins.
- In Virtio10Dxe, we don't set
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE.
- In ArmVirtPkg, allocations that used to be plain AllocatePool()
or AllocatePages(), will also be restricted to 32-bit. While
"ArmVirtPkg/Library/FdtPciHostBridgeLib" *sets* DmaAbove4G, lack
of EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in Virtio10Dxe alone
is suffices to trigger the restriction.
* Therefore, please extend this patch with the following: in the
Virtio10BindingStart() function [OvmfPkg/Virtio10Dxe/Virtio10.c],
please replace
SetAttributes = 0;
with
SetAttributes = EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
It will not help OvmfPkg, but it will keep the 4GB restriction out
of ArmVirtPkg. (I'm unsure if we can simply flip "DmaAbove4G" to
TRUE in OvmfPkg.)
* (In the past, Ard wrote a number of similar patches for physical
device drivers:
167c3fb45674 MdeModulePkg/EhciDxe: enable 64-bit PCI DMA
4e28ea2c29e0 MdeModulePkg/NvmExpressDxe: enable 64-bit PCI DMA
df0a0e4b6fae MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA
5c1b371a8839 MdeModulePkg/XhciDxe: enable 64-bit PCI DMA
e58a71d9c50b MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA
to devices that support it
4c0b2d25c61c ArmVirtPkg/FdtPciHostBridgeLib: enable 64-bit PCI
DMA
and that's what we should do here, in separate patches, for the
virtio-pci transport drivers.)
(5) Looking more at the PciIo attribute setting in both virtio-PCI
transport drivers, it seems that I failed to set
EFI_PCI_IO_ATTRIBUTE_BUS_MASTER in both. Virtio devices read and
write guest RAM (they don't just decode their IO and/or MMIO BARs),
which translates to "bus master".
So, please clean up my mess :) , and prepend two separate patches to
the series:
- "OvmfPkg/VirtioPciDeviceDxe: supply missing BUS_MASTER attribute"
Please change
EFI_PCI_IO_ATTRIBUTE_IO
to
EFI_PCI_IO_ATTRIBUTE_IO | EFI_PCI_IO_ATTRIBUTE_BUS_MASTER
in VirtioPciDeviceBindingStart()
[OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c].
- "OvmfPkg/Virtio10Dxe: supply missing BUS_MASTER attribute"
Please change
SetAttributes = 0;
to
SetAttributes = EFI_PCI_IO_ATTRIBUTE_BUS_MASTER;
in Virtio10BindingStart() [OvmfPkg/Virtio10Dxe/Virtio10.c].
(And then do point (4) on top of that, separately.)
Okay, summary of points (3), (4) and (5):
- please prepend *two* patches, setting EFI_PCI_IO_ATTRIBUTE_BUS_MASTER
in each virtio-pci transport driver -- point (5)
- please set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in the modern-only
driver, in this patch -- point (4)
- please pass Attributes=EFI_PCI_ATTRIBUTE_MEMORY_CACHED to
Dev->PciIo->AllocateBuffer(), in this patch -- point (3).
Phew, this is complex. :)
> + );
> + return Status;
> +}
> +
> +STATIC
> +VOID
> +EFIAPI
> +Virtio10FreeSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN UINTN Pages,
> + IN VOID *HostAddress
> + )
> +{
> + VIRTIO_1_0_DEV *Dev;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + Dev->PciIo->FreeBuffer (
> + Dev->PciIo,
> + Pages,
> + HostAddress
> + );
> +}
Looks OK.
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10MapSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN VIRTIO_MAP_OPERATION Operation,
> + IN VOID *HostAddress,
> + IN OUT UINTN *NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + )
> +{
> + EFI_STATUS Status;
> + VIRTIO_1_0_DEV *Dev;
> + EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + //
> + // Map VIRTIO_MAP_OPERATION to EFI_PCI_IO_PROTOCOL_OPERATION
> + //
> + if (Operation == EfiVirtIoOperationBusMasterRead) {
> + PciIoOperation = EfiPciIoOperationBusMasterRead;
> + } else if (Operation == EfiVirtIoOperationBusMasterWrite) {
> + PciIoOperation = EfiPciIoOperationBusMasterWrite;
> + } else if (Operation == EfiVirtIoOperationBusMasterCommonBuffer) {
> + PciIoOperation = EfiPciIoOperationBusMasterCommonBuffer;
> + } else {
> + return EFI_UNSUPPORTED;
> + }
(6) This mapping lends itself to simplification by virtue of a switch
statement. (Please remember that the "case" labels are indented
*zero* positions relative to the "switch" keyword.)
(7) Rather than EFI_UNSUPPORTED, EFI_INVALID_PARAMETER should be
returned ("One or more parameters are invalid.")
(8) You don't seem to be using "VirtioOperationMaximum" for range
checking (that's fine, with the switch we can check each value
individually, and then add a "default" case) -- please consider
removing VirtioOperationMaximum from the protocol interface then.
Regarding 64-bit transparency: this code is good, as long as we cover
point (4) above:
- EFI_PCI_IO_PROTOCOL_OPERATION has no separate 32-bit and 64-bit
constants,
- EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION does make such a
distinction,
- PciIoMap() in "MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c" shifts the
former op constants up to the 64-bit latter op constants, if
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE is enabled for the device
(hence my reference to (4)).
The rest also looks good.
Thanks,
Laszlo
> +
> + Status = Dev->PciIo->Map (
> + Dev->PciIo,
> + PciIoOperation,
> + HostAddress,
> + NumberOfBytes,
> + DeviceAddress,
> + Mapping
> + );
> + return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10UnmapSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *This,
> + IN VOID *Mapping
> + )
> +{
> + EFI_STATUS Status;
> + VIRTIO_1_0_DEV *Dev;
> +
> + Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> + Status = Dev->PciIo->Unmap (
> + Dev->PciIo,
> + Mapping
> + );
> +
> + return Status;
> +}
>
> STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
> VIRTIO_SPEC_REVISION (1, 0, 0),
> @@ -788,7 +896,11 @@ STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
> Virtio10GetDeviceStatus,
> Virtio10SetDeviceStatus,
> Virtio10WriteDevice,
> - Virtio10ReadDevice
> + Virtio10ReadDevice,
> + Virtio10AllocateSharedPages,
> + Virtio10FreeSharedPages,
> + Virtio10MapSharedBuffer,
> + Virtio10UnmapSharedBuffer
> };
>
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 02/14] OvmfPkg/Virtio10Dxe: Implement new member functions Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-09 17:09 ` Laszlo Ersek
2017-08-07 11:58 ` [PATCH v1 04/14] OvmfPkg/VirtioLib: Add SharedBuffer helper functions Brijesh Singh
` (12 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The patch implements the newly added member functions by respectively
delegating the job to:
- MemoryApplicationLib.AllocatePages () -- with BootServicesData
- MemoryApplicationLib.FreePages ()
- no-op (host address is same as device DMA address)
- no-op
Suggested-by: Laszlo Ersek <lersek@redhat.com>
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/VirtioPciDeviceDxe/VirtioPciDevice.h | 34 ++++++++++
OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 7 ++-
OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 66 ++++++++++++++++++++
3 files changed, 106 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
index 8f17a16c88f5..da98de123000 100644
--- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
+++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
@@ -3,6 +3,7 @@
Internal definitions for the VirtIo PCI Device driver
Copyright (C) 2013, ARM Ltd
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -156,4 +157,37 @@ VirtioPciSetDeviceStatus (
UINT8 DeviceStatus
);
+EFI_STATUS
+EFIAPI
+VirtioPciAllocateSharedPages (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ UINTN NumPages,
+ VOID **HostAddress
+ );
+
+VOID
+EFIAPI
+VirtioPciFreeSharedPages (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ UINTN NumPages,
+ VOID *HostAddress
+ );
+
+EFI_STATUS
+EFIAPI
+VirtioPciMapSharedBuffer (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ VIRTIO_MAP_OPERATION Operation,
+ VOID *HostAddress,
+ UINTN *NumberOfBytes,
+ EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ VOID **Mapping
+ );
+
+EFI_STATUS
+EFIAPI
+VirtioPciUnmapSharedBuffer (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ VOID *Mapping
+ );
#endif // _VIRTIO_PCI_DEVICE_DXE_H_
diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
index bc4f6fe8bfa3..4e4e21d9a477 100644
--- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
+++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
@@ -5,6 +5,7 @@
Copyright (C) 2012, Red Hat, Inc.
Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
Copyright (C) 2013, ARM Ltd.
+ Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -40,7 +41,11 @@ STATIC VIRTIO_DEVICE_PROTOCOL mDeviceProtocolTemplate = {
VirtioPciGetDeviceStatus, // GetDeviceStatus
VirtioPciSetDeviceStatus, // SetDeviceStatus
VirtioPciDeviceWrite, // WriteDevice
- VirtioPciDeviceRead // ReadDevice
+ VirtioPciDeviceRead, // ReadDevice
+ VirtioPciAllocateSharedPages, // AllocateSharedPages
+ VirtioPciFreeSharedPages, // FreeSharedPages
+ VirtioPciMapSharedBuffer, // MapSharedBuffer
+ VirtioPciUnmapSharedBuffer, // UnmapSharedBuffer
};
/**
diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
index 243aa14c2421..1c587e184311 100644
--- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
+++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
@@ -5,6 +5,7 @@
Copyright (C) 2012, Red Hat, Inc.
Copyright (c) 2012, Intel Corporation. All rights reserved.<BR>
Copyright (C) 2013, ARM Ltd.
+ Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -271,3 +272,68 @@ VirtioPciSetDeviceStatus (
return VirtioPciIoWrite (Dev, VIRTIO_PCI_OFFSET_QUEUE_DEVICE_STATUS,
sizeof (UINT8), DeviceStatus);
}
+
+EFI_STATUS
+EFIAPI
+VirtioPciAllocateSharedPages (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ UINTN NumPages,
+ VOID **HostAddress
+ )
+{
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS PhysicalAddress;
+
+ Status = gBS->AllocatePages (
+ AllocateAnyPages,
+ EfiBootServicesData,
+ NumPages,
+ &PhysicalAddress
+ );
+ if (!EFI_ERROR (Status)) {
+ *HostAddress = (VOID *) (UINTN) PhysicalAddress;
+ }
+
+ return Status;
+}
+
+VOID
+EFIAPI
+VirtioPciFreeSharedPages (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ UINTN NumPages,
+ VOID *HostAddress
+ )
+{
+ gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN)HostAddress, NumPages);
+}
+
+EFI_STATUS
+EFIAPI
+VirtioPciMapSharedBuffer (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ VIRTIO_MAP_OPERATION Operation,
+ VOID *HostAddress,
+ UINTN *NumberOfBytes,
+ EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ VOID **Mapping
+ )
+{
+ EFI_PHYSICAL_ADDRESS PhysicalAddress;
+
+ PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
+
+ *DeviceAddress = PhysicalAddress;
+
+ return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+VirtioPciUnmapSharedBuffer (
+ VIRTIO_DEVICE_PROTOCOL *This,
+ VOID *Mapping
+ )
+{
+ return EFI_SUCCESS;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
2017-08-07 11:58 ` [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: " Brijesh Singh
@ 2017-08-09 17:09 ` Laszlo Ersek
2017-08-10 18:41 ` Brijesh Singh
0 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 17:09 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/07/17 13:58, Brijesh Singh wrote:
> The patch implements the newly added member functions by respectively
> delegating the job to:
>
> - MemoryApplicationLib.AllocatePages () -- with BootServicesData
> - MemoryApplicationLib.FreePages ()
> - no-op (host address is same as device DMA address)
> - no-op
(1) (2) Please see points (1) and (2) in my "PATCH v1 02/14" feedback.
(3) s/Application/Allocation/
(4) the "with BootServicesData" remark is not needed after the
MemoryAllocationLib.AllocatePages() reference. MemoryAllocationLib
encodes the memory type in the API names. So you have AllocatePool /
AllocatePages, AllocateReservedPool / AllocateReservedPages,
AllocateRuntimePool / AllocateRuntimePages, etc.
Please see the lib class header
MdePkg/Include/Library/MemoryAllocationLib.h
>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> 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/VirtioPciDeviceDxe/VirtioPciDevice.h | 34 ++++++++++
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 7 ++-
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 66 ++++++++++++++++++++
> 3 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
> index 8f17a16c88f5..da98de123000 100644
> --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
> @@ -3,6 +3,7 @@
> Internal definitions for the VirtIo PCI Device driver
>
> Copyright (C) 2013, ARM Ltd
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -156,4 +157,37 @@ VirtioPciSetDeviceStatus (
> UINT8 DeviceStatus
> );
>
> +EFI_STATUS
> +EFIAPI
> +VirtioPciAllocateSharedPages (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINTN NumPages,
> + VOID **HostAddress
> + );
> +
> +VOID
> +EFIAPI
> +VirtioPciFreeSharedPages (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINTN NumPages,
> + VOID *HostAddress
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioPciMapSharedBuffer (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + VIRTIO_MAP_OPERATION Operation,
> + VOID *HostAddress,
> + UINTN *NumberOfBytes,
> + EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + VOID **Mapping
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioPciUnmapSharedBuffer (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + VOID *Mapping
> + );
> #endif // _VIRTIO_PCI_DEVICE_DXE_H_
> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
> index bc4f6fe8bfa3..4e4e21d9a477 100644
> --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
> @@ -5,6 +5,7 @@
> Copyright (C) 2012, Red Hat, Inc.
> Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
> Copyright (C) 2013, ARM Ltd.
> + Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -40,7 +41,11 @@ STATIC VIRTIO_DEVICE_PROTOCOL mDeviceProtocolTemplate = {
> VirtioPciGetDeviceStatus, // GetDeviceStatus
> VirtioPciSetDeviceStatus, // SetDeviceStatus
> VirtioPciDeviceWrite, // WriteDevice
> - VirtioPciDeviceRead // ReadDevice
> + VirtioPciDeviceRead, // ReadDevice
> + VirtioPciAllocateSharedPages, // AllocateSharedPages
> + VirtioPciFreeSharedPages, // FreeSharedPages
> + VirtioPciMapSharedBuffer, // MapSharedBuffer
> + VirtioPciUnmapSharedBuffer, // UnmapSharedBuffer
> };
>
> /**
> diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
> index 243aa14c2421..1c587e184311 100644
> --- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
> +++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
> @@ -5,6 +5,7 @@
> Copyright (C) 2012, Red Hat, Inc.
> Copyright (c) 2012, Intel Corporation. All rights reserved.<BR>
> Copyright (C) 2013, ARM Ltd.
> + Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -271,3 +272,68 @@ VirtioPciSetDeviceStatus (
> return VirtioPciIoWrite (Dev, VIRTIO_PCI_OFFSET_QUEUE_DEVICE_STATUS,
> sizeof (UINT8), DeviceStatus);
> }
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioPciAllocateSharedPages (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINTN NumPages,
> + VOID **HostAddress
> + )
> +{
> + EFI_STATUS Status;
> + EFI_PHYSICAL_ADDRESS PhysicalAddress;
> +
> + Status = gBS->AllocatePages (
> + AllocateAnyPages,
> + EfiBootServicesData,
> + NumPages,
> + &PhysicalAddress
> + );
> + if (!EFI_ERROR (Status)) {
> + *HostAddress = (VOID *) (UINTN) PhysicalAddress;
> + }
> +
> + return Status;
> +}
(5) We might want do what we promise in the commit message :) Namely,
use MemoryAllocationLib APIs (for simplicity), rather than boot
services. Such as:
VOID *Buffer;
Buffer = AllocatePages (NumPages);
if (Buffer == NULL) {
return EFI_OUT_OF_RESOURCES;
}
*HostAddress = Buffer;
return EFI_SUCCESS;
> +
> +VOID
> +EFIAPI
> +VirtioPciFreeSharedPages (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + UINTN NumPages,
> + VOID *HostAddress
> + )
> +{
> + gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN)HostAddress, NumPages);
> +}
(6) Similarly:
FreePages (HostAddress, NumPages);
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioPciMapSharedBuffer (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + VIRTIO_MAP_OPERATION Operation,
> + VOID *HostAddress,
> + UINTN *NumberOfBytes,
> + EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + VOID **Mapping
> + )
> +{
> + EFI_PHYSICAL_ADDRESS PhysicalAddress;
> +
> + PhysicalAddress = (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress;
> +
> + *DeviceAddress = PhysicalAddress;
> +
> + return EFI_SUCCESS;
> +}
(7) We could condense it to a single assignment to *DeviceAddress. Up to
you.
(8) Please set *Mapping to NULL -- let's not cause the future caller of
Unmap() to evaluate (for argument passing) a pointer with garbage
contents at that point; that's undefined behavior in itself, at least in
theory.
> +
> +EFI_STATUS
> +EFIAPI
> +VirtioPciUnmapSharedBuffer (
> + VIRTIO_DEVICE_PROTOCOL *This,
> + VOID *Mapping
> + )
> +{
> + return EFI_SUCCESS;
> +}
>
(9) Please refresh the function signatures in both "VirtioPciDevice.h"
and "VirtioPciFunctions.c", from the protocol definition in
"OvmfPkg/Include/Protocol/VirtioDevice.h".
In particular, all the IN and OUT decoration is missing here.
To be continued.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
2017-08-09 17:09 ` Laszlo Ersek
@ 2017-08-10 18:41 ` Brijesh Singh
2017-08-10 19:47 ` Laszlo Ersek
0 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-10 18:41 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel
Hi Laszlo,
On 08/09/2017 12:09 PM, Laszlo Ersek wrote:
[Snip]
>
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +VirtioPciUnmapSharedBuffer (
>> + VIRTIO_DEVICE_PROTOCOL *This,
>> + VOID *Mapping
>> + )
>> +{
>> + return EFI_SUCCESS;
>> +}
>>
>
> (9) Please refresh the function signatures in both "VirtioPciDevice.h"
> and "VirtioPciFunctions.c", from the protocol definition in
> "OvmfPkg/Include/Protocol/VirtioDevice.h".
>
> In particular, all the IN and OUT decoration is missing here.
I see that several functions defined in VirtioPciDevice.h is missing IN and OUT,
do you want me to send a separate patch to fix that too ?
-Brijesh
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
2017-08-10 18:41 ` Brijesh Singh
@ 2017-08-10 19:47 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-10 19:47 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/10/17 20:41, Brijesh Singh wrote:
> Hi Laszlo,
>
> On 08/09/2017 12:09 PM, Laszlo Ersek wrote:
> [Snip]
>
>>
>>> +
>>> +EFI_STATUS
>>> +EFIAPI
>>> +VirtioPciUnmapSharedBuffer (
>>> + VIRTIO_DEVICE_PROTOCOL *This,
>>> + VOID *Mapping
>>> + )
>>> +{
>>> + return EFI_SUCCESS;
>>> +}
>>>
>>
>> (9) Please refresh the function signatures in both "VirtioPciDevice.h"
>> and "VirtioPciFunctions.c", from the protocol definition in
>> "OvmfPkg/Include/Protocol/VirtioDevice.h".
>>
>> In particular, all the IN and OUT decoration is missing here.
>
> I see that several functions defined in VirtioPciDevice.h is missing IN
> and OUT,
Ouch, you are right!
> do you want me to send a separate patch to fix that too ?
That would be very kind of you.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 04/14] OvmfPkg/VirtioLib: Add SharedBuffer helper functions
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (2 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 03/14] OvmfPkg/VirtioPciDeviceDxe: " Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-09 20:30 ` Laszlo Ersek
2017-08-07 11:58 ` [PATCH v1 05/14] OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit() Brijesh Singh
` (11 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
Patch adds convenience helper functions to call VIRTIO_DEVICE_PROTOCOL
AllocateSharedPages, FreeSharedPages, MapSharedBuffer and UnmapSharedBuffer
member functions.
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/Include/Library/VirtioLib.h | 143 +++++++++++++
OvmfPkg/Library/VirtioLib/VirtioLib.c | 220 ++++++++++++++++++++
2 files changed, 363 insertions(+)
diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
index 5badfb32917f..fa82734f1852 100644
--- a/OvmfPkg/Include/Library/VirtioLib.h
+++ b/OvmfPkg/Include/Library/VirtioLib.h
@@ -3,6 +3,7 @@
Declarations of utility functions used by virtio device drivers.
Copyright (C) 2012-2016, Red Hat, Inc.
+ Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -235,4 +236,146 @@ Virtio10WriteFeatures (
IN OUT UINT8 *DeviceStatus
);
+/**
+ Helper function to allocate pages that is suitable for sharing with
+ hypervisor.
+
+ @param[in] VirtIo The target virtio device to use. It must be valid.
+
+ @param[in] Pages The number of pages to allocate.
+
+ @param[out] Buffer A pointer to store the base system memory address of
+ the allocated range.
+
+ return Returns error code from VirtIo->AllocateSharedPages()
+**/
+EFI_STATUS
+EFIAPI
+VirtioAllocateSharedPages (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN UINTN NumPages,
+ OUT VOID **Buffer
+ );
+
+/**
+ Helper function to free pages allocated using VirtioAllocateSharedPages().
+
+ @param[in] VirtIo The target virtio device to use. It must be valid.
+
+ @param[in] Pages The number of allocated pages.
+
+ @param[in] Buffer System memory address allocated from
+ VirtioAllocateSharedPages ().
+**/
+VOID
+EFIAPI
+VirtioFreeSharedPages (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN UINTN NumPages,
+ IN VOID *Buffer
+ );
+
+/**
+ A helper function to map a system memory to a shared bus master memory for
+ read operation from DMA bus master.
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] HostAddress The system memory address to map to shared bus
+ master address.
+
+ @param[in] NumberOfBytes Number of bytes to be mapped.
+
+ @param[out] DeviceAddress The resulting shared map address for the bus
+ master to access the hosts HostAddress.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ return Returns error code from
+ VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioMapSharedBufferRead (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ OUT VOID **Mapping
+ );
+
+/**
+ A helper function to map a system memory to a shared bus master memory for
+ write operation from DMA bus master.
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] HostAddress The system memory address to map to shared bus
+ master address.
+
+ @param[in] NumberOfBytes Number of bytes to be mapped.
+
+ @param[out] DeviceAddress The resulting shared map address for the bus
+ master to access the hosts HostAddress.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ return Returns error code from
+ VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioMapSharedBufferWrite (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ OUT VOID **Mapping
+ );
+
+/**
+ A helper function to map a system memory to a shared bus master memory for
+ common operation from DMA bus master.
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] HostAddress The system memory address to map to shared bus
+ master address.
+
+ @param[in] NumberOfBytes Number of bytes to be mapped.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ return Returns error code from
+ VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioMapSharedBufferCommon (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT VOID **Mapping
+ );
+
+/**
+ A helper function to unmap shared bus master memory mapped using Map().
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] Mapping A mapping value return from Map().
+
+ return Returns error code from
+ VirtIo->UnmapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioUnmapSharedBuffer (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *Mapping
+ );
#endif // _VIRTIO_LIB_H_
diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
index 845f206369a3..f6b464b6cdf8 100644
--- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
+++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
@@ -4,6 +4,7 @@
Copyright (C) 2012-2016, Red Hat, Inc.
Portion of Copyright (C) 2013, ARM Ltd.
+ Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -414,3 +415,222 @@ Virtio10WriteFeatures (
return Status;
}
+
+/**
+ Helper function to allocate pages that is suitable for sharing with
+ hypervisor.
+
+ @param[in] VirtIo The target virtio device to use. It must be valid.
+
+ @param[in] Pages The number of pages to allocate.
+
+ @param[out] Buffer A pointer to store the base system memory address of
+ the allocated range.
+
+ return Returns error code from VirtIo->AllocateSharedPages()
+**/
+EFI_STATUS
+EFIAPI
+VirtioAllocateSharedPages (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN UINTN NumPages,
+ OUT VOID **Buffer
+ )
+{
+ return VirtIo->AllocateSharedPages (VirtIo, NumPages, Buffer);
+}
+
+/**
+ Helper function to free pages allocated using VirtioAllocateSharedPages().
+
+ @param[in] VirtIo The target virtio device to use. It must be valid.
+
+ @param[in] Pages The number of allocated pages.
+
+ @param[in] Buffer System memory address allocated from
+ VirtioAllocateSharedPages ().
+**/
+VOID
+EFIAPI
+VirtioFreeSharedPages (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN UINTN NumPages,
+ IN VOID *Buffer
+ )
+{
+ VirtIo->FreeSharedPages (VirtIo, NumPages, Buffer);
+}
+
+STATIC
+EFI_STATUS
+VirtioMapSharedBuffer (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VIRTIO_MAP_OPERATION Operation,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ OUT VOID **Mapping
+ )
+{
+ EFI_STATUS Status;
+ VOID *MapInfo;
+ UINTN Size;
+ EFI_PHYSICAL_ADDRESS PhysicalAddress;
+
+ Size = NumberOfBytes;
+ Status = VirtIo->MapSharedBuffer (
+ VirtIo,
+ Operation,
+ HostAddress,
+ &Size,
+ &PhysicalAddress,
+ &MapInfo
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (Size < NumberOfBytes) {
+ goto Failed;
+ }
+
+ *Mapping = MapInfo;
+ *DeviceAddress = PhysicalAddress;
+
+ return EFI_SUCCESS;
+Failed:
+ VirtIo->UnmapSharedBuffer (VirtIo, MapInfo);
+ return EFI_OUT_OF_RESOURCES;
+}
+
+/**
+ A helper function to map a system memory to a shared bus master memory for
+ read operation from DMA bus master.
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] HostAddress The system memory address to map to shared bus
+ master address.
+
+ @param[in] NumberOfBytes Number of bytes to be mapped.
+
+ @param[out] DeviceAddress The resulting shared map address for the bus
+ master to access the hosts HostAddress.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ return Returns error code from
+ VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioMapSharedBufferRead (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ OUT VOID **Mapping
+ )
+{
+ return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterRead,
+ HostAddress, NumberOfBytes, DeviceAddress, Mapping);
+}
+
+/**
+ A helper function to map a system memory to a shared bus master memory for
+ write operation from DMA bus master.
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] HostAddress The system memory address to map to shared bus
+ master address.
+
+ @param[in] NumberOfBytes Number of bytes to be mapped.
+
+ @param[out] DeviceAddress The resulting shared map address for the bus
+ master to access the hosts HostAddress.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ return Returns error code from
+ VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioMapSharedBufferWrite (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
+ OUT VOID **Mapping
+ )
+{
+ return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterWrite,
+ HostAddress, NumberOfBytes, DeviceAddress, Mapping);
+}
+
+/**
+ A helper function to map a system memory to a shared bus master memory for
+ common operation from DMA bus master.
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] HostAddress The system memory address to map to shared bus
+ master address.
+
+ @param[in] NumberOfBytes Number of bytes to be mapped.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ return Returns error code from
+ VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioMapSharedBufferCommon (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT VOID **Mapping
+ )
+{
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+
+ Status = VirtioMapSharedBuffer (VirtIo,
+ EfiVirtIoOperationBusMasterCommonBuffer, HostAddress,
+ NumberOfBytes, &DeviceAddress, Mapping);
+
+ //
+ // On Success, lets verify that DeviceAddress same as HostAddress
+ //
+ if (!EFI_ERROR (Status)) {
+ ASSERT (DeviceAddress == (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress);
+ }
+
+ return Status;
+}
+
+/**
+ A helper function to unmap shared bus master memory mapped using Map().
+
+ @param[in] VirtIo The target virtio device to use. It must be
+ valid.
+
+ @param[in] Mapping A mapping value return from Map().
+
+ return Returns error code from
+ VirtIo->UnmapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioUnmapSharedBuffer (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VOID *Mapping
+ )
+{
+ return VirtIo->UnmapSharedBuffer (VirtIo, Mapping);
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 04/14] OvmfPkg/VirtioLib: Add SharedBuffer helper functions
2017-08-07 11:58 ` [PATCH v1 04/14] OvmfPkg/VirtioLib: Add SharedBuffer helper functions Brijesh Singh
@ 2017-08-09 20:30 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 20:30 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/07/17 13:58, Brijesh Singh wrote:
> Patch adds convenience helper functions to call VIRTIO_DEVICE_PROTOCOL
> AllocateSharedPages, FreeSharedPages, MapSharedBuffer and UnmapSharedBuffer
> member functions.
>
> 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/Include/Library/VirtioLib.h | 143 +++++++++++++
> OvmfPkg/Library/VirtioLib/VirtioLib.c | 220 ++++++++++++++++++++
> 2 files changed, 363 insertions(+)
I prefer to add functions to VirtioLib that actually buy us something.
So let me evaluate that for each function:
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 5badfb32917f..fa82734f1852 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -3,6 +3,7 @@
> Declarations of utility functions used by virtio device drivers.
>
> Copyright (C) 2012-2016, Red Hat, Inc.
> + Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -235,4 +236,146 @@ Virtio10WriteFeatures (
> IN OUT UINT8 *DeviceStatus
> );
>
> +/**
> + Helper function to allocate pages that is suitable for sharing with
> + hypervisor.
> +
> + @param[in] VirtIo The target virtio device to use. It must be valid.
> +
> + @param[in] Pages The number of pages to allocate.
> +
> + @param[out] Buffer A pointer to store the base system memory address of
> + the allocated range.
> +
> + return Returns error code from VirtIo->AllocateSharedPages()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioAllocateSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN UINTN NumPages,
> + OUT VOID **Buffer
> + );
> +
> +/**
> + Helper function to free pages allocated using VirtioAllocateSharedPages().
> +
> + @param[in] VirtIo The target virtio device to use. It must be valid.
> +
> + @param[in] Pages The number of allocated pages.
> +
> + @param[in] Buffer System memory address allocated from
> + VirtioAllocateSharedPages ().
> +**/
> +VOID
> +EFIAPI
> +VirtioFreeSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN UINTN NumPages,
> + IN VOID *Buffer
> + );
> +
> +/**
> + A helper function to map a system memory to a shared bus master memory for
> + read operation from DMA bus master.
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] HostAddress The system memory address to map to shared bus
> + master address.
> +
> + @param[in] NumberOfBytes Number of bytes to be mapped.
> +
> + @param[out] DeviceAddress The resulting shared map address for the bus
> + master to access the hosts HostAddress.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
> +
> + return Returns error code from
> + VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferRead (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + );
> +
> +/**
> + A helper function to map a system memory to a shared bus master memory for
> + write operation from DMA bus master.
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] HostAddress The system memory address to map to shared bus
> + master address.
> +
> + @param[in] NumberOfBytes Number of bytes to be mapped.
> +
> + @param[out] DeviceAddress The resulting shared map address for the bus
> + master to access the hosts HostAddress.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
> +
> + return Returns error code from
> + VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferWrite (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + );
> +
> +/**
> + A helper function to map a system memory to a shared bus master memory for
> + common operation from DMA bus master.
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] HostAddress The system memory address to map to shared bus
> + master address.
> +
> + @param[in] NumberOfBytes Number of bytes to be mapped.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
> +
> + return Returns error code from
> + VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferCommon (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT VOID **Mapping
> + );
> +
> +/**
> + A helper function to unmap shared bus master memory mapped using Map().
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] Mapping A mapping value return from Map().
> +
> + return Returns error code from
> + VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioUnmapSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *Mapping
> + );
> #endif // _VIRTIO_LIB_H_
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 845f206369a3..f6b464b6cdf8 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -4,6 +4,7 @@
>
> Copyright (C) 2012-2016, Red Hat, Inc.
> Portion of Copyright (C) 2013, ARM Ltd.
> + Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -414,3 +415,222 @@ Virtio10WriteFeatures (
>
> return Status;
> }
> +
> +/**
> + Helper function to allocate pages that is suitable for sharing with
> + hypervisor.
> +
> + @param[in] VirtIo The target virtio device to use. It must be valid.
> +
> + @param[in] Pages The number of pages to allocate.
> +
> + @param[out] Buffer A pointer to store the base system memory address of
> + the allocated range.
> +
> + return Returns error code from VirtIo->AllocateSharedPages()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioAllocateSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN UINTN NumPages,
> + OUT VOID **Buffer
> + )
> +{
> + return VirtIo->AllocateSharedPages (VirtIo, NumPages, Buffer);
> +}
(1) I think this function buys us nothing. I suggest to drop it.
> +
> +/**
> + Helper function to free pages allocated using VirtioAllocateSharedPages().
> +
> + @param[in] VirtIo The target virtio device to use. It must be valid.
> +
> + @param[in] Pages The number of allocated pages.
> +
> + @param[in] Buffer System memory address allocated from
> + VirtioAllocateSharedPages ().
> +**/
> +VOID
> +EFIAPI
> +VirtioFreeSharedPages (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN UINTN NumPages,
> + IN VOID *Buffer
> + )
> +{
> + VirtIo->FreeSharedPages (VirtIo, NumPages, Buffer);
> +}
(2) Same here.
> +
> +STATIC
> +EFI_STATUS
> +VirtioMapSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VIRTIO_MAP_OPERATION Operation,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + )
(3) Please add a comment block above the function. Basically you can
copy the MapSharedBuffer() documentation, just point out the extra logic
for NumberOfBytes.
In fact, please rename this function to:
VirtioMapAllBytesInSharedBuffer
> +{
> + EFI_STATUS Status;
> + VOID *MapInfo;
> + UINTN Size;
> + EFI_PHYSICAL_ADDRESS PhysicalAddress;
> +
> + Size = NumberOfBytes;
> + Status = VirtIo->MapSharedBuffer (
> + VirtIo,
> + Operation,
> + HostAddress,
> + &Size,
> + &PhysicalAddress,
> + &MapInfo
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (Size < NumberOfBytes) {
> + goto Failed;
> + }
> +
> + *Mapping = MapInfo;
> + *DeviceAddress = PhysicalAddress;
> +
> + return EFI_SUCCESS;
> +Failed:
> + VirtIo->UnmapSharedBuffer (VirtIo, MapInfo);
> + return EFI_OUT_OF_RESOURCES;
> +}
Now, I think this helper function does actually buy us convenience -- it
helps us centralize error handling, and it allows callers to pass in
constants or other evaluated expressions for NumberOfBytes.
I've given quite a bit of thought to our "hardliner" handling of the
case when NumberOfBytes is decreased on output. The PciIo spec says that
in such cases the transfer should be broken up into smaller chunks:
> In all mapping requests the resulting NumberOfBytes actually mapped
> may be less than the requested amount. In this case, the DMA operation
> will have to be broken up into smaller chunks. The Map() function will
> map as much of the DMA operation as it can at one time. The caller may
> have to loop on Map() and Unmap() in order to complete a large DMA
> transfer.
While generally speaking that could be a valid idea, I think it doesn't
apply well to virtio, for the following reasons:
- I don't think that CommonBuffer areas are possible to split up in any
sensible way (e.g., rings),
- Even for unidirectional transfers, the request framing is usually
dictated on the virtio request level (i.e., request framing is usually
matched closely by the individual virtio descriptors that constitute the
descriptor chain that *is* the virtio request). If PciIo (ultimately:
the IOMMU) "suggests" that we split the request into smaller chunks,
that could affect the descriptors / descriptor chaining too, and I
simply don't want to go there.
- Turning a single "outer request" (like EFI_BLOCK_IO_PROTOCOL read or
write request) into a series of "inner virtio requests" is a mess as
well -- what if you get a failure from the hypervisor mid-loop?
IMO, we should forward the outer (higher level) request as transparently
over virtio as possible. If the IOMMU limits that for whatever reason,
then we should fail the request immediately, and propagate the error to
the client of the higher level protocol.
So, IMO, the handling of NumberOfBytes is valid here, and this function
is useful.
(4) Please make this function extern (add it to the header file too).
> +
> +/**
> + A helper function to map a system memory to a shared bus master memory for
> + read operation from DMA bus master.
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] HostAddress The system memory address to map to shared bus
> + master address.
> +
> + @param[in] NumberOfBytes Number of bytes to be mapped.
> +
> + @param[out] DeviceAddress The resulting shared map address for the bus
> + master to access the hosts HostAddress.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
> +
> + return Returns error code from
> + VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferRead (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + )
> +{
> + return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterRead,
> + HostAddress, NumberOfBytes, DeviceAddress, Mapping);
> +}
(5) I don't think this function buys us much. The only parameter you can
save at the call site is Operation, but the rest (together with the
saving of the return code) will require so many characters still that
you'll have to break it to multiple lines anyway. So I suggest to drop
this function.
> +
> +/**
> + A helper function to map a system memory to a shared bus master memory for
> + write operation from DMA bus master.
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] HostAddress The system memory address to map to shared bus
> + master address.
> +
> + @param[in] NumberOfBytes Number of bytes to be mapped.
> +
> + @param[out] DeviceAddress The resulting shared map address for the bus
> + master to access the hosts HostAddress.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
> +
> + return Returns error code from
> + VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferWrite (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + )
> +{
> + return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterWrite,
> + HostAddress, NumberOfBytes, DeviceAddress, Mapping);
> +}
(6) Same as (5) here.
> +
> +/**
> + A helper function to map a system memory to a shared bus master memory for
> + common operation from DMA bus master.
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] HostAddress The system memory address to map to shared bus
> + master address.
> +
> + @param[in] NumberOfBytes Number of bytes to be mapped.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
> +
> + return Returns error code from
> + VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferCommon (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT VOID **Mapping
> + )
> +{
> + EFI_STATUS Status;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> +
> + Status = VirtioMapSharedBuffer (VirtIo,
> + EfiVirtIoOperationBusMasterCommonBuffer, HostAddress,
> + NumberOfBytes, &DeviceAddress, Mapping);
> +
> + //
> + // On Success, lets verify that DeviceAddress same as HostAddress
> + //
> + if (!EFI_ERROR (Status)) {
> + ASSERT (DeviceAddress == (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress);
> + }
> +
> + return Status;
> +}
So this function seems to save us two parameters:
(7) Operation -- I commented on that under (5).
(8) DeviceAddress -- I actually disagree with hiding that. In my
opinion, this is different from the NumberOfBytes question.
We can be inflexible with NumberOfBytes because the in-out NumberOfBytes
parameter of PciIo.Map() reflects an *accidental* trait of some IOMMUs
(namely that their map sizes may be limited). I think that in the virtio
scope we can safely say that we simply don't support IOMMU protocol
instances that can't accommodate our desired request framing.
(Case in point: versions of the virtio spec have presented limits on
virtio request sizes, and thus the virtio drivers already enforce
suitable (stricter) limits on the outer protocol interfaces. Search
"VirtioBlk.c" and "VirtioScsi.c" for SIZE_1GB, for example. Therefore
refusing oversized requests is nothing new, except the definition of
"oversized" might vary, dependent on IOMMU.)
However, DeviceAddress being (potentially) different from HostAddress is
the core idea (an *inherent* trait) of the IOMMU protocol. If we ever
implement a different IOMMU protocol -- e.g. for QEMU's vIOMMU --, I
wouldn't like this assert -- or function prototype -- to cause issues.
So, please drop this helper function, and update all call sites to pass
DeviceAddress to the virtio device.
(I'm aware that this might cause major churn, and more review work for
me, but after all, this is the entire point of the IOMMU abstraction.)
> +
> +/**
> + A helper function to unmap shared bus master memory mapped using Map().
> +
> + @param[in] VirtIo The target virtio device to use. It must be
> + valid.
> +
> + @param[in] Mapping A mapping value return from Map().
> +
> + return Returns error code from
> + VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioUnmapSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VOID *Mapping
> + )
> +{
> + return VirtIo->UnmapSharedBuffer (VirtIo, Mapping);
> +}
>
(9) I think this function also doesn't buy us much, please drop it.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 05/14] OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit()
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (3 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 04/14] OvmfPkg/VirtioLib: Add SharedBuffer helper functions Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-09 21:13 ` Laszlo Ersek
2017-08-07 11:58 ` [PATCH v1 06/14] OvmfPkg/VirtioLib: Add functions to map/unmap VRING Brijesh Singh
` (10 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
Passing the VirtIo protocol instance will allow the vring to use
VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages () to allocate vring buffer.
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/Include/Library/VirtioLib.h | 14 ++++++++++----
OvmfPkg/Library/VirtioLib/VirtioLib.c | 14 ++++++++++----
OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 7 ++++---
OvmfPkg/VirtioGpuDxe/Commands.c | 7 ++++---
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 9 +++++----
OvmfPkg/VirtioNetDxe/SnpShutdown.c | 5 +++--
OvmfPkg/VirtioRngDxe/VirtioRng.c | 7 ++++---
OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 7 ++++---
8 files changed, 44 insertions(+), 26 deletions(-)
diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
index fa82734f1852..610654225de7 100644
--- a/OvmfPkg/Include/Library/VirtioLib.h
+++ b/OvmfPkg/Include/Library/VirtioLib.h
@@ -35,6 +35,8 @@
- 1.1 Virtqueues,
- 2.3 Virtqueue Configuration.
+ @param[in] VirtIo The virtio device which will use the ring.
+
@param[in] The number of descriptors to allocate for the
virtio ring, as requested by the host.
@@ -52,8 +54,9 @@
EFI_STATUS
EFIAPI
VirtioRingInit (
- IN UINT16 QueueSize,
- OUT VRING *Ring
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN UINT16 QueueSize,
+ OUT VRING *Ring
);
@@ -65,13 +68,16 @@ VirtioRingInit (
invoking this function: the VSTAT_DRIVER_OK bit must be clear in
VhdrDeviceStatus.
- @param[out] Ring The virtio ring to clean up.
+ @param[in] VirtIo The virtio device which will was using the ring.
+
+ @param[out] Ring The virtio ring to clean up.
**/
VOID
EFIAPI
VirtioRingUninit (
- IN OUT VRING *Ring
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN OUT VRING *Ring
);
diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
index f6b464b6cdf8..50e5ec28ea1b 100644
--- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
+++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
@@ -37,6 +37,8 @@
- 1.1 Virtqueues,
- 2.3 Virtqueue Configuration.
+ @param[in] VirtIo The virtio device which will use the ring.
+
@param[in] The number of descriptors to allocate for the
virtio ring, as requested by the host.
@@ -54,8 +56,9 @@
EFI_STATUS
EFIAPI
VirtioRingInit (
- IN UINT16 QueueSize,
- OUT VRING *Ring
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN UINT16 QueueSize,
+ OUT VRING *Ring
)
{
UINTN RingSize;
@@ -128,13 +131,16 @@ VirtioRingInit (
invoking this function: the VSTAT_DRIVER_OK bit must be clear in
VhdrDeviceStatus.
- @param[out] Ring The virtio ring to clean up.
+ @param[in] VirtIo The virtio device which will was using the ring.
+
+ @param[out] Ring The virtio ring to clean up.
**/
VOID
EFIAPI
VirtioRingUninit (
- IN OUT VRING *Ring
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN OUT VRING *Ring
)
{
FreePages (Ring->Base, Ring->NumPages);
diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 3ce72281c204..61b9cab4ff02 100644
--- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
+++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
@@ -12,6 +12,7 @@
Copyright (C) 2012, Red Hat, Inc.
Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -722,7 +723,7 @@ VirtioBlkInit (
goto Failed;
}
- Status = VirtioRingInit (QueueSize, &Dev->Ring);
+ Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
if (EFI_ERROR (Status)) {
goto Failed;
}
@@ -811,7 +812,7 @@ VirtioBlkInit (
return EFI_SUCCESS;
ReleaseQueue:
- VirtioRingUninit (&Dev->Ring);
+ VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
Failed:
//
@@ -848,7 +849,7 @@ VirtioBlkUninit (
//
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
- VirtioRingUninit (&Dev->Ring);
+ VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
SetMem (&Dev->BlockIo, sizeof Dev->BlockIo, 0x00);
SetMem (&Dev->BlockIoMedia, sizeof Dev->BlockIoMedia, 0x00);
diff --git a/OvmfPkg/VirtioGpuDxe/Commands.c b/OvmfPkg/VirtioGpuDxe/Commands.c
index 962087cfec97..c2e4d72feb67 100644
--- a/OvmfPkg/VirtioGpuDxe/Commands.c
+++ b/OvmfPkg/VirtioGpuDxe/Commands.c
@@ -3,6 +3,7 @@
VirtIo GPU initialization, and commands (primitives) for the GPU device.
Copyright (C) 2016, Red Hat, Inc.
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -127,7 +128,7 @@ VirtioGpuInit (
//
// [...] population of virtqueues [...]
//
- Status = VirtioRingInit (QueueSize, &VgpuDev->Ring);
+ Status = VirtioRingInit (VgpuDev->VirtIo, QueueSize, &VgpuDev->Ring);
if (EFI_ERROR (Status)) {
goto Failed;
}
@@ -148,7 +149,7 @@ VirtioGpuInit (
return EFI_SUCCESS;
ReleaseQueue:
- VirtioRingUninit (&VgpuDev->Ring);
+ VirtioRingUninit (VgpuDev->VirtIo, &VgpuDev->Ring);
Failed:
//
@@ -183,7 +184,7 @@ VirtioGpuUninit (
// configuration.
//
VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, 0);
- VirtioRingUninit (&VgpuDev->Ring);
+ VirtioRingUninit (VgpuDev->VirtIo, &VgpuDev->Ring);
}
/**
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 430670a980f2..6d9b81a9f939 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -5,6 +5,7 @@
Copyright (C) 2013, Red Hat, Inc.
Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -73,7 +74,7 @@ VirtioNetInitRing (
if (QueueSize < 2) {
return EFI_UNSUPPORTED;
}
- Status = VirtioRingInit (QueueSize, Ring);
+ Status = VirtioRingInit (Dev->VirtIo, QueueSize, Ring);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -103,7 +104,7 @@ VirtioNetInitRing (
return EFI_SUCCESS;
ReleaseQueue:
- VirtioRingUninit (Ring);
+ VirtioRingUninit (Dev->VirtIo, Ring);
return Status;
}
@@ -509,10 +510,10 @@ AbortDevice:
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
ReleaseTxRing:
- VirtioRingUninit (&Dev->TxRing);
+ VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
ReleaseRxRing:
- VirtioRingUninit (&Dev->RxRing);
+ VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
DeviceFailed:
//
diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
index 01409c0ce714..5e84191fbbdd 100644
--- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
+++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
@@ -4,6 +4,7 @@
Copyright (C) 2013, Red Hat, Inc.
Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -66,8 +67,8 @@ VirtioNetShutdown (
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
VirtioNetShutdownRx (Dev);
VirtioNetShutdownTx (Dev);
- VirtioRingUninit (&Dev->TxRing);
- VirtioRingUninit (&Dev->RxRing);
+ VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
+ VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
Dev->Snm.State = EfiSimpleNetworkStarted;
Status = EFI_SUCCESS;
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
index 1a186d04082a..e20602ac7225 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -6,6 +6,7 @@
Copyright (C) 2012, Red Hat, Inc.
Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This driver:
@@ -275,7 +276,7 @@ VirtioRngInit (
goto Failed;
}
- Status = VirtioRingInit (QueueSize, &Dev->Ring);
+ Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
if (EFI_ERROR (Status)) {
goto Failed;
}
@@ -331,7 +332,7 @@ VirtioRngInit (
return EFI_SUCCESS;
ReleaseQueue:
- VirtioRingUninit (&Dev->Ring);
+ VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
Failed:
//
@@ -358,7 +359,7 @@ VirtioRngUninit (
// the old comms area.
//
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
- VirtioRingUninit (&Dev->Ring);
+ VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
}
//
diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index c080404330e5..c2f6f412ff40 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -27,6 +27,7 @@
Copyright (C) 2012, Red Hat, Inc.
Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -832,7 +833,7 @@ VirtioScsiInit (
goto Failed;
}
- Status = VirtioRingInit (QueueSize, &Dev->Ring);
+ Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
if (EFI_ERROR (Status)) {
goto Failed;
}
@@ -926,7 +927,7 @@ VirtioScsiInit (
return EFI_SUCCESS;
ReleaseQueue:
- VirtioRingUninit (&Dev->Ring);
+ VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
Failed:
//
@@ -964,7 +965,7 @@ VirtioScsiUninit (
Dev->MaxLun = 0;
Dev->MaxSectors = 0;
- VirtioRingUninit (&Dev->Ring);
+ VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
SetMem (&Dev->PassThru, sizeof Dev->PassThru, 0x00);
SetMem (&Dev->PassThruMode, sizeof Dev->PassThruMode, 0x00);
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 05/14] OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit()
2017-08-07 11:58 ` [PATCH v1 05/14] OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit() Brijesh Singh
@ 2017-08-09 21:13 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 21:13 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
(1) the subject has a typo ("Uinit()") so I suggest:
OvmfPkg/VirtioLib: take VirtIo instance in VirtioRingInit/VirtioRingUninit
(74 characters).
On 08/07/17 13:58, Brijesh Singh wrote:
> Passing the VirtIo protocol instance will allow the vring to use
> VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages () to allocate vring buffer.
>
> 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/Include/Library/VirtioLib.h | 14 ++++++++++----
> OvmfPkg/Library/VirtioLib/VirtioLib.c | 14 ++++++++++----
> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 7 ++++---
> OvmfPkg/VirtioGpuDxe/Commands.c | 7 ++++---
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 9 +++++----
> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 5 +++--
> OvmfPkg/VirtioRngDxe/VirtioRng.c | 7 ++++---
> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 7 ++++---
> 8 files changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index fa82734f1852..610654225de7 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -35,6 +35,8 @@
> - 1.1 Virtqueues,
> - 2.3 Virtqueue Configuration.
>
> + @param[in] VirtIo The virtio device which will use the ring.
> +
> @param[in] The number of descriptors to allocate for the
> virtio ring, as requested by the host.
>
> @@ -52,8 +54,9 @@
> EFI_STATUS
> EFIAPI
> VirtioRingInit (
> - IN UINT16 QueueSize,
> - OUT VRING *Ring
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN UINT16 QueueSize,
> + OUT VRING *Ring
> );
>
>
> @@ -65,13 +68,16 @@ VirtioRingInit (
> invoking this function: the VSTAT_DRIVER_OK bit must be clear in
> VhdrDeviceStatus.
>
> - @param[out] Ring The virtio ring to clean up.
> + @param[in] VirtIo The virtio device which will was using the ring.
(2) s/will was/was/
> +
> + @param[out] Ring The virtio ring to clean up.
>
> **/
> VOID
> EFIAPI
> VirtioRingUninit (
> - IN OUT VRING *Ring
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN OUT VRING *Ring
> );
>
>
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index f6b464b6cdf8..50e5ec28ea1b 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -37,6 +37,8 @@
> - 1.1 Virtqueues,
> - 2.3 Virtqueue Configuration.
>
> + @param[in] VirtIo The virtio device which will use the ring.
> +
> @param[in] The number of descriptors to allocate for the
> virtio ring, as requested by the host.
>
> @@ -54,8 +56,9 @@
> EFI_STATUS
> EFIAPI
> VirtioRingInit (
> - IN UINT16 QueueSize,
> - OUT VRING *Ring
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN UINT16 QueueSize,
> + OUT VRING *Ring
> )
> {
> UINTN RingSize;
> @@ -128,13 +131,16 @@ VirtioRingInit (
> invoking this function: the VSTAT_DRIVER_OK bit must be clear in
> VhdrDeviceStatus.
>
> - @param[out] Ring The virtio ring to clean up.
> + @param[in] VirtIo The virtio device which will was using the ring.
(3) Same as (2).
With these fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> +
> + @param[out] Ring The virtio ring to clean up.
>
> **/
> VOID
> EFIAPI
> VirtioRingUninit (
> - IN OUT VRING *Ring
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN OUT VRING *Ring
> )
> {
> FreePages (Ring->Base, Ring->NumPages);
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 3ce72281c204..61b9cab4ff02 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -12,6 +12,7 @@
>
> Copyright (C) 2012, Red Hat, Inc.
> Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -722,7 +723,7 @@ VirtioBlkInit (
> goto Failed;
> }
>
> - Status = VirtioRingInit (QueueSize, &Dev->Ring);
> + Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -811,7 +812,7 @@ VirtioBlkInit (
> return EFI_SUCCESS;
>
> ReleaseQueue:
> - VirtioRingUninit (&Dev->Ring);
> + VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>
> Failed:
> //
> @@ -848,7 +849,7 @@ VirtioBlkUninit (
> //
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> - VirtioRingUninit (&Dev->Ring);
> + VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>
> SetMem (&Dev->BlockIo, sizeof Dev->BlockIo, 0x00);
> SetMem (&Dev->BlockIoMedia, sizeof Dev->BlockIoMedia, 0x00);
> diff --git a/OvmfPkg/VirtioGpuDxe/Commands.c b/OvmfPkg/VirtioGpuDxe/Commands.c
> index 962087cfec97..c2e4d72feb67 100644
> --- a/OvmfPkg/VirtioGpuDxe/Commands.c
> +++ b/OvmfPkg/VirtioGpuDxe/Commands.c
> @@ -3,6 +3,7 @@
> VirtIo GPU initialization, and commands (primitives) for the GPU device.
>
> Copyright (C) 2016, Red Hat, Inc.
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -127,7 +128,7 @@ VirtioGpuInit (
> //
> // [...] population of virtqueues [...]
> //
> - Status = VirtioRingInit (QueueSize, &VgpuDev->Ring);
> + Status = VirtioRingInit (VgpuDev->VirtIo, QueueSize, &VgpuDev->Ring);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -148,7 +149,7 @@ VirtioGpuInit (
> return EFI_SUCCESS;
>
> ReleaseQueue:
> - VirtioRingUninit (&VgpuDev->Ring);
> + VirtioRingUninit (VgpuDev->VirtIo, &VgpuDev->Ring);
>
> Failed:
> //
> @@ -183,7 +184,7 @@ VirtioGpuUninit (
> // configuration.
> //
> VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, 0);
> - VirtioRingUninit (&VgpuDev->Ring);
> + VirtioRingUninit (VgpuDev->VirtIo, &VgpuDev->Ring);
> }
>
> /**
> diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> index 430670a980f2..6d9b81a9f939 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
> @@ -5,6 +5,7 @@
>
> Copyright (C) 2013, Red Hat, Inc.
> Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -73,7 +74,7 @@ VirtioNetInitRing (
> if (QueueSize < 2) {
> return EFI_UNSUPPORTED;
> }
> - Status = VirtioRingInit (QueueSize, Ring);
> + Status = VirtioRingInit (Dev->VirtIo, QueueSize, Ring);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> @@ -103,7 +104,7 @@ VirtioNetInitRing (
> return EFI_SUCCESS;
>
> ReleaseQueue:
> - VirtioRingUninit (Ring);
> + VirtioRingUninit (Dev->VirtIo, Ring);
>
> return Status;
> }
> @@ -509,10 +510,10 @@ AbortDevice:
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>
> ReleaseTxRing:
> - VirtioRingUninit (&Dev->TxRing);
> + VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
>
> ReleaseRxRing:
> - VirtioRingUninit (&Dev->RxRing);
> + VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
>
> DeviceFailed:
> //
> diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> index 01409c0ce714..5e84191fbbdd 100644
> --- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> +++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
> @@ -4,6 +4,7 @@
>
> Copyright (C) 2013, Red Hat, Inc.
> Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -66,8 +67,8 @@ VirtioNetShutdown (
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> VirtioNetShutdownRx (Dev);
> VirtioNetShutdownTx (Dev);
> - VirtioRingUninit (&Dev->TxRing);
> - VirtioRingUninit (&Dev->RxRing);
> + VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
> + VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
>
> Dev->Snm.State = EfiSimpleNetworkStarted;
> Status = EFI_SUCCESS;
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> index 1a186d04082a..e20602ac7225 100644
> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -6,6 +6,7 @@
>
> Copyright (C) 2012, Red Hat, Inc.
> Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This driver:
>
> @@ -275,7 +276,7 @@ VirtioRngInit (
> goto Failed;
> }
>
> - Status = VirtioRingInit (QueueSize, &Dev->Ring);
> + Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -331,7 +332,7 @@ VirtioRngInit (
> return EFI_SUCCESS;
>
> ReleaseQueue:
> - VirtioRingUninit (&Dev->Ring);
> + VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>
> Failed:
> //
> @@ -358,7 +359,7 @@ VirtioRngUninit (
> // the old comms area.
> //
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> - VirtioRingUninit (&Dev->Ring);
> + VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
> }
>
> //
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index c080404330e5..c2f6f412ff40 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -27,6 +27,7 @@
>
> Copyright (C) 2012, Red Hat, Inc.
> Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -832,7 +833,7 @@ VirtioScsiInit (
> goto Failed;
> }
>
> - Status = VirtioRingInit (QueueSize, &Dev->Ring);
> + Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
> if (EFI_ERROR (Status)) {
> goto Failed;
> }
> @@ -926,7 +927,7 @@ VirtioScsiInit (
> return EFI_SUCCESS;
>
> ReleaseQueue:
> - VirtioRingUninit (&Dev->Ring);
> + VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>
> Failed:
> //
> @@ -964,7 +965,7 @@ VirtioScsiUninit (
> Dev->MaxLun = 0;
> Dev->MaxSectors = 0;
>
> - VirtioRingUninit (&Dev->Ring);
> + VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>
> SetMem (&Dev->PassThru, sizeof Dev->PassThru, 0x00);
> SetMem (&Dev->PassThruMode, sizeof Dev->PassThruMode, 0x00);
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 06/14] OvmfPkg/VirtioLib: Add functions to map/unmap VRING
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (4 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 05/14] OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit() Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-09 23:51 ` Laszlo Ersek
2017-08-07 11:58 ` [PATCH v1 07/14] OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer Brijesh Singh
` (9 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
Add functions to map and unmap the ring buffer with BusMasterCommonBuffer.
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/Include/Library/VirtioLib.h | 41 +++++++++++++++
OvmfPkg/Library/VirtioLib/VirtioLib.c | 52 ++++++++++++++++++++
2 files changed, 93 insertions(+)
diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
index 610654225de7..877961af0514 100644
--- a/OvmfPkg/Include/Library/VirtioLib.h
+++ b/OvmfPkg/Include/Library/VirtioLib.h
@@ -62,6 +62,47 @@ VirtioRingInit (
/**
+ Map the ring buffer so that it can be accessed equally by both guest
+ and hypervisor.
+
+ @param[in] VirtIo The virtio device instance.
+
+ @param[in] Ring The virtio ring to map.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ @retval Value returned from VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioRingMap (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VRING *Ring,
+ OUT VOID **Mapping
+ );
+
+/**
+
+ Unmap the ring buffer mapped using VirtioRingMap()
+
+ @param[in] VirtIo The virtio device instance.
+
+ @param[in] Ring The virtio ring to unmap.
+
+ @param[in] Mapping A value obtained through Map().
+
+ @retval Value returned from VirtIo->UnmapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioRingUnmap (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VRING *Ring,
+ IN VOID *Mapping
+ );
+
+/**
+
Tear down the internal resources of a configured virtio ring.
The caller is responsible to stop the host from using this ring before
diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
index 50e5ec28ea1b..09a3f9b7f2e5 100644
--- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
+++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
@@ -640,3 +640,55 @@ VirtioUnmapSharedBuffer (
{
return VirtIo->UnmapSharedBuffer (VirtIo, Mapping);
}
+
+/**
+
+ Map the ring buffer so that it can be accessed equally by both guest
+ and hypervisor.
+
+ @param[in] VirtIo The virtio device instance.
+
+ @param[in] Ring The virtio ring to map.
+
+ @param[out] Mapping A resulting value to pass to Unmap().
+
+ @retval Value returned from VirtIo->MapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioRingMap (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VRING *Ring,
+ OUT VOID **Mapping
+ )
+{
+ UINTN NumberOfBytes;
+
+ NumberOfBytes = Ring->NumPages * EFI_PAGE_SIZE;
+
+ return VirtioMapSharedBufferCommon (VirtIo, Ring->Base,
+ NumberOfBytes, Mapping);
+}
+
+/**
+
+ Unmap the ring buffer mapped using VirtioRingMap()
+
+ @param[in] VirtIo The virtio device instance.
+
+ @param[in] Ring The virtio ring to unmap.
+
+ @param[in] Mapping A value obtained through Map().
+
+ @retval Value returned from VirtIo->UnmapSharedBuffer()
+**/
+EFI_STATUS
+EFIAPI
+VirtioRingUnmap (
+ IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
+ IN VRING *Ring,
+ IN VOID *Mapping
+ )
+{
+ return VirtioUnmapSharedBuffer (VirtIo, Mapping);
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 06/14] OvmfPkg/VirtioLib: Add functions to map/unmap VRING
2017-08-07 11:58 ` [PATCH v1 06/14] OvmfPkg/VirtioLib: Add functions to map/unmap VRING Brijesh Singh
@ 2017-08-09 23:51 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 23:51 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
First some superficial comments, then some more serious ones.
On 08/07/17 13:58, Brijesh Singh wrote:
> Add functions to map and unmap the ring buffer with BusMasterCommonBuffer.
>
> 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/Include/Library/VirtioLib.h | 41 +++++++++++++++
> OvmfPkg/Library/VirtioLib/VirtioLib.c | 52 ++++++++++++++++++++
> 2 files changed, 93 insertions(+)
>
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 610654225de7..877961af0514 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -62,6 +62,47 @@ VirtioRingInit (
>
> /**
>
> + Map the ring buffer so that it can be accessed equally by both guest
> + and hypervisor.
> +
> + @param[in] VirtIo The virtio device instance.
> +
> + @param[in] Ring The virtio ring to map.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
(1) Please say "token" here (to be consistent with my earlier request).
(2) Please replace Unmap() with VirtIo->UnmapSharedBuffer().
> +
> + @retval Value returned from VirtIo->MapSharedBuffer()
(3) This should be @return, not @retval. @retval is for concrete
constants. Also I would suggest "Status code" rather than "Value".
(4) Please sync the comment block in the C file.
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingMap (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VRING *Ring,
> + OUT VOID **Mapping
> + );
> +
> +/**
> +
> + Unmap the ring buffer mapped using VirtioRingMap()
> +
> + @param[in] VirtIo The virtio device instance.
> +
> + @param[in] Ring The virtio ring to unmap.
> +
> + @param[in] Mapping A value obtained through Map().
> +
> + @retval Value returned from VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingUnmap (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VRING *Ring,
> + IN VOID *Mapping
> + );
(5) Please drop this function. It doesn't save us anything. (The Ring
parameter isn't even used in the implementation.)
> +
> +/**
> +
> Tear down the internal resources of a configured virtio ring.
>
> The caller is responsible to stop the host from using this ring before
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 50e5ec28ea1b..09a3f9b7f2e5 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -640,3 +640,55 @@ VirtioUnmapSharedBuffer (
> {
> return VirtIo->UnmapSharedBuffer (VirtIo, Mapping);
> }
> +
> +/**
> +
> + Map the ring buffer so that it can be accessed equally by both guest
> + and hypervisor.
> +
> + @param[in] VirtIo The virtio device instance.
> +
> + @param[in] Ring The virtio ring to map.
> +
> + @param[out] Mapping A resulting value to pass to Unmap().
> +
> + @retval Value returned from VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingMap (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VRING *Ring,
> + OUT VOID **Mapping
> + )
> +{
> + UINTN NumberOfBytes;
> +
> + NumberOfBytes = Ring->NumPages * EFI_PAGE_SIZE;
(6) "EFI_PAGES_TO_SIZE (Ring->NumPages)" is more idiomatic.
> +
> + return VirtioMapSharedBufferCommon (VirtIo, Ring->Base,
> + NumberOfBytes, Mapping);
(7) So, based on my earlier request, this should become a call to
VirtioMapAllBytesInSharedBuffer(). Please break every argument to a
separate line.
Now, my important comments for this patch. :)
VirtioMapAllBytesInSharedBuffer() will give you a DeviceAddress. Here's
what we should do with it:
(8) Please introduce a new patch that modifies the
VIRTIO_SET_QUEUE_ADDRESS typedef, in
"OvmfPkg/Include/Protocol/VirtioDevice.h". Namely, please add a third
parameter:
IN UINT64 RingBaseShift
In this initial patch, simply update all call sites to pass in 0.
In parallel (in the same initial patch), update all three implementations:
- VirtioMmioSetQueueAddress()
[OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c]
- VirtioPciSetQueueAddress()
[OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c]
- Virtio10SetQueueAddress() [OvmfPkg/Virtio10Dxe/Virtio10.c]
as follows:
- accept the new parameter (obviously),
- simply assert that it is zero, at the top of the function.
(9) Modify Virtio10SetQueueAddress() as follows, in another separate patch:
- every time after the local variable "Address" is assigned, please insert:
Address += RingBaseShift;
This is automatically going to happen in UINT64 arithmetic, which has
well-defined wrap-around semantics. Three such insertions will be necessary.
- remove the ASSERT added in (8)
(10) Then please include the present patch, with the following update
(beyond the changes requested above):
- Add the following output parameter to VirtioRingMap():
OUT UINT64 *RingBaseShift
- In the implementation, assign it like this, if
VirtioMapAllBytesInSharedBuffer() succeeds:
*RingBaseShift = DeviceAddress - (UINT64)(UINTN)Ring->Base;
(DeviceAddress has type EFI_PHYSICAL_ADDRESS, which is equivalent to
UINT64, on the spec level.)
(11) Please make sure that no output parameter is modified before we can
guarantee returning EFI_SUCCESS.
(12) In the individual virtio device drivers, whenever you insert the
VirtioRingMap() function call, carry "RingBaseShift" from
VirtioRingMap() to VirtIo->SetQueueAddress() in a new local UINT64 variable.
End result:
- the MMIO and legacy PCI transport implementations will always set
DeviceAddress to HostAddress in their no-op MapSharedBuffer() member
functions,
- therefore VirtioRingMap() will set RingBaseShift to zero,
- the ASSERT()s in the SetQueueAddress() members of those same transport
implementations will be satisfied,
- with the modern-only (1.0) transport, RingBaseShift will also be zero
(because of the SEV IOMMU that we have), and "Address += 0" will have no
effect,
- if we ever add an IOMMU driver that translates addresses, the
translation will happen automatically in the modern-only transport.
Thanks,
Laszlo
> +}
> +
> +/**
> +
> + Unmap the ring buffer mapped using VirtioRingMap()
> +
> + @param[in] VirtIo The virtio device instance.
> +
> + @param[in] Ring The virtio ring to unmap.
> +
> + @param[in] Mapping A value obtained through Map().
> +
> + @retval Value returned from VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingUnmap (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VRING *Ring,
> + IN VOID *Mapping
> + )
> +{
> + return VirtioUnmapSharedBuffer (VirtIo, Mapping);
> +}
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 07/14] OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (5 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 06/14] OvmfPkg/VirtioLib: Add functions to map/unmap VRING Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-10 0:02 ` Laszlo Ersek
2017-08-07 11:58 ` [PATCH v1 08/14] OvmfPkg/VirtioBlkDxe: Use DeviceAddresses in vring descriptors Brijesh Singh
` (8 subsequent siblings)
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The vring buffer is communication block between guest and hypervisor,
allocate the vring buffer using AllocateSharedPages() so that it can be
Map() with BusMasterCommonBufer for bi-directional access.
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/Library/VirtioLib/VirtioLib.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
index 09a3f9b7f2e5..9ba64204326f 100644
--- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
+++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
@@ -61,6 +61,7 @@ VirtioRingInit (
OUT VRING *Ring
)
{
+ EFI_STATUS Status;
UINTN RingSize;
volatile UINT8 *RingPagesPtr;
@@ -79,9 +80,12 @@ VirtioRingInit (
sizeof *Ring->Used.AvailEvent,
EFI_PAGE_SIZE);
+ //
+ // Allocate a shared ring buffer
+ //
Ring->NumPages = EFI_SIZE_TO_PAGES (RingSize);
- Ring->Base = AllocatePages (Ring->NumPages);
- if (Ring->Base == NULL) {
+ Status = VirtioAllocateSharedPages (VirtIo, Ring->NumPages, &Ring->Base);
+ if (EFI_ERROR (Status)) {
return EFI_OUT_OF_RESOURCES;
}
SetMem (Ring->Base, RingSize, 0x00);
@@ -143,7 +147,7 @@ VirtioRingUninit (
IN OUT VRING *Ring
)
{
- FreePages (Ring->Base, Ring->NumPages);
+ VirtioFreeSharedPages (VirtIo, Ring->NumPages, Ring->Base);
SetMem (Ring, sizeof *Ring, 0x00);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 07/14] OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer
2017-08-07 11:58 ` [PATCH v1 07/14] OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer Brijesh Singh
@ 2017-08-10 0:02 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-10 0:02 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/07/17 13:58, Brijesh Singh wrote:
> The vring buffer is communication block between guest and hypervisor,
> allocate the vring buffer using AllocateSharedPages() so that it can be
> Map() with BusMasterCommonBufer for bi-directional access.
>
> 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/Library/VirtioLib/VirtioLib.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 09a3f9b7f2e5..9ba64204326f 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -61,6 +61,7 @@ VirtioRingInit (
> OUT VRING *Ring
> )
> {
> + EFI_STATUS Status;
> UINTN RingSize;
> volatile UINT8 *RingPagesPtr;
>
> @@ -79,9 +80,12 @@ VirtioRingInit (
> sizeof *Ring->Used.AvailEvent,
> EFI_PAGE_SIZE);
>
> + //
> + // Allocate a shared ring buffer
> + //
> Ring->NumPages = EFI_SIZE_TO_PAGES (RingSize);
> - Ring->Base = AllocatePages (Ring->NumPages);
> - if (Ring->Base == NULL) {
> + Status = VirtioAllocateSharedPages (VirtIo, Ring->NumPages, &Ring->Base);
> + if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
> SetMem (Ring->Base, RingSize, 0x00);
> @@ -143,7 +147,7 @@ VirtioRingUninit (
> IN OUT VRING *Ring
> )
> {
> - FreePages (Ring->Base, Ring->NumPages);
> + VirtioFreeSharedPages (VirtIo, Ring->NumPages, Ring->Base);
> SetMem (Ring, sizeof *Ring, 0x00);
> }
>
>
The logic looks good, I have some superficial comments:
(1) Please update the subject, the commit message, and the code to refer
to VirtIo->AllocateSharedPages() (or even VIRTIO_ALLOCATE_SHARED, as you
see fit -- just not the helper function, because we won't have that).
(2) Please update the documentation of "@retval EFI_OUT_OF_RESOURCES" --
replace it with
@return Status codes propagated from VirtIo->AllocateSharedPages().
(3) Accordingly, please forward the Status received, on the error branch.
(4) Please remove MemoryAllocationLib from the #includes and also from
[LibraryClasses].
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v1 08/14] OvmfPkg/VirtioBlkDxe: Use DeviceAddresses in vring descriptors
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (6 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 07/14] OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 09/14] OvmfPkg/VirtioScsiDxe: " Brijesh Singh
` (7 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The SynchronousRequest(), programs the vring descriptor with the buffers
pointed-by virtio-blk requests, status and memory that is referenced
inside the request header.
The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
function to map system memory to device address and programs the vring
descriptor with device addresses.
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/VirtioBlkDxe/VirtioBlk.h | 1 +
OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 97 ++++++++++++++++++--
2 files changed, 91 insertions(+), 7 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;
#define VIRTIO_BLK_FROM_BLOCK_IO(BlockIoPointer) \
diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 61b9cab4ff02..324403543f24 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,24 @@ SynchronousRequest (
ASSERT (Dev->Ring.QueueSize >= 3);
//
+ // Map virtio-blk request header
+ //
+ Status = VirtioMapSharedBufferRead (Dev->VirtIo, (VOID *) &Request,
+ sizeof Request, &DeviceAddress, &RequestMapping);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
// 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);
//
// data buffer for read/write in second desc
//
+ BufferMapping = NULL;
if (BufferSize > 0) {
//
// From virtio-0.9.5, 2.3.2 Descriptor Table:
@@ -309,29 +324,70 @@ SynchronousRequest (
ASSERT (BufferSize <= SIZE_1GB);
//
+ // Map data buffer
+ //
+ if (RequestIsWrite) {
+ Status = VirtioMapSharedBufferRead (Dev->VirtIo, (VOID *) Buffer,
+ BufferSize, &DeviceAddress, &BufferMapping);
+ } else {
+ Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *) Buffer,
+ BufferSize, &DeviceAddress, &BufferMapping);
+ }
+
+ if (EFI_ERROR (Status)) {
+ goto Mapping_Failed;
+ }
+
+ //
// 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);
}
//
+ // Map virtio-blk status header
+ //
+ Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *) &HostStatus,
+ sizeof HostStatus, &DeviceAddress, &StatusMapping);
+ if (EFI_ERROR (Status)) {
+ goto Mapping_Failed;
+ }
+
+ //
// 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);
//
// 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;
+ }
+
+ //
+ // Unmap the HostStatus buffer before accessing it
+ //
+ VirtioUnmapSharedBuffer (Dev->VirtIo, StatusMapping);
+
+ 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;
+Mapping_Failed:
+ VirtioUnmapSharedBuffer (Dev->VirtIo, RequestMapping);
+
+ if (BufferMapping != NULL) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, BufferMapping);
+ }
+
+ return Status;
}
@@ -728,6 +784,11 @@ VirtioBlkInit (
goto Failed;
}
+ Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingMapping);
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
//
// Additional steps for MMIO: align the queue appropriately, and set the
// size. If anything fails from here on, we must release the ring resources.
@@ -812,6 +873,11 @@ VirtioBlkInit (
return EFI_SUCCESS;
ReleaseQueue:
+ if (Dev->RingMapping != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMapping);
+ Dev->RingMapping = NULL;
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
Failed:
@@ -849,6 +915,14 @@ VirtioBlkUninit (
//
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+ //
+ // If ring buffer is mapped then Umap() it before uninitializing it.
+ //
+ if (Dev->RingMapping != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMapping);
+ Dev->RingMapping = NULL;
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
SetMem (&Dev->BlockIo, sizeof Dev->BlockIo, 0x00);
@@ -885,6 +959,15 @@ 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.
+ //
+ if (Dev->RingMapping != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMapping);
+ Dev->RingMapping = NULL;
+ }
}
/**
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 09/14] OvmfPkg/VirtioScsiDxe: Use DeviceAddresses in vring descriptors
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (7 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 08/14] OvmfPkg/VirtioBlkDxe: Use DeviceAddresses in vring descriptors Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 10/14] OvmfPkg/VirtioNetDxe: Allocate Tx and Rx ring using AllocateSharedPage() Brijesh Singh
` (6 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The VirtioScsiPassThru(), programs the vring descriptor using the host
addresses pointed-by virtio-scsi request, response and memory that is
referenced inside the request and response header.
The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
function to map system memory to device address and programs the vring
descriptors with device addresses.
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.h | 1 +
OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 114 ++++++++++++++++++--
2 files changed, 107 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
index 6d00567e8cb8..bb1c5c70ef74 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
@@ -60,6 +60,7 @@ typedef struct {
VRING Ring; // VirtioRingInit 2
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; // VirtioScsiInit 1
EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; // VirtioScsiInit 1
+ VOID *RingBufMapping;// VirtioScsiInit 1
} VSCSI_DEV;
#define VIRTIO_SCSI_FROM_PASS_THRU(PassThruPointer) \
diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index c2f6f412ff40..779104567694 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -409,8 +409,14 @@ VirtioScsiPassThru (
UINT16 TargetValue;
EFI_STATUS Status;
volatile VIRTIO_SCSI_REQ Request;
- volatile VIRTIO_SCSI_RESP Response;
+ VIRTIO_SCSI_RESP *Response;
DESC_INDICES Indices;
+ VOID *RequestMapping;
+ VOID *ResponseMapping;
+ VOID *InDataMapping;
+ VOID *OutDataMapping;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ UINTN NumPages;
ZeroMem ((VOID*) &Request, sizeof (Request));
ZeroMem ((VOID*) &Response, sizeof (Response));
@@ -418,9 +424,33 @@ VirtioScsiPassThru (
Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
CopyMem (&TargetValue, Target, sizeof TargetValue);
+ Response = NULL;
+ ResponseMapping = NULL;
+ RequestMapping = NULL;
+ InDataMapping = NULL;
+ OutDataMapping = NULL;
+
+ //
+ // 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.
+ //
+ NumPages = EFI_SIZE_TO_PAGES (sizeof (*Response));
+ Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages,
+ (VOID *) &Response);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = VirtioMapSharedBufferCommon (Dev->VirtIo, (VOID *) Response,
+ sizeof (*Response), &ResponseMapping);
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Failed;
}
VirtioPrepare (&Dev->Ring, &Indices);
@@ -428,7 +458,7 @@ VirtioScsiPassThru (
//
// 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;
//
// ensured by VirtioScsiInit() -- this predicate, in combination with the
@@ -437,23 +467,37 @@ VirtioScsiPassThru (
ASSERT (Dev->Ring.QueueSize >= 4);
//
+ // Map the scsi-blk request header HostAddress to DeviceAddress
+ //
+ Status = VirtioMapSharedBufferRead (Dev->VirtIo, (VOID *) &Request,
+ sizeof Request, &DeviceAddress, &RequestMapping);
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
+ //
// enqueue Request
//
- VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
+ VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, sizeof Request,
VRING_DESC_F_NEXT, &Indices);
//
// enqueue "dataout" if any
//
if (Packet->OutTransferLength > 0) {
- VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
+ Status = VirtioMapSharedBufferRead (Dev->VirtIo, Packet->OutDataBuffer,
+ Packet->OutTransferLength, &DeviceAddress, &OutDataMapping);
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+ VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress,
Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
}
//
// enqueue Response, to be written by the host
//
- VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
+ VirtioAppendDesc (&Dev->Ring, (UINTN) Response, sizeof *Response,
VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
VRING_DESC_F_NEXT : 0),
&Indices);
@@ -462,7 +506,13 @@ VirtioScsiPassThru (
// enqueue "datain" if any, to be written by the host
//
if (Packet->InTransferLength > 0) {
- VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
+ Status = VirtioMapSharedBufferWrite (Dev->VirtIo, Packet->InDataBuffer,
+ Packet->InTransferLength, &DeviceAddress, &InDataMapping);
+ if (EFI_ERROR (Status)) {
+ goto Failed;
+ }
+
+ VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress,
Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
}
@@ -480,7 +530,28 @@ VirtioScsiPassThru (
return EFI_DEVICE_ERROR;
}
- return ParseResponse (Packet, &Response);
+ Status = ParseResponse (Packet, Response);
+
+Failed:
+ if (RequestMapping) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, RequestMapping);
+ }
+
+ if (InDataMapping) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, InDataMapping);
+ }
+
+ if (OutDataMapping) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
+ }
+
+ if (ResponseMapping) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
+ }
+
+ VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *) Response);
+
+ return Status;
}
@@ -838,6 +909,11 @@ VirtioScsiInit (
goto Failed;
}
+ Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingBufMapping);
+ 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.
@@ -927,6 +1003,11 @@ VirtioScsiInit (
return EFI_SUCCESS;
ReleaseQueue:
+ if (Dev->RingBufMapping != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, &Dev->RingBufMapping);
+ Dev->RingBufMapping = NULL;
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
Failed:
@@ -965,6 +1046,14 @@ VirtioScsiUninit (
Dev->MaxLun = 0;
Dev->MaxSectors = 0;
+ //
+ // If Ring buffer is mapped then unmap it.
+ //
+ if (Dev->RingBufMapping != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, &Dev->RingBufMapping);
+ Dev->RingBufMapping = NULL;
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
SetMem (&Dev->PassThru, sizeof Dev->PassThru, 0x00);
@@ -995,6 +1084,15 @@ VirtioScsiExitBoot (
//
Dev = Context;
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ //
+ // If Ring buffer mapping exist then unmap it so that hypervisor can
+ // not get readable data after device reset.
+ //
+ if (Dev->RingBufMapping != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingBufMapping);
+ Dev->RingBufMapping = NULL;
+ }
}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 10/14] OvmfPkg/VirtioNetDxe: Allocate Tx and Rx ring using AllocateSharedPage()
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (8 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 09/14] OvmfPkg/VirtioScsiDxe: " Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 11/14] OvmfPkg/VirtioNetDxe: Allocate RxBuf using AllocateSharedPages() Brijesh Singh
` (5 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The Tx and Rx rings are accessed by both guest and hypervisor, allocate
the rings using newly added VirtIo->AllocateSharedPages() and map it with
BusMasterCommonBuffer so that it can be accessed by both guest and
hypervisor.
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/VirtioNetDxe/VirtioNet.h | 2 ++
OvmfPkg/VirtioNetDxe/Events.c | 14 ++++++++++++++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 19 +++++++++++++++++++
OvmfPkg/VirtioNetDxe/SnpShutdown.c | 11 +++++++++++
4 files changed, 46 insertions(+)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 710859bc6115..2964c946e26e 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -82,10 +82,12 @@ typedef struct {
EFI_HANDLE MacHandle; // VirtioNetDriverBindingStart
VRING RxRing; // VirtioNetInitRing
+ VOID *RxRingMap; // VirtioNetInitRing
UINT8 *RxBuf; // VirtioNetInitRx
UINT16 RxLastUsed; // VirtioNetInitRx
VRING TxRing; // VirtioNetInitRing
+ VOID *TxRingMap; // VirtioNetInitRing
UINT16 TxMaxPending; // VirtioNetInitTx
UINT16 TxCurPending; // VirtioNetInitTx
UINT16 *TxFreeStack; // VirtioNetInitTx
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 5be1af6ffbee..9eb129ca2006 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -88,4 +88,18 @@ VirtioNetExitBoot (
if (Dev->Snm.State == EfiSimpleNetworkInitialized) {
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
}
+
+ //
+ // If Rx and Tx Ring exist then unmap it so that hypervisor is not able to
+ // get readable data after device is reset.
+ //
+ if (Dev->TxRingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->TxRing, Dev->TxRingMap);
+ Dev->TxRingMap = NULL;
+ }
+
+ if (Dev->RxRingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->RxRing, Dev->RxRingMap);
+ Dev->RxRingMap = NULL;
+ }
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 6d9b81a9f939..cbc9c51cb643 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -461,11 +461,21 @@ VirtioNetInitialize (
goto DeviceFailed;
}
+ Status = VirtioRingMap (Dev->VirtIo, &Dev->RxRing, &Dev->RxRingMap);
+ if (EFI_ERROR (Status)) {
+ goto ReleaseRxRing;
+ }
+
Status = VirtioNetInitRing (Dev, VIRTIO_NET_Q_TX, &Dev->TxRing);
if (EFI_ERROR (Status)) {
goto ReleaseRxRing;
}
+ Status = VirtioRingMap (Dev->VirtIo, &Dev->TxRing, &Dev->TxRingMap);
+ if (EFI_ERROR (Status)) {
+ goto ReleaseTxRing;
+ }
+
//
// step 5 -- keep only the features we want
//
@@ -510,9 +520,18 @@ AbortDevice:
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
ReleaseTxRing:
+ if (Dev->TxRingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->TxRing, Dev->TxRingMap);
+ Dev->TxRingMap = NULL;
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
ReleaseRxRing:
+ if (Dev->RxRingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->TxRing, Dev->RxRingMap);
+ Dev->TxRingMap = NULL;
+ }
VirtioRingUninit (Dev->VirtIo, &Dev->RxRing);
DeviceFailed:
diff --git a/OvmfPkg/VirtioNetDxe/SnpShutdown.c b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
index 5e84191fbbdd..08524ab94006 100644
--- a/OvmfPkg/VirtioNetDxe/SnpShutdown.c
+++ b/OvmfPkg/VirtioNetDxe/SnpShutdown.c
@@ -65,6 +65,17 @@ VirtioNetShutdown (
}
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ if (Dev->RxRingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->RxRing, Dev->RxRingMap);
+ Dev->RxRingMap = NULL;
+ }
+
+ if (Dev->TxRingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->TxRing, Dev->TxRingMap);
+ Dev->TxRingMap = NULL;
+ }
+
VirtioNetShutdownRx (Dev);
VirtioNetShutdownTx (Dev);
VirtioRingUninit (Dev->VirtIo, &Dev->TxRing);
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 11/14] OvmfPkg/VirtioNetDxe: Allocate RxBuf using AllocateSharedPages()
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (9 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 10/14] OvmfPkg/VirtioNetDxe: Allocate Tx and Rx ring using AllocateSharedPage() Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 12/14] OvmfPkg/VirtioNetDxe: Dynamically allocate transmit header Brijesh Singh
` (4 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
RxBuf is shared between guest and hypervisor, use AllocateSharedPages()
to allocate this memory region and Map() it with BusMasterCommonBuffer
operation so that it can be accessed by guest and hypervisor.
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/VirtioNetDxe/VirtioNet.h | 3 ++
OvmfPkg/VirtioNetDxe/Events.c | 5 +++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 33 +++++++++++++++++---
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 10 +++++-
4 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 2964c946e26e..a4661bd5d2fe 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -4,6 +4,7 @@
Protocol instances for virtio-net devices.
Copyright (C) 2013, Red Hat, Inc.
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -85,6 +86,8 @@ typedef struct {
VOID *RxRingMap; // VirtioNetInitRing
UINT8 *RxBuf; // VirtioNetInitRx
UINT16 RxLastUsed; // VirtioNetInitRx
+ UINTN RxBufNoPages; // VirtioNetInitRx
+ VOID *RxBufMap; // VirtioNetInitRx
VRING TxRing; // VirtioNetInitRing
VOID *TxRingMap; // VirtioNetInitRing
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 9eb129ca2006..171b144518a9 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -102,4 +102,9 @@ VirtioNetExitBoot (
VirtioRingUnmap (Dev->VirtIo, &Dev->RxRing, Dev->RxRingMap);
Dev->RxRingMap = NULL;
}
+
+ if (Dev->RxBufMap != NULL) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+ Dev->RxBufMap = NULL;
+ }
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index cbc9c51cb643..59c9f1ed3516 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -244,6 +244,7 @@ VirtioNetInitRx (
UINTN PktIdx;
UINT16 DescIdx;
UINT8 *RxPtr;
+ UINTN NumBytes;
//
// In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
@@ -268,9 +269,24 @@ VirtioNetInitRx (
//
RxAlwaysPending = (UINT16) MIN (Dev->RxRing.QueueSize / 2, VNET_MAX_PENDING);
- Dev->RxBuf = AllocatePool (RxAlwaysPending * RxBufSize);
- if (Dev->RxBuf == NULL) {
- return EFI_OUT_OF_RESOURCES;
+ //
+ // The RxBuf is shared between guest and hypervisor, use SharedPages() to
+ // allocate this memory region and Map() it using BusMasterCommonBuffer
+ // operation so that it can be accessed equally by both guest and
+ // hypervisor.
+ //
+ NumBytes = RxAlwaysPending * RxBufSize;
+ Dev->RxBufNoPages = EFI_SIZE_TO_PAGES (NumBytes);
+ Status = VirtioAllocateSharedPages (Dev->VirtIo, Dev->RxBufNoPages,
+ (VOID *) &Dev->RxBuf);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = VirtioMapSharedBufferCommon (Dev->VirtIo, Dev->RxBuf, NumBytes,
+ &Dev->RxBufMap);
+ if (EFI_ERROR (Status)) {
+ goto Failed;
}
//
@@ -333,10 +349,19 @@ VirtioNetInitRx (
Status = Dev->VirtIo->SetQueueNotify (Dev->VirtIo, VIRTIO_NET_Q_RX);
if (EFI_ERROR (Status)) {
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
- FreePool (Dev->RxBuf);
+ goto Failed;
}
return Status;
+
+Failed:
+ if (Dev->RxBufMap != NULL) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+ Dev->RxBufMap = NULL;
+ }
+
+ VirtioFreeSharedPages (Dev->VirtIo, Dev->RxBufNoPages, (VOID *) Dev->RxBuf);
+ return Status;
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 9fedb72fdbd4..48cd4900911c 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -39,7 +39,15 @@ VirtioNetShutdownRx (
IN OUT VNET_DEV *Dev
)
{
- FreePool (Dev->RxBuf);
+ //
+ // If RxBuf mapping exist then unmap it.
+ //
+ if (Dev->RxBufMap != NULL) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
+ Dev->RxBufMap = NULL;
+ }
+
+ VirtioFreeSharedPages (Dev->VirtIo, Dev->RxBufNoPages, (VOID *) Dev->RxBuf);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 12/14] OvmfPkg/VirtioNetDxe: Dynamically allocate transmit header
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (10 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 11/14] OvmfPkg/VirtioNetDxe: Allocate RxBuf using AllocateSharedPages() Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 13/14] OvmfPkg/VirtioNetDxe: Use DeviceAddress in transmit vring descriptors Brijesh Singh
` (3 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
A network packets are transmitted by placing then into a transmit queue,
each packet is preceded by a VIRTIO_1_0_NET_REQ header. VirtioNetInitTx(),
builds the header once and fills the vring descriptors with the system
physical address of header.
The patch uses VirtIo->AllocateSharedPages() to allocate the header buffer
and map it with BusMasterCommonBuffer so that it can be equally accessed by
both processor and device.
We could map it with BusMasterRead but since the header pointer is
used after it was added into vring hence I choose to dynamically allocate
it and map it with BusMasterCommonBuffer to avoid the code complexity.
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/VirtioNetDxe/VirtioNet.h | 3 +-
OvmfPkg/VirtioNetDxe/Events.c | 5 +++
OvmfPkg/VirtioNetDxe/SnpInitialize.c | 41 ++++++++++++++++----
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 9 +++++
4 files changed, 49 insertions(+), 9 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index a4661bd5d2fe..873069e9de82 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -94,7 +94,8 @@ typedef struct {
UINT16 TxMaxPending; // VirtioNetInitTx
UINT16 TxCurPending; // VirtioNetInitTx
UINT16 *TxFreeStack; // VirtioNetInitTx
- VIRTIO_1_0_NET_REQ TxSharedReq; // VirtioNetInitTx
+ VIRTIO_1_0_NET_REQ *TxSharedReq; // VirtioNetInitTx
+ VOID *TxSharedReqMap; // VirtioNetInitTx
UINT16 TxLastUsed; // VirtioNetInitTx
} VNET_DEV;
diff --git a/OvmfPkg/VirtioNetDxe/Events.c b/OvmfPkg/VirtioNetDxe/Events.c
index 171b144518a9..0004898eab6e 100644
--- a/OvmfPkg/VirtioNetDxe/Events.c
+++ b/OvmfPkg/VirtioNetDxe/Events.c
@@ -107,4 +107,9 @@ VirtioNetExitBoot (
VirtioUnmapSharedBuffer (Dev->VirtIo, Dev->RxBufMap);
Dev->RxBufMap = NULL;
}
+
+ if (Dev->TxSharedReqMap != NULL) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
+ Dev->TxSharedReqMap = NULL;
+ }
}
diff --git a/OvmfPkg/VirtioNetDxe/SnpInitialize.c b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
index 59c9f1ed3516..9df9f7af32cc 100644
--- a/OvmfPkg/VirtioNetDxe/SnpInitialize.c
+++ b/OvmfPkg/VirtioNetDxe/SnpInitialize.c
@@ -18,6 +18,7 @@
**/
#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
@@ -139,8 +140,10 @@ VirtioNetInitTx (
IN OUT VNET_DEV *Dev
)
{
- UINTN TxSharedReqSize;
- UINTN PktIdx;
+ UINTN TxSharedReqSize;
+ UINTN PktIdx;
+ UINTN NumPages;
+ EFI_STATUS Status;
Dev->TxMaxPending = (UINT16) MIN (Dev->TxRing.QueueSize / 2,
VNET_MAX_PENDING);
@@ -152,12 +155,34 @@ VirtioNetInitTx (
}
//
+ // Allocate TxSharedReq header and map with BusMasterCommonBuffer so that it
+ // can be accessed equally by both processor and device.
+ //
+ NumPages = EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq));
+ Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages,
+ (VOID *) &Dev->TxSharedReq);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = VirtioMapSharedBufferCommon (Dev->VirtIo,
+ (VOID *) Dev->TxSharedReq, sizeof *(Dev->TxSharedReq),
+ &Dev->TxSharedReqMap);
+ if (EFI_ERROR (Status)) {
+ VirtioFreeSharedPages (Dev->VirtIo, NumPages,
+ (VOID *) Dev->TxSharedReqMap);
+ return Status;
+ }
+
+ ZeroMem ((VOID *) Dev->TxSharedReq, sizeof *(Dev->TxSharedReq));
+
+ //
// In VirtIo 1.0, the NumBuffers field is mandatory. In 0.9.5, it depends on
// VIRTIO_NET_F_MRG_RXBUF, which we never negotiate.
//
TxSharedReqSize = (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) ?
- sizeof Dev->TxSharedReq.V0_9_5 :
- sizeof Dev->TxSharedReq;
+ sizeof ((Dev->TxSharedReq)->V0_9_5) :
+ sizeof *(Dev->TxSharedReq);
for (PktIdx = 0; PktIdx < Dev->TxMaxPending; ++PktIdx) {
UINT16 DescIdx;
@@ -169,7 +194,7 @@ VirtioNetInitTx (
// For each possibly pending packet, lay out the descriptor for the common
// (unmodified by the host) virtio-net request header.
//
- Dev->TxRing.Desc[DescIdx].Addr = (UINTN) &Dev->TxSharedReq;
+ Dev->TxRing.Desc[DescIdx].Addr = (UINTN) Dev->TxSharedReq;
Dev->TxRing.Desc[DescIdx].Len = (UINT32) TxSharedReqSize;
Dev->TxRing.Desc[DescIdx].Flags = VRING_DESC_F_NEXT;
Dev->TxRing.Desc[DescIdx].Next = (UINT16) (DescIdx + 1);
@@ -184,13 +209,13 @@ VirtioNetInitTx (
//
// virtio-0.9.5, Appendix C, Packet Transmission
//
- Dev->TxSharedReq.V0_9_5.Flags = 0;
- Dev->TxSharedReq.V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
+ Dev->TxSharedReq->V0_9_5.Flags = 0;
+ Dev->TxSharedReq->V0_9_5.GsoType = VIRTIO_NET_HDR_GSO_NONE;
//
// For VirtIo 1.0 only -- the field exists, but it is unused
//
- Dev->TxSharedReq.NumBuffers = 0;
+ Dev->TxSharedReq->NumBuffers = 0;
//
// virtio-0.9.5, 2.4.2 Receiving Used Buffers From the Device
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 48cd4900911c..3ed7132be531 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -57,5 +57,14 @@ VirtioNetShutdownTx (
IN OUT VNET_DEV *Dev
)
{
+ if (Dev->TxSharedReqMap != NULL) {
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Dev->TxSharedReqMap);
+ Dev->TxSharedReqMap = NULL;
+ }
+
+ VirtioFreeSharedPages (Dev->VirtIo,
+ EFI_SIZE_TO_PAGES (sizeof *(Dev->TxSharedReq)),
+ (VOID *) Dev->TxSharedReq);
+
FreePool (Dev->TxFreeStack);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 13/14] OvmfPkg/VirtioNetDxe: Use DeviceAddress in transmit vring descriptors
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (11 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 12/14] OvmfPkg/VirtioNetDxe: Dynamically allocate transmit header Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-07 11:58 ` [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in " Brijesh Singh
` (2 subsequent siblings)
15 siblings, 0 replies; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
Update VirtioNetTransmit() to map the callers transmit buffer to a device
DMA address and use the DeviceAddress in vring descriptors.
Since the transmit buffers are returned back to caller in
VirtioNetGetStatus() hence we maintain a list to map DeviceAddress to
HostAddress. The list is consulted to find the correct HostAddress upon
transmit completion interruption.
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/VirtioNetDxe/VirtioNet.h | 17 ++++
OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 19 +++-
OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 100 ++++++++++++++++++++
OvmfPkg/VirtioNetDxe/SnpTransmit.c | 22 +++--
4 files changed, 149 insertions(+), 9 deletions(-)
diff --git a/OvmfPkg/VirtioNetDxe/VirtioNet.h b/OvmfPkg/VirtioNetDxe/VirtioNet.h
index 873069e9de82..6c5129f178cd 100644
--- a/OvmfPkg/VirtioNetDxe/VirtioNet.h
+++ b/OvmfPkg/VirtioNetDxe/VirtioNet.h
@@ -269,6 +269,23 @@ VirtioNetShutdownTx (
IN OUT VNET_DEV *Dev
);
+EFI_STATUS
+EFIAPI
+VirtioMapTxBuf (
+ IN VNET_DEV *Dev,
+ IN EFI_PHYSICAL_ADDRESS HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
+ );
+
+EFI_STATUS
+EFIAPI
+VirtioUnmapTxBuf (
+ IN VNET_DEV *Dev,
+ OUT EFI_PHYSICAL_ADDRESS *HostAddress,
+ IN EFI_PHYSICAL_ADDRESS DeviceAddress
+ );
+
//
// event callbacks
//
diff --git a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
index 694940ea1d97..ddea29e65d57 100644
--- a/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
+++ b/OvmfPkg/VirtioNetDxe/SnpGetStatus.c
@@ -5,6 +5,7 @@
Copyright (C) 2013, Red Hat, Inc.
Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
This program and the accompanying materials are licensed and made available
under the terms and conditions of the BSD License which accompanies this
@@ -126,8 +127,10 @@ VirtioNetGetStatus (
*TxBuf = NULL;
}
else {
- UINT16 UsedElemIdx;
- UINT32 DescIdx;
+ UINT16 UsedElemIdx;
+ UINT32 DescIdx;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ EFI_PHYSICAL_ADDRESS HostAddress;
//
// fetch the first descriptor among those that the hypervisor reports
@@ -141,9 +144,19 @@ VirtioNetGetStatus (
ASSERT (DescIdx < (UINT32) (2 * Dev->TxMaxPending - 1));
//
+ // Unmap the DeviceAddress
+ //
+ DeviceAddress = Dev->TxRing.Desc[DescIdx + 1].Addr;
+ Status = VirtioUnmapTxBuf (Dev, &HostAddress, DeviceAddress);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ //
+ //
// report buffer address to caller that has been enqueued by caller
//
- *TxBuf = (VOID *)(UINTN) Dev->TxRing.Desc[DescIdx + 1].Addr;
+ *TxBuf = (VOID *)(UINTN) HostAddress;
//
// now this descriptor can be used again to enqueue a transmit buffer
diff --git a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
index 3ed7132be531..6a6866e65b5f 100644
--- a/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
+++ b/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c
@@ -14,10 +14,25 @@
**/
+#include <Library/BaseLib.h>
#include <Library/MemoryAllocationLib.h>
#include "VirtioNet.h"
+#define PRIVATE_TXBUF_SIGNATURE SIGNATURE_32 ('t', 'x', 'b', 'f')
+typedef struct {
+ UINT32 Signature;
+ LIST_ENTRY Link;
+ EFI_PHYSICAL_ADDRESS HostAddress;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *Mapping;
+} PRIVATE_TXBUF_ENTRY;
+#define PRIVATE_TXBUF_FROM_LINK(a) CR (a, PRIVATE_TXBUF_ENTRY, Link, \
+ PRIVATE_TXBUF_SIGNATURE)
+
+STATIC LIST_ENTRY mTxBufMapList = INITIALIZE_LIST_HEAD_VARIABLE (mTxBufMapList);
+
+
/**
Release RX and TX resources on the boundary of the
EfiSimpleNetworkInitialized state.
@@ -68,3 +83,88 @@ VirtioNetShutdownTx (
FreePool (Dev->TxFreeStack);
}
+
+EFI_STATUS
+EFIAPI
+VirtioMapTxBuf (
+ IN VNET_DEV *Dev,
+ IN EFI_PHYSICAL_ADDRESS HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT EFI_PHYSICAL_ADDRESS *DeviceAddress
+ )
+{
+ EFI_STATUS Status;
+ PRIVATE_TXBUF_ENTRY *Private;
+
+ Private = AllocatePool (sizeof (*Private));
+ if (Private == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Private->Signature = PRIVATE_TXBUF_SIGNATURE;
+ Private->HostAddress = HostAddress;
+
+ Status = VirtioMapSharedBufferRead (Dev->VirtIo,
+ (VOID *) (UINTN) Private->HostAddress, NumberOfBytes,
+ &Private->DeviceAddress, &Private->Mapping);
+ if (EFI_ERROR (Status)) {
+ FreePool (Private);
+ return Status;
+ }
+
+ *DeviceAddress = Private->DeviceAddress;
+
+ //
+ // Add the mapping information into internal list so that we can retrieve
+ // the HostAddress upon Unmap().
+ //
+ InsertTailList (&mTxBufMapList, &Private->Link);
+
+ return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+VirtioUnmapTxBuf (
+ IN VNET_DEV *Dev,
+ OUT EFI_PHYSICAL_ADDRESS *HostAddress,
+ IN EFI_PHYSICAL_ADDRESS DeviceAddress
+ )
+{
+ EFI_STATUS Status;
+ PRIVATE_TXBUF_ENTRY *Private;
+ LIST_ENTRY *Link;
+ BOOLEAN Found;
+
+ Found = FALSE;
+
+ //
+ // Iterate through internal txbuf list to find mapping for a given
+ // DeviceAddress.
+ //
+ for (Link = GetFirstNode (&mTxBufMapList)
+ ; !IsNull (&mTxBufMapList, Link)
+ ; Link = GetNextNode (&mTxBufMapList, Link)
+ ) {
+ Private = PRIVATE_TXBUF_FROM_LINK (Link);
+ if (Private->DeviceAddress == DeviceAddress) {
+ Found = TRUE;
+ break;
+ }
+ }
+
+ //
+ // We failed to find mapping for the given DeviceAddress
+ // (this should never happen)
+ //
+ ASSERT (Found);
+
+ Status = VirtioUnmapSharedBuffer (Dev->VirtIo, Private->Mapping);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ *HostAddress = Private->HostAddress;
+
+ return EFI_SUCCESS;
+}
diff --git a/OvmfPkg/VirtioNetDxe/SnpTransmit.c b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
index 7ca40d5d0650..20c56c12df04 100644
--- a/OvmfPkg/VirtioNetDxe/SnpTransmit.c
+++ b/OvmfPkg/VirtioNetDxe/SnpTransmit.c
@@ -73,11 +73,12 @@ VirtioNetTransmit (
IN UINT16 *Protocol OPTIONAL
)
{
- VNET_DEV *Dev;
- EFI_TPL OldTpl;
- EFI_STATUS Status;
- UINT16 DescIdx;
- UINT16 AvailIdx;
+ VNET_DEV *Dev;
+ EFI_TPL OldTpl;
+ EFI_STATUS Status;
+ UINT16 DescIdx;
+ UINT16 AvailIdx;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
if (This == NULL || BufferSize == 0 || Buffer == NULL) {
return EFI_INVALID_PARAMETER;
@@ -144,10 +145,19 @@ VirtioNetTransmit (
}
//
+ // Map the transmit buffer HostAddress to a DeviceAddress
+ //
+ Status = VirtioMapTxBuf (Dev, (EFI_PHYSICAL_ADDRESS) (UINTN) Buffer,
+ BufferSize, &DeviceAddress);
+ if (EFI_ERROR (Status)) {
+ goto Exit;
+ }
+
+ //
// virtio-0.9.5, 2.4.1 Supplying Buffers to The Device
//
DescIdx = Dev->TxFreeStack[Dev->TxCurPending++];
- Dev->TxRing.Desc[DescIdx + 1].Addr = (UINTN) Buffer;
+ Dev->TxRing.Desc[DescIdx + 1].Addr = (UINTN) DeviceAddress;
Dev->TxRing.Desc[DescIdx + 1].Len = (UINT32) BufferSize;
//
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (12 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 13/14] OvmfPkg/VirtioNetDxe: Use DeviceAddress in transmit vring descriptors Brijesh Singh
@ 2017-08-07 11:58 ` Brijesh Singh
2017-08-10 0:25 ` Laszlo Ersek
2017-08-09 14:39 ` [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Laszlo Ersek
2017-08-09 22:38 ` Laszlo Ersek
15 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-07 11:58 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
to map system memory to device address and programs the vring descriptors
with device addresses.
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/VirtioRngDxe/VirtioRng.h | 1 +
OvmfPkg/VirtioRngDxe/VirtioRng.c | 47 +++++++++++++++++---
2 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
index 998f9fae48c2..b372d84c1ebc 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
@@ -38,6 +38,7 @@ typedef struct {
EFI_EVENT ExitBoot; // DriverBindingStart 0
VRING Ring; // VirtioRingInit 2
EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1
+ VOID *RingMap; // VirtioRngInit 1
} VIRTIO_RNG_DEV;
#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
index e20602ac7225..5ff54867616a 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -140,11 +140,15 @@ VirtioRngGetRNG (
UINT32 Len;
UINT32 BufferSize;
EFI_STATUS Status;
+ UINTN NumPages;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *Mapping;
if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
return EFI_INVALID_PARAMETER;
}
+ Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
//
// We only support the raw algorithm, so reject requests for anything else
//
@@ -153,12 +157,18 @@ VirtioRngGetRNG (
return EFI_UNSUPPORTED;
}
- Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength);
- if (Buffer == NULL) {
+ NumPages = EFI_SIZE_TO_PAGES (RNGValueLength);
+ Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID *)&Buffer);
+ if (EFI_ERROR (Status)) {
return EFI_DEVICE_ERROR;
}
- Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
+ Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer,
+ RNGValueLength, &DeviceAddress, &Mapping);
+ if (EFI_ERROR (Status)) {
+ VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
+ return EFI_DEVICE_ERROR;
+ }
//
// The Virtio RNG device may return less data than we asked it to, and can
@@ -170,7 +180,7 @@ VirtioRngGetRNG (
VirtioPrepare (&Dev->Ring, &Indices);
VirtioAppendDesc (&Dev->Ring,
- (UINTN)Buffer + Index,
+ (UINTN)DeviceAddress + Index,
BufferSize,
VRING_DESC_F_WRITE,
&Indices);
@@ -178,19 +188,22 @@ VirtioRngGetRNG (
if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
EFI_SUCCESS) {
Status = EFI_DEVICE_ERROR;
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
goto FreeBuffer;
}
ASSERT (Len > 0);
ASSERT (Len <= BufferSize);
}
+ VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
+
for (Index = 0; Index < RNGValueLength; Index++) {
RNGValue[Index] = Buffer[Index];
}
Status = EFI_SUCCESS;
FreeBuffer:
- FreePool ((VOID *)Buffer);
+ VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
return Status;
}
@@ -281,6 +294,11 @@ VirtioRngInit (
goto Failed;
}
+ Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingMap);
+ 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.
@@ -332,6 +350,11 @@ VirtioRngInit (
return EFI_SUCCESS;
ReleaseQueue:
+ if (Dev->RingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
+ Dev->RingMap = NULL;
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
Failed:
@@ -359,6 +382,11 @@ VirtioRngUninit (
// the old comms area.
//
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ if (Dev->RingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
+ }
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
}
@@ -385,6 +413,15 @@ VirtioRngExitBoot (
//
Dev = Context;
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ //
+ // If Ring mapping exist then umap it so that hypervisor will not able to
+ // get readable data after device reset.
+ //
+ if (Dev->RingMap != NULL) {
+ VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
+ Dev->RingMap = NULL;
+ }
}
--
2.7.4
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors
2017-08-07 11:58 ` [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in " Brijesh Singh
@ 2017-08-10 0:25 ` Laszlo Ersek
2017-08-10 0:46 ` Laszlo Ersek
0 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-10 0:25 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/07/17 13:58, Brijesh Singh wrote:
> The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
> to map system memory to device address and programs the vring descriptors
> with device addresses.
>
> 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/VirtioRngDxe/VirtioRng.h | 1 +
> OvmfPkg/VirtioRngDxe/VirtioRng.c | 47 +++++++++++++++++---
> 2 files changed, 43 insertions(+), 5 deletions(-)
I'm skipping all the other device driver conversion patches for now (v1
8-13), because I think that this is the simplest driver, and we already
have enough changes queued from this review.
I suggest that for v2, you please update and test all of the drivers (so
that we can be sure that my requests are feasible), but move this driver
patch in front of the others.
I'll say a number of generalities:
* Please never roll-back earlier actions directly within an
error-catching "if" -- instead, insert a new error handling label at
the appropriate place, and jump to it with "goto". Sooner or later
we'll grow yet more actions, and then we'll have to convert those "if"
bodies to goto's anyway.
* NULL for RingMap is a valid value () -- see also point (8) in my v1
03/14 feedback -- and device driver logic should not depend on it.
Instead, use additional (precise) labels -- such as "UnmapQueue" and
"UnmapBuffer" -- and matching goto statements.
If a VirtIo->MapSharedBuffer() implementation gives you RingMap=NULL
(on success), then VirtIo->UnmapSharedBuffer() will also take
RingMap=NULL -- you simply don't have to be aware of the value.
* In most cases, the fact that you have a live exit-boot-services
callback implies that your device is "live" as well. So no need for
additional checks before unmapping the queue. (There are exceptions of
course, such as virtio-net -- IIRC the SimpleNetworkProtocol has an
internal state flag that affects this question.)
For example, in the virtio-rng case, the exit-boot-services
notification function is installed in VirtioRngDriverBindingStart()
*after* the call to VirtioRngInit(). In addition, it is uninstalled --
its associated event is closed -- in VirtioRngDriverBindingStop(),
*before* the call to VirtioRngUninit(). So whenever the notification
function can run, the device is guaranteed to be live.
Thanks,
Laszlo
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
> index 998f9fae48c2..b372d84c1ebc 100644
> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
> @@ -38,6 +38,7 @@ typedef struct {
> EFI_EVENT ExitBoot; // DriverBindingStart 0
> VRING Ring; // VirtioRingInit 2
> EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1
> + VOID *RingMap; // VirtioRngInit 1
> } VIRTIO_RNG_DEV;
>
> #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> index e20602ac7225..5ff54867616a 100644
> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -140,11 +140,15 @@ VirtioRngGetRNG (
> UINT32 Len;
> UINT32 BufferSize;
> EFI_STATUS Status;
> + UINTN NumPages;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *Mapping;
>
> if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
> return EFI_INVALID_PARAMETER;
> }
>
> + Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
> //
> // We only support the raw algorithm, so reject requests for anything else
> //
> @@ -153,12 +157,18 @@ VirtioRngGetRNG (
> return EFI_UNSUPPORTED;
> }
>
> - Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength);
> - if (Buffer == NULL) {
> + NumPages = EFI_SIZE_TO_PAGES (RNGValueLength);
> + Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID *)&Buffer);
> + if (EFI_ERROR (Status)) {
> return EFI_DEVICE_ERROR;
> }
>
> - Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
> + Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer,
> + RNGValueLength, &DeviceAddress, &Mapping);
> + if (EFI_ERROR (Status)) {
> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
> + return EFI_DEVICE_ERROR;
> + }
>
> //
> // The Virtio RNG device may return less data than we asked it to, and can
> @@ -170,7 +180,7 @@ VirtioRngGetRNG (
>
> VirtioPrepare (&Dev->Ring, &Indices);
> VirtioAppendDesc (&Dev->Ring,
> - (UINTN)Buffer + Index,
> + (UINTN)DeviceAddress + Index,
> BufferSize,
> VRING_DESC_F_WRITE,
> &Indices);
> @@ -178,19 +188,22 @@ VirtioRngGetRNG (
> if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
> EFI_SUCCESS) {
> Status = EFI_DEVICE_ERROR;
> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
> goto FreeBuffer;
> }
> ASSERT (Len > 0);
> ASSERT (Len <= BufferSize);
> }
>
> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
> +
> for (Index = 0; Index < RNGValueLength; Index++) {
> RNGValue[Index] = Buffer[Index];
> }
> Status = EFI_SUCCESS;
>
> FreeBuffer:
> - FreePool ((VOID *)Buffer);
> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
> return Status;
> }
>
> @@ -281,6 +294,11 @@ VirtioRngInit (
> goto Failed;
> }
>
> + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingMap);
> + 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.
> @@ -332,6 +350,11 @@ VirtioRngInit (
> return EFI_SUCCESS;
>
> ReleaseQueue:
> + if (Dev->RingMap != NULL) {
> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
> + Dev->RingMap = NULL;
> + }
> +
> VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>
> Failed:
> @@ -359,6 +382,11 @@ VirtioRngUninit (
> // the old comms area.
> //
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> + if (Dev->RingMap != NULL) {
> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
> + }
> +
> VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
> }
>
> @@ -385,6 +413,15 @@ VirtioRngExitBoot (
> //
> Dev = Context;
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> + //
> + // If Ring mapping exist then umap it so that hypervisor will not able to
> + // get readable data after device reset.
> + //
> + if (Dev->RingMap != NULL) {
> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
> + Dev->RingMap = NULL;
> + }
> }
>
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors
2017-08-10 0:25 ` Laszlo Ersek
@ 2017-08-10 0:46 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-10 0:46 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/10/17 02:25, Laszlo Ersek wrote:
> On 08/07/17 13:58, Brijesh Singh wrote:
>> The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer()
>> to map system memory to device address and programs the vring descriptors
>> with device addresses.
>>
>> 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/VirtioRngDxe/VirtioRng.h | 1 +
>> OvmfPkg/VirtioRngDxe/VirtioRng.c | 47 +++++++++++++++++---
>> 2 files changed, 43 insertions(+), 5 deletions(-)
>
> I'm skipping all the other device driver conversion patches for now (v1
> 8-13), because I think that this is the simplest driver, and we already
> have enough changes queued from this review.
>
> I suggest that for v2, you please update and test all of the drivers (so
> that we can be sure that my requests are feasible), but move this driver
> patch in front of the others.
>
> I'll say a number of generalities:
>
> * Please never roll-back earlier actions directly within an
> error-catching "if" -- instead, insert a new error handling label at
> the appropriate place, and jump to it with "goto". Sooner or later
> we'll grow yet more actions, and then we'll have to convert those "if"
> bodies to goto's anyway.
>
> * NULL for RingMap is a valid value () -- see also point (8) in my v1
> 03/14 feedback -- and device driver logic should not depend on it.
> Instead, use additional (precise) labels -- such as "UnmapQueue" and
> "UnmapBuffer" -- and matching goto statements.
>
> If a VirtIo->MapSharedBuffer() implementation gives you RingMap=NULL
> (on success), then VirtIo->UnmapSharedBuffer() will also take
> RingMap=NULL -- you simply don't have to be aware of the value.
>
> * In most cases, the fact that you have a live exit-boot-services
> callback implies that your device is "live" as well. So no need for
> additional checks before unmapping the queue. (There are exceptions of
> course, such as virtio-net -- IIRC the SimpleNetworkProtocol has an
> internal state flag that affects this question.)
>
> For example, in the virtio-rng case, the exit-boot-services
> notification function is installed in VirtioRngDriverBindingStart()
> *after* the call to VirtioRngInit(). In addition, it is uninstalled --
> its associated event is closed -- in VirtioRngDriverBindingStop(),
> *before* the call to VirtioRngUninit(). So whenever the notification
> function can run, the device is guaranteed to be live.
Something else: please don't forget about the VIRTIO_F_IOMMU_PLATFORM
flag -- pls see point (5.3) in
<http://mid.mail-archive.com/841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com>.
After this conversion is complete, we can claim that our virtio device
drivers are "IOMMU aware", whenever we negotiate VIRTIO_F_VERSION_1.
(It's a different question what IOMMU drivers we have.)
Thanks,
Laszlo
>> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
>> index 998f9fae48c2..b372d84c1ebc 100644
>> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
>> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
>> @@ -38,6 +38,7 @@ typedef struct {
>> EFI_EVENT ExitBoot; // DriverBindingStart 0
>> VRING Ring; // VirtioRingInit 2
>> EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1
>> + VOID *RingMap; // VirtioRngInit 1
>> } VIRTIO_RNG_DEV;
>>
>> #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
>> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
>> index e20602ac7225..5ff54867616a 100644
>> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
>> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
>> @@ -140,11 +140,15 @@ VirtioRngGetRNG (
>> UINT32 Len;
>> UINT32 BufferSize;
>> EFI_STATUS Status;
>> + UINTN NumPages;
>> + EFI_PHYSICAL_ADDRESS DeviceAddress;
>> + VOID *Mapping;
>>
>> if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
>> return EFI_INVALID_PARAMETER;
>> }
>>
>> + Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
>> //
>> // We only support the raw algorithm, so reject requests for anything else
>> //
>> @@ -153,12 +157,18 @@ VirtioRngGetRNG (
>> return EFI_UNSUPPORTED;
>> }
>>
>> - Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength);
>> - if (Buffer == NULL) {
>> + NumPages = EFI_SIZE_TO_PAGES (RNGValueLength);
>> + Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID *)&Buffer);
>> + if (EFI_ERROR (Status)) {
>> return EFI_DEVICE_ERROR;
>> }
>>
>> - Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
>> + Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer,
>> + RNGValueLength, &DeviceAddress, &Mapping);
>> + if (EFI_ERROR (Status)) {
>> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
>> + return EFI_DEVICE_ERROR;
>> + }
>>
>> //
>> // The Virtio RNG device may return less data than we asked it to, and can
>> @@ -170,7 +180,7 @@ VirtioRngGetRNG (
>>
>> VirtioPrepare (&Dev->Ring, &Indices);
>> VirtioAppendDesc (&Dev->Ring,
>> - (UINTN)Buffer + Index,
>> + (UINTN)DeviceAddress + Index,
>> BufferSize,
>> VRING_DESC_F_WRITE,
>> &Indices);
>> @@ -178,19 +188,22 @@ VirtioRngGetRNG (
>> if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
>> EFI_SUCCESS) {
>> Status = EFI_DEVICE_ERROR;
>> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
>> goto FreeBuffer;
>> }
>> ASSERT (Len > 0);
>> ASSERT (Len <= BufferSize);
>> }
>>
>> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping);
>> +
>> for (Index = 0; Index < RNGValueLength; Index++) {
>> RNGValue[Index] = Buffer[Index];
>> }
>> Status = EFI_SUCCESS;
>>
>> FreeBuffer:
>> - FreePool ((VOID *)Buffer);
>> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer);
>> return Status;
>> }
>>
>> @@ -281,6 +294,11 @@ VirtioRngInit (
>> goto Failed;
>> }
>>
>> + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &Dev->RingMap);
>> + 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.
>> @@ -332,6 +350,11 @@ VirtioRngInit (
>> return EFI_SUCCESS;
>>
>> ReleaseQueue:
>> + if (Dev->RingMap != NULL) {
>> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
>> + Dev->RingMap = NULL;
>> + }
>> +
>> VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>>
>> Failed:
>> @@ -359,6 +382,11 @@ VirtioRngUninit (
>> // the old comms area.
>> //
>> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>> +
>> + if (Dev->RingMap != NULL) {
>> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
>> + }
>> +
>> VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>> }
>>
>> @@ -385,6 +413,15 @@ VirtioRngExitBoot (
>> //
>> Dev = Context;
>> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>> +
>> + //
>> + // If Ring mapping exist then umap it so that hypervisor will not able to
>> + // get readable data after device reset.
>> + //
>> + if (Dev->RingMap != NULL) {
>> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap);
>> + Dev->RingMap = NULL;
>> + }
>> }
>>
>>
>>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (13 preceding siblings ...)
2017-08-07 11:58 ` [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in " Brijesh Singh
@ 2017-08-09 14:39 ` Laszlo Ersek
2017-08-09 17:35 ` Brijesh Singh
2017-08-09 22:38 ` Laszlo Ersek
15 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 14:39 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/07/17 13:58, Brijesh Singh wrote:
> Currently, virtio drivers provides the system physical address to the device.
> However, some systems may feature an IOMMU that requires the drivers to pass
> the device addresses to the device - which are then translated by the IOMMU
> into physical addresses in memory. The patch series introduces new member
> functions in VIRTIO_DEVICE_PROTOCOL which can be used for mapping a system
> physical address to device address.
>
> The approach that this patch series takes is to maps the system physical
> address to device address for buffers (including rings, device specifc
> request and response pointed by vring descriptor, and any further memory
> reference by those request and response).
>
> Patch 1 - 3:
> Defines and implements new member functions to map a system physical address
> to device address. The patch implements Laszlo's suggestion [1].
(1) I guess under [1] you meant the following message:
http://mid.mail-archive.com/841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com
If you want, you can add that link to the commit message of patch #1.
(2) But, that's not my main point here. Before I forget, I'd like to
point out that you missed one of the three virtio protocol
implementations -- see my point (5.4) in the above-referenced message
--, namely:
> - "ArmVirtPkg/VirtioFdtDxe" (via
> "OvmfPkg/Library/VirtioMmioDeviceLib") binds virtio-mmio devices,
> and offers virtio 0.9.5 semantics.
So please replicate patch v1 03/14 to
"OvmfPkg/Library/VirtioMmioDeviceLib". Otherwise, the modified virtio
device drivers will crash when they are built for ArmVirtPkg and used
over virtio-mmio transports.
(3) Starting with your v2, please add a reminder to your blurb -- for
Ard and myself -- that before merging this, we should regression-test it
on aarch64.
Thanks!
Laszlo
>
> Patch 4 - 7:
> Allocates the vring buffer using newly added member functions and provides
> some helper functions.
>
> Patch 8:
> Update the virtio-blk driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device virtio-blk-pci,drive=disk0
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device virtio-blk-pci,drive=disk0,disable-legacy=on
>
> Patch 9:
> Update the virtio-scsi driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device scsi-hd,drive=disk0
> -device virtio-scsi-pci,id=scsi \
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device scsi-hd,drive=disk0 \
> -device virtio-scsi-pci,id=scsi,disable-legacy=on \
>
> Patch 10 - 13:
> Update the virtio-net driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -netdev type=tap,id=net0 \
> -device virtio-net-pci,netdev=net0,romfile=
>
> # $QEMU \
> -netdev type=tap,id=net0 \
> -device virtio-net-pci,netdev=net0,disable-legacy=on,romfile=
>
> Patch 14:
> Update the virtio-rng driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -device virtio-rng-pci
>
> # $QEMU \
> -device virtio-rng-pci,disable-legacy=on
>
> And succesfully ran RngTest.efi from SecurityPkg/Application
>
> Repo: https://github.com/codomania/edk2
> Branch: virtio-support
>
> 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>
>
> Brijesh Singh (14):
> OvmfPkg/Virtio: Introduce new member functions in
> VIRTIO_DEVICE_PROTOCOL
> OvmfPkg/Virtio10Dxe: Implement new member functions
> OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
> OvmfPkg/VirtioLib: Add SharedBuffer helper functions
> OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit()
> OvmfPkg/VirtioLib: Add functions to map/unmap VRING
> OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer
> OvmfPkg/VirtioBlkDxe: Use DeviceAddresses in vring descriptors
> OvmfPkg/VirtioScsiDxe: Use DeviceAddresses in vring descriptors
> OvmfPkg/VirtioNetDxe: Allocate Tx and Rx ring using
> AllocateSharedPage()
> OvmfPkg/VirtioNetDxe: Allocate RxBuf using AllocateSharedPages()
> OvmfPkg/VirtioNetDxe: Dynamically allocate transmit header
> OvmfPkg/VirtioNetDxe: Use DeviceAddress in transmit vring descriptors
> OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors
>
> OvmfPkg/Include/Library/VirtioLib.h | 198 ++++++++++++-
> OvmfPkg/Include/Protocol/VirtioDevice.h | 121 ++++++++
> OvmfPkg/VirtioBlkDxe/VirtioBlk.h | 1 +
> OvmfPkg/VirtioNetDxe/VirtioNet.h | 25 +-
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h | 34 +++
> OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 +
> OvmfPkg/VirtioScsiDxe/VirtioScsi.h | 1 +
> OvmfPkg/Library/VirtioLib/VirtioLib.c | 296 +++++++++++++++++++-
> OvmfPkg/Virtio10Dxe/Virtio10.c | 114 +++++++-
> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 104 ++++++-
> OvmfPkg/VirtioGpuDxe/Commands.c | 7 +-
> OvmfPkg/VirtioNetDxe/Events.c | 24 ++
> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 19 +-
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 102 +++++--
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 119 +++++++-
> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 16 +-
> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 22 +-
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 7 +-
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 66 +++++
> OvmfPkg/VirtioRngDxe/VirtioRng.c | 54 +++-
> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 121 +++++++-
> 21 files changed, 1378 insertions(+), 74 deletions(-)
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-09 14:39 ` [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Laszlo Ersek
@ 2017-08-09 17:35 ` Brijesh Singh
2017-08-09 17:56 ` Laszlo Ersek
0 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-09 17:35 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/09/2017 09:39 AM, Laszlo Ersek wrote:
> On 08/07/17 13:58, Brijesh Singh wrote:
>> Currently, virtio drivers provides the system physical address to the device.
>> However, some systems may feature an IOMMU that requires the drivers to pass
>> the device addresses to the device - which are then translated by the IOMMU
>> into physical addresses in memory. The patch series introduces new member
>> functions in VIRTIO_DEVICE_PROTOCOL which can be used for mapping a system
>> physical address to device address.
>>
>> The approach that this patch series takes is to maps the system physical
>> address to device address for buffers (including rings, device specifc
>> request and response pointed by vring descriptor, and any further memory
>> reference by those request and response).
>>
>> Patch 1 - 3:
>> Defines and implements new member functions to map a system physical address
>> to device address. The patch implements Laszlo's suggestion [1].
>
> (1) I guess under [1] you meant the following message:
>
> http://mid.mail-archive.com/841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com
>
Yes, thank you :) I did not realized that I forgot adding the link.
> If you want, you can add that link to the commit message of patch #1.
>
> (2) But, that's not my main point here. Before I forget, I'd like to
> point out that you missed one of the three virtio protocol
> implementations -- see my point (5.4) in the above-referenced message
> --, namely:
>
>> - "ArmVirtPkg/VirtioFdtDxe" (via
>> "OvmfPkg/Library/VirtioMmioDeviceLib") binds virtio-mmio devices,
>> and offers virtio 0.9.5 semantics.
>
> So please replicate patch v1 03/14 to
> "OvmfPkg/Library/VirtioMmioDeviceLib". Otherwise, the modified virtio
> device drivers will crash when they are built for ArmVirtPkg and used
> over virtio-mmio transports.
>
Sure, I will make the necessary changes in VirtioMmioDeviceLib and try
do the build test but I don't have aarch64 platform to verify at the
runtime.
> (3) Starting with your v2, please add a reminder to your blurb -- for
> Ard and myself -- that before merging this, we should regression-test it
> on aarch64.
>
Will do
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-09 17:35 ` Brijesh Singh
@ 2017-08-09 17:56 ` Laszlo Ersek
2017-08-09 19:29 ` Laszlo Ersek
2017-08-11 22:22 ` Brijesh Singh
0 siblings, 2 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 17:56 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/09/17 19:35, Brijesh Singh wrote:
>
>
> On 08/09/2017 09:39 AM, Laszlo Ersek wrote:
>> On 08/07/17 13:58, Brijesh Singh wrote:
>>> Currently, virtio drivers provides the system physical address to the
>>> device.
>>> However, some systems may feature an IOMMU that requires the drivers
>>> to pass
>>> the device addresses to the device - which are then translated by the
>>> IOMMU
>>> into physical addresses in memory. The patch series introduces new
>>> member
>>> functions in VIRTIO_DEVICE_PROTOCOL which can be used for mapping a
>>> system
>>> physical address to device address.
>>>
>>> The approach that this patch series takes is to maps the system physical
>>> address to device address for buffers (including rings, device specifc
>>> request and response pointed by vring descriptor, and any further
>>> memory
>>> reference by those request and response).
>>>
>>> Patch 1 - 3:
>>> Defines and implements new member functions to map a system
>>> physical address
>>> to device address. The patch implements Laszlo's suggestion [1].
>>
>> (1) I guess under [1] you meant the following message:
>>
>> http://mid.mail-archive.com/841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com
>>
>>
>
> Yes, thank you :) I did not realized that I forgot adding the link.
>
>
>> If you want, you can add that link to the commit message of patch #1.
>>
>> (2) But, that's not my main point here. Before I forget, I'd like to
>> point out that you missed one of the three virtio protocol
>> implementations -- see my point (5.4) in the above-referenced message
>> --, namely:
>>
>>> - "ArmVirtPkg/VirtioFdtDxe" (via
>>> "OvmfPkg/Library/VirtioMmioDeviceLib") binds virtio-mmio devices,
>>> and offers virtio 0.9.5 semantics.
>>
>> So please replicate patch v1 03/14 to
>> "OvmfPkg/Library/VirtioMmioDeviceLib". Otherwise, the modified virtio
>> device drivers will crash when they are built for ArmVirtPkg and used
>> over virtio-mmio transports.
>>
>
> Sure, I will make the necessary changes in VirtioMmioDeviceLib and try
> do the build test but I don't have aarch64 platform to verify at the
> runtime.
Actually, dependent on your GNU/Linux distribution, it is pretty easy to
do on x86_64 too. It comes together from two parts:
(1) installing an aarch64 cross compiler
(2) installing qemu-system-aarch64 (from source or distro package),
and then either using it directly (from the cmdline) or with the
libvirt toolstack
The only "real" difference is that it's going to use TCG and not KVM
(software emulation rather than hardware virtualization), so it will be
slower, but that's not really a problem if you only care about your VM
until the firmware boots the OS :)
On Fedora, the cross-compiler (and cross-binutils) packages are built
from the following SRPMs:
https://koji.fedoraproject.org/koji/buildinfo?buildID=921790
https://koji.fedoraproject.org/koji/buildinfo?buildID=912429
("cross-gcc", "cross-binutils")
Linaro distributes distro-independent cross compilers:
http://www.linaro.org/downloads/
Build instructions for ArmVirtQemu, and usage hints for the QEMU command
line, can be found in the Linaro Wiki:
https://wiki.linaro.org/LEG/UEFIforQEMU
Thanks
Laszlo
>> (3) Starting with your v2, please add a reminder to your blurb -- for
>> Ard and myself -- that before merging this, we should regression-test it
>> on aarch64.
>>
>
> Will do
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-09 17:56 ` Laszlo Ersek
@ 2017-08-09 19:29 ` Laszlo Ersek
2017-08-11 22:22 ` Brijesh Singh
1 sibling, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 19:29 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel, Tom Lendacky
On 08/09/17 19:56, Laszlo Ersek wrote:
> Build instructions for ArmVirtQemu, and usage hints for the QEMU command
> line, can be found in the Linaro Wiki:
>
> https://wiki.linaro.org/LEG/UEFIforQEMU
It's visible in the Wiki article, but I'd like to point it out
nevertheless, that you can select the virtio-mmio transport for a given
virtio device model by typing
-device virtio-whatever-device
rather than
-device virtio-whatever-pci
Thanks
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-09 17:56 ` Laszlo Ersek
2017-08-09 19:29 ` Laszlo Ersek
@ 2017-08-11 22:22 ` Brijesh Singh
2017-08-15 10:42 ` Laszlo Ersek
1 sibling, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-11 22:22 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/09/2017 12:56 PM, Laszlo Ersek wrote:
>>
>> Sure, I will make the necessary changes in VirtioMmioDeviceLib and try
>> do the build test but I don't have aarch64 platform to verify at the
>> runtime.
>
> Actually, dependent on your GNU/Linux distribution, it is pretty easy to
> do on x86_64 too. It comes together from two parts:
>
> (1) installing an aarch64 cross compiler
> (2) installing qemu-system-aarch64 (from source or distro package),
> and then either using it directly (from the cmdline) or with the
> libvirt toolstack
>
> The only "real" difference is that it's going to use TCG and not KVM
> (software emulation rather than hardware virtualization), so it will be
> slower, but that's not really a problem if you only care about your VM
> until the firmware boots the OS :)
>
> On Fedora, the cross-compiler (and cross-binutils) packages are built
> from the following SRPMs:
>
> https://koji.fedoraproject.org/koji/buildinfo?buildID=921790
> https://koji.fedoraproject.org/koji/buildinfo?buildID=912429
>
> ("cross-gcc", "cross-binutils")
>
> Linaro distributes distro-independent cross compilers:
>
> http://www.linaro.org/downloads/
>
> Build instructions for ArmVirtQemu, and usage hints for the QEMU command
> line, can be found in the Linaro Wiki:
>
> https://wiki.linaro.org/LEG/UEFIforQEMU
v2 is almost ready for review, I have made all the necessary changes in
VirtioMmioDeviceLib and it builds fine for aarch64 but I am having trouble
booting the qemu-aarch64 in general. On serial console I see the UEFI debug
messages but it never reaches to UEFI shell.
I have been following the steps from https://wiki.linaro.org/LEG/UEFIforQEMU
qemu-system-aarch64 \
-m 1024 \
-cpu cortex-a57 \
-M virt \
-bios QEMU_EFI.fd \
-serial stdio
I tried this steps with and without my patches and it resulted in the same.
It seems like I am missing something in the qemu cli, do I need to pass
special dtb file or something similar ?
P.S: the wiki talks about a prebuilt qcow2 image but the link is dead, instead
I downloaded a qcow2 file from Ubuntu repo.
-Brijesh
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-11 22:22 ` Brijesh Singh
@ 2017-08-15 10:42 ` Laszlo Ersek
2017-08-15 19:32 ` Brijesh Singh
0 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-15 10:42 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/12/17 00:22, Brijesh Singh wrote:
>
>
> On 08/09/2017 12:56 PM, Laszlo Ersek wrote:
>
>>>
>>> Sure, I will make the necessary changes in VirtioMmioDeviceLib and try
>>> do the build test but I don't have aarch64 platform to verify at the
>>> runtime.
>>
>> Actually, dependent on your GNU/Linux distribution, it is pretty easy to
>> do on x86_64 too. It comes together from two parts:
>>
>> (1) installing an aarch64 cross compiler
>> (2) installing qemu-system-aarch64 (from source or distro package),
>> and then either using it directly (from the cmdline) or with the
>> libvirt toolstack
>>
>> The only "real" difference is that it's going to use TCG and not KVM
>> (software emulation rather than hardware virtualization), so it will be
>> slower, but that's not really a problem if you only care about your VM
>> until the firmware boots the OS :)
>>
>> On Fedora, the cross-compiler (and cross-binutils) packages are built
>> from the following SRPMs:
>>
>> https://koji.fedoraproject.org/koji/buildinfo?buildID=921790
>> https://koji.fedoraproject.org/koji/buildinfo?buildID=912429
>>
>> ("cross-gcc", "cross-binutils")
>>
>> Linaro distributes distro-independent cross compilers:
>>
>> http://www.linaro.org/downloads/
>>
>> Build instructions for ArmVirtQemu, and usage hints for the QEMU command
>> line, can be found in the Linaro Wiki:
>>
>> https://wiki.linaro.org/LEG/UEFIforQEMU
>
> v2 is almost ready for review, I have made all the necessary changes in
> VirtioMmioDeviceLib and it builds fine for aarch64 but I am having trouble
> booting the qemu-aarch64 in general. On serial console I see the UEFI debug
> messages but it never reaches to UEFI shell.
>
> I have been following the steps from
> https://wiki.linaro.org/LEG/UEFIforQEMU
>
> qemu-system-aarch64 \
> -m 1024 \
> -cpu cortex-a57 \
> -M virt \
> -bios QEMU_EFI.fd \
> -serial stdio
>
> I tried this steps with and without my patches and it resulted in the same.
> It seems like I am missing something in the qemu cli, do I need to pass
> special dtb file or something similar ?
The above command line is not right ("-bios"). Please scroll down the
wiki page, to the section heading saying "Using persistent UEFI
variables". There it explains how to pad the images and how to use two
-pflash options. ... Perhaps even that part of the article is a bit
out-of-date now.
Basically, today ArmVirtQemu should be used the same way as OVMF, except
for the padding. The build produces two files:
- QEMU_EFI.fd (fw binary)
- QEMU_VARS.fd (varstore template)
Each should be padded to 64MiB with zeros at the end (write a small
script for that), then use them with two pflash drives similarly to OVMF.
Thanks!
Laszlo
> P.S: the wiki talks about a prebuilt qcow2 image but the link is dead,
> instead
> I downloaded a qcow2 file from Ubuntu repo.
>
> -Brijesh
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-15 10:42 ` Laszlo Ersek
@ 2017-08-15 19:32 ` Brijesh Singh
2017-08-15 19:48 ` Laszlo Ersek
0 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-15 19:32 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel
Hi Laszlo,
On 08/15/2017 05:42 AM, Laszlo Ersek wrote:
[snip]
>>
>> I have been following the steps from
>> https://wiki.linaro.org/LEG/UEFIforQEMU
>>
>> qemu-system-aarch64 \
>> -m 1024 \
>> -cpu cortex-a57 \
>> -M virt \
>> -bios QEMU_EFI.fd \
>> -serial stdio
>>
>> I tried this steps with and without my patches and it resulted in the same.
>> It seems like I am missing something in the qemu cli, do I need to pass
>> special dtb file or something similar ?
>
> The above command line is not right ("-bios"). Please scroll down the
> wiki page, to the section heading saying "Using persistent UEFI
> variables". There it explains how to pad the images and how to use two
> -pflash options. ... Perhaps even that part of the article is a bit
> out-of-date now.
>
> Basically, today ArmVirtQemu should be used the same way as OVMF, except
> for the padding. The build produces two files:
> - QEMU_EFI.fd (fw binary)
> - QEMU_VARS.fd (varstore template)
>
> Each should be padded to 64MiB with zeros at the end (write a small
> script for that), then use them with two pflash drives similarly to OVMF.
>
Still no luck, you can see my log error [1]. I never get to UEFI shell, I have
tried with and without virtio disk.
https://gist.github.com/codomania/0aed024702b817761ee55fd30929200a
I will continuing googling ...
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-15 19:32 ` Brijesh Singh
@ 2017-08-15 19:48 ` Laszlo Ersek
2017-08-15 20:26 ` Brijesh Singh
0 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-15 19:48 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel, Tom Lendacky
On 08/15/17 21:32, Brijesh Singh wrote:
> Hi Laszlo,
>
> On 08/15/2017 05:42 AM, Laszlo Ersek wrote:
> [snip]
>
>>>
>>> I have been following the steps from
>>> https://wiki.linaro.org/LEG/UEFIforQEMU
>>>
>>> qemu-system-aarch64 \
>>> -m 1024 \
>>> -cpu cortex-a57 \
>>> -M virt \
>>> -bios QEMU_EFI.fd \
>>> -serial stdio
>>>
>>> I tried this steps with and without my patches and it resulted in the
>>> same.
>>> It seems like I am missing something in the qemu cli, do I need to pass
>>> special dtb file or something similar ?
>>
>> The above command line is not right ("-bios"). Please scroll down the
>> wiki page, to the section heading saying "Using persistent UEFI
>> variables". There it explains how to pad the images and how to use two
>> -pflash options. ... Perhaps even that part of the article is a bit
>> out-of-date now.
>>
>> Basically, today ArmVirtQemu should be used the same way as OVMF, except
>> for the padding. The build produces two files:
>> - QEMU_EFI.fd (fw binary)
>> - QEMU_VARS.fd (varstore template)
>>
>> Each should be padded to 64MiB with zeros at the end (write a small
>> script for that), then use them with two pflash drives similarly to OVMF.
>>
>
> Still no luck, you can see my log error [1]. I never get to UEFI shell,
> I have
> tried with and without virtio disk.
>
> https://gist.github.com/codomania/0aed024702b817761ee55fd30929200a
>
> I will continuing googling ...
In order to get as detailed as possible logs, I suggest adding the
following option to the ArmVirtQemu build command line:
-D DEBUG_PRINT_ERROR_LEVEL=0x8040004F
The current log looks quite strange to me in places, but I'm not sure if
that's because there are problems in those parts, or because the log
does not contain DEBUG_VERBOSE entries.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-15 19:48 ` Laszlo Ersek
@ 2017-08-15 20:26 ` Brijesh Singh
2017-08-15 20:39 ` Laszlo Ersek
0 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-15 20:26 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Ard Biesheuvel, Tom Lendacky
On 08/15/2017 02:48 PM, Laszlo Ersek wrote:
> On 08/15/17 21:32, Brijesh Singh wrote:
>> Hi Laszlo,
>>
>> On 08/15/2017 05:42 AM, Laszlo Ersek wrote:
>> [snip]
>>
>>>>
>>>> I have been following the steps from
>>>> https://wiki.linaro.org/LEG/UEFIforQEMU
>>>>
>>>> qemu-system-aarch64 \
>>>> -m 1024 \
>>>> -cpu cortex-a57 \
>>>> -M virt \
>>>> -bios QEMU_EFI.fd \
>>>> -serial stdio
>>>>
>>>> I tried this steps with and without my patches and it resulted in the
>>>> same.
>>>> It seems like I am missing something in the qemu cli, do I need to pass
>>>> special dtb file or something similar ?
>>>
>>> The above command line is not right ("-bios"). Please scroll down the
>>> wiki page, to the section heading saying "Using persistent UEFI
>>> variables". There it explains how to pad the images and how to use two
>>> -pflash options. ... Perhaps even that part of the article is a bit
>>> out-of-date now.
>>>
>>> Basically, today ArmVirtQemu should be used the same way as OVMF, except
>>> for the padding. The build produces two files:
>>> - QEMU_EFI.fd (fw binary)
>>> - QEMU_VARS.fd (varstore template)
>>>
>>> Each should be padded to 64MiB with zeros at the end (write a small
>>> script for that), then use them with two pflash drives similarly to OVMF.
>>>
>>
>> Still no luck, you can see my log error [1]. I never get to UEFI shell,
>> I have
>> tried with and without virtio disk.
>>
>> https://gist.github.com/codomania/0aed024702b817761ee55fd30929200a
>>
>> I will continuing googling ...
>
> In order to get as detailed as possible logs, I suggest adding the
> following option to the ArmVirtQemu build command line:
>
> -D DEBUG_PRINT_ERROR_LEVEL=0x8040004F
>
> The current log looks quite strange to me in places, but I'm not sure if
> that's because there are problems in those parts, or because the log
> does not contain DEBUG_VERBOSE entries.
>
https://gist.github.com/codomania/8b2fc5424fda259236405c5e257d8f47
I am using Ubuntu 16.04 for builds and runs
$ qemu-system-aarch64 --version
QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.14), Copyright (c) 2003-2008 Fabrice Bellard
-Brijesh
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-15 20:26 ` Brijesh Singh
@ 2017-08-15 20:39 ` Laszlo Ersek
2017-08-15 20:44 ` Brijesh Singh
0 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-15 20:39 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel, Tom Lendacky
On 08/15/17 22:26, Brijesh Singh wrote:
>
>
> On 08/15/2017 02:48 PM, Laszlo Ersek wrote:
>> On 08/15/17 21:32, Brijesh Singh wrote:
>>> Hi Laszlo,
>>>
>>> On 08/15/2017 05:42 AM, Laszlo Ersek wrote:
>>> [snip]
>>>
>>>>>
>>>>> I have been following the steps from
>>>>> https://wiki.linaro.org/LEG/UEFIforQEMU
>>>>>
>>>>> qemu-system-aarch64 \
>>>>> -m 1024 \
>>>>> -cpu cortex-a57 \
>>>>> -M virt \
>>>>> -bios QEMU_EFI.fd \
>>>>> -serial stdio
>>>>>
>>>>> I tried this steps with and without my patches and it resulted in the
>>>>> same.
>>>>> It seems like I am missing something in the qemu cli, do I need to
>>>>> pass
>>>>> special dtb file or something similar ?
>>>>
>>>> The above command line is not right ("-bios"). Please scroll down the
>>>> wiki page, to the section heading saying "Using persistent UEFI
>>>> variables". There it explains how to pad the images and how to use two
>>>> -pflash options. ... Perhaps even that part of the article is a bit
>>>> out-of-date now.
>>>>
>>>> Basically, today ArmVirtQemu should be used the same way as OVMF,
>>>> except
>>>> for the padding. The build produces two files:
>>>> - QEMU_EFI.fd (fw binary)
>>>> - QEMU_VARS.fd (varstore template)
>>>>
>>>> Each should be padded to 64MiB with zeros at the end (write a small
>>>> script for that), then use them with two pflash drives similarly to
>>>> OVMF.
>>>>
>>>
>>> Still no luck, you can see my log error [1]. I never get to UEFI shell,
>>> I have
>>> tried with and without virtio disk.
>>>
>>> https://gist.github.com/codomania/0aed024702b817761ee55fd30929200a
>>>
>>> I will continuing googling ...
>>
>> In order to get as detailed as possible logs, I suggest adding the
>> following option to the ArmVirtQemu build command line:
>>
>> -D DEBUG_PRINT_ERROR_LEVEL=0x8040004F
>>
>> The current log looks quite strange to me in places, but I'm not sure if
>> that's because there are problems in those parts, or because the log
>> does not contain DEBUG_VERBOSE entries.
>>
>
>
> https://gist.github.com/codomania/8b2fc5424fda259236405c5e257d8f47
>
> I am using Ubuntu 16.04 for builds and runs
>
> $ qemu-system-aarch64 --version
> QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.14), Copyright
> (c) 2003-2008 Fabrice Bellard
What is your complete QEMU command line?
Thanks
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-15 20:39 ` Laszlo Ersek
@ 2017-08-15 20:44 ` Brijesh Singh
2017-08-15 21:57 ` Laszlo Ersek
0 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-15 20:44 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Ard Biesheuvel, Tom Lendacky
On 08/15/2017 03:39 PM, Laszlo Ersek wrote:
> On 08/15/17 22:26, Brijesh Singh wrote:
>>
>>
>> On 08/15/2017 02:48 PM, Laszlo Ersek wrote:
>>> On 08/15/17 21:32, Brijesh Singh wrote:
>>>> Hi Laszlo,
>>>>
>>>> On 08/15/2017 05:42 AM, Laszlo Ersek wrote:
>>>> [snip]
>>>>
>>>>>>
>>>>>> I have been following the steps from
>>>>>> https://wiki.linaro.org/LEG/UEFIforQEMU
>>>>>>
>>>>>> qemu-system-aarch64 \
>>>>>> -m 1024 \
>>>>>> -cpu cortex-a57 \
>>>>>> -M virt \
>>>>>> -bios QEMU_EFI.fd \
>>>>>> -serial stdio
>>>>>>
>>>>>> I tried this steps with and without my patches and it resulted in the
>>>>>> same.
>>>>>> It seems like I am missing something in the qemu cli, do I need to
>>>>>> pass
>>>>>> special dtb file or something similar ?
>>>>>
>>>>> The above command line is not right ("-bios"). Please scroll down the
>>>>> wiki page, to the section heading saying "Using persistent UEFI
>>>>> variables". There it explains how to pad the images and how to use two
>>>>> -pflash options. ... Perhaps even that part of the article is a bit
>>>>> out-of-date now.
>>>>>
>>>>> Basically, today ArmVirtQemu should be used the same way as OVMF,
>>>>> except
>>>>> for the padding. The build produces two files:
>>>>> - QEMU_EFI.fd (fw binary)
>>>>> - QEMU_VARS.fd (varstore template)
>>>>>
>>>>> Each should be padded to 64MiB with zeros at the end (write a small
>>>>> script for that), then use them with two pflash drives similarly to
>>>>> OVMF.
>>>>>
>>>>
>>>> Still no luck, you can see my log error [1]. I never get to UEFI shell,
>>>> I have
>>>> tried with and without virtio disk.
>>>>
>>>> https://gist.github.com/codomania/0aed024702b817761ee55fd30929200a
>>>>
>>>> I will continuing googling ...
>>>
>>> In order to get as detailed as possible logs, I suggest adding the
>>> following option to the ArmVirtQemu build command line:
>>>
>>> -D DEBUG_PRINT_ERROR_LEVEL=0x8040004F
>>>
>>> The current log looks quite strange to me in places, but I'm not sure if
>>> that's because there are problems in those parts, or because the log
>>> does not contain DEBUG_VERBOSE entries.
>>>
>>
>>
>> https://gist.github.com/codomania/8b2fc5424fda259236405c5e257d8f47
>>
>> I am using Ubuntu 16.04 for builds and runs
>>
>> $ qemu-system-aarch64 --version
>> QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.14), Copyright
>> (c) 2003-2008 Fabrice Bellard
>
> What is your complete QEMU command line?
>
I have been using the following two qemu cli
# qemu-system-aarch64 -m 2048 -cpu cortex-a57 -M virt \
-pflash flash0.img -pflash flash1.img \
-nographic
# qemu-system-aarch64 -m 2048 -cpu cortex-a57 -M virt \
-pflash flash0.img -pflash flash1.img \
-drive if=none,file=/home/brijesh/xenial-server-cloudimg-arm64.img,id=hd0,format=raw -device virtio-blk-device,drive=hd0 \
-nographic
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-15 20:44 ` Brijesh Singh
@ 2017-08-15 21:57 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-15 21:57 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/15/17 22:44, Brijesh Singh wrote:
>
>
> On 08/15/2017 03:39 PM, Laszlo Ersek wrote:
>> On 08/15/17 22:26, Brijesh Singh wrote:
>>>
>>>
>>> On 08/15/2017 02:48 PM, Laszlo Ersek wrote:
>>>> On 08/15/17 21:32, Brijesh Singh wrote:
>>>>> Hi Laszlo,
>>>>>
>>>>> On 08/15/2017 05:42 AM, Laszlo Ersek wrote:
>>>>> [snip]
>>>>>
>>>>>>>
>>>>>>> I have been following the steps from
>>>>>>> https://wiki.linaro.org/LEG/UEFIforQEMU
>>>>>>>
>>>>>>> qemu-system-aarch64 \
>>>>>>> -m 1024 \
>>>>>>> -cpu cortex-a57 \
>>>>>>> -M virt \
>>>>>>> -bios QEMU_EFI.fd \
>>>>>>> -serial stdio
>>>>>>>
>>>>>>> I tried this steps with and without my patches and it resulted in
>>>>>>> the
>>>>>>> same.
>>>>>>> It seems like I am missing something in the qemu cli, do I need to
>>>>>>> pass
>>>>>>> special dtb file or something similar ?
>>>>>>
>>>>>> The above command line is not right ("-bios"). Please scroll down the
>>>>>> wiki page, to the section heading saying "Using persistent UEFI
>>>>>> variables". There it explains how to pad the images and how to use
>>>>>> two
>>>>>> -pflash options. ... Perhaps even that part of the article is a bit
>>>>>> out-of-date now.
>>>>>>
>>>>>> Basically, today ArmVirtQemu should be used the same way as OVMF,
>>>>>> except
>>>>>> for the padding. The build produces two files:
>>>>>> - QEMU_EFI.fd (fw binary)
>>>>>> - QEMU_VARS.fd (varstore template)
>>>>>>
>>>>>> Each should be padded to 64MiB with zeros at the end (write a small
>>>>>> script for that), then use them with two pflash drives similarly to
>>>>>> OVMF.
>>>>>>
>>>>>
>>>>> Still no luck, you can see my log error [1]. I never get to UEFI
>>>>> shell,
>>>>> I have
>>>>> tried with and without virtio disk.
>>>>>
>>>>> https://gist.github.com/codomania/0aed024702b817761ee55fd30929200a
>>>>>
>>>>> I will continuing googling ...
>>>>
>>>> In order to get as detailed as possible logs, I suggest adding the
>>>> following option to the ArmVirtQemu build command line:
>>>>
>>>> -D DEBUG_PRINT_ERROR_LEVEL=0x8040004F
>>>>
>>>> The current log looks quite strange to me in places, but I'm not
>>>> sure if
>>>> that's because there are problems in those parts, or because the log
>>>> does not contain DEBUG_VERBOSE entries.
>>>>
>>>
>>>
>>> https://gist.github.com/codomania/8b2fc5424fda259236405c5e257d8f47
>>>
>>> I am using Ubuntu 16.04 for builds and runs
>>>
>>> $ qemu-system-aarch64 --version
>>> QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.14), Copyright
>>> (c) 2003-2008 Fabrice Bellard
>>
>> What is your complete QEMU command line?
>>
>
> I have been using the following two qemu cli
>
> # qemu-system-aarch64 -m 2048 -cpu cortex-a57 -M virt \
> -pflash flash0.img -pflash flash1.img \
> -nographic
>
> # qemu-system-aarch64 -m 2048 -cpu cortex-a57 -M virt \
> -pflash flash0.img -pflash flash1.img \
> -drive
> if=none,file=/home/brijesh/xenial-server-cloudimg-arm64.img,id=hd0,format=raw
> -device virtio-blk-device,drive=hd0 \
> -nographic
I can reproduce the boot hang. It looks like an edk2 regression. I'm
currently bisecting the issue.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-07 11:58 [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Brijesh Singh
` (14 preceding siblings ...)
2017-08-09 14:39 ` [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address Laszlo Ersek
@ 2017-08-09 22:38 ` Laszlo Ersek
2017-08-09 22:44 ` Brijesh Singh
15 siblings, 1 reply; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-09 22:38 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/07/17 13:58, Brijesh Singh wrote:
> Currently, virtio drivers provides the system physical address to the device.
> However, some systems may feature an IOMMU that requires the drivers to pass
> the device addresses to the device - which are then translated by the IOMMU
> into physical addresses in memory. The patch series introduces new member
> functions in VIRTIO_DEVICE_PROTOCOL which can be used for mapping a system
> physical address to device address.
>
> The approach that this patch series takes is to maps the system physical
> address to device address for buffers (including rings, device specifc
> request and response pointed by vring descriptor, and any further memory
> reference by those request and response).
>
> Patch 1 - 3:
> Defines and implements new member functions to map a system physical address
> to device address. The patch implements Laszlo's suggestion [1].
>
> Patch 4 - 7:
> Allocates the vring buffer using newly added member functions and provides
> some helper functions.
>
> Patch 8:
> Update the virtio-blk driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device virtio-blk-pci,drive=disk0
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device virtio-blk-pci,drive=disk0,disable-legacy=on
>
> Patch 9:
> Update the virtio-scsi driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device scsi-hd,drive=disk0
> -device virtio-scsi-pci,id=scsi \
>
> # $QEMU \
> -drive file=${IMAGE},if=none,id=disk0 \
> -device scsi-hd,drive=disk0 \
> -device virtio-scsi-pci,id=scsi,disable-legacy=on \
>
> Patch 10 - 13:
> Update the virtio-net driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -netdev type=tap,id=net0 \
> -device virtio-net-pci,netdev=net0,romfile=
>
> # $QEMU \
> -netdev type=tap,id=net0 \
> -device virtio-net-pci,netdev=net0,disable-legacy=on,romfile=
>
> Patch 14:
> Update the virtio-rng driver to use newly added member functions to map the
> addresses.
> Verified using the following qemu cli
>
> # $QEMU \
> -device virtio-rng-pci
>
> # $QEMU \
> -device virtio-rng-pci,disable-legacy=on
>
> And succesfully ran RngTest.efi from SecurityPkg/Application
>
> Repo: https://github.com/codomania/edk2
> Branch: virtio-support
>
> 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>
>
> Brijesh Singh (14):
> OvmfPkg/Virtio: Introduce new member functions in
> VIRTIO_DEVICE_PROTOCOL
> OvmfPkg/Virtio10Dxe: Implement new member functions
> OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
> OvmfPkg/VirtioLib: Add SharedBuffer helper functions
> OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit()
> OvmfPkg/VirtioLib: Add functions to map/unmap VRING
> OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer
> OvmfPkg/VirtioBlkDxe: Use DeviceAddresses in vring descriptors
> OvmfPkg/VirtioScsiDxe: Use DeviceAddresses in vring descriptors
> OvmfPkg/VirtioNetDxe: Allocate Tx and Rx ring using
> AllocateSharedPage()
> OvmfPkg/VirtioNetDxe: Allocate RxBuf using AllocateSharedPages()
> OvmfPkg/VirtioNetDxe: Dynamically allocate transmit header
> OvmfPkg/VirtioNetDxe: Use DeviceAddress in transmit vring descriptors
> OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors
>
> OvmfPkg/Include/Library/VirtioLib.h | 198 ++++++++++++-
> OvmfPkg/Include/Protocol/VirtioDevice.h | 121 ++++++++
> OvmfPkg/VirtioBlkDxe/VirtioBlk.h | 1 +
> OvmfPkg/VirtioNetDxe/VirtioNet.h | 25 +-
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h | 34 +++
> OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 +
> OvmfPkg/VirtioScsiDxe/VirtioScsi.h | 1 +
> OvmfPkg/Library/VirtioLib/VirtioLib.c | 296 +++++++++++++++++++-
> OvmfPkg/Virtio10Dxe/Virtio10.c | 114 +++++++-
> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 104 ++++++-
> OvmfPkg/VirtioGpuDxe/Commands.c | 7 +-
> OvmfPkg/VirtioNetDxe/Events.c | 24 ++
> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 19 +-
> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 102 +++++--
> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 119 +++++++-
> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 16 +-
> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 22 +-
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 7 +-
> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 66 +++++
> OvmfPkg/VirtioRngDxe/VirtioRng.c | 54 +++-
> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 121 +++++++-
> 21 files changed, 1378 insertions(+), 74 deletions(-)
>
The conversion for VirtioGpuDxe seems to be missing; is it on your todo
list? (Not that it's urgent at all; in fact I'll suggest delaying the
more complex drivers in a minute.)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-09 22:38 ` Laszlo Ersek
@ 2017-08-09 22:44 ` Brijesh Singh
2017-08-10 9:53 ` Laszlo Ersek
0 siblings, 1 reply; 42+ messages in thread
From: Brijesh Singh @ 2017-08-09 22:44 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel
Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/09/2017 05:38 PM, Laszlo Ersek wrote:
> On 08/07/17 13:58, Brijesh Singh wrote:
>> Currently, virtio drivers provides the system physical address to the device.
>> However, some systems may feature an IOMMU that requires the drivers to pass
>> the device addresses to the device - which are then translated by the IOMMU
>> into physical addresses in memory. The patch series introduces new member
>> functions in VIRTIO_DEVICE_PROTOCOL which can be used for mapping a system
>> physical address to device address.
>>
>> The approach that this patch series takes is to maps the system physical
>> address to device address for buffers (including rings, device specifc
>> request and response pointed by vring descriptor, and any further memory
>> reference by those request and response).
>>
>> Patch 1 - 3:
>> Defines and implements new member functions to map a system physical address
>> to device address. The patch implements Laszlo's suggestion [1].
>>
>> Patch 4 - 7:
>> Allocates the vring buffer using newly added member functions and provides
>> some helper functions.
>>
>> Patch 8:
>> Update the virtio-blk driver to use newly added member functions to map the
>> addresses.
>> Verified using the following qemu cli
>>
>> # $QEMU \
>> -drive file=${IMAGE},if=none,id=disk0 \
>> -device virtio-blk-pci,drive=disk0
>>
>> # $QEMU \
>> -drive file=${IMAGE},if=none,id=disk0 \
>> -device virtio-blk-pci,drive=disk0,disable-legacy=on
>>
>> Patch 9:
>> Update the virtio-scsi driver to use newly added member functions to map the
>> addresses.
>> Verified using the following qemu cli
>>
>> # $QEMU \
>> -drive file=${IMAGE},if=none,id=disk0 \
>> -device scsi-hd,drive=disk0
>> -device virtio-scsi-pci,id=scsi \
>>
>> # $QEMU \
>> -drive file=${IMAGE},if=none,id=disk0 \
>> -device scsi-hd,drive=disk0 \
>> -device virtio-scsi-pci,id=scsi,disable-legacy=on \
>>
>> Patch 10 - 13:
>> Update the virtio-net driver to use newly added member functions to map the
>> addresses.
>> Verified using the following qemu cli
>>
>> # $QEMU \
>> -netdev type=tap,id=net0 \
>> -device virtio-net-pci,netdev=net0,romfile=
>>
>> # $QEMU \
>> -netdev type=tap,id=net0 \
>> -device virtio-net-pci,netdev=net0,disable-legacy=on,romfile=
>>
>> Patch 14:
>> Update the virtio-rng driver to use newly added member functions to map the
>> addresses.
>> Verified using the following qemu cli
>>
>> # $QEMU \
>> -device virtio-rng-pci
>>
>> # $QEMU \
>> -device virtio-rng-pci,disable-legacy=on
>>
>> And succesfully ran RngTest.efi from SecurityPkg/Application
>>
>> Repo: https://github.com/codomania/edk2
>> Branch: virtio-support
>>
>> 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>
>>
>> Brijesh Singh (14):
>> OvmfPkg/Virtio: Introduce new member functions in
>> VIRTIO_DEVICE_PROTOCOL
>> OvmfPkg/Virtio10Dxe: Implement new member functions
>> OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
>> OvmfPkg/VirtioLib: Add SharedBuffer helper functions
>> OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit()
>> OvmfPkg/VirtioLib: Add functions to map/unmap VRING
>> OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer
>> OvmfPkg/VirtioBlkDxe: Use DeviceAddresses in vring descriptors
>> OvmfPkg/VirtioScsiDxe: Use DeviceAddresses in vring descriptors
>> OvmfPkg/VirtioNetDxe: Allocate Tx and Rx ring using
>> AllocateSharedPage()
>> OvmfPkg/VirtioNetDxe: Allocate RxBuf using AllocateSharedPages()
>> OvmfPkg/VirtioNetDxe: Dynamically allocate transmit header
>> OvmfPkg/VirtioNetDxe: Use DeviceAddress in transmit vring descriptors
>> OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors
>>
>> OvmfPkg/Include/Library/VirtioLib.h | 198 ++++++++++++-
>> OvmfPkg/Include/Protocol/VirtioDevice.h | 121 ++++++++
>> OvmfPkg/VirtioBlkDxe/VirtioBlk.h | 1 +
>> OvmfPkg/VirtioNetDxe/VirtioNet.h | 25 +-
>> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h | 34 +++
>> OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 +
>> OvmfPkg/VirtioScsiDxe/VirtioScsi.h | 1 +
>> OvmfPkg/Library/VirtioLib/VirtioLib.c | 296 +++++++++++++++++++-
>> OvmfPkg/Virtio10Dxe/Virtio10.c | 114 +++++++-
>> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 104 ++++++-
>> OvmfPkg/VirtioGpuDxe/Commands.c | 7 +-
>> OvmfPkg/VirtioNetDxe/Events.c | 24 ++
>> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 19 +-
>> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 102 +++++--
>> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 119 +++++++-
>> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 16 +-
>> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 22 +-
>> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 7 +-
>> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 66 +++++
>> OvmfPkg/VirtioRngDxe/VirtioRng.c | 54 +++-
>> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 121 +++++++-
>> 21 files changed, 1378 insertions(+), 74 deletions(-)
>>
>
> The conversion for VirtioGpuDxe seems to be missing; is it on your todo
> list? (Not that it's urgent at all; in fact I'll suggest delaying the
> more complex drivers in a minute.)
>
Actually I was thinking to delay it for now, once we get a basic support
enabled then we can revisit the VirtioGpuDxe, I still need to figure out
qemu cli and Uefi app to test it. I will add in TODO list so that.
> Thanks,
> Laszlo
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v1 00/14] OvmfPkg/Virtio: Add APIs to map system physical to device address
2017-08-09 22:44 ` Brijesh Singh
@ 2017-08-10 9:53 ` Laszlo Ersek
0 siblings, 0 replies; 42+ messages in thread
From: Laszlo Ersek @ 2017-08-10 9:53 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel
On 08/10/17 00:44, Brijesh Singh wrote:
>
>
> On 08/09/2017 05:38 PM, Laszlo Ersek wrote:
>> On 08/07/17 13:58, Brijesh Singh wrote:
>>> Currently, virtio drivers provides the system physical address to the
>>> device.
>>> However, some systems may feature an IOMMU that requires the drivers
>>> to pass
>>> the device addresses to the device - which are then translated by the
>>> IOMMU
>>> into physical addresses in memory. The patch series introduces new
>>> member
>>> functions in VIRTIO_DEVICE_PROTOCOL which can be used for mapping a
>>> system
>>> physical address to device address.
>>>
>>> The approach that this patch series takes is to maps the system physical
>>> address to device address for buffers (including rings, device specifc
>>> request and response pointed by vring descriptor, and any further
>>> memory
>>> reference by those request and response).
>>>
>>> Patch 1 - 3:
>>> Defines and implements new member functions to map a system
>>> physical address
>>> to device address. The patch implements Laszlo's suggestion [1].
>>>
>>> Patch 4 - 7:
>>> Allocates the vring buffer using newly added member functions and
>>> provides
>>> some helper functions.
>>>
>>> Patch 8:
>>> Update the virtio-blk driver to use newly added member functions to
>>> map the
>>> addresses.
>>> Verified using the following qemu cli
>>>
>>> # $QEMU \
>>> -drive file=${IMAGE},if=none,id=disk0 \
>>> -device virtio-blk-pci,drive=disk0
>>>
>>> # $QEMU \
>>> -drive file=${IMAGE},if=none,id=disk0 \
>>> -device virtio-blk-pci,drive=disk0,disable-legacy=on
>>>
>>> Patch 9:
>>> Update the virtio-scsi driver to use newly added member functions
>>> to map the
>>> addresses.
>>> Verified using the following qemu cli
>>>
>>> # $QEMU \
>>> -drive file=${IMAGE},if=none,id=disk0 \
>>> -device scsi-hd,drive=disk0
>>> -device virtio-scsi-pci,id=scsi \
>>>
>>> # $QEMU \
>>> -drive file=${IMAGE},if=none,id=disk0 \
>>> -device scsi-hd,drive=disk0 \
>>> -device virtio-scsi-pci,id=scsi,disable-legacy=on \
>>>
>>> Patch 10 - 13:
>>> Update the virtio-net driver to use newly added member functions to
>>> map the
>>> addresses.
>>> Verified using the following qemu cli
>>>
>>> # $QEMU \
>>> -netdev type=tap,id=net0 \
>>> -device virtio-net-pci,netdev=net0,romfile=
>>>
>>> # $QEMU \
>>> -netdev type=tap,id=net0 \
>>> -device virtio-net-pci,netdev=net0,disable-legacy=on,romfile=
>>>
>>> Patch 14:
>>> Update the virtio-rng driver to use newly added member functions to
>>> map the
>>> addresses.
>>> Verified using the following qemu cli
>>>
>>> # $QEMU \
>>> -device virtio-rng-pci
>>>
>>> # $QEMU \
>>> -device virtio-rng-pci,disable-legacy=on
>>>
>>> And succesfully ran RngTest.efi from SecurityPkg/Application
>>>
>>> Repo: https://github.com/codomania/edk2
>>> Branch: virtio-support
>>>
>>> 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>
>>>
>>> Brijesh Singh (14):
>>> OvmfPkg/Virtio: Introduce new member functions in
>>> VIRTIO_DEVICE_PROTOCOL
>>> OvmfPkg/Virtio10Dxe: Implement new member functions
>>> OvmfPkg/VirtioPciDeviceDxe: Implement new member functions
>>> OvmfPkg/VirtioLib: Add SharedBuffer helper functions
>>> OvmfPkg/VirtioLib: Pass VirtIo instance in VringInit/Uinit()
>>> OvmfPkg/VirtioLib: Add functions to map/unmap VRING
>>> OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer
>>> OvmfPkg/VirtioBlkDxe: Use DeviceAddresses in vring descriptors
>>> OvmfPkg/VirtioScsiDxe: Use DeviceAddresses in vring descriptors
>>> OvmfPkg/VirtioNetDxe: Allocate Tx and Rx ring using
>>> AllocateSharedPage()
>>> OvmfPkg/VirtioNetDxe: Allocate RxBuf using AllocateSharedPages()
>>> OvmfPkg/VirtioNetDxe: Dynamically allocate transmit header
>>> OvmfPkg/VirtioNetDxe: Use DeviceAddress in transmit vring descriptors
>>> OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors
>>>
>>> OvmfPkg/Include/Library/VirtioLib.h | 198 ++++++++++++-
>>> OvmfPkg/Include/Protocol/VirtioDevice.h | 121 ++++++++
>>> OvmfPkg/VirtioBlkDxe/VirtioBlk.h | 1 +
>>> OvmfPkg/VirtioNetDxe/VirtioNet.h | 25 +-
>>> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h | 34 +++
>>> OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 +
>>> OvmfPkg/VirtioScsiDxe/VirtioScsi.h | 1 +
>>> OvmfPkg/Library/VirtioLib/VirtioLib.c | 296
>>> +++++++++++++++++++-
>>> OvmfPkg/Virtio10Dxe/Virtio10.c | 114 +++++++-
>>> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 104 ++++++-
>>> OvmfPkg/VirtioGpuDxe/Commands.c | 7 +-
>>> OvmfPkg/VirtioNetDxe/Events.c | 24 ++
>>> OvmfPkg/VirtioNetDxe/SnpGetStatus.c | 19 +-
>>> OvmfPkg/VirtioNetDxe/SnpInitialize.c | 102 +++++--
>>> OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c | 119 +++++++-
>>> OvmfPkg/VirtioNetDxe/SnpShutdown.c | 16 +-
>>> OvmfPkg/VirtioNetDxe/SnpTransmit.c | 22 +-
>>> OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c | 7 +-
>>> OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c | 66 +++++
>>> OvmfPkg/VirtioRngDxe/VirtioRng.c | 54 +++-
>>> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 121 +++++++-
>>> 21 files changed, 1378 insertions(+), 74 deletions(-)
>>>
>>
>> The conversion for VirtioGpuDxe seems to be missing; is it on your todo
>> list? (Not that it's urgent at all; in fact I'll suggest delaying the
>> more complex drivers in a minute.)
>>
>
> Actually I was thinking to delay it for now,
OK, that makes sense even from the complexity standpoint.
> once we get a basic support
> enabled then we can revisit the VirtioGpuDxe, I still need to figure out
> qemu cli and Uefi app to test it.
You can read about my original test cases and test instructions in the
following message:
[2] http://mid.mail-archive.com/20160819124932.29711-1-lersek@redhat.com
(
Some background:
The virtio-vga device combines the Virtio GPU and the traditional VGA
framebuffer that is also seen on QXL and stdvga ("Bochs"). If you use
"-device virtio-vga", then QemuVideoDxe will bind the device (this is
intended). So, the right option for specifically using the virtio GPU is
"-device virtio-gpu-pci".
You can read more about this distinction in the following libvirt commit:
[3] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=706b5b627719
)
Regarding a UEFI app for testing, the UEFI shell and grub suffice:
- do you see the TianoCore boot logo?
- does the UEFI shell work?
- does the shell scroll fine when you press Enter at the bottom of the
screen?
- does the MODE command work in the shell?
- typing on the serial port, can you disconnect / reconnect the display?
- does grub work?
See again my message [2].
> I will add in TODO list so that.
Sounds good. Once we lay down the foundation (and I can find some time),
I might be able to take on the conversion of VirtioGpuDxe. (If you're OK
with that.)
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 42+ messages in thread