* [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers
@ 2020-03-04 19:22 Nikita Leshenko
2020-03-04 19:22 ` [PATCH v3 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
` (14 more replies)
0 siblings, 15 replies; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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.
We have also developed support for PVSCSI which we will submit in a
separate patch series.
I pushed a copy of these patches to
https://github.com/nikital/edk2/tree/mptscsi_v3
Note that I didn't address Laszlo's comment on v2 about BSD vs
BSD+patent licensing, it needs some internal discussion. I would still
like move forward with the review so I'm submitting v3 with the old
license for now.
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 DevicePath for discovered devices
OvmfPkg/MptScsiDxe: Implement GetTargetLun
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
.../Include/IndustryStandard/FusionMptScsi.h | 162 +++
OvmfPkg/MptScsiDxe/MptScsi.c | 1060 +++++++++++++++++
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 46 +
OvmfPkg/OvmfPkg.dec | 7 +
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32.fdf | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfPkgX64.fdf | 1 +
10 files changed, 1281 insertions(+)
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] 31+ messages in thread
* [PATCH v3 01/13] OvmfPkg/MptScsiDxe: Create empty driver
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:32 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol Nikita Leshenko
` (13 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 30 +++++++++++++++++++++++++++++
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 32 +++++++++++++++++++++++++++++++
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32.fdf | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.fdf | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfPkgX64.fdf | 1 +
8 files changed, 68 insertions(+)
create mode 100644 OvmfPkg/MptScsiDxe/MptScsi.c
create mode 100644 OvmfPkg/MptScsiDxe/MptScsiDxe.inf
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
new file mode 100644
index 000000000000..e1ad0b16b81b
--- /dev/null
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -0,0 +1,30 @@
+/** @file
+
+ This driver produces Extended SCSI Pass Thru Protocol instances for
+ LSI Fusion MPT SCSI devices.
+
+ Copyright (C) 2020, Oracle and/or its affiliates.
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+//
+// 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..91f7e8aa9ea5
--- /dev/null
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -0,0 +1,32 @@
+## @file
+# This driver produces Extended SCSI Pass Thru Protocol instances for
+# LSI Fusion MPT SCSI devices.
+#
+# Copyright (C) 2020, Oracle and/or its affiliates.
+#
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License which accompanies this
+# distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+##
+
+[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 19728f20b34e..6d9c7c92dbd9 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -718,6 +718,7 @@ [Components]
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+ OvmfPkg/MptScsiDxe/MptScsiDxe.inf
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 63607551ed75..548ce0b614dc 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -227,6 +227,7 @@ [FV.DXEFV]
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 3c0c229e3a72..fdaebef73d28 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -731,6 +731,7 @@ [Components.X64]
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+ OvmfPkg/MptScsiDxe/MptScsiDxe.inf
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 0488e5d95ffe..09364102dea6 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -228,6 +228,7 @@ [FV.DXEFV]
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f6c1d8d228c6..ee43202e09c0 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -729,6 +729,7 @@ [Components]
OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
OvmfPkg/XenBusDxe/XenBusDxe.inf
OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+ OvmfPkg/MptScsiDxe/MptScsiDxe.inf
MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 0488e5d95ffe..09364102dea6 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -228,6 +228,7 @@ [FV.DXEFV]
INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
INF OvmfPkg/XenBusDxe/XenBusDxe.inf
INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+INF OvmfPkg/MptScsiDxe/MptScsiDxe.inf
!if $(SECURE_BOOT_ENABLE) == TRUE
INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
2020-03-04 19:22 ` [PATCH v3 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:34 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 03/13] OvmfPkg/MptScsiDxe: Report name of driver Nikita Leshenko
` (12 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 68 ++++++++++++++++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
2 files changed, 68 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index e1ad0b16b81b..087822f71b0f 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -15,6 +15,65 @@
**/
+#include <Library/UefiLib.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
//
@@ -26,5 +85,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 91f7e8aa9ea5..cf072a0b2e75 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -30,3 +30,4 @@ [Packages]
[LibraryClasses]
UefiDriverEntryPoint
+ UefiLib
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 03/13] OvmfPkg/MptScsiDxe: Report name of driver
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
2020-03-04 19:22 ` [PATCH v3 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
2020-03-04 19:22 ` [PATCH v3 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:34 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi Nikita Leshenko
` (11 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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>
---
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 087822f71b0f..2961c3df3c45 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -74,6 +74,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
//
@@ -90,7 +147,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] 31+ messages in thread
* [PATCH v3 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (2 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 03/13] OvmfPkg/MptScsiDxe: Report name of driver Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:36 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
` (10 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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>
---
.../Include/IndustryStandard/FusionMptScsi.h | 29 +++++++++++
OvmfPkg/MptScsiDxe/MptScsi.c | 49 ++++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 5 ++
3 files changed, 82 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..1bd65ed40b1c
--- /dev/null
+++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
@@ -0,0 +1,29 @@
+/** @file
+
+ Macros and type definitions for LSI Fusion MPT SCSI devices.
+
+ Copyright (C) 2020, Oracle and/or its affiliates.
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#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 2961c3df3c45..07f8fe267fc2 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -15,7 +15,11 @@
**/
+#include <IndustryStandard/FusionMptScsi.h>
+#include <IndustryStandard/Pci.h>
+#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
+#include <Protocol/PciIo.h>
//
// Higher versions will be used before lower, 0x10-0xffffffef is the version
@@ -36,7 +40,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 cf072a0b2e75..105af20f325f 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -27,7 +27,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] 31+ messages in thread
* [PATCH v3 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (3 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:42 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN Nikita Leshenko
` (9 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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 | 179 +++++++++++++++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 5 +-
2 files changed, 181 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 07f8fe267fc2..9598b82fda53 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -17,9 +17,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>
//
// Higher versions will be used before lower, 0x10-0xffffffef is the version
@@ -27,6 +30,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
//
@@ -95,7 +201,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
@@ -108,7 +256,34 @@ 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
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ FreePool (Dev);
+
+ return Status;
}
STATIC
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 105af20f325f..a253c5d96916 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -30,9 +30,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] 31+ messages in thread
* [PATCH v3 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (4 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:45 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices Nikita Leshenko
` (8 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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 | 38 +++++++++++++++++++++++++++++--
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 9598b82fda53..e898a6024f73 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -17,6 +17,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>
@@ -62,6 +63,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
@@ -71,7 +88,16 @@ 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;
+ return EFI_SUCCESS;
+ } else {
+ return EFI_NOT_FOUND;
+ }
}
STATIC
@@ -82,7 +108,15 @@ 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);
+ return EFI_SUCCESS;
+ } else {
+ return EFI_NOT_FOUND;
+ }
}
STATIC
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index a253c5d96916..8f366b92eb72 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -30,6 +30,7 @@ [Packages]
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseMemoryLib
DebugLib
MemoryAllocationLib
UefiBootServicesTableLib
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (5 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:47 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 08/13] OvmfPkg/MptScsiDxe: Implement GetTargetLun Nikita Leshenko
` (7 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
ard.biesheuvel
Used to identify the individual disks in the hardware tree
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index e898a6024f73..cba564b5f648 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -129,7 +129,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
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 08/13] OvmfPkg/MptScsiDxe: Implement GetTargetLun
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (6 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:51 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
` (6 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
ard.biesheuvel
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>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index cba564b5f648..a77599dd4b27 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -169,7 +169,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] 31+ messages in thread
* [PATCH v3 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (7 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 08/13] OvmfPkg/MptScsiDxe: Implement GetTargetLun Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:52 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 10/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
` (5 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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>
---
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 a77599dd4b27..f05383e479b6 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -40,6 +40,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) \
@@ -298,6 +299,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
//
@@ -322,11 +335,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);
@@ -368,6 +389,13 @@ MptScsiControllerStop (
);
ASSERT_EFI_ERROR (Status);
+ gBS->CloseProtocol (
+ ControllerHandle,
+ &gEfiPciIoProtocolGuid,
+ This->DriverBindingHandle,
+ ControllerHandle
+ );
+
FreePool (Dev);
return Status;
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 10/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (8 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-04 23:55 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
` (4 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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>
---
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 f05383e479b6..4a52dee902c7 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -41,6 +41,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) \
@@ -311,6 +312,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
//
@@ -335,11 +360,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,
@@ -389,6 +422,13 @@ MptScsiControllerStop (
);
ASSERT_EFI_ERROR (Status);
+ Dev->PciIo->Attributes (
+ Dev->PciIo,
+ EfiPciIoAttributeOperationEnable,
+ Dev->OriginalPciAttributes,
+ NULL
+ );
+
gBS->CloseProtocol (
ControllerHandle,
&gEfiPciIoProtocolGuid,
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (9 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 10/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-05 0:29 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 12/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
` (3 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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 | 115 ++++++++++++
OvmfPkg/MptScsiDxe/MptScsi.c | 168 ++++++++++++++++++
2 files changed, 283 insertions(+)
diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
index 1bd65ed40b1c..1ce2432bd3c2 100644
--- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
+++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
@@ -26,4 +26,119 @@
#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 struct {
+#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;
+ UINT32 BufferContainsData: 1;
+ UINT32 LocalAddress: 1;
+ UINT32 ElementType: 2;
+ UINT32 EndOfBuffer: 1;
+ UINT32 LastElement: 1;
+ UINT64 DataBufferAddress;
+} MPT_SG_ENTRY_SIMPLE;
+#pragma pack ()
+typedef struct {
+#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 4a52dee902c7..37f1ea4b3506 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -47,6 +47,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,
+ 0, // BAR0
+ 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,
+ 0, // BAR0
+ 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 Reply32;
+
+ 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 read produces 16bit of data
+ //
+ ReplyBytes = (UINT8 *)&Reply;
+ while (ReplyBytes != (UINT8 *)(&Reply + 1)) {
+ Status = In32 (Dev, MPT_REG_DOORBELL, &Reply32);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ CopyMem (ReplyBytes, &Reply32, 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
//
@@ -336,6 +497,11 @@ MptScsiControllerStart (
goto CloseProtocol;
}
+ Status = MptScsiInit (Dev);
+ if (EFI_ERROR (Status)) {
+ goto CloseProtocol;
+ }
+
//
// Host adapter channel, doesn't exist
//
@@ -422,6 +588,8 @@ MptScsiControllerStop (
);
ASSERT_EFI_ERROR (Status);
+ MptScsiReset (Dev);
+
Dev->PciIo->Attributes (
Dev->PciIo,
EfiPciIoAttributeOperationEnable,
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 12/13] OvmfPkg/MptScsiDxe: Implement the PassThru method
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (10 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-05 1:35 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 13/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
` (2 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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 | 18 +
OvmfPkg/MptScsiDxe/MptScsi.c | 344 +++++++++++++++++-
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 +
OvmfPkg/OvmfPkg.dec | 3 +
4 files changed, 365 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
index 1ce2432bd3c2..e793f4856d0b 100644
--- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
+++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
@@ -50,6 +50,12 @@
#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_IO_ERROR_IOCSTATUS_DEVICE_NOT_THERE 0x0043
+
//
// Device structures
//
@@ -109,6 +115,10 @@ 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;
@@ -141,4 +151,12 @@ typedef struct {
UINT64 Uint64; // 8 byte alignment required by HW
} MPT_SCSI_IO_ERROR_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 37f1ea4b3506..0985be07bc8e 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -23,6 +23,7 @@
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Protocol/PciIo.h>
+#include <Protocol/PciRootBridgeIo.h>
#include <Protocol/ScsiPassThruExt.h>
//
@@ -35,6 +36,13 @@
// Runtime Structures
//
+typedef struct {
+ MPT_SCSI_IO_ERROR_REPLY IoErrorReply;
+ MPT_SCSI_REQUEST_WITH_SG IoRequest;
+ 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;
@@ -42,11 +50,18 @@ 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;
} 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
//
@@ -147,6 +162,8 @@ MptScsiInit (
UINT8 *ReplyBytes;
UINT32 Reply32;
+ Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
+
Status = MptScsiReset (Dev);
if (EFI_ERROR (Status)) {
return Status;
@@ -205,6 +222,227 @@ MptScsiInit (
return Status;
}
+ //
+ // 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, IoErrorReply));
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ 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->CdbLength > sizeof (Request->Data.Header.CDB)) {
+ return EFI_UNSUPPORTED;
+ }
+
+ if (Target > 0 || Lun > 0) {
+ 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;
+
+ //
+ // Make sure Request is fully written
+ //
+ MemoryFence ();
+
+ Status = Out32 (Dev, MPT_REG_REQ_Q, MPT_SCSI_DMA_ADDR (Dev, IoRequest));
+ if (EFI_ERROR (Status)) {
+ //
+ // We couldn't enqueue the request, report it as an adapter error
+ //
+ Packet->InTransferLength = 0;
+ Packet->OutTransferLength = 0;
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+ Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
+ Packet->SenseDataLength = 0;
+ return EFI_DEVICE_ERROR;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+MptScsiGetReply (
+ IN MPT_SCSI_DEV *Dev,
+ OUT UINT32 *Reply
+ )
+{
+ EFI_STATUS Status;
+ UINT32 Istatus;
+ UINT32 EmptyReply;
+
+ 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
+ )
+{
+ EFI_STATUS Status;
+
+ 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) {
+ //
+ // 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, it's an error frame.
+ //
+
+ switch (Dev->Dma->IoErrorReply.Data.IOCStatus) {
+ case MPT_SCSI_IO_ERROR_IOCSTATUS_DEVICE_NOT_THERE:
+ Packet->HostAdapterStatus =
+ EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
+ break;
+ default:
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+ break;
+ }
+
+ //
+ // Resubmit the reply frame to the reply queue
+ //
+ Status = Out32 (Dev, MPT_REG_REP_Q, MPT_SCSI_DMA_ADDR (Dev, IoErrorReply));
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ } else {
+ DEBUG ((DEBUG_ERROR, "%a: unexpected reply\n", __FUNCTION__));
+ return EFI_DEVICE_ERROR;
+ }
+
return EFI_SUCCESS;
}
@@ -223,7 +461,50 @@ 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)) {
+ return Status;
+ }
+
+ Status = MptScsiSendRequest (Dev, Packet);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+
+ Status = MptScsiGetReply (Dev, &Reply);
+ if (EFI_ERROR (Status)) {
+ goto Fatal;
+ }
+
+ Status = MptScsiHandleReply (Dev, Reply, Packet);
+ if (EFI_ERROR (Status)) {
+ goto Fatal;
+ }
+
+ return Status;
+
+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;
+ if (Packet->HostAdapterStatus == EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK) {
+ 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
@@ -453,6 +734,7 @@ MptScsiControllerStart (
{
EFI_STATUS Status;
MPT_SCSI_DEV *Dev;
+ UINTN BytesMapped;
Dev = AllocateZeroPool (sizeof (*Dev));
if (Dev == NULL) {
@@ -497,9 +779,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 CloseProtocol;
+ goto Unmap;
}
//
@@ -526,11 +841,23 @@ MptScsiControllerStart (
&Dev->PassThru
);
if (EFI_ERROR (Status)) {
- goto RestoreAttributes;
+ goto Unmap;
}
return EFI_SUCCESS;
+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,
@@ -590,6 +917,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 8f366b92eb72..1ba65f2fbbdf 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -40,3 +40,6 @@ [LibraryClasses]
[Protocols]
gEfiPciIoProtocolGuid ## TO_START
gEfiExtScsiPassThruProtocolGuid ## BY_START
+
+[Pcd]
+ gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4c5b6511cb97..7e8097f9952e 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -228,6 +228,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|0x36
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v3 13/13] OvmfPkg/MptScsiDxe: Report multiple targets
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (11 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 12/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
@ 2020-03-04 19:22 ` Nikita Leshenko
2020-03-05 1:41 ` Liran Alon
2020-03-05 23:31 ` [edk2-devel] [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Laszlo Ersek
2020-03-06 20:14 ` Laszlo Ersek
14 siblings, 1 reply; 31+ messages in thread
From: Nikita Leshenko @ 2020-03-04 19:22 UTC (permalink / raw)
To: devel
Cc: Nikita Leshenko, liran.alon, aaron.young, jordan.l.justen, lersek,
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>
---
OvmfPkg/MptScsiDxe/MptScsi.c | 44 ++++++++++++++++++++++++-------
OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
OvmfPkg/OvmfPkg.dec | 4 +++
3 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 0985be07bc8e..dbc765f94136 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -51,6 +51,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;
@@ -163,6 +164,7 @@ MptScsiInit (
UINT32 Reply32;
Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
+ Dev->MaxTarget = PcdGet8 (PcdMptScsiMaxTargetLimit);
Status = MptScsiReset (Dev);
if (EFI_ERROR (Status)) {
@@ -173,7 +175,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_ERROR_REPLY);
@@ -252,7 +254,7 @@ MptScsiPopulateRequest (
return EFI_UNSUPPORTED;
}
- if (Target > 0 || Lun > 0) {
+ if (Target > Dev->MaxTarget || Lun > 0) {
return EFI_INVALID_PARAMETER;
}
@@ -532,16 +534,27 @@ 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;
- return EFI_SUCCESS;
+ } 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;
}
+
+ return EFI_SUCCESS;
}
STATIC
@@ -552,15 +565,22 @@ 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);
- return EFI_SUCCESS;
+ } 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;
}
+
+ return EFI_SUCCESS;
}
STATIC
@@ -573,6 +593,7 @@ MptScsiBuildDevicePath (
IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
)
{
+ MPT_SCSI_DEV *Dev;
SCSI_DEVICE_PATH *ScsiDevicePath;
if (DevicePath == NULL) {
@@ -583,7 +604,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;
}
@@ -613,6 +635,7 @@ MptScsiGetTargetLun (
OUT UINT64 *Lun
)
{
+ MPT_SCSI_DEV *Dev;
SCSI_DEVICE_PATH *ScsiDevicePath;
if (DevicePath == NULL ||
@@ -625,8 +648,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 1ba65f2fbbdf..b21655bc7f81 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -43,3 +43,4 @@ [Protocols]
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit ## CONSUMES
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 7e8097f9952e..1e17df0316a1 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -231,6 +231,10 @@ [PcdsFixedAtBuild]
## Microseconds to stall between polling for MptScsi request result
gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x36
+ ## Set the *inclusive* number of targets that MptScsi exposes for scan
+ # by ScsiBusDxe.
+ gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x37
+
[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
--
2.20.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v3 01/13] OvmfPkg/MptScsiDxe: Create empty driver
2020-03-04 19:22 ` [PATCH v3 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
@ 2020-03-04 23:32 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:32 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, 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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol
2020-03-04 19:22 ` [PATCH v3 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol Nikita Leshenko
@ 2020-03-04 23:34 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:34 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, Nikita Leshenko wrote:
> 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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 03/13] OvmfPkg/MptScsiDxe: Report name of driver
2020-03-04 19:22 ` [PATCH v3 03/13] OvmfPkg/MptScsiDxe: Report name of driver Nikita Leshenko
@ 2020-03-04 23:34 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:34 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel,
Jaben Carsey
On 04/03/2020 21:22, Nikita Leshenko wrote:
> 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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi
2020-03-04 19:22 ` [PATCH v3 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi Nikita Leshenko
@ 2020-03-04 23:36 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:36 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, Nikita Leshenko wrote:
> 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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU
2020-03-04 19:22 ` [PATCH v3 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
@ 2020-03-04 23:42 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:42 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, 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 | 179 +++++++++++++++++++++++++++++-
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 5 +-
> 2 files changed, 181 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 07f8fe267fc2..9598b82fda53 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -17,9 +17,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>
>
> //
> // Higher versions will be used before lower, 0x10-0xffffffef is the version
> @@ -27,6 +30,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)
> +
Nit: I personally prefer to put these structures & macros on a dedicated
header file for driver. i.e. OvmfPkg/MptScsiDxe/MptScsi.h.
Similar to how this is for VirtioScsi (And also upcoming PvScsi)
implementation.
> +//
> +// 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
> //
> @@ -95,7 +201,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;
Shouldn't "|" be on the line before?
> +
> + 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
> @@ -108,7 +256,34 @@ 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
> + );
> + ASSERT_EFI_ERROR (Status);
I think this should be replaced with:
if (EFI_ERROR (Status)) {
return Status;
}
Because otherwise, next line will free Dev->PassThru memory while the
protocol is still registered.
This is also consistent with VirtioScsi implementation.
> +
> + FreePool (Dev);
> +
> + return Status;
> }
>
> STATIC
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 105af20f325f..a253c5d96916 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -30,9 +30,12 @@ [Packages]
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + DebugLib
> + MemoryAllocationLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
> UefiLib
>
> [Protocols]
> - gEfiPciIoProtocolGuid ## TO_START
> + gEfiPciIoProtocolGuid ## TO_START
> + gEfiExtScsiPassThruProtocolGuid ## BY_START
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN
2020-03-04 19:22 ` [PATCH v3 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN Nikita Leshenko
@ 2020-03-04 23:45 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:45 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, Nikita Leshenko wrote:
> 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 | 38 +++++++++++++++++++++++++++++--
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 1 +
> 2 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 9598b82fda53..e898a6024f73 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -17,6 +17,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>
> @@ -62,6 +63,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
> @@ -71,7 +88,16 @@ 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;
> + return EFI_SUCCESS;
> + } else {
> + return EFI_NOT_FOUND;
> + }
Nit: I prefer to remove the "else" clause and just write afterwards
return EFI_NOT_FOUND. To make it clear that all code-flows have return
statements.
In addition, the comment should be above "return EFI_NOT_FOUND;".
> }
>
> STATIC
> @@ -82,7 +108,15 @@ 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);
> + return EFI_SUCCESS;
> + } else {
> + return EFI_NOT_FOUND;
> + }
Same as above.
> }
>
> STATIC
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index a253c5d96916..8f366b92eb72 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -30,6 +30,7 @@ [Packages]
> OvmfPkg/OvmfPkg.dec
>
> [LibraryClasses]
> + BaseMemoryLib
> DebugLib
> MemoryAllocationLib
> UefiBootServicesTableLib
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices
2020-03-04 19:22 ` [PATCH v3 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices Nikita Leshenko
@ 2020-03-04 23:47 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:47 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, Nikita Leshenko wrote:
> Used to identify the individual disks in the hardware tree
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index e898a6024f73..cba564b5f648 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -129,7 +129,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));
Nit: It should also be sufficient to use AllocatePool() as below
overrides all fields of SCSI_DEVICE_PATH.
Reviewed-by: Liran Alon <liran.alon@oracle.com>
> + 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 08/13] OvmfPkg/MptScsiDxe: Implement GetTargetLun
2020-03-04 19:22 ` [PATCH v3 08/13] OvmfPkg/MptScsiDxe: Implement GetTargetLun Nikita Leshenko
@ 2020-03-04 23:51 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:51 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, Nikita Leshenko wrote:
> 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>
> ---
> OvmfPkg/MptScsiDxe/MptScsi.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index cba564b5f648..a77599dd4b27 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -169,7 +169,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);
Why do you need to ZeroMem() if you only use the first byte of Target?
Seems redundant.
In addition, I suggest squashing this patch with the previous one as
GetTargetLun() and BuildDevicePath() kinda complement each other.
Reviewed-by: Liran Alon <liran.alon@oracle.com>
> + //
> + // 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use
2020-03-04 19:22 ` [PATCH v3 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
@ 2020-03-04 23:52 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:52 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, 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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 10/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes
2020-03-04 19:22 ` [PATCH v3 10/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
@ 2020-03-04 23:55 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-04 23:55 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, 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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware
2020-03-04 19:22 ` [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
@ 2020-03-05 0:29 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-05 0:29 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, 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 | 115 ++++++++++++
> OvmfPkg/MptScsiDxe/MptScsi.c | 168 ++++++++++++++++++
> 2 files changed, 283 insertions(+)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> index 1bd65ed40b1c..1ce2432bd3c2 100644
> --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> @@ -26,4 +26,119 @@
> #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 struct {
> +#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
Can you further explain this part? Not sure I understood why this cause
MPT_IO_CONTROLLER_INIT_REQUEST to be 8-byte aligned when used.
In addition, isn't it a bit misleading that it's defined as part of the
structure? E.g. sizeof(MPT_IO_CONTROLLER_INIT_REQUEST) doesn't return
the size of the real INIT_REQUEST.
Examining SeaBIOS mpt-scsi.c driver, the MptIOCInitRequest global
variable doesn't seem to be defined as required to be 8-byte aligned. Is
that a bug in SeaBIOS?
> +} MPT_IO_CONTROLLER_INIT_REQUEST;
Missing new-line here.
> +#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;
Missing new-line.
> +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;
Missing new-line.
> +typedef struct {
> + UINT32 Length: 24;
> + UINT32 EndOfList: 1;
> + UINT32 Is64BitAddress: 1;
> + UINT32 BufferContainsData: 1;
> + UINT32 LocalAddress: 1;
> + UINT32 ElementType: 2;
> + UINT32 EndOfBuffer: 1;
> + UINT32 LastElement: 1;
> + UINT64 DataBufferAddress;
> +} MPT_SG_ENTRY_SIMPLE;
> +#pragma pack ()
Missing new-line.
> +typedef struct {
> +#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
Same comment regarding this as mentioned above.
> +} MPT_SCSI_IO_ERROR_REPLY;
> +
> #endif // __FUSION_MPT_SCSI_H__
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 4a52dee902c7..37f1ea4b3506 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -47,6 +47,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,
> + 0, // BAR0
> + 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,
> + 0, // BAR0
> + 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);
Nit: Add spaces between "|" sides.
> + 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 Reply32;
Nit: I suggest renaming to "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)
This is exactly the case I was worried about when you added a dummy
UINT64 field to MPT_IO_CONTROLLER_INIT_REQUEST for alignment purposes.
As now sizeof (Req) is not really what should be sizeof
(MPT_IO_CONTROLLER_INIT_REQUEST).
I understand this probably work because device probably parse the
request only once you write to BAR0 IOPort the amount of bytes you
specified here.
But it does seem a little bizarre. i.e. Device parse request from less
bytes than actually sent to it.
In addition, where does the alignment request comes from? As it seems
you write this request as a stream of UINT32 to IOPort (Probably
eventually by using "rep outsl").
> + );
> + 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;
> + }
Nit: I think it will be nice wrapping the sending here of INIT_REQUEST
on it's own function that both does MptDoorbell() and writes Req with
PciIo->Io.Write().
> +
> + //
> + // Read reply through doorbell
> + // Each 32bit read produces 16bit of data
> + //
> + ReplyBytes = (UINT8 *)&Reply;
> + while (ReplyBytes != (UINT8 *)(&Reply + 1)) {
> + Status = In32 (Dev, MPT_REG_DOORBELL, &Reply32);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + CopyMem (ReplyBytes, &Reply32, 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
> //
> @@ -336,6 +497,11 @@ MptScsiControllerStart (
> goto CloseProtocol;
> }
>
> + Status = MptScsiInit (Dev);
> + if (EFI_ERROR (Status)) {
> + goto CloseProtocol;
> + }
> +
> //
> // Host adapter channel, doesn't exist
> //
> @@ -422,6 +588,8 @@ MptScsiControllerStop (
> );
> ASSERT_EFI_ERROR (Status);
>
> + MptScsiReset (Dev);
> +
> Dev->PciIo->Attributes (
> Dev->PciIo,
> EfiPciIoAttributeOperationEnable,
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 12/13] OvmfPkg/MptScsiDxe: Implement the PassThru method
2020-03-04 19:22 ` [PATCH v3 12/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
@ 2020-03-05 1:35 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-05 1:35 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, 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 | 18 +
> OvmfPkg/MptScsiDxe/MptScsi.c | 344 +++++++++++++++++-
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 +
> OvmfPkg/OvmfPkg.dec | 3 +
> 4 files changed, 365 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> index 1ce2432bd3c2..e793f4856d0b 100644
> --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> @@ -50,6 +50,12 @@
>
> #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_IO_ERROR_IOCSTATUS_DEVICE_NOT_THERE 0x0043
> +
> //
> // Device structures
> //
> @@ -109,6 +115,10 @@ 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
> + //
Why is this comment added on this patch and not the previous one which
introduced the struct?
> UINT32 BufferContainsData: 1;
> UINT32 LocalAddress: 1;
> UINT32 ElementType: 2;
> @@ -141,4 +151,12 @@ typedef struct {
> UINT64 Uint64; // 8 byte alignment required by HW
> } MPT_SCSI_IO_ERROR_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 37f1ea4b3506..0985be07bc8e 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -23,6 +23,7 @@
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
> #include <Protocol/ScsiPassThruExt.h>
>
> //
> @@ -35,6 +36,13 @@
> // Runtime Structures
> //
>
> +typedef struct {
> + MPT_SCSI_IO_ERROR_REPLY IoErrorReply;
> + MPT_SCSI_REQUEST_WITH_SG IoRequest;
> + 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;
> @@ -42,11 +50,18 @@ 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;
> } 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
> //
> @@ -147,6 +162,8 @@ MptScsiInit (
> UINT8 *ReplyBytes;
> UINT32 Reply32;
>
> + Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
> +
> Status = MptScsiReset (Dev);
> if (EFI_ERROR (Status)) {
> return Status;
> @@ -205,6 +222,227 @@ MptScsiInit (
> return Status;
> }
>
> + //
> + // 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, IoErrorReply));
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + 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->CdbLength > sizeof (Request->Data.Header.CDB)) {
> + return EFI_UNSUPPORTED;
> + }
According to VirtioScsi implementation, you should also check for
bidirectional direction by adding the following condition:
"(Packet->InTransferLength > 0 && Packet->OutTransferLength > 0).
> +
> + if (Target > 0 || Lun > 0) {
> + return EFI_INVALID_PARAMETER;
> + }
According to VirtioScsi implementation, you should also check for the
following contradicting parameters (I don't know if it's really
required...):
//
// 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)
)
)
> +
> + 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;
Why explicitly init Control field to 0? You have already
ZeroMem(Request). Seems redundant.
> + 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;
> +
> + //
> + // Make sure Request is fully written
> + //
> + MemoryFence ();
MemoryFence() shouldn't be required here and thus should be removed.
EDK2 IOPort and MMIO accessors should be responsible for performing
required compiler-barrier and memory-barriers (Based on architecture).
If that's not the case, then this is a bug in EDK2 that should be fixed.
On x86 specific (As we are talking about IOPorts which are x86
specific), IOPort instruction (e.g. "outl") guarantees it is completed
only after all previous reads/writes were completed and performed on
memory (E.g. Store-buffer is flushed). In addition, writes to IOSpace
are guaranteed to never be buffered. See AMD System Programming Manual
Volume 2 section 7.5 Buffering and Combining Memory Writes.
Specifically, it can be seen in
MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c that IoWrite32() use
_ReadWriteBarrier() to perform required compiler-barrier. In contrast,
MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c IoWrite32() seems to only
execute "outl" using gcc inline asm. It seems a bit bizzare to me that
it doesn't specify the "memory" clobber to perform required
compiler-barrier. Lazlo, is this a bug? If so, I can submit a trivial
commit to fix it.
> +
> + Status = Out32 (Dev, MPT_REG_REQ_Q, MPT_SCSI_DMA_ADDR (Dev, IoRequest));
> + if (EFI_ERROR (Status)) {
> + //
> + // We couldn't enqueue the request, report it as an adapter error
> + //
> + Packet->InTransferLength = 0;
> + Packet->OutTransferLength = 0;
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> + Packet->SenseDataLength = 0;
> + return EFI_DEVICE_ERROR;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiGetReply (
> + IN MPT_SCSI_DEV *Dev,
> + OUT UINT32 *Reply
> + )
> +{
> + EFI_STATUS Status;
> + UINT32 Istatus;
> + UINT32 EmptyReply;
> +
Nit: Add a comment here that this loop is infinite as you currently
don't support EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET Timeout.
> + 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
> + )
> +{
> + EFI_STATUS Status;
> +
> + 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);
> + }
You should populate Packet->TargetStatus with
Dev->Dma->IoErrorReply.Data.SCSIStatus.
In theory, you should've also updated Packet->SenseDataLength according
to Dev->Dma->IoErrorReply.Data.SenseCount, and
Packet->InTransferLength/OutTransferLength according to
Dev->Dma->IoErrorReply.Data.TransferCount. This is documented in these
EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET documentation.
However, examining VirtualBox implementation for this device
(https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Devices/Storage/DevLsiLogicSCSI.cpp)
seems to reveal that it only updates SenseCount and TransferCount fields
on I/O error. Therefore, it seems that the above suggestion won't work
on this device emulation.
I would result to at least documenting why you don't update these fields
if this is the case. i.e. To preserve working on buggy device emulation
implementations.
> +
> + if (Reply == Dev->Dma->IoRequest.Data.Header.MessageContext) {
> + //
> + // 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, it's an error frame.
> + //
> +
> + switch (Dev->Dma->IoErrorReply.Data.IOCStatus) {
> + case MPT_SCSI_IO_ERROR_IOCSTATUS_DEVICE_NOT_THERE:
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> + break;
> + default:
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + break;
> + }
> +
> + //
> + // Resubmit the reply frame to the reply queue
> + //
> + Status = Out32 (Dev, MPT_REG_REP_Q, MPT_SCSI_DMA_ADDR (Dev, IoErrorReply));
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + } else {
> + DEBUG ((DEBUG_ERROR, "%a: unexpected reply\n", __FUNCTION__));
> + return EFI_DEVICE_ERROR;
> + }
> +
> return EFI_SUCCESS;
> }
>
> @@ -223,7 +461,50 @@ 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)) {
> + return Status;
> + }
> +
> + Status = MptScsiSendRequest (Dev, Packet);
> + if (EFI_ERROR (Status)) {
Nit: I suggest adding here a comment that in case of failure,
MptScsiSendRequest() have modified Packet fields accordingly. As it's
not immediately obvious.
> + return Status;
> + }
> +
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +
> + Status = MptScsiGetReply (Dev, &Reply);
> + if (EFI_ERROR (Status)) {
> + goto Fatal;
> + }
> +
> + Status = MptScsiHandleReply (Dev, Reply, Packet);
> + if (EFI_ERROR (Status)) {
> + goto Fatal;
> + }
> +
> + return Status;
> +
> +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;
> + if (Packet->HostAdapterStatus == EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK) {
> + 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
> @@ -453,6 +734,7 @@ MptScsiControllerStart (
> {
> EFI_STATUS Status;
> MPT_SCSI_DEV *Dev;
> + UINTN BytesMapped;
>
> Dev = AllocateZeroPool (sizeof (*Dev));
> if (Dev == NULL) {
> @@ -497,9 +779,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;
> + }
> +
Nit: I suggest introducing the DMA communication buffer in a separate
patch. It ease review and makes code easier to bisect.
> Status = MptScsiInit (Dev);
> if (EFI_ERROR (Status)) {
> - goto CloseProtocol;
> + goto Unmap;
> }
>
> //
> @@ -526,11 +841,23 @@ MptScsiControllerStart (
> &Dev->PassThru
> );
> if (EFI_ERROR (Status)) {
> - goto RestoreAttributes;
> + goto Unmap;
> }
>
> return EFI_SUCCESS;
>
> +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,
> @@ -590,6 +917,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 8f366b92eb72..1ba65f2fbbdf 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -40,3 +40,6 @@ [LibraryClasses]
> [Protocols]
> gEfiPciIoProtocolGuid ## TO_START
> gEfiExtScsiPassThruProtocolGuid ## BY_START
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 4c5b6511cb97..7e8097f9952e 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -228,6 +228,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|0x36
> +
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v3 13/13] OvmfPkg/MptScsiDxe: Report multiple targets
2020-03-04 19:22 ` [PATCH v3 13/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
@ 2020-03-05 1:41 ` Liran Alon
0 siblings, 0 replies; 31+ messages in thread
From: Liran Alon @ 2020-03-05 1:41 UTC (permalink / raw)
To: Nikita Leshenko, devel
Cc: aaron.young, jordan.l.justen, lersek, ard.biesheuvel
On 04/03/2020 21:22, 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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (12 preceding siblings ...)
2020-03-04 19:22 ` [PATCH v3 13/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
@ 2020-03-05 23:31 ` Laszlo Ersek
2020-03-06 20:14 ` Laszlo Ersek
14 siblings, 0 replies; 31+ messages in thread
From: Laszlo Ersek @ 2020-03-05 23:31 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, jordan.l.justen, ard.biesheuvel
On 03/04/20 20:22, Nikita Leshenko wrote:
> 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.
>
> We have also developed support for PVSCSI which we will submit in a
> separate patch series.
>
> I pushed a copy of these patches to
> https://github.com/nikital/edk2/tree/mptscsi_v3
>
> Note that I didn't address Laszlo's comment on v2 about BSD vs
> BSD+patent licensing, it needs some internal discussion. I would still
> like move forward with the review so I'm submitting v3 with the old
> license for now.
Sorry, this doesn't work for me.
You seem to have removed the old "Contributed-under: TianoCore
Contribution Agreement 1.1" lines from the commit messages, and that's
great.
(My understanding is that those lines are now deal-breakers, because
said "Contribution Agreement" is no longer in effect, or even described
in the project, except in the "License-History.txt" file.)
What does not work for me is reviewing a patch set that the submitter
*knows* is unmergeable. I absolutely don't have time for that. Please
submit a patch set that you honestly believe can be merged as-is.
To be clear, the 2-Clause BSD License (SPDX short identifier:
BSD-2-Clause) *is* acceptable, according to "Readme.md"; and I'm not
trying to force you to contribute under "SPDX-License-Identifier:
BSD-2-Clause-Patent".
However, I explained at [1] that "Readme.md" contains the following
passage:
> The majority of the content in the EDK II open source project uses a
> [BSD-2-Clause Plus Patent License](License.txt). The EDK II open source project
> contains the following components that are covered by additional licenses:
> * [...]
> * [OvmfPkg](OvmfPkg/License.txt)
> * [...]
and I asked that you please extend "OvmfPkg/License.txt", should you
prefer to make this contribution under "BSD-2-Clause".
[1] http://mid.mail-archive.com/a202d92e-61e1-187b-be47-e60ad282c575@redhat.com
https://edk2.groups.io/g/devel/message/55049
This (v3) posting is under "BSD-2-Clause" (which is fine), but the
cumulative diffstat does not mention "OvmfPkg/License.txt", against my
express request. Similarly counter to my express request, you have not
adopted the SPDX notation even for "BSD-2-Clause".
I think you may have thought that we could make progress on the
technical details while you figured out your preferred license, and in
the end, you'd *unconditionally* repost the series (even if it were
technically perfect at v3), with one of the following modifications:
- you'd stick with "BSD-2-Clause", and extend "OvmfPkg/License.txt",
- or else you'd switch to "BSD-2-Clause-Patent".
To be clear, this approach does not work for me. I don't have time for
spurious reviews. When you post v(n+1) of the series, I have to:
- fetch that from your repo and/or apply it from the list,
- pull up my review notes that I had given for v(n),
- compare every single patch in the v(n+1) series against the v(n)
counterpart, and verify that your changes are in sync with my requests
-- even if my only feedback for v(n) was a "Reviewed-by",
- and generally page-in the whole topic against a "cold cache", possibly
from a distance of a week or more.
I can't do this *spuriously*. The bottleneck is at the review side, not
at the contribution side.
Of course, people do sometimes post RFC patches (marked as such). That's
a great tool to discuss prototypes and new ideas. I give RFC series a
*fraction* of the attention that I give to real PATCH series. I might
ignore RFCs completely.
Please post v4 with either the license flipped to "BSD-2-Clause-Patent",
or with "OvmfPkg/License.txt" modified. If you need time to decide,
please post v4 when you have decided.
Per <https://bugzilla.tianocore.org/show_bug.cgi?id=2390>:
- Your v1 posting was in January 2019,
- you announced starting work on v2 in November 2019,
- you posted v2 in February 2020.
In response to every one of the v1 through v3 postings, I followed up in
at most 3 days, as much as I can tell. I think we can now wait for a
week or two until your Legal Department figures out the license under
which they would like you to make this contribution.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
` (13 preceding siblings ...)
2020-03-05 23:31 ` [edk2-devel] [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Laszlo Ersek
@ 2020-03-06 20:14 ` Laszlo Ersek
2020-03-06 21:52 ` Liran Alon
14 siblings, 1 reply; 31+ messages in thread
From: Laszlo Ersek @ 2020-03-06 20:14 UTC (permalink / raw)
To: devel, nikita.leshchenko
Cc: liran.alon, aaron.young, jordan.l.justen, ard.biesheuvel
Hi Nikita,
On 03/04/20 20:22, Nikita Leshenko wrote:
> 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.
>
> We have also developed support for PVSCSI which we will submit in a
> separate patch series.
I'd like to learn more of this general agenda ("feature parity with
SeaBIOS"). I have never felt the need for any SCSI controller offered by
QEMU other than virtio-scsi.
Because you guys are contributing Fusion-MPT and PVSCSI to OVMF,
obviously such a practical need must exist ("feature parity with
SeaBIOS" is vague, I'm not really buying it :) ).
So I have two requests:
(1) please describe the actual use case (hypervisor, guest OS, maybe
performance, etc) for these drivers, in the associated bugzilla,
(2) please make the inclusion of these drivers in the OVMF DSC and FDF
files dependent on a new build flag (-D). It's up to you whether you
want to gate PVSCSI and Fusion-MPT with the same flag, or if you want to
assign separate flags to them.
It's fine if the default value is TRUE, for that flag (those flags). I'm
asking for them from a downstream perspective -- some distros follow a
"we ship it, we support it" model, and so they must be careful with
rebases to new usptream releases. I'd like to permit such downstreams to
easily disable these drivers, with just -D flags, without
downstream-only patches for the OVMF DSC and FDF files, that might need
repeated downstream rebasing and review.
Again, it's perfectly fine if the upstream defaults are TRUE.
... If you feel tempted to point out the Xen paravirt drivers: you are
entirely right, but those are already covered by
<https://bugzilla.tianocore.org/show_bug.cgi?id=2122>.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers
2020-03-06 20:14 ` Laszlo Ersek
@ 2020-03-06 21:52 ` Liran Alon
2020-03-06 22:24 ` Laszlo Ersek
0 siblings, 1 reply; 31+ messages in thread
From: Liran Alon @ 2020-03-06 21:52 UTC (permalink / raw)
To: Laszlo Ersek, devel, nikita.leshchenko
Cc: aaron.young, jordan.l.justen, ard.biesheuvel
Hi Lazlo,
On 06/03/2020 22:14, Laszlo Ersek wrote:
> Hi Nikita,
>
> On 03/04/20 20:22, Nikita Leshenko wrote:
>> 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.
>>
>> We have also developed support for PVSCSI which we will submit in a
>> separate patch series.
> I'd like to learn more of this general agenda ("feature parity with
> SeaBIOS"). I have never felt the need for any SCSI controller offered by
> QEMU other than virtio-scsi.
>
> Because you guys are contributing Fusion-MPT and PVSCSI to OVMF,
> obviously such a practical need must exist ("feature parity with
> SeaBIOS" is vague, I'm not really buying it :) ).
The motivation behind supporting booting from these devices is being
able to run VMs that originally run on other hypervisors, such as VMware
ESXi, on top of QEMU/KVM without any changes to the image (In contrast
to approaches such as virt-v2v, Amazon CloudEndure or Google
Velostrata). This technology was developed in Ravello Systems (Acquired
by Oracle), which offered a product to run VMware-based images on top of
any public cloud without any modification to the VMs.
This is also why you would also see QEMU PVSCSI device emulation and
SeaBIOS PVSCSI driver were contributed by Ravello as-well.
Similar work to Ravello was done by Virtuozzo for Hyper-V based VMs (See
their enhancements to QEMU of emulating Hyper-V PV devices). They also
have EDK2 drivers for these devices that are not upstream.
Having said that, it is true that these two patch-series provide feature
parity with SeaBIOS. I do believe that it makes sense OVMF will support
booting from any storage device that QEMU is able to emulate. Especially
if SeaBIOS is already able to boot from that device as-well.
>
> So I have two requests:
>
> (1) please describe the actual use case (hypervisor, guest OS, maybe
> performance, etc) for these drivers, in the associated bugzilla,
I don't think the description we have written in the relevant BugZilla
tickets should change much. It's still true that these are contributed
as part of feature-parity with SeaBIOS and allowing to boot from these
devices. How one use this depends on it's own use-case. It could be to
be able to boot VMware-based VMs as-is under QEMU/KVM, but it could also
be used to boot a VM with a PVSCSI controller that was originally
created with QEMU/KVM.
This is similar to upstream commit c137d9508169 ("OvmfPkg/QemuVideoDxe:
VMWare SVGA device support") which added support for VMware-SVGA GPU.
>
> (2) please make the inclusion of these drivers in the OVMF DSC and FDF
> files dependent on a new build flag (-D). It's up to you whether you
> want to gate PVSCSI and Fusion-MPT with the same flag, or if you want to
> assign separate flags to them.
>
> It's fine if the default value is TRUE, for that flag (those flags). I'm
> asking for them from a downstream perspective -- some distros follow a
> "we ship it, we support it" model, and so they must be careful with
> rebases to new usptream releases. I'd like to permit such downstreams to
> easily disable these drivers, with just -D flags, without
> downstream-only patches for the OVMF DSC and FDF files, that might need
> repeated downstream rebasing and review.
>
> Again, it's perfectly fine if the upstream defaults are TRUE.
>
> ... If you feel tempted to point out the Xen paravirt drivers: you are
> entirely right, but those are already covered by
> <https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2122__;!!GqivPVa7Brio!JJpn5PCAL9AX7xdt4qLhQJMF7L19-QEEbGF_o9bHTGsccK6a5AeDD0-_j3_UPCk$ >.
Funny. I was about to point out that the Xen PV drivers are not
conditioned by any flag! :)
OK. I think it's appropriate we will add a separate flag for each device
and make them both TRUE by default (As I think should be appropriate for
any device built-in by default into QEMU).
Thanks,
-Liran
>
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers
2020-03-06 21:52 ` Liran Alon
@ 2020-03-06 22:24 ` Laszlo Ersek
0 siblings, 0 replies; 31+ messages in thread
From: Laszlo Ersek @ 2020-03-06 22:24 UTC (permalink / raw)
To: Liran Alon, devel, nikita.leshchenko
Cc: aaron.young, jordan.l.justen, ard.biesheuvel
On 03/06/20 22:52, Liran Alon wrote:
>
> Hi Lazlo,
("Laszlo")
> On 06/03/2020 22:14, Laszlo Ersek wrote:
>> Hi Nikita,
>>
>> On 03/04/20 20:22, Nikita Leshenko wrote:
>>> 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.
>>>
>>> We have also developed support for PVSCSI which we will submit in a
>>> separate patch series.
>> I'd like to learn more of this general agenda ("feature parity with
>> SeaBIOS"). I have never felt the need for any SCSI controller offered by
>> QEMU other than virtio-scsi.
>>
>> Because you guys are contributing Fusion-MPT and PVSCSI to OVMF,
>> obviously such a practical need must exist ("feature parity with
>> SeaBIOS" is vague, I'm not really buying it :) ).
>
> The motivation behind supporting booting from these devices is being
> able to run VMs that originally run on other hypervisors, such as VMware
> ESXi, on top of QEMU/KVM without any changes to the image (In contrast
> to approaches such as virt-v2v, Amazon CloudEndure or Google
> Velostrata). This technology was developed in Ravello Systems (Acquired
> by Oracle), which offered a product to run VMware-based images on top of
> any public cloud without any modification to the VMs.
> This is also why you would also see QEMU PVSCSI device emulation and
> SeaBIOS PVSCSI driver were contributed by Ravello as-well.
>
> Similar work to Ravello was done by Virtuozzo for Hyper-V based VMs (See
> their enhancements to QEMU of emulating Hyper-V PV devices). They also
> have EDK2 drivers for these devices that are not upstream.
Thank you for the detailed explanation. Very helpful.
>
> Having said that, it is true that these two patch-series provide feature
> parity with SeaBIOS. I do believe that it makes sense OVMF will support
> booting from any storage device that QEMU is able to emulate. Especially
> if SeaBIOS is already able to boot from that device as-well.
Yes, as long as a user can control relatively fine-grained what support
they'd like to include in the firmware binary.
>
>>
>> So I have two requests:
>>
>> (1) please describe the actual use case (hypervisor, guest OS, maybe
>> performance, etc) for these drivers, in the associated bugzilla,
>
> I don't think the description we have written in the relevant BugZilla
> tickets should change much. It's still true that these are contributed
> as part of feature-parity with SeaBIOS and allowing to boot from these
> devices. How one use this depends on it's own use-case. It could be to
> be able to boot VMware-based VMs as-is under QEMU/KVM, but it could also
> be used to boot a VM with a PVSCSI controller that was originally
> created with QEMU/KVM.
Right. All I'm going to do now is to link your message in the mailing
list archive in a new BZ comment (on both BZs).
> This is similar to upstream commit c137d9508169 ("OvmfPkg/QemuVideoDxe:
> VMWare SVGA device support") which added support for VMware-SVGA GPU.
Yes, I agree about the similarity.
(Two side comments about that commit:
- the commit explains the motivation in the message ("Drivers for this
device exist for some guest OSes which do not support Qemu's other
display adapters, so supporting it in OVMF is useful in conjunction with
those OSes").
- that commit ended up being reverted (even though not because the use
case got invalidated, but because it was replaced by a simpler solution
-- see commit range 7f3b0bad4bbb..d021868ccf49).)
>
>>
>> (2) please make the inclusion of these drivers in the OVMF DSC and FDF
>> files dependent on a new build flag (-D). It's up to you whether you
>> want to gate PVSCSI and Fusion-MPT with the same flag, or if you want to
>> assign separate flags to them.
>>
>> It's fine if the default value is TRUE, for that flag (those flags). I'm
>> asking for them from a downstream perspective -- some distros follow a
>> "we ship it, we support it" model, and so they must be careful with
>> rebases to new usptream releases. I'd like to permit such downstreams to
>> easily disable these drivers, with just -D flags, without
>> downstream-only patches for the OVMF DSC and FDF files, that might need
>> repeated downstream rebasing and review.
>>
>> Again, it's perfectly fine if the upstream defaults are TRUE.
>>
>> ... If you feel tempted to point out the Xen paravirt drivers: you are
>> entirely right, but those are already covered by
>> <https://urldefense.com/v3/__https://bugzilla.tianocore.org/show_bug.cgi?id=2122__;!!GqivPVa7Brio!JJpn5PCAL9AX7xdt4qLhQJMF7L19-QEEbGF_o9bHTGsccK6a5AeDD0-_j3_UPCk$
>> >.
>
> Funny. I was about to point out that the Xen PV drivers are not
> conditioned by any flag! :)
:)
> OK. I think it's appropriate we will add a separate flag for each device
> and make them both TRUE by default (As I think should be appropriate for
> any device built-in by default into QEMU).
Thank you very much!
Laszlo
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-03-06 22:24 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-04 19:22 [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Nikita Leshenko
2020-03-04 19:22 ` [PATCH v3 01/13] OvmfPkg/MptScsiDxe: Create empty driver Nikita Leshenko
2020-03-04 23:32 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 02/13] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol Nikita Leshenko
2020-03-04 23:34 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 03/13] OvmfPkg/MptScsiDxe: Report name of driver Nikita Leshenko
2020-03-04 23:34 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 04/13] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi Nikita Leshenko
2020-03-04 23:36 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 05/13] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU Nikita Leshenko
2020-03-04 23:42 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 06/13] OvmfPkg/MptScsiDxe: Report one Target and one LUN Nikita Leshenko
2020-03-04 23:45 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 07/13] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices Nikita Leshenko
2020-03-04 23:47 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 08/13] OvmfPkg/MptScsiDxe: Implement GetTargetLun Nikita Leshenko
2020-03-04 23:51 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use Nikita Leshenko
2020-03-04 23:52 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 10/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes Nikita Leshenko
2020-03-04 23:55 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 11/13] OvmfPkg/MptScsiDxe: Initialize hardware Nikita Leshenko
2020-03-05 0:29 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 12/13] OvmfPkg/MptScsiDxe: Implement the PassThru method Nikita Leshenko
2020-03-05 1:35 ` Liran Alon
2020-03-04 19:22 ` [PATCH v3 13/13] OvmfPkg/MptScsiDxe: Report multiple targets Nikita Leshenko
2020-03-05 1:41 ` Liran Alon
2020-03-05 23:31 ` [edk2-devel] [PATCH v3 00/13] OvmfPkg: Support booting from Fusion-MPT SCSI controllers Laszlo Ersek
2020-03-06 20:14 ` Laszlo Ersek
2020-03-06 21:52 ` Liran Alon
2020-03-06 22:24 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox