* [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller
@ 2020-03-28 20:00 Liran Alon
2020-03-28 20:00 ` [PATCH v3 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
` (18 more replies)
0 siblings, 19 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
Hi,
This series adds driver support for VMware PVSCSI controller.
This controller is supported by VMware and QEMU. This work is part of
the more general agenda of enhancing OVMF boot device support to have
feature parity with SeaBIOS (Which supports booting from VMware PVSCSI).
I pushed a copy of these (v3) patches to https://github.com/nikital/edk2/tree/pvscsi6
The v2 patches can be found at https://github.com/nikital/edk2/tree/pvscsi5
The v1 patches can be found at https://github.com/nikital/edk2/tree/pvscsi4
Regards,
-Liran
v2->v3:
* Add function documentation to PvScsiWriteCmdDesc with “@param” to explain DescWords alignment requirement. [Laszlo]
* Also set Packet’s HostAdapterStatus and TargetStatus when returning EFI_BAD_BUFFER_SIZE from PassThru() method. [Liran]
* Add comments explaining why DMA communication buffer fields sizes are defined as they are. [Laszlo]
* Remove unnecessary (UINT64) casts from EFI_PHYSICAL_ADDRESS expressions. [Laszlo]
* Changed HandleResponse() to always copy SenseData based on Response->SenseLen. Not only if ScsiStatus equal to CHECK. [Liran]
* Changed HandleResponse() to update Packet TransferLength only on underrun. [Liran]
* Changed PassThru() to return EFI_STATUS_SUCCESS on device return overrun/underrun. [Laszlo & Liran]
* Removed unnecessary PvScsiAllocatePages(), PvScsiFreePages(), PvScsiMapBuffer() and PvScsiUnmapBuffer() utility functions. [Laszlo]
* Changed PvScsiInitRings() to align PVSCSI_CMD_DESC_SETUP_RINGS command to UINT32 before using it with EfiPciIoWidthFifoUint32. [Laszlo]
* Removed PciIoOperation parameter from PvScsiAllocateSharedPages(). [Laszlo
* Added STATIC_ASSERT() to verify PVSCSI_CMD_DESC_SETUP_RINGS is of size multiple of UINT32 words [Laszlo].
* Added reset device before either freeing PVSCSI rings or DMA communication buffer. [Laszlo]
* Removed unnecessary cast to (VOID **) in call to PvScsiFreeSharedPages() in PvScsiUninit(). [Laszlo]
* Added #include <Library/BaseLib.h> and BaseLib to PvScsiDxe.inf because of RShiftU64() usage. [Laszlo]
v1->v2:
* Removed Nikita’s Reviewed-By tags. [Laszlo]
* Renamed PvScsi.inf to PvScsiDxe.inf and fixed references from all DSC files. [Laszlo]
* Changed “!ifdef $(PVSCSI_ENABLE)” in DSC files to “!if $(PVSCSI_ENABLE) == TRUE”. [Laszlo]
* Fix Identation in various places. [Laszlo]
* Added “#include <Uefi/UefiSpec.h>” for EFI_SYSTEM_TABLE. [Laszlo]
* Fix various typos. [Laszlo]
* Made “STATIC” on same line of object delcerations. [Laszlo]
* Added Laszlo’s Reviewed-by tags on some patches. [Laszlo]
* Added missing spaces before “(“ on various function calls. [Laszlo]
* Added PvScsi.h header file to INF [Sources] section. [Laszlo]
* Changed [Protocols] section in INF file to be lexicographically sorted. [Laszlo]
* Changed [PCDs] section in INF file to be lexigraphically sorted. [Laszlo]
* Fixed function comments blocks to be “/** **/” instead of “//” style. [Laszlo]
* Changed PvScsiGetTargetLun() to ZeroMem() all target bytes except first one. [Laszlo]
* Replaced “IOSpace” with “MMIO-Space” in comments. [Laszlo]
* Changed enums to match EDK2 coding convention. [Laszlo]
* Use PCI_BAR_IDX0 instead of hard-coded 0. [Laszlo]
* Use EFI_PAGES_TO_SIZE() instead of manually multiplying with EFI_PAGE_SIZE. [Laszlo]
* Use RShiftU64() to shift UINT64 vars. [Laszlo]
* Changed ReqNumEntries var to UINT32 and shift to use “1U <<” instead of “1 <<”. [Laszlo]
* Changed condition on flag (In PvScsiWaitForRequestCompletion()) to be a boolean expression. [Laszlo]
* Replaced “FakeHostAdapterError” label with a utility function. [Laszlo]
* Added debug message to PvScsiExitBoot() to assist debugging. [Laszlo]
* Fixed resource management to make each function either completely succeed or completely fail and free all resources. [Laszlo]
* Changed PvScsiWriteCmdDesc() to use EfiPciIoWidthFifoUint32. [Laszlo]
* Changed PvScsiWriteCmdDesc() prototype to make clear it descriptor must be an array of words. [Laszlo]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 01/17] OvmfPkg/PvScsiDxe: Create empty driver
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol Liran Alon
` (17 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
In preparation for support booting from PvScsi devices, create a
basic scaffolding for a driver.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 8 ++++++++
OvmfPkg/OvmfPkgIa32.fdf | 3 +++
OvmfPkg/OvmfPkgIa32X64.dsc | 8 ++++++++
OvmfPkg/OvmfPkgIa32X64.fdf | 3 +++
OvmfPkg/OvmfPkgX64.dsc | 8 ++++++++
OvmfPkg/OvmfPkgX64.fdf | 3 +++
OvmfPkg/PvScsiDxe/PvScsi.c | 26 ++++++++++++++++++++++++++
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 27 +++++++++++++++++++++++++++
8 files changed, 86 insertions(+)
create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.c
create mode 100644 OvmfPkg/PvScsiDxe/PvScsiDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 19728f20b34e..af985b4f7826 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -44,6 +44,11 @@
!include NetworkPkg/NetworkDefines.dsc.inc
+ #
+ # Device drivers
+ #
+ DEFINE PVSCSI_ENABLE = TRUE
+
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
# one of the supported values, in place of any of the convenience macros, is
@@ -718,6 +723,9 @@
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+ OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 63607551ed75..a442e133b952 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -227,6 +227,9 @@ INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 3c0c229e3a72..267a83bf86e2 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -44,6 +44,11 @@
!include NetworkPkg/NetworkDefines.dsc.inc
+ #
+ # Device drivers
+ #
+ DEFINE PVSCSI_ENABLE = TRUE
+
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
# one of the supported values, in place of any of the convenience macros, is
@@ -731,6 +736,9 @@
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+ OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 0488e5d95ffe..5fddaac2b0cb 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -228,6 +228,9 @@ INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f6c1d8d228c6..98cc2a955c3e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -44,6 +44,11 @@
!include NetworkPkg/NetworkDefines.dsc.inc
+ #
+ # Device drivers
+ #
+ DEFINE PVSCSI_ENABLE = TRUE
+
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
# one of the supported values, in place of any of the convenience macros, is
@@ -729,6 +734,9 @@
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+ OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 0488e5d95ffe..5fddaac2b0cb 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -228,6 +228,9 @@ INF OvmfPkg/VirtioRngDxe/VirtioRng.inf
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
new file mode 100644
index 000000000000..1ae4de9869c1
--- /dev/null
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -0,0 +1,26 @@
+/** @file
+
+ This driver produces Extended SCSI Pass Thru Protocol instances for
+ pvscsi devices.
+
+ Copyright (C) 2020, Oracle and/or its affiliates.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi/UefiSpec.h>
+
+//
+// Entry Point
+//
+
+EFI_STATUS
+EFIAPI
+PvScsiEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ return EFI_UNSUPPORTED;
+}
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
new file mode 100644
index 000000000000..093cc0171338
--- /dev/null
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -0,0 +1,27 @@
+## @file
+#
+# This driver produces Extended SCSI Pass Thru Protocol instances for
+# pvscsi devices.
+#
+# Copyright (C) 2020, Oracle and/or its affiliates.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 1.29
+ BASE_NAME = PvScsiDxe
+ FILE_GUID = 30346B14-1580-4781-879D-BA0C55AE9BB2
+ MODULE_TYPE = UEFI_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = PvScsiEntryPoint
+
+[Sources]
+ PvScsi.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ UefiDriverEntryPoint
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
2020-03-28 20:00 ` [PATCH v3 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 03/17] OvmfPkg/PvScsiDxe: Report name of driver Liran Alon
` (16 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
In order to probe and connect to the PvScsi device we need this
protocol. Currently it does nothing.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 66 ++++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 1 +
2 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 1ae4de9869c1..77b28b326784 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -9,8 +9,65 @@
**/
+#include <Library/UefiLib.h>
#include <Uefi/UefiSpec.h>
+//
+// Higher versions will be used before lower, 0x10-0xffffffef is the version
+// range for IHV (Indie Hardware Vendors)
+//
+#define PVSCSI_BINDING_VERSION 0x10
+
+//
+// Driver Binding
+//
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiDriverBindingSupported (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE ControllerHandle,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiDriverBindingStart (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE ControllerHandle,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiDriverBindingStop (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE ControllerHandle,
+ IN UINTN NumberOfChildren,
+ IN EFI_HANDLE *ChildHandleBuffer
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC EFI_DRIVER_BINDING_PROTOCOL mPvScsiDriverBinding = {
+ &PvScsiDriverBindingSupported,
+ &PvScsiDriverBindingStart,
+ &PvScsiDriverBindingStop,
+ PVSCSI_BINDING_VERSION,
+ NULL, // ImageHandle, filled by EfiLibInstallDriverBindingComponentName2()
+ NULL // DriverBindingHandle, filled as well
+};
+
//
// Entry Point
//
@@ -22,5 +79,12 @@ PvScsiEntryPoint (
IN EFI_SYSTEM_TABLE *SystemTable
)
{
- return EFI_UNSUPPORTED;
+ return EfiLibInstallDriverBindingComponentName2 (
+ ImageHandle,
+ SystemTable,
+ &mPvScsiDriverBinding,
+ ImageHandle,
+ NULL, // TODO Component name
+ NULL // TODO Component name
+ );
}
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index 093cc0171338..d1d0e963f96d 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -25,3 +25,4 @@
[LibraryClasses]
UefiDriverEntryPoint
+ UefiLib
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 03/17] OvmfPkg/PvScsiDxe: Report name of driver
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
2020-03-28 20:00 ` [PATCH v3 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
2020-03-28 20:00 ` [PATCH v3 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi Liran Alon
` (15 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
Install Component Name protocols to have a nice display name for the
driver in places such as UEFI shell.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 59 ++++++++++++++++++++++++++++++++++++--
1 file changed, 57 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 77b28b326784..51b03f709040 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -68,6 +68,61 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL mPvScsiDriverBinding = {
NULL // DriverBindingHandle, filled as well
};
+//
+// Component Name
+//
+
+STATIC EFI_UNICODE_STRING_TABLE mDriverNameTable[] = {
+ { "eng;en", L"PVSCSI Host Driver" },
+ { NULL, NULL }
+};
+
+STATIC EFI_COMPONENT_NAME_PROTOCOL mComponentName;
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetDriverName (
+ IN EFI_COMPONENT_NAME_PROTOCOL *This,
+ IN CHAR8 *Language,
+ OUT CHAR16 **DriverName
+ )
+{
+ return LookupUnicodeString2 (
+ Language,
+ This->SupportedLanguages,
+ mDriverNameTable,
+ DriverName,
+ (BOOLEAN)(This == &mComponentName) // Iso639Language
+ );
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetDeviceName (
+ IN EFI_COMPONENT_NAME_PROTOCOL *This,
+ IN EFI_HANDLE DeviceHandle,
+ IN EFI_HANDLE ChildHandle,
+ IN CHAR8 *Language,
+ OUT CHAR16 **ControllerName
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC EFI_COMPONENT_NAME_PROTOCOL mComponentName = {
+ &PvScsiGetDriverName,
+ &PvScsiGetDeviceName,
+ "eng" // SupportedLanguages, ISO 639-2 language codes
+};
+
+STATIC EFI_COMPONENT_NAME2_PROTOCOL mComponentName2 = {
+ (EFI_COMPONENT_NAME2_GET_DRIVER_NAME) &PvScsiGetDriverName,
+ (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) &PvScsiGetDeviceName,
+ "en" // SupportedLanguages, RFC 4646 language codes
+};
+
//
// Entry Point
//
@@ -84,7 +139,7 @@ PvScsiEntryPoint (
SystemTable,
&mPvScsiDriverBinding,
ImageHandle,
- NULL, // TODO Component name
- NULL // TODO Component name
+ &mComponentName,
+ &mComponentName2
);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (2 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 03/17] OvmfPkg/PvScsiDxe: Report name of driver Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Liran Alon
` (14 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
PvScsiControllerSupported() is called on handles passed in
by the ConnectController() boot service and if the handle is the
PVSCSI controller, the function would return success. A success
return value will attach our driver to the device.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/Include/IndustryStandard/PvScsi.h | 21 ++++++++++
OvmfPkg/PvScsiDxe/PvScsi.c | 49 ++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 +++
3 files changed, 74 insertions(+), 1 deletion(-)
create mode 100644 OvmfPkg/Include/IndustryStandard/PvScsi.h
diff --git a/OvmfPkg/Include/IndustryStandard/PvScsi.h b/OvmfPkg/Include/IndustryStandard/PvScsi.h
new file mode 100644
index 000000000000..004c0af84989
--- /dev/null
+++ b/OvmfPkg/Include/IndustryStandard/PvScsi.h
@@ -0,0 +1,21 @@
+/** @file
+
+ VMware PVSCSI Device specific type and macro definitions.
+
+ Copyright (C) 2020, Oracle and/or its affiliates.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PVSCSI_H_
+#define __PVSCSI_H_
+
+//
+// Device offsets and constants
+//
+
+#define PCI_VENDOR_ID_VMWARE (0x15ad)
+#define PCI_DEVICE_ID_VMWARE_PVSCSI (0x07c0)
+
+#endif // __PVSCSI_H_
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 51b03f709040..9923a31d25d7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -9,7 +9,11 @@
**/
+#include <IndustryStandard/Pci.h>
+#include <IndustryStandard/PvScsi.h>
+#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
+#include <Protocol/PciIo.h>
#include <Uefi/UefiSpec.h>
//
@@ -31,7 +35,50 @@ PvScsiDriverBindingSupported (
IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
)
{
- return EFI_UNSUPPORTED;
+ EFI_STATUS Status;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ PCI_TYPE00 Pci;
+
+ Status = gBS->OpenProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ (VOID **)&PciIo,
+ This->DriverBindingHandle,
+ ControllerHandle,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = PciIo->Pci.Read (
+ PciIo,
+ EfiPciIoWidthUint32,
+ 0,
+ sizeof (Pci) / sizeof (UINT32),
+ &Pci
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ if ((Pci.Hdr.VendorId != PCI_VENDOR_ID_VMWARE) ||
+ (Pci.Hdr.DeviceId != PCI_DEVICE_ID_VMWARE_PVSCSI)) {
+ Status = EFI_UNSUPPORTED;
+ goto Done;
+ }
+
+ Status = EFI_SUCCESS;
+
+Done:
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ControllerHandle
+ );
+
+ return Status;
}
STATIC
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index d1d0e963f96d..c1f0663832ed 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -22,7 +22,12 @@
[Packages]
MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
+
+[Protocols]
+ gEfiPciIoProtocolGuid ## TO_START
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (3 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs Liran Alon
` (13 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
Support dynamic insertion and removal of the protocol.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 209 +++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 29 +++++
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 6 +-
3 files changed, 241 insertions(+), 3 deletions(-)
create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.h
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 9923a31d25d7..04c08036b799 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -11,17 +11,156 @@
#include <IndustryStandard/Pci.h>
#include <IndustryStandard/PvScsi.h>
+#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Protocol/PciIo.h>
#include <Uefi/UefiSpec.h>
+#include "PvScsi.h"
+
//
// Higher versions will be used before lower, 0x10-0xffffffef is the version
// range for IHV (Indie Hardware Vendors)
//
#define PVSCSI_BINDING_VERSION 0x10
+//
+// Ext SCSI Pass Thru
+//
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiPassThru (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN UINT8 *Target,
+ IN UINT64 Lun,
+ IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+ IN EFI_EVENT Event OPTIONAL
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetNextTargetLun (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN OUT UINT8 **Target,
+ IN OUT UINT64 *Lun
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiBuildDevicePath (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN UINT8 *Target,
+ IN UINT64 Lun,
+ IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetTargetLun (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN EFI_DEVICE_PATH_PROTOCOL *DevicePath,
+ OUT UINT8 **Target,
+ OUT UINT64 *Lun
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiResetChannel (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiResetTargetLun (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN UINT8 *Target,
+ IN UINT64 Lun
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetNextTarget (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN OUT UINT8 **Target
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+PvScsiInit (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ //
+ // Populate the exported interface's attributes
+ //
+ Dev->PassThru.Mode = &Dev->PassThruMode;
+ Dev->PassThru.PassThru = &PvScsiPassThru;
+ Dev->PassThru.GetNextTargetLun = &PvScsiGetNextTargetLun;
+ Dev->PassThru.BuildDevicePath = &PvScsiBuildDevicePath;
+ Dev->PassThru.GetTargetLun = &PvScsiGetTargetLun;
+ Dev->PassThru.ResetChannel = &PvScsiResetChannel;
+ Dev->PassThru.ResetTargetLun = &PvScsiResetTargetLun;
+ Dev->PassThru.GetNextTarget = &PvScsiGetNextTarget;
+
+ //
+ // AdapterId is a target for which no handle will be created during bus scan.
+ // Prevent any conflict with real devices.
+ //
+ Dev->PassThruMode.AdapterId = MAX_UINT32;
+
+ //
+ // Set both physical and logical attributes for non-RAID SCSI channel
+ //
+ Dev->PassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL |
+ EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
+
+ //
+ // No restriction on transfer buffer alignment
+ //
+ Dev->PassThruMode.IoAlign = 0;
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+VOID
+PvScsiUninit (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ // Currently nothing to do here
+}
+
//
// Driver Binding
//
@@ -90,7 +229,42 @@ PvScsiDriverBindingStart (
IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
)
{
- return EFI_UNSUPPORTED;
+ PVSCSI_DEV *Dev;
+ EFI_STATUS Status;
+
+ Dev = (PVSCSI_DEV *) AllocateZeroPool (sizeof (*Dev));
+ if (Dev == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Status = PvScsiInit (Dev);
+ if (EFI_ERROR (Status)) {
+ goto FreePvScsi;
+ }
+
+ //
+ // Setup complete, attempt to export the driver instance's PassThru interface
+ //
+ Dev->Signature = PVSCSI_SIG;
+ Status = gBS->InstallProtocolInterface (
+ &ControllerHandle,
+ &gEfiExtScsiPassThruProtocolGuid,
+ EFI_NATIVE_INTERFACE,
+ &Dev->PassThru
+ );
+ if (EFI_ERROR (Status)) {
+ goto UninitDev;
+ }
+
+ return EFI_SUCCESS;
+
+UninitDev:
+ PvScsiUninit (Dev);
+
+FreePvScsi:
+ FreePool (Dev);
+
+ return Status;
}
STATIC
@@ -103,7 +277,38 @@ PvScsiDriverBindingStop (
IN EFI_HANDLE *ChildHandleBuffer
)
{
- return EFI_UNSUPPORTED;
+ EFI_STATUS Status;
+ EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru;
+ PVSCSI_DEV *Dev;
+
+ Status = gBS->OpenProtocol (
+ ControllerHandle,
+ &gEfiExtScsiPassThruProtocolGuid,
+ (VOID **)&PassThru,
+ This->DriverBindingHandle,
+ ControllerHandle,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL // Lookup only
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Dev = PVSCSI_FROM_PASS_THRU (PassThru);
+
+ Status = gBS->UninstallProtocolInterface (
+ ControllerHandle,
+ &gEfiExtScsiPassThruProtocolGuid,
+ &Dev->PassThru
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ PvScsiUninit (Dev);
+
+ FreePool (Dev);
+
+ return EFI_SUCCESS;
}
STATIC EFI_DRIVER_BINDING_PROTOCOL mPvScsiDriverBinding = {
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
new file mode 100644
index 000000000000..3940b4c20019
--- /dev/null
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -0,0 +1,29 @@
+/** @file
+
+ Internal definitions for the PVSCSI driver, which produces Extended SCSI
+ Pass Thru Protocol instances for pvscsi devices.
+
+ Copyright (C) 2020, Oracle and/or its affiliates.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PVSCSI_DXE_H_
+#define __PVSCSI_DXE_H_
+
+#include <Library/DebugLib.h>
+#include <Protocol/ScsiPassThruExt.h>
+
+#define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
+
+typedef struct {
+ UINT32 Signature;
+ EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
+ EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
+} PVSCSI_DEV;
+
+#define PVSCSI_FROM_PASS_THRU(PassThruPointer) \
+ CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG)
+
+#endif // __PVSCSI_DXE_H_
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index c1f0663832ed..f4d452c6c3d2 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -19,15 +19,19 @@
[Sources]
PvScsi.c
+ PvScsi.h
[Packages]
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ DebugLib
+ MemoryAllocationLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
[Protocols]
- gEfiPciIoProtocolGuid ## TO_START
+ gEfiExtScsiPassThruProtocolGuid ## BY_START
+ gEfiPciIoProtocolGuid ## TO_START
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (4 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath Liran Alon
` (12 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
Implement EXT_SCSI_PASS_THRU.GetNextTarget() and
EXT_SCSI_PASS_THRU.GetNextTargetLun().
ScsiBusDxe scans all MaxTarget * MaxLun possible devices.
This can take unnecessarily long for large number of targets.
To deal with this, VirtioScsiDxe has defined PCDs to limit the
MaxTarget & MaxLun to desired values which gives sufficient
performance. It is very important in virtio-scsi as it can have
very big MaxTarget & MaxLun.
Even though a common PVSCSI device has a default MaxTarget=64 and
MaxLun=0, we implement similar mechanism as virtio-scsi for completeness.
This may be useful in the future when PVSCSI will have bigger values
for MaxTarget and MaxLun.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/OvmfPkg.dec | 9 +++
OvmfPkg/PvScsiDxe/PvScsi.c | 122 +++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 2 +
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 ++
4 files changed, 136 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4c5b6511cb97..a04aee5c2cd4 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -121,6 +121,15 @@
gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxTargetLimit|31|UINT16|6
gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxLunLimit|7|UINT32|7
+ ## Sets the *inclusive* number of targets and LUNs that PvScsi exposes for
+ # scan by ScsiBusDxe.
+ # As specified above for VirtioScsi, ScsiBusDxe scans all MaxTarget * MaxLun
+ # possible devices, which can take extremely long. Thus, the below constants
+ # are used so that scanning the number of devices given by their product
+ # is still acceptably fast.
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x36
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x37
+
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 04c08036b799..7f51ada19a1a 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -11,6 +11,7 @@
#include <IndustryStandard/Pci.h>
#include <IndustryStandard/PvScsi.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
@@ -25,6 +26,30 @@
//
#define PVSCSI_BINDING_VERSION 0x10
+//
+// Ext SCSI Pass Thru utilities
+//
+
+/**
+ Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
+ EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
+**/
+STATIC
+BOOLEAN
+IsTargetInitialized (
+ IN UINT8 *Target
+ )
+{
+ UINTN Idx;
+
+ for (Idx = 0; Idx < TARGET_MAX_BYTES; ++Idx) {
+ if (Target[Idx] != 0xFF) {
+ return TRUE;
+ }
+ }
+ return FALSE;
+}
+
//
// Ext SCSI Pass Thru
//
@@ -52,7 +77,54 @@ PvScsiGetNextTargetLun (
IN OUT UINT64 *Lun
)
{
- return EFI_UNSUPPORTED;
+ UINT8 *TargetPtr;
+ UINT8 LastTarget;
+ PVSCSI_DEV *Dev;
+
+ if (Target == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // The Target input parameter is unnecessarily a pointer-to-pointer
+ //
+ TargetPtr = *Target;
+
+ //
+ // If target not initialized, return first target & LUN
+ //
+ if (!IsTargetInitialized (TargetPtr)) {
+ ZeroMem (TargetPtr, TARGET_MAX_BYTES);
+ *Lun = 0;
+ return EFI_SUCCESS;
+ }
+
+ //
+ // We only use first byte of target identifer
+ //
+ LastTarget = *TargetPtr;
+
+ //
+ // Increment (target, LUN) pair if valid on input
+ //
+ Dev = PVSCSI_FROM_PASS_THRU (This);
+ if (LastTarget > Dev->MaxTarget || *Lun > Dev->MaxLun) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (*Lun < Dev->MaxLun) {
+ ++*Lun;
+ return EFI_SUCCESS;
+ }
+
+ if (LastTarget < Dev->MaxTarget) {
+ *Lun = 0;
+ ++LastTarget;
+ *TargetPtr = LastTarget;
+ return EFI_SUCCESS;
+ }
+
+ return EFI_NOT_FOUND;
}
STATIC
@@ -111,7 +183,47 @@ PvScsiGetNextTarget (
IN OUT UINT8 **Target
)
{
- return EFI_UNSUPPORTED;
+ UINT8 *TargetPtr;
+ UINT8 LastTarget;
+ PVSCSI_DEV *Dev;
+
+ if (Target == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // The Target input parameter is unnecessarily a pointer-to-pointer
+ //
+ TargetPtr = *Target;
+
+ //
+ // If target not initialized, return first target
+ //
+ if (!IsTargetInitialized (TargetPtr)) {
+ ZeroMem (TargetPtr, TARGET_MAX_BYTES);
+ return EFI_SUCCESS;
+ }
+
+ //
+ // We only use first byte of target identifer
+ //
+ LastTarget = *TargetPtr;
+
+ //
+ // Increment target if valid on input
+ //
+ Dev = PVSCSI_FROM_PASS_THRU (This);
+ if (LastTarget > Dev->MaxTarget) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (LastTarget < Dev->MaxTarget) {
+ ++LastTarget;
+ *TargetPtr = LastTarget;
+ return EFI_SUCCESS;
+ }
+
+ return EFI_NOT_FOUND;
}
STATIC
@@ -120,6 +232,12 @@ PvScsiInit (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Init configuration
+ //
+ Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
+ Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
+
//
// Populate the exported interface's attributes
//
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 3940b4c20019..dd3e0c68e6da 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -19,6 +19,8 @@
typedef struct {
UINT32 Signature;
+ UINT8 MaxTarget;
+ UINT8 MaxLun;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
} PVSCSI_DEV;
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index f4d452c6c3d2..fcffc90d46c8 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -26,6 +26,7 @@
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseMemoryLib
DebugLib
MemoryAllocationLib
UefiBootServicesTableLib
@@ -35,3 +36,7 @@
[Protocols]
gEfiExtScsiPassThruProtocolGuid ## BY_START
gEfiPciIoProtocolGuid ## TO_START
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (5 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use Liran Alon
` (11 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
Implement EXT_SCSI_PASS_THRU.BuildDevicePath() and
EXT_SCSI_PASS_THRU.GetTargetLun().
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 61 ++++++++++++++++++++++++++++++++++++--
1 file changed, 59 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 7f51ada19a1a..76fc1eb910f2 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -137,7 +137,38 @@ PvScsiBuildDevicePath (
IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
)
{
- return EFI_UNSUPPORTED;
+ UINT8 TargetValue;
+ PVSCSI_DEV *Dev;
+ SCSI_DEVICE_PATH *ScsiDevicePath;
+
+ if (DevicePath == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // We only use first byte of target identifer
+ //
+ TargetValue = *Target;
+
+ Dev = PVSCSI_FROM_PASS_THRU (This);
+ if (TargetValue > Dev->MaxTarget || Lun > Dev->MaxLun) {
+ return EFI_NOT_FOUND;
+ }
+
+ ScsiDevicePath = AllocatePool (sizeof (*ScsiDevicePath));
+ if (ScsiDevicePath == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ ScsiDevicePath->Header.Type = MESSAGING_DEVICE_PATH;
+ ScsiDevicePath->Header.SubType = MSG_SCSI_DP;
+ ScsiDevicePath->Header.Length[0] = (UINT8)sizeof (*ScsiDevicePath);
+ ScsiDevicePath->Header.Length[1] = (UINT8)(sizeof (*ScsiDevicePath) >> 8);
+ ScsiDevicePath->Pun = TargetValue;
+ ScsiDevicePath->Lun = (UINT16)Lun;
+
+ *DevicePath = &ScsiDevicePath->Header;
+ return EFI_SUCCESS;
}
STATIC
@@ -150,7 +181,33 @@ PvScsiGetTargetLun (
OUT UINT64 *Lun
)
{
- return EFI_UNSUPPORTED;
+ SCSI_DEVICE_PATH *ScsiDevicePath;
+ PVSCSI_DEV *Dev;
+
+ if (DevicePath == NULL || Target == NULL || *Target == NULL || Lun == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (DevicePath->Type != MESSAGING_DEVICE_PATH ||
+ DevicePath->SubType != MSG_SCSI_DP) {
+ return EFI_UNSUPPORTED;
+ }
+
+ ScsiDevicePath = (SCSI_DEVICE_PATH *)DevicePath;
+ Dev = PVSCSI_FROM_PASS_THRU (This);
+ if (ScsiDevicePath->Pun > Dev->MaxTarget ||
+ ScsiDevicePath->Lun > Dev->MaxLun) {
+ return EFI_NOT_FOUND;
+ }
+
+ //
+ // We only use first byte of target identifer
+ //
+ **Target = (UINT8)ScsiDevicePath->Pun;
+ ZeroMem (*Target + 1, TARGET_MAX_BYTES - 1);
+ *Lun = ScsiDevicePath->Lun;
+
+ return EFI_SUCCESS;
}
STATIC
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (6 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit Liran Alon
` (10 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
This will give us an exclusive access to the PciIo of this device
after it was started and until it will be stopped.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 29 ++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 76fc1eb910f2..e0380d729b3c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -412,11 +412,23 @@ PvScsiDriverBindingStart (
return EFI_OUT_OF_RESOURCES;
}
- Status = PvScsiInit (Dev);
+ Status = gBS->OpenProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ (VOID **)&Dev->PciIo,
+ This->DriverBindingHandle,
+ ControllerHandle,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
if (EFI_ERROR (Status)) {
goto FreePvScsi;
}
+ Status = PvScsiInit (Dev);
+ if (EFI_ERROR (Status)) {
+ goto ClosePciIo;
+ }
+
//
// Setup complete, attempt to export the driver instance's PassThru interface
//
@@ -436,6 +448,14 @@ PvScsiDriverBindingStart (
UninitDev:
PvScsiUninit (Dev);
+ClosePciIo:
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ControllerHandle
+ );
+
FreePvScsi:
FreePool (Dev);
@@ -481,6 +501,13 @@ PvScsiDriverBindingStop (
PvScsiUninit (Dev);
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ControllerHandle
+ );
+
FreePool (Dev);
return EFI_SUCCESS;
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index dd3e0c68e6da..e1e5ae18ebf2 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -19,6 +19,7 @@
typedef struct {
UINT32 Signature;
+ EFI_PCI_IO_PROTOCOL *PciIo;
UINT8 MaxTarget;
UINT8 MaxLun;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (7 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes Liran Alon
` (9 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
This commit doesn't change semantics.
It is done as a preparation for future commits which will modify
PCI attributes.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 54 +++++++++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
2 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index e0380d729b3c..5566b4cce467 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -283,18 +283,70 @@ PvScsiGetNextTarget (
return EFI_NOT_FOUND;
}
+STATIC
+EFI_STATUS
+PvScsiSetPciAttributes (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ EFI_STATUS Status;
+
+ //
+ // Backup original PCI Attributes
+ //
+ Status = Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationGet,
+ 0,
+ &Dev->OriginalPciAttributes
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // TODO: Change PCI Attributes
+ //
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+VOID
+PvScsiRestorePciAttributes (
+ IN PVSCSI_DEV *Dev
+ )
+{
+ Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationSet,
+ Dev->OriginalPciAttributes,
+ NULL
+ );
+}
+
STATIC
EFI_STATUS
PvScsiInit (
IN OUT PVSCSI_DEV *Dev
)
{
+ EFI_STATUS Status;
+
//
// Init configuration
//
Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
+ //
+ // Set PCI Attributes
+ //
+ Status = PvScsiSetPciAttributes (Dev);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -333,7 +385,7 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *Dev
)
{
- // Currently nothing to do here
+ PvScsiRestorePciAttributes (Dev);
}
//
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index e1e5ae18ebf2..5f611dbbc98c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -20,6 +20,7 @@
typedef struct {
UINT32 Signature;
EFI_PCI_IO_PROTOCOL *PciIo;
+ UINT64 OriginalPciAttributes;
UINT8 MaxTarget;
UINT8 MaxLun;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (8 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants Liran Alon
` (8 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
Enable MMIO-Space & Bus-Mastering PCI attributes when device is started.
Note that original PCI attributes are restored when device is stopped.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 5566b4cce467..531bed4e5ab7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -305,8 +305,18 @@ PvScsiSetPciAttributes (
}
//
- // TODO: Change PCI Attributes
+ // Enable MMIO-Space & Bus-Mastering
//
+ Status = Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationEnable,
+ (EFI_PCI_IO_ATTRIBUTE_MEMORY |
+ EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
return EFI_SUCCESS;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (9 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-28 20:00 ` [PATCH v3 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
` (7 subsequent siblings)
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
These definitions will be used by the following commits to complete the
implementation of PVSCSI device driver.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Acked-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/Include/IndustryStandard/PvScsi.h | 165 ++++++++++++++++++++++
1 file changed, 165 insertions(+)
diff --git a/OvmfPkg/Include/IndustryStandard/PvScsi.h b/OvmfPkg/Include/IndustryStandard/PvScsi.h
index 004c0af84989..a4d6634f3ba0 100644
--- a/OvmfPkg/Include/IndustryStandard/PvScsi.h
+++ b/OvmfPkg/Include/IndustryStandard/PvScsi.h
@@ -18,4 +18,169 @@
#define PCI_VENDOR_ID_VMWARE (0x15ad)
#define PCI_DEVICE_ID_VMWARE_PVSCSI (0x07c0)
+//
+// CDB (Command Descriptor Block) with size above this constant
+// should be considered out-of-band
+//
+#define PVSCSI_CDB_MAX_SIZE (16)
+
+typedef enum {
+ PvScsiRegOffsetCommand = 0x0,
+ PvScsiRegOffsetCommandData = 0x4,
+ PvScsiRegOffsetCommandStatus = 0x8,
+ PvScsiRegOffsetLastSts0 = 0x100,
+ PvScsiRegOffsetLastSts1 = 0x104,
+ PvScsiRegOffsetLastSts2 = 0x108,
+ PvScsiRegOffsetLastSts3 = 0x10c,
+ PvScsiRegOffsetIntrStatus = 0x100c,
+ PvScsiRegOffsetIntrMask = 0x2010,
+ PvScsiRegOffsetKickNonRwIo = 0x3014,
+ PvScsiRegOffsetDebug = 0x3018,
+ PvScsiRegOffsetKickRwIo = 0x4018,
+} PVSCSI_BAR0_OFFSETS;
+
+//
+// Define Interrupt-Status register flags
+//
+#define PVSCSI_INTR_CMPL_0 BIT0
+#define PVSCSI_INTR_CMPL_1 BIT1
+#define PVSCSI_INTR_CMPL_MASK (PVSCSI_INTR_CMPL_0 | PVSCSI_INTR_CMPL_1)
+
+typedef enum {
+ PvScsiCmdFirst = 0,
+ PvScsiCmdAdapterReset = 1,
+ PvScsiCmdIssueScsi = 2,
+ PvScsiCmdSetupRings = 3,
+ PvScsiCmdResetBus = 4,
+ PvScsiCmdResetDevice = 5,
+ PvScsiCmdAbortCmd = 6,
+ PvScsiCmdConfig = 7,
+ PvScsiCmdSetupMsgRing = 8,
+ PvScsiCmdDeviceUnplug = 9,
+ PvScsiCmdLast = 10
+} PVSCSI_COMMANDS;
+
+#define PVSCSI_SETUP_RINGS_MAX_NUM_PAGES (32)
+
+#pragma pack (1)
+typedef struct {
+ UINT32 ReqRingNumPages;
+ UINT32 CmpRingNumPages;
+ UINT64 RingsStatePPN;
+ UINT64 ReqRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
+ UINT64 CmpRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
+} PVSCSI_CMD_DESC_SETUP_RINGS;
+#pragma pack ()
+
+#define PVSCSI_MAX_CMD_DATA_WORDS \
+ (sizeof (PVSCSI_CMD_DESC_SETUP_RINGS) / sizeof (UINT32))
+
+#pragma pack (1)
+typedef struct {
+ UINT32 ReqProdIdx;
+ UINT32 ReqConsIdx;
+ UINT32 ReqNumEntriesLog2;
+
+ UINT32 CmpProdIdx;
+ UINT32 CmpConsIdx;
+ UINT32 CmpNumEntriesLog2;
+
+ UINT8 Pad[104];
+
+ UINT32 MsgProdIdx;
+ UINT32 MsgConsIdx;
+ UINT32 MsgNumEntriesLog2;
+} PVSCSI_RINGS_STATE;
+#pragma pack ()
+
+//
+// Define PVSCSI request descriptor tags
+//
+#define PVSCSI_SIMPLE_QUEUE_TAG (0x20)
+
+//
+// Define PVSCSI request descriptor flags
+//
+#define PVSCSI_FLAG_CMD_WITH_SG_LIST BIT0
+#define PVSCSI_FLAG_CMD_OUT_OF_BAND_CDB BIT1
+#define PVSCSI_FLAG_CMD_DIR_NONE BIT2
+#define PVSCSI_FLAG_CMD_DIR_TOHOST BIT3
+#define PVSCSI_FLAG_CMD_DIR_TODEVICE BIT4
+
+#pragma pack (1)
+typedef struct {
+ UINT64 Context;
+ UINT64 DataAddr;
+ UINT64 DataLen;
+ UINT64 SenseAddr;
+ UINT32 SenseLen;
+ UINT32 Flags;
+ UINT8 Cdb[16];
+ UINT8 CdbLen;
+ UINT8 Lun[8];
+ UINT8 Tag;
+ UINT8 Bus;
+ UINT8 Target;
+ UINT8 VcpuHint;
+ UINT8 Unused[59];
+} PVSCSI_RING_REQ_DESC;
+#pragma pack ()
+
+//
+// Host adapter status/error codes
+//
+typedef enum {
+ PvScsiBtStatSuccess = 0x00, // CCB complete normally with no errors
+ PvScsiBtStatLinkedCommandCompleted = 0x0a,
+ PvScsiBtStatLinkedCommandCompletedWithFlag = 0x0b,
+ PvScsiBtStatDataUnderrun = 0x0c,
+ PvScsiBtStatSelTimeout = 0x11, // SCSI selection timeout
+ PvScsiBtStatDatarun = 0x12, // Data overrun/underrun
+ PvScsiBtStatBusFree = 0x13, // Unexpected bus free
+ PvScsiBtStatInvPhase = 0x14, //
+ // Invalid bus phase or sequence requested
+ // by target
+ //
+ PvScsiBtStatLunMismatch = 0x17, //
+ // Linked CCB has different LUN from first
+ // CCB
+ //
+ PvScsiBtStatSensFailed = 0x1b, // Auto request sense failed
+ PvScsiBtStatTagReject = 0x1c, //
+ // SCSI II tagged queueing message rejected
+ // by target
+ //
+ PvScsiBtStatBadMsg = 0x1d, //
+ // Unsupported message received by the host
+ // adapter
+ //
+ PvScsiBtStatHaHardware = 0x20, // Host adapter hardware failed
+ PvScsiBtStatNoResponse = 0x21, //
+ // Target did not respond to SCSI ATN sent
+ // a SCSI RST
+ //
+ PvScsiBtStatSentRst = 0x22, // Host adapter asserted a SCSI RST
+ PvScsiBtStatRecvRst = 0x23, // Other SCSI devices asserted a SCSI RST
+ PvScsiBtStatDisconnect = 0x24, //
+ // Target device reconnected improperly
+ // (w/o tag)
+ //
+ PvScsiBtStatBusReset = 0x25, // Host adapter issued BUS device reset
+ PvScsiBtStatAbortQueue = 0x26, // Abort queue generated
+ PvScsiBtStatHaSoftware = 0x27, // Host adapter software error
+ PvScsiBtStatHaTimeout = 0x30, // Host adapter hardware timeout error
+ PvScsiBtStatScsiParity = 0x34, // SCSI parity error detected
+} PVSCSI_HOST_BUS_ADAPTER_STATUS;
+
+#pragma pack (1)
+typedef struct {
+ UINT64 Context;
+ UINT64 DataLen;
+ UINT32 SenseLen;
+ UINT16 HostStatus;
+ UINT16 ScsiStatus;
+ UINT32 Pad[2];
+} PVSCSI_RING_CMP_DESC;
+#pragma pack ()
+
#endif // __PVSCSI_H_
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (10 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-30 15:19 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
` (6 subsequent siblings)
18 siblings, 1 reply; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
The following commits will complete the implementation of
device initialization.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 114 +++++++++++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 531bed4e5ab7..cf75884350ee 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -30,6 +30,107 @@
// Ext SCSI Pass Thru utilities
//
+/**
+ Writes a 32-bit value into BAR0 using MMIO
+**/
+STATIC
+EFI_STATUS
+PvScsiMmioWrite32 (
+ IN CONST PVSCSI_DEV *Dev,
+ IN UINT64 Offset,
+ IN UINT32 Value
+ )
+{
+ return Dev->PciIo->Mem.Write (
+ Dev->PciIo,
+ EfiPciIoWidthUint32,
+ PCI_BAR_IDX0,
+ Offset,
+ 1, // Count
+ &Value
+ );
+}
+
+/**
+ Writes multiple words of data into BAR0 using MMIO
+**/
+STATIC
+EFI_STATUS
+PvScsiMmioWrite32Multiple (
+ IN CONST PVSCSI_DEV *Dev,
+ IN UINT64 Offset,
+ IN UINTN Count,
+ IN UINT32 *Words
+ )
+{
+ return Dev->PciIo->Mem.Write (
+ Dev->PciIo,
+ EfiPciIoWidthFifoUint32,
+ PCI_BAR_IDX0,
+ Offset,
+ Count,
+ Words
+ );
+}
+
+/**
+ Send a PVSCSI command to device.
+
+ @param[in] Dev The pvscsi host device.
+ @param[in] Cmd The command to send to device.
+ @param[in] OPTIONAL DescWords An optional command descriptor (If command
+ have a descriptor). The descriptor is
+ provided as an array of UINT32 words and
+ is must be 32-bit aligned.
+ @param[in] DescWordsCount The number of words in command descriptor.
+ Caller must specify here 0 if DescWords
+ is not supplied (It is optional). In that
+ case, DescWords is ignored.
+
+ @return Status codes returned by Dev->PciIo->Mem.Write().
+
+**/
+STATIC
+EFI_STATUS
+PvScsiWriteCmdDesc (
+ IN CONST PVSCSI_DEV *Dev,
+ IN UINT32 Cmd,
+ IN UINT32 *DescWords OPTIONAL,
+ IN UINTN DescWordsCount
+ )
+{
+ EFI_STATUS Status;
+
+ if (DescWordsCount > PVSCSI_MAX_CMD_DATA_WORDS) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetCommand, Cmd);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (DescWordsCount > 0) {
+ return PvScsiMmioWrite32Multiple (
+ Dev,
+ PvScsiRegOffsetCommandData,
+ DescWordsCount,
+ DescWords
+ );
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+PvScsiResetAdapter (
+ IN CONST PVSCSI_DEV *Dev
+ )
+{
+ return PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
+}
+
/**
Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
@@ -357,6 +458,14 @@ PvScsiInit (
return Status;
}
+ //
+ // Reset adapter
+ //
+ Status = PvScsiResetAdapter (Dev);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -387,6 +496,11 @@ PvScsiInit (
Dev->PassThruMode.IoAlign = 0;
return EFI_SUCCESS;
+
+RestorePciAttributes:
+ PvScsiRestorePciAttributes (Dev);
+
+ return Status;
}
STATIC
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (11 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-30 15:54 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
` (5 subsequent siblings)
18 siblings, 1 reply; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
These rings are shared memory buffers between host and device in which
a cyclic buffer is managed to send request descriptors from host to
device and receive completion descriptors from device to host.
Note that because device may be constrained by IOMMU or guest may be run
under AMD SEV, we make sure to map these rings to device by using
PciIo->Map().
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 219 ++++++++++++++++++++++++++++++++
OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 1 +
3 files changed, 237 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index cf75884350ee..c7d367e83a2d 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -11,11 +11,13 @@
#include <IndustryStandard/Pci.h>
#include <IndustryStandard/PvScsi.h>
+#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Protocol/PciIo.h>
+#include <Protocol/PciRootBridgeIo.h>
#include <Uefi/UefiSpec.h>
#include "PvScsi.h"
@@ -436,6 +438,207 @@ PvScsiRestorePciAttributes (
);
}
+STATIC
+EFI_STATUS
+PvScsiAllocateSharedPages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ OUT VOID **HostAddress,
+ OUT PVSCSI_DMA_DESC *DmaDesc
+ )
+{
+ EFI_STATUS Status;
+ UINTN NumberOfBytes;
+
+ Status = Dev->PciIo->AllocateBuffer (
+ Dev->PciIo,
+ AllocateAnyPages,
+ EfiBootServicesData,
+ Pages,
+ HostAddress,
+ EFI_PCI_ATTRIBUTE_MEMORY_CACHED
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ NumberOfBytes = EFI_PAGES_TO_SIZE (Pages);
+ Status = Dev->PciIo->Map (
+ Dev->PciIo,
+ EfiPciIoOperationBusMasterCommonBuffer,
+ *HostAddress,
+ &NumberOfBytes,
+ &DmaDesc->DeviceAddress,
+ &DmaDesc->Mapping
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBuffer;
+ }
+
+ if (NumberOfBytes != EFI_PAGES_TO_SIZE (Pages)) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Unmap;
+ }
+
+ return EFI_SUCCESS;
+
+Unmap:
+ Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+
+FreeBuffer:
+ Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, *HostAddress);
+
+ return Status;
+}
+
+STATIC
+VOID
+PvScsiFreeSharedPages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ IN VOID *HostAddress,
+ IN PVSCSI_DMA_DESC *DmaDesc
+ )
+{
+ Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+ Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, HostAddress);
+}
+
+STATIC
+EFI_STATUS
+PvScsiInitRings (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ EFI_STATUS Status;
+ union {
+ PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
+ UINT32 Uint32;
+ } AlignedCmd;
+ PVSCSI_CMD_DESC_SETUP_RINGS *Cmd;
+
+ Cmd = &AlignedCmd.Cmd;
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ (VOID **)&Dev->RingDesc.RingState,
+ &Dev->RingDesc.RingStateDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ (VOID **)&Dev->RingDesc.RingReqs,
+ &Dev->RingDesc.RingReqsDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRingState;
+ }
+ ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ (VOID **)&Dev->RingDesc.RingCmps,
+ &Dev->RingDesc.RingCmpsDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRingReqs;
+ }
+ ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
+
+ ZeroMem (Cmd, sizeof (*Cmd));
+ Cmd->ReqRingNumPages = 1;
+ Cmd->CmpRingNumPages = 1;
+ Cmd->RingsStatePPN = RShiftU64 (
+ Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+ Cmd->ReqRingPPNs[0] = RShiftU64 (
+ Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+ Cmd->CmpRingPPNs[0] = RShiftU64 (
+ Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+
+ STATIC_ASSERT (
+ sizeof (*Cmd) % sizeof (UINT32) == 0,
+ "Cmd must be multiple of 32-bit words"
+ );
+ Status = PvScsiWriteCmdDesc (
+ Dev,
+ PvScsiCmdSetupRings,
+ (UINT32 *)Cmd,
+ sizeof (*Cmd) / sizeof (UINT32)
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRingCmps;
+ }
+
+ return EFI_SUCCESS;
+
+FreeRingCmps:
+ PvScsiFreeSharedPages (
+ Dev,
+ 1,
+ Dev->RingDesc.RingCmps,
+ &Dev->RingDesc.RingCmpsDmaDesc
+ );
+
+FreeRingReqs:
+ PvScsiFreeSharedPages (
+ Dev,
+ 1,
+ Dev->RingDesc.RingReqs,
+ &Dev->RingDesc.RingReqsDmaDesc
+ );
+
+FreeRingState:
+ PvScsiFreeSharedPages (
+ Dev,
+ 1,
+ Dev->RingDesc.RingState,
+ &Dev->RingDesc.RingStateDmaDesc
+ );
+
+ return Status;
+}
+
+STATIC
+VOID
+PvScsiFreeRings (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ PvScsiFreeSharedPages (
+ Dev,
+ 1,
+ Dev->RingDesc.RingCmps,
+ &Dev->RingDesc.RingCmpsDmaDesc
+ );
+
+ PvScsiFreeSharedPages (
+ Dev,
+ 1,
+ Dev->RingDesc.RingReqs,
+ &Dev->RingDesc.RingReqsDmaDesc
+ );
+
+ PvScsiFreeSharedPages (
+ Dev,
+ 1,
+ Dev->RingDesc.RingState,
+ &Dev->RingDesc.RingStateDmaDesc
+ );
+}
+
STATIC
EFI_STATUS
PvScsiInit (
@@ -466,6 +669,14 @@ PvScsiInit (
goto RestorePciAttributes;
}
+ //
+ // Init PVSCSI rings
+ //
+ Status = PvScsiInitRings (Dev);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -509,6 +720,14 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Reset device to stop device usage of the rings.
+ // This is required to safely free the rings.
+ //
+ PvScsiResetAdapter (Dev);
+
+ PvScsiFreeRings (Dev);
+
PvScsiRestorePciAttributes (Dev);
}
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 5f611dbbc98c..6d23b6e1eccf 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -15,12 +15,29 @@
#include <Library/DebugLib.h>
#include <Protocol/ScsiPassThruExt.h>
+typedef struct {
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *Mapping;
+} PVSCSI_DMA_DESC;
+
+typedef struct {
+ PVSCSI_RINGS_STATE *RingState;
+ PVSCSI_DMA_DESC RingStateDmaDesc;
+
+ PVSCSI_RING_REQ_DESC *RingReqs;
+ PVSCSI_DMA_DESC RingReqsDmaDesc;
+
+ PVSCSI_RING_CMP_DESC *RingCmps;
+ PVSCSI_DMA_DESC RingCmpsDmaDesc;
+} PVSCSI_RING_DESC;
+
#define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
typedef struct {
UINT32 Signature;
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 OriginalPciAttributes;
+ PVSCSI_RING_DESC RingDesc;
UINT8 MaxTarget;
UINT8 MaxLun;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index fcffc90d46c8..6200533698fc 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -26,6 +26,7 @@
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseLib
BaseMemoryLib
DebugLib
MemoryAllocationLib
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (12 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-30 16:06 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
` (4 subsequent siblings)
18 siblings, 1 reply; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
In case device is constrained by IOMMU or guest is running under AMD SEV,
input/output buffers provided to device (DataBuffer and SenseData) needs
to be explicitly mapped to device by PciIo->Map().
To avoid the overhead of mapping/unmapping the DataBuffer and SenseData
to the device for every SCSI requst (and to simplify code), introduce a
single DMA communication buffer that will be mapped to device on
initialization. When a SCSI request needs to be sent to device, the
DataBuffer and SenseData will be copied from/to the DMA communication
buffer as required. This will be done by the following commits.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 60 ++++++++++++++++++++++++++++++--------
OvmfPkg/PvScsiDxe/PvScsi.h | 20 +++++++++++++
2 files changed, 68 insertions(+), 12 deletions(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index c7d367e83a2d..6e350bb2d6e0 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -677,6 +677,19 @@ PvScsiInit (
goto RestorePciAttributes;
}
+ //
+ // Allocate DMA communication buffer
+ //
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
+ (VOID **)&Dev->DmaBuf,
+ &Dev->DmaBufDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRings;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -708,18 +721,7 @@ PvScsiInit (
return EFI_SUCCESS;
-RestorePciAttributes:
- PvScsiRestorePciAttributes (Dev);
-
- return Status;
-}
-
-STATIC
-VOID
-PvScsiUninit (
- IN OUT PVSCSI_DEV *Dev
- )
-{
+FreeRings:
//
// Reset device to stop device usage of the rings.
// This is required to safely free the rings.
@@ -728,6 +730,40 @@ PvScsiUninit (
PvScsiFreeRings (Dev);
+RestorePciAttributes:
+ PvScsiRestorePciAttributes (Dev);
+
+ return Status;
+}
+
+STATIC
+VOID
+PvScsiUninit (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ //
+ // Reset device to:
+ // - Make device stop processing all requests.
+ // - Stop device usage of the rings.
+ //
+ // This is required to safely free the DMA communication buffer
+ // and the rings.
+ //
+ PvScsiResetAdapter (Dev);
+
+ //
+ // Free DMA communication buffer
+ //
+ PvScsiFreeSharedPages (
+ Dev,
+ EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
+ Dev->DmaBuf,
+ &Dev->DmaBufDmaDesc
+ );
+
+ PvScsiFreeRings (Dev);
+
PvScsiRestorePciAttributes (Dev);
}
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 6d23b6e1eccf..fff12146dc75 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -31,6 +31,21 @@ typedef struct {
PVSCSI_DMA_DESC RingCmpsDmaDesc;
} PVSCSI_RING_DESC;
+typedef struct {
+ //
+ // As EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.SenseDataLength is defined
+ // as UINT8, defining here SenseData size to MAX_UINT8 will guarantee it
+ // cannot overflow when passed to device.
+ //
+ UINT8 SenseData[MAX_UINT8];
+ //
+ // This size of the data is arbitrarily chosen.
+ // It seems to be sufficient for all I/O requests sent through
+ // EFI_SCSI_PASS_THRU_PROTOCOL.PassThru() for common boot scenarios.
+ //
+ UINT8 Data[0x2000];
+} PVSCSI_DMA_BUFFER;
+
#define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
typedef struct {
@@ -38,6 +53,8 @@ typedef struct {
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 OriginalPciAttributes;
PVSCSI_RING_DESC RingDesc;
+ PVSCSI_DMA_BUFFER *DmaBuf;
+ PVSCSI_DMA_DESC DmaBufDmaDesc;
UINT8 MaxTarget;
UINT8 MaxLun;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
@@ -47,4 +64,7 @@ typedef struct {
#define PVSCSI_FROM_PASS_THRU(PassThruPointer) \
CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG)
+#define PVSCSI_DMA_BUF_DEV_ADDR(Dev, MemberName) \
+ (Dev->DmaBufDmaDesc.DeviceAddress + OFFSET_OF(PVSCSI_DMA_BUFFER, MemberName))
+
#endif // __PVSCSI_DXE_H_
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (13 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-30 16:22 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices() Liran Alon
` (3 subsequent siblings)
18 siblings, 1 reply; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
Implement EXT_SCSI_PASS_THRU.PassThru().
Machines should be able to boot after this commit.
Tested with Ubuntu 16.04 guest.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/OvmfPkg.dec | 6 +
OvmfPkg/PvScsiDxe/PvScsi.c | 456 +++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 +-
4 files changed, 465 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index a04aee5c2cd4..ff49ec0e9e6a 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -130,6 +130,12 @@
gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x36
gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x37
+ ## After PvScsiDxe sends a SCSI request to the device, it waits for
+ # the request completion in a polling loop.
+ # This constant defines how many micro-seconds to wait between each
+ # polling loop iteration.
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38
+
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 6e350bb2d6e0..da3535c75220 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -32,6 +32,27 @@
// Ext SCSI Pass Thru utilities
//
+/**
+ Reads a 32-bit value into BAR0 using MMIO
+**/
+STATIC
+EFI_STATUS
+PvScsiMmioRead32 (
+ IN CONST PVSCSI_DEV *Dev,
+ IN UINT64 Offset,
+ OUT UINT32 *Value
+ )
+{
+ return Dev->PciIo->Mem.Read (
+ Dev->PciIo,
+ EfiPciIoWidthUint32,
+ PCI_BAR_IDX0,
+ Offset,
+ 1, // Count
+ Value
+ );
+}
+
/**
Writes a 32-bit value into BAR0 using MMIO
**/
@@ -133,6 +154,383 @@ PvScsiResetAdapter (
return PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
}
+/**
+ Returns if PVSCSI request ring is full
+**/
+STATIC
+BOOLEAN
+PvScsiIsReqRingFull (
+ IN CONST PVSCSI_DEV *Dev
+ )
+{
+ PVSCSI_RINGS_STATE *RingsState;
+ UINT32 ReqNumEntries;
+
+ RingsState = Dev->RingDesc.RingState;
+ ReqNumEntries = 1U << RingsState->ReqNumEntriesLog2;
+ return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >= ReqNumEntries;
+}
+
+/**
+ Returns pointer to current request descriptor to produce
+**/
+STATIC
+PVSCSI_RING_REQ_DESC *
+PvScsiGetCurrentRequest (
+ IN CONST PVSCSI_DEV *Dev
+ )
+{
+ PVSCSI_RINGS_STATE *RingState;
+ UINT32 ReqNumEntries;
+
+ RingState = Dev->RingDesc.RingState;
+ ReqNumEntries = 1U << RingState->ReqNumEntriesLog2;
+ return Dev->RingDesc.RingReqs +
+ (RingState->ReqProdIdx & (ReqNumEntries - 1));
+}
+
+/**
+ Returns pointer to current completion descriptor to consume
+**/
+STATIC
+PVSCSI_RING_CMP_DESC *
+PvScsiGetCurrentResponse (
+ IN CONST PVSCSI_DEV *Dev
+ )
+{
+ PVSCSI_RINGS_STATE *RingState;
+ UINT32 CmpNumEntries;
+
+ RingState = Dev->RingDesc.RingState;
+ CmpNumEntries = 1U << RingState->CmpNumEntriesLog2;
+ return Dev->RingDesc.RingCmps +
+ (RingState->CmpConsIdx & (CmpNumEntries - 1));
+}
+
+/**
+ Wait for device to signal completion of submitted requests
+**/
+STATIC
+EFI_STATUS
+PvScsiWaitForRequestCompletion (
+ IN CONST PVSCSI_DEV *Dev
+ )
+{
+ EFI_STATUS Status;
+ UINT32 IntrStatus;
+
+ //
+ // Note: We don't yet support Timeout according to
+ // EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.Timeout.
+ //
+ // This is consistent with some other Scsi PassThru drivers
+ // such as VirtioScsi.
+ //
+ for (;;) {
+ Status = PvScsiMmioRead32 (Dev, PvScsiRegOffsetIntrStatus, &IntrStatus);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // PVSCSI_INTR_CMPL_MASK is set if device completed submitted requests
+ //
+ if ((IntrStatus & PVSCSI_INTR_CMPL_MASK) != 0) {
+ break;
+ }
+
+ gBS->Stall (Dev->WaitForCmpStallInUsecs);
+ }
+
+ //
+ // Acknowledge PVSCSI_INTR_CMPL_MASK in device interrupt-status register
+ //
+ return PvScsiMmioWrite32 (
+ Dev,
+ PvScsiRegOffsetIntrStatus,
+ PVSCSI_INTR_CMPL_MASK
+ );
+}
+
+/**
+ Create a fake host adapter error
+**/
+STATIC
+EFI_STATUS
+ReportHostAdapterError (
+ OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
+ )
+{
+ Packet->InTransferLength = 0;
+ Packet->OutTransferLength = 0;
+ Packet->SenseDataLength = 0;
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+ Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
+ return EFI_DEVICE_ERROR;
+}
+
+/**
+ Create a fake host adapter overrun error
+**/
+STATIC
+EFI_STATUS
+ReportHostAdapterOverrunError (
+ OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
+ )
+{
+ Packet->SenseDataLength = 0;
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+ Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
+ return EFI_BAD_BUFFER_SIZE;
+}
+
+/**
+ Populate a PVSCSI request descriptor from the Extended SCSI Pass Thru
+ Protocol packet.
+**/
+STATIC
+EFI_STATUS
+PopulateRequest (
+ IN CONST PVSCSI_DEV *Dev,
+ IN UINT8 *Target,
+ IN UINT64 Lun,
+ IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+ OUT PVSCSI_RING_REQ_DESC *Request
+ )
+{
+ UINT8 TargetValue;
+
+ //
+ // We only use first byte of target identifer
+ //
+ TargetValue = *Target;
+
+ //
+ // Check for unsupported requests
+ //
+ if (
+ //
+ // Bidirectional transfer was requested
+ //
+ (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
+ (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
+ //
+ // Command Descriptor Block bigger than this constant should be considered
+ // out-of-band. We currently don't support these CDBs.
+ //
+ (Packet->CdbLength > PVSCSI_CDB_MAX_SIZE)
+ ) {
+
+ //
+ // This error code doesn't require updates to the Packet output fields
+ //
+ return EFI_UNSUPPORTED;
+ }
+
+ //
+ // Check for invalid parameters
+ //
+ if (
+ //
+ // Addressed invalid device
+ //
+ (TargetValue > Dev->MaxTarget) || (Lun > Dev->MaxLun) ||
+ //
+ // Invalid direction (there doesn't seem to be a macro for the "no data
+ // transferred" "direction", eg. for TEST UNIT READY)
+ //
+ (Packet->DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
+ //
+ // Trying to receive, but destination pointer is NULL, or contradicting
+ // transfer direction
+ //
+ ((Packet->InTransferLength > 0) &&
+ ((Packet->InDataBuffer == NULL) ||
+ (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE)
+ )
+ ) ||
+ //
+ // Trying to send, but source pointer is NULL, or contradicting
+ // transfer direction
+ //
+ ((Packet->OutTransferLength > 0) &&
+ ((Packet->OutDataBuffer == NULL) ||
+ (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ)
+ )
+ )
+ ) {
+
+ //
+ // This error code doesn't require updates to the Packet output fields
+ //
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // Check for input/output buffer too large for DMA communication buffer
+ //
+ if (Packet->InTransferLength > sizeof (Dev->DmaBuf->Data)) {
+ Packet->InTransferLength = sizeof (Dev->DmaBuf->Data);
+ return ReportHostAdapterOverrunError (Packet);
+ }
+ if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) {
+ Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data);
+ return ReportHostAdapterOverrunError (Packet);
+ }
+
+ //
+ // Encode PVSCSI request
+ //
+ ZeroMem (Request, sizeof (*Request));
+
+ Request->Bus = 0;
+ Request->Target = TargetValue;
+ //
+ // This cast is safe as PVSCSI_DEV.MaxLun is defined as UINT8
+ //
+ Request->Lun[1] = (UINT8)Lun;
+ Request->SenseLen = Packet->SenseDataLength;
+ //
+ // DMA communication buffer SenseData overflow is not possible
+ // due to Packet->SenseDataLength defined as UINT8
+ //
+ Request->SenseAddr = PVSCSI_DMA_BUF_DEV_ADDR (Dev, SenseData);
+ Request->CdbLen = Packet->CdbLength;
+ CopyMem (Request->Cdb, Packet->Cdb, Packet->CdbLength);
+ Request->VcpuHint = 0;
+ Request->Tag = PVSCSI_SIMPLE_QUEUE_TAG;
+ if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+ Request->Flags = PVSCSI_FLAG_CMD_DIR_TOHOST;
+ Request->DataLen = Packet->InTransferLength;
+ } else {
+ Request->Flags = PVSCSI_FLAG_CMD_DIR_TODEVICE;
+ Request->DataLen = Packet->OutTransferLength;
+ CopyMem (
+ Dev->DmaBuf->Data,
+ Packet->OutDataBuffer,
+ Packet->OutTransferLength
+ );
+ }
+ Request->DataAddr = PVSCSI_DMA_BUF_DEV_ADDR (Dev, Data);
+
+ return EFI_SUCCESS;
+}
+
+/**
+ Handle the PVSCSI device response:
+ - Copy returned data from DMA communication buffer.
+ - Update fields in Extended SCSI Pass Thru Protocol packet as required.
+ - Translate response code to EFI status code and host adapter status.
+**/
+STATIC
+EFI_STATUS
+HandleResponse (
+ IN PVSCSI_DEV *Dev,
+ IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+ IN CONST PVSCSI_RING_CMP_DESC *Response
+ )
+{
+ //
+ // Fix SenseDataLength to amount of data returned
+ //
+ if (Packet->SenseDataLength > Response->SenseLen) {
+ Packet->SenseDataLength = (UINT8)Response->SenseLen;
+ }
+ //
+ // Copy sense data from DMA communication buffer
+ //
+ CopyMem (
+ Packet->SenseData,
+ Dev->DmaBuf->SenseData,
+ Packet->SenseDataLength
+ );
+
+ //
+ // Copy device output from DMA communication buffer
+ //
+ if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+ CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
+ }
+
+ //
+ // Report target status
+ //
+ Packet->TargetStatus = Response->ScsiStatus;
+
+ //
+ // Host adapter status and function return value depend on
+ // device response's host status
+ //
+ switch (Response->HostStatus) {
+ case PvScsiBtStatSuccess:
+ case PvScsiBtStatLinkedCommandCompleted:
+ case PvScsiBtStatLinkedCommandCompletedWithFlag:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+ return EFI_SUCCESS;
+
+ case PvScsiBtStatDataUnderrun:
+ //
+ // Report transferred amount in underrun
+ //
+ if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+ Packet->InTransferLength = (UINT32)Response->DataLen;
+ } else {
+ Packet->OutTransferLength = (UINT32)Response->DataLen;
+ }
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+ return EFI_SUCCESS;
+
+ case PvScsiBtStatDatarun:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+ return EFI_SUCCESS;
+
+ case PvScsiBtStatSelTimeout:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
+ return EFI_TIMEOUT;
+
+ case PvScsiBtStatBusFree:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE;
+ break;
+
+ case PvScsiBtStatInvPhase:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR;
+ break;
+
+ case PvScsiBtStatSensFailed:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED;
+ break;
+
+ case PvScsiBtStatTagReject:
+ case PvScsiBtStatBadMsg:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT;
+ break;
+
+ case PvScsiBtStatBusReset:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
+ break;
+
+ case PvScsiBtStatHaTimeout:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
+ return EFI_TIMEOUT;
+
+ case PvScsiBtStatScsiParity:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
+ break;
+
+ default:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+ break;
+ }
+
+ return EFI_DEVICE_ERROR;
+}
+
/**
Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
@@ -168,7 +566,62 @@ PvScsiPassThru (
IN EFI_EVENT Event OPTIONAL
)
{
- return EFI_UNSUPPORTED;
+ PVSCSI_DEV *Dev;
+ EFI_STATUS Status;
+ PVSCSI_RING_REQ_DESC *Request;
+ PVSCSI_RING_CMP_DESC *Response;
+
+ Dev = PVSCSI_FROM_PASS_THRU (This);
+
+ if (PvScsiIsReqRingFull (Dev)) {
+ return EFI_NOT_READY;
+ }
+
+ Request = PvScsiGetCurrentRequest (Dev);
+
+ Status = PopulateRequest (Dev, Target, Lun, Packet, Request);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Writes to Request must be globally visible before making request
+ // available to device
+ //
+ MemoryFence ();
+ Dev->RingDesc.RingState->ReqProdIdx++;
+
+ Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetKickRwIo, 0);
+ if (EFI_ERROR (Status)) {
+ //
+ // If kicking the host fails, we must fake a host adapter error.
+ // EFI_NOT_READY would save us the effort, but it would also suggest that
+ // the caller retry.
+ //
+ return ReportHostAdapterError (Packet);
+ }
+
+ Status = PvScsiWaitForRequestCompletion (Dev);
+ if (EFI_ERROR (Status)) {
+ //
+ // If waiting for request completion fails, we must fake a host adapter
+ // error. EFI_NOT_READY would save us the effort, but it would also suggest
+ // that the caller retry.
+ //
+ return ReportHostAdapterError (Packet);
+ }
+
+ Response = PvScsiGetCurrentResponse (Dev);
+ Status = HandleResponse (Dev, Packet, Response);
+
+ //
+ // Reads from response must complete before releasing completion entry
+ // to device
+ //
+ MemoryFence ();
+ Dev->RingDesc.RingState->CmpConsIdx++;
+
+ return Status;
}
STATIC
@@ -652,6 +1105,7 @@ PvScsiInit (
//
Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
+ Dev->WaitForCmpStallInUsecs = PcdGet32 (PcdPvScsiWaitForCmpStallInUsecs);
//
// Set PCI Attributes
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index fff12146dc75..02feac734743 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -57,6 +57,7 @@ typedef struct {
PVSCSI_DMA_DESC DmaBufDmaDesc;
UINT8 MaxTarget;
UINT8 MaxLun;
+ UINTN WaitForCmpStallInUsecs;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
} PVSCSI_DEV;
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index 6200533698fc..284035fb10d4 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -39,5 +39,6 @@
gEfiPciIoProtocolGuid ## TO_START
[Pcd]
- gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES
- gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs ## CONSUMES
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices()
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (14 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
@ 2020-03-28 20:00 ` Liran Alon
2020-03-30 16:23 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:01 ` [PATCH v3 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses Liran Alon
` (2 subsequent siblings)
18 siblings, 1 reply; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:00 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
This causes the device to forget about the request/completion rings.
We allocated said rings in EfiBootServicesData type memory, and code
executing after ExitBootServices() is permitted to overwrite it.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 43 +++++++++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index da3535c75220..d7f0d3c8790c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -1221,6 +1221,31 @@ PvScsiUninit (
PvScsiRestorePciAttributes (Dev);
}
+/**
+ Event notification called by ExitBootServices()
+**/
+STATIC
+VOID
+EFIAPI
+PvScsiExitBoot (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ PVSCSI_DEV *Dev;
+
+ Dev = Context;
+ DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
+
+ //
+ // Reset the device to stop device usage of the rings.
+ //
+ // We allocated said rings in EfiBootServicesData type memory, and code
+ // executing after ExitBootServices() is permitted to overwrite it.
+ //
+ PvScsiResetAdapter (Dev);
+}
+
//
// Driver Binding
//
@@ -1314,6 +1339,17 @@ PvScsiDriverBindingStart (
goto ClosePciIo;
}
+ Status = gBS->CreateEvent (
+ EVT_SIGNAL_EXIT_BOOT_SERVICES,
+ TPL_CALLBACK,
+ &PvScsiExitBoot,
+ Dev,
+ &Dev->ExitBoot
+ );
+ if (EFI_ERROR (Status)) {
+ goto UninitDev;
+ }
+
//
// Setup complete, attempt to export the driver instance's PassThru interface
//
@@ -1325,11 +1361,14 @@ PvScsiDriverBindingStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto UninitDev;
+ goto CloseExitBoot;
}
return EFI_SUCCESS;
+CloseExitBoot:
+ gBS->CloseEvent (Dev->ExitBoot);
+
UninitDev:
PvScsiUninit (Dev);
@@ -1384,6 +1423,8 @@ PvScsiDriverBindingStop (
return Status;
}
+ gBS->CloseEvent (Dev->ExitBoot);
+
PvScsiUninit (Dev);
gBS->CloseProtocol (
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 02feac734743..544359ebc05c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -51,6 +51,7 @@ typedef struct {
typedef struct {
UINT32 Signature;
EFI_PCI_IO_PROTOCOL *PciIo;
+ EFI_EVENT ExitBoot;
UINT64 OriginalPciAttributes;
PVSCSI_RING_DESC RingDesc;
PVSCSI_DMA_BUFFER *DmaBuf;
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (15 preceding siblings ...)
2020-03-28 20:00 ` [PATCH v3 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices() Liran Alon
@ 2020-03-28 20:01 ` Liran Alon
2020-03-29 9:29 ` [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Nikita Leshenko
2020-03-30 16:53 ` [edk2-devel] " Laszlo Ersek
18 siblings, 0 replies; 27+ messages in thread
From: Liran Alon @ 2020-03-28 20:01 UTC (permalink / raw)
To: devel, lersek
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel,
Liran Alon
Enable PCI dual-address cycle attribute to signal device
supports 64-bit DMA addresses.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index d7f0d3c8790c..0a66c98421a9 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -874,6 +874,29 @@ PvScsiSetPciAttributes (
return Status;
}
+ //
+ // Signal device supports 64-bit DMA addresses
+ //
+ Status = Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationEnable,
+ EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ //
+ // Warn user that device will only be using 32-bit DMA addresses.
+ //
+ // Note that this does not prevent the device/driver from working
+ // and therefore we only warn and continue as usual.
+ //
+ DEBUG ((
+ DEBUG_WARN,
+ "%a: failed to enable 64-bit DMA addresses\n",
+ __FUNCTION__
+ ));
+ }
+
return EFI_SUCCESS;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (16 preceding siblings ...)
2020-03-28 20:01 ` [PATCH v3 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses Liran Alon
@ 2020-03-29 9:29 ` Nikita Leshenko
2020-03-30 16:53 ` [edk2-devel] " Laszlo Ersek
18 siblings, 0 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-03-29 9:29 UTC (permalink / raw)
To: Liran Alon; +Cc: devel, lersek, aaron.young, jordan.l.justen, ard.biesheuvel
> On 28 Mar 2020, at 23:00, Liran Alon <liran.alon@oracle.com> wrote:
>
> Hi,
>
> This series adds driver support for VMware PVSCSI controller.
>
> This controller is supported by VMware and QEMU. This work is part of
> the more general agenda of enhancing OVMF boot device support to have
> feature parity with SeaBIOS (Which supports booting from VMware PVSCSI).
>
> I pushed a copy of these (v3) patches to https://github.com/nikital/edk2/tree/pvscsi6
> The v2 patches can be found at https://github.com/nikital/edk2/tree/pvscsi5
> The v1 patches can be found at https://github.com/nikital/edk2/tree/pvscsi4
>
> Regards,
> -Liran
>
> v2->v3:
> * Add function documentation to PvScsiWriteCmdDesc with “@param” to explain DescWords alignment requirement. [Laszlo]
> * Also set Packet’s HostAdapterStatus and TargetStatus when returning EFI_BAD_BUFFER_SIZE from PassThru() method. [Liran]
> * Add comments explaining why DMA communication buffer fields sizes are defined as they are. [Laszlo]
> * Remove unnecessary (UINT64) casts from EFI_PHYSICAL_ADDRESS expressions. [Laszlo]
> * Changed HandleResponse() to always copy SenseData based on Response->SenseLen. Not only if ScsiStatus equal to CHECK. [Liran]
> * Changed HandleResponse() to update Packet TransferLength only on underrun. [Liran]
> * Changed PassThru() to return EFI_STATUS_SUCCESS on device return overrun/underrun. [Laszlo & Liran]
> * Removed unnecessary PvScsiAllocatePages(), PvScsiFreePages(), PvScsiMapBuffer() and PvScsiUnmapBuffer() utility functions. [Laszlo]
> * Changed PvScsiInitRings() to align PVSCSI_CMD_DESC_SETUP_RINGS command to UINT32 before using it with EfiPciIoWidthFifoUint32. [Laszlo]
> * Removed PciIoOperation parameter from PvScsiAllocateSharedPages(). [Laszlo
> * Added STATIC_ASSERT() to verify PVSCSI_CMD_DESC_SETUP_RINGS is of size multiple of UINT32 words [Laszlo].
> * Added reset device before either freeing PVSCSI rings or DMA communication buffer. [Laszlo]
> * Removed unnecessary cast to (VOID **) in call to PvScsiFreeSharedPages() in PvScsiUninit(). [Laszlo]
> * Added #include <Library/BaseLib.h> and BaseLib to PvScsiDxe.inf because of RShiftU64() usage. [Laszlo]
>
> v1->v2:
> * Removed Nikita’s Reviewed-By tags. [Laszlo]
> * Renamed PvScsi.inf to PvScsiDxe.inf and fixed references from all DSC files. [Laszlo]
> * Changed “!ifdef $(PVSCSI_ENABLE)” in DSC files to “!if $(PVSCSI_ENABLE) == TRUE”. [Laszlo]
> * Fix Identation in various places. [Laszlo]
> * Added “#include <Uefi/UefiSpec.h>” for EFI_SYSTEM_TABLE. [Laszlo]
> * Fix various typos. [Laszlo]
> * Made “STATIC” on same line of object delcerations. [Laszlo]
> * Added Laszlo’s Reviewed-by tags on some patches. [Laszlo]
> * Added missing spaces before “(“ on various function calls. [Laszlo]
> * Added PvScsi.h header file to INF [Sources] section. [Laszlo]
> * Changed [Protocols] section in INF file to be lexicographically sorted. [Laszlo]
> * Changed [PCDs] section in INF file to be lexigraphically sorted. [Laszlo]
> * Fixed function comments blocks to be “/** **/” instead of “//” style. [Laszlo]
> * Changed PvScsiGetTargetLun() to ZeroMem() all target bytes except first one. [Laszlo]
> * Replaced “IOSpace” with “MMIO-Space” in comments. [Laszlo]
> * Changed enums to match EDK2 coding convention. [Laszlo]
> * Use PCI_BAR_IDX0 instead of hard-coded 0. [Laszlo]
> * Use EFI_PAGES_TO_SIZE() instead of manually multiplying with EFI_PAGE_SIZE. [Laszlo]
> * Use RShiftU64() to shift UINT64 vars. [Laszlo]
> * Changed ReqNumEntries var to UINT32 and shift to use “1U <<” instead of “1 <<”. [Laszlo]
> * Changed condition on flag (In PvScsiWaitForRequestCompletion()) to be a boolean expression. [Laszlo]
> * Replaced “FakeHostAdapterError” label with a utility function. [Laszlo]
> * Added debug message to PvScsiExitBoot() to assist debugging. [Laszlo]
> * Fixed resource management to make each function either completely succeed or completely fail and free all resources. [Laszlo]
> * Changed PvScsiWriteCmdDesc() to use EfiPciIoWidthFifoUint32. [Laszlo]
> * Changed PvScsiWriteCmdDesc() prototype to make clear it descriptor must be an array of words. [Laszlo]
>
The entire series looks good to me:
Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init
2020-03-28 20:00 ` [PATCH v3 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
@ 2020-03-30 15:19 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-03-30 15:19 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/28/20 21:00, Liran Alon wrote:
> The following commits will complete the implementation of
> device initialization.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 114 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 114 insertions(+)
Thanks for the updates in this patch, my R-b stands.
Laszlo
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 531bed4e5ab7..cf75884350ee 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -30,6 +30,107 @@
> // Ext SCSI Pass Thru utilities
> //
>
> +/**
> + Writes a 32-bit value into BAR0 using MMIO
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiMmioWrite32 (
> + IN CONST PVSCSI_DEV *Dev,
> + IN UINT64 Offset,
> + IN UINT32 Value
> + )
> +{
> + return Dev->PciIo->Mem.Write (
> + Dev->PciIo,
> + EfiPciIoWidthUint32,
> + PCI_BAR_IDX0,
> + Offset,
> + 1, // Count
> + &Value
> + );
> +}
> +
> +/**
> + Writes multiple words of data into BAR0 using MMIO
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiMmioWrite32Multiple (
> + IN CONST PVSCSI_DEV *Dev,
> + IN UINT64 Offset,
> + IN UINTN Count,
> + IN UINT32 *Words
> + )
> +{
> + return Dev->PciIo->Mem.Write (
> + Dev->PciIo,
> + EfiPciIoWidthFifoUint32,
> + PCI_BAR_IDX0,
> + Offset,
> + Count,
> + Words
> + );
> +}
> +
> +/**
> + Send a PVSCSI command to device.
> +
> + @param[in] Dev The pvscsi host device.
> + @param[in] Cmd The command to send to device.
> + @param[in] OPTIONAL DescWords An optional command descriptor (If command
> + have a descriptor). The descriptor is
> + provided as an array of UINT32 words and
> + is must be 32-bit aligned.
> + @param[in] DescWordsCount The number of words in command descriptor.
> + Caller must specify here 0 if DescWords
> + is not supplied (It is optional). In that
> + case, DescWords is ignored.
> +
> + @return Status codes returned by Dev->PciIo->Mem.Write().
> +
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiWriteCmdDesc (
> + IN CONST PVSCSI_DEV *Dev,
> + IN UINT32 Cmd,
> + IN UINT32 *DescWords OPTIONAL,
> + IN UINTN DescWordsCount
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (DescWordsCount > PVSCSI_MAX_CMD_DATA_WORDS) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetCommand, Cmd);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (DescWordsCount > 0) {
> + return PvScsiMmioWrite32Multiple (
> + Dev,
> + PvScsiRegOffsetCommandData,
> + DescWordsCount,
> + DescWords
> + );
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiResetAdapter (
> + IN CONST PVSCSI_DEV *Dev
> + )
> +{
> + return PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
> +}
> +
> /**
> Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
> EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -357,6 +458,14 @@ PvScsiInit (
> return Status;
> }
>
> + //
> + // Reset adapter
> + //
> + Status = PvScsiResetAdapter (Dev);
> + if (EFI_ERROR (Status)) {
> + goto RestorePciAttributes;
> + }
> +
> //
> // Populate the exported interface's attributes
> //
> @@ -387,6 +496,11 @@ PvScsiInit (
> Dev->PassThruMode.IoAlign = 0;
>
> return EFI_SUCCESS;
> +
> +RestorePciAttributes:
> + PvScsiRestorePciAttributes (Dev);
> +
> + return Status;
> }
>
> STATIC
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings
2020-03-28 20:00 ` [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
@ 2020-03-30 15:54 ` Laszlo Ersek
2020-03-30 17:24 ` Liran Alon
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-03-30 15:54 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/28/20 21:00, Liran Alon wrote:
> These rings are shared memory buffers between host and device in which
> a cyclic buffer is managed to send request descriptors from host to
> device and receive completion descriptors from device to host.
>
> Note that because device may be constrained by IOMMU or guest may be run
> under AMD SEV, we make sure to map these rings to device by using
> PciIo->Map().
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 219 ++++++++++++++++++++++++++++++++
> OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++
> OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 1 +
> 3 files changed, 237 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index cf75884350ee..c7d367e83a2d 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -11,11 +11,13 @@
>
> #include <IndustryStandard/Pci.h>
> #include <IndustryStandard/PvScsi.h>
> +#include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
> #include <Uefi/UefiSpec.h>
>
> #include "PvScsi.h"
> @@ -436,6 +438,207 @@ PvScsiRestorePciAttributes (
> );
> }
>
> +STATIC
> +EFI_STATUS
> +PvScsiAllocateSharedPages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + OUT VOID **HostAddress,
> + OUT PVSCSI_DMA_DESC *DmaDesc
> + )
> +{
> + EFI_STATUS Status;
> + UINTN NumberOfBytes;
> +
> + Status = Dev->PciIo->AllocateBuffer (
> + Dev->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + Pages,
> + HostAddress,
> + EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + NumberOfBytes = EFI_PAGES_TO_SIZE (Pages);
> + Status = Dev->PciIo->Map (
> + Dev->PciIo,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + *HostAddress,
> + &NumberOfBytes,
> + &DmaDesc->DeviceAddress,
> + &DmaDesc->Mapping
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBuffer;
> + }
> +
> + if (NumberOfBytes != EFI_PAGES_TO_SIZE (Pages)) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Unmap;
> + }
> +
> + return EFI_SUCCESS;
> +
> +Unmap:
> + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +
> +FreeBuffer:
> + Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, *HostAddress);
> +
> + return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeSharedPages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + IN VOID *HostAddress,
> + IN PVSCSI_DMA_DESC *DmaDesc
> + )
> +{
> + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> + Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, HostAddress);
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiInitRings (
> + IN OUT PVSCSI_DEV *Dev
> + )
> +{
> + EFI_STATUS Status;
> + union {
> + PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
> + UINT32 Uint32;
> + } AlignedCmd;
> + PVSCSI_CMD_DESC_SETUP_RINGS *Cmd;
> +
> + Cmd = &AlignedCmd.Cmd;
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + (VOID **)&Dev->RingDesc.RingState,
> + &Dev->RingDesc.RingStateDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + (VOID **)&Dev->RingDesc.RingReqs,
> + &Dev->RingDesc.RingReqsDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeRingState;
> + }
> + ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + (VOID **)&Dev->RingDesc.RingCmps,
> + &Dev->RingDesc.RingCmpsDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeRingReqs;
> + }
> + ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
> +
> + ZeroMem (Cmd, sizeof (*Cmd));
> + Cmd->ReqRingNumPages = 1;
> + Cmd->CmpRingNumPages = 1;
> + Cmd->RingsStatePPN = RShiftU64 (
> + Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> + Cmd->ReqRingPPNs[0] = RShiftU64 (
> + Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> + Cmd->CmpRingPPNs[0] = RShiftU64 (
> + Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> +
> + STATIC_ASSERT (
> + sizeof (*Cmd) % sizeof (UINT32) == 0,
> + "Cmd must be multiple of 32-bit words"
> + );
> + Status = PvScsiWriteCmdDesc (
> + Dev,
> + PvScsiCmdSetupRings,
> + (UINT32 *)Cmd,
> + sizeof (*Cmd) / sizeof (UINT32)
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeRingCmps;
> + }
> +
> + return EFI_SUCCESS;
> +
> +FreeRingCmps:
> + PvScsiFreeSharedPages (
> + Dev,
> + 1,
> + Dev->RingDesc.RingCmps,
> + &Dev->RingDesc.RingCmpsDmaDesc
> + );
> +
> +FreeRingReqs:
> + PvScsiFreeSharedPages (
> + Dev,
> + 1,
> + Dev->RingDesc.RingReqs,
> + &Dev->RingDesc.RingReqsDmaDesc
> + );
> +
> +FreeRingState:
> + PvScsiFreeSharedPages (
> + Dev,
> + 1,
> + Dev->RingDesc.RingState,
> + &Dev->RingDesc.RingStateDmaDesc
> + );
> +
> + return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeRings (
> + IN OUT PVSCSI_DEV *Dev
> + )
> +{
> + PvScsiFreeSharedPages (
> + Dev,
> + 1,
> + Dev->RingDesc.RingCmps,
> + &Dev->RingDesc.RingCmpsDmaDesc
> + );
> +
> + PvScsiFreeSharedPages (
> + Dev,
> + 1,
> + Dev->RingDesc.RingReqs,
> + &Dev->RingDesc.RingReqsDmaDesc
> + );
> +
> + PvScsiFreeSharedPages (
> + Dev,
> + 1,
> + Dev->RingDesc.RingState,
> + &Dev->RingDesc.RingStateDmaDesc
> + );
> +}
> +
> STATIC
> EFI_STATUS
> PvScsiInit (
> @@ -466,6 +669,14 @@ PvScsiInit (
> goto RestorePciAttributes;
> }
>
> + //
> + // Init PVSCSI rings
> + //
> + Status = PvScsiInitRings (Dev);
> + if (EFI_ERROR (Status)) {
> + goto RestorePciAttributes;
> + }
> +
> //
> // Populate the exported interface's attributes
> //
> @@ -509,6 +720,14 @@ PvScsiUninit (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> + //
> + // Reset device to stop device usage of the rings.
> + // This is required to safely free the rings.
> + //
> + PvScsiResetAdapter (Dev);
If I understand correctly (at the end of the series):
in order to save one of the (ultimately) two reset calls in
PvScsiUninit(), namely
- one for releasing the DMA buf
- and the other (internal to PvScsiFreeRings()) for releasing the rings,
you unnested the latter reset call from PvScsiFreeRings(), and added it
manually to the error path of PvScsiInit().
To me, that's more brittle and more difficult to reason about than what
I proposed, because now PvScsiFreeRings() does not completely undo
PvScsiInitRings(). I specifically requested that we please not try to
save on the two (seemingly) redundant reset calls in PvScsiUninit() --
see the paragraph containing "bad idea" here:
http://mid.mail-archive.com/45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com
https://edk2.groups.io/g/devel/message/56425
(Note: we could be tempted to somehow "centralize" all of these
Reset operations into a single spot. Bad idea. We are revoking the
device's access rights to different resources, so the revocation
operations will show up in different spots. It's a mere circumstance
that the revocations all happen to be Reset operations.)
I might be paranoid of course -- I just feel that maybe-superfluous
reset operations on error paths are much better than silently
corrupted guest memory and/or disk contents.
You reacted to that message of mine, but not this paragraph specifically
(it was snipped from your followup) -- so I thought you were OK with it.
I'm a bit disappointed that you disagreed with my request *silently*.
To summarize: technically, your solution is correct; from a code hygiene
and resource ownership question, I'm convinced it is wrong.
But, I'll live with it.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(Also, you didn't set up the "diff.orderFile" knob the way I requested
in point (8):
http://mid.mail-archive.com/0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com
https://edk2.groups.io/g/devel/message/56420
and so the header file changes are again at the end of the patch...
Another thing I'll have to live with, I guess.)
Laszlo
> +
> + PvScsiFreeRings (Dev);
> +
> PvScsiRestorePciAttributes (Dev);
> }
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 5f611dbbc98c..6d23b6e1eccf 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -15,12 +15,29 @@
> #include <Library/DebugLib.h>
> #include <Protocol/ScsiPassThruExt.h>
>
> +typedef struct {
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *Mapping;
> +} PVSCSI_DMA_DESC;
> +
> +typedef struct {
> + PVSCSI_RINGS_STATE *RingState;
> + PVSCSI_DMA_DESC RingStateDmaDesc;
> +
> + PVSCSI_RING_REQ_DESC *RingReqs;
> + PVSCSI_DMA_DESC RingReqsDmaDesc;
> +
> + PVSCSI_RING_CMP_DESC *RingCmps;
> + PVSCSI_DMA_DESC RingCmpsDmaDesc;
> +} PVSCSI_RING_DESC;
> +
> #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>
> typedef struct {
> UINT32 Signature;
> EFI_PCI_IO_PROTOCOL *PciIo;
> UINT64 OriginalPciAttributes;
> + PVSCSI_RING_DESC RingDesc;
> UINT8 MaxTarget;
> UINT8 MaxLun;
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> index fcffc90d46c8..6200533698fc 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> @@ -26,6 +26,7 @@
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + BaseLib
> BaseMemoryLib
> DebugLib
> MemoryAllocationLib
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-28 20:00 ` [PATCH v3 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
@ 2020-03-30 16:06 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-03-30 16:06 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/28/20 21:00, Liran Alon wrote:
> In case device is constrained by IOMMU or guest is running under AMD SEV,
> input/output buffers provided to device (DataBuffer and SenseData) needs
> to be explicitly mapped to device by PciIo->Map().
>
> To avoid the overhead of mapping/unmapping the DataBuffer and SenseData
> to the device for every SCSI requst (and to simplify code), introduce a
> single DMA communication buffer that will be mapped to device on
> initialization. When a SCSI request needs to be sent to device, the
> DataBuffer and SenseData will be copied from/to the DMA communication
> buffer as required. This will be done by the following commits.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 60 ++++++++++++++++++++++++++++++--------
> OvmfPkg/PvScsiDxe/PvScsi.h | 20 +++++++++++++
> 2 files changed, 68 insertions(+), 12 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-28 20:00 ` [PATCH v3 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
@ 2020-03-30 16:22 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-03-30 16:22 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/28/20 21:00, Liran Alon wrote:
> Implement EXT_SCSI_PASS_THRU.PassThru().
>
> Machines should be able to boot after this commit.
> Tested with Ubuntu 16.04 guest.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/OvmfPkg.dec | 6 +
> OvmfPkg/PvScsiDxe/PvScsi.c | 456 +++++++++++++++++++++++++++++++-
> OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
> OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 +-
> 4 files changed, 465 insertions(+), 3 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices()
2020-03-28 20:00 ` [PATCH v3 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices() Liran Alon
@ 2020-03-30 16:23 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-03-30 16:23 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/28/20 21:00, Liran Alon wrote:
> This causes the device to forget about the request/completion rings.
> We allocated said rings in EfiBootServicesData type memory, and code
> executing after ExitBootServices() is permitted to overwrite it.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 43 +++++++++++++++++++++++++++++++++++++-
> OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index da3535c75220..d7f0d3c8790c 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -1221,6 +1221,31 @@ PvScsiUninit (
> PvScsiRestorePciAttributes (Dev);
> }
>
> +/**
> + Event notification called by ExitBootServices()
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PvScsiExitBoot (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + PVSCSI_DEV *Dev;
> +
> + Dev = Context;
> + DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
> +
> + //
> + // Reset the device to stop device usage of the rings.
> + //
> + // We allocated said rings in EfiBootServicesData type memory, and code
> + // executing after ExitBootServices() is permitted to overwrite it.
> + //
> + PvScsiResetAdapter (Dev);
> +}
> +
My R-b stands.
Thanks
Laszlo
> //
> // Driver Binding
> //
> @@ -1314,6 +1339,17 @@ PvScsiDriverBindingStart (
> goto ClosePciIo;
> }
>
> + Status = gBS->CreateEvent (
> + EVT_SIGNAL_EXIT_BOOT_SERVICES,
> + TPL_CALLBACK,
> + &PvScsiExitBoot,
> + Dev,
> + &Dev->ExitBoot
> + );
> + if (EFI_ERROR (Status)) {
> + goto UninitDev;
> + }
> +
> //
> // Setup complete, attempt to export the driver instance's PassThru interface
> //
> @@ -1325,11 +1361,14 @@ PvScsiDriverBindingStart (
> &Dev->PassThru
> );
> if (EFI_ERROR (Status)) {
> - goto UninitDev;
> + goto CloseExitBoot;
> }
>
> return EFI_SUCCESS;
>
> +CloseExitBoot:
> + gBS->CloseEvent (Dev->ExitBoot);
> +
> UninitDev:
> PvScsiUninit (Dev);
>
> @@ -1384,6 +1423,8 @@ PvScsiDriverBindingStop (
> return Status;
> }
>
> + gBS->CloseEvent (Dev->ExitBoot);
> +
> PvScsiUninit (Dev);
>
> gBS->CloseProtocol (
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 02feac734743..544359ebc05c 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -51,6 +51,7 @@ typedef struct {
> typedef struct {
> UINT32 Signature;
> EFI_PCI_IO_PROTOCOL *PciIo;
> + EFI_EVENT ExitBoot;
> UINT64 OriginalPciAttributes;
> PVSCSI_RING_DESC RingDesc;
> PVSCSI_DMA_BUFFER *DmaBuf;
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (17 preceding siblings ...)
2020-03-29 9:29 ` [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Nikita Leshenko
@ 2020-03-30 16:53 ` Laszlo Ersek
18 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-03-30 16:53 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/28/20 21:00, Liran Alon wrote:
> Hi,
>
> This series adds driver support for VMware PVSCSI controller.
>
> This controller is supported by VMware and QEMU. This work is part of
> the more general agenda of enhancing OVMF boot device support to have
> feature parity with SeaBIOS (Which supports booting from VMware PVSCSI).
>
> I pushed a copy of these (v3) patches to https://github.com/nikital/edk2/tree/pvscsi6
> The v2 patches can be found at https://github.com/nikital/edk2/tree/pvscsi5
> The v1 patches can be found at https://github.com/nikital/edk2/tree/pvscsi4
Merged in commit range 6c1fb56802d5..f34c7645bd87, via
<https://github.com/tianocore/edk2/pull/474>.
Thanks for the contribution!
Laszlo
> v2->v3:
> * Add function documentation to PvScsiWriteCmdDesc with “@param” to explain DescWords alignment requirement. [Laszlo]
> * Also set Packet’s HostAdapterStatus and TargetStatus when returning EFI_BAD_BUFFER_SIZE from PassThru() method. [Liran]
> * Add comments explaining why DMA communication buffer fields sizes are defined as they are. [Laszlo]
> * Remove unnecessary (UINT64) casts from EFI_PHYSICAL_ADDRESS expressions. [Laszlo]
> * Changed HandleResponse() to always copy SenseData based on Response->SenseLen. Not only if ScsiStatus equal to CHECK. [Liran]
> * Changed HandleResponse() to update Packet TransferLength only on underrun. [Liran]
> * Changed PassThru() to return EFI_STATUS_SUCCESS on device return overrun/underrun. [Laszlo & Liran]
> * Removed unnecessary PvScsiAllocatePages(), PvScsiFreePages(), PvScsiMapBuffer() and PvScsiUnmapBuffer() utility functions. [Laszlo]
> * Changed PvScsiInitRings() to align PVSCSI_CMD_DESC_SETUP_RINGS command to UINT32 before using it with EfiPciIoWidthFifoUint32. [Laszlo]
> * Removed PciIoOperation parameter from PvScsiAllocateSharedPages(). [Laszlo
> * Added STATIC_ASSERT() to verify PVSCSI_CMD_DESC_SETUP_RINGS is of size multiple of UINT32 words [Laszlo].
> * Added reset device before either freeing PVSCSI rings or DMA communication buffer. [Laszlo]
> * Removed unnecessary cast to (VOID **) in call to PvScsiFreeSharedPages() in PvScsiUninit(). [Laszlo]
> * Added #include <Library/BaseLib.h> and BaseLib to PvScsiDxe.inf because of RShiftU64() usage. [Laszlo]
>
> v1->v2:
> * Removed Nikita’s Reviewed-By tags. [Laszlo]
> * Renamed PvScsi.inf to PvScsiDxe.inf and fixed references from all DSC files. [Laszlo]
> * Changed “!ifdef $(PVSCSI_ENABLE)” in DSC files to “!if $(PVSCSI_ENABLE) == TRUE”. [Laszlo]
> * Fix Identation in various places. [Laszlo]
> * Added “#include <Uefi/UefiSpec.h>” for EFI_SYSTEM_TABLE. [Laszlo]
> * Fix various typos. [Laszlo]
> * Made “STATIC” on same line of object delcerations. [Laszlo]
> * Added Laszlo’s Reviewed-by tags on some patches. [Laszlo]
> * Added missing spaces before “(“ on various function calls. [Laszlo]
> * Added PvScsi.h header file to INF [Sources] section. [Laszlo]
> * Changed [Protocols] section in INF file to be lexicographically sorted. [Laszlo]
> * Changed [PCDs] section in INF file to be lexigraphically sorted. [Laszlo]
> * Fixed function comments blocks to be “/** **/” instead of “//” style. [Laszlo]
> * Changed PvScsiGetTargetLun() to ZeroMem() all target bytes except first one. [Laszlo]
> * Replaced “IOSpace” with “MMIO-Space” in comments. [Laszlo]
> * Changed enums to match EDK2 coding convention. [Laszlo]
> * Use PCI_BAR_IDX0 instead of hard-coded 0. [Laszlo]
> * Use EFI_PAGES_TO_SIZE() instead of manually multiplying with EFI_PAGE_SIZE. [Laszlo]
> * Use RShiftU64() to shift UINT64 vars. [Laszlo]
> * Changed ReqNumEntries var to UINT32 and shift to use “1U <<” instead of “1 <<”. [Laszlo]
> * Changed condition on flag (In PvScsiWaitForRequestCompletion()) to be a boolean expression. [Laszlo]
> * Replaced “FakeHostAdapterError” label with a utility function. [Laszlo]
> * Added debug message to PvScsiExitBoot() to assist debugging. [Laszlo]
> * Fixed resource management to make each function either completely succeed or completely fail and free all resources. [Laszlo]
> * Changed PvScsiWriteCmdDesc() to use EfiPciIoWidthFifoUint32. [Laszlo]
> * Changed PvScsiWriteCmdDesc() prototype to make clear it descriptor must be an array of words. [Laszlo]
>
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings
2020-03-30 15:54 ` [edk2-devel] " Laszlo Ersek
@ 2020-03-30 17:24 ` Liran Alon
2020-03-30 20:46 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Liran Alon @ 2020-03-30 17:24 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 30/03/2020 18:54, Laszlo Ersek wrote:
> On 03/28/20 21:00, Liran Alon wrote:
>> STATIC
>> EFI_STATUS
>> PvScsiInit (
>> @@ -466,6 +669,14 @@ PvScsiInit (
>> goto RestorePciAttributes;
>> }
>>
>> + //
>> + // Init PVSCSI rings
>> + //
>> + Status = PvScsiInitRings (Dev);
>> + if (EFI_ERROR (Status)) {
>> + goto RestorePciAttributes;
>> + }
>> +
>> //
>> // Populate the exported interface's attributes
>> //
>> @@ -509,6 +720,14 @@ PvScsiUninit (
>> IN OUT PVSCSI_DEV *Dev
>> )
>> {
>> + //
>> + // Reset device to stop device usage of the rings.
>> + // This is required to safely free the rings.
>> + //
>> + PvScsiResetAdapter (Dev);
> If I understand correctly (at the end of the series):
>
> in order to save one of the (ultimately) two reset calls in
> PvScsiUninit(), namely
> - one for releasing the DMA buf
> - and the other (internal to PvScsiFreeRings()) for releasing the rings,
> you unnested the latter reset call from PvScsiFreeRings(), and added it
> manually to the error path of PvScsiInit().
Right.
>
> To me, that's more brittle and more difficult to reason about than what
> I proposed, because now PvScsiFreeRings() does not completely undo
> PvScsiInitRings().
Hmm right. Because PvScsiInitRings() also setup the ring to the device
with a command. Haven't thought about it.
> I specifically requested that we please not try to
> save on the two (seemingly) redundant reset calls in PvScsiUninit() --
> see the paragraph containing "bad idea" here:
>
> https://urldefense.com/v3/__http://mid.mail-archive.com/45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-Fn-EyPU$
> https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56425__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-4bfLR8U$
>
> (Note: we could be tempted to somehow "centralize" all of these
> Reset operations into a single spot. Bad idea. We are revoking the
> device's access rights to different resources, so the revocation
> operations will show up in different spots. It's a mere circumstance
> that the revocations all happen to be Reset operations.)
>
> I might be paranoid of course -- I just feel that maybe-superfluous
> reset operations on error paths are much better than silently
> corrupted guest memory and/or disk contents.
>
> You reacted to that message of mine, but not this paragraph specifically
> (it was snipped from your followup) -- so I thought you were OK with it.
> I'm a bit disappointed that you disagreed with my request *silently*.
Oh now I got what you meant! I misinterpreted it. Not silently ignored
it of course!
I can change this in a v4 patch-series if you would like that. Sorry for
the misunderstanding.
I do agree with your statement that PvScsiFreeRings() is not completely
an undo of PvScsiInitRings().
And maybe for making code a little bit easier to reason about, it's
preferred to just do one additional unnecessary device reset.
>
> To summarize: technically, your solution is correct; from a code hygiene
> and resource ownership question, I'm convinced it is wrong.
I agree.
>
> But, I'll live with it.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
I can send a v4 patch-series just changing that if you would like.
Or submit a patch to change it after it will be merged.
Or that you will change it when applying. Or leave it as is.
I don't have any objection to any of the above.
>
> (Also, you didn't set up the "diff.orderFile" knob the way I requested
> in point (8):
I actually did set it up in my ~/.gitconfig. It seems that it doesn't
work from some reason...
>
> https://urldefense.com/v3/__http://mid.mail-archive.com/0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-bs34O4s$
> https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56420__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-msgEmGo$
>
> and so the header file changes are again at the end of the patch...
> Another thing I'll have to live with, I guess.)
>
> Laszlo
Thanks for the understanding,
-Liran
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings
2020-03-30 17:24 ` Liran Alon
@ 2020-03-30 20:46 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-03-30 20:46 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/30/20 19:24, Liran Alon wrote:
>
> On 30/03/2020 18:54, Laszlo Ersek wrote:
>> On 03/28/20 21:00, Liran Alon wrote:
>>> @@ -509,6 +720,14 @@ PvScsiUninit (
>>> IN OUT PVSCSI_DEV *Dev
>>> )
>>> {
>>> + //
>>> + // Reset device to stop device usage of the rings.
>>> + // This is required to safely free the rings.
>>> + //
>>> + PvScsiResetAdapter (Dev);
>> If I understand correctly (at the end of the series):
>>
>> in order to save one of the (ultimately) two reset calls in
>> PvScsiUninit(), namely
>> - one for releasing the DMA buf
>> - and the other (internal to PvScsiFreeRings()) for releasing the
>> rings, you unnested the latter reset call from PvScsiFreeRings(), and
>> added it manually to the error path of PvScsiInit().
> Right.
>> To me, that's more brittle and more difficult to reason about than
>> what I proposed, because now PvScsiFreeRings() does not completely
>> undo PvScsiInitRings().
> Hmm right. Because PvScsiInitRings() also setup the ring to the device
> with a command. Haven't thought about it.
>> I specifically requested that we please not try to save on the two
>> (seemingly) redundant reset calls in PvScsiUninit() -- see the
>> paragraph containing "bad idea" here:
>>
>> [...]
>>
>> (Note: we could be tempted to somehow "centralize" all of these
>> Reset operations into a single spot. Bad idea. We are revoking
>> the device's access rights to different resources, so the
>> revocation operations will show up in different spots. It's a
>> mere circumstance that the revocations all happen to be Reset
>> operations.)
>>
>> I might be paranoid of course -- I just feel that
>> maybe-superfluous reset operations on error paths are much
>> better than silently corrupted guest memory and/or disk
>> contents.
>>
>> You reacted to that message of mine, but not this paragraph
>> specifically (it was snipped from your followup) -- so I thought you
>> were OK with it. I'm a bit disappointed that you disagreed with my
>> request *silently*.
> Oh now I got what you meant! I misinterpreted it. Not silently ignored
> it of course!
> I can change this in a v4 patch-series if you would like that. Sorry
> for the misunderstanding.
>
> I do agree with your statement that PvScsiFreeRings() is not
> completely an undo of PvScsiInitRings().
> And maybe for making code a little bit easier to reason about, it's
> preferred to just do one additional unnecessary device reset.
>> To summarize: technically, your solution is correct; from a code
>> hygiene and resource ownership question, I'm convinced it is wrong.
> I agree.
>> But, I'll live with it.
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> I can send a v4 patch-series just changing that if you would like.
> Or submit a patch to change it after it will be merged.
> Or that you will change it when applying. Or leave it as is.
>
> I don't have any objection to any of the above.
OK. That's a relief to me, thank you! And I apologize for insinuating
that you deliberately & silently ignored said feedback from me. I tend
to be pedantic about email (it's not natural for me to think that
someone simply missed a comment) -- sorry about that!
Given that I've already merged v3 -- can you please post a followup
patch? Something like:
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 0a66c98421a9..3066b83b96fc 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -1093,6 +1093,12 @@ PvScsiFreeRings (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> + //
> + // Reset device to stop device usage of the rings.
> + // This is required to safely free the rings.
> + //
> + PvScsiResetAdapter (Dev);
> +
> PvScsiFreeSharedPages (
> Dev,
> 1,
> @@ -1199,12 +1205,6 @@ PvScsiInit (
> return EFI_SUCCESS;
>
> FreeRings:
> - //
> - // Reset device to stop device usage of the rings.
> - // This is required to safely free the rings.
> - //
> - PvScsiResetAdapter (Dev);
> -
> PvScsiFreeRings (Dev);
>
> RestorePciAttributes:
> @@ -1220,12 +1220,8 @@ PvScsiUninit (
> )
> {
> //
> - // Reset device to:
> - // - Make device stop processing all requests.
> - // - Stop device usage of the rings.
> - //
> - // This is required to safely free the DMA communication buffer
> - // and the rings.
> + // Reset device to make device stop processing all requests.
> + // This is required to safely free the DMA communication buffer.
> //
> PvScsiResetAdapter (Dev);
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-03-30 20:46 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-28 20:00 [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
2020-03-28 20:00 ` [PATCH v3 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
2020-03-28 20:00 ` [PATCH v3 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol Liran Alon
2020-03-28 20:00 ` [PATCH v3 03/17] OvmfPkg/PvScsiDxe: Report name of driver Liran Alon
2020-03-28 20:00 ` [PATCH v3 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi Liran Alon
2020-03-28 20:00 ` [PATCH v3 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Liran Alon
2020-03-28 20:00 ` [PATCH v3 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs Liran Alon
2020-03-28 20:00 ` [PATCH v3 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath Liran Alon
2020-03-28 20:00 ` [PATCH v3 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use Liran Alon
2020-03-28 20:00 ` [PATCH v3 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit Liran Alon
2020-03-28 20:00 ` [PATCH v3 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes Liran Alon
2020-03-28 20:00 ` [PATCH v3 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants Liran Alon
2020-03-28 20:00 ` [PATCH v3 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
2020-03-30 15:19 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
2020-03-30 15:54 ` [edk2-devel] " Laszlo Ersek
2020-03-30 17:24 ` Liran Alon
2020-03-30 20:46 ` Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
2020-03-30 16:06 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
2020-03-30 16:22 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:00 ` [PATCH v3 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices() Liran Alon
2020-03-30 16:23 ` [edk2-devel] " Laszlo Ersek
2020-03-28 20:01 ` [PATCH v3 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses Liran Alon
2020-03-29 9:29 ` [PATCH v3 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Nikita Leshenko
2020-03-30 16:53 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox