* [PATCH v2 01/17] OvmfPkg/PvScsiDxe: Create empty driver
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-26 14:44 ` [edk2-devel] " Laszlo Ersek
2020-03-25 16:09 ` [PATCH v2 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol Liran Alon
` (15 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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
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] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 01/17] OvmfPkg/PvScsiDxe: Create empty driver
2020-03-25 16:09 ` [PATCH v2 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
@ 2020-03-26 14:44 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 14:44 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:09, Liran Alon wrote:
> 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
> 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
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
2020-03-25 16:09 ` [PATCH v2 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-25 16:09 ` [PATCH v2 03/17] OvmfPkg/PvScsiDxe: Report name of driver Liran Alon
` (14 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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] 41+ messages in thread* [PATCH v2 03/17] OvmfPkg/PvScsiDxe: Report name of driver
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
2020-03-25 16:09 ` [PATCH v2 01/17] OvmfPkg/PvScsiDxe: Create empty driver Liran Alon
2020-03-25 16:09 ` [PATCH v2 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-25 16:09 ` [PATCH v2 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi Liran Alon
` (13 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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] 41+ messages in thread* [PATCH v2 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (2 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 03/17] OvmfPkg/PvScsiDxe: Report name of driver Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-25 16:09 ` [PATCH v2 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Liran Alon
` (12 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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] 41+ messages in thread* [PATCH v2 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (3 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-25 16:09 ` [PATCH v2 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs Liran Alon
` (11 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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] 41+ messages in thread* [PATCH v2 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (4 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-25 16:09 ` [PATCH v2 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath Liran Alon
` (10 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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] 41+ messages in thread* [PATCH v2 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (5 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-25 16:09 ` [PATCH v2 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use Liran Alon
` (9 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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] 41+ messages in thread* [PATCH v2 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (6 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-25 16:09 ` [PATCH v2 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit Liran Alon
` (8 subsequent siblings)
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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] 41+ messages in thread* [PATCH v2 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (7 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-26 17:04 ` [edk2-devel] " Laszlo Ersek
2020-03-25 16:09 ` [PATCH v2 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes Liran Alon
` (7 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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
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] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit
2020-03-25 16:09 ` [PATCH v2 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit Liran Alon
@ 2020-03-26 17:04 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 17:04 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:09, Liran Alon wrote:
> 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
> 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(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
>
> 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;
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (8 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-26 17:12 ` Laszlo Ersek
2020-03-25 16:09 ` [PATCH v2 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants Liran Alon
` (6 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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
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] 41+ messages in thread* Re: [PATCH v2 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes
2020-03-25 16:09 ` [PATCH v2 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes Liran Alon
@ 2020-03-26 17:12 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 17:12 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:09, Liran Alon wrote:
> 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
> 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;
> }
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (9 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes Liran Alon
@ 2020-03-25 16:09 ` Liran Alon
2020-03-26 17:19 ` [edk2-devel] " Laszlo Ersek
2020-03-25 16:10 ` [PATCH v2 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
` (5 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:09 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
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] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants
2020-03-25 16:09 ` [PATCH v2 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants Liran Alon
@ 2020-03-26 17:19 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 17:19 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:09, Liran Alon wrote:
> 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
> 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_
>
Acked-by: Laszlo Ersek <lersek@redhat.com>
(Not an R-b because I haven't validated the various structures and
constants against the device model. But stylistically, this patch can
certainly be merged, now.)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (10 preceding siblings ...)
2020-03-25 16:09 ` [PATCH v2 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants Liran Alon
@ 2020-03-25 16:10 ` Liran Alon
2020-03-26 18:25 ` [edk2-devel] " Laszlo Ersek
2020-03-25 16:10 ` [PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
` (4 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:10 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
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/PvScsiDxe/PvScsi.c | 91 ++++++++++++++++++++++++++++++++++++++
1 file changed, 91 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 531bed4e5ab7..831a78cc18c7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -30,6 +30,84 @@
// 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 PVSCSI command to device
+**/
+STATIC
+EFI_STATUS
+PvScsiWriteCmdDesc (
+ IN CONST PVSCSI_DEV *Dev,
+ IN UINT32 Cmd,
+ IN UINT32 *DescWords,
+ 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;
+}
+
/**
Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
@@ -357,6 +435,14 @@ PvScsiInit (
return Status;
}
+ //
+ // Reset adapter
+ //
+ Status = PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -387,6 +473,11 @@ PvScsiInit (
Dev->PassThruMode.IoAlign = 0;
return EFI_SUCCESS;
+
+RestorePciAttributes:
+ PvScsiRestorePciAttributes (Dev);
+
+ return Status;
}
STATIC
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init
2020-03-25 16:10 ` [PATCH v2 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
@ 2020-03-26 18:25 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 18:25 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:10, Liran Alon wrote:
> The following commits will complete the implementation of
> device initialization.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/PvScsiDxe/PvScsi.c | 91 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 91 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 531bed4e5ab7..831a78cc18c7 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -30,6 +30,84 @@
> // 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 PVSCSI command to device
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiWriteCmdDesc (
> + IN CONST PVSCSI_DEV *Dev,
> + IN UINT32 Cmd,
> + IN UINT32 *DescWords,
> + 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;
> +}
> +
> /**
> Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
> EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -357,6 +435,14 @@ PvScsiInit (
> return Status;
> }
>
> + //
> + // Reset adapter
> + //
> + Status = PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
> + if (EFI_ERROR (Status)) {
> + goto RestorePciAttributes;
> + }
> +
> //
> // Populate the exported interface's attributes
> //
> @@ -387,6 +473,11 @@ PvScsiInit (
> Dev->PassThruMode.IoAlign = 0;
>
> return EFI_SUCCESS;
> +
> +RestorePciAttributes:
> + PvScsiRestorePciAttributes (Dev);
> +
> + return Status;
> }
>
> STATIC
>
Nice.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (11 preceding siblings ...)
2020-03-25 16:10 ` [PATCH v2 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init Liran Alon
@ 2020-03-25 16:10 ` Liran Alon
2020-03-26 20:51 ` Laszlo Ersek
2020-03-25 16:10 ` [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
` (3 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:10 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 | 269 +++++++++++++++++++++++++++++++++++++
OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++
2 files changed, 286 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 831a78cc18c7..59863f83c60c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -16,6 +16,7 @@
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Protocol/PciIo.h>
+#include <Protocol/PciRootBridgeIo.h>
#include <Uefi/UefiSpec.h>
#include "PvScsi.h"
@@ -413,6 +414,264 @@ PvScsiRestorePciAttributes (
);
}
+STATIC
+EFI_STATUS
+PvScsiAllocatePages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ IN OUT VOID **HostAddress
+ )
+{
+ return Dev->PciIo->AllocateBuffer (
+ Dev->PciIo,
+ AllocateAnyPages,
+ EfiBootServicesData,
+ Pages,
+ HostAddress,
+ EFI_PCI_ATTRIBUTE_MEMORY_CACHED
+ );
+}
+
+STATIC
+VOID
+PvScsiFreePages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ IN VOID *HostAddress
+ )
+{
+ Dev->PciIo->FreeBuffer (
+ Dev->PciIo,
+ Pages,
+ HostAddress
+ );
+}
+
+STATIC
+EFI_STATUS
+PvScsiMapBuffer (
+ IN PVSCSI_DEV *Dev,
+ IN EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation,
+ IN VOID *HostAddress,
+ IN UINTN NumberOfBytes,
+ OUT PVSCSI_DMA_DESC *DmaDesc
+ )
+{
+ EFI_STATUS Status;
+ UINTN BytesMapped;
+
+ BytesMapped = NumberOfBytes;
+ Status = Dev->PciIo->Map (
+ Dev->PciIo,
+ PciIoOperation,
+ HostAddress,
+ &BytesMapped,
+ &DmaDesc->DeviceAddress,
+ &DmaDesc->Mapping
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (BytesMapped != NumberOfBytes) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Unmap;
+ }
+
+ return EFI_SUCCESS;
+
+Unmap:
+ Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+
+ return Status;
+}
+
+STATIC
+VOID
+PvScsiUnmapBuffer (
+ IN PVSCSI_DEV *Dev,
+ IN OUT PVSCSI_DMA_DESC *DmaDesc)
+{
+ Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+}
+
+STATIC
+EFI_STATUS
+PvScsiAllocateSharedPages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ IN EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation,
+ OUT VOID **HostAddress,
+ OUT PVSCSI_DMA_DESC *DmaDesc
+ )
+{
+ EFI_STATUS Status;
+
+ Status = PvScsiAllocatePages (Dev, Pages, HostAddress);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = PvScsiMapBuffer (
+ Dev,
+ PciIoOperation,
+ *HostAddress,
+ EFI_PAGES_TO_SIZE (Pages),
+ DmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreePages;
+ }
+
+ return EFI_SUCCESS;
+
+FreePages:
+ PvScsiFreePages (Dev, Pages, *HostAddress);
+
+ return Status;
+}
+
+STATIC
+VOID
+PvScsiFreeSharedPages (
+ IN PVSCSI_DEV *Dev,
+ IN UINTN Pages,
+ IN OUT VOID *HostAddress,
+ IN OUT PVSCSI_DMA_DESC *DmaDesc
+ )
+{
+ PvScsiUnmapBuffer (Dev, DmaDesc);
+ PvScsiFreePages (Dev, Pages, HostAddress);
+}
+
+STATIC
+EFI_STATUS
+PvScsiInitRings (
+ IN OUT PVSCSI_DEV *Dev
+ )
+{
+ EFI_STATUS Status;
+ PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ EfiPciIoOperationBusMasterCommonBuffer,
+ (VOID **)&Dev->RingDesc.RingState,
+ &Dev->RingDesc.RingStateDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ EfiPciIoOperationBusMasterCommonBuffer,
+ (VOID **)&Dev->RingDesc.RingReqs,
+ &Dev->RingDesc.RingReqsDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRingState;
+ }
+ ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
+
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ 1,
+ EfiPciIoOperationBusMasterCommonBuffer,
+ (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 (
+ (UINT64)Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+ Cmd.ReqRingPPNs[0] = RShiftU64 (
+ (UINT64)Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+ Cmd.CmpRingPPNs[0] = RShiftU64 (
+ (UINT64)Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
+ EFI_PAGE_SHIFT
+ );
+
+ 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 (
@@ -443,6 +702,14 @@ PvScsiInit (
goto RestorePciAttributes;
}
+ //
+ // Init PVSCSI rings
+ //
+ Status = PvScsiInitRings (Dev);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -486,6 +753,8 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *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;
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings
2020-03-25 16:10 ` [PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
@ 2020-03-26 20:51 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 20:51 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:10, 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 | 269 +++++++++++++++++++++++++++++++++++++
> OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++
> 2 files changed, 286 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 831a78cc18c7..59863f83c60c 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -16,6 +16,7 @@
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
> #include <Uefi/UefiSpec.h>
>
> #include "PvScsi.h"
> @@ -413,6 +414,264 @@ PvScsiRestorePciAttributes (
> );
> }
>
> +STATIC
> +EFI_STATUS
> +PvScsiAllocatePages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + IN OUT VOID **HostAddress
> + )
> +{
> + return Dev->PciIo->AllocateBuffer (
> + Dev->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + Pages,
> + HostAddress,
> + EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> + );
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreePages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + IN VOID *HostAddress
> + )
> +{
> + Dev->PciIo->FreeBuffer (
> + Dev->PciIo,
> + Pages,
> + HostAddress
> + );
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiMapBuffer (
> + IN PVSCSI_DEV *Dev,
> + IN EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT PVSCSI_DMA_DESC *DmaDesc
> + )
> +{
> + EFI_STATUS Status;
> + UINTN BytesMapped;
> +
> + BytesMapped = NumberOfBytes;
> + Status = Dev->PciIo->Map (
> + Dev->PciIo,
> + PciIoOperation,
> + HostAddress,
> + &BytesMapped,
> + &DmaDesc->DeviceAddress,
> + &DmaDesc->Mapping
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (BytesMapped != NumberOfBytes) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Unmap;
> + }
> +
> + return EFI_SUCCESS;
> +
> +Unmap:
> + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +
> + return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiUnmapBuffer (
> + IN PVSCSI_DEV *Dev,
> + IN OUT PVSCSI_DMA_DESC *DmaDesc)
> +{
> + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiAllocateSharedPages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + IN EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation,
> + OUT VOID **HostAddress,
> + OUT PVSCSI_DMA_DESC *DmaDesc
> + )
> +{
> + EFI_STATUS Status;
> +
> + Status = PvScsiAllocatePages (Dev, Pages, HostAddress);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + Status = PvScsiMapBuffer (
> + Dev,
> + PciIoOperation,
> + *HostAddress,
> + EFI_PAGES_TO_SIZE (Pages),
> + DmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreePages;
> + }
> +
> + return EFI_SUCCESS;
> +
> +FreePages:
> + PvScsiFreePages (Dev, Pages, *HostAddress);
> +
> + return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeSharedPages (
> + IN PVSCSI_DEV *Dev,
> + IN UINTN Pages,
> + IN OUT VOID *HostAddress,
> + IN OUT PVSCSI_DMA_DESC *DmaDesc
> + )
> +{
> + PvScsiUnmapBuffer (Dev, DmaDesc);
> + PvScsiFreePages (Dev, Pages, HostAddress);
> +}
(1) From all of the above functions, please keep only two:
PvScsiAllocateSharedPages() and PvScsiFreeSharedPages(). The rest of the
helper functions should be flattened into them.
This is not a "frivolous" request -- it will make more sense once you
read my next request.
(2) Please eliminate the "PciIoOperation" parameter from the
PvScsiAllocateSharedPages() prototype, and open-code
"EfiPciIoOperationBusMasterCommonBuffer" in its place, in the
(flattened) implementation of the function.
Rationale: the buffer should be allocated separately with the
PciIo->AllocateBuffer() API *if and only if* a common buffer operation
is being set up. In other cases (one-shot bus master read, one-shot bus
master write), *only* the Map() operation should be used, and then Map()
and Unmap() will handle the bounce-buffering internally.
The functions above are misleading because they suggest they handle all
three bus master operations -- but for the one-shot BM read and write
operations, the PvScsiAllocateSharedPages() / PvScsiFreeSharedPages()
implementations are not correct.
Now, given that the call sites only set up common buffer operations, the
ultimately exercised logic is OK. And therefore the solution is not to
generalize the implementation uselessly (to BM read and BM write
operations too), but to tighten the interfaces such that they precisely
reflect the actual intended use case.
Hence, please open-code the common buffer operation internally to
PvScsiAllocateSharedPages(), and also stop exposing the separate
allocation / freeing / mapping / unmapping interfaces. The call sites
need nothing else than (a) "give me a completely mapped range for common
buffer operation" and (b) "release that".
Here's the (untested, not even compiled) implementation I propose:
> 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);
> }
Back to your patch:
On 03/25/20 17:10, Liran Alon wrote:
> +
> +STATIC
> +EFI_STATUS
> +PvScsiInitRings (
> + IN OUT PVSCSI_DEV *Dev
> + )
> +{
> + EFI_STATUS Status;
> + PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + (VOID **)&Dev->RingDesc.RingState,
> + &Dev->RingDesc.RingStateDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + (VOID **)&Dev->RingDesc.RingReqs,
> + &Dev->RingDesc.RingReqsDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeRingState;
> + }
> + ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
> +
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + 1,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + (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 (
> + (UINT64)Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> + Cmd.ReqRingPPNs[0] = RShiftU64 (
> + (UINT64)Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
> + Cmd.CmpRingPPNs[0] = RShiftU64 (
> + (UINT64)Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
> + EFI_PAGE_SHIFT
> + );
(3) Please drop the explicit UINT64 casts -- for brevity --,
EFI_PHYSICAL_ADDRESS is just a different name (a typedef) for UINT64, by
spec. (See in the EFI_BOOT_SERVICES.AllocatePages() chapter in the UEFI
spec.)
(4) Please #include <Library/BaseLib.h> in the C file, and also list
BaseLib under [LibraryClasses] in the INF file. RShiftU64() comes from
there.
> +
> + Status = PvScsiWriteCmdDesc (
> + Dev,
> + PvScsiCmdSetupRings,
> + (UINT32 *)&Cmd,
(5) So I guess this is where the "union trick" will come, in v3.
> + sizeof (Cmd) / sizeof (UINT32)
(6) Can you please try inserting, before the PvScsiWriteCmdDesc() call:
STATIC_ASSERT (sizeof Cmd % sizeof (UINT32) == 0, "invalid Cmd size");
> + );
> + 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
> + );
> +}
> +
(7) This function is almost correct.
Note that your last *such* action in PvScsiInitRings() that impacts any
resource life cycle is the *exposure* of the rings to the device model
(the hypervisor).
Therefore, after PvScsiInitRings() succeeds, then on any subsequently
occurring error path, you first need to make the device model forget
about the rings, before you release the rings. A hypervisor-side
reference to guest memory is just another resource you "allocate", so
you need to drop it too, in reverse order of construction.
In brief, please add the following to the top of the PvScsiFreeRings()
function:
PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
(Consider, for example, what would happen *otherwise*, if the DmaBuf
allocation failed -- in the next patch -- in the PvScsiInit() function.
There, you jump to "FreeRings" correctly, and release the rings with
PvScsiFreeRings() -- but the hypervisor still remembers them!)
> STATIC
> EFI_STATUS
> PvScsiInit (
> @@ -443,6 +702,14 @@ PvScsiInit (
> goto RestorePciAttributes;
> }
>
> + //
> + // Init PVSCSI rings
> + //
> + Status = PvScsiInitRings (Dev);
> + if (EFI_ERROR (Status)) {
> + goto RestorePciAttributes;
> + }
> +
> //
> // Populate the exported interface's attributes
> //
> @@ -486,6 +753,8 @@ PvScsiUninit (
> IN OUT PVSCSI_DEV *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;
>
These look fine.
(8) A general request regarding your git setup: please run the
"BaseTools/Scripts/SetupGit.py" script in your edk2 clone.
The reason I'm asking for that right now is that the script will point
the "diff.orderFile" setting to "BaseTools/Conf/diff.order". And that
will make git-format-patch, on your end, place the header file changes
ahead of the C file changes, in the generated emails.
It helps reviewers greatly if they can first see the declarative changes
(such as new types, new structure members, and so on), before they face
the code using the new constructs.
The script in question configures a bunch of other cool stuff too that
boosts reviewer productivity.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (12 preceding siblings ...)
2020-03-25 16:10 ` [PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings Liran Alon
@ 2020-03-25 16:10 ` Liran Alon
2020-03-26 22:17 ` Laszlo Ersek
2020-03-25 16:10 ` [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
` (2 subsequent siblings)
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:10 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 | 27 +++++++++++++++++++++++++++
OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++
2 files changed, 37 insertions(+)
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 59863f83c60c..928984099520 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -710,6 +710,20 @@ PvScsiInit (
goto RestorePciAttributes;
}
+ //
+ // Allocate DMA communication buffer
+ //
+ Status = PvScsiAllocateSharedPages (
+ Dev,
+ EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
+ EfiPciIoOperationBusMasterCommonBuffer,
+ (VOID **)&Dev->DmaBuf,
+ &Dev->DmaBufDmaDesc
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeRings;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -741,6 +755,9 @@ PvScsiInit (
return EFI_SUCCESS;
+FreeRings:
+ PvScsiFreeRings (Dev);
+
RestorePciAttributes:
PvScsiRestorePciAttributes (Dev);
@@ -753,6 +770,16 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Free DMA communication buffer
+ //
+ PvScsiFreeSharedPages (
+ Dev,
+ EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
+ (VOID **)&Dev->DmaBuf,
+ &Dev->DmaBufDmaDesc
+ );
+
PvScsiFreeRings (Dev);
PvScsiRestorePciAttributes (Dev);
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 6d23b6e1eccf..7f91d70fec79 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -31,6 +31,11 @@ typedef struct {
PVSCSI_DMA_DESC RingCmpsDmaDesc;
} PVSCSI_RING_DESC;
+typedef struct {
+ UINT8 SenseData[MAX_UINT8];
+ UINT8 Data[0x2000];
+} PVSCSI_DMA_BUFFER;
+
#define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
typedef struct {
@@ -38,6 +43,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 +54,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] 41+ messages in thread* Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-25 16:10 ` [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
@ 2020-03-26 22:17 ` Laszlo Ersek
2020-03-27 0:05 ` Liran Alon
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 22:17 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:10, 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
(1) s/And/and/
> 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 | 27 +++++++++++++++++++++++++++
> OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++
> 2 files changed, 37 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 59863f83c60c..928984099520 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -710,6 +710,20 @@ PvScsiInit (
> goto RestorePciAttributes;
> }
>
> + //
> + // Allocate DMA communication buffer
> + //
> + Status = PvScsiAllocateSharedPages (
> + Dev,
> + EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> + EfiPciIoOperationBusMasterCommonBuffer,
> + (VOID **)&Dev->DmaBuf,
> + &Dev->DmaBufDmaDesc
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeRings;
> + }
> +
> //
> // Populate the exported interface's attributes
> //
> @@ -741,6 +755,9 @@ PvScsiInit (
>
> return EFI_SUCCESS;
>
> +FreeRings:
> + PvScsiFreeRings (Dev);
> +
> RestorePciAttributes:
> PvScsiRestorePciAttributes (Dev);
>
> @@ -753,6 +770,16 @@ PvScsiUninit (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> + //
> + // Free DMA communication buffer
> + //
> + PvScsiFreeSharedPages (
(2) From peeking ahead at the next patch, the following seems possible:
- we send a data transfer request to the device model, passing some
pointers into the DMA communication buffer to the hypervisor
- PvScsiWaitForRequestCompletion() fails (for whatever reason), and so
we can't be sure whether the device is completely done with the buffer
that we exposed to it.
Therefore, please *prepend* a Reset operation to this
PvScsiFreeSharedPages() call as well, at the top of PvScsiUninit().
(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.
> + Dev,
> + EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> + (VOID **)&Dev->DmaBuf,
(3) Copy-paste typo: you should only pass "Dev->DmaBuf".
The compiler doesn't catch this for you because PvScsiFreeSharedPages()
takes a "VOID *HostAddress" here, and (VOID **) -- like all other
pointer-to-object types -- converts to (VOID *) silently.
> + &Dev->DmaBufDmaDesc
> + );
> +
> PvScsiFreeRings (Dev);
>
> PvScsiRestorePciAttributes (Dev);
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 6d23b6e1eccf..7f91d70fec79 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -31,6 +31,11 @@ typedef struct {
> PVSCSI_DMA_DESC RingCmpsDmaDesc;
> } PVSCSI_RING_DESC;
>
> +typedef struct {
> + UINT8 SenseData[MAX_UINT8];
(4) Is the maximum possible size of the sense data specified somewhere?
If so, it would be nice to document it with a comment at least.
> + UINT8 Data[0x2000];
(5) Same here. From peeking at the next patch, we seem to be choosing
this size arbitrarily.
If it works for you in all relevant boot scenarios, I'm OK with it, but
we should be clear that this value is arbitrarily chosen. No need for a
#define, but a comment would be nice.
(6) Should we declare this structure as packed?
> +} PVSCSI_DMA_BUFFER;
> +
> #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>
> typedef struct {
> @@ -38,6 +43,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 +54,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_
>
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-26 22:17 ` Laszlo Ersek
@ 2020-03-27 0:05 ` Liran Alon
2020-03-27 13:35 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-27 0:05 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 27/03/2020 0:17, Laszlo Ersek wrote:
> On 03/25/20 17:10, Liran Alon wrote:
>> PvScsiRestorePciAttributes (Dev);
>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>> index 6d23b6e1eccf..7f91d70fec79 100644
>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>> @@ -31,6 +31,11 @@ typedef struct {
>> PVSCSI_DMA_DESC RingCmpsDmaDesc;
>> } PVSCSI_RING_DESC;
>>
>> +typedef struct {
>> + UINT8 SenseData[MAX_UINT8];
> (4) Is the maximum possible size of the sense data specified somewhere?
> If so, it would be nice to document it with a comment at least.
This is a good point. Thanks for pointing it out.
Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1 Descriptor
format sense data overview specifies:
"The additional sense length shall be less than or equal to 244 (i.e.,
limiting the total length of the sense data to 252 bytes)"
i.e. The max possible size of sense data is 252.
As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed defining:
#define SCSI_SENSE_BUF_SIZE 252
This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
sense buf size to 252") which says in it's commit message:
"According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
Interestingly, this QEMU commit changed the value of SCSI_SENSE_BUF_SIZE
from 96 to 252.
But then I found this in EDK2 OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
//
// We expect these maximum sizes from the host. Also we force the
CdbLength and
// SenseDataLength parameters of
EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
// to exceed these limits. See UEFI 2.3.1 errata C 14.7.
//
#define VIRTIO_SCSI_CDB_SIZE 32
#define VIRTIO_SCSI_SENSE_SIZE 96
Which seems to be wrong...
It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to EDK2
MdePkg/Include/IndustryStandard/Scsi.h.
And then use that from my PvScsi driver.
I can also submit a separate patch (Not part of this series) to remove
this VIRTIO_SCSI_SENSE_SIZE constant.
Note that VirtioScsi doesn't have a technical issue here because it
provides this max sense length to the device in VirtioScsiInit():
Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
So if it's OK by you, I think I will just add a patch to this series
defining SCSI_MAX_SENSE_SIZE in MdePkg/Include/IndustryStandard/Scsi.h,
and then rely on that when defining SenseData in PVSCSI_DMA_BUFFER.
>> + UINT8 Data[0x2000];
> (5) Same here. From peeking at the next patch, we seem to be choosing
> this size arbitrarily.
This is indeed arbitrary...
>
> If it works for you in all relevant boot scenarios, I'm OK with it, but
> we should be clear that this value is arbitrarily chosen. No need for a
> #define, but a comment would be nice.
OK. Will just add a comment such as:
//
// This size is arbitrarily chosen,
// It seems to be sufficient for all I/O requests sent through
EFI_SCSI_PASS_THRU_PROTOCOL.PassThru().
//
If you wish to have something else, please say so. :)
>
> (6) Should we declare this structure as packed?
There is no such requirement as it's not a wire-format or anything like
that.
The only rational of defining it as packed is to avoid mapping
unnecessary pages to device.
But this structure is less than a page-size anyway. So I think it's fine.
If you still think it should be marked as packed, I can change this.
Thanks!
-Liran
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-27 0:05 ` Liran Alon
@ 2020-03-27 13:35 ` Laszlo Ersek
2020-03-27 21:31 ` Liran Alon
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-27 13:35 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/27/20 01:05, Liran Alon wrote:
>
> On 27/03/2020 0:17, Laszlo Ersek wrote:
>> On 03/25/20 17:10, Liran Alon wrote:
>>> PvScsiRestorePciAttributes (Dev);
>>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>>> index 6d23b6e1eccf..7f91d70fec79 100644
>>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>>> @@ -31,6 +31,11 @@ typedef struct {
>>> PVSCSI_DMA_DESC RingCmpsDmaDesc;
>>> } PVSCSI_RING_DESC;
>>> +typedef struct {
>>> + UINT8 SenseData[MAX_UINT8];
>> (4) Is the maximum possible size of the sense data specified
>> somewhere? If so, it would be nice to document it with a comment at
>> least.
> This is a good point. Thanks for pointing it out.
> Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1
> Descriptor format sense data overview specifies:
> "The additional sense length shall be less than or equal to 244 (i.e.,
> limiting the total length of the sense data to 252 bytes)"
>
> i.e. The max possible size of sense data is 252.
> As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed
> defining:
>
> #define SCSI_SENSE_BUF_SIZE 252
>
> This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
> sense buf size to 252") which says in it's commit message:
> "According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
>
> Interestingly, this QEMU commit changed the value of
> SCSI_SENSE_BUF_SIZE from 96 to 252.
> But then I found this in EDK2
> OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
>
> //
> // We expect these maximum sizes from the host. Also we force the
> CdbLength and
> // SenseDataLength parameters of
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
> // to exceed these limits. See UEFI 2.3.1 errata C 14.7.
> //
> #define VIRTIO_SCSI_CDB_SIZE 32
> #define VIRTIO_SCSI_SENSE_SIZE 96
>
> Which seems to be wrong...
No, these macros / limits are valid. They match the virtio specification
(both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size"
configuration values that the virtio-scsi device is required to expose
by default.
While the driver could theoretically ask for larger sizes, these values
have never caused a problem in practice. I don't see a reason to change
the constants (let alone to complicate the code).
>
> It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to
> EDK2 MdePkg/Include/IndustryStandard/Scsi.h.
> And then use that from my PvScsi driver.
>
> I can also submit a separate patch (Not part of this series) to remove
> this VIRTIO_SCSI_SENSE_SIZE constant.
Please don't; there's no reason to change the VirtioScsiDxe driver.
> Note that VirtioScsi doesn't have a technical issue here because it
> provides this max sense length to the device in VirtioScsiInit():
>
> Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
Right, and even those config writes are mostly documentation / "just to
be sure" oriented. Because, there's also a comment in those parts:
//
// We expect these maximum sizes from the host. Since they are
// guest-negotiable, ask for them rather than just checking them.
//
>
> So if it's OK by you, I think I will just add a patch to this series
> defining SCSI_MAX_SENSE_SIZE in
> MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when
> defining SenseData in PVSCSI_DMA_BUFFER.
My experience tells me that making any OvmfPkg change dependent on new
patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...)
slows down the upstreaming of the OvmfPkg change significantly.
Therefore, I would strongly advise against this ordering of changes.
Today, upon reviewing patch#15, I commented again on the "SenseData"
array size (see under point (2) in my patch#15 feedback). Accordingly,
for now, please stick with the existent "SenseData" array declaration,
just add a comment. I expect / hope to merge your v3 posting.
(Side comment: I would also like to ask Nikita to R-b the full final
patch set on the list, once I'm ready to merge the series, because I'd
like to testify to Nikita's review effort in the upstream git history.)
Then, with the series merged, you can propose a separate patch series (2
patches), later -- introducing the new constant in MdePkg, plus putting
the new constant to use in the (then-upstream) PvScsi feature. No matter
how long that is delayed, the main feature will have been merged.
>
>>> + UINT8 Data[0x2000];
>> (5) Same here. From peeking at the next patch, we seem to be choosing
>> this size arbitrarily.
> This is indeed arbitrary...
>>
>> If it works for you in all relevant boot scenarios, I'm OK with it,
>> but we should be clear that this value is arbitrarily chosen. No need
>> for a #define, but a comment would be nice.
>
> OK. Will just add a comment such as:
>
> //
> // This size is arbitrarily chosen,
> // It seems to be sufficient for all I/O requests sent through
> EFI_SCSI_PASS_THRU_PROTOCOL.PassThru().
> //
>
> If you wish to have something else, please say so. :)
This comment looks nice, I'd only append: "for common boot scenarios".
>
>>
>> (6) Should we declare this structure as packed?
> There is no such requirement as it's not a wire-format or anything
> like that.
My thinking was that we shouldn't have any "padding" in a structure
that's exposed to a device.
But, admittedly, due to rounding up the allocation to page size anyway,
we're virtually guaranteed to have unused space at the end. Possibly
having some padding in the middle makes no difference, so you are right:
no need for packing this.
> The only rational of defining it as packed is to avoid mapping
> unnecessary pages to device.
> But this structure is less than a page-size anyway. So I think it's
> fine.
I agree with your general point, but I'd like to comment on the size
independently (just for the record): the PVSCSI_DMA_BUFFER structure is
not smaller than a page. It is slightly larger than two pages -- it
contains at least (255 + 2 * 4096) bytes. Again, that doesn't change the
main point.
>
> If you still think it should be marked as packed, I can change this.
No, I agree it need not be packed.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-27 13:35 ` Laszlo Ersek
@ 2020-03-27 21:31 ` Liran Alon
2020-03-30 11:29 ` Laszlo Ersek
0 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-27 21:31 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 27/03/2020 16:35, Laszlo Ersek wrote:
> On 03/27/20 01:05, Liran Alon wrote:
>> On 27/03/2020 0:17, Laszlo Ersek wrote:
>>> On 03/25/20 17:10, Liran Alon wrote:
>>>> PvScsiRestorePciAttributes (Dev);
>>>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>> index 6d23b6e1eccf..7f91d70fec79 100644
>>>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>>>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>> @@ -31,6 +31,11 @@ typedef struct {
>>>> PVSCSI_DMA_DESC RingCmpsDmaDesc;
>>>> } PVSCSI_RING_DESC;
>>>> +typedef struct {
>>>> + UINT8 SenseData[MAX_UINT8];
>>> (4) Is the maximum possible size of the sense data specified
>>> somewhere? If so, it would be nice to document it with a comment at
>>> least.
>> This is a good point. Thanks for pointing it out.
>> Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1
>> Descriptor format sense data overview specifies:
>> "The additional sense length shall be less than or equal to 244 (i.e.,
>> limiting the total length of the sense data to 252 bytes)"
>>
>> i.e. The max possible size of sense data is 252.
>> As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed
>> defining:
>>
>> #define SCSI_SENSE_BUF_SIZE 252
>>
>> This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
>> sense buf size to 252") which says in it's commit message:
>> "According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
>>
>> Interestingly, this QEMU commit changed the value of
>> SCSI_SENSE_BUF_SIZE from 96 to 252.
>> But then I found this in EDK2
>> OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
>>
>> //
>> // We expect these maximum sizes from the host. Also we force the
>> CdbLength and
>> // SenseDataLength parameters of
>> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
>> // to exceed these limits. See UEFI 2.3.1 errata C 14.7.
>> //
>> #define VIRTIO_SCSI_CDB_SIZE 32
>> #define VIRTIO_SCSI_SENSE_SIZE 96
>>
>> Which seems to be wrong...
> No, these macros / limits are valid. They match the virtio specification
> (both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size"
> configuration values that the virtio-scsi device is required to expose
> by default.
>
> While the driver could theoretically ask for larger sizes, these values
> have never caused a problem in practice. I don't see a reason to change
> the constants (let alone to complicate the code).
>
>> It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to
>> EDK2 MdePkg/Include/IndustryStandard/Scsi.h.
>> And then use that from my PvScsi driver.
>>
>> I can also submit a separate patch (Not part of this series) to remove
>> this VIRTIO_SCSI_SENSE_SIZE constant.
> Please don't; there's no reason to change the VirtioScsiDxe driver.
>
>> Note that VirtioScsi doesn't have a technical issue here because it
>> provides this max sense length to the device in VirtioScsiInit():
>>
>> Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
> Right, and even those config writes are mostly documentation / "just to
> be sure" oriented. Because, there's also a comment in those parts:
>
> //
> // We expect these maximum sizes from the host. Since they are
> // guest-negotiable, ask for them rather than just checking them.
> //
>
>> So if it's OK by you, I think I will just add a patch to this series
>> defining SCSI_MAX_SENSE_SIZE in
>> MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when
>> defining SenseData in PVSCSI_DMA_BUFFER.
> My experience tells me that making any OvmfPkg change dependent on new
> patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...)
> slows down the upstreaming of the OvmfPkg change significantly.
>
> Therefore, I would strongly advise against this ordering of changes.
>
> Today, upon reviewing patch#15, I commented again on the "SenseData"
> array size (see under point (2) in my patch#15 feedback). Accordingly,
> for now, please stick with the existent "SenseData" array declaration,
> just add a comment. I expect / hope to merge your v3 posting.
As an alternative, I can also define SenseData to be of size 252 (which
I would #define as a constant in PvScsi.h),
and explain that it is the limit of total length of SenseData according
to SCSI specification.
And indeed leave for a separate patch-series to move this PvScsi.h
private constant and put it in the generic
IndustryStandard/Scsi.h header file.
I think it's nicer than just defining SenseData to be MAX_UINT8 because
of the field size in PassThru packet.
But I will of course do as you prefer. Just let me know your choice :)
>
> (Side comment: I would also like to ask Nikita to R-b the full final
> patch set on the list, once I'm ready to merge the series, because I'd
> like to testify to Nikita's review effort in the upstream git history.)
Ok. I will let him know to review my v3 submission upstream.
-Liran
>
> Then, with the series merged, you can propose a separate patch series (2
> patches), later -- introducing the new constant in MdePkg, plus putting
> the new constant to use in the (then-upstream) PvScsi feature. No matter
> how long that is delayed, the main feature will have been merged.
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer
2020-03-27 21:31 ` Liran Alon
@ 2020-03-30 11:29 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-30 11:29 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/27/20 22:31, Liran Alon wrote:
>
> On 27/03/2020 16:35, Laszlo Ersek wrote:
>> On 03/27/20 01:05, Liran Alon wrote:
>>> On 27/03/2020 0:17, Laszlo Ersek wrote:
>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>> PvScsiRestorePciAttributes (Dev);
>>>>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>>> index 6d23b6e1eccf..7f91d70fec79 100644
>>>>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>>>>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>>> @@ -31,6 +31,11 @@ typedef struct {
>>>>> PVSCSI_DMA_DESC RingCmpsDmaDesc;
>>>>> } PVSCSI_RING_DESC;
>>>>> +typedef struct {
>>>>> + UINT8 SenseData[MAX_UINT8];
>>>> (4) Is the maximum possible size of the sense data specified
>>>> somewhere? If so, it would be nice to document it with a comment at
>>>> least.
>>> This is a good point. Thanks for pointing it out.
>>> Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1
>>> Descriptor format sense data overview specifies:
>>> "The additional sense length shall be less than or equal to 244 (i.e.,
>>> limiting the total length of the sense data to 252 bytes)"
>>>
>>> i.e. The max possible size of sense data is 252.
>>> As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed
>>> defining:
>>>
>>> #define SCSI_SENSE_BUF_SIZE 252
>>>
>>> This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
>>> sense buf size to 252") which says in it's commit message:
>>> "According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
>>>
>>> Interestingly, this QEMU commit changed the value of
>>> SCSI_SENSE_BUF_SIZE from 96 to 252.
>>> But then I found this in EDK2
>>> OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
>>>
>>> //
>>> // We expect these maximum sizes from the host. Also we force the
>>> CdbLength and
>>> // SenseDataLength parameters of
>>> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
>>> // to exceed these limits. See UEFI 2.3.1 errata C 14.7.
>>> //
>>> #define VIRTIO_SCSI_CDB_SIZE 32
>>> #define VIRTIO_SCSI_SENSE_SIZE 96
>>>
>>> Which seems to be wrong...
>> No, these macros / limits are valid. They match the virtio specification
>> (both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size"
>> configuration values that the virtio-scsi device is required to expose
>> by default.
>>
>> While the driver could theoretically ask for larger sizes, these values
>> have never caused a problem in practice. I don't see a reason to change
>> the constants (let alone to complicate the code).
>>
>>> It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to
>>> EDK2 MdePkg/Include/IndustryStandard/Scsi.h.
>>> And then use that from my PvScsi driver.
>>>
>>> I can also submit a separate patch (Not part of this series) to remove
>>> this VIRTIO_SCSI_SENSE_SIZE constant.
>> Please don't; there's no reason to change the VirtioScsiDxe driver.
>>
>>> Note that VirtioScsi doesn't have a technical issue here because it
>>> provides this max sense length to the device in VirtioScsiInit():
>>>
>>> Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
>> Right, and even those config writes are mostly documentation / "just to
>> be sure" oriented. Because, there's also a comment in those parts:
>>
>> //
>> // We expect these maximum sizes from the host. Since they are
>> // guest-negotiable, ask for them rather than just checking them.
>> //
>>
>>> So if it's OK by you, I think I will just add a patch to this series
>>> defining SCSI_MAX_SENSE_SIZE in
>>> MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when
>>> defining SenseData in PVSCSI_DMA_BUFFER.
>> My experience tells me that making any OvmfPkg change dependent on new
>> patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...)
>> slows down the upstreaming of the OvmfPkg change significantly.
>>
>> Therefore, I would strongly advise against this ordering of changes.
>>
>> Today, upon reviewing patch#15, I commented again on the "SenseData"
>> array size (see under point (2) in my patch#15 feedback). Accordingly,
>> for now, please stick with the existent "SenseData" array declaration,
>> just add a comment. I expect / hope to merge your v3 posting.
> As an alternative, I can also define SenseData to be of size 252 (which
> I would #define as a constant in PvScsi.h),
> and explain that it is the limit of total length of SenseData according
> to SCSI specification.
>
> And indeed leave for a separate patch-series to move this PvScsi.h
> private constant and put it in the generic
> IndustryStandard/Scsi.h header file.
>
> I think it's nicer than just defining SenseData to be MAX_UINT8 because
> of the field size in PassThru packet.
> But I will of course do as you prefer. Just let me know your choice :)
A new PvScsi.h-internal macro, with replacement text "252", sounds nice.
But in that case, you'll have to check (or trim) the
"EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.SenseDataLength" field value
on input.
I'm fine either way -- my main point is that we shouldn't report more
sense room to the device than what we have available in the DMA buffer.
If the caller passes in a SenseDataLength that's larger than 252 (such
as 254, for example), and our DMA buffer field is 252 bytes large, then
we should give 252 to the device, and report back to the caller
accordingly (= no more than 252 bytes).
Thanks,
Laszlo
>
>>
>> (Side comment: I would also like to ask Nikita to R-b the full final
>> patch set on the list, once I'm ready to merge the series, because I'd
>> like to testify to Nikita's review effort in the upstream git history.)
>
> Ok. I will let him know to review my v3 submission upstream.
>
> -Liran
>
>>
>> Then, with the series merged, you can propose a separate patch series (2
>> patches), later -- introducing the new constant in MdePkg, plus putting
>> the new constant to use in the (then-upstream) PvScsi feature. No matter
>> how long that is delayed, the main feature will have been merged.
>>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (13 preceding siblings ...)
2020-03-25 16:10 ` [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer Liran Alon
@ 2020-03-25 16:10 ` Liran Alon
2020-03-27 11:26 ` [edk2-devel] " Laszlo Ersek
2020-03-25 16:10 ` [PATCH v2 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices() Liran Alon
2020-03-25 16:10 ` [PATCH v2 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses Liran Alon
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:10 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 | 442 +++++++++++++++++++++++++++++++-
OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 +-
4 files changed, 451 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 928984099520..de4122e39a81 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -31,6 +31,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
**/
@@ -109,6 +130,352 @@ PvScsiWriteCmdDesc (
return EFI_SUCCESS;
}
+/**
+ 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
+ );
+}
+
+/**
+ 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 EFI_BAD_BUFFER_SIZE;
+ }
+ if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) {
+ Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data);
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ //
+ // Encode PVSCSI request
+ //
+ ZeroMem (Request, sizeof (*Request));
+
+ Request->Bus = 0;
+ Request->Target = TargetValue;
+ //
+ // This cast is safe as MaxLun is defined as UINT8
+ //
+ Request->Lun[1] = (UINT8)Lun;
+ Request->SenseLen = Packet->SenseDataLength;
+ Request->SenseAddr = (UINT64)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 = (UINT64)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
+ )
+{
+ //
+ // Check if device returned sense data
+ //
+ if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
+ //
+ // 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
+ );
+ } else {
+ //
+ // Signal no sense data returned
+ //
+ Packet->SenseDataLength = 0;
+ }
+
+ //
+ // 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 PvScsiBtStatSelTimeout:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
+ return EFI_TIMEOUT;
+
+ case PvScsiBtStatDatarun:
+ case PvScsiBtStatDataUnderrun:
+ //
+ // Report residual data in overrun/underrun
+ //
+ if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+ Packet->InTransferLength = Response->DataLen;
+ } else {
+ Packet->OutTransferLength = Response->DataLen;
+ }
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+ return EFI_BAD_BUFFER_SIZE;
+
+ 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
@@ -129,6 +496,23 @@ IsTargetInitialized (
return FALSE;
}
+/**
+ 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;
+}
+
//
// Ext SCSI Pass Thru
//
@@ -144,7 +528,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
@@ -685,6 +1124,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 7f91d70fec79..08e876b75930 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -47,6 +47,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 fcffc90d46c8..ae6d8753623f 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -38,5 +38,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] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-25 16:10 ` [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
@ 2020-03-27 11:26 ` Laszlo Ersek
2020-03-27 13:04 ` Liran Alon
0 siblings, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-27 11:26 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:10, 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 | 442 +++++++++++++++++++++++++++++++-
> OvmfPkg/PvScsiDxe/PvScsi.h | 1 +
> OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 +-
> 4 files changed, 451 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 928984099520..de4122e39a81 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -31,6 +31,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
> **/
> @@ -109,6 +130,352 @@ PvScsiWriteCmdDesc (
> return EFI_SUCCESS;
> }
>
> +/**
> + 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;
> +}
(Just some thoughts, not a request for changing the code.)
Normally I prefer accessing buffers shared with the device though
volatile-qualified pointers.
Meaning, in this case, that every "PCI host" pointer (i.e., each pointer
that is associated with a PVSCSI_DMA_DESC) would have to be
volatile-qualified. In particular:
- in patch#13, PVSCSI_RING_DESC would have to be updated like this:
> typedef struct {
> volatile PVSCSI_RINGS_STATE *RingState;
> PVSCSI_DMA_DESC RingStateDmaDesc;
>
> volatile PVSCSI_RING_REQ_DESC *RingReqs;
> PVSCSI_DMA_DESC RingReqsDmaDesc;
>
> volatile PVSCSI_RING_CMP_DESC *RingCmps;
> PVSCSI_DMA_DESC RingCmpsDmaDesc;
> } PVSCSI_RING_DESC;
- in patch#14, PVSCSI_DEV would change as follows:
> typedef struct {
> UINT32 Signature;
> EFI_PCI_IO_PROTOCOL *PciIo;
> EFI_EVENT ExitBoot;
> UINT64 OriginalPciAttributes;
> PVSCSI_RING_DESC RingDesc;
> volatile PVSCSI_DMA_BUFFER *DmaBuf;
> 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;
After these changes, the compiler would (justifiedly) flag a bunch of
code locations casting away the volatile qualification -- for example,
in the above function, in the assignment to the "RingsState" local
variable.
Clearly, most of these compilation errors would have to be fixed (not
suppressed), because they would be valid. Meaning:
- you'd have to volatile-qualify the "RingsState" local variable in all
of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(),
PvScsiWaitForRequestCompletion();
- you'd also have to volatile-qualify the return types of
PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion();
- you'd have to update PopulateRequest() and HandleResponse() too; and
the most annoying part of that would be that you could no longer use
CopyMem() and ZeroMem() -- because those functions take
pointer-to-void parameters, rather than pointer-to-volatile-void ones.
(FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype
-- it would be OK to cast away volatile in those calls, as we wouldn't
dereference the pointers in that case.)
So... the reason I'm not actually requesting these
volatile-qualifications is that (a) your use of MemoryFence() seems
mostly OK, and (b) the UEFI Driver Writer's guide recommends *either*
volatile *or* MemoryFence(). Of course using both techniques at the same
time is not a problem -- and in code I write I actually like to use both
at the same time --, but just one suffices too. (See section 4.2.6
"Memory ordering" in the DWG.)
The reason I'm writing this up here is because I want the "record" (the
mailing list archive) to show that we have considered this topic
explicitly.
Back to your patch:
On 03/25/20 17:10, Liran Alon wrote:
> +
> +/**
> + 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;
(1) Wrong indentation.
> + }
> +
> + gBS->Stall (Dev->WaitForCmpStallInUsecs);
> + }
> +
> + //
> + // Acknowledge PVSCSI_INTR_CMPL_MASK in device interrupt-status register
> + //
> + return PvScsiMmioWrite32 (
> + Dev,
> + PvScsiRegOffsetIntrStatus,
> + PVSCSI_INTR_CMPL_MASK
> + );
> +}
> +
> +/**
> + 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 EFI_BAD_BUFFER_SIZE;
> + }
> + if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) {
> + Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data);
> + return EFI_BAD_BUFFER_SIZE;
> + }
> +
> + //
> + // Encode PVSCSI request
> + //
> + ZeroMem (Request, sizeof (*Request));
> +
> + Request->Bus = 0;
> + Request->Target = TargetValue;
> + //
> + // This cast is safe as MaxLun is defined as UINT8
> + //
> + Request->Lun[1] = (UINT8)Lun;
> + Request->SenseLen = Packet->SenseDataLength;
Ah, *now* I understand why you chose MAX_UINT8 as the size of
"PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has
type UINT8, and this way you guarantee that the SCSI client's
"Packet->SenseDataLength" will always fit in the DMA buffer.
Good solution, but it *absolutely* needs to be documented in patch#14
("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact,
see my question (4) under patch#14.
(2) Also, please add a comment here that a "Dev->DmaBuf->SenseData"
overflow is not possible due to "Packet->SenseDataLength" having type
UINT8.
This would be a comment in the same vein as the "MaxLun" reference just
above -- I find *that* comment very helpful, too.
> + Request->SenseAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, SenseData);
(3) Please drop the (UINT64) cast -- see my earlier comment about
EFI_PHYSICAL_ADDRESS being a typedef to UINT64.
> + Request->CdbLen = Packet->CdbLength;
> + CopyMem (Request->Cdb, Packet->Cdb, Packet->CdbLength);
Right, this is safe, due to the PVSCSI_CDB_MAX_SIZE near the top of the
function (PVSCSI_CDB_MAX_SIZE is 16, and "sizeof
PVSCSI_RING_REQ_DESC.Cdb" is also 16).
> + 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 = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, Data);
(4) Same as (3).
> +
> + 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
> + )
> +{
> + //
> + // Check if device returned sense data
> + //
> + if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
> + //
> + // 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
> + );
> + } else {
> + //
> + // Signal no sense data returned
> + //
> + Packet->SenseDataLength = 0;
> + }
> +
> + //
> + // Copy device output from DMA communication buffer
> + //
> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> + CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
> + }
I'm unfamilar with the PVSCSI device model, but I think this is not
general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that
short reads are possible at least in theory.
(5) If a short read occurs (Response->DataLen <
Packet->InTransferLength), then we should adjust
"Packet->InTransferLength", and also copy that many bytes only.
(6) I think it would be prudent to update "Packet->OutTransferLength"
too, for short writes.
> +
> + //
> + // 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 PvScsiBtStatSelTimeout:
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> + return EFI_TIMEOUT;
> +
> + case PvScsiBtStatDatarun:
> + case PvScsiBtStatDataUnderrun:
> + //
> + // Report residual data in overrun/underrun
> + //
> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> + Packet->InTransferLength = Response->DataLen;
> + } else {
> + Packet->OutTransferLength = Response->DataLen;
> + }
OK, if we are sure that (a) the device will always report short
reads/writes like this, and that (b) the above assignments will never
cause InTransferLength / OutTransferLength to *grow*, then the
InTransferLength / OutTransferLength adjustments are sufficiently
covered.
Still:
(7) Please cast DataLen to UINT32, otherwise Visual Studio might whine.
(8) The CopyMem() call above should not copy garbage (at the tail).
Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
then I would prefer if:
- we always updated InTransferLength / OutTransferLength (regardless of
"Response->HostStatus"),
- and we only used these case labels (PvScsiBtStatDatarun /
PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> + return EFI_BAD_BUFFER_SIZE;
I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
But that's not the case here -- we do have a partially completed
transfer.
(9) Thus I feel we should use a "break" here.
> +
> + 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
> @@ -129,6 +496,23 @@ IsTargetInitialized (
> return FALSE;
> }
>
> +/**
> + 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;
> +}
> +
> //
> // Ext SCSI Pass Thru
> //
> @@ -144,7 +528,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++;
> +
(10) Please insert another MemoryFence () here.
> + 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);
> + }
> +
(11) Please insert a MemoryFence() here.
> + 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
> @@ -685,6 +1124,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 7f91d70fec79..08e876b75930 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -47,6 +47,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 fcffc90d46c8..ae6d8753623f 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> @@ -38,5 +38,6 @@
> gEfiPciIoProtocolGuid ## TO_START
>
> [Pcd]
> - gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES
> - gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES
> + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES
> + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES
> + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs ## CONSUMES
>
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 11:26 ` [edk2-devel] " Laszlo Ersek
@ 2020-03-27 13:04 ` Liran Alon
2020-03-27 13:20 ` Liran Alon
2020-03-27 21:05 ` Laszlo Ersek
0 siblings, 2 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-27 13:04 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 27/03/2020 14:26, Laszlo Ersek wrote:
> On 03/25/20 17:10, Liran Alon wrote:
>> +/**
>> + 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;
>> +}
> (Just some thoughts, not a request for changing the code.)
>
> Normally I prefer accessing buffers shared with the device though
> volatile-qualified pointers.
>
> Meaning, in this case, that every "PCI host" pointer (i.e., each pointer
> that is associated with a PVSCSI_DMA_DESC) would have to be
> volatile-qualified. In particular:
>
> - in patch#13, PVSCSI_RING_DESC would have to be updated like this:
>
>> typedef struct {
>> volatile PVSCSI_RINGS_STATE *RingState;
>> PVSCSI_DMA_DESC RingStateDmaDesc;
>>
>> volatile PVSCSI_RING_REQ_DESC *RingReqs;
>> PVSCSI_DMA_DESC RingReqsDmaDesc;
>>
>> volatile PVSCSI_RING_CMP_DESC *RingCmps;
>> PVSCSI_DMA_DESC RingCmpsDmaDesc;
>> } PVSCSI_RING_DESC;
> - in patch#14, PVSCSI_DEV would change as follows:
>
>> typedef struct {
>> UINT32 Signature;
>> EFI_PCI_IO_PROTOCOL *PciIo;
>> EFI_EVENT ExitBoot;
>> UINT64 OriginalPciAttributes;
>> PVSCSI_RING_DESC RingDesc;
>> volatile PVSCSI_DMA_BUFFER *DmaBuf;
>> 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;
> After these changes, the compiler would (justifiedly) flag a bunch of
> code locations casting away the volatile qualification -- for example,
> in the above function, in the assignment to the "RingsState" local
> variable.
>
> Clearly, most of these compilation errors would have to be fixed (not
> suppressed), because they would be valid. Meaning:
>
> - you'd have to volatile-qualify the "RingsState" local variable in all
> of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(),
> PvScsiWaitForRequestCompletion();
>
> - you'd also have to volatile-qualify the return types of
> PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion();
>
> - you'd have to update PopulateRequest() and HandleResponse() too; and
> the most annoying part of that would be that you could no longer use
> CopyMem() and ZeroMem() -- because those functions take
> pointer-to-void parameters, rather than pointer-to-volatile-void ones.
>
> (FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype
> -- it would be OK to cast away volatile in those calls, as we wouldn't
> dereference the pointers in that case.)
>
> So... the reason I'm not actually requesting these
> volatile-qualifications is that (a) your use of MemoryFence() seems
> mostly OK, and (b) the UEFI Driver Writer's guide recommends *either*
> volatile *or* MemoryFence(). Of course using both techniques at the same
> time is not a problem -- and in code I write I actually like to use both
> at the same time --, but just one suffices too. (See section 4.2.6
> "Memory ordering" in the DWG.)
>
> The reason I'm writing this up here is because I want the "record" (the
> mailing list archive) to show that we have considered this topic
> explicitly.
I prefer to remain with only memory fences if that's OK by you. As the
code is written now.
As it's allows for potential compiler optimization and leads to more
readable code in my opinion.
> Back to your patch:
>
> On 03/25/20 17:10, Liran Alon wrote:
>> + //
>> + // This cast is safe as MaxLun is defined as UINT8
>> + //
>> + Request->Lun[1] = (UINT8)Lun;
>> + Request->SenseLen = Packet->SenseDataLength;
> Ah, *now* I understand why you chose MAX_UINT8 as the size of
> "PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has
> type UINT8, and this way you guarantee that the SCSI client's
> "Packet->SenseDataLength" will always fit in the DMA buffer.
>
> Good solution, but it *absolutely* needs to be documented in patch#14
> ("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact,
> see my question (4) under patch#14.
Please read the response I have written you to your patch#14 review.
Where I suggest we define a constant in IndustryStandard/Scsi.h for the
limit of the total length of SenseData that is defined to be 252
according to SCSI specification.
>
> (2) Also, please add a comment here that a "Dev->DmaBuf->SenseData"
> overflow is not possible due to "Packet->SenseDataLength" having type
> UINT8.
>
> This would be a comment in the same vein as the "MaxLun" reference just
> above -- I find *that* comment very helpful, too.
OK.
>> +
>> + 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
>> + )
>> +{
>> + //
>> + // Check if device returned sense data
>> + //
>> + if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
>> + //
>> + // 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
>> + );
>> + } else {
>> + //
>> + // Signal no sense data returned
>> + //
>> + Packet->SenseDataLength = 0;
>> + }
>> +
>> + //
>> + // Copy device output from DMA communication buffer
>> + //
>> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>> + CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
>> + }
> I'm unfamilar with the PVSCSI device model, but I think this is not
> general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that
> short reads are possible at least in theory.
>
> (5) If a short read occurs (Response->DataLen <
> Packet->InTransferLength), then we should adjust
> "Packet->InTransferLength", and also copy that many bytes only.
>
> (6) I think it would be prudent to update "Packet->OutTransferLength"
> too, for short writes.
As you can see below, this is done in case device return
Response->HostStatus as either PvScsiBtStatDatarun or
PvScsiBtStatDataUnderrun.
>
>> +
>> + //
>> + // 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 PvScsiBtStatSelTimeout:
>> + Packet->HostAdapterStatus =
>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>> + return EFI_TIMEOUT;
>> +
>> + case PvScsiBtStatDatarun:
>> + case :
>> + //
>> + // Report residual data in overrun/underrun
>> + //
>> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>> + Packet->InTransferLength = Response->DataLen;
>> + } else {
>> + Packet->OutTransferLength = Response->DataLen;
>> + }
> OK, if we are sure that (a) the device will always report short
> reads/writes like this, and that (b) the above assignments will never
> cause InTransferLength / OutTransferLength to *grow*, then the
> InTransferLength / OutTransferLength adjustments are sufficiently
> covered.
I believe both of these are indeed true.
Even though that current QEMU VMware PVSCSI device emulation code have a
bug that it never sets this in pvscsi_command_complete() when it does
set BTSTAT_DATARUN...
> Still:
>
> (8) The CopyMem() call above should not copy garbage (at the tail).
I don't think it matters. We don't guarantee anything on the content in
Packet->InDataBuffer beyond Packet->InTransferLength.
I think the code is simpler how it is currently written.
>
> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
I don't think this is the case.
> then I would prefer if:
>
> - we always updated InTransferLength / OutTransferLength (regardless of
> "Response->HostStatus"),
>
> - and we only used these case labels (PvScsiBtStatDatarun /
> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
>
>> + Packet->HostAdapterStatus =
>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>> + return EFI_BAD_BUFFER_SIZE;
> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
> But that's not the case here -- we do have a partially completed
> transfer.
Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
in MdePkg/Include/Protocol/ScsiPassThru.h:
@retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
executed, but the
entire DataBuffer could not be
transferred.
The actual number of bytes
transferred is returned
in TransferLength. See
HostAdapterStatus,
TargetStatus, SenseDataLength, and
SenseData in
that order for additional status
information.
So I don't know who to believe... It does seem to me that this
documentation in the code makes more sense
and then my current code is correct. What do you think?
>
> (9) Thus I feel we should use a "break" here.
>
>> +
>> + 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;
>> +}
>> +
>>
>> //
>> // Ext SCSI Pass Thru
>> //
>> @@ -144,7 +528,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++;
>> +
> (10) Please insert another MemoryFence () here.
That would be unnecessary and wrong.
The MemoryFence() here is used to make sure the request is globally
visible before the update to the producer-index. As in any
circular-buffer implementation.
There is no need for an additional MemoryFence() here.
Note that the MMIO access below is guaranteed to be globally visible
only after the write to the producer-index.
If EDK2 MMIO accessors wouldn't have guaranteed this, you would have a
very broken code base...
Similar to why Linux MMIO accessors (e.g. writel()) macros guarantee these.
For example, see how MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
MmioWrite32() internally calls MemoryFence() before and after MMIO
access itself.
>
>> + 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);
>> + }
>> +
> (11) Please insert a MemoryFence() here.
Why is a MemoryFence() needed here? I don't think that's true.
PvScsiWaitForRequestCompletion() ends with an MMIO write which is
guaranteed to be a memory fence.
Thus, there is no need for a MemoryFence() here (to serve as a rmb()) to
make sure the completion-descriptor is globally visible.
Thanks,
-Liran
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 13:04 ` Liran Alon
@ 2020-03-27 13:20 ` Liran Alon
2020-03-27 21:05 ` Laszlo Ersek
2020-03-27 21:05 ` Laszlo Ersek
1 sibling, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-27 13:20 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 27/03/2020 16:04, Liran Alon wrote:
>
> On 27/03/2020 14:26, Laszlo Ersek wrote:
>> On 03/25/20 17:10, Liran Alon wrote:
>>
>>> +
>>> + //
>>> + // 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 PvScsiBtStatSelTimeout:
>>> + Packet->HostAdapterStatus =
>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>>> + return EFI_TIMEOUT;
>>> +
>>> + case PvScsiBtStatDatarun:
>>> + case :
>>> + //
>>> + // Report residual data in overrun/underrun
>>> + //
>>> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>> + Packet->InTransferLength = Response->DataLen;
>>> + } else {
>>> + Packet->OutTransferLength = Response->DataLen;
>>> + }
>> OK, if we are sure that (a) the device will always report short
>> reads/writes like this, and that (b) the above assignments will never
>> cause InTransferLength / OutTransferLength to *grow*, then the
>> InTransferLength / OutTransferLength adjustments are sufficiently
>> covered.
> I believe both of these are indeed true.
> Even though that current QEMU VMware PVSCSI device emulation code have
> a bug that it never sets this in pvscsi_command_complete() when it
> does set BTSTAT_DATARUN...
>> Still:
>>
>> (8) The CopyMem() call above should not copy garbage (at the tail).
> I don't think it matters. We don't guarantee anything on the content
> in Packet->InDataBuffer beyond Packet->InTransferLength.
> I think the code is simpler how it is currently written.
>>
>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
> I don't think this is the case.
>> then I would prefer if:
>>
>> - we always updated InTransferLength / OutTransferLength (regardless of
>> "Response->HostStatus"),
>>
>> - and we only used these case labels (PvScsiBtStatDatarun /
>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
Regarding all the above:
You can also see that Linux PVSCSI driver (drivers/scsi/vmw_pvscsi.c)
reads the "DataLen" field only in case the "HostStatus" is
BTSTAT_DATARUN or BTSTAT_DATA_UNDERRUN.
As I have done in my driver. In the lack of more detailed device
specification (As PVSCSI is a proprietary VMware PV device), I prefer to
remain with my implementation which seems
to be guaranteed to be safe and working. Please tell me if you think
otherwise.
-Liran
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 13:20 ` Liran Alon
@ 2020-03-27 21:05 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-27 21:05 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/27/20 14:20, Liran Alon wrote:
>
> On 27/03/2020 16:04, Liran Alon wrote:
>>
>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>> On 03/25/20 17:10, Liran Alon wrote:
>>>
>>>> +
>>>> + //
>>>> + // 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 PvScsiBtStatSelTimeout:
>>>> + Packet->HostAdapterStatus =
>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>>>> + return EFI_TIMEOUT;
>>>> +
>>>> + case PvScsiBtStatDatarun:
>>>> + case :
>>>> + //
>>>> + // Report residual data in overrun/underrun
>>>> + //
>>>> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>>> + Packet->InTransferLength = Response->DataLen;
>>>> + } else {
>>>> + Packet->OutTransferLength = Response->DataLen;
>>>> + }
>>> OK, if we are sure that (a) the device will always report short
>>> reads/writes like this, and that (b) the above assignments will never
>>> cause InTransferLength / OutTransferLength to *grow*, then the
>>> InTransferLength / OutTransferLength adjustments are sufficiently
>>> covered.
>> I believe both of these are indeed true.
>> Even though that current QEMU VMware PVSCSI device emulation code have
>> a bug that it never sets this in pvscsi_command_complete() when it
>> does set BTSTAT_DATARUN...
>>> Still:
>>>
>>> (8) The CopyMem() call above should not copy garbage (at the tail).
>> I don't think it matters. We don't guarantee anything on the content
>> in Packet->InDataBuffer beyond Packet->InTransferLength.
>> I think the code is simpler how it is currently written.
>>>
>>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
>> I don't think this is the case.
>>> then I would prefer if:
>>>
>>> - we always updated InTransferLength / OutTransferLength (regardless of
>>> "Response->HostStatus"),
>>>
>>> - and we only used these case labels (PvScsiBtStatDatarun /
>>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
>
> Regarding all the above:
> You can also see that Linux PVSCSI driver (drivers/scsi/vmw_pvscsi.c)
> reads the "DataLen" field only in case the "HostStatus" is
> BTSTAT_DATARUN or BTSTAT_DATA_UNDERRUN.
> As I have done in my driver. In the lack of more detailed device
> specification (As PVSCSI is a proprietary VMware PV device), I prefer to
> remain with my implementation which seems
> to be guaranteed to be safe and working. Please tell me if you think
> otherwise.
No, I'm fine with your reasoning.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 13:04 ` Liran Alon
2020-03-27 13:20 ` Liran Alon
@ 2020-03-27 21:05 ` Laszlo Ersek
2020-03-27 22:04 ` Liran Alon
1 sibling, 1 reply; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-27 21:05 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/27/20 14:04, Liran Alon wrote:
>
> On 27/03/2020 14:26, Laszlo Ersek wrote:
>> On 03/25/20 17:10, Liran Alon wrote:
>>> +/**
>>> + 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;
>>> +}
>> (Just some thoughts, not a request for changing the code.)
>>
>> Normally I prefer accessing buffers shared with the device though
>> volatile-qualified pointers.
>>
>> Meaning, in this case, that every "PCI host" pointer (i.e., each pointer
>> that is associated with a PVSCSI_DMA_DESC) would have to be
>> volatile-qualified. In particular:
>>
>> - in patch#13, PVSCSI_RING_DESC would have to be updated like this:
>>
>>> typedef struct {
>>> volatile PVSCSI_RINGS_STATE *RingState;
>>> PVSCSI_DMA_DESC RingStateDmaDesc;
>>>
>>> volatile PVSCSI_RING_REQ_DESC *RingReqs;
>>> PVSCSI_DMA_DESC RingReqsDmaDesc;
>>>
>>> volatile PVSCSI_RING_CMP_DESC *RingCmps;
>>> PVSCSI_DMA_DESC RingCmpsDmaDesc;
>>> } PVSCSI_RING_DESC;
>> - in patch#14, PVSCSI_DEV would change as follows:
>>
>>> typedef struct {
>>> UINT32 Signature;
>>> EFI_PCI_IO_PROTOCOL *PciIo;
>>> EFI_EVENT ExitBoot;
>>> UINT64 OriginalPciAttributes;
>>> PVSCSI_RING_DESC RingDesc;
>>> volatile PVSCSI_DMA_BUFFER *DmaBuf;
>>> 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;
>> After these changes, the compiler would (justifiedly) flag a bunch of
>> code locations casting away the volatile qualification -- for example,
>> in the above function, in the assignment to the "RingsState" local
>> variable.
>>
>> Clearly, most of these compilation errors would have to be fixed (not
>> suppressed), because they would be valid. Meaning:
>>
>> - you'd have to volatile-qualify the "RingsState" local variable in all
>> of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(),
>> PvScsiWaitForRequestCompletion();
>>
>> - you'd also have to volatile-qualify the return types of
>> PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion();
>>
>> - you'd have to update PopulateRequest() and HandleResponse() too; and
>> the most annoying part of that would be that you could no longer use
>> CopyMem() and ZeroMem() -- because those functions take
>> pointer-to-void parameters, rather than pointer-to-volatile-void ones.
>>
>> (FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype
>> -- it would be OK to cast away volatile in those calls, as we wouldn't
>> dereference the pointers in that case.)
>>
>> So... the reason I'm not actually requesting these
>> volatile-qualifications is that (a) your use of MemoryFence() seems
>> mostly OK, and (b) the UEFI Driver Writer's guide recommends *either*
>> volatile *or* MemoryFence(). Of course using both techniques at the same
>> time is not a problem -- and in code I write I actually like to use both
>> at the same time --, but just one suffices too. (See section 4.2.6
>> "Memory ordering" in the DWG.)
>>
>> The reason I'm writing this up here is because I want the "record" (the
>> mailing list archive) to show that we have considered this topic
>> explicitly.
> I prefer to remain with only memory fences if that's OK by you.
Yes, that's fine.
> As the code is written now.
> As it's allows for potential compiler optimization and leads to more
> readable code in my opinion.
The UEFI Driver Writer's Guide makes the same argument -- it favors
explicit MemoryFence()s over volatile. So your suggestion is entirely
valid and I agree with it.
>> Back to your patch:
>>
>> On 03/25/20 17:10, Liran Alon wrote:
>>> + //
>>> + // This cast is safe as MaxLun is defined as UINT8
>>> + //
>>> + Request->Lun[1] = (UINT8)Lun;
>>> + Request->SenseLen = Packet->SenseDataLength;
>> Ah, *now* I understand why you chose MAX_UINT8 as the size of
>> "PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has
>> type UINT8, and this way you guarantee that the SCSI client's
>> "Packet->SenseDataLength" will always fit in the DMA buffer.
>>
>> Good solution, but it *absolutely* needs to be documented in patch#14
>> ("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact,
>> see my question (4) under patch#14.
> Please read the response I have written you to your patch#14 review.
> Where I suggest we define a constant in IndustryStandard/Scsi.h for the
> limit of the total length of SenseData that is defined to be 252
> according to SCSI specification.
MdePkg macro is good, but it should be decoupled from this series.
>>
>> (2) Also, please add a comment here that a "Dev->DmaBuf->SenseData"
>> overflow is not possible due to "Packet->SenseDataLength" having type
>> UINT8.
>>
>> This would be a comment in the same vein as the "MaxLun" reference just
>> above -- I find *that* comment very helpful, too.
> OK.
>>> +
>>> + 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
>>> + )
>>> +{
>>> + //
>>> + // Check if device returned sense data
>>> + //
>>> + if (Response->ScsiStatus ==
>>> EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
>>> + //
>>> + // 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
>>> + );
>>> + } else {
>>> + //
>>> + // Signal no sense data returned
>>> + //
>>> + Packet->SenseDataLength = 0;
>>> + }
>>> +
>>> + //
>>> + // Copy device output from DMA communication buffer
>>> + //
>>> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>> + CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data,
>>> Packet->InTransferLength);
>>> + }
>> I'm unfamilar with the PVSCSI device model, but I think this is not
>> general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that
>> short reads are possible at least in theory.
>>
>> (5) If a short read occurs (Response->DataLen <
>> Packet->InTransferLength), then we should adjust
>> "Packet->InTransferLength", and also copy that many bytes only.
>>
>> (6) I think it would be prudent to update "Packet->OutTransferLength"
>> too, for short writes.
> As you can see below, this is done in case device return
> Response->HostStatus as either PvScsiBtStatDatarun or
> PvScsiBtStatDataUnderrun.
>>
>>> +
>>> + //
>>> + // 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 PvScsiBtStatSelTimeout:
>>> + Packet->HostAdapterStatus =
>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>>> + return EFI_TIMEOUT;
>>> +
>>> + case PvScsiBtStatDatarun:
>>> + case :
>>> + //
>>> + // Report residual data in overrun/underrun
>>> + //
>>> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>> + Packet->InTransferLength = Response->DataLen;
>>> + } else {
>>> + Packet->OutTransferLength = Response->DataLen;
>>> + }
>> OK, if we are sure that (a) the device will always report short
>> reads/writes like this, and that (b) the above assignments will never
>> cause InTransferLength / OutTransferLength to *grow*, then the
>> InTransferLength / OutTransferLength adjustments are sufficiently
>> covered.
> I believe both of these are indeed true.
> Even though that current QEMU VMware PVSCSI device emulation code have a
> bug that it never sets this in pvscsi_command_complete() when it does
> set BTSTAT_DATARUN...
>> Still:
>>
>> (8) The CopyMem() call above should not copy garbage (at the tail).
> I don't think it matters. We don't guarantee anything on the content in
> Packet->InDataBuffer beyond Packet->InTransferLength.
> I think the code is simpler how it is currently written.
I'm not convinced, but this is not a question I feel very strongly
about. I OK to go with your preference.
>>
>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
> I don't think this is the case.
>> then I would prefer if:
>>
>> - we always updated InTransferLength / OutTransferLength (regardless of
>> "Response->HostStatus"),
>>
>> - and we only used these case labels (PvScsiBtStatDatarun /
>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
>>
>>> + Packet->HostAdapterStatus =
>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>> + return EFI_BAD_BUFFER_SIZE;
>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>> But that's not the case here -- we do have a partially completed
>> transfer.
>
> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
> in MdePkg/Include/Protocol/ScsiPassThru.h:
>
> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
> executed, but the
> entire DataBuffer could not be
> transferred.
> The actual number of bytes
> transferred is returned
> in TransferLength. See
> HostAdapterStatus,
> TargetStatus, SenseDataLength, and
> SenseData in
> that order for additional status
> information.
>
> So I don't know who to believe... It does seem to me that this
> documentation in the code makes more sense
> and then my current code is correct. What do you think?
You are looking at the wrong protocol header file. The top of this
header file bears the comment
SCSI Pass Through protocol as defined in EFI 1.1.
and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
only refers to Mantis ticket 845
<https://mantis.uefi.org/mantis/view.php?id=845> with subject
"EFI_SCSI_PASS_THRU_PROTOCOL replacement".
Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
"MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
SCSI Request Packet was not executed").
>
>>
>> (9) Thus I feel we should use a "break" here.
>>
>>> +
>>> + 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;
>>> +}
>>> +
>>>
>>> //
>>> // Ext SCSI Pass Thru
>>> //
>>> @@ -144,7 +528,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++;
>>> +
>> (10) Please insert another MemoryFence () here.
>
> That would be unnecessary and wrong.
>
> The MemoryFence() here is used to make sure the request is globally
> visible before the update to the producer-index.
I agree.
> As in any
> circular-buffer implementation.
> There is no need for an additional MemoryFence() here.
>
> Note that the MMIO access below is guaranteed to be globally visible
> only after the write to the producer-index.
Yes, that was the goal of my suggestion. What guarantees it?
> If EDK2 MMIO accessors wouldn't have guaranteed this, you would have a
> very broken code base...
> Similar to why Linux MMIO accessors (e.g. writel()) macros guarantee these.
>
> For example, see how MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> MmioWrite32() internally calls MemoryFence() before and after MMIO
> access itself.
So basically you are saying that I proposed the right thing, except
there is no need to spell it out here, because the MMIO accessor
primitives already cover that internally :)
I admit that I have not been aware of the internal fences!
(And given that there is a specific commit in the git history to push
the fences into the source file you mention, namely 9de780dcd6208, I do
think my suggestion was not "wrong", only unnecessary.)
I do agree that the MemoryFence() need not be added in this spot. Thanks
for making me aware of the internal fences!
>
>>
>>> + 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);
>>> + }
>>> +
>> (11) Please insert a MemoryFence() here.
>
> Why is a MemoryFence() needed here? I don't think that's true.
>
> PvScsiWaitForRequestCompletion() ends with an MMIO write which is
> guaranteed to be a memory fence.
Yes, I see that now. My point was that a fence needed to *occur* here. I
didn't realize it was already covered, internally.
> Thus, there is no need for a MemoryFence() here (to serve as a rmb()) to
> make sure the completion-descriptor is globally visible.
Agreed.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 21:05 ` Laszlo Ersek
@ 2020-03-27 22:04 ` Liran Alon
2020-03-27 22:17 ` Liran Alon
2020-03-30 10:30 ` Laszlo Ersek
0 siblings, 2 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-27 22:04 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 28/03/2020 0:05, Laszlo Ersek wrote:
> On 03/27/20 14:04, Liran Alon wrote:
>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>> On 03/25/20 17:10, Liran Alon wrote:
>>>> + Packet->HostAdapterStatus =
>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>> + return EFI_BAD_BUFFER_SIZE;
>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>> But that's not the case here -- we do have a partially completed
>>> transfer.
>> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>
>> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
>> executed, but the
>> entire DataBuffer could not be
>> transferred.
>> The actual number of bytes
>> transferred is returned
>> in TransferLength. See
>> HostAdapterStatus,
>> TargetStatus, SenseDataLength, and
>> SenseData in
>> that order for additional status
>> information.
>>
>> So I don't know who to believe... It does seem to me that this
>> documentation in the code makes more sense
>> and then my current code is correct. What do you think?
> You are looking at the wrong protocol header file.
Oops. You are right.
> The top of this
> header file bears the comment
>
> SCSI Pass Through protocol as defined in EFI 1.1.
>
> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
> only refers to Mantis ticket 845
> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$ > with subject
> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>
> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
> SCSI Request Packet was not executed").
Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and
ScsiPassThruExt.h header file. It doesn't make sense.
In the new header file, there is no way for PassThru() to indicate to
caller that a partial data-transfer have occurred.
Which seems to be a quite standard functionality. Almost all SCSI
devices are able to return information on partial data-transfer completions.
Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete()
implementation, we can see how it handles partial data-transfer completions.
It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual
accordingly.
Looking at EDK2 VirtioScsi driver, function ParseResponse(), this will
be handled by updating Packet->InTransferLength and
Packet->OutTransferLength based on Response->Residual,
but then setting Packet->HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
(In contrast to your suggestion for me to return EFI_DEVICE_ERROR in
this case).
The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS
specifies:
@retval EFI_SUCCESS The SCSI Request Packet was sent by the
host. For bi-directional
commands, InTransferLength bytes were
transferred from
InDataBuffer. For write and
bi-directional commands,
OutTransferLength bytes were transferred by
OutDataBuffer.
But it seems to reference the InTransferLength and OutTransferLength
*before* the call to PassThru().
In contrast to old header file, ScsiPassThru.h, which for
EFI_BAD_BUFFER_SIZE specifies explicitly:
@retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
executed, but the
entire DataBuffer could not be
transferred.
The actual number of bytes
transferred is returned
in TransferLength. See
HostAdapterStatus,
TargetStatus, SenseDataLength, and
SenseData in
that order for additional status
information.
In addition, looking at EDK2 iSCSI ScsiPassThru driver
(NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use
EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
(Which matches the new header file ScsiPassThruExt.h and not the old
ScsiPassThru.h).
If you would have a look at how IScsiOnScsiRspRcvd() handles
SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW bits,
it seems to always
set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating
Packet->InTransferLength / Packet->OutTransferLength.
What do you think? Do you have any further insights? This is quite
confusing to me.
-Liran
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 22:04 ` Liran Alon
@ 2020-03-27 22:17 ` Liran Alon
2020-03-28 19:18 ` Liran Alon
2020-03-30 11:12 ` Laszlo Ersek
2020-03-30 10:30 ` Laszlo Ersek
1 sibling, 2 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-27 22:17 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 28/03/2020 1:04, Liran Alon wrote:
>
> On 28/03/2020 0:05, Laszlo Ersek wrote:
>> On 03/27/20 14:04, Liran Alon wrote:
>>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>> + Packet->HostAdapterStatus =
>>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>>> + return EFI_BAD_BUFFER_SIZE;
>>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI
>>>> spec,
>>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>>> But that's not the case here -- we do have a partially completed
>>>> transfer.
>>> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
>>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>>
>>> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
>>> executed, but the
>>> entire DataBuffer could not be
>>> transferred.
>>> The actual number of bytes
>>> transferred is returned
>>> in TransferLength. See
>>> HostAdapterStatus,
>>> TargetStatus, SenseDataLength, and
>>> SenseData in
>>> that order for additional status
>>> information.
>>>
>>> So I don't know who to believe... It does seem to me that this
>>> documentation in the code makes more sense
>>> and then my current code is correct. What do you think?
>> You are looking at the wrong protocol header file.
> Oops. You are right.
>> The top of this
>> header file bears the comment
>>
>> SCSI Pass Through protocol as defined in EFI 1.1.
>>
>> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
>> only refers to Mantis ticket 845
>> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$
>> > with subject
>> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>>
>> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
>> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
>> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
>> SCSI Request Packet was not executed").
>
> Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and
> ScsiPassThruExt.h header file. It doesn't make sense.
> In the new header file, there is no way for PassThru() to indicate to
> caller that a partial data-transfer have occurred.
> Which seems to be a quite standard functionality. Almost all SCSI
> devices are able to return information on partial data-transfer
> completions.
>
> Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete()
> implementation, we can see how it handles partial data-transfer
> completions.
> It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual
> accordingly.
> Looking at EDK2 VirtioScsi driver, function ParseResponse(), this will
> be handled by updating Packet->InTransferLength and
> Packet->OutTransferLength based on Response->Residual,
> but then setting Packet->HostAdapterStatus to
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
> (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in
> this case).
>
> The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS
> specifies:
>
> @retval EFI_SUCCESS The SCSI Request Packet was sent by
> the host. For bi-directional
> commands, InTransferLength bytes were
> transferred from
> InDataBuffer. For write and
> bi-directional commands,
> OutTransferLength bytes were
> transferred by
> OutDataBuffer.
>
> But it seems to reference the InTransferLength and OutTransferLength
> *before* the call to PassThru().
>
> In contrast to old header file, ScsiPassThru.h, which for
> EFI_BAD_BUFFER_SIZE specifies explicitly:
>
> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
> executed, but the
> entire DataBuffer could not be
> transferred.
> The actual number of bytes
> transferred is returned
> in TransferLength. See
> HostAdapterStatus,
> TargetStatus, SenseDataLength, and
> SenseData in
> that order for additional status
> information.
>
> In addition, looking at EDK2 iSCSI ScsiPassThru driver
> (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use
> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
> (Which matches the new header file ScsiPassThruExt.h and not the old
> ScsiPassThru.h).
> If you would have a look at how IScsiOnScsiRspRcvd() handles
> SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW
> bits, it seems to always
> set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating
> Packet->InTransferLength / Packet->OutTransferLength.
>
> What do you think? Do you have any further insights? This is quite
> confusing to me.
>
> -Liran
>
Furthermore, I have later found ScsiExecuteSCSICommand() in
MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls the
PassThru() method.
Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is
relevant to us), one can see it just simply executes the PassThru()
device and returns.
Examining ScsiExecuteSCSICommand() documentation specifies for
EFI_BAD_BUFFER_SIZE:
@retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
but the entire DataBuffer could not be
transferred.
The actual number of bytes transferred is
returned
in TransferLength. See HostAdapterStatus,
TargetStatus, SenseDataLength, and
SenseData in
that order for additional status information.
Which again seems to align to what I have done in my PvScsi driver...
-Liran
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 22:17 ` Liran Alon
@ 2020-03-28 19:18 ` Liran Alon
2020-03-30 11:23 ` Laszlo Ersek
2020-03-30 11:12 ` Laszlo Ersek
1 sibling, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-28 19:18 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 28/03/2020 1:17, Liran Alon wrote:
>
> On 28/03/2020 1:04, Liran Alon wrote:
>>
>> On 28/03/2020 0:05, Laszlo Ersek wrote:
>>> On 03/27/20 14:04, Liran Alon wrote:
>>>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>>> + Packet->HostAdapterStatus =
>>>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>>>> + return EFI_BAD_BUFFER_SIZE;
>>>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI
>>>>> spec,
>>>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>>>> But that's not the case here -- we do have a partially completed
>>>>> transfer.
>>>> Hmm... According to the documentation above
>>>> EFI_SCSI_PASS_THRU_PASSTHRU
>>>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>>>
>>>> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
>>>> executed, but the
>>>> entire DataBuffer could not be
>>>> transferred.
>>>> The actual number of bytes
>>>> transferred is returned
>>>> in TransferLength. See
>>>> HostAdapterStatus,
>>>> TargetStatus, SenseDataLength,
>>>> and
>>>> SenseData in
>>>> that order for additional status
>>>> information.
>>>>
>>>> So I don't know who to believe... It does seem to me that this
>>>> documentation in the code makes more sense
>>>> and then my current code is correct. What do you think?
>>> You are looking at the wrong protocol header file.
>> Oops. You are right.
>>> The top of this
>>> header file bears the comment
>>>
>>> SCSI Pass Through protocol as defined in EFI 1.1.
>>>
>>> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
>>> only refers to Mantis ticket 845
>>> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$
>>> > with subject
>>> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>>>
>>> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
>>> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
>>> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
>>> SCSI Request Packet was not executed").
>>
>> Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and
>> ScsiPassThruExt.h header file. It doesn't make sense.
>> In the new header file, there is no way for PassThru() to indicate to
>> caller that a partial data-transfer have occurred.
>> Which seems to be a quite standard functionality. Almost all SCSI
>> devices are able to return information on partial data-transfer
>> completions.
>>
>> Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete()
>> implementation, we can see how it handles partial data-transfer
>> completions.
>> It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual
>> accordingly.
>> Looking at EDK2 VirtioScsi driver, function ParseResponse(), this
>> will be handled by updating Packet->InTransferLength and
>> Packet->OutTransferLength based on Response->Residual,
>> but then setting Packet->HostAdapterStatus to
>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
>> (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in
>> this case).
>>
>> The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS
>> specifies:
>>
>> @retval EFI_SUCCESS The SCSI Request Packet was sent by
>> the host. For bi-directional
>> commands, InTransferLength bytes were
>> transferred from
>> InDataBuffer. For write and
>> bi-directional commands,
>> OutTransferLength bytes were
>> transferred by
>> OutDataBuffer.
>>
>> But it seems to reference the InTransferLength and OutTransferLength
>> *before* the call to PassThru().
>>
>> In contrast to old header file, ScsiPassThru.h, which for
>> EFI_BAD_BUFFER_SIZE specifies explicitly:
>>
>> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
>> executed, but the
>> entire DataBuffer could not be
>> transferred.
>> The actual number of bytes
>> transferred is returned
>> in TransferLength. See
>> HostAdapterStatus,
>> TargetStatus, SenseDataLength,
>> and SenseData in
>> that order for additional status
>> information.
>>
>> In addition, looking at EDK2 iSCSI ScsiPassThru driver
>> (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use
>> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
>> (Which matches the new header file ScsiPassThruExt.h and not the old
>> ScsiPassThru.h).
>> If you would have a look at how IScsiOnScsiRspRcvd() handles
>> SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW
>> bits, it seems to always
>> set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating
>> Packet->InTransferLength / Packet->OutTransferLength.
>>
>> What do you think? Do you have any further insights? This is quite
>> confusing to me.
>>
>> -Liran
>>
> Furthermore, I have later found ScsiExecuteSCSICommand() in
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls
> the PassThru() method.
> Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is
> relevant to us), one can see it just simply executes the PassThru()
> device and returns.
> Examining ScsiExecuteSCSICommand() documentation specifies for
> EFI_BAD_BUFFER_SIZE:
>
> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
> but the entire DataBuffer could not be
> transferred.
> The actual number of bytes transferred
> is returned
> in TransferLength. See HostAdapterStatus,
> TargetStatus, SenseDataLength, and
> SenseData in
> that order for additional status
> information.
>
> Which again seems to align to what I have done in my PvScsi driver...
>
> -Liran
>
Sorry for the spam but I think I eventually figured it out.
The call-chain in EDK2 looks like this:
ScsiDiskWriteSectors() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
ScsiDiskWrite16() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
ScsiWrite16Command() (MdePkg/Library/UefiScsiLib/UefiScsiLib.c)
ScsiExecuteSCSICommand() (MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c)
ExtScsiPassThru->PassThru()
It can be seen that:
* ScsiExecuteSCSICommand() just passes Packet to
ExtScsiPassThru->PassThru() and returns it's return value.
* ScsiWrite16Command() always updates DataLength with
Packet->OutTransferLength and returns ScsiExecuteSCSICommand() return value.
* In ScsiDiskWrite16():
** If EFI_BAD_BUFFER_SIZE is returned, NeedRetry is set to TRUE and
EFI_DEVICE_ERROR is returned.
** Otherwise, if EFI_SUCCESS is returned but HostAdapterStatus is set
to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, then again
NeedRetry is set to TRUE and EFI_DEVICE_ERROR is returned.
** Otherwise, just return EFI_SUCCESS to caller.
* In ScsiDiskWriteSectors() we finally see the logic we were looking for:
** If ScsiDiskWrite16() returned EFI_SUCCESS, then we break out of
retry loop and indeed take into account ByteCount. (Which was updated
from Packet->OutTransferLength).
** Otherwise, (e.g. EFI_BAD_BUFFER_SIZE was returned), if NeedRetry
is set to TRUE, then next request retry is updated to be done with
ByteCount (Which was updated from Packet->OutTransferLength).
So it seems the UEFI-2.8 spec is right and a lot of EDK2 function
documentation is out-of-date.
Therefore, I will update my code accordingly. i.e.:
1) Change PvScsi.c PopulateRequest() such that in case TransferLength is
too big for DMA communication buffer Data field, we update
TransferLength and return EFI_BAD_BUFFER_SIZE.
But in addition, for completion, we should also set
HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and TargetStatus
to EFI_EXT_SCSI_STATUS_TARGET_GOOD
and SenseDataLength to 0 as done in VirtioScsi.c PopulateRequest()
in case it returns EFI_BAD_BUFFER_SIZE.
2) Change PvScsi.c HandleResponse() such that:
** If device returned PvScsiBtStatDataUnderrun: Update TransferLength
with Response->DataLen, set HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
EFI_SUCCESS.
** If device returned PvScsiBtStatDatarun: Just set HostAdapterStatus
to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
EFI_SUCCESS.
Regards,
-Liran
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-28 19:18 ` Liran Alon
@ 2020-03-30 11:23 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-30 11:23 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/28/20 20:18, Liran Alon wrote:
> Sorry for the spam but I think I eventually figured it out.
It's not spam; thank you for the thorough investigation!
> The call-chain in EDK2 looks like this:
> ScsiDiskWriteSectors() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
> ScsiDiskWrite16() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
> ScsiWrite16Command() (MdePkg/Library/UefiScsiLib/UefiScsiLib.c)
> ScsiExecuteSCSICommand() (MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c)
> ExtScsiPassThru->PassThru()
>
> It can be seen that:
> * ScsiExecuteSCSICommand() just passes Packet to
> ExtScsiPassThru->PassThru() and returns it's return value.
> * ScsiWrite16Command() always updates DataLength with
> Packet->OutTransferLength and returns ScsiExecuteSCSICommand() return
> value.
> * In ScsiDiskWrite16():
> ** If EFI_BAD_BUFFER_SIZE is returned, NeedRetry is set to TRUE and
> EFI_DEVICE_ERROR is returned.
> ** Otherwise, if EFI_SUCCESS is returned but HostAdapterStatus is set
> to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, then again
> NeedRetry is set to TRUE and EFI_DEVICE_ERROR is returned.
> ** Otherwise, just return EFI_SUCCESS to caller.
> * In ScsiDiskWriteSectors() we finally see the logic we were looking for:
> ** If ScsiDiskWrite16() returned EFI_SUCCESS, then we break out of
> retry loop and indeed take into account ByteCount. (Which was updated
> from Packet->OutTransferLength).
> ** Otherwise, (e.g. EFI_BAD_BUFFER_SIZE was returned), if NeedRetry is
> set to TRUE, then next request retry is updated to be done with
> ByteCount (Which was updated from Packet->OutTransferLength).
>
> So it seems the UEFI-2.8 spec is right and a lot of EDK2 function
> documentation is out-of-date.
On a tangent... Since you mention ScsiDiskWriteSectors(), let me mention commit 5abc2a70da4f ("MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers", 2015-09-10), and the related mailing list discussion (this is what I referred to before, when I mentioned that Paolo had looked at the VirtioScsiDxe code twice -- this is the second occasion, from September 2015):
http://mid.mail-archive.com/1441390936-27763-1-git-send-email-lersek@redhat.com
> Therefore, I will update my code accordingly. i.e.:
> 1) Change PvScsi.c PopulateRequest() such that in case TransferLength is
> too big for DMA communication buffer Data field, we update
> TransferLength and return EFI_BAD_BUFFER_SIZE.
Sounds OK.
> But in addition, for completion, we should also set
> HostAdapterStatus to
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and TargetStatus
> to EFI_EXT_SCSI_STATUS_TARGET_GOOD
> and SenseDataLength to 0 as done in VirtioScsi.c PopulateRequest()
> in case it returns EFI_BAD_BUFFER_SIZE.
OK, thanks.
> 2) Change PvScsi.c HandleResponse() such that:
> ** If device returned PvScsiBtStatDataUnderrun: Update TransferLength
> with Response->DataLen, set HostAdapterStatus to
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
> EFI_SUCCESS.
OK.
> ** If device returned PvScsiBtStatDatarun: Just set HostAdapterStatus
> to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
> EFI_SUCCESS.
Again I'm not familiar with PvScsiBtStatDataUnderrun / PvScsiBtStatDatarun (let alone their differences); I'm OK with this too.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 22:17 ` Liran Alon
2020-03-28 19:18 ` Liran Alon
@ 2020-03-30 11:12 ` Laszlo Ersek
1 sibling, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-30 11:12 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/27/20 23:17, Liran Alon wrote:
> Furthermore, I have later found ScsiExecuteSCSICommand() in
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls the
> PassThru() method.
> Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is
> relevant to us), one can see it just simply executes the PassThru()
> device and returns.
> Examining ScsiExecuteSCSICommand() documentation specifies for
> EFI_BAD_BUFFER_SIZE:
>
> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
> but the entire DataBuffer could not be
> transferred.
> The actual number of bytes transferred is
> returned
> in TransferLength. See HostAdapterStatus,
> TargetStatus, SenseDataLength, and
> SenseData in
> that order for additional status information.
>
(1) The commit that added the "ScsiIoDevice->ExtScsiSupport" branch to ScsiExecuteSCSICommand() was 70c94b3b6ddf ("Porting R8's PI-enabled ScsiBus driver", 2007-07-02).
$ git show -U100 70c94b3b6ddf
At that time, the ScsiExecuteSCSICommand() function had the following documentation:
EFI_SUCCESS - The SCSI Request Packet was sent by the host
successfully, and TransferLength bytes were
transferred to/from DataBuffer.See
HostAdapterStatus, TargetStatus,
SenseDataLength, and SenseData in that order
for additional status information.
EFI_WARN_BUFFER_TOO_SMALL - The SCSI Request Packet was executed,
but the entire DataBuffer could not be transferred.
The actual number of bytes transferred is returned
in TransferLength. See HostAdapterStatus,
TargetStatus, SenseDataLength, and SenseData in
that order for additional status information.
No "EFI_BAD_BUFFER_SIZE".
So commit 70c94b3b6ddf was not entirely correct, because after it, the ScsiExecuteSCSICommand() could return EFI_BAD_BUFFER_SIZE (propagating it from ScsiIoDevice->ExtScsiPassThru->PassThru()), but the function's documentation was not updated.
(At the same commit, "MdePkg/Include/Protocol/ScsiPassThruExt.h" already specified EFI_BAD_BUFFER_SIZE as "The SCSI Request Packet was not executed" -- that had come from commit d1f950002362, "Checked in the Protocols introduced in UEFI/PI.", 2007-06-19.)
(2) In commit f36d6e669c97 (2007-09-20), the leading comment block on ScsiExecuteSCSICommand() was updated. The EFI_WARN_BUFFER_TOO_SMALL retval disappeared, and the incorrect EFI_BAD_BUFFER_SIZE language ("The SCSI Request Packet was executed") appeared. (Incorrect for the "EXT" passthru, anyway.)
Interestingly, in this commit message, we see:
3. Correctify some return status to sync with newest Uefi Spec 2.1
However, in the UEFI 2.0 spec <http://www.uefi.org/sites/default/files/resources/UEFI_Specification_2_and_Errata_Sept16_08.pdf>, EFI_BAD_BUFFER_SIZE is already defined as "The SCSI Request Packet was not executed".
So I think it's a bug in edk2 (at least a documentation bug).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
2020-03-27 22:04 ` Liran Alon
2020-03-27 22:17 ` Liran Alon
@ 2020-03-30 10:30 ` Laszlo Ersek
1 sibling, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-30 10:30 UTC (permalink / raw)
To: Liran Alon, devel
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/27/20 23:04, Liran Alon wrote:
>
> On 28/03/2020 0:05, Laszlo Ersek wrote:
>> On 03/27/20 14:04, Liran Alon wrote:
>>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>> + Packet->HostAdapterStatus =
>>>>> +
>>>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>>> + return EFI_BAD_BUFFER_SIZE;
>>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI
>>>> spec,
>>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>>> But that's not the case here -- we do have a partially completed
>>>> transfer.
>>> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
>>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>>
>>> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was
>>> executed, but the
>>> entire DataBuffer could not be
>>> transferred.
>>> The actual number of bytes
>>> transferred is returned
>>> in TransferLength. See
>>> HostAdapterStatus,
>>> TargetStatus, SenseDataLength, and
>>> SenseData in
>>> that order for additional status
>>> information.
>>>
>>> So I don't know who to believe... It does seem to me that this
>>> documentation in the code makes more sense
>>> and then my current code is correct. What do you think?
>> You are looking at the wrong protocol header file.
> Oops. You are right.
>> The top of this
>> header file bears the comment
>>
>> SCSI Pass Through protocol as defined in EFI 1.1.
>>
>> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
>> only refers to Mantis ticket 845
>> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$
>> > with subject
>> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>>
>> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
>> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
>> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
>> SCSI Request Packet was not executed").
>
> Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and
> ScsiPassThruExt.h header file. It doesn't make sense.
I agree there is a bug in the UEFI spec (but I do think the intent is
mostly clear). More below.
> In the new header file, there is no way for PassThru() to indicate to
> caller that a partial data-transfer have occurred.
> Which seems to be a quite standard functionality. Almost all SCSI
> devices are able to return information on partial data-transfer
> completions.
My understanding from the spec is that EFI_BAD_BUFFER_SIZE is for the
case when the driver can determine up-front -- without talking to the
device for the specific request's sake -- that the request is over-sized.
Otherwise (= it makes sense to submit the request to the device), a
short transfer is normal / expected. If there is no other issue with the
transfer, then InTransferLength / OutTransferLength should be updated on
output, and EFI_SUCCESS should be returned.
>
> Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete()
> implementation, we can see how it handles partial data-transfer
> completions.
> It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual
> accordingly.
> Looking at EDK2 VirtioScsi driver, function ParseResponse(), this will
> be handled by updating Packet->InTransferLength and
> Packet->OutTransferLength based on Response->Residual,
> but then setting Packet->HostAdapterStatus to
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
> (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in
> this case).
This is what I wrote, verbatim:
'(9) Thus I feel we should use a "break" here.'
Note: "feel".
VirtioScsiDxe returns EFI_SUCCESS (and modifies
InTransferLength/OutTransferLength, per "residual") when the device
reports VIRTIO_SCSI_S_OK. That's a top-level success report from the device.
PVSCSI does not return a top-level success code in this case
(apparently). I have never before worked with PVSCSI, and I don't know
what exactly its return codes stand for. If "PvScsiBtStatDatarun" and
"PvScsiBtStatDataUnderrun" mean "normal partial transfer", then I fully
agree with you that EFI_DEVICE_ERROR would not be correct.
> The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS
> specifies:
>
> @retval EFI_SUCCESS The SCSI Request Packet was sent by the
> host. For bi-directional
> commands, InTransferLength bytes were
> transferred from
> InDataBuffer. For write and
> bi-directional commands,
> OutTransferLength bytes were transferred by
> OutDataBuffer.
>
> But it seems to reference the InTransferLength and OutTransferLength
> *before* the call to PassThru().
If you look at the individual "InTransferLength" and "OutTransferLength"
parameter (i.e., Packet member) specifications, in the spec (not the
header file), they do clarify "on input" vs. "on output". They make
sense to me, in connection with the EFI_SUCCESS return status.
(There *is* a bug, IMO, in the documentation of EFI_BAD_BUFFER_SIZE, in
the spec.)
Quote:
----v----
InTransferLength On Input, the size, in bytes, of InDataBuffer. On
output, the number of bytes transferred between the
SCSI controller and the SCSI device. If
InTransferLength is larger than the SCSI controller
can handle, no data will be transferred,
InTransferLength will be updated to contain the number
of bytes that the SCSI controller is able to transfer,
and EFI_BAD_BUFFER_SIZE will be returned.
OutTransferLength On Input, the size, in bytes of OutDataBuffer. On
Output, the Number of bytes transferred between SCSI
Controller and the SCSI device. If OutTransferLength
is larger than the SCSI controller can handle, no data
will be transferred, OutTransferLength will be updated
to contain the number of bytes that the SCSI
controller is able to transfer, and
EFI_BAD_BUFFER_SIZE will be returned.
If the data buffer described by InDataBuffer and InTransferLength is too
big to be transferred in a single command, then no data is transferred
and EFI_BAD_BUFFER_SIZE is returned. The number of bytes that can be
transferred in a single command are returned in InTransferLength.
If the data buffer described by OutDataBuffer and OutTransferLength is
too big to be transferred in a single command, then no data is
transferred and EFI_BAD_BUFFER_SIZE is returned. The number of bytes
that can be transferred in a single command are returned in
OutTransferLength.
EFI_SUCCESS The SCSI Request Packet was sent by the host. For
bi-directional commands, InTransferLength bytes were
transferred from InDataBuffer. For write and
bi-directional commands, OutTransferLength bytes were
transferred by OutDataBuffer. See HostAdapterStatus,
TargetStatus, SenseDataLength, and SenseData in that
order for additional status information.
EFI_BAD_BUFFER_SIZE The SCSI Request Packet was not executed. The number
of bytes that could be transferred is returned in
InTransferLength. For write and bi-directional
commands, OutTransferLength bytes were transferred
by OutDataBuffer. See HostAdapterStatus,
TargetStatus, and in that order for additional
status information.
----^----
The spec bug (IMO) is the penultimate sentence of the
"EFI_BAD_BUFFER_SIZE" documentation -- "were transferred" seems to make
no sense, given that "The SCSI Request Packet was not executed".
Regarding why VirtioScsiDxe sets HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK when reporting an (otherwise
successful) short transfer: I can list two reasons.
(1) Paolo had reviewed this code at least on two separate occasions --
once in October 2012 and another time in September 2015.
(2) To me, EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN seemed
much closer to VIRTIO_SCSI_S_OVERRUN, which -- per virtio spec -- stands
for something very different:
VIRTIO_SCSI_S_OVERRUN if the content of the CDB (such as the
allocation length, parameter length or transfer
size) requires more data than is available in
the datain and dataout buffers.
Given that this part of VirtioScsiDxe has not caused any problems in the
past 7.5 years (it goes back to commit 37078a63b1911), i.e. setting
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK on a partial (but otherwise OK)
transfer, I would like to change it (to what?) only if:
- Paolo recommends that too,
- there is an actual industry standard or spec that indicates the
current host status is wrong.
To summarize my (updated) opinion on your handling of
PvScsiBtStatDatarun / PvScsiBtStatDataUnderrun:
- I don't know what the HostAdapterStatus should be (on output), or
whether you should report EFI_SUCCESS vs. EFI_DEVICE_ERROR. (If those
PVSCSI response codes stand for "normal, just short, transfer", then I
would suggest to follow VirtioScsiDxe's handling -- but I don't insist
at all.)
- Either way, I am quite convinced that returning EFI_BAD_BUFFER_SIZE is
wrong, because the packet *was* submitted to the device.
> In contrast to old header file, ScsiPassThru.h, which for
> EFI_BAD_BUFFER_SIZE specifies explicitly:
>
> @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
> but the
> entire DataBuffer could not be
> transferred.
> The actual number of bytes
> transferred is returned
> in TransferLength. See
> HostAdapterStatus,
> TargetStatus, SenseDataLength, and
> SenseData in
> that order for additional status
> information.
>
> In addition, looking at EDK2 iSCSI ScsiPassThru driver
> (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use
> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
> (Which matches the new header file ScsiPassThruExt.h and not the old
> ScsiPassThru.h).
> If you would have a look at how IScsiOnScsiRspRcvd() handles
> SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW bits,
> it seems to always
> set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating
> Packet->InTransferLength / Packet->OutTransferLength.
Great example!
And yes, I do feel that returning EFI_BAD_BUFFER_SIZE, after updating
InTransferLength / OutTransferLength from a field called
"ResidualCount", is wrong. "ResidualCount" is coming back in a response
structure, and its name ("residual") indicates the device transfer was
actually initiated on the remote end.
To me EFI_BAD_BUFFER_SIZE just has a different "spirit". It generally
means "I couldn't begin to act upon the request due to the wrong buffer
size. Adjust the buffer size, and try again".
A partial / short transfer is different from that.
> What do you think? Do you have any further insights? This is quite
> confusing to me.
I agree that it's very confusing.
I'm not sure if this is covered by the UEFI SCT (self conformance test).
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices()
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (14 preceding siblings ...)
2020-03-25 16:10 ` [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response Liran Alon
@ 2020-03-25 16:10 ` Liran Alon
2020-03-25 16:10 ` [PATCH v2 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses Liran Alon
16 siblings, 0 replies; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:10 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 de4122e39a81..42a18494efb7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -1225,6 +1225,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. 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.
+ //
+ PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
+}
+
//
// Driver Binding
//
@@ -1318,6 +1343,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
//
@@ -1329,11 +1365,14 @@ PvScsiDriverBindingStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto UninitDev;
+ goto CloseExitBoot;
}
return EFI_SUCCESS;
+CloseExitBoot:
+ gBS->CloseEvent (Dev->ExitBoot);
+
UninitDev:
PvScsiUninit (Dev);
@@ -1388,6 +1427,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 08e876b75930..e68a7dedf71f 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -41,6 +41,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] 41+ messages in thread* [PATCH v2 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses
2020-03-25 16:09 [PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller Liran Alon
` (15 preceding siblings ...)
2020-03-25 16:10 ` [PATCH v2 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices() Liran Alon
@ 2020-03-25 16:10 ` Liran Alon
2020-03-26 22:29 ` [edk2-devel] " Laszlo Ersek
16 siblings, 1 reply; 41+ messages in thread
From: Liran Alon @ 2020-03-25 16:10 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
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 42a18494efb7..3aea5c7f8eda 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -836,6 +836,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] 41+ messages in thread* Re: [edk2-devel] [PATCH v2 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses
2020-03-25 16:10 ` [PATCH v2 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses Liran Alon
@ 2020-03-26 22:29 ` Laszlo Ersek
0 siblings, 0 replies; 41+ messages in thread
From: Laszlo Ersek @ 2020-03-26 22:29 UTC (permalink / raw)
To: devel, liran.alon
Cc: nikita.leshchenko, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/25/20 17:10, Liran Alon wrote:
> Enable PCI dual-address cycle attribute to signal device
> supports 64-bit DMA addresses.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> 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 42a18494efb7..3aea5c7f8eda 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -836,6 +836,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;
> }
>
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread