* [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers
@ 2020-04-14 17:38 Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
` (12 more replies)
0 siblings, 13 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
This series adds driver support for:
- LSI53C1030
- SAS1068
- SAS1068E
These controllers are widely supported by QEMU, VirtualBox and VMWare.
This work is part of the more general agenda of enhancing OVMF boot
device support to have feature parity with SeaBIOS.
I pushed a copy of these patches to
https://github.com/nikital/edk2/tree/mptscsi_v4
Previous versions:
https://github.com/nikital/edk2/tree/mptscsi_v3
https://github.com/nikital/edk2/tree/mptscsi (v2)
v3->v4:
- Add ExitBootServices
- Rework error handling in PassThru
- SPDX license
- Made compilation conditional
- Squash GetTargetLun and BuildDevicePath commits
- Added #include <Uefi/UefiSpec.h>
- Use PCI_BAR_IDX0
- Code convention improvements
v2->v3:
- Change error handling style
- Add comments about target size and zero unused target bytes
- Remove internal Reviewed-by
- Fix problems reported by PatchCheck.py
- Use SetupGit.py
v1->v2:
- Map() DMAed buffers
- Fixed various code convention issues
- Newer debug macros
- Updated INF version
Thanks,
Nikita
Nikita Leshenko (13):
OvmfPkg/MptScsiDxe: Create empty driver
OvmfPkg/MptScsiDxe: Install DriverBinding Protocol
OvmfPkg/MptScsiDxe: Report name of driver
OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi
OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
OvmfPkg/MptScsiDxe: Report one Target and one LUN
OvmfPkg/MptScsiDxe: Build and decode DevicePath
OvmfPkg/MptScsiDxe: Open PciIo protocol for later use
OvmfPkg/MptScsiDxe: Set and restore PCI attributes
OvmfPkg/MptScsiDxe: Initialize hardware
OvmfPkg/MptScsiDxe: Implement the PassThru method
OvmfPkg/MptScsiDxe: Report multiple targets
OvmfPkg/MptScsiDxe: Reset device on ExitBootServices()
Maintainers.txt | 3 +-
.../Include/IndustryStandard/FusionMptScsi.h | 163 +++
OvmfPkg/MptScsiDxe/MptScsi.c | 1115 +++++++++++++++++
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 40 +
OvmfPkg/OvmfPkg.dec | 7 +
OvmfPkg/OvmfPkgIa32.dsc | 4 +
OvmfPkg/OvmfPkgIa32.fdf | 3 +
OvmfPkg/OvmfPkgIa32X64.dsc | 4 +
OvmfPkg/OvmfPkgIa32X64.fdf | 3 +
OvmfPkg/OvmfPkgX64.dsc | 4 +
OvmfPkg/OvmfPkgX64.fdf | 3 +
11 files changed, 1348 insertions(+), 1 deletion(-)
create mode 100644 OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
create mode 100644 OvmfPkg/MptScsiDxe/MptScsi.c
create mode 100644 OvmfPkg/MptScsiDxe/MptScsiDxe.inf
--
2.20.1
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 01/13] OvmfPkg/MptScsiDxe: Create empty driver
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-15 6:31 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol Nikita Leshenko
` (11 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
In preparation for implementing LSI Fusion MPT SCSI devices, create a
basic scaffolding for a driver.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
Maintainers.txt | 3 ++-
OvmfPkg/MptScsiDxe/MptScsi.c | 26 ++++++++++++++++++++++++++
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 26 ++++++++++++++++++++++++++
OvmfPkg/OvmfPkgIa32.dsc | 4 ++++
OvmfPkg/OvmfPkgIa32.fdf | 3 +++
OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
OvmfPkg/OvmfPkgIa32X64.fdf | 3 +++
OvmfPkg/OvmfPkgX64.dsc | 4 ++++
OvmfPkg/OvmfPkgX64.fdf | 3 +++
9 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 OvmfPkg/MptScsiDxe/MptScsi.c
create mode 100644 OvmfPkg/MptScsiDxe/MptScsiDxe.inf
diff --git a/Maintainers.txt b/Maintainers.txt
index 1733225722b6..01b5b8188158 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -435,8 +435,9 @@ OvmfPkg: CSM modules
F: OvmfPkg/Csm/
R: David Woodhouse <dwmw2@infradead.org>
-OvmfPkg: PVSCSI driver
+OvmfPkg: PVSCSI and MptScsi driver
F: OvmfPkg/PvScsiDxe/
+F: OvmfPkg/MptScsiDxe/
R: Liran Alon <liran.alon@oracle.com>
R: Nikita Leshenko <nikita.leshchenko@oracle.com>
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
new file mode 100644
index 000000000000..c6c8142dfde6
--- /dev/null
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -0,0 +1,26 @@
+/** @file
+
+ This driver produces Extended SCSI Pass Thru Protocol instances for
+ LSI Fusion MPT SCSI 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
+MptScsiEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ return EFI_UNSUPPORTED;
+}
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
new file mode 100644
index 000000000000..b4006a7c2d97
--- /dev/null
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -0,0 +1,26 @@
+## @file
+# This driver produces Extended SCSI Pass Thru Protocol instances for
+# LSI Fusion MPT SCSI devices.
+#
+# Copyright (C) 2020, Oracle and/or its affiliates.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 1.29
+ BASE_NAME = MptScsiDxe
+ FILE_GUID = 2B3DB5DD-B315-4961-8454-0AFF3C811B19
+ MODULE_TYPE = UEFI_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = MptScsiEntryPoint
+
+[Sources]
+ MptScsi.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ UefiDriverEntryPoint
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index cbc5f0e583bc..158a5e9f39bd 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -49,6 +49,7 @@ [Defines]
# Device drivers
#
DEFINE PVSCSI_ENABLE = TRUE
+ DEFINE MPT_SCSI_ENABLE = TRUE
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -744,6 +745,9 @@ [Components]
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
!if $(PVSCSI_ENABLE) == TRUE
OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
+!if $(MPT_SCSI_ENABLE) == TRUE
+ OvmfPkg/MptScsiDxe/MptScsiDxe.inf
!endif
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 8e43f4264ecc..fd81b6fa8bed 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -233,6 +233,9 @@ [FV.DXEFV]
!if $(PVSCSI_ENABLE) == TRUE
INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
!endif
+!if $(MPT_SCSI_ENABLE) == TRUE
+INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+!endif
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 6d69cc6cb56f..a6c5a1d9d050 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -48,6 +48,7 @@ [Defines]
# Device drivers
#
DEFINE PVSCSI_ENABLE = TRUE
+ DEFINE MPT_SCSI_ENABLE = TRUE
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -756,6 +757,9 @@ [Components.X64]
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
!if $(PVSCSI_ENABLE) == TRUE
OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
+!if $(MPT_SCSI_ENABLE) == TRUE
+ OvmfPkg/MptScsiDxe/MptScsiDxe.inf
!endif
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 25af9fbed48a..f71134a65931 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -234,6 +234,9 @@ [FV.DXEFV]
!if $(PVSCSI_ENABLE) == TRUE
INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
!endif
+!if $(MPT_SCSI_ENABLE) == TRUE
+INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+!endif
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 5ad4f461ce52..9aa8dd9e5fe1 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -48,6 +48,7 @@ [Defines]
# Device drivers
#
DEFINE PVSCSI_ENABLE = TRUE
+ DEFINE MPT_SCSI_ENABLE = TRUE
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -754,6 +755,9 @@ [Components]
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
!if $(PVSCSI_ENABLE) == TRUE
OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
+!if $(MPT_SCSI_ENABLE) == TRUE
+ OvmfPkg/MptScsiDxe/MptScsiDxe.inf
!endif
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 25af9fbed48a..f71134a65931 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -234,6 +234,9 @@ [FV.DXEFV]
!if $(PVSCSI_ENABLE) == TRUE
INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
!endif
+!if $(MPT_SCSI_ENABLE) == TRUE
+INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+!endif
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 03/13] OvmfPkg/MptScsiDxe: Report name of driver Nikita Leshenko
` (10 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
In order to probe and connect to the MptScsi device we need this
protocol. Currently it does nothing.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 67 ++++++++++++++++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
2 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index c6c8142dfde6..581d3909b84d 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -9,8 +9,66 @@
**/
+#include <Library/UefiLib.h>
#include <Uefi/UefiSpec.h>
+//
+// Higher versions will be used before lower, 0x10-0xffffffef is the version
+// range for IVH (Indie Hardware Vendors)
+//
+#define MPT_SCSI_BINDING_VERSION 0x10
+
+//
+// Driver Binding
+//
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiControllerSupported (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE ControllerHandle,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiControllerStart (
+ IN EFI_DRIVER_BINDING_PROTOCOL *This,
+ IN EFI_HANDLE ControllerHandle,
+ IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiControllerStop (
+ 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 mMptScsiDriverBinding = {
+ &MptScsiControllerSupported,
+ &MptScsiControllerStart,
+ &MptScsiControllerStop,
+ MPT_SCSI_BINDING_VERSION,
+ NULL, // ImageHandle, filled by EfiLibInstallDriverBindingComponentName2
+ NULL, // DriverBindingHandle, filled as well
+};
+
//
// Entry Point
//
@@ -22,5 +80,12 @@ MptScsiEntryPoint (
IN EFI_SYSTEM_TABLE *SystemTable
)
{
- return EFI_UNSUPPORTED;
+ return EfiLibInstallDriverBindingComponentName2 (
+ ImageHandle,
+ SystemTable,
+ &mMptScsiDriverBinding,
+ ImageHandle, // The handle to install onto
+ NULL, // TODO Component name
+ NULL // TODO Component name
+ );
}
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index b4006a7c2d97..53585068684f 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -24,3 +24,4 @@ [Packages]
[LibraryClasses]
UefiDriverEntryPoint
+ UefiLib
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 03/13] OvmfPkg/MptScsiDxe: Report name of driver
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi Nikita Leshenko
` (9 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel, Jaben Carsey
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=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 61 ++++++++++++++++++++++++++++++++++--
1 file changed, 59 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 581d3909b84d..64949a809022 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -69,6 +69,63 @@ EFI_DRIVER_BINDING_PROTOCOL mMptScsiDriverBinding = {
NULL, // DriverBindingHandle, filled as well
};
+//
+// Component Name
+//
+
+STATIC
+EFI_UNICODE_STRING_TABLE mDriverNameTable[] = {
+ { "eng;en", L"LSI Fusion MPT SCSI Driver" },
+ { NULL, NULL }
+};
+
+STATIC
+EFI_COMPONENT_NAME_PROTOCOL mComponentName;
+
+EFI_STATUS
+EFIAPI
+MptScsiGetDriverName (
+ IN EFI_COMPONENT_NAME_PROTOCOL *This,
+ IN CHAR8 *Language,
+ OUT CHAR16 **DriverName
+ )
+{
+ return LookupUnicodeString2 (
+ Language,
+ This->SupportedLanguages,
+ mDriverNameTable,
+ DriverName,
+ (BOOLEAN)(This == &mComponentName) // Iso639Language
+ );
+}
+
+EFI_STATUS
+EFIAPI
+MptScsiGetDeviceName (
+ 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 = {
+ &MptScsiGetDriverName,
+ &MptScsiGetDeviceName,
+ "eng" // SupportedLanguages, ISO 639-2 language codes
+};
+
+STATIC
+EFI_COMPONENT_NAME2_PROTOCOL mComponentName2 = {
+ (EFI_COMPONENT_NAME2_GET_DRIVER_NAME) &MptScsiGetDriverName,
+ (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) &MptScsiGetDeviceName,
+ "en" // SupportedLanguages, RFC 4646 language codes
+};
+
//
// Entry Point
//
@@ -85,7 +142,7 @@ MptScsiEntryPoint (
SystemTable,
&mMptScsiDriverBinding,
ImageHandle, // The handle to install onto
- NULL, // TODO Component name
- NULL // TODO Component name
+ &mComponentName,
+ &mComponentName2
);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (2 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 03/13] OvmfPkg/MptScsiDxe: Report name of driver Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
` (8 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
The MptScsiControllerSupported function is called on handles passed in
by the ConnectController() boot service and if the handle is the
lsi53c1030 controller the function would return success. A successful
return value will attach our driver to the device.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
.../Include/IndustryStandard/FusionMptScsi.h | 23 +++++++++
OvmfPkg/MptScsiDxe/MptScsi.c | 49 ++++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 5 ++
3 files changed, 76 insertions(+), 1 deletion(-)
create mode 100644 OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
new file mode 100644
index 000000000000..df9bdc2f0348
--- /dev/null
+++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
@@ -0,0 +1,23 @@
+/** @file
+
+ Macros and type definitions for LSI Fusion MPT SCSI devices.
+
+ Copyright (C) 2020, Oracle and/or its affiliates.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __FUSION_MPT_SCSI_H__
+#define __FUSION_MPT_SCSI_H__
+
+//
+// Device offsets and constants
+//
+
+#define LSI_LOGIC_PCI_VENDOR_ID 0x1000
+#define LSI_53C1030_PCI_DEVICE_ID 0x0030
+#define LSI_SAS1068_PCI_DEVICE_ID 0x0054
+#define LSI_SAS1068E_PCI_DEVICE_ID 0x0058
+
+#endif // __FUSION_MPT_SCSI_H__
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 64949a809022..4e2f8f2296fb 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -9,7 +9,11 @@
**/
+#include <IndustryStandard/FusionMptScsi.h>
+#include <IndustryStandard/Pci.h>
+#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
+#include <Protocol/PciIo.h>
#include <Uefi/UefiSpec.h>
//
@@ -31,7 +35,50 @@ MptScsiControllerSupported (
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 == LSI_LOGIC_PCI_VENDOR_ID &&
+ (Pci.Hdr.DeviceId == LSI_53C1030_PCI_DEVICE_ID ||
+ Pci.Hdr.DeviceId == LSI_SAS1068_PCI_DEVICE_ID ||
+ Pci.Hdr.DeviceId == LSI_SAS1068E_PCI_DEVICE_ID)) {
+ Status = EFI_SUCCESS;
+ } else {
+ Status = EFI_UNSUPPORTED;
+ }
+
+Done:
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ControllerHandle
+ );
+ return Status;
}
STATIC
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 53585068684f..414b96e5a248 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -21,7 +21,12 @@ [Sources]
[Packages]
MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
+
+[Protocols]
+ gEfiPciIoProtocolGuid ## TO_START
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (3 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-15 6:54 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN Nikita Leshenko
` (7 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
Support dynamic insertion and removal of the protocol
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 181 +++++++++++++++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 5 +-
2 files changed, 183 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 4e2f8f2296fb..40d392c2346f 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -11,9 +11,12 @@
#include <IndustryStandard/FusionMptScsi.h>
#include <IndustryStandard/Pci.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Protocol/PciIo.h>
+#include <Protocol/ScsiPassThruExt.h>
#include <Uefi/UefiSpec.h>
//
@@ -22,6 +25,109 @@
//
#define MPT_SCSI_BINDING_VERSION 0x10
+//
+// Runtime Structures
+//
+
+#define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
+typedef struct {
+ UINT32 Signature;
+ EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
+ EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
+} MPT_SCSI_DEV;
+
+#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
+ CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
+
+//
+// Ext SCSI Pass Thru
+//
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiPassThru (
+ 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
+MptScsiGetNextTargetLun (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN OUT UINT8 **Target,
+ IN OUT UINT64 *Lun
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiGetNextTarget (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN OUT UINT8 **Target
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiBuildDevicePath (
+ 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
+MptScsiGetTargetLun (
+ 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
+MptScsiResetChannel (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiResetTargetLun (
+ IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
+ IN UINT8 *Target,
+ IN UINT64 Lun
+ )
+{
+ return EFI_UNSUPPORTED;
+}
+
//
// Driver Binding
//
@@ -90,7 +196,49 @@ MptScsiControllerStart (
IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
)
{
- return EFI_UNSUPPORTED;
+ EFI_STATUS Status;
+ MPT_SCSI_DEV *Dev;
+
+ Dev = AllocateZeroPool (sizeof (*Dev));
+ if (Dev == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
+
+ //
+ // Host adapter channel, doesn't exist
+ //
+ Dev->PassThruMode.AdapterId = MAX_UINT32;
+ Dev->PassThruMode.Attributes =
+ EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL |
+ EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
+
+ Dev->PassThru.Mode = &Dev->PassThruMode;
+ Dev->PassThru.PassThru = &MptScsiPassThru;
+ Dev->PassThru.GetNextTargetLun = &MptScsiGetNextTargetLun;
+ Dev->PassThru.BuildDevicePath = &MptScsiBuildDevicePath;
+ Dev->PassThru.GetTargetLun = &MptScsiGetTargetLun;
+ Dev->PassThru.ResetChannel = &MptScsiResetChannel;
+ Dev->PassThru.ResetTargetLun = &MptScsiResetTargetLun;
+ Dev->PassThru.GetNextTarget = &MptScsiGetNextTarget;
+
+ Status = gBS->InstallProtocolInterface (
+ &ControllerHandle,
+ &gEfiExtScsiPassThruProtocolGuid,
+ EFI_NATIVE_INTERFACE,
+ &Dev->PassThru
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreePool;
+ }
+
+ return EFI_SUCCESS;
+
+FreePool:
+ FreePool (Dev);
+
+ return Status;
}
STATIC
@@ -103,7 +251,36 @@ MptScsiControllerStop (
IN EFI_HANDLE *ChildHandleBuffer
)
{
- return EFI_UNSUPPORTED;
+ EFI_STATUS Status;
+ EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru;
+ MPT_SCSI_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 = MPT_SCSI_FROM_PASS_THRU (PassThru);
+
+ Status = gBS->UninstallProtocolInterface (
+ ControllerHandle,
+ &gEfiExtScsiPassThruProtocolGuid,
+ &Dev->PassThru
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ FreePool (Dev);
+
+ return Status;
}
STATIC
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 414b96e5a248..5bdbc63f3ac6 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -24,9 +24,12 @@ [Packages]
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ DebugLib
+ MemoryAllocationLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiLib
[Protocols]
- gEfiPciIoProtocolGuid ## TO_START
+ gEfiPciIoProtocolGuid ## TO_START
+ gEfiExtScsiPassThruProtocolGuid ## BY_START
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (4 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 07/13] OvmfPkg/MptScsiDxe: Build and decode DevicePath Nikita Leshenko
` (6 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
Support for multiple targets will be implemented in a later commit in
this series.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 40 +++++++++++++++++++++++++++++--
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 40d392c2346f..30fb084379db 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -11,6 +11,7 @@
#include <IndustryStandard/FusionMptScsi.h>
#include <IndustryStandard/Pci.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
@@ -57,6 +58,22 @@ MptScsiPassThru (
return EFI_UNSUPPORTED;
}
+STATIC
+BOOLEAN
+IsTargetInitialized (
+ IN UINT8 *Target
+ )
+{
+ UINTN Idx;
+
+ for (Idx = 0; Idx < TARGET_MAX_BYTES; ++Idx) {
+ if (Target[Idx] != 0xFF) {
+ return TRUE;
+ }
+ }
+ return FALSE;
+}
+
STATIC
EFI_STATUS
EFIAPI
@@ -66,7 +83,17 @@ MptScsiGetNextTargetLun (
IN OUT UINT64 *Lun
)
{
- return EFI_UNSUPPORTED;
+ //
+ // Currently support only target 0 LUN 0, so hardcode it
+ //
+ if (!IsTargetInitialized (*Target)) {
+ ZeroMem (*Target, TARGET_MAX_BYTES);
+ *Lun = 0;
+ } else {
+ return EFI_NOT_FOUND;
+ }
+
+ return EFI_SUCCESS;
}
STATIC
@@ -77,7 +104,16 @@ MptScsiGetNextTarget (
IN OUT UINT8 **Target
)
{
- return EFI_UNSUPPORTED;
+ //
+ // Currently support only target 0 LUN 0, so hardcode it
+ //
+ if (!IsTargetInitialized (*Target)) {
+ ZeroMem (*Target, TARGET_MAX_BYTES);
+ } else {
+ return EFI_NOT_FOUND;
+ }
+
+ return EFI_SUCCESS;
}
STATIC
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 5bdbc63f3ac6..809f12173bb8 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -24,6 +24,7 @@ [Packages]
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseMemoryLib
DebugLib
MemoryAllocationLib
UefiBootServicesTableLib
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 07/13] OvmfPkg/MptScsiDxe: Build and decode DevicePath
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (5 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-15 12:03 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 08/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
` (5 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
Used to identify the individual disks in the hardware tree.
Currently we accept only Pun=0 and Lun=0, but we will relax this in a
later patch.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 57 ++++++++++++++++++++++++++++++++++--
1 file changed, 55 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 30fb084379db..69ab947c0da2 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -126,7 +126,34 @@ MptScsiBuildDevicePath (
IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
)
{
- return EFI_UNSUPPORTED;
+ SCSI_DEVICE_PATH *ScsiDevicePath;
+
+ if (DevicePath == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // This device support 256 targets only, so it's enough to dereference
+ // the LSB of Target.
+ //
+ if (*Target > 0 || Lun > 0) {
+ return EFI_NOT_FOUND;
+ }
+
+ ScsiDevicePath = AllocateZeroPool (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 = *Target;
+ ScsiDevicePath->Lun = (UINT16)Lun;
+
+ *DevicePath = &ScsiDevicePath->Header;
+ return EFI_SUCCESS;
}
STATIC
@@ -139,7 +166,33 @@ MptScsiGetTargetLun (
OUT UINT64 *Lun
)
{
- return EFI_UNSUPPORTED;
+ SCSI_DEVICE_PATH *ScsiDevicePath;
+
+ 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;
+ if (ScsiDevicePath->Pun > 0 ||
+ ScsiDevicePath->Lun > 0) {
+ return EFI_NOT_FOUND;
+ }
+
+ ZeroMem (*Target, TARGET_MAX_BYTES);
+ //
+ // This device support 256 targets only, so it's enough to set the LSB
+ // of Target.
+ //
+ **Target = (UINT8)ScsiDevicePath->Pun;
+ *Lun = ScsiDevicePath->Lun;
+
+ return EFI_SUCCESS;
}
STATIC
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 08/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (6 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 07/13] OvmfPkg/MptScsiDxe: Build and decode DevicePath Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-16 8:05 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 09/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
` (4 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
This will give us an exclusive access to the PciIo of this device
after it was started and until is will be stopped.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 69ab947c0da2..275265774252 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -35,6 +35,7 @@ typedef struct {
UINT32 Signature;
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
+ EFI_PCI_IO_PROTOCOL *PciIo;
} MPT_SCSI_DEV;
#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
@@ -295,6 +296,18 @@ MptScsiControllerStart (
Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
+ Status = gBS->OpenProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ (VOID **)&Dev->PciIo,
+ This->DriverBindingHandle,
+ ControllerHandle,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreePool;
+ }
+
//
// Host adapter channel, doesn't exist
//
@@ -319,11 +332,19 @@ MptScsiControllerStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto FreePool;
+ goto CloseProtocol;
}
return EFI_SUCCESS;
+CloseProtocol:
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ControllerHandle
+ );
+
FreePool:
FreePool (Dev);
@@ -367,6 +388,13 @@ MptScsiControllerStop (
return Status;
}
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ControllerHandle
+ );
+
FreePool (Dev);
return Status;
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 09/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (7 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 08/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-16 8:11 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
` (3 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
Enable the IO Space and Bus Mastering and restore the original values
when the device is stopped. This is a standard procedure in PCI
drivers.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 42 +++++++++++++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 275265774252..4bfd03d2acb0 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -36,6 +36,7 @@ typedef struct {
EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
EFI_PCI_IO_PROTOCOL *PciIo;
+ UINT64 OriginalPciAttributes;
} MPT_SCSI_DEV;
#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
@@ -308,6 +309,30 @@ MptScsiControllerStart (
goto FreePool;
}
+ Status = Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationGet,
+ 0,
+ &Dev->OriginalPciAttributes
+ );
+ if (EFI_ERROR (Status)) {
+ goto CloseProtocol;
+ }
+
+ //
+ // Enable I/O Space & Bus-Mastering
+ //
+ Status = Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationEnable,
+ (EFI_PCI_IO_ATTRIBUTE_IO |
+ EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto CloseProtocol;
+ }
+
//
// Host adapter channel, doesn't exist
//
@@ -332,11 +357,19 @@ MptScsiControllerStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto CloseProtocol;
+ goto RestoreAttributes;
}
return EFI_SUCCESS;
+RestoreAttributes:
+ Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationEnable,
+ Dev->OriginalPciAttributes,
+ NULL
+ );
+
CloseProtocol:
gBS->CloseProtocol (
ControllerHandle,
@@ -388,6 +421,13 @@ MptScsiControllerStop (
return Status;
}
+ Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationEnable,
+ Dev->OriginalPciAttributes,
+ NULL
+ );
+
gBS->CloseProtocol (
ControllerHandle,
&gEfiPciIoProtocolGuid,
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (8 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 09/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-16 9:53 ` [edk2-devel] " Laszlo Ersek
2020-04-20 14:10 ` Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
` (2 subsequent siblings)
12 siblings, 2 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
Reset and send the IO controller initialization request. The reply is
read back to complete the doorbell function but it isn't useful to us
because it doesn't contain relevant data or status codes.
See "LSI53C1030 PCI-X to Dual Channel Ultra320 SCSI Multifunction
Controller" technical manual for more information.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
---
.../Include/IndustryStandard/FusionMptScsi.h | 123 +++++++++++++
OvmfPkg/MptScsiDxe/MptScsi.c | 173 +++++++++++++++++-
2 files changed, 295 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
index df9bdc2f0348..d00a9e6db0bf 100644
--- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
+++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
@@ -20,4 +20,127 @@
#define LSI_SAS1068_PCI_DEVICE_ID 0x0054
#define LSI_SAS1068E_PCI_DEVICE_ID 0x0058
+#define MPT_REG_DOORBELL 0x00
+#define MPT_REG_WRITE_SEQ 0x04
+#define MPT_REG_HOST_DIAG 0x08
+#define MPT_REG_TEST 0x0c
+#define MPT_REG_DIAG_DATA 0x10
+#define MPT_REG_DIAG_ADDR 0x14
+#define MPT_REG_ISTATUS 0x30
+#define MPT_REG_IMASK 0x34
+#define MPT_REG_REQ_Q 0x40
+#define MPT_REG_REP_Q 0x44
+
+#define MPT_DOORBELL_RESET 0x40
+#define MPT_DOORBELL_HANDSHAKE 0x42
+
+#define MPT_IMASK_DOORBELL 0x01
+#define MPT_IMASK_REPLY 0x08
+
+#define MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST 0x00
+#define MPT_MESSAGE_HDR_FUNCTION_IOC_INIT 0x02
+
+#define MPT_SG_ENTRY_TYPE_SIMPLE 0x01
+
+#define MPT_IOC_WHOINIT_ROM_BIOS 0x02
+
+//
+// Device structures
+//
+
+typedef union {
+#pragma pack (1)
+ struct {
+ UINT8 WhoInit;
+ UINT8 Reserved1;
+ UINT8 ChainOffset;
+ UINT8 Function;
+ UINT8 Flags;
+ UINT8 MaxDevices;
+ UINT8 MaxBuses;
+ UINT8 MessageFlags;
+ UINT32 MessageContext;
+ UINT16 ReplyFrameSize;
+ UINT16 Reserved2;
+ UINT32 HostMfaHighAddr;
+ UINT32 SenseBufferHighAddr;
+ } Data;
+#pragma pack ()
+ UINT64 Uint64; // 8 byte alignment required by HW
+} MPT_IO_CONTROLLER_INIT_REQUEST;
+
+#pragma pack (1)
+typedef struct {
+ UINT8 WhoInit;
+ UINT8 Reserved1;
+ UINT8 MessageLength;
+ UINT8 Function;
+ UINT8 Flags;
+ UINT8 MaxDevices;
+ UINT8 MaxBuses;
+ UINT8 MessageFlags;
+ UINT32 MessageContext;
+ UINT16 Reserved2;
+ UINT16 IOCStatus;
+ UINT32 IOCLogInfo;
+} MPT_IO_CONTROLLER_INIT_REPLY;
+
+typedef struct {
+ UINT8 TargetID;
+ UINT8 Bus;
+ UINT8 ChainOffset;
+ UINT8 Function;
+ UINT8 CDBLength;
+ UINT8 SenseBufferLength;
+ UINT8 Reserved;
+ UINT8 MessageFlags;
+ UINT32 MessageContext;
+ UINT8 LUN[8];
+ UINT32 Control;
+ UINT8 CDB[16];
+ UINT32 DataLength;
+ UINT32 SenseBufferLowAddress;
+} MPT_SCSI_IO_REQUEST;
+
+typedef struct {
+ UINT32 Length: 24;
+ UINT32 EndOfList: 1;
+ UINT32 Is64BitAddress: 1;
+ //
+ // True when the buffer contains data to be transfered. Otherwise it's the
+ // destination buffer
+ //
+ UINT32 BufferContainsData: 1;
+ UINT32 LocalAddress: 1;
+ UINT32 ElementType: 2;
+ UINT32 EndOfBuffer: 1;
+ UINT32 LastElement: 1;
+ UINT64 DataBufferAddress;
+} MPT_SG_ENTRY_SIMPLE;
+#pragma pack ()
+
+typedef union {
+#pragma pack (1)
+ struct {
+ UINT8 TargetID;
+ UINT8 Bus;
+ UINT8 MessageLength;
+ UINT8 Function;
+ UINT8 CDBLength;
+ UINT8 SenseBufferLength;
+ UINT8 Reserved;
+ UINT8 MessageFlags;
+ UINT32 MessageContext;
+ UINT8 SCSIStatus;
+ UINT8 SCSIState;
+ UINT16 IOCStatus;
+ UINT32 IOCLogInfo;
+ UINT32 TransferCount;
+ UINT32 SenseCount;
+ UINT32 ResponseInfo;
+ } Data;
+#pragma pack ()
+ UINT64 Uint64; // 8 byte alignment required by HW
+} MPT_SCSI_IO_ERROR_REPLY;
+
#endif // __FUSION_MPT_SCSI_H__
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 4bfd03d2acb0..9c3bdc430e1a 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -42,6 +42,167 @@ typedef struct {
#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
+//
+// Hardware functions
+//
+
+STATIC
+EFI_STATUS
+Out32 (
+ IN MPT_SCSI_DEV *Dev,
+ IN UINT32 Addr,
+ IN UINT32 Data
+ )
+{
+ return Dev->PciIo->Io.Write (
+ Dev->PciIo,
+ EfiPciIoWidthUint32,
+ PCI_BAR_IDX0,
+ Addr,
+ 1,
+ &Data
+ );
+}
+
+STATIC
+EFI_STATUS
+In32 (
+ IN MPT_SCSI_DEV *Dev,
+ IN UINT32 Addr,
+ OUT UINT32 *Data
+ )
+{
+ return Dev->PciIo->Io.Read (
+ Dev->PciIo,
+ EfiPciIoWidthUint32,
+ PCI_BAR_IDX0,
+ Addr,
+ 1,
+ Data
+ );
+}
+
+STATIC
+EFI_STATUS
+MptDoorbell (
+ IN MPT_SCSI_DEV *Dev,
+ IN UINT8 DoorbellFunc,
+ IN UINT8 DoorbellArg
+ )
+{
+ return Out32 (
+ Dev,
+ MPT_REG_DOORBELL,
+ (((UINT32)DoorbellFunc) << 24) | (DoorbellArg << 16)
+ );
+}
+
+STATIC
+EFI_STATUS
+MptScsiReset (
+ IN MPT_SCSI_DEV *Dev
+ )
+{
+ EFI_STATUS Status;
+
+ //
+ // Reset hardware
+ //
+ Status = MptDoorbell (Dev, MPT_DOORBELL_RESET, 0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ //
+ // Mask interrupts
+ //
+ Status = Out32 (Dev, MPT_REG_IMASK, MPT_IMASK_DOORBELL | MPT_IMASK_REPLY);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ //
+ // Clear interrupt status
+ //
+ Status = Out32 (Dev, MPT_REG_ISTATUS, 0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+MptScsiInit (
+ IN MPT_SCSI_DEV *Dev
+ )
+{
+ EFI_STATUS Status;
+ MPT_IO_CONTROLLER_INIT_REQUEST Req;
+ MPT_IO_CONTROLLER_INIT_REPLY Reply;
+ UINT8 *ReplyBytes;
+ UINT32 ReplyWord;
+
+ Status = MptScsiReset (Dev);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ ZeroMem (&Req, sizeof (Req));
+ ZeroMem (&Reply, sizeof (Reply));
+ Req.Data.WhoInit = MPT_IOC_WHOINIT_ROM_BIOS;
+ Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
+ Req.Data.MaxDevices = 1;
+ Req.Data.MaxBuses = 1;
+ Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
+
+ //
+ // Send controller init through doorbell
+ //
+ Status = MptDoorbell (
+ Dev,
+ MPT_DOORBELL_HANDSHAKE,
+ sizeof (Req) / sizeof (UINT32)
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = Dev->PciIo->Io.Write (
+ Dev->PciIo,
+ EfiPciIoWidthFifoUint32,
+ 0,
+ MPT_REG_DOORBELL,
+ sizeof (Req) / sizeof (UINT32),
+ &Req
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Read reply through doorbell
+ // Each 32bit (Dword) read produces 16bit (Word) of data
+ //
+ ReplyBytes = (UINT8 *)&Reply;
+ while (ReplyBytes != (UINT8 *)(&Reply + 1)) {
+ Status = In32 (Dev, MPT_REG_DOORBELL, &ReplyWord);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ CopyMem (ReplyBytes, &ReplyWord, sizeof (UINT16));
+ ReplyBytes += sizeof (UINT16);
+ }
+
+ //
+ // Clear interrupts generated by doorbell reply
+ //
+ Status = Out32 (Dev, MPT_REG_ISTATUS, 0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+}
+
//
// Ext SCSI Pass Thru
//
@@ -333,6 +494,11 @@ MptScsiControllerStart (
goto CloseProtocol;
}
+ Status = MptScsiInit (Dev);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Host adapter channel, doesn't exist
//
@@ -357,11 +523,14 @@ MptScsiControllerStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto RestoreAttributes;
+ goto UninitDev;
}
return EFI_SUCCESS;
+UninitDev:
+ MptScsiReset (Dev);
+
RestoreAttributes:
Dev->PciIo->Attributes (
Dev->PciIo,
@@ -421,6 +590,8 @@ MptScsiControllerStop (
return Status;
}
+ MptScsiReset (Dev);
+
Dev->PciIo->Attributes (
Dev->PciIo,
EfiPciIoAttributeOperationEnable,
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (9 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-20 17:30 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 12/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 13/13] OvmfPkg/MptScsiDxe: Reset device on ExitBootServices() Nikita Leshenko
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
Machines should be able to boot after this commit. Tested with different
Linux distributions (Ubuntu, CentOS) and different Windows
versions (Windows 7, Windows 10, Server 2016).
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
---
.../Include/IndustryStandard/FusionMptScsi.h | 19 +-
OvmfPkg/MptScsiDxe/MptScsi.c | 369 +++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 +
OvmfPkg/OvmfPkg.dec | 3 +
4 files changed, 390 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
index d00a9e6db0bf..4be36adedd8f 100644
--- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
+++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
@@ -44,6 +44,15 @@
#define MPT_IOC_WHOINIT_ROM_BIOS 0x02
+#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE (0x00 << 24)
+#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE (0x01 << 24)
+#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ (0x02 << 24)
+
+#define MPT_SCSI_IOCSTATUS_SUCCESS 0x0000
+#define MPT_SCSI_IOCSTATUS_DEVICE_NOT_THERE 0x0043
+#define MPT_SCSI_IOCSTATUS_DATA_OVERRUN 0x0044
+#define MPT_SCSI_IOCSTATUS_DATA_UNDERRUN 0x0045
+
//
// Device structures
//
@@ -141,6 +150,14 @@ typedef union {
} Data;
#pragma pack ()
UINT64 Uint64; // 8 byte alignment required by HW
-} MPT_SCSI_IO_ERROR_REPLY;
+} MPT_SCSI_IO_REPLY;
+
+typedef union {
+ struct {
+ MPT_SCSI_IO_REQUEST Header;
+ MPT_SG_ENTRY_SIMPLE Sg;
+ } Data;
+ UINT64 Uint64; // 8 byte alignment required by HW
+} MPT_SCSI_REQUEST_WITH_SG;
#endif // __FUSION_MPT_SCSI_H__
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 9c3bdc430e1a..fcdaa4c338a4 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -17,6 +17,7 @@
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Protocol/PciIo.h>
+#include <Protocol/PciRootBridgeIo.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Uefi/UefiSpec.h>
@@ -30,6 +31,13 @@
// Runtime Structures
//
+typedef struct {
+ MPT_SCSI_REQUEST_WITH_SG IoRequest;
+ MPT_SCSI_IO_REPLY IoReply;
+ UINT8 Sense[MAX_UINT8];
+ UINT8 Data[0x2000];
+} MPT_SCSI_DMA_BUFFER;
+
#define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
typedef struct {
UINT32 Signature;
@@ -37,11 +45,19 @@ typedef struct {
EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 OriginalPciAttributes;
+ UINT32 StallPerPollUsec;
+ MPT_SCSI_DMA_BUFFER *Dma;
+ EFI_PHYSICAL_ADDRESS DmaPhysical;
+ VOID *DmaMapping;
+ BOOLEAN IoReplyEnqueued;
} MPT_SCSI_DEV;
#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
+#define MPT_SCSI_DMA_ADDR(Dev, MemberName) \
+ (Dev->DmaPhysical + OFFSET_OF(MPT_SCSI_DMA_BUFFER, MemberName))
+
//
// Hardware functions
//
@@ -142,6 +158,8 @@ MptScsiInit (
UINT8 *ReplyBytes;
UINT32 ReplyWord;
+ Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
+
Status = MptScsiReset (Dev);
if (EFI_ERROR (Status)) {
return Status;
@@ -153,7 +171,7 @@ MptScsiInit (
Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
Req.Data.MaxDevices = 1;
Req.Data.MaxBuses = 1;
- Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
+ Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_REPLY);
//
// Send controller init through doorbell
@@ -203,6 +221,257 @@ MptScsiInit (
return EFI_SUCCESS;
}
+STATIC
+EFI_STATUS
+MptScsiPopulateRequest (
+ IN MPT_SCSI_DEV *Dev,
+ IN UINT8 Target,
+ IN UINT64 Lun,
+ IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
+ )
+{
+ MPT_SCSI_REQUEST_WITH_SG *Request;
+
+ Request = &Dev->Dma->IoRequest;
+
+ if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
+ (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
+ Packet->CdbLength > sizeof (Request->Data.Header.CDB)) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (Target > 0 || Lun > 0 ||
+ 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
+ )
+ )
+ ) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (Packet->InTransferLength > sizeof (Dev->Dma->Data)) {
+ Packet->InTransferLength = sizeof (Dev->Dma->Data);
+ return EFI_BAD_BUFFER_SIZE;
+ }
+ if (Packet->OutTransferLength > sizeof (Dev->Dma->Data)) {
+ Packet->OutTransferLength = sizeof (Dev->Dma->Data);
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ ZeroMem (Request, sizeof (*Request));
+ Request->Data.Header.TargetID = Target;
+ //
+ // It's 1 and not 0, for some reason...
+ //
+ Request->Data.Header.LUN[1] = Lun;
+ Request->Data.Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST;
+ Request->Data.Header.MessageContext = 1; // We handle one request at a time
+
+ Request->Data.Header.CDBLength = Packet->CdbLength;
+ CopyMem (Request->Data.Header.CDB, Packet->Cdb, Packet->CdbLength);
+
+ //
+ // SenseDataLength is UINT8, Sense[] is MAX_UINT8, so we can't overflow
+ //
+ ZeroMem (&Dev->Dma->Sense, Packet->SenseDataLength);
+ Request->Data.Header.SenseBufferLength = Packet->SenseDataLength;
+ Request->Data.Header.SenseBufferLowAddress = MPT_SCSI_DMA_ADDR (Dev, Sense);
+
+ Request->Data.Sg.EndOfList = 1;
+ Request->Data.Sg.EndOfBuffer = 1;
+ Request->Data.Sg.LastElement = 1;
+ Request->Data.Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE;
+ Request->Data.Sg.DataBufferAddress = MPT_SCSI_DMA_ADDR (Dev, Data);
+
+ Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE;
+ switch (Packet->DataDirection)
+ {
+ case EFI_EXT_SCSI_DATA_DIRECTION_READ:
+ if (Packet->InTransferLength == 0) {
+ break;
+ }
+ Request->Data.Header.DataLength = Packet->InTransferLength;
+ Request->Data.Sg.Length = Packet->InTransferLength;
+ Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
+ break;
+ case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
+ if (Packet->OutTransferLength == 0) {
+ break;
+ }
+ Request->Data.Header.DataLength = Packet->OutTransferLength;
+ Request->Data.Sg.Length = Packet->OutTransferLength;
+ Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
+
+ CopyMem (Dev->Dma->Data, Packet->OutDataBuffer, Packet->OutTransferLength);
+ Request->Data.Sg.BufferContainsData = 1;
+ break;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+MptScsiSendRequest (
+ IN MPT_SCSI_DEV *Dev,
+ IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
+ )
+{
+ EFI_STATUS Status;
+
+ if (!Dev->IoReplyEnqueued) {
+ //
+ // Put one free reply frame on the reply queue, the hardware may use it to
+ // report an error to us.
+ //
+ Status = Out32 (Dev, MPT_REG_REP_Q, MPT_SCSI_DMA_ADDR (Dev, IoReply));
+ if (EFI_ERROR (Status)) {
+ return EFI_DEVICE_ERROR;
+ }
+ Dev->IoReplyEnqueued = TRUE;
+ }
+
+ Status = Out32 (Dev, MPT_REG_REQ_Q, MPT_SCSI_DMA_ADDR (Dev, IoRequest));
+ if (EFI_ERROR (Status)) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+MptScsiGetReply (
+ IN MPT_SCSI_DEV *Dev,
+ OUT UINT32 *Reply
+ )
+{
+ EFI_STATUS Status;
+ UINT32 Istatus;
+ UINT32 EmptyReply;
+
+ //
+ // Timeouts are not supported for
+ // EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() in this implementation.
+ //
+ for (;;) {
+ Status = In32 (Dev, MPT_REG_ISTATUS, &Istatus);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Interrupt raised
+ //
+ if (Istatus & MPT_IMASK_REPLY) {
+ break;
+ }
+
+ gBS->Stall (Dev->StallPerPollUsec);
+ }
+
+ Status = In32 (Dev, MPT_REG_REP_Q, Reply);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // The driver is supposed to fetch replies until 0xffffffff is returned, which
+ // will reset the interrupt status. We put only one request, so we expect the
+ // next read reply to be the last.
+ //
+ Status = In32 (Dev, MPT_REG_REP_Q, &EmptyReply);
+ if (EFI_ERROR (Status) || EmptyReply != MAX_UINT32) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+MptScsiHandleReply (
+ IN MPT_SCSI_DEV *Dev,
+ IN UINT32 Reply,
+ OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
+ )
+{
+ CopyMem (Packet->SenseData, Dev->Dma->Sense, Packet->SenseDataLength);
+ if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+ CopyMem (Packet->InDataBuffer, Dev->Dma->Data, Packet->InTransferLength);
+ }
+
+ if (Reply == Dev->Dma->IoRequest.Data.Header.MessageContext) {
+ //
+ // This is a turbo reply, everything is good
+ //
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+ Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
+
+ } else if (Reply & (1 << 31)) {
+ DEBUG ((DEBUG_ERROR, "%a: request failed\n", __FUNCTION__));
+ //
+ // When reply MSB is set, we got a full reply. Since we submitted only one
+ // reply frame, we know it's IoReply.
+ //
+ Dev->IoReplyEnqueued = FALSE;
+
+ Packet->TargetStatus = Dev->Dma->IoReply.Data.SCSIStatus;
+ Packet->SenseDataLength = Dev->Dma->IoReply.Data.SenseCount;
+
+ switch (Dev->Dma->IoReply.Data.IOCStatus) {
+ case MPT_SCSI_IOCSTATUS_SUCCESS:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+ break;
+ case MPT_SCSI_IOCSTATUS_DEVICE_NOT_THERE:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
+ return EFI_TIMEOUT;
+ case MPT_SCSI_IOCSTATUS_DATA_UNDERRUN:
+ if (Packet->TargetStatus == EFI_EXT_SCSI_STATUS_TARGET_GOOD) {
+ if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+ Packet->InTransferLength = Dev->Dma->IoReply.Data.TransferCount;
+ } else {
+ Packet->OutTransferLength = Dev->Dma->IoReply.Data.TransferCount;
+ }
+ }
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+ return EFI_SUCCESS;
+ case MPT_SCSI_IOCSTATUS_DATA_OVERRUN:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+ return EFI_SUCCESS;
+ default:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+ return EFI_DEVICE_ERROR;
+ }
+
+ } else {
+ DEBUG ((DEBUG_ERROR, "%a: unexpected reply\n", __FUNCTION__));
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+ return EFI_DEVICE_ERROR;
+ }
+
+ return EFI_SUCCESS;
+}
+
//
// Ext SCSI Pass Thru
//
@@ -218,7 +487,44 @@ MptScsiPassThru (
IN EFI_EVENT Event OPTIONAL
)
{
- return EFI_UNSUPPORTED;
+ EFI_STATUS Status;
+ MPT_SCSI_DEV *Dev;
+ UINT32 Reply;
+
+ Dev = MPT_SCSI_FROM_PASS_THRU (This);
+ Status = MptScsiPopulateRequest (Dev, *Target, Lun, Packet);
+ if (EFI_ERROR (Status)) {
+ //
+ // MptScsiPopulateRequest modified packet according to the error
+ //
+ return Status;
+ }
+
+ Status = MptScsiSendRequest (Dev, Packet);
+ if (EFI_ERROR (Status)) {
+ goto Fatal;
+ }
+
+ Status = MptScsiGetReply (Dev, &Reply);
+ if (EFI_ERROR (Status)) {
+ goto Fatal;
+ }
+
+ return MptScsiHandleReply (Dev, Reply, Packet);
+
+Fatal:
+ //
+ // We erred in the middle of a transaction, a very serious problem has occured
+ // and it's not clear if it's possible to recover without leaving the hardware
+ // in an inconsistent state. Perhaps we would want to reset the device...
+ //
+ DEBUG ((DEBUG_ERROR, "%a: fatal error in scsi request\n", __FUNCTION__));
+ Packet->InTransferLength = 0;
+ Packet->OutTransferLength = 0;
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+ Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTED;
+ Packet->SenseDataLength = 0;
+ return EFI_DEVICE_ERROR;
}
STATIC
@@ -450,6 +756,7 @@ MptScsiControllerStart (
{
EFI_STATUS Status;
MPT_SCSI_DEV *Dev;
+ UINTN BytesMapped;
Dev = AllocateZeroPool (sizeof (*Dev));
if (Dev == NULL) {
@@ -494,9 +801,42 @@ MptScsiControllerStart (
goto CloseProtocol;
}
+ //
+ // Create buffers for data transfer
+ //
+ Status = Dev->PciIo->AllocateBuffer (
+ Dev->PciIo,
+ AllocateAnyPages,
+ EfiBootServicesData,
+ EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
+ (VOID **)&Dev->Dma,
+ EFI_PCI_ATTRIBUTE_MEMORY_CACHED
+ );
+ if (EFI_ERROR (Status)) {
+ goto RestoreAttributes;
+ }
+
+ BytesMapped = sizeof (*Dev->Dma);
+ Status = Dev->PciIo->Map (
+ Dev->PciIo,
+ EfiPciIoOperationBusMasterCommonBuffer,
+ Dev->Dma,
+ &BytesMapped,
+ &Dev->DmaPhysical,
+ &Dev->DmaMapping
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBuffer;
+ }
+
+ if (BytesMapped != sizeof (*Dev->Dma)) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Unmap;
+ }
+
Status = MptScsiInit (Dev);
if (EFI_ERROR (Status)) {
- goto RestorePciAttributes;
+ goto Unmap;
}
//
@@ -531,6 +871,18 @@ MptScsiControllerStart (
UninitDev:
MptScsiReset (Dev);
+Unmap:
+ Dev->PciIo->Unmap (
+ Dev->PciIo,
+ Dev->DmaMapping
+ );
+
+FreeBuffer:
+ Dev->PciIo->FreeBuffer (
+ Dev->PciIo,
+ EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
+ Dev->Dma
+ );
RestoreAttributes:
Dev->PciIo->Attributes (
Dev->PciIo,
@@ -592,6 +944,17 @@ MptScsiControllerStop (
MptScsiReset (Dev);
+ Dev->PciIo->Unmap (
+ Dev->PciIo,
+ Dev->DmaMapping
+ );
+
+ Dev->PciIo->FreeBuffer (
+ Dev->PciIo,
+ EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
+ Dev->Dma
+ );
+
Dev->PciIo->Attributes (
Dev->PciIo,
EfiPciIoAttributeOperationEnable,
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 809f12173bb8..ef1f6a5ebb3a 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -34,3 +34,6 @@ [LibraryClasses]
[Protocols]
gEfiPciIoProtocolGuid ## TO_START
gEfiExtScsiPassThruProtocolGuid ## BY_START
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 28030391cff2..7fa1581f2101 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -270,6 +270,9 @@ [PcdsFixedAtBuild]
## Number of page frames to use for storing grant table entries.
gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
+ ## Microseconds to stall between polling for MptScsi request result
+ gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x39
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 12/13] OvmfPkg/MptScsiDxe: Report multiple targets
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (10 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-20 18:31 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 13/13] OvmfPkg/MptScsiDxe: Reset device on ExitBootServices() Nikita Leshenko
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
The controller supports up to 8 targets (Not reported by the
controller, but based on the implementation of the virtual device),
report them in GetNextTarget and GetNextTargetLun. The firmware will
then try to communicate with them and create a block device for each
one that responds.
Support for multiple LUNs will be implemented in another series.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 38 ++++++++++++++++++++++++-------
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
OvmfPkg/OvmfPkg.dec | 4 ++++
3 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index fcdaa4c338a4..3ea08857df5d 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -46,6 +46,7 @@ typedef struct {
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 OriginalPciAttributes;
UINT32 StallPerPollUsec;
+ UINT8 MaxTarget;
MPT_SCSI_DMA_BUFFER *Dma;
EFI_PHYSICAL_ADDRESS DmaPhysical;
VOID *DmaMapping;
@@ -159,6 +160,7 @@ MptScsiInit (
UINT32 ReplyWord;
Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
+ Dev->MaxTarget = PcdGet8 (PcdMptScsiMaxTargetLimit);
Status = MptScsiReset (Dev);
if (EFI_ERROR (Status)) {
@@ -169,7 +171,7 @@ MptScsiInit (
ZeroMem (&Reply, sizeof (Reply));
Req.Data.WhoInit = MPT_IOC_WHOINIT_ROM_BIOS;
Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
- Req.Data.MaxDevices = 1;
+ Req.Data.MaxDevices = Dev->MaxTarget + 1;
Req.Data.MaxBuses = 1;
Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_REPLY);
@@ -240,7 +242,7 @@ MptScsiPopulateRequest (
return EFI_UNSUPPORTED;
}
- if (Target > 0 || Lun > 0 ||
+ if (Target > Dev->MaxTarget || Lun > 0 ||
Packet->DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
//
// Trying to receive, but destination pointer is NULL, or contradicting
@@ -552,12 +554,22 @@ MptScsiGetNextTargetLun (
IN OUT UINT64 *Lun
)
{
+ MPT_SCSI_DEV *Dev;
+
+ Dev = MPT_SCSI_FROM_PASS_THRU (This);
//
- // Currently support only target 0 LUN 0, so hardcode it
+ // Currently support only LUN 0, so hardcode it
//
if (!IsTargetInitialized (*Target)) {
ZeroMem (*Target, TARGET_MAX_BYTES);
*Lun = 0;
+ } else if (**Target < Dev->MaxTarget) {
+ //
+ // This device support 256 targets only, so it's enough to increment
+ // the LSB of Target, as it will never overflow.
+ //
+ **Target += 1;
+ *Lun = 0;
} else {
return EFI_NOT_FOUND;
}
@@ -573,11 +585,17 @@ MptScsiGetNextTarget (
IN OUT UINT8 **Target
)
{
- //
- // Currently support only target 0 LUN 0, so hardcode it
- //
+ MPT_SCSI_DEV *Dev;
+
+ Dev = MPT_SCSI_FROM_PASS_THRU (This);
if (!IsTargetInitialized (*Target)) {
ZeroMem (*Target, TARGET_MAX_BYTES);
+ } else if (**Target < Dev->MaxTarget) {
+ //
+ // This device support 256 targets only, so it's enough to increment
+ // the LSB of Target, as it will never overflow.
+ //
+ **Target += 1;
} else {
return EFI_NOT_FOUND;
}
@@ -595,6 +613,7 @@ MptScsiBuildDevicePath (
IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
)
{
+ MPT_SCSI_DEV *Dev;
SCSI_DEVICE_PATH *ScsiDevicePath;
if (DevicePath == NULL) {
@@ -605,7 +624,8 @@ MptScsiBuildDevicePath (
// This device support 256 targets only, so it's enough to dereference
// the LSB of Target.
//
- if (*Target > 0 || Lun > 0) {
+ Dev = MPT_SCSI_FROM_PASS_THRU (This);
+ if (*Target > Dev->MaxTarget || Lun > 0) {
return EFI_NOT_FOUND;
}
@@ -635,6 +655,7 @@ MptScsiGetTargetLun (
OUT UINT64 *Lun
)
{
+ MPT_SCSI_DEV *Dev;
SCSI_DEVICE_PATH *ScsiDevicePath;
if (DevicePath == NULL ||
@@ -647,8 +668,9 @@ MptScsiGetTargetLun (
return EFI_UNSUPPORTED;
}
+ Dev = MPT_SCSI_FROM_PASS_THRU (This);
ScsiDevicePath = (SCSI_DEVICE_PATH *)DevicePath;
- if (ScsiDevicePath->Pun > 0 ||
+ if (ScsiDevicePath->Pun > Dev->MaxTarget ||
ScsiDevicePath->Lun > 0) {
return EFI_NOT_FOUND;
}
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index ef1f6a5ebb3a..26aca7f95315 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -37,3 +37,4 @@ [Protocols]
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit ## CONSUMES
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 7fa1581f2101..7b56998abc9b 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -273,6 +273,10 @@ [PcdsFixedAtBuild]
## Microseconds to stall between polling for MptScsi request result
gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x39
+ ## Set the *inclusive* number of targets that MptScsi exposes for scan
+ # by ScsiBusDxe.
+ gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x40
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 13/13] OvmfPkg/MptScsiDxe: Reset device on ExitBootServices()
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (11 preceding siblings ...)
2020-04-14 17:38 ` [PATCH v4 12/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
@ 2020-04-14 17:38 ` Nikita Leshenko
2020-04-20 19:02 ` [edk2-devel] " Laszlo Ersek
12 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-14 17:38 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, Jordan Justen,
Laszlo Ersek, Ard Biesheuvel
This causes the device to forget about the reply frame. We allocated the
reply frame in EfiBootServicesData type memory, and code executing after
ExitBootServices() is permitted to overwrite it.
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 3ea08857df5d..e632b076fc4a 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -45,6 +45,7 @@ typedef struct {
EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
EFI_PCI_IO_PROTOCOL *PciIo;
UINT64 OriginalPciAttributes;
+ EFI_EVENT ExitBoot;
UINT32 StallPerPollUsec;
UINT8 MaxTarget;
MPT_SCSI_DMA_BUFFER *Dma;
@@ -696,6 +697,19 @@ MptScsiResetChannel (
return EFI_UNSUPPORTED;
}
+STATIC
+VOID
+EFIAPI
+MptScsiExitBoot (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ MPT_SCSI_DEV *Dev;
+
+ Dev = Context;
+ MptScsiReset (Dev);
+}
STATIC
EFI_STATUS
EFIAPI
@@ -861,6 +875,17 @@ MptScsiControllerStart (
goto Unmap;
}
+ Status = gBS->CreateEvent (
+ EVT_SIGNAL_EXIT_BOOT_SERVICES,
+ TPL_CALLBACK,
+ &MptScsiExitBoot,
+ Dev,
+ &Dev->ExitBoot
+ );
+ if (EFI_ERROR (Status)) {
+ goto UninitDev;
+ }
+
//
// Host adapter channel, doesn't exist
//
@@ -885,11 +910,14 @@ MptScsiControllerStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto UninitDev;
+ goto CloseExitBoot;
}
return EFI_SUCCESS;
+CloseExitBoot:
+ gBS->CloseEvent (Dev->ExitBoot);
+
UninitDev:
MptScsiReset (Dev);
@@ -964,6 +992,8 @@ MptScsiControllerStop (
return Status;
}
+ gBS->CloseEvent (Dev->ExitBoot);
+
MptScsiReset (Dev);
Dev->PciIo->Unmap (
--
2.20.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 01/13] OvmfPkg/MptScsiDxe: Create empty driver
2020-04-14 17:38 ` [PATCH v4 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
@ 2020-04-15 6:31 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-15 6:31 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> In preparation for implementing LSI Fusion MPT SCSI devices, create a
> basic scaffolding for a driver.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
> Maintainers.txt | 3 ++-
> OvmfPkg/MptScsiDxe/MptScsi.c | 26 ++++++++++++++++++++++++++
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 26 ++++++++++++++++++++++++++
> OvmfPkg/OvmfPkgIa32.dsc | 4 ++++
> OvmfPkg/OvmfPkgIa32.fdf | 3 +++
> OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
> OvmfPkg/OvmfPkgIa32X64.fdf | 3 +++
> OvmfPkg/OvmfPkgX64.dsc | 4 ++++
> OvmfPkg/OvmfPkgX64.fdf | 3 +++
> 9 files changed, 75 insertions(+), 1 deletion(-)
> create mode 100644 OvmfPkg/MptScsiDxe/MptScsi.c
> create mode 100644 OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 1733225722b6..01b5b8188158 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -435,8 +435,9 @@ OvmfPkg: CSM modules
> F: OvmfPkg/Csm/
> R: David Woodhouse <dwmw2@infradead.org>
>
> -OvmfPkg: PVSCSI driver
> +OvmfPkg: PVSCSI and MptScsi driver
> F: OvmfPkg/PvScsiDxe/
> +F: OvmfPkg/MptScsiDxe/
(1) Please keep the "F:" lines sorted.
With that:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> R: Liran Alon <liran.alon@oracle.com>
> R: Nikita Leshenko <nikita.leshchenko@oracle.com>
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> new file mode 100644
> index 000000000000..c6c8142dfde6
> --- /dev/null
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -0,0 +1,26 @@
> +/** @file
> +
> + This driver produces Extended SCSI Pass Thru Protocol instances for
> + LSI Fusion MPT SCSI 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
> +MptScsiEntryPoint (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + return EFI_UNSUPPORTED;
> +}
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> new file mode 100644
> index 000000000000..b4006a7c2d97
> --- /dev/null
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -0,0 +1,26 @@
> +## @file
> +# This driver produces Extended SCSI Pass Thru Protocol instances for
> +# LSI Fusion MPT SCSI devices.
> +#
> +# Copyright (C) 2020, Oracle and/or its affiliates.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 1.29
> + BASE_NAME = MptScsiDxe
> + FILE_GUID = 2B3DB5DD-B315-4961-8454-0AFF3C811B19
> + MODULE_TYPE = UEFI_DRIVER
> + VERSION_STRING = 1.0
> + ENTRY_POINT = MptScsiEntryPoint
> +
> +[Sources]
> + MptScsi.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> + UefiDriverEntryPoint
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index cbc5f0e583bc..158a5e9f39bd 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -49,6 +49,7 @@ [Defines]
> # Device drivers
> #
> DEFINE PVSCSI_ENABLE = TRUE
> + DEFINE MPT_SCSI_ENABLE = TRUE
>
> #
> # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> @@ -744,6 +745,9 @@ [Components]
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> !if $(PVSCSI_ENABLE) == TRUE
> OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +!endif
> +!if $(MPT_SCSI_ENABLE) == TRUE
> + OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> !endif
> MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index 8e43f4264ecc..fd81b6fa8bed 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -233,6 +233,9 @@ [FV.DXEFV]
> !if $(PVSCSI_ENABLE) == TRUE
> INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> !endif
> +!if $(MPT_SCSI_ENABLE) == TRUE
> +INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +!endif
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 6d69cc6cb56f..a6c5a1d9d050 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -48,6 +48,7 @@ [Defines]
> # Device drivers
> #
> DEFINE PVSCSI_ENABLE = TRUE
> + DEFINE MPT_SCSI_ENABLE = TRUE
>
> #
> # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> @@ -756,6 +757,9 @@ [Components.X64]
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> !if $(PVSCSI_ENABLE) == TRUE
> OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +!endif
> +!if $(MPT_SCSI_ENABLE) == TRUE
> + OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> !endif
> MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index 25af9fbed48a..f71134a65931 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -234,6 +234,9 @@ [FV.DXEFV]
> !if $(PVSCSI_ENABLE) == TRUE
> INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> !endif
> +!if $(MPT_SCSI_ENABLE) == TRUE
> +INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +!endif
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 5ad4f461ce52..9aa8dd9e5fe1 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -48,6 +48,7 @@ [Defines]
> # Device drivers
> #
> DEFINE PVSCSI_ENABLE = TRUE
> + DEFINE MPT_SCSI_ENABLE = TRUE
>
> #
> # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> @@ -754,6 +755,9 @@ [Components]
> OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> !if $(PVSCSI_ENABLE) == TRUE
> OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +!endif
> +!if $(MPT_SCSI_ENABLE) == TRUE
> + OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> !endif
> MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index 25af9fbed48a..f71134a65931 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -234,6 +234,9 @@ [FV.DXEFV]
> !if $(PVSCSI_ENABLE) == TRUE
> INF OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> !endif
> +!if $(MPT_SCSI_ENABLE) == TRUE
> +INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +!endif
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
2020-04-14 17:38 ` [PATCH v4 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
@ 2020-04-15 6:54 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-15 6:54 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> Support dynamic insertion and removal of the protocol
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 181 +++++++++++++++++++++++++++++-
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 5 +-
> 2 files changed, 183 insertions(+), 3 deletions(-)
My R-b stands, just one small request:
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 4e2f8f2296fb..40d392c2346f 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -11,9 +11,12 @@
>
> #include <IndustryStandard/FusionMptScsi.h>
> #include <IndustryStandard/Pci.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Protocol/PciIo.h>
> +#include <Protocol/ScsiPassThruExt.h>
> #include <Uefi/UefiSpec.h>
>
> //
> @@ -22,6 +25,109 @@
> //
> #define MPT_SCSI_BINDING_VERSION 0x10
>
> +//
> +// Runtime Structures
> +//
> +
> +#define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
> +typedef struct {
> + UINT32 Signature;
> + EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> + EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
> +} MPT_SCSI_DEV;
> +
> +#define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> + CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
> +
> +//
> +// Ext SCSI Pass Thru
> +//
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiPassThru (
> + 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
> +MptScsiGetNextTargetLun (
> + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> + IN OUT UINT8 **Target,
> + IN OUT UINT64 *Lun
> + )
> +{
> + return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiGetNextTarget (
> + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> + IN OUT UINT8 **Target
> + )
> +{
> + return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiBuildDevicePath (
> + 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
> +MptScsiGetTargetLun (
> + 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
> +MptScsiResetChannel (
> + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This
> + )
> +{
> + return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiResetTargetLun (
> + IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL *This,
> + IN UINT8 *Target,
> + IN UINT64 Lun
> + )
> +{
> + return EFI_UNSUPPORTED;
> +}
> +
> //
> // Driver Binding
> //
> @@ -90,7 +196,49 @@ MptScsiControllerStart (
> IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath OPTIONAL
> )
> {
> - return EFI_UNSUPPORTED;
> + EFI_STATUS Status;
> + MPT_SCSI_DEV *Dev;
> +
> + Dev = AllocateZeroPool (sizeof (*Dev));
> + if (Dev == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
> +
> + //
> + // Host adapter channel, doesn't exist
> + //
> + Dev->PassThruMode.AdapterId = MAX_UINT32;
> + Dev->PassThruMode.Attributes =
> + EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL |
> + EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
> +
> + Dev->PassThru.Mode = &Dev->PassThruMode;
> + Dev->PassThru.PassThru = &MptScsiPassThru;
> + Dev->PassThru.GetNextTargetLun = &MptScsiGetNextTargetLun;
> + Dev->PassThru.BuildDevicePath = &MptScsiBuildDevicePath;
> + Dev->PassThru.GetTargetLun = &MptScsiGetTargetLun;
> + Dev->PassThru.ResetChannel = &MptScsiResetChannel;
> + Dev->PassThru.ResetTargetLun = &MptScsiResetTargetLun;
> + Dev->PassThru.GetNextTarget = &MptScsiGetNextTarget;
> +
> + Status = gBS->InstallProtocolInterface (
> + &ControllerHandle,
> + &gEfiExtScsiPassThruProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + &Dev->PassThru
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreePool;
> + }
> +
> + return EFI_SUCCESS;
> +
> +FreePool:
> + FreePool (Dev);
> +
> + return Status;
> }
>
> STATIC
> @@ -103,7 +251,36 @@ MptScsiControllerStop (
> IN EFI_HANDLE *ChildHandleBuffer
> )
> {
> - return EFI_UNSUPPORTED;
> + EFI_STATUS Status;
> + EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru;
> + MPT_SCSI_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 = MPT_SCSI_FROM_PASS_THRU (PassThru);
> +
> + Status = gBS->UninstallProtocolInterface (
> + ControllerHandle,
> + &gEfiExtScsiPassThruProtocolGuid,
> + &Dev->PassThru
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + FreePool (Dev);
> +
> + return Status;
> }
>
> STATIC
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 414b96e5a248..5bdbc63f3ac6 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -24,9 +24,12 @@ [Packages]
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + DebugLib
> + MemoryAllocationLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> UefiLib
>
> [Protocols]
> - gEfiPciIoProtocolGuid ## TO_START
> + gEfiPciIoProtocolGuid ## TO_START
> + gEfiExtScsiPassThruProtocolGuid ## BY_START
>
(1) Please keep this section sorted as well.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 07/13] OvmfPkg/MptScsiDxe: Build and decode DevicePath
2020-04-14 17:38 ` [PATCH v4 07/13] OvmfPkg/MptScsiDxe: Build and decode DevicePath Nikita Leshenko
@ 2020-04-15 12:03 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-15 12:03 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> Used to identify the individual disks in the hardware tree.
>
> Currently we accept only Pun=0 and Lun=0, but we will relax this in a
> later patch.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 57 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 30fb084379db..69ab947c0da2 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -126,7 +126,34 @@ MptScsiBuildDevicePath (
> IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
> )
> {
> - return EFI_UNSUPPORTED;
> + SCSI_DEVICE_PATH *ScsiDevicePath;
> +
> + if (DevicePath == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // This device support 256 targets only, so it's enough to dereference
> + // the LSB of Target.
> + //
> + if (*Target > 0 || Lun > 0) {
> + return EFI_NOT_FOUND;
> + }
> +
> + ScsiDevicePath = AllocateZeroPool (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 = *Target;
> + ScsiDevicePath->Lun = (UINT16)Lun;
> +
> + *DevicePath = &ScsiDevicePath->Header;
> + return EFI_SUCCESS;
> }
>
> STATIC
> @@ -139,7 +166,33 @@ MptScsiGetTargetLun (
> OUT UINT64 *Lun
> )
> {
> - return EFI_UNSUPPORTED;
> + SCSI_DEVICE_PATH *ScsiDevicePath;
> +
> + 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;
> + if (ScsiDevicePath->Pun > 0 ||
> + ScsiDevicePath->Lun > 0) {
> + return EFI_NOT_FOUND;
> + }
> +
> + ZeroMem (*Target, TARGET_MAX_BYTES);
> + //
> + // This device support 256 targets only, so it's enough to set the LSB
> + // of Target.
> + //
> + **Target = (UINT8)ScsiDevicePath->Pun;
> + *Lun = ScsiDevicePath->Lun;
> +
> + return EFI_SUCCESS;
> }
>
> STATIC
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 08/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use
2020-04-14 17:38 ` [PATCH v4 08/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
@ 2020-04-16 8:05 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-16 8:05 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> This will give us an exclusive access to the PciIo of this device
> after it was started and until is will be stopped.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 69ab947c0da2..275265774252 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -35,6 +35,7 @@ typedef struct {
> UINT32 Signature;
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
> + EFI_PCI_IO_PROTOCOL *PciIo;
> } MPT_SCSI_DEV;
>
> #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> @@ -295,6 +296,18 @@ MptScsiControllerStart (
>
> Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
>
> + Status = gBS->OpenProtocol (
> + ControllerHandle,
> + &gEfiPciIoProtocolGuid,
> + (VOID **)&Dev->PciIo,
> + This->DriverBindingHandle,
> + ControllerHandle,
> + EFI_OPEN_PROTOCOL_BY_DRIVER
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreePool;
> + }
> +
> //
> // Host adapter channel, doesn't exist
> //
> @@ -319,11 +332,19 @@ MptScsiControllerStart (
> &Dev->PassThru
> );
> if (EFI_ERROR (Status)) {
> - goto FreePool;
> + goto CloseProtocol;
> }
>
> return EFI_SUCCESS;
>
> +CloseProtocol:
> + gBS->CloseProtocol (
> + ControllerHandle,
> + &gEfiPciIoProtocolGuid,
> + This->DriverBindingHandle,
> + ControllerHandle
> + );
> +
> FreePool:
> FreePool (Dev);
>
> @@ -367,6 +388,13 @@ MptScsiControllerStop (
> return Status;
> }
>
> + gBS->CloseProtocol (
> + ControllerHandle,
> + &gEfiPciIoProtocolGuid,
> + This->DriverBindingHandle,
> + ControllerHandle
> + );
> +
> FreePool (Dev);
>
> return Status;
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 09/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes
2020-04-14 17:38 ` [PATCH v4 09/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
@ 2020-04-16 8:11 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-16 8:11 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> Enable the IO Space and Bus Mastering and restore the original values
> when the device is stopped. This is a standard procedure in PCI
> drivers.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 42 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 275265774252..4bfd03d2acb0 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -36,6 +36,7 @@ typedef struct {
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
> EFI_PCI_IO_PROTOCOL *PciIo;
> + UINT64 OriginalPciAttributes;
> } MPT_SCSI_DEV;
>
> #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> @@ -308,6 +309,30 @@ MptScsiControllerStart (
> goto FreePool;
> }
>
> + Status = Dev->PciIo->Attributes (
> + Dev->PciIo,
> + EfiPciIoAttributeOperationGet,
> + 0,
> + &Dev->OriginalPciAttributes
> + );
> + if (EFI_ERROR (Status)) {
> + goto CloseProtocol;
> + }
> +
> + //
> + // Enable I/O Space & Bus-Mastering
> + //
> + Status = Dev->PciIo->Attributes (
> + Dev->PciIo,
> + EfiPciIoAttributeOperationEnable,
> + (EFI_PCI_IO_ATTRIBUTE_IO |
> + EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
> + NULL
> + );
> + if (EFI_ERROR (Status)) {
> + goto CloseProtocol;
> + }
> +
> //
> // Host adapter channel, doesn't exist
> //
> @@ -332,11 +357,19 @@ MptScsiControllerStart (
> &Dev->PassThru
> );
> if (EFI_ERROR (Status)) {
> - goto CloseProtocol;
> + goto RestoreAttributes;
> }
>
> return EFI_SUCCESS;
>
> +RestoreAttributes:
> + Dev->PciIo->Attributes (
> + Dev->PciIo,
> + EfiPciIoAttributeOperationEnable,
(1) This should be "...Set", not "...Enable".
> + Dev->OriginalPciAttributes,
> + NULL
> + );
> +
> CloseProtocol:
> gBS->CloseProtocol (
> ControllerHandle,
> @@ -388,6 +421,13 @@ MptScsiControllerStop (
> return Status;
> }
>
> + Dev->PciIo->Attributes (
> + Dev->PciIo,
> + EfiPciIoAttributeOperationEnable,
(2) Same as (1).
With (1) and (2) fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> + Dev->OriginalPciAttributes,
> + NULL
> + );
> +
> gBS->CloseProtocol (
> ControllerHandle,
> &gEfiPciIoProtocolGuid,
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware
2020-04-14 17:38 ` [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
@ 2020-04-16 9:53 ` Laszlo Ersek
2020-04-16 16:00 ` Nikita Leshenko
2020-04-20 14:10 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-16 9:53 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> Reset and send the IO controller initialization request. The reply is
> read back to complete the doorbell function but it isn't useful to us
> because it doesn't contain relevant data or status codes.
>
> See "LSI53C1030 PCI-X to Dual Channel Ultra320 SCSI Multifunction
> Controller" technical manual for more information.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
> .../Include/IndustryStandard/FusionMptScsi.h | 123 +++++++++++++
> OvmfPkg/MptScsiDxe/MptScsi.c | 173 +++++++++++++++++-
> 2 files changed, 295 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> index df9bdc2f0348..d00a9e6db0bf 100644
> --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> @@ -20,4 +20,127 @@
> #define LSI_SAS1068_PCI_DEVICE_ID 0x0054
> #define LSI_SAS1068E_PCI_DEVICE_ID 0x0058
>
> +#define MPT_REG_DOORBELL 0x00
> +#define MPT_REG_WRITE_SEQ 0x04
> +#define MPT_REG_HOST_DIAG 0x08
> +#define MPT_REG_TEST 0x0c
> +#define MPT_REG_DIAG_DATA 0x10
> +#define MPT_REG_DIAG_ADDR 0x14
> +#define MPT_REG_ISTATUS 0x30
> +#define MPT_REG_IMASK 0x34
> +#define MPT_REG_REQ_Q 0x40
> +#define MPT_REG_REP_Q 0x44
> +
> +#define MPT_DOORBELL_RESET 0x40
> +#define MPT_DOORBELL_HANDSHAKE 0x42
(1) I suggest (but don't insist) that we adjust the whitespace after
MPT_DOORBELL_RESET so that 0x40 line up with 0x42 below it. Just an idea.
> +
> +#define MPT_IMASK_DOORBELL 0x01
> +#define MPT_IMASK_REPLY 0x08
> +
> +#define MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST 0x00
> +#define MPT_MESSAGE_HDR_FUNCTION_IOC_INIT 0x02
> +
> +#define MPT_SG_ENTRY_TYPE_SIMPLE 0x01
> +
> +#define MPT_IOC_WHOINIT_ROM_BIOS 0x02
> +
> +//
> +// Device structures
> +//
> +
> +typedef union {
> +#pragma pack (1)
> + struct {
> + UINT8 WhoInit;
> + UINT8 Reserved1;
> + UINT8 ChainOffset;
> + UINT8 Function;
> + UINT8 Flags;
> + UINT8 MaxDevices;
> + UINT8 MaxBuses;
> + UINT8 MessageFlags;
> + UINT32 MessageContext;
> + UINT16 ReplyFrameSize;
> + UINT16 Reserved2;
> + UINT32 HostMfaHighAddr;
> + UINT32 SenseBufferHighAddr;
> + } Data;
> +#pragma pack ()
> + UINT64 Uint64; // 8 byte alignment required by HW
> +} MPT_IO_CONTROLLER_INIT_REQUEST;
> +
> +#pragma pack (1)
> +typedef struct {
> + UINT8 WhoInit;
> + UINT8 Reserved1;
> + UINT8 MessageLength;
> + UINT8 Function;
> + UINT8 Flags;
> + UINT8 MaxDevices;
> + UINT8 MaxBuses;
> + UINT8 MessageFlags;
> + UINT32 MessageContext;
> + UINT16 Reserved2;
> + UINT16 IOCStatus;
> + UINT32 IOCLogInfo;
> +} MPT_IO_CONTROLLER_INIT_REPLY;
> +
> +typedef struct {
> + UINT8 TargetID;
> + UINT8 Bus;
> + UINT8 ChainOffset;
> + UINT8 Function;
> + UINT8 CDBLength;
> + UINT8 SenseBufferLength;
> + UINT8 Reserved;
> + UINT8 MessageFlags;
> + UINT32 MessageContext;
> + UINT8 LUN[8];
> + UINT32 Control;
> + UINT8 CDB[16];
> + UINT32 DataLength;
> + UINT32 SenseBufferLowAddress;
> +} MPT_SCSI_IO_REQUEST;
> +
> +typedef struct {
> + UINT32 Length: 24;
> + UINT32 EndOfList: 1;
> + UINT32 Is64BitAddress: 1;
> + //
> + // True when the buffer contains data to be transfered. Otherwise it's the
> + // destination buffer
> + //
> + UINT32 BufferContainsData: 1;
> + UINT32 LocalAddress: 1;
> + UINT32 ElementType: 2;
> + UINT32 EndOfBuffer: 1;
> + UINT32 LastElement: 1;
> + UINT64 DataBufferAddress;
> +} MPT_SG_ENTRY_SIMPLE;
> +#pragma pack ()
> +
> +typedef union {
> +#pragma pack (1)
> + struct {
> + UINT8 TargetID;
> + UINT8 Bus;
> + UINT8 MessageLength;
> + UINT8 Function;
> + UINT8 CDBLength;
> + UINT8 SenseBufferLength;
> + UINT8 Reserved;
> + UINT8 MessageFlags;
> + UINT32 MessageContext;
> + UINT8 SCSIStatus;
> + UINT8 SCSIState;
> + UINT16 IOCStatus;
> + UINT32 IOCLogInfo;
> + UINT32 TransferCount;
> + UINT32 SenseCount;
> + UINT32 ResponseInfo;
> + } Data;
> +#pragma pack ()
> + UINT64 Uint64; // 8 byte alignment required by HW
> +} MPT_SCSI_IO_ERROR_REPLY;
> +
> #endif // __FUSION_MPT_SCSI_H__
(2) If you really want to sink the pragmas into the unions, so that they
only surround the embedded structs, I'm OK with that. If you feel
flexible about them, I'd suggest wrapping the entire sequence of
typedefs into a single #pragma pack (1) / pack () pair, which is easier
to read and feels more idiomatic in edk2. But, again, if you feel it's
important to express the packing goals with this specific syntax, I'm OK
that way too.
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 4bfd03d2acb0..9c3bdc430e1a 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -42,6 +42,167 @@ typedef struct {
> #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
>
> +//
> +// Hardware functions
> +//
> +
> +STATIC
> +EFI_STATUS
> +Out32 (
> + IN MPT_SCSI_DEV *Dev,
> + IN UINT32 Addr,
> + IN UINT32 Data
> + )
> +{
> + return Dev->PciIo->Io.Write (
> + Dev->PciIo,
> + EfiPciIoWidthUint32,
> + PCI_BAR_IDX0,
> + Addr,
> + 1,
> + &Data
> + );
> +}
> +
> +STATIC
> +EFI_STATUS
> +In32 (
> + IN MPT_SCSI_DEV *Dev,
> + IN UINT32 Addr,
> + OUT UINT32 *Data
> + )
> +{
> + return Dev->PciIo->Io.Read (
> + Dev->PciIo,
> + EfiPciIoWidthUint32,
> + PCI_BAR_IDX0,
> + Addr,
> + 1,
> + Data
> + );
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptDoorbell (
> + IN MPT_SCSI_DEV *Dev,
> + IN UINT8 DoorbellFunc,
> + IN UINT8 DoorbellArg
> + )
> +{
> + return Out32 (
> + Dev,
> + MPT_REG_DOORBELL,
> + (((UINT32)DoorbellFunc) << 24) | (DoorbellArg << 16)
> + );
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiReset (
> + IN MPT_SCSI_DEV *Dev
> + )
> +{
> + EFI_STATUS Status;
> +
> + //
> + // Reset hardware
> + //
> + Status = MptDoorbell (Dev, MPT_DOORBELL_RESET, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + //
> + // Mask interrupts
> + //
> + Status = Out32 (Dev, MPT_REG_IMASK, MPT_IMASK_DOORBELL | MPT_IMASK_REPLY);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + //
> + // Clear interrupt status
> + //
> + Status = Out32 (Dev, MPT_REG_ISTATUS, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiInit (
> + IN MPT_SCSI_DEV *Dev
> + )
> +{
> + EFI_STATUS Status;
> + MPT_IO_CONTROLLER_INIT_REQUEST Req;
> + MPT_IO_CONTROLLER_INIT_REPLY Reply;
> + UINT8 *ReplyBytes;
> + UINT32 ReplyWord;
> +
> + Status = MptScsiReset (Dev);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + ZeroMem (&Req, sizeof (Req));
> + ZeroMem (&Reply, sizeof (Reply));
> + Req.Data.WhoInit = MPT_IOC_WHOINIT_ROM_BIOS;
> + Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
> + Req.Data.MaxDevices = 1;
> + Req.Data.MaxBuses = 1;
> + Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
(3) The local variable that you're going to read the reply into has type
MPT_IO_CONTROLLER_INIT_REPLY. But the number of bytes you announce for
reading back matches MPT_SCSI_IO_ERROR_REPLY.
I haven't checked the technical manual that the commit message
references, so I don't know which reply format is the one that's needed
here -- but whichever is needed, the number of bytes that we read back
should actually match the structure that we populate.
(General remark: for this reason, I avoid "sizeof (type)" on principle,
whenever I can, and use "sizeof object" instead. With the latter, it's
more difficult to get "out of sync" if I change the type of the object
later on.)
Hmmm, wait a second!... Is it possible that "ReplyFrameSize" is for
configuring *future* replies, and the MPT_MESSAGE_HDR_FUNCTION_IOC_INIT
request code guarantees that the device will produce an
MPT_IO_CONTROLLER_INIT_REPLY object in response, regardless of
"ReplyFrameSize"?
In that case, please ignore my comment (3).
(4) We don't align MPT_IO_CONTROLLER_INIT_REPLY to an 8-byte boundary,
unlike MPT_IO_CONTROLLER_INIT_REQUEST.
Can you please confirm that it's because we read
MPT_IO_CONTROLLER_INIT_REPLY gradually (dword-wise), via "ReplyWord" --
which is automatically naturally aligned? (I.e., I'm not suggesting a
code change, just asking for a confirmation.)
> +
> + //
> + // Send controller init through doorbell
> + //
> + Status = MptDoorbell (
> + Dev,
> + MPT_DOORBELL_HANDSHAKE,
> + sizeof (Req) / sizeof (UINT32)
(5) Please use a STATIC_ASSERT here, to express that this division
produces a 0 remainder. Please see the similar STATIC_ASSERT in the
PvScsi driver.
(6) Furthermore, I'd suggest another STATIC_ASSERT to express that the
quotient fits in a UINT8, and then maybe explicitly casting the quotient
to UINT8.
I'm asking for the explicit cast only because I'm concerned that the VS
compiler will whine about "possible loss of precision". (I realize the
quotient is a constant that's known at compile time -- but I'm afraid VS
might still whine about the implicit UINTN->UINT8 conversion, for the
"DoorbellArg" parameter.)
(7) Note that the union pattern you use for alignment is *slightly*
different from the one used in PvScsi. And this difference could hide a
trap -- an obscure trap, admittedly, but I'd like to be clear about it.
In PvScsi we have a local variable called "AlignedCmd", which is of the
union type. The "AlignedCmd.Cmd" union member, which is itself a packed
structure, is the actual datum that we serialize and send to the device.
Here however, we seem to send the full union to the device.
Now, per ISO C99 "6.7.2.1 Structure and union specifiers", paragraph 15,
"[t]here may be unnamed padding at the end of a structure or union". And
see my comment (2) above -- you specifically do *not* pack the unions,
only the internals of their "Data" members. (The gcc documentation
confirms that #pragma pack affects unions too, so packing a union *can*
make a difference.)
Thus, when you send the whole MPT_IO_CONTROLLER_INIT_REQUEST union to
the device, and not just its "Data" member (which is itself packed), you
could be writing from unnamed padding at the end of the union.
Therefore I would:
(7a) either send "Req.Data" (rather than "Req") to the device in this
function,
(7b) or else address my remark (2), and pack the unions too.
(8) I believe same observation as (7) holds for the
Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
assignment. Even if my comment (3) falls away, and "ReplyFrameSize"
configures the size of *future* responses, those responses should likely
not include any (potential) unnamed padding at the end of the
MPT_SCSI_IO_ERROR_REPLY union.
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = Dev->PciIo->Io.Write (
> + Dev->PciIo,
> + EfiPciIoWidthFifoUint32,
> + 0,
(9) Please use PCI_BAR_IDX0 here too.
> + MPT_REG_DOORBELL,
> + sizeof (Req) / sizeof (UINT32),
> + &Req
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // Read reply through doorbell
> + // Each 32bit (Dword) read produces 16bit (Word) of data
> + //
(10) Please use another STATIC_ASSERT here for expressing that the
response structure size is an integer multiple of sizeof (UINT16).
(11) Please repeat the statement from the commit message here -- as a
code comment -- that the reply is only read to complete the doorbell
function of the device, and that we intentionally ignore the contents.
(BTW, if we do not parse the response even at the end of this series,
then saving the response into the "Reply" variable looks useless -- I
guess we could remove the "Reply" variable altogether, in that case. But
I'll have to see the rest of the patches.)
> + ReplyBytes = (UINT8 *)&Reply;
> + while (ReplyBytes != (UINT8 *)(&Reply + 1)) {
> + Status = In32 (Dev, MPT_REG_DOORBELL, &ReplyWord);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + CopyMem (ReplyBytes, &ReplyWord, sizeof (UINT16));
> + ReplyBytes += sizeof (UINT16);
> + }
> +
> + //
> + // Clear interrupts generated by doorbell reply
> + //
> + Status = Out32 (Dev, MPT_REG_ISTATUS, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + return EFI_SUCCESS;
> +}
(12) So in general I would have suggested that, in case the
initialization fails and we take an early "return" branch in this
function, we should first jump to an error handling label, and reset the
device. (Because, when we return an error code from the function, we
shouldn't leave the device in half-initialized state.)
*But*, it seems like the reset operation *itself* occurs through the
doorbell register and the MPT_REG_ISTATUS register -- and if any step in
MptScsiInit() fails, then it's exactly those registers that are
(apparently) in unknown / unreliable state. In other words, if
MptScsiInit() fails, then we *can't* (expect to) reset the device
(successfully).
Can you please confirm my understanding? (Again, just a question, not a
code change request.)
> +
> //
> // Ext SCSI Pass Thru
> //
> @@ -333,6 +494,11 @@ MptScsiControllerStart (
> goto CloseProtocol;
> }
>
> + Status = MptScsiInit (Dev);
> + if (EFI_ERROR (Status)) {
> + goto RestorePciAttributes;
> + }
> +
> //
> // Host adapter channel, doesn't exist
> //
> @@ -357,11 +523,14 @@ MptScsiControllerStart (
> &Dev->PassThru
> );
> if (EFI_ERROR (Status)) {
> - goto RestoreAttributes;
> + goto UninitDev;
> }
>
> return EFI_SUCCESS;
>
> +UninitDev:
> + MptScsiReset (Dev);
> +
> RestoreAttributes:
> Dev->PciIo->Attributes (
> Dev->PciIo,
> @@ -421,6 +590,8 @@ MptScsiControllerStop (
> return Status;
> }
>
> + MptScsiReset (Dev);
> +
> Dev->PciIo->Attributes (
> Dev->PciIo,
> EfiPciIoAttributeOperationEnable,
>
These parts look fine to me.
Before proceeding to patch#11 in this series, I'll wait for your answer;
I think I need to read & understand it in order to continue the review.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware
2020-04-16 9:53 ` [edk2-devel] " Laszlo Ersek
@ 2020-04-16 16:00 ` Nikita Leshenko
2020-04-20 11:58 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-16 16:00 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
> On 16 Apr 2020, at 12:53, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 04/14/20 19:38, Nikita Leshenko wrote:
>> Reset and send the IO controller initialization request. The reply is
>> read back to complete the doorbell function but it isn't useful to us
>> because it doesn't contain relevant data or status codes.
>>
>> See "LSI53C1030 PCI-X to Dual Channel Ultra320 SCSI Multifunction
>> Controller" technical manual for more information.
>>
>> Ref: https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2390__;!!GqivPVa7Brio!Ph-KBaVDeTP4F0oR9YHybLz2_YpCTUBsXM5rymNRMgRKVeUaRsWXxogKvM9T1fsEfQg7pQ$
>> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> ---
>> .../Include/IndustryStandard/FusionMptScsi.h | 123 +++++++++++++
>> OvmfPkg/MptScsiDxe/MptScsi.c | 173 +++++++++++++++++-
>> 2 files changed, 295 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
>> index df9bdc2f0348..d00a9e6db0bf 100644
>> --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
>> +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
>> @@ -20,4 +20,127 @@
>> #define LSI_SAS1068_PCI_DEVICE_ID 0x0054
>> #define LSI_SAS1068E_PCI_DEVICE_ID 0x0058
>>
>> +#define MPT_REG_DOORBELL 0x00
>> +#define MPT_REG_WRITE_SEQ 0x04
>> +#define MPT_REG_HOST_DIAG 0x08
>> +#define MPT_REG_TEST 0x0c
>> +#define MPT_REG_DIAG_DATA 0x10
>> +#define MPT_REG_DIAG_ADDR 0x14
>> +#define MPT_REG_ISTATUS 0x30
>> +#define MPT_REG_IMASK 0x34
>> +#define MPT_REG_REQ_Q 0x40
>> +#define MPT_REG_REP_Q 0x44
>> +
>> +#define MPT_DOORBELL_RESET 0x40
>> +#define MPT_DOORBELL_HANDSHAKE 0x42
>
> (1) I suggest (but don't insist) that we adjust the whitespace after
> MPT_DOORBELL_RESET so that 0x40 line up with 0x42 below it. Just an idea.
Good idea, will do in v5
>
>> +
>> +#define MPT_IMASK_DOORBELL 0x01
>> +#define MPT_IMASK_REPLY 0x08
>> +
>> +#define MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST 0x00
>> +#define MPT_MESSAGE_HDR_FUNCTION_IOC_INIT 0x02
>> +
>> +#define MPT_SG_ENTRY_TYPE_SIMPLE 0x01
>> +
>> +#define MPT_IOC_WHOINIT_ROM_BIOS 0x02
>> +
>> +//
>> +// Device structures
>> +//
>> +
>> +typedef union {
>> +#pragma pack (1)
>> + struct {
>> + UINT8 WhoInit;
>> + UINT8 Reserved1;
>> + UINT8 ChainOffset;
>> + UINT8 Function;
>> + UINT8 Flags;
>> + UINT8 MaxDevices;
>> + UINT8 MaxBuses;
>> + UINT8 MessageFlags;
>> + UINT32 MessageContext;
>> + UINT16 ReplyFrameSize;
>> + UINT16 Reserved2;
>> + UINT32 HostMfaHighAddr;
>> + UINT32 SenseBufferHighAddr;
>> + } Data;
>> +#pragma pack ()
>> + UINT64 Uint64; // 8 byte alignment required by HW
>> +} MPT_IO_CONTROLLER_INIT_REQUEST;
>> +
>> +#pragma pack (1)
>> +typedef struct {
>> + UINT8 WhoInit;
>> + UINT8 Reserved1;
>> + UINT8 MessageLength;
>> + UINT8 Function;
>> + UINT8 Flags;
>> + UINT8 MaxDevices;
>> + UINT8 MaxBuses;
>> + UINT8 MessageFlags;
>> + UINT32 MessageContext;
>> + UINT16 Reserved2;
>> + UINT16 IOCStatus;
>> + UINT32 IOCLogInfo;
>> +} MPT_IO_CONTROLLER_INIT_REPLY;
>> +
>> +typedef struct {
>> + UINT8 TargetID;
>> + UINT8 Bus;
>> + UINT8 ChainOffset;
>> + UINT8 Function;
>> + UINT8 CDBLength;
>> + UINT8 SenseBufferLength;
>> + UINT8 Reserved;
>> + UINT8 MessageFlags;
>> + UINT32 MessageContext;
>> + UINT8 LUN[8];
>> + UINT32 Control;
>> + UINT8 CDB[16];
>> + UINT32 DataLength;
>> + UINT32 SenseBufferLowAddress;
>> +} MPT_SCSI_IO_REQUEST;
>> +
>> +typedef struct {
>> + UINT32 Length: 24;
>> + UINT32 EndOfList: 1;
>> + UINT32 Is64BitAddress: 1;
>> + //
>> + // True when the buffer contains data to be transfered. Otherwise it's the
>> + // destination buffer
>> + //
>> + UINT32 BufferContainsData: 1;
>> + UINT32 LocalAddress: 1;
>> + UINT32 ElementType: 2;
>> + UINT32 EndOfBuffer: 1;
>> + UINT32 LastElement: 1;
>> + UINT64 DataBufferAddress;
>> +} MPT_SG_ENTRY_SIMPLE;
>> +#pragma pack ()
>> +
>> +typedef union {
>> +#pragma pack (1)
>> + struct {
>> + UINT8 TargetID;
>> + UINT8 Bus;
>> + UINT8 MessageLength;
>> + UINT8 Function;
>> + UINT8 CDBLength;
>> + UINT8 SenseBufferLength;
>> + UINT8 Reserved;
>> + UINT8 MessageFlags;
>> + UINT32 MessageContext;
>> + UINT8 SCSIStatus;
>> + UINT8 SCSIState;
>> + UINT16 IOCStatus;
>> + UINT32 IOCLogInfo;
>> + UINT32 TransferCount;
>> + UINT32 SenseCount;
>> + UINT32 ResponseInfo;
>> + } Data;
>> +#pragma pack ()
>> + UINT64 Uint64; // 8 byte alignment required by HW
>> +} MPT_SCSI_IO_ERROR_REPLY;
>> +
>> #endif // __FUSION_MPT_SCSI_H__
>
> (2) If you really want to sink the pragmas into the unions, so that they
> only surround the embedded structs, I'm OK with that. If you feel
> flexible about them, I'd suggest wrapping the entire sequence of
> typedefs into a single #pragma pack (1) / pack () pair, which is easier
> to read and feels more idiomatic in edk2. But, again, if you feel it's
> important to express the packing goals with this specific syntax, I'm OK
> that way too.
I don't have a strong opinion about it, I prefer to do what feels more idiomatic
in EDK2 (although I don't always know what is idiomatic in EDK2 so feel free to
point out such things :) ).
My intent is to have the structs packed but still aligned to 8 bytes (for some
reason the hardware aligns the descriptor address to 8). I couldn't find any code
in EDK2 that does so I came up with my own pattern.
However, if I understand your suggestion correctly, if I wrap the
entire typedef in #pragma pack (1) / pack (), I would lose the alignment.
That's why I wrapped only the inner structs. Are are you suggesting
that I break out the inner structs into separate typedefs, wrap all of them in
#pragma pack (1) / pack () and then create the wrapping aligning unions?
>
>> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
>> index 4bfd03d2acb0..9c3bdc430e1a 100644
>> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
>> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
>> @@ -42,6 +42,167 @@ typedef struct {
>> #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
>> CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
>>
>> +//
>> +// Hardware functions
>> +//
>> +
>> +STATIC
>> +EFI_STATUS
>> +Out32 (
>> + IN MPT_SCSI_DEV *Dev,
>> + IN UINT32 Addr,
>> + IN UINT32 Data
>> + )
>> +{
>> + return Dev->PciIo->Io.Write (
>> + Dev->PciIo,
>> + EfiPciIoWidthUint32,
>> + PCI_BAR_IDX0,
>> + Addr,
>> + 1,
>> + &Data
>> + );
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +In32 (
>> + IN MPT_SCSI_DEV *Dev,
>> + IN UINT32 Addr,
>> + OUT UINT32 *Data
>> + )
>> +{
>> + return Dev->PciIo->Io.Read (
>> + Dev->PciIo,
>> + EfiPciIoWidthUint32,
>> + PCI_BAR_IDX0,
>> + Addr,
>> + 1,
>> + Data
>> + );
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +MptDoorbell (
>> + IN MPT_SCSI_DEV *Dev,
>> + IN UINT8 DoorbellFunc,
>> + IN UINT8 DoorbellArg
>> + )
>> +{
>> + return Out32 (
>> + Dev,
>> + MPT_REG_DOORBELL,
>> + (((UINT32)DoorbellFunc) << 24) | (DoorbellArg << 16)
>> + );
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +MptScsiReset (
>> + IN MPT_SCSI_DEV *Dev
>> + )
>> +{
>> + EFI_STATUS Status;
>> +
>> + //
>> + // Reset hardware
>> + //
>> + Status = MptDoorbell (Dev, MPT_DOORBELL_RESET, 0);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> + //
>> + // Mask interrupts
>> + //
>> + Status = Out32 (Dev, MPT_REG_IMASK, MPT_IMASK_DOORBELL | MPT_IMASK_REPLY);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> + //
>> + // Clear interrupt status
>> + //
>> + Status = Out32 (Dev, MPT_REG_ISTATUS, 0);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> +
>> + return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +MptScsiInit (
>> + IN MPT_SCSI_DEV *Dev
>> + )
>> +{
>> + EFI_STATUS Status;
>> + MPT_IO_CONTROLLER_INIT_REQUEST Req;
>> + MPT_IO_CONTROLLER_INIT_REPLY Reply;
>> + UINT8 *ReplyBytes;
>> + UINT32 ReplyWord;
>> +
>> + Status = MptScsiReset (Dev);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> +
>> + ZeroMem (&Req, sizeof (Req));
>> + ZeroMem (&Reply, sizeof (Reply));
>> + Req.Data.WhoInit = MPT_IOC_WHOINIT_ROM_BIOS;
>> + Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
>> + Req.Data.MaxDevices = 1;
>> + Req.Data.MaxBuses = 1;
>> + Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
>
> (3) The local variable that you're going to read the reply into has type
> MPT_IO_CONTROLLER_INIT_REPLY. But the number of bytes you announce for
> reading back matches MPT_SCSI_IO_ERROR_REPLY.
>
> I haven't checked the technical manual that the commit message
> references, so I don't know which reply format is the one that's needed
> here -- but whichever is needed, the number of bytes that we read back
> should actually match the structure that we populate.
>
> (General remark: for this reason, I avoid "sizeof (type)" on principle,
> whenever I can, and use "sizeof object" instead. With the latter, it's
> more difficult to get "out of sync" if I change the type of the object
> later on.)
Good idea, I will change it to `sizeof Dev->Dma->IoReply.Data`
>
> Hmmm, wait a second!... Is it possible that "ReplyFrameSize" is for
> configuring *future* replies, and the MPT_MESSAGE_HDR_FUNCTION_IOC_INIT
> request code guarantees that the device will produce an
> MPT_IO_CONTROLLER_INIT_REPLY object in response, regardless of
> "ReplyFrameSize"?
>
> In that case, please ignore my comment (3).
Exactly, it's for future replies
>
>
> (4) We don't align MPT_IO_CONTROLLER_INIT_REPLY to an 8-byte boundary,
> unlike MPT_IO_CONTROLLER_INIT_REQUEST.
>
> Can you please confirm that it's because we read
> MPT_IO_CONTROLLER_INIT_REPLY gradually (dword-wise), via "ReplyWord" --
> which is automatically naturally aligned? (I.e., I'm not suggesting a
> code change, just asking for a confirmation.)
Yes. Now that you mention it, I see that it's enough to align
MPT_IO_CONTROLLER_INIT_REQUEST to 4 bytes since it's read in 32-bit
chunks.
I will add a comment to the code.
>
>
>> +
>> + //
>> + // Send controller init through doorbell
>> + //
>> + Status = MptDoorbell (
>> + Dev,
>> + MPT_DOORBELL_HANDSHAKE,
>> + sizeof (Req) / sizeof (UINT32)
>
> (5) Please use a STATIC_ASSERT here, to express that this division
> produces a 0 remainder. Please see the similar STATIC_ASSERT in the
> PvScsi driver.
>
>
> (6) Furthermore, I'd suggest another STATIC_ASSERT to express that the
> quotient fits in a UINT8, and then maybe explicitly casting the quotient
> to UINT8.
>
> I'm asking for the explicit cast only because I'm concerned that the VS
> compiler will whine about "possible loss of precision". (I realize the
> quotient is a constant that's known at compile time -- but I'm afraid VS
> might still whine about the implicit UINTN->UINT8 conversion, for the
> "DoorbellArg" parameter.)
>
>
> (7) Note that the union pattern you use for alignment is *slightly*
> different from the one used in PvScsi. And this difference could hide a
> trap -- an obscure trap, admittedly, but I'd like to be clear about it.
>
> In PvScsi we have a local variable called "AlignedCmd", which is of the
> union type. The "AlignedCmd.Cmd" union member, which is itself a packed
> structure, is the actual datum that we serialize and send to the device.
>
> Here however, we seem to send the full union to the device.
>
> Now, per ISO C99 "6.7.2.1 Structure and union specifiers", paragraph 15,
> "[t]here may be unnamed padding at the end of a structure or union". And
> see my comment (2) above -- you specifically do *not* pack the unions,
> only the internals of their "Data" members. (The gcc documentation
> confirms that #pragma pack affects unions too, so packing a union *can*
> make a difference.)
>
> Thus, when you send the whole MPT_IO_CONTROLLER_INIT_REQUEST union to
> the device, and not just its "Data" member (which is itself packed), you
> could be writing from unnamed padding at the end of the union.
>
> Therefore I would:
>
> (7a) either send "Req.Data" (rather than "Req") to the device in this
> function,
>
> (7b) or else address my remark (2), and pack the unions too.
>
>
> (8) I believe same observation as (7) holds for the
>
> Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
>
> assignment. Even if my comment (3) falls away, and "ReplyFrameSize"
> configures the size of *future* responses, those responses should likely
> not include any (potential) unnamed padding at the end of the
> MPT_SCSI_IO_ERROR_REPLY union.
I wasn't aware of this padding, so I will update the code to use suggestion (7a).
We can't do (7b) since packing the union would discard the alignment.
>
>
>> + );
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> + Status = Dev->PciIo->Io.Write (
>> + Dev->PciIo,
>> + EfiPciIoWidthFifoUint32,
>> + 0,
>
> (9) Please use PCI_BAR_IDX0 here too.
>
>> + MPT_REG_DOORBELL,
>> + sizeof (Req) / sizeof (UINT32),
>> + &Req
>> + );
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> +
>> + //
>> + // Read reply through doorbell
>> + // Each 32bit (Dword) read produces 16bit (Word) of data
>> + //
>
> (10) Please use another STATIC_ASSERT here for expressing that the
> response structure size is an integer multiple of sizeof (UINT16).
>
> (11) Please repeat the statement from the commit message here -- as a
> code comment -- that the reply is only read to complete the doorbell
> function of the device, and that we intentionally ignore the contents.
>
> (BTW, if we do not parse the response even at the end of this series,
> then saving the response into the "Reply" variable looks useless -- I
> guess we could remove the "Reply" variable altogether, in that case. But
> I'll have to see the rest of the patches.)
We must read the reply fully to initialise the device (the device would reset the
interrupt status only after the host has fetched the entire reply). We could discard
the reply, but I thought it would be nicer to keep it on the stack temporary in
case someone would like to use it in later changes or in debugging (since the
reply is very small and should be hot in the cache, I don't this has any
performance impact).
>
>> + ReplyBytes = (UINT8 *)&Reply;
>> + while (ReplyBytes != (UINT8 *)(&Reply + 1)) {
>> + Status = In32 (Dev, MPT_REG_DOORBELL, &ReplyWord);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> + CopyMem (ReplyBytes, &ReplyWord, sizeof (UINT16));
>> + ReplyBytes += sizeof (UINT16);
>> + }
>> +
>> + //
>> + // Clear interrupts generated by doorbell reply
>> + //
>> + Status = Out32 (Dev, MPT_REG_ISTATUS, 0);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> + }
>> +
>> + return EFI_SUCCESS;
>> +}
>
> (12) So in general I would have suggested that, in case the
> initialization fails and we take an early "return" branch in this
> function, we should first jump to an error handling label, and reset the
> device. (Because, when we return an error code from the function, we
> shouldn't leave the device in half-initialized state.)
>
> *But*, it seems like the reset operation *itself* occurs through the
> doorbell register and the MPT_REG_ISTATUS register -- and if any step in
> MptScsiInit() fails, then it's exactly those registers that are
> (apparently) in unknown / unreliable state. In other words, if
> MptScsiInit() fails, then we *can't* (expect to) reset the device
> (successfully).
>
> Can you please confirm my understanding? (Again, just a question, not a
> code change request.)
Your understanding it correct. In VirtualBox it looks like you can't write a
reset command to the doorbell while a handshake is in progress. In any case, wonder
what could realistically cause the initialization to fail. On EDK2 side, we're just
talking about a bunch of "in/out" opcodes (the API for which could fail, but I would
assume that if it fails then further "in/out"s will fail as well). On the hypervisor
side, the QEMU and VirtualBox don't even have a failure path for the init call.
So failure seems very theoretical here and I couldn't find anything realistically
helpful to do if the initialization fails in the middle.
>
>
>> +
>> //
>> // Ext SCSI Pass Thru
>> //
>> @@ -333,6 +494,11 @@ MptScsiControllerStart (
>> goto CloseProtocol;
>> }
>>
>> + Status = MptScsiInit (Dev);
>> + if (EFI_ERROR (Status)) {
>> + goto RestorePciAttributes;
>> + }
>> +
>> //
>> // Host adapter channel, doesn't exist
>> //
>> @@ -357,11 +523,14 @@ MptScsiControllerStart (
>> &Dev->PassThru
>> );
>> if (EFI_ERROR (Status)) {
>> - goto RestoreAttributes;
>> + goto UninitDev;
>> }
>>
>> return EFI_SUCCESS;
>>
>> +UninitDev:
>> + MptScsiReset (Dev);
>> +
>> RestoreAttributes:
>> Dev->PciIo->Attributes (
>> Dev->PciIo,
>> @@ -421,6 +590,8 @@ MptScsiControllerStop (
>> return Status;
>> }
>>
>> + MptScsiReset (Dev);
>> +
>> Dev->PciIo->Attributes (
>> Dev->PciIo,
>> EfiPciIoAttributeOperationEnable,
>>
>
> These parts look fine to me.
>
> Before proceeding to patch#11 in this series, I'll wait for your answer;
> I think I need to read & understand it in order to continue the review.
ACK on the rest of the points that I didn't leave a comment under, I will change
in v5.
Thanks for the quick review!
Nikita
>
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware
2020-04-16 16:00 ` Nikita Leshenko
@ 2020-04-20 11:58 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-20 11:58 UTC (permalink / raw)
To: Nikita Leshenko
Cc: devel, liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/16/20 18:00, Nikita Leshenko wrote:
>
>
>> On 16 Apr 2020, at 12:53, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 04/14/20 19:38, Nikita Leshenko wrote:
[...]
> I break out the inner structs into separate typedefs, wrap all of them in
> #pragma pack (1) / pack () and then create the wrapping aligning unions?
>> (7a) either send "Req.Data" (rather than "Req") to the device in this
>> function,
> I wasn't aware of this padding, so I will update the code to use suggestion (7a).
OK, thanks!
>> (11) Please repeat the statement from the commit message here -- as a
>> code comment -- that the reply is only read to complete the doorbell
>> function of the device, and that we intentionally ignore the contents.
>>
>> (BTW, if we do not parse the response even at the end of this series,
>> then saving the response into the "Reply" variable looks useless -- I
>> guess we could remove the "Reply" variable altogether, in that case. But
>> I'll have to see the rest of the patches.)
> We must read the reply fully to initialise the device (the device would reset the
> interrupt status only after the host has fetched the entire reply). We could discard
> the reply, but I thought it would be nicer to keep it on the stack temporary in
> case someone would like to use it in later changes or in debugging (since the
> reply is very small and should be hot in the cache, I don't this has any
> performance impact).
Makes sense. The comment will help. Thanks!
>> (12) So in general I would have suggested that, in case the
>> initialization fails and we take an early "return" branch in this
>> function, we should first jump to an error handling label, and reset the
>> device. (Because, when we return an error code from the function, we
>> shouldn't leave the device in half-initialized state.)
>>
>> *But*, it seems like the reset operation *itself* occurs through the
>> doorbell register and the MPT_REG_ISTATUS register -- and if any step in
>> MptScsiInit() fails, then it's exactly those registers that are
>> (apparently) in unknown / unreliable state. In other words, if
>> MptScsiInit() fails, then we *can't* (expect to) reset the device
>> (successfully).
>>
>> Can you please confirm my understanding? (Again, just a question, not a
>> code change request.)
> Your understanding it correct. In VirtualBox it looks like you can't write a
> reset command to the doorbell while a handshake is in progress. In any case, wonder
> what could realistically cause the initialization to fail. On EDK2 side, we're just
> talking about a bunch of "in/out" opcodes (the API for which could fail, but I would
> assume that if it fails then further "in/out"s will fail as well). On the hypervisor
> side, the QEMU and VirtualBox don't even have a failure path for the init call.
> So failure seems very theoretical here and I couldn't find anything realistically
> helpful to do if the initialization fails in the middle.
Thank you for explaining. I'm proceeding with the v4 review.
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware
2020-04-14 17:38 ` [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
2020-04-16 9:53 ` [edk2-devel] " Laszlo Ersek
@ 2020-04-20 14:10 ` Laszlo Ersek
1 sibling, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-20 14:10 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
Some notes related to edk2's CamelCase style:
> +#pragma pack (1)
> +typedef struct {
> + UINT8 WhoInit;
> + UINT8 Reserved1;
> + UINT8 MessageLength;
> + UINT8 Function;
> + UINT8 Flags;
> + UINT8 MaxDevices;
> + UINT8 MaxBuses;
> + UINT8 MessageFlags;
> + UINT32 MessageContext;
> + UINT16 Reserved2;
> + UINT16 IOCStatus;
> + UINT32 IOCLogInfo;
(13) In both above fields, please spell "IOC" as "Ioc".
> +} MPT_IO_CONTROLLER_INIT_REPLY;
> +
> +typedef struct {
> + UINT8 TargetID;
(14) TargetId
> + UINT8 Bus;
> + UINT8 ChainOffset;
> + UINT8 Function;
> + UINT8 CDBLength;
(15) CdbLength
> + UINT8 SenseBufferLength;
> + UINT8 Reserved;
> + UINT8 MessageFlags;
> + UINT32 MessageContext;
> + UINT8 LUN[8];
(16) Lun
> + UINT32 Control;
> + UINT8 CDB[16];
(17) Cdb
> + UINT32 DataLength;
> + UINT32 SenseBufferLowAddress;
> +} MPT_SCSI_IO_REQUEST;
> +
> +typedef struct {
> + UINT32 Length: 24;
> + UINT32 EndOfList: 1;
> + UINT32 Is64BitAddress: 1;
> + //
> + // True when the buffer contains data to be transfered. Otherwise it's the
> + // destination buffer
> + //
> + UINT32 BufferContainsData: 1;
> + UINT32 LocalAddress: 1;
> + UINT32 ElementType: 2;
> + UINT32 EndOfBuffer: 1;
> + UINT32 LastElement: 1;
> + UINT64 DataBufferAddress;
> +} MPT_SG_ENTRY_SIMPLE;
> +#pragma pack ()
> +
> +typedef union {
> +#pragma pack (1)
> + struct {
> + UINT8 TargetID;
(18) TargetId
> + UINT8 Bus;
> + UINT8 MessageLength;
> + UINT8 Function;
> + UINT8 CDBLength;
(19) CdbLength
> + UINT8 SenseBufferLength;
> + UINT8 Reserved;
> + UINT8 MessageFlags;
> + UINT32 MessageContext;
> + UINT8 SCSIStatus;
> + UINT8 SCSIState;
(20) In both fields, s/SCSI/Scsi/ please
> + UINT16 IOCStatus;
> + UINT32 IOCLogInfo;
(21) Same as (13)
> + UINT32 TransferCount;
> + UINT32 SenseCount;
> + UINT32 ResponseInfo;
> + } Data;
> +#pragma pack ()
> + UINT64 Uint64; // 8 byte alignment required by HW
> +} MPT_SCSI_IO_ERROR_REPLY;
> +
> #endif // __FUSION_MPT_SCSI_H__
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method
2020-04-14 17:38 ` [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
@ 2020-04-20 17:30 ` Laszlo Ersek
2020-04-24 17:03 ` Nikita Leshenko
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-20 17:30 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> Machines should be able to boot after this commit. Tested with different
> Linux distributions (Ubuntu, CentOS) and different Windows
> versions (Windows 7, Windows 10, Server 2016).
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
> .../Include/IndustryStandard/FusionMptScsi.h | 19 +-
> OvmfPkg/MptScsiDxe/MptScsi.c | 369 +++++++++++++++++-
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 +
> OvmfPkg/OvmfPkg.dec | 3 +
> 4 files changed, 390 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> index d00a9e6db0bf..4be36adedd8f 100644
> --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> @@ -44,6 +44,15 @@
>
> #define MPT_IOC_WHOINIT_ROM_BIOS 0x02
>
> +#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE (0x00 << 24)
> +#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE (0x01 << 24)
> +#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ (0x02 << 24)
> +
> +#define MPT_SCSI_IOCSTATUS_SUCCESS 0x0000
> +#define MPT_SCSI_IOCSTATUS_DEVICE_NOT_THERE 0x0043
> +#define MPT_SCSI_IOCSTATUS_DATA_OVERRUN 0x0044
> +#define MPT_SCSI_IOCSTATUS_DATA_UNDERRUN 0x0045
> +
> //
> // Device structures
> //
> @@ -141,6 +150,14 @@ typedef union {
> } Data;
> #pragma pack ()
> UINT64 Uint64; // 8 byte alignment required by HW
> -} MPT_SCSI_IO_ERROR_REPLY;
> +} MPT_SCSI_IO_REPLY;
> +
(1) Is it really useful to call this union "MPT_SCSI_IO_ERROR_REPLY"
temporarily, just for patch#10?
Can patch#10 introduce the union as "MPT_SCSI_IO_REPLY" at once?
> +typedef union {
> + struct {
> + MPT_SCSI_IO_REQUEST Header;
> + MPT_SG_ENTRY_SIMPLE Sg;
> + } Data;
(2) Do we intend to prevent padding between "Header" and "Sg"? Because
then we should pack "Data" too.
> + UINT64 Uint64; // 8 byte alignment required by HW
> +} MPT_SCSI_REQUEST_WITH_SG;
>
> #endif // __FUSION_MPT_SCSI_H__
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 9c3bdc430e1a..fcdaa4c338a4 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -17,6 +17,7 @@
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
> #include <Protocol/ScsiPassThruExt.h>
> #include <Uefi/UefiSpec.h>
>
> @@ -30,6 +31,13 @@
> // Runtime Structures
> //
>
> +typedef struct {
> + MPT_SCSI_REQUEST_WITH_SG IoRequest;
> + MPT_SCSI_IO_REPLY IoReply;
> + UINT8 Sense[MAX_UINT8];
> + UINT8 Data[0x2000];
(3) Please refer to "PVSCSI_DMA_BUFFER" in "OvmfPkg/PvScsiDxe/PvScsi.h"
-- the element counts "MAX_UINT8" and "0x2000" are (I think) arbitrary
here too, so similar comments as in "PvScsi.h" would be welcome.
> +} MPT_SCSI_DMA_BUFFER;
> +
> #define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
> typedef struct {
> UINT32 Signature;
> @@ -37,11 +45,19 @@ typedef struct {
> EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
> EFI_PCI_IO_PROTOCOL *PciIo;
> UINT64 OriginalPciAttributes;
> + UINT32 StallPerPollUsec;
> + MPT_SCSI_DMA_BUFFER *Dma;
> + EFI_PHYSICAL_ADDRESS DmaPhysical;
> + VOID *DmaMapping;
> + BOOLEAN IoReplyEnqueued;
> } MPT_SCSI_DEV;
>
> #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
>
> +#define MPT_SCSI_DMA_ADDR(Dev, MemberName) \
> + (Dev->DmaPhysical + OFFSET_OF(MPT_SCSI_DMA_BUFFER, MemberName))
> +
(4) (I missed this in PvScsi, alas.)
Please insert a space character after "OFFSET_OF".
> //
> // Hardware functions
> //
> @@ -142,6 +158,8 @@ MptScsiInit (
> UINT8 *ReplyBytes;
> UINT32 ReplyWord;
>
> + Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
> +
> Status = MptScsiReset (Dev);
> if (EFI_ERROR (Status)) {
> return Status;
> @@ -153,7 +171,7 @@ MptScsiInit (
> Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
> Req.Data.MaxDevices = 1;
> Req.Data.MaxBuses = 1;
> - Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
> + Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_REPLY);
(5) Same as (1).
>
> //
> // Send controller init through doorbell
> @@ -203,6 +221,257 @@ MptScsiInit (
> return EFI_SUCCESS;
> }
>
> +STATIC
> +EFI_STATUS
> +MptScsiPopulateRequest (
> + IN MPT_SCSI_DEV *Dev,
> + IN UINT8 Target,
> + IN UINT64 Lun,
> + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> + )
> +{
> + MPT_SCSI_REQUEST_WITH_SG *Request;
> +
> + Request = &Dev->Dma->IoRequest;
> +
> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
> + (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
> + Packet->CdbLength > sizeof (Request->Data.Header.CDB)) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + if (Target > 0 || Lun > 0 ||
> + 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
> + )
> + )
> + ) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (Packet->InTransferLength > sizeof (Dev->Dma->Data)) {
> + Packet->InTransferLength = sizeof (Dev->Dma->Data);
> + return EFI_BAD_BUFFER_SIZE;
> + }
> + if (Packet->OutTransferLength > sizeof (Dev->Dma->Data)) {
> + Packet->OutTransferLength = sizeof (Dev->Dma->Data);
> + return EFI_BAD_BUFFER_SIZE;
> + }
(6) Please adopt the ReportHostAdapterOverrunError() helper function,
and call, from "PvScsi.c".
> +
> + ZeroMem (Request, sizeof (*Request));
> + Request->Data.Header.TargetID = Target;
> + //
> + // It's 1 and not 0, for some reason...
> + //
(7) I think this comment doesn't add any information. Unless we can
explain why offset 1 is used, I suggest we just remove the comment.
> + Request->Data.Header.LUN[1] = Lun;
(8) This deserves an explicit cast to UINT8 (Lun is UINT64, and Visual
Studio might complain about "loss of precision").
(9) Furthermore, to parallel the comment in "PvScsi.c" ("This cast is
safe as PVSCSI_DEV.MaxLun is defined as UINT8"), we could add a comment
here saying that "only LUN 0 is supported, hence the cast is safe".
> + Request->Data.Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST;
> + Request->Data.Header.MessageContext = 1; // We handle one request at a time
> +
> + Request->Data.Header.CDBLength = Packet->CdbLength;
> + CopyMem (Request->Data.Header.CDB, Packet->Cdb, Packet->CdbLength);
> +
> + //
> + // SenseDataLength is UINT8, Sense[] is MAX_UINT8, so we can't overflow
> + //
> + ZeroMem (&Dev->Dma->Sense, Packet->SenseDataLength);
(10) Style nit: while this is not a bug, we usually just let arrays
decay to pointers to their first elements. Right now you are passing a
pointer to the whole array, not just to the first element. (Put
differently, the type of the first argument is not (UINT8*), but
(UINT8(*)[255]).)
IOW, please drop the ampersand "&" from the first arg of ZeroMem().
> + Request->Data.Header.SenseBufferLength = Packet->SenseDataLength;
> + Request->Data.Header.SenseBufferLowAddress = MPT_SCSI_DMA_ADDR (Dev, Sense);
(11) Please add an explicit (UINT32) cast here; MPT_SCSI_DMA_ADDR()
produces a UINT64 (aka EFI_PHYSICAL_ADDRESS) result.
(12) Does this device support 64-bit DMA?
(12a) If it does, then I think:
- we should explicitly *clear* the DUAL_ADDRESS_CYCLE attribute when
setting up the PCI attributes (--> EfiPciIoAttributeOperationDisable),
- and make a comment here that we cleared DUAL_ADDRESS_CYCLE, therefore
the cast is safe.
(12b) If the device does not support 64-bit DMA, then we should make a
comment about *that*, here (i.e., the cast is safe because the device
does not support 64-bit DMA).
> +
> + Request->Data.Sg.EndOfList = 1;
> + Request->Data.Sg.EndOfBuffer = 1;
> + Request->Data.Sg.LastElement = 1;
> + Request->Data.Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE;
> + Request->Data.Sg.DataBufferAddress = MPT_SCSI_DMA_ADDR (Dev, Data);
Right, this seems to support my concern about 64-bit DMA.
Namely, if the "DataBufferAddress" field were 32-bit, then I'd assume
the device is only 32-bit capable. And then (12b) would apply.
However, because "DataBufferAddress" is 64-bit, it looks like the device
is generally capable of 64-bit DMA, and the 32-bit restriction applies
only to the sense buffer. (This is probably the justification for the
word "Low", in the field name "SenseBuffer*Low*Address").
That means we need to keep the DMA mapping for at least the sense array
in 32-bit space. And because we want a single mapping for the whole
thing, I think (12a) applies.
See EFI_PCI_IO_PROTOCOL.AllocateBuffer() in the UEFI spec:
"""
If the current attributes of the PCI controller has the
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE bit set, then when the buffer
allocated by this function is mapped with a call to Map(), the device
address that is returned by Map() must be within the 64-bit device
address space of the PCI Bus Master. The attributes for a PCI controller
can be managed by calling Attributes().
If the current attributes for the PCI controller has the
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE bit clear, then when the buffer
allocated by this function is mapped with a call to Map(), the device
address that is returned by Map() must be within the 32-bit device
address space of the PCI Bus Master. The attributes for a PCI controller
can be managed by calling Attributes().
"""
(13) I further observe that the "Is64BitAddress" field is cleared (with
the ZeroMem() call on Request), and we can hard-code that -- I believe
anyway -- only if we explicitly clear DUAL_ADDRESS_CYCLE.
> +
> + Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE;
> + switch (Packet->DataDirection)
> + {
> + case EFI_EXT_SCSI_DATA_DIRECTION_READ:
> + if (Packet->InTransferLength == 0) {
> + break;
> + }
> + Request->Data.Header.DataLength = Packet->InTransferLength;
> + Request->Data.Sg.Length = Packet->InTransferLength;
> + Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
> + break;
> + case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
> + if (Packet->OutTransferLength == 0) {
> + break;
> + }
> + Request->Data.Header.DataLength = Packet->OutTransferLength;
> + Request->Data.Sg.Length = Packet->OutTransferLength;
> + Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
> +
> + CopyMem (Dev->Dma->Data, Packet->OutDataBuffer, Packet->OutTransferLength);
> + Request->Data.Sg.BufferContainsData = 1;
> + break;
> + }
(14) I request that we please simplify this switch statement.
My primary reason is that edk2 really dislikes "switch" statements
without "default" case labels. And, from that remark, we can further
simplify this "switch".
Near the top of the function, we exclude both
EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL (EFI_UNSUPPORTED) and
undefined DataDirection values (EFI_INVALID_PARAMETER).
Therefore, here we only need an
if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
...
} else {
...
}
(maybe with a comment on the data directions).
(15) I don't see why hoisting MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE
before the "switch" is helpful. That value can only take effect if the
XxxTransferLength field (matching the direction) is zero. But I don't
think those explicit zero checks buy us much.
I would simply remove the zero comparisons, and always go with either
TXDIR_READ or TXDIR_WRITE (dependent on Packet->DataDirection). And, in
case the data size is zero, then just set / copy zero bytes.
(16) "Sg.Length" is a 24-bit value. It will never overflow when assigned
from XxxTransferLength, due to the initial "EFI_BAD_BUFFER_SIZE" checks
(higher up in the function), which enforce 0x2000 bytes as an upper
limit on XxxTransferLength.
But this guarantee would be nice to spell out somewhere. For example:
//
// "MPT_SG_ENTRY_SIMPLE.Length" is a 24-bit quantity.
//
STATIC_ASSERT (
sizeof (Dev->Dma->Data) < SIZE_16MB,
"MPT_SCSI_DMA_BUFFER.Data must be smaller than 16MB"
);
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiSendRequest (
> + IN MPT_SCSI_DEV *Dev,
> + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> + )
> +{
> + EFI_STATUS Status;
> +
> + if (!Dev->IoReplyEnqueued) {
> + //
> + // Put one free reply frame on the reply queue, the hardware may use it to
> + // report an error to us.
> + //
> + Status = Out32 (Dev, MPT_REG_REP_Q, MPT_SCSI_DMA_ADDR (Dev, IoReply));
(17) Same as (11) -- please explicitly cast the result of
MPT_SCSI_DMA_ADDR() to UINT32.
Furthermore, a comment (according to (12a) vs. (12b)) would be nice,
regarding the safety of the cast.
> + if (EFI_ERROR (Status)) {
> + return EFI_DEVICE_ERROR;
> + }
> + Dev->IoReplyEnqueued = TRUE;
> + }
> +
> + Status = Out32 (Dev, MPT_REG_REQ_Q, MPT_SCSI_DMA_ADDR (Dev, IoRequest));
(18) Same as (17).
> + if (EFI_ERROR (Status)) {
> + return EFI_DEVICE_ERROR;
> + }
(19) Assuming we can expose the reply frame, but not the request, will
the driver continue to behave fine (without explicit rollback actions
for the reply frame)?
Does "IoReplyEnqueued" help with handling this?
(Just a question, not necessarily a code change request.)
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiGetReply (
> + IN MPT_SCSI_DEV *Dev,
> + OUT UINT32 *Reply
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 Istatus;
> + UINT32 EmptyReply;
> +
> + //
> + // Timeouts are not supported for
> + // EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() in this implementation.
> + //
> + for (;;) {
> + Status = In32 (Dev, MPT_REG_ISTATUS, &Istatus);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // Interrupt raised
> + //
> + if (Istatus & MPT_IMASK_REPLY) {
> + break;
> + }
> +
> + gBS->Stall (Dev->StallPerPollUsec);
> + }
> +
> + Status = In32 (Dev, MPT_REG_REP_Q, Reply);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + //
> + // The driver is supposed to fetch replies until 0xffffffff is returned, which
> + // will reset the interrupt status. We put only one request, so we expect the
> + // next read reply to be the last.
> + //
> + Status = In32 (Dev, MPT_REG_REP_Q, &EmptyReply);
> + if (EFI_ERROR (Status) || EmptyReply != MAX_UINT32) {
> + return EFI_DEVICE_ERROR;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiHandleReply (
> + IN MPT_SCSI_DEV *Dev,
> + IN UINT32 Reply,
> + OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> + )
> +{
> + CopyMem (Packet->SenseData, Dev->Dma->Sense, Packet->SenseDataLength);
> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> + CopyMem (Packet->InDataBuffer, Dev->Dma->Data, Packet->InTransferLength);
> + }
> +
> + if (Reply == Dev->Dma->IoRequest.Data.Header.MessageContext) {
> + //
> + // This is a turbo reply, everything is good
> + //
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
(20) Does "turbo reply" mean that you have to update neither
XxxTransferLength, nor SenseDataLength, on output?
> +
> + } else if (Reply & (1 << 31)) {
(21) Please write:
(Reply & BIT31) != 0
You could also consider adding a new macro to
"OvmfPkg/Include/IndustryStandard/FusionMptScsi.h", for this bitmask.
(If you choose to do that, then please use BIT31 there as well.)
> + DEBUG ((DEBUG_ERROR, "%a: request failed\n", __FUNCTION__));
> + //
> + // When reply MSB is set, we got a full reply. Since we submitted only one
> + // reply frame, we know it's IoReply.
> + //
> + Dev->IoReplyEnqueued = FALSE;
> +
> + Packet->TargetStatus = Dev->Dma->IoReply.Data.SCSIStatus;
OK, both are UINT8 objects.
> + Packet->SenseDataLength = Dev->Dma->IoReply.Data.SenseCount;
(22) Please add an explicit cast at least; the LHS is UINT8, the RHS is
UINT32. Visual Studio could emit a warning otherwise.
(Obviously if you know *why* the cast is safe, capturing that in a
comment would also be nice.)
(23) Would it make sense to check that the device only *lowers*
SenseDataLength with the above assignment?
> +
> + switch (Dev->Dma->IoReply.Data.IOCStatus) {
> + case MPT_SCSI_IOCSTATUS_SUCCESS:
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> + break;
(24) This branch seems to lead to an EFI_SUCCESS return code from the
function. (And then it's going to be propagated from MptScsiPassThru(),
IIRC.)
The success return is OK, I believe -- but then the DEBUG_ERROR above
("request failed") seems unjustified. I just feel that the "request
failed" debug message is not consistent with EFI_SUCCESS.
(25) Don't we need to update XxxTransferLength here?
> + case MPT_SCSI_IOCSTATUS_DEVICE_NOT_THERE:
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> + return EFI_TIMEOUT;
> + case MPT_SCSI_IOCSTATUS_DATA_UNDERRUN:
> + if (Packet->TargetStatus == EFI_EXT_SCSI_STATUS_TARGET_GOOD) {
> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> + Packet->InTransferLength = Dev->Dma->IoReply.Data.TransferCount;
> + } else {
> + Packet->OutTransferLength = Dev->Dma->IoReply.Data.TransferCount;
> + }
> + }
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> + return EFI_SUCCESS;
(26) Basically, same as (24) -- do we want to debug-log a short transfer
as "request failed"? In particular a short read doesn't look unusual at all.
(27) XxxTransferLength is modified only if
"Dev->Dma->IoReply.Data.SCSIStatus" brought us
EFI_EXT_SCSI_STATUS_TARGET_GOOD.
I don't think that's a good idea: if "SCSIStatus" is different from
TARGET_GOOD, then we still return EFI_SUCCESS to the caller. And, per spec:
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.
(I think the spec has a small typo above: InTransferLength applies to
read *and* bi-dir commands.)
So the caller will go and check InTransferLength and OutTransferLength,
and look at HostAdapterStatus etc. for "additional" info.
I think TransferCount should be set in all cases. Does the device really
only set TransferCount for TARGET_GOOD?
> + case MPT_SCSI_IOCSTATUS_DATA_OVERRUN:
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> + return EFI_SUCCESS;
(28) Same as (25).
I guess my general point is that, we should update XxxTransferLength
*whenever* we return EFI_SUCCESS.
(29) Style nit:
under MPT_SCSI_IOCSTATUS_SUCCESS, we use "break" for finishing the
function with EFI_SUCCESS. However, under UNDERRUN / OVERRUN, we use
explicit "return EFI_SUCCESS" statements. Please make this easier to
read by sticking with one -- just "break", or "just return EFI_SUCCESS".
> + default:
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + return EFI_DEVICE_ERROR;
> + }
> +
> + } else {
> + DEBUG ((DEBUG_ERROR, "%a: unexpected reply\n", __FUNCTION__));
(30) We could also log Reply itself (%x), but that's just an idea.
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + return EFI_DEVICE_ERROR;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> //
> // Ext SCSI Pass Thru
> //
> @@ -218,7 +487,44 @@ MptScsiPassThru (
> IN EFI_EVENT Event OPTIONAL
> )
> {
> - return EFI_UNSUPPORTED;
> + EFI_STATUS Status;
> + MPT_SCSI_DEV *Dev;
> + UINT32 Reply;
> +
> + Dev = MPT_SCSI_FROM_PASS_THRU (This);
> + Status = MptScsiPopulateRequest (Dev, *Target, Lun, Packet);
(31) Please add a comment that only the first byte of "Target" is used,
by design. (See PopulateRequest() in "PvScsi.c".)
> + if (EFI_ERROR (Status)) {
> + //
> + // MptScsiPopulateRequest modified packet according to the error
> + //
> + return Status;
> + }
> +
> + Status = MptScsiSendRequest (Dev, Packet);
> + if (EFI_ERROR (Status)) {
> + goto Fatal;
> + }
> +
> + Status = MptScsiGetReply (Dev, &Reply);
> + if (EFI_ERROR (Status)) {
> + goto Fatal;
> + }
> +
> + return MptScsiHandleReply (Dev, Reply, Packet);
> +
> +Fatal:
> + //
> + // We erred in the middle of a transaction, a very serious problem has occured
> + // and it's not clear if it's possible to recover without leaving the hardware
> + // in an inconsistent state. Perhaps we would want to reset the device...
> + //
> + DEBUG ((DEBUG_ERROR, "%a: fatal error in scsi request\n", __FUNCTION__));
> + Packet->InTransferLength = 0;
> + Packet->OutTransferLength = 0;
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTED;
> + Packet->SenseDataLength = 0;
> + return EFI_DEVICE_ERROR;
> }
(32) This logic looks OK to me; just please move it to a separate helper
function called ReportHostAdapterError(). Then please replace the "goto
Fatal" statements with "return ReportHostAdapterError()".
See the same function in "PvScsi.c". (I realize that TargetStatus is set
differently -- I'm OK with either value, I'd just like this to be a
separate function.)
>
> STATIC
> @@ -450,6 +756,7 @@ MptScsiControllerStart (
> {
> EFI_STATUS Status;
> MPT_SCSI_DEV *Dev;
> + UINTN BytesMapped;
>
> Dev = AllocateZeroPool (sizeof (*Dev));
> if (Dev == NULL) {
> @@ -494,9 +801,42 @@ MptScsiControllerStart (
> goto CloseProtocol;
> }
>
> + //
> + // Create buffers for data transfer
> + //
> + Status = Dev->PciIo->AllocateBuffer (
> + Dev->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
> + (VOID **)&Dev->Dma,
> + EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> + );
> + if (EFI_ERROR (Status)) {
> + goto RestoreAttributes;
> + }
> +
> + BytesMapped = sizeof (*Dev->Dma);
(33) I think it's slightly more idiomatic to map all pages in full that
were allocated. In "PvScsi.c", a separate "Pages" helper variable (of
type UINTN) is used. So we could do:
UINTN Pages;
Pages = EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma));
AllocateBuffer ( ... Pages ... );
BytesMapped = EFI_PAGES_TO_SIZE (Pages);
Map ( ... &BytesMapped ...);
if (BytesMapped != EFI_PAGES_TO_SIZE (Pages)) {
...
}
(This would also simplify the FreeBuffer() call on the error path; you
could use "Pages" there at once.)
Feel free to disagree -- I'm certainly not saying the code is "wrong".
> + Status = Dev->PciIo->Map (
> + Dev->PciIo,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + Dev->Dma,
> + &BytesMapped,
> + &Dev->DmaPhysical,
> + &Dev->DmaMapping
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBuffer;
> + }
> +
> + if (BytesMapped != sizeof (*Dev->Dma)) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Unmap;
> + }
> +
> Status = MptScsiInit (Dev);
> if (EFI_ERROR (Status)) {
> - goto RestorePciAttributes;
> + goto Unmap;
> }
>
> //
> @@ -531,6 +871,18 @@ MptScsiControllerStart (
> UninitDev:
> MptScsiReset (Dev);
>
> +Unmap:
> + Dev->PciIo->Unmap (
> + Dev->PciIo,
> + Dev->DmaMapping
> + );
> +
> +FreeBuffer:
> + Dev->PciIo->FreeBuffer (
> + Dev->PciIo,
> + EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
> + Dev->Dma
> + );
(34) The indentations of the arguments as well as the closing
parenthesis are wrong.
(35) Please insert an empty line too, before the subsequent
"RestoreAttributes" label.
> RestoreAttributes:
> Dev->PciIo->Attributes (
> Dev->PciIo,
> @@ -592,6 +944,17 @@ MptScsiControllerStop (
>
> MptScsiReset (Dev);
>
> + Dev->PciIo->Unmap (
> + Dev->PciIo,
> + Dev->DmaMapping
> + );
> +
> + Dev->PciIo->FreeBuffer (
> + Dev->PciIo,
> + EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
> + Dev->Dma
> + );
> +
> Dev->PciIo->Attributes (
> Dev->PciIo,
> EfiPciIoAttributeOperationEnable,
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 809f12173bb8..ef1f6a5ebb3a 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -34,3 +34,6 @@ [LibraryClasses]
> [Protocols]
> gEfiPciIoProtocolGuid ## TO_START
> gEfiExtScsiPassThruProtocolGuid ## BY_START
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
(36) General -- can you run "BaseTools/Scripts/SetupGit.py" in your edk2
clone? It will improve the file order in which changes are formatted
into patch files. A patch is easier to read if the INF, DEC, and header
file changes come first.
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 28030391cff2..7fa1581f2101 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -270,6 +270,9 @@ [PcdsFixedAtBuild]
> ## Number of page frames to use for storing grant table entries.
> gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
>
> + ## Microseconds to stall between polling for MptScsi request result
> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x39
> +
(37) Can you introduce this near "PcdPvScsiWaitForCmpStallInUsecs"? I
quite like that the virtio-scsi and PvScsi PCDs are grouped together;
keeping the MPT SCSI ones close would be nice.
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 12/13] OvmfPkg/MptScsiDxe: Report multiple targets
2020-04-14 17:38 ` [PATCH v4 12/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
@ 2020-04-20 18:31 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-20 18:31 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> The controller supports up to 8 targets (Not reported by the
> controller, but based on the implementation of the virtual device),
> report them in GetNextTarget and GetNextTargetLun. The firmware will
> then try to communicate with them and create a block device for each
> one that responds.
>
> Support for multiple LUNs will be implemented in another series.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 38 ++++++++++++++++++++++++-------
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
> OvmfPkg/OvmfPkg.dec | 4 ++++
> 3 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index fcdaa4c338a4..3ea08857df5d 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -46,6 +46,7 @@ typedef struct {
> EFI_PCI_IO_PROTOCOL *PciIo;
> UINT64 OriginalPciAttributes;
> UINT32 StallPerPollUsec;
> + UINT8 MaxTarget;
> MPT_SCSI_DMA_BUFFER *Dma;
> EFI_PHYSICAL_ADDRESS DmaPhysical;
> VOID *DmaMapping;
> @@ -159,6 +160,7 @@ MptScsiInit (
> UINT32 ReplyWord;
>
> Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
> + Dev->MaxTarget = PcdGet8 (PcdMptScsiMaxTargetLimit);
>
> Status = MptScsiReset (Dev);
> if (EFI_ERROR (Status)) {
> @@ -169,7 +171,7 @@ MptScsiInit (
> ZeroMem (&Reply, sizeof (Reply));
> Req.Data.WhoInit = MPT_IOC_WHOINIT_ROM_BIOS;
> Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
> - Req.Data.MaxDevices = 1;
> + Req.Data.MaxDevices = Dev->MaxTarget + 1;
(1) If Dev->MaxTarget is 255, then the RHS will evaluate to 256. But the
LHS can only fit a UINT8.
Can you introduce the PCD in the INF file under a [FixedPcd] section,
and add:
STATIC_ASSERT (FixedPcdGet8 (...) < 255, "...");
to MptScsiInit()?
(I'm unsure (and I'm too tired to check) whether FixedPcdGet8() will
expand to an integer constant. If it doesn't, then STATIC_ASSERT() won't
work. In that case we should likely use a plain ASSERT().)
> Req.Data.MaxBuses = 1;
> Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_REPLY);
>
> @@ -240,7 +242,7 @@ MptScsiPopulateRequest (
> return EFI_UNSUPPORTED;
> }
>
> - if (Target > 0 || Lun > 0 ||
> + if (Target > Dev->MaxTarget || Lun > 0 ||
> Packet->DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
> //
> // Trying to receive, but destination pointer is NULL, or contradicting
> @@ -552,12 +554,22 @@ MptScsiGetNextTargetLun (
> IN OUT UINT64 *Lun
> )
> {
> + MPT_SCSI_DEV *Dev;
> +
> + Dev = MPT_SCSI_FROM_PASS_THRU (This);
> //
> - // Currently support only target 0 LUN 0, so hardcode it
> + // Currently support only LUN 0, so hardcode it
> //
> if (!IsTargetInitialized (*Target)) {
> ZeroMem (*Target, TARGET_MAX_BYTES);
> *Lun = 0;
> + } else if (**Target < Dev->MaxTarget) {
> + //
> + // This device support 256 targets only, so it's enough to increment
> + // the LSB of Target, as it will never overflow.
> + //
> + **Target += 1;
> + *Lun = 0;
> } else {
> return EFI_NOT_FOUND;
> }
(2) Please implement the EFI_INVALID_PARAMETER branch as well, as seen
in PvScsiGetNextTargetLun(). It's a check on input:
if (**Target > Dev->MaxTarget || *Lun > 0) {
return EFI_INVALID_PARAMETER;
}
And then, as a consequence, the "*Lun = 0" assignment is no longer
necessary, near the target increment. (Until you implement multi-LUN.)
To confirm, (Target==MaxTarget) on input should return EFI_NOT_FOUND,
while (Target>MaxTarget) on input should return EFI_INVALID_PARAMETER.
> @@ -573,11 +585,17 @@ MptScsiGetNextTarget (
> IN OUT UINT8 **Target
> )
> {
> - //
> - // Currently support only target 0 LUN 0, so hardcode it
> - //
> + MPT_SCSI_DEV *Dev;
> +
> + Dev = MPT_SCSI_FROM_PASS_THRU (This);
> if (!IsTargetInitialized (*Target)) {
> ZeroMem (*Target, TARGET_MAX_BYTES);
> + } else if (**Target < Dev->MaxTarget) {
> + //
> + // This device support 256 targets only, so it's enough to increment
> + // the LSB of Target, as it will never overflow.
> + //
> + **Target += 1;
> } else {
> return EFI_NOT_FOUND;
> }
(3) Same as (2).
Hmmm. Maybe I should have pointed these out under patch#6
("OvmfPkg/MptScsiDxe: Report one Target and one LUN").
Because, even while we only support Target=0, the EFI_NOT_FOUND and
EFI_INVALID_PARAMETER return values of GetNextTarget() and
GetNextTargetLun(), could be distinguished.
Sorry for missing this under patch#6.
> @@ -595,6 +613,7 @@ MptScsiBuildDevicePath (
> IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
> )
> {
> + MPT_SCSI_DEV *Dev;
> SCSI_DEVICE_PATH *ScsiDevicePath;
>
> if (DevicePath == NULL) {
> @@ -605,7 +624,8 @@ MptScsiBuildDevicePath (
> // This device support 256 targets only, so it's enough to dereference
> // the LSB of Target.
> //
> - if (*Target > 0 || Lun > 0) {
> + Dev = MPT_SCSI_FROM_PASS_THRU (This);
> + if (*Target > Dev->MaxTarget || Lun > 0) {
> return EFI_NOT_FOUND;
> }
>
> @@ -635,6 +655,7 @@ MptScsiGetTargetLun (
> OUT UINT64 *Lun
> )
> {
> + MPT_SCSI_DEV *Dev;
> SCSI_DEVICE_PATH *ScsiDevicePath;
>
> if (DevicePath == NULL ||
> @@ -647,8 +668,9 @@ MptScsiGetTargetLun (
> return EFI_UNSUPPORTED;
> }
>
> + Dev = MPT_SCSI_FROM_PASS_THRU (This);
> ScsiDevicePath = (SCSI_DEVICE_PATH *)DevicePath;
> - if (ScsiDevicePath->Pun > 0 ||
> + if (ScsiDevicePath->Pun > Dev->MaxTarget ||
> ScsiDevicePath->Lun > 0) {
> return EFI_NOT_FOUND;
> }
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index ef1f6a5ebb3a..26aca7f95315 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -37,3 +37,4 @@ [Protocols]
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit ## CONSUMES
(4) Please keep the PCD list alphabetically sorted, and also keep the
"## ... " comments lined up.
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 7fa1581f2101..7b56998abc9b 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -273,6 +273,10 @@ [PcdsFixedAtBuild]
> ## Microseconds to stall between polling for MptScsi request result
> gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x39
>
> + ## Set the *inclusive* number of targets that MptScsi exposes for scan
> + # by ScsiBusDxe.
> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x40
> +
(5) Please keep this near the PcdPvScsi* group of PCDs.
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 13/13] OvmfPkg/MptScsiDxe: Reset device on ExitBootServices()
2020-04-14 17:38 ` [PATCH v4 13/13] OvmfPkg/MptScsiDxe: Reset device on ExitBootServices() Nikita Leshenko
@ 2020-04-20 19:02 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-04-20 19:02 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
On 04/14/20 19:38, Nikita Leshenko wrote:
> This causes the device to forget about the reply frame. We allocated the
> reply frame in EfiBootServicesData type memory, and code executing after
> ExitBootServices() is permitted to overwrite it.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 3ea08857df5d..e632b076fc4a 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -45,6 +45,7 @@ typedef struct {
> EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
> EFI_PCI_IO_PROTOCOL *PciIo;
> UINT64 OriginalPciAttributes;
> + EFI_EVENT ExitBoot;
> UINT32 StallPerPollUsec;
> UINT8 MaxTarget;
> MPT_SCSI_DMA_BUFFER *Dma;
> @@ -696,6 +697,19 @@ MptScsiResetChannel (
> return EFI_UNSUPPORTED;
> }
>
> +STATIC
> +VOID
> +EFIAPI
> +MptScsiExitBoot (
> + IN EFI_EVENT Event,
> + IN VOID *Context
> + )
> +{
> + MPT_SCSI_DEV *Dev;
> +
> + Dev = Context;
> + MptScsiReset (Dev);
> +}
(1) Please copy the DEBUG_VERBOSE message seen in PvScsiExitBoot() into
this function.
With that:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
> STATIC
> EFI_STATUS
> EFIAPI
> @@ -861,6 +875,17 @@ MptScsiControllerStart (
> goto Unmap;
> }
>
> + Status = gBS->CreateEvent (
> + EVT_SIGNAL_EXIT_BOOT_SERVICES,
> + TPL_CALLBACK,
> + &MptScsiExitBoot,
> + Dev,
> + &Dev->ExitBoot
> + );
> + if (EFI_ERROR (Status)) {
> + goto UninitDev;
> + }
> +
> //
> // Host adapter channel, doesn't exist
> //
> @@ -885,11 +910,14 @@ MptScsiControllerStart (
> &Dev->PassThru
> );
> if (EFI_ERROR (Status)) {
> - goto UninitDev;
> + goto CloseExitBoot;
> }
>
> return EFI_SUCCESS;
>
> +CloseExitBoot:
> + gBS->CloseEvent (Dev->ExitBoot);
> +
> UninitDev:
> MptScsiReset (Dev);
>
> @@ -964,6 +992,8 @@ MptScsiControllerStop (
> return Status;
> }
>
> + gBS->CloseEvent (Dev->ExitBoot);
> +
> MptScsiReset (Dev);
>
> Dev->PciIo->Unmap (
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method
2020-04-20 17:30 ` [edk2-devel] " Laszlo Ersek
@ 2020-04-24 17:03 ` Nikita Leshenko
0 siblings, 0 replies; 27+ messages in thread
From: Nikita Leshenko @ 2020-04-24 17:03 UTC (permalink / raw)
To: devel, Laszlo Ersek
Cc: liran.alon, aaron.young, Jordan Justen, Ard Biesheuvel
> On 20 Apr 2020, at 20:30, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 04/14/20 19:38, Nikita Leshenko wrote:
>> Machines should be able to boot after this commit. Tested with different
>> Linux distributions (Ubuntu, CentOS) and different Windows
>> versions (Windows 7, Windows 10, Server 2016).
>>
>> Ref: https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2390__;!!GqivPVa7Brio!JfeG4MsveGF5K9VB4618njXWmMprNv9SIfBg5g6ZHp5jolyqIheckAunOt0SAymJ0j1Ywg$
>> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> ---
>> .../Include/IndustryStandard/FusionMptScsi.h | 19 +-
>> OvmfPkg/MptScsiDxe/MptScsi.c | 369 +++++++++++++++++-
>> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 +
>> OvmfPkg/OvmfPkg.dec | 3 +
>> 4 files changed, 390 insertions(+), 4 deletions(-)
>>
> [...]
>
> (12) Does this device support 64-bit DMA?
Yes it does, but in an inconvenient way. The high 32 bits are passed in the
handshake (SenseBufferHighAddr, HostMfaHighAddr) and the low 32 bits are passed
on the request queue/SenseBufferLowAddress. Now that I think about it again,
since we have only one request, reply and buffer, I'll just enable dual-cycle
and pass the high addresses on initialization.
> [...]
> (15) I don't see why hoisting MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE
> before the "switch" is helpful. That value can only take effect if the
> XxxTransferLength field (matching the direction) is zero. But I don't
> think those explicit zero checks buy us much.
>
> I would simply remove the zero comparisons, and always go with either
> TXDIR_READ or TXDIR_WRITE (dependent on Packet->DataDirection). And, in
> case the data size is zero, then just set / copy zero bytes.
We can't just set/copy zero bytes. If transfer size is zero, we must set
MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE, otherwise the device (at least under
QEMU) will complain. But I'll simplify the code by setting TXDIR_NONE only if
the transfer size is zero.
> [...]
> (19) Assuming we can expose the reply frame, but not the request, will
> the driver continue to behave fine (without explicit rollback actions
> for the reply frame)?
>
> Does "IoReplyEnqueued" help with handling this?
Yes, IoReply is put on a queue and the device may use it to communicate errors
in case the request fails. If the request fails and we get an IoReply, we set
IoReplyEnqueued to FALSE so that we know to enqueue it on the next request. If
we fail while *submitting* the request, the reply frame will remain enqueued
(IoReplyEnqueued == TRUE) and we'll be able to use it for a following request.
>
> (Just a question, not necessarily a code change request.)
>
> [...]
>> + if (Reply == Dev->Dma->IoRequest.Data.Header.MessageContext) {
>> + //
>> + // This is a turbo reply, everything is good
>> + //
>> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
>> + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
>
> (20) Does "turbo reply" mean that you have to update neither
> XxxTransferLength, nor SenseDataLength, on output?
Turbo reply means that the command was successful, which means that we don't
need to update XxxTransferLength. I think we need to zero SenseDataLength
since successful commands don't generate SENSE data.
> [...]
> (23) Would it make sense to check that the device only *lowers*
> SenseDataLength with the above assignment?
Yes, will add that.
> [...]
> I think TransferCount should be set in all cases. Does the device really
> only set TransferCount for TARGET_GOOD?
I assumed that it only updates it when TARGET_GOOD for some reason, but I
checked again and I was wrong. I will read TransferCount unconditionally.
> [...]
>> +[Pcd]
>> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
>
> (36) General -- can you run "BaseTools/Scripts/SetupGit.py" in your edk2
> clone? It will improve the file order in which changes are formatted
> into patch files. A patch is easier to read if the INF, DEC, and header
> file changes come first.
Sorry, I'm pretty sure I ran it in the past, I'll run it again...
ACK on the rest of the comments (also in the other patches).
Nikita
>
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-04-24 17:03 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-14 17:38 [PATCH v4 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
2020-04-15 6:31 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 03/13] OvmfPkg/MptScsiDxe: Report name of driver Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
2020-04-15 6:54 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 07/13] OvmfPkg/MptScsiDxe: Build and decode DevicePath Nikita Leshenko
2020-04-15 12:03 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 08/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
2020-04-16 8:05 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 09/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
2020-04-16 8:11 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
2020-04-16 9:53 ` [edk2-devel] " Laszlo Ersek
2020-04-16 16:00 ` Nikita Leshenko
2020-04-20 11:58 ` Laszlo Ersek
2020-04-20 14:10 ` Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
2020-04-20 17:30 ` [edk2-devel] " Laszlo Ersek
2020-04-24 17:03 ` Nikita Leshenko
2020-04-14 17:38 ` [PATCH v4 12/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
2020-04-20 18:31 ` [edk2-devel] " Laszlo Ersek
2020-04-14 17:38 ` [PATCH v4 13/13] OvmfPkg/MptScsiDxe: Reset device on ExitBootServices() Nikita Leshenko
2020-04-20 19:02 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox