OvmfPkg: Support booting from Fusion-MPT SCSI controllers

OvmfPkg: Support booting from Fusion-MPT SCSI controllers

[Nikita Leshenko]

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.

Thanks,
Nikita

[PATCH 01/14] OvmfPkg/MptScsiDxe: Create empty driver

[Nikita Leshenko]

In preparation for implementing LSI Fusion MPT SCSI devices, create a
basic scaffolding for a driver.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c      | 30 ++++++++++++++++++++++++++++
 OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 33 +++++++++++++++++++++++++++++++
 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, 69 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 0000000000..73a693741d
--- /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) 2018, Oracle and/or its affiliates. All rights reserved.
+
+  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 0000000000..c558d78034
--- /dev/null
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -0,0 +1,33 @@
+## @file
+# This driver produces Extended SCSI Pass Thru Protocol instances for
+# LSI Fusion MPT SCSI devices.
+#
+# Copyright (C) 2018, Oracle and/or its affiliates. All rights reserved.
+#
+# 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                    = 0x00010005
+  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
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  UefiDriverEntryPoint
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index aee19b75d7..52458859b6 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -708,6 +708,7 @@
   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 e013099136..73e36636e0 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -233,6 +233,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
 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 90cbd8e341..d8ea2addb2 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -717,6 +717,7 @@
   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 afaa334384..e22a223e7e 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
 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 83d16eb00b..daf03cd1b5 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -715,6 +715,7 @@
   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 afaa334384..e22a223e7e 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
 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

Re: [PATCH 01/14] OvmfPkg/MptScsiDxe: Create empty driver

[Bi, Dandan]

Hi Nikita,

I have one comment here, could you help to verify this patch series with VS tool chain build before commit the patches?


Thanks,
Dandan

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Nikita Leshenko
> Sent: Thursday, January 31, 2019 6:07 PM
> To: edk2-devel@lists.01.org
> Cc: liran.alon@oracle.com
> Subject: [edk2] [PATCH 01/14] OvmfPkg/MptScsiDxe: Create empty driver
> 
> In preparation for implementing LSI Fusion MPT SCSI devices, create a basic
> scaffolding for a driver.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 30 ++++++++++++++++++++++++++++
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 33
> +++++++++++++++++++++++++++++++
>  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, 69 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
> 0000000000..73a693741d
> --- /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) 2018, Oracle and/or its affiliates. All rights reserved.
> +
> +  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 0000000000..c558d78034
> --- /dev/null
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -0,0 +1,33 @@
> +## @file
> +# This driver produces Extended SCSI Pass Thru Protocol instances for #
> +LSI Fusion MPT SCSI devices.
> +#
> +# Copyright (C) 2018, Oracle and/or its affiliates. All rights reserved.
> +#
> +# 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                    = 0x00010005
> +  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
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  UefiDriverEntryPoint
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index
> aee19b75d7..52458859b6 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -708,6 +708,7 @@
>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +  OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> 
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCoun
> terRuntimeDxe.inf
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index
> e013099136..73e36636e0 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -233,6 +233,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>  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/SecureBootConfi
> gDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 90cbd8e341..d8ea2addb2 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -717,6 +717,7 @@
>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +  OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> 
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCoun
> terRuntimeDxe.inf
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index afaa334384..e22a223e7e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>  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/SecureBootConfi
> gDxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index
> 83d16eb00b..daf03cd1b5 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -715,6 +715,7 @@
>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
> +  OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
> 
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCoun
> terRuntimeDxe.inf
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index
> afaa334384..e22a223e7e 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>  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/SecureBootConfi
> gDxe.inf
> --
> 2.20.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 01/14] OvmfPkg/MptScsiDxe: Create empty driver

[Laszlo Ersek]

On 02/01/19 03:16, Bi, Dandan wrote:
> Hi Nikita,
> 
> I have one comment here, could you help to verify this patch series with VS tool chain build before commit the patches?

(1) Nikita, when you post the v2 series, please push it to a public
repo/branch of yours as wel. So that developers with access to the VS
toolchain can test-build your work for us (assuming you can't build with
those toolchains -- I know I can't).

(2) A note on topic branches, in advance -- when you push an updated
version, please don't force-push earlier versions; instead, push to a
branch called <topic_branch>_v<n>.

(3) Some more administrativa -- please register in the TianoCore
Bugzilla at <https://bugzilla.tianocore.org/>, file a feature request
for this work (Product := TianoCore Feature Requests), and please assign
it to yourself. Status should be IN_PROGRESS.

(4) The commit messages should reference the BZ.

(5) I also prefer if every posting (the blurb of it) is mentioned
separately in the bugzilla, in a new comment; with subject line and
mailing list archive URL. That way people can easily re-trace the
evolution of the feature months or years later, by going through the BZ.

(6) This seems like an important feature; I think it should be mentioned in

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#proposed-features

Once we have a BZ, I can add an item to the list. (I'm not exactly sure
how "editable" the wiki is to new developers. We can figure that out later.)

Thanks,
Laszlo



>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Nikita Leshenko
>> Sent: Thursday, January 31, 2019 6:07 PM
>> To: edk2-devel@lists.01.org
>> Cc: liran.alon@oracle.com
>> Subject: [edk2] [PATCH 01/14] OvmfPkg/MptScsiDxe: Create empty driver
>>
>> In preparation for implementing LSI Fusion MPT SCSI devices, create a basic
>> scaffolding for a driver.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Reviewed-by: Aaron Young <aaron.young@oracle.com>
>> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>  OvmfPkg/MptScsiDxe/MptScsi.c      | 30 ++++++++++++++++++++++++++++
>>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 33
>> +++++++++++++++++++++++++++++++
>>  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, 69 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
>> 0000000000..73a693741d
>> --- /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) 2018, Oracle and/or its affiliates. All rights reserved.
>> +
>> +  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 0000000000..c558d78034
>> --- /dev/null
>> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>> @@ -0,0 +1,33 @@
>> +## @file
>> +# This driver produces Extended SCSI Pass Thru Protocol instances for #
>> +LSI Fusion MPT SCSI devices.
>> +#
>> +# Copyright (C) 2018, Oracle and/or its affiliates. All rights reserved.
>> +#
>> +# 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                    = 0x00010005
>> +  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
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> +  UefiDriverEntryPoint
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index
>> aee19b75d7..52458859b6 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -708,6 +708,7 @@
>>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>> +  OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>>
>> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCoun
>> terRuntimeDxe.inf
>>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index
>> e013099136..73e36636e0 100644
>> --- a/OvmfPkg/OvmfPkgIa32.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32.fdf
>> @@ -233,6 +233,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>>  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/SecureBootConfi
>> gDxe.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 90cbd8e341..d8ea2addb2 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -717,6 +717,7 @@
>>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>> +  OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>>
>> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCoun
>> terRuntimeDxe.inf
>>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
>> index afaa334384..e22a223e7e 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
>> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
>> @@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>>  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/SecureBootConfi
>> gDxe.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index
>> 83d16eb00b..daf03cd1b5 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -715,6 +715,7 @@
>>    OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
>>    OvmfPkg/XenBusDxe/XenBusDxe.inf
>>    OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
>> +  OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>>
>> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCoun
>> terRuntimeDxe.inf
>>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index
>> afaa334384..e22a223e7e 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>>  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/SecureBootConfi
>> gDxe.inf
>> --
>> 2.20.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 01/14] OvmfPkg/MptScsiDxe: Create empty driver

[Laszlo Ersek]

On 01/31/19 11:07, Nikita Leshenko wrote:
> In preparation for implementing LSI Fusion MPT SCSI devices, create a
> basic scaffolding for a driver.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 30 ++++++++++++++++++++++++++++
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 33 +++++++++++++++++++++++++++++++
>  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, 69 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 0000000000..73a693741d
> --- /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) 2018, Oracle and/or its affiliates. All rights reserved.

(1) Since (to my knowledge) this is the first public posting of this
driver, I suggest stating the years 2018-2019, or at least the year
2019, in the copyright notices of all files that are introduced in this
series.

> +
> +  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 0000000000..c558d78034
> --- /dev/null
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -0,0 +1,33 @@
> +## @file
> +# This driver produces Extended SCSI Pass Thru Protocol instances for
> +# LSI Fusion MPT SCSI devices.
> +#
> +# Copyright (C) 2018, Oracle and/or its affiliates. All rights reserved.
> +#
> +# 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                    = 0x00010005

(2) The latest version of the INF spec is 1.27:

https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/

And, the INF_VERSION define accepts "1.27" just fine:

https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/2_inf_overview/24_[defines]_section.html#24-defines-section

so please use that format.

> +  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
> +  OvmfPkg/OvmfPkg.dec

(3) I appreciate that this series takes a step-by-step approach for
constructing the driver. Thus, this patch intends to be minimal. Under
that motto, please remove "OvmfPkg/OvmfPkg.dec" from [Packages]; I think
no OvmfPkg artifacts are consumed here (PCDs, GUIDs, lib classes
declared in the DEC, header files uncluded from under Include/, and so on).

The rest looks good.

(4) General question for this series (raised by the fact that we're
adding new files here): can you please double-check that all new files
have CRLF line terminators?

Thanks,
Laszlo

> +
> +[LibraryClasses]
> +  UefiDriverEntryPoint
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index aee19b75d7..52458859b6 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -708,6 +708,7 @@
>    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 e013099136..73e36636e0 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -233,6 +233,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>  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 90cbd8e341..d8ea2addb2 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -717,6 +717,7 @@
>    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 afaa334384..e22a223e7e 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>  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 83d16eb00b..daf03cd1b5 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -715,6 +715,7 @@
>    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 afaa334384..e22a223e7e 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -234,6 +234,7 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
>  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
> 

[PATCH 02/14] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol

[Nikita Leshenko]

In order to probe and connect to the MptScsi device we need this
protocol. Currently it does nothing.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c      | 65 ++++++++++++++++++++++++++++++-
 OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  1 +
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 73a693741d..4dcb1b1ae5 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -15,6 +15,62 @@
 
 **/
 
+#include <Library/UefiLib.h>
+
+//
+// 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;
+}
+
+// Higher versions will be used before lower, 0x10-0xffffffef is the version
+// range for IVH (Indie Hardware Vendors)
+#define MPT_SCSI_BINDING_VERSION 0x10
+STATIC
+EFI_DRIVER_BINDING_PROTOCOL gMptScsiDriverBinding = {
+  &MptScsiControllerSupported,
+  &MptScsiControllerStart,
+  &MptScsiControllerStop,
+  MPT_SCSI_BINDING_VERSION,
+  NULL, // ImageHandle filled by EfiLibInstallDriverBindingComponentName2
+  NULL, // DriverBindingHandle filled by EfiLibInstallDriverBindingComponentName2
+};
+
 //
 // Entry Point
 //
@@ -26,5 +82,12 @@ MptScsiEntryPoint (
   IN EFI_SYSTEM_TABLE *SystemTable
   )
 {
-  return EFI_UNSUPPORTED;
+  return EfiLibInstallDriverBindingComponentName2 (
+    ImageHandle,
+    SystemTable,
+    &gMptScsiDriverBinding,
+    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 c558d78034..8a780a661e 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -31,3 +31,4 @@
 
 [LibraryClasses]
   UefiDriverEntryPoint
+  UefiLib
-- 
2.20.1

Re: [PATCH 02/14] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol

[Laszlo Ersek]

On 01/31/19 11:07, Nikita Leshenko wrote:
> In order to probe and connect to the MptScsi device we need this
> protocol. Currently it does nothing.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 65 ++++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  1 +
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 73a693741d..4dcb1b1ae5 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -15,6 +15,62 @@
>  
>  **/
>  
> +#include <Library/UefiLib.h>
> +
> +//
> +// 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;
> +}
> +
> +// Higher versions will be used before lower, 0x10-0xffffffef is the version
> +// range for IVH (Indie Hardware Vendors)
> +#define MPT_SCSI_BINDING_VERSION 0x10

If we'd like to use a macro + a comment for this, then:

(1) I suggest moving them near the top of the file (just below the
#include(s))

(2) The comment should be formatted as

//
// Higher versions...
// ...
//

I.e., add an empty "//" comment line, both before and after.


> +STATIC
> +EFI_DRIVER_BINDING_PROTOCOL gMptScsiDriverBinding = {

(3) Please replace the "g" prefix with "m". This object is not
firmware-"g"lobal but specific to the "m"odule. (Yes, I agree we used it
inconsistently in OvmfPkg in the past.)

> +  &MptScsiControllerSupported,
> +  &MptScsiControllerStart,
> +  &MptScsiControllerStop,
> +  MPT_SCSI_BINDING_VERSION,
> +  NULL, // ImageHandle filled by EfiLibInstallDriverBindingComponentName2
> +  NULL, // DriverBindingHandle filled by EfiLibInstallDriverBindingComponentName2

(4) This line is too long (81 chars). Please make sure that no new line
added in this series exceeds 80 characters in length.

> +};
> +
>  //
>  // Entry Point
>  //
> @@ -26,5 +82,12 @@ MptScsiEntryPoint (
>    IN EFI_SYSTEM_TABLE *SystemTable
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  return EfiLibInstallDriverBindingComponentName2 (
> +    ImageHandle,
> +    SystemTable,
> +    &gMptScsiDriverBinding,
> +    ImageHandle, // The handle to install onto
> +    NULL, // TODO Component name
> +    NULL // TODO Component name
> +    );

(5) The indentation of the arguments is not idiomatic; it should be two
spaces relative to the first letter ("E") in
"EfiLibInstallDriverBindingComponentName2".

>  }
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index c558d78034..8a780a661e 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -31,3 +31,4 @@
>  
>  [LibraryClasses]
>    UefiDriverEntryPoint
> +  UefiLib
> 

Thanks
Laszlo

[PATCH 03/14] OvmfPkg/MptScsiDxe: Report name of driver

[Nikita Leshenko]

Install Component Name protocols to have a nice display name for the
driver in places such as UEFI shell.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 61 ++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 4dcb1b1ae5..38cdda1abe 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -71,6 +71,63 @@ EFI_DRIVER_BINDING_PROTOCOL gMptScsiDriverBinding = {
   NULL, // DriverBindingHandle filled by EfiLibInstallDriverBindingComponentName2
 };
 
+//
+// Component Name
+//
+
+STATIC
+EFI_UNICODE_STRING_TABLE mDriverNameTable[] = {
+  { "eng;en", L"LSI Fusion MPT SCSI Driver" },
+  { NULL,     NULL                   }
+};
+
+STATIC
+EFI_COMPONENT_NAME_PROTOCOL gComponentName;
+
+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 == &gComponentName) // 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 gComponentName = {
+  &MptScsiGetDriverName,
+  &MptScsiGetDeviceName,
+  "eng" // SupportedLanguages, ISO 639-2 language codes
+};
+
+STATIC
+EFI_COMPONENT_NAME2_PROTOCOL gComponentName2 = {
+  (EFI_COMPONENT_NAME2_GET_DRIVER_NAME)     &MptScsiGetDriverName,
+  (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) &MptScsiGetDeviceName,
+  "en" // SupportedLanguages, RFC 4646 language codes
+};
+
 //
 // Entry Point
 //
@@ -87,7 +144,7 @@ MptScsiEntryPoint (
     SystemTable,
     &gMptScsiDriverBinding,
     ImageHandle, // The handle to install onto
-    NULL, // TODO Component name
-    NULL // TODO Component name
+    &gComponentName,
+    &gComponentName2
     );
 }
-- 
2.20.1

Re: [PATCH 03/14] OvmfPkg/MptScsiDxe: Report name of driver

[Laszlo Ersek]

On 01/31/19 11:07, Nikita Leshenko wrote:
> Install Component Name protocols to have a nice display name for the
> driver in places such as UEFI shell.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c | 61 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 4dcb1b1ae5..38cdda1abe 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -71,6 +71,63 @@ EFI_DRIVER_BINDING_PROTOCOL gMptScsiDriverBinding = {
>    NULL, // DriverBindingHandle filled by EfiLibInstallDriverBindingComponentName2
>  };
>  
> +//
> +// Component Name
> +//
> +
> +STATIC
> +EFI_UNICODE_STRING_TABLE mDriverNameTable[] = {
> +  { "eng;en", L"LSI Fusion MPT SCSI Driver" },
> +  { NULL,     NULL                   }
> +};
> +
> +STATIC
> +EFI_COMPONENT_NAME_PROTOCOL gComponentName;
> +
> +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 == &gComponentName) // 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 gComponentName = {
> +  &MptScsiGetDriverName,
> +  &MptScsiGetDeviceName,
> +  "eng" // SupportedLanguages, ISO 639-2 language codes
> +};
> +
> +STATIC
> +EFI_COMPONENT_NAME2_PROTOCOL gComponentName2 = {
> +  (EFI_COMPONENT_NAME2_GET_DRIVER_NAME)     &MptScsiGetDriverName,
> +  (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) &MptScsiGetDeviceName,
> +  "en" // SupportedLanguages, RFC 4646 language codes
> +};
> +
>  //
>  // Entry Point
>  //
> @@ -87,7 +144,7 @@ MptScsiEntryPoint (
>      SystemTable,
>      &gMptScsiDriverBinding,
>      ImageHandle, // The handle to install onto
> -    NULL, // TODO Component name
> -    NULL // TODO Component name
> +    &gComponentName,
> +    &gComponentName2
>      );
>  }
> 

(1) Please replace the "g" prefix with "m" in gComponentName and
gComponentName2. With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH 03/14] OvmfPkg/MptScsiDxe: Report name of driver

[Carsey, Jaben]

Improving experience is the shell is good.

Assuming that Laszlo's suggestion is implemented.

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, February 01, 2019 2:25 AM
> To: Nikita Leshenko <nikita.leshchenko@oracle.com>; edk2-
> devel@lists.01.org
> Cc: liran.alon@oracle.com
> Subject: Re: [edk2] [PATCH 03/14] OvmfPkg/MptScsiDxe: Report name of
> driver
> Importance: High
> 
> On 01/31/19 11:07, Nikita Leshenko wrote:
> > Install Component Name protocols to have a nice display name for the
> > driver in places such as UEFI shell.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Reviewed-by: Aaron Young <aaron.young@oracle.com>
> > Reviewed-by: Liran Alon <liran.alon@oracle.com>
> > ---
> >  OvmfPkg/MptScsiDxe/MptScsi.c | 61
> ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c
> b/OvmfPkg/MptScsiDxe/MptScsi.c
> > index 4dcb1b1ae5..38cdda1abe 100644
> > --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> > @@ -71,6 +71,63 @@ EFI_DRIVER_BINDING_PROTOCOL
> gMptScsiDriverBinding = {
> >    NULL, // DriverBindingHandle filled by
> EfiLibInstallDriverBindingComponentName2
> >  };
> >
> > +//
> > +// Component Name
> > +//
> > +
> > +STATIC
> > +EFI_UNICODE_STRING_TABLE mDriverNameTable[] = {
> > +  { "eng;en", L"LSI Fusion MPT SCSI Driver" },
> > +  { NULL,     NULL                   }
> > +};
> > +
> > +STATIC
> > +EFI_COMPONENT_NAME_PROTOCOL gComponentName;
> > +
> > +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 == &gComponentName) // 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 gComponentName = {
> > +  &MptScsiGetDriverName,
> > +  &MptScsiGetDeviceName,
> > +  "eng" // SupportedLanguages, ISO 639-2 language codes
> > +};
> > +
> > +STATIC
> > +EFI_COMPONENT_NAME2_PROTOCOL gComponentName2 = {
> > +  (EFI_COMPONENT_NAME2_GET_DRIVER_NAME)
> &MptScsiGetDriverName,
> > +  (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME)
> &MptScsiGetDeviceName,
> > +  "en" // SupportedLanguages, RFC 4646 language codes
> > +};
> > +
> >  //
> >  // Entry Point
> >  //
> > @@ -87,7 +144,7 @@ MptScsiEntryPoint (
> >      SystemTable,
> >      &gMptScsiDriverBinding,
> >      ImageHandle, // The handle to install onto
> > -    NULL, // TODO Component name
> > -    NULL // TODO Component name
> > +    &gComponentName,
> > +    &gComponentName2
> >      );
> >  }
> >
> 
> (1) Please replace the "g" prefix with "m" in gComponentName and
> gComponentName2. With that:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 04/14] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi

[Nikita Leshenko]

The MptScsiControllerSupported function is called for every handle
that appears on the system and if the handle is the lsi53c1030
controller the function would return success. A successful return
value will attach our driver to the device.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c      | 55 ++++++++++++++++++++++++++++++-
 OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  5 +++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 38cdda1abe..57a17ca0cb 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -15,7 +15,20 @@
 
 **/
 
+#include <IndustryStandard/Pci.h>
+#include <Library/DebugLib.h>
 #include <Library/UefiLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/PciIo.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
 
 //
 // Driver Binding
@@ -30,7 +43,46 @@ MptScsiControllerSupported (
   IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
   )
 {
-  return EFI_UNSUPPORTED;
+  EFI_STATUS          Status;
+  EFI_PCI_IO_PROTOCOL *PciIo = NULL;
+  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
@@ -42,6 +94,7 @@ MptScsiControllerStart (
   IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
   )
 {
+  DEBUG ((EFI_D_INFO, "Attempted to start MptScsi\n"));
   return EFI_UNSUPPORTED;
 }
 
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 8a780a661e..3608cecaab 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -30,5 +30,10 @@
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
+  DebugLib
+  UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiLib
+
+[Protocols]
+  gEfiPciIoProtocolGuid        ## TO_START
\ No newline at end of file
-- 
2.20.1

Re: [PATCH 04/14] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi

[Laszlo Ersek]

On 01/31/19 11:07, Nikita Leshenko wrote:
> The MptScsiControllerSupported function is called for every handle
> that appears on the system

This statement is incorrect. The Supported() function is generally
called on such handles that the ConnectController() boot service tries
to connect. More distantly, that depends on what devices the platform
BDS attempts to connect (which is platform policy). In some cases, the
platform BDS policy is to "connect all devices to all drivers", and then
you indeed end up with the above behavior.

(1) I suggest "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.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 55 ++++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  5 +++
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 38cdda1abe..57a17ca0cb 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -15,7 +15,20 @@
>  
>  **/
>  
> +#include <IndustryStandard/Pci.h>
> +#include <Library/DebugLib.h>
>  #include <Library/UefiLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/PciIo.h>

(2) Please keep this list alphabetically sorted. Sorting helps with
keeping (longer) #include lists unique, plus it also helps with matching
#include <Library/*.h> directives against the [LibraryClasses] in the
INF file (assuming we keep the latter sorted as well).

> +
> +//
> +// 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

(3) These should go into a new file under OvmfPkg/Include/IndustryStandard.

I guess the filename could be "FusionMptScsi.h", but feel free to
propose another name.

... Also, referring back to my comment (3) under [PATCH 01/14], once you
#include <IndustryStandard/FusionMptScsi.h> here, that will be the
actual first need to spell out "OvmfPkg/OvmfPkg.dec" under [Packages],
for the INF file.

>  
>  //
>  // Driver Binding
> @@ -30,7 +43,46 @@ MptScsiControllerSupported (
>    IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  EFI_STATUS          Status;
> +  EFI_PCI_IO_PROTOCOL *PciIo = NULL;

(4) The edk2 coding style forbids initialization of variables with
automatic storage duration. If you need PciIo set to NULL, please add a
separate assignment.

> +  PCI_TYPE00          Pci;
> +
> +  Status = gBS->OpenProtocol (
> +                  ControllerHandle,
> +                  &gEfiPciIoProtocolGuid,
> +                  (VOID **) &PciIo,

(5) In patches written for OvmfPkg, please never insert a space in the
middle of a cast expression. It should be

  (VOID **)&PciIo

Type casts have one of the strongest operator bindings in the C
language, and inserting a space in the middle suggests otherwise. We've
had actual bugs due to misleading formatting like this. (I know that
other package maintainers have different opinions, that's fine; they
don't review OvmfPkg, and I don't review their packages. :) )


> +                  This->DriverBindingHandle,
> +                  ControllerHandle,
> +                  EFI_OPEN_PROTOCOL_BY_DRIVER);

(6) The closing paren of the function call is incorrectly placed. It
should be on a separate line, aligned with the arguments. (Not a typo:
it should be aligned with the arguments, and not with the OpenProtocol
member name.)

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = PciIo->Pci.Read (
> +                  PciIo,
> +                  EfiPciIoWidthUint32,
> +                  0, sizeof (Pci) / sizeof (UINT32),
> +                  &Pci);

(7) The arguments should be indented two spaces relative to [R]ead.

(8) Same comment as (5) applies to the closing paren.

(9) The edk2 coding style requires us to place all arguments on separate
lines in a multi-line function call. As a concession in OvmfPkg, we also
use (or even prefer) a more condensed style:


  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0,
                        sizeof (Pci) / sizeof (UINT32), &Pci);

In this case, we're free to "flow" the arguments; however, the
indentation remains the same (two spaces relative to the member function
name).

> +  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)) {

(10) Here's the right formatting for the controlling expression:

  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)) {

Basically, logical operators go to the end of the line, and nesting
implies *one* space character.

> +    Status = EFI_SUCCESS;
> +  } else {
> +    Status = EFI_UNSUPPORTED;
> +  }
> +
> +Done:
> +  gBS->CloseProtocol (
> +                  ControllerHandle,
> +                  &gEfiPciIoProtocolGuid,
> +                  This->DriverBindingHandle,
> +                  ControllerHandle);

(11) See comments (7) through (9).

> +  return Status;
>  }
>  
>  STATIC
> @@ -42,6 +94,7 @@ MptScsiControllerStart (
>    IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
>    )
>  {
> +  DEBUG ((EFI_D_INFO, "Attempted to start MptScsi\n"));

(12) We no longer use the EFI_D_ prefix; please use DEBUG_ instead.

(13) If we can express the same debug information by referencing the
function name, that's better. In such cases (entry/exit messages in
functions), we should use:

  DEBUG ((DEBUG_VERBOSE, "%a: enter\n", __FUNCTION__));

because:
- it will not clutter the log of a non-verbose build,
- the message will refresh itself if the function is renamed,
- if you have an editor with ctags support, the log message lets you
jump to the function without grepping,
- the format string is more likely to match other such format strings,
which leads to better LZMA compression over the firmware volume(s).

(Side comment: in library functions, it may also make sense to print
"gEfiCallerBasename", also with "%a"; it contains the name of the driver
or application into which the library instance was statically linked.
Not necessary here, the function name is unique enough.)

>    return EFI_UNSUPPORTED;
>  }
>  
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 8a780a661e..3608cecaab 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -30,5 +30,10 @@
>    OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> +  DebugLib
> +  UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib

Right, this looks well ordered.

> +
> +[Protocols]
> +  gEfiPciIoProtocolGuid        ## TO_START
> \ No newline at end of file
> 

(14) Hmmm, not sure how I missed "No newline at end of file" in the
earlier patches, but that should be fixed.

OK, I think I'll stop reviewing version 1 at this point. The style
issues force me to comment on them, and it's hard to focus on the actual
functionality that way. Please rework the series (all patches) based on
these guidelines. To re-iterate those that affect multiple (or all) patches:

- fix the years in the copyright notices
- make sure you use CRLF line terminators
- reference the BZ in the commit messages
- move interface macros to OvmfPkg/Include/IndustryStandard/...
- move remaining macros to a module-specific .h file, or at least to the
  top of the .c file
- comment style -- use empty "//" before and after
- use "m" prefix for module global variables
- stick with 80 chars line length
- indentation and general formatting of:
  - controlling expressions,
  - function calls
- don't initialize local variables
- keep #include lists and [LibraryClasses] sections sorted
- no extra space within cast expressions
- debug messages: use DEBUG_* for debug masks, and %a/__FUNCTION__ for
  referring to the containing function

I'll make an effort to review v2 reasonably quickly.

Thanks
Laszlo

[PATCH 05/14] OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU

[Nikita Leshenko]

Support dynamic insertion and removal of the protocol

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c      | 204 +++++++++++++++++++++++++++++-
 OvmfPkg/MptScsiDxe/MptScsiDxe.inf |   5 +-
 2 files changed, 205 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 57a17ca0cb..6b655d9fe8 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -16,10 +16,13 @@
 **/
 
 #include <IndustryStandard/Pci.h>
+#include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
 #include <Library/UefiLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Protocol/PciIo.h>
+#include <Protocol/ScsiPassThruExt.h>
 
 //
 // Device offsets and constants
@@ -30,6 +33,118 @@
 #define LSI_SAS1068_PCI_DEVICE_ID 0x0054
 #define LSI_SAS1068E_PCI_DEVICE_ID 0x0058
 
+//
+// 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
+  )
+{
+  DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n",
+          Packet->DataDirection,
+          Packet->InTransferLength, Packet->OutTransferLength));
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiGetNextTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
+  IN OUT UINT8                                      **Target,
+  IN OUT UINT64                                     *Lun
+  )
+{
+  DEBUG ((EFI_D_INFO, "MptScsiGetNextTargetLun %d %d\n", **Target, *Lun));
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiGetNextTarget (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN OUT UINT8                                     **Target
+  )
+{
+  DEBUG ((EFI_D_INFO, "MptScsiGetNextTarget\n"));
+  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
+  )
+{
+  DEBUG ((EFI_D_INFO, "MptScsiBuildDevicePath %d %d\n", *Target, Lun));
+  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
+  )
+{
+  DEBUG ((EFI_D_INFO, "MptScsiGetTargetLun\n"));
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiResetChannel (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This
+  )
+{
+  DEBUG ((EFI_D_INFO, "MptScsiResetChannel\n"));
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+MptScsiResetTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL               *This,
+  IN UINT8                                         *Target,
+  IN UINT64                                        Lun
+  )
+{
+  DEBUG ((EFI_D_INFO, "MptScsiResetTargetLun\n"));
+  return EFI_UNSUPPORTED;
+}
+
 //
 // Driver Binding
 //
@@ -94,8 +209,65 @@ MptScsiControllerStart (
   IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
   )
 {
-  DEBUG ((EFI_D_INFO, "Attempted to start MptScsi\n"));
-  return EFI_UNSUPPORTED;
+  EFI_STATUS           Status;
+  EFI_PHYSICAL_ADDRESS PhysicalAddress;
+  MPT_SCSI_DEV         *Dev;
+
+  DEBUG ((EFI_D_INFO, "Starting MptScsi\n"));
+
+  //
+  // MPT_SCSI_DEV contains descriptors that are passed to the controller. The
+  // controller doesn't easily support addresses larger than 4GB, so we allocate
+  // the struct below 4GB. (Technically we can use HostMfaHighAddr and make sure
+  // all descriptors have the same high address but it's more complicated than
+  // just allocating below 4GB. Also, we allocate it only once per device so the
+  // overhead of allocating a whole page is minimal.)
+  //
+  PhysicalAddress = MAX_UINT32;
+  Status = gBS->AllocatePages (
+                  AllocateMaxAddress,
+                  EfiRuntimeServicesData,
+                  EFI_SIZE_TO_PAGES (sizeof (*Dev)),
+                  &PhysicalAddress
+                  );
+  if (EFI_ERROR (Status)) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // PhysicalAddress is page aligned, so MPT_SCSI_DEV alignment is correct
+  Dev = (MPT_SCSI_DEV *) (UINTN) PhysicalAddress;
+  ZeroMem (Dev, sizeof (*Dev));
+  Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
+
+  Dev->PassThruMode.AdapterId = MAX_UINT32; // Host adapter channel, doesn't exist
+  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 Done;
+  }
+
+  DEBUG ((EFI_D_INFO, "MptScsi Installed\n"));
+Done:
+  if (EFI_ERROR (Status)) {
+    gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) Dev, EFI_SIZE_TO_PAGES (sizeof (*Dev)));
+  }
+
+  return Status;
 }
 
 STATIC
@@ -108,7 +280,33 @@ MptScsiControllerStop (
   IN  EFI_HANDLE                            *ChildHandleBuffer
   )
 {
-  return EFI_UNSUPPORTED;
+  EFI_STATUS Status;
+  EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru;
+  MPT_SCSI_DEV *Dev = NULL;
+
+  DEBUG ((EFI_D_INFO, "Stopping MptScsi\n"));
+
+  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);
+
+  gBS->UninstallProtocolInterface (
+                  ControllerHandle,
+                  &gEfiExtScsiPassThruProtocolGuid,
+                  &Dev->PassThru);
+
+  gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) Dev, EFI_SIZE_TO_PAGES (sizeof (*Dev)));
+
+  return Status;
 }
 
 // Higher versions will be used before lower, 0x10-0xffffffef is the version
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 3608cecaab..5d424606a5 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -30,10 +30,13 @@
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
+  BaseMemoryLib
   DebugLib
+  MemoryAllocationLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiLib
 
 [Protocols]
-  gEfiPciIoProtocolGuid        ## TO_START
\ No newline at end of file
+  gEfiPciIoProtocolGuid                  ## TO_START
+  gEfiExtScsiPassThruProtocolGuid        ## BY_START
-- 
2.20.1

[PATCH 06/14] OvmfPkg/MptScsiDxe: Report one Target and one LUN

[Nikita Leshenko]

Support for multiple targets will be implemented in a later commit in
this series.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 6b655d9fe8..8ce7986b1d 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -68,6 +68,21 @@ MptScsiPassThru (
   return EFI_UNSUPPORTED;
 }
 
+STATIC
+BOOLEAN
+IsTargetInitialized (
+  IN UINT8                                          *Target
+  )
+{
+  int i;
+  for (i = 0; i < TARGET_MAX_BYTES; ++i) {
+    if (Target[i] != 0xFF) {
+      return TRUE;
+    }
+  }
+  return FALSE;
+}
+
 STATIC
 EFI_STATUS
 EFIAPI
@@ -78,7 +93,15 @@ MptScsiGetNextTargetLun (
   )
 {
   DEBUG ((EFI_D_INFO, "MptScsiGetNextTargetLun %d %d\n", **Target, *Lun));
-  return EFI_UNSUPPORTED;
+
+  // Currently support only target 0 LUN 0, so hardcode it
+  if (!IsTargetInitialized (*Target)) {
+    **Target = 0;
+    *Lun = 0;
+    return EFI_SUCCESS;
+  } else {
+    return EFI_NOT_FOUND;
+  }
 }
 
 STATIC
@@ -90,7 +113,14 @@ MptScsiGetNextTarget (
   )
 {
   DEBUG ((EFI_D_INFO, "MptScsiGetNextTarget\n"));
-  return EFI_UNSUPPORTED;
+
+  // Currently support only target 0 LUN 0, so hardcode it
+  if (!IsTargetInitialized (*Target)) {
+    **Target = 0;
+    return EFI_SUCCESS;
+  } else {
+    return EFI_NOT_FOUND;
+  }
 }
 
 STATIC
-- 
2.20.1

[PATCH 07/14] OvmfPkg/MptScsiDxe: Build DevicePath for discovered devices

[Nikita Leshenko]

Used to identify the individual disks in the hardware tree

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 8ce7986b1d..7f360158e7 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -133,8 +133,24 @@ MptScsiBuildDevicePath (
   IN OUT EFI_DEVICE_PATH_PROTOCOL                  **DevicePath
   )
 {
+  SCSI_DEVICE_PATH *ScsiDevicePath;
+
   DEBUG ((EFI_D_INFO, "MptScsiBuildDevicePath %d %d\n", *Target, Lun));
-  return EFI_UNSUPPORTED;
+
+  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

[PATCH 08/14] OvmfPkg/MptScsiDxe: Implement GetTargetLun

[Nikita Leshenko]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 7f360158e7..719948a73d 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -164,7 +164,17 @@ MptScsiGetTargetLun (
   )
 {
   DEBUG ((EFI_D_INFO, "MptScsiGetTargetLun\n"));
-  return EFI_UNSUPPORTED;
+
+  if (DevicePath->Type    != MESSAGING_DEVICE_PATH ||
+      DevicePath->SubType != MSG_SCSI_DP) {
+    return EFI_UNSUPPORTED;
+  }
+
+  SCSI_DEVICE_PATH *ScsiDevicePath = (SCSI_DEVICE_PATH *) DevicePath;
+  **Target = ScsiDevicePath->Pun;
+  *Lun = ScsiDevicePath->Lun;
+
+  return EFI_SUCCESS;
 }
 
 STATIC
-- 
2.20.1

[PATCH 09/14] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use

[Nikita Leshenko]

This will give us an exclusive access to the PciIo of this device
after it was started and until is will be stopped.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 719948a73d..29816fb98c 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -42,6 +42,7 @@ typedef struct {
   UINT32                          Signature;
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
   EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
+  EFI_PCI_IO_PROTOCOL             *PciIo;
 } MPT_SCSI_DEV;
 
 #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
@@ -295,6 +296,17 @@ MptScsiControllerStart (
   ZeroMem (Dev, sizeof (*Dev));
   Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
 
+  Status = gBS->OpenProtocol (
+                  ControllerHandle,
+                  &gEfiPciIoProtocolGuid,
+                  (VOID **) &Dev->PciIo,
+                  This->DriverBindingHandle,
+                  ControllerHandle,
+                  EFI_OPEN_PROTOCOL_BY_DRIVER); // exclusive for us until stopped
+  if (EFI_ERROR (Status)) {
+    goto Done;
+  }
+
   Dev->PassThruMode.AdapterId = MAX_UINT32; // Host adapter channel, doesn't exist
   Dev->PassThruMode.Attributes =
     EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL
@@ -320,6 +332,13 @@ MptScsiControllerStart (
   DEBUG ((EFI_D_INFO, "MptScsi Installed\n"));
 Done:
   if (EFI_ERROR (Status)) {
+    if (Dev->PciIo) {
+      gBS->CloseProtocol (
+                      ControllerHandle,
+                      &gEfiPciIoProtocolGuid,
+                      This->DriverBindingHandle,
+                      ControllerHandle);
+    }
     gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) Dev, EFI_SIZE_TO_PAGES (sizeof (*Dev)));
   }
 
@@ -359,6 +378,11 @@ MptScsiControllerStop (
                   ControllerHandle,
                   &gEfiExtScsiPassThruProtocolGuid,
                   &Dev->PassThru);
+  gBS->CloseProtocol (
+                  ControllerHandle,
+                  &gEfiPciIoProtocolGuid,
+                  This->DriverBindingHandle,
+                  ControllerHandle);
 
   gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) Dev, EFI_SIZE_TO_PAGES (sizeof (*Dev)));
 
-- 
2.20.1

[PATCH 10/14] OvmfPkg/MptScsiDxe: Set and restore PCI attributes

[Nikita Leshenko]

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.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 29816fb98c..92c190499e 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -43,6 +43,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) \
@@ -269,6 +270,7 @@ MptScsiControllerStart (
   EFI_STATUS           Status;
   EFI_PHYSICAL_ADDRESS PhysicalAddress;
   MPT_SCSI_DEV         *Dev;
+  BOOLEAN              PciAttributesChanged = FALSE;
 
   DEBUG ((EFI_D_INFO, "Starting MptScsi\n"));
 
@@ -307,6 +309,25 @@ MptScsiControllerStart (
     goto Done;
   }
 
+  Status = Dev->PciIo->Attributes (
+                  Dev->PciIo,
+                  EfiPciIoAttributeOperationGet, 0,
+                  &Dev->OriginalPciAttributes);
+  if (EFI_ERROR (Status)) {
+    goto Done;
+  }
+
+  // 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 Done;
+  }
+  PciAttributesChanged = TRUE;
+
   Dev->PassThruMode.AdapterId = MAX_UINT32; // Host adapter channel, doesn't exist
   Dev->PassThruMode.Attributes =
     EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL
@@ -332,6 +353,13 @@ MptScsiControllerStart (
   DEBUG ((EFI_D_INFO, "MptScsi Installed\n"));
 Done:
   if (EFI_ERROR (Status)) {
+    if (PciAttributesChanged) {
+      Dev->PciIo->Attributes (
+                      Dev->PciIo,
+                      EfiPciIoAttributeOperationEnable,
+                      Dev->OriginalPciAttributes,
+                      NULL);
+    }
     if (Dev->PciIo) {
       gBS->CloseProtocol (
                       ControllerHandle,
@@ -378,6 +406,13 @@ MptScsiControllerStop (
                   ControllerHandle,
                   &gEfiExtScsiPassThruProtocolGuid,
                   &Dev->PassThru);
+
+  Dev->PciIo->Attributes (
+                  Dev->PciIo,
+                  EfiPciIoAttributeOperationEnable,
+                  Dev->OriginalPciAttributes,
+                  NULL);
+
   gBS->CloseProtocol (
                   ControllerHandle,
                   &gEfiPciIoProtocolGuid,
-- 
2.20.1

[PATCH 11/14] OvmfPkg/MptScsiDxe: Initialize hardware

[Nikita Leshenko]

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.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 234 +++++++++++++++++++++++++++++++++++
 1 file changed, 234 insertions(+)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 92c190499e..fbabeb22ca 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -33,10 +33,109 @@
 #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
+
 //
 // Runtime Structures
 //
 
+typedef 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;
+} __attribute__((aligned(8), packed)) MPT_IO_CONTROLLER_INIT_REQUEST; // Align required by HW
+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;
+} __attribute__((packed)) 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;
+} __attribute__((packed)) 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;
+} __attribute__((packed)) MPT_SG_ENTRY_SIMPLE;
+typedef 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;
+} __attribute__((aligned(8), packed)) MPT_SCSI_IO_ERROR_REPLY; // Align required by HW
+
 #define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
 typedef struct {
   UINT32                          Signature;
@@ -49,6 +148,134 @@ 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, (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;
+
+  Status = MptScsiReset (Dev);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  MPT_IO_CONTROLLER_INIT_REQUEST Req = {
+    .WhoInit = MPT_IOC_WHOINIT_ROM_BIOS,
+    .Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT,
+    .MaxDevices = 1,
+    .MaxBuses = 1,
+    .ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY),
+  };
+  CONST UINT32 ReqSizeInDwords = sizeof (Req) / sizeof (UINT32);
+  MPT_IO_CONTROLLER_INIT_REPLY Reply = {0};
+
+  // Send controller init through doorbell
+  Status = MptDoorbell (Dev, MPT_DOORBELL_HANDSHAKE, ReqSizeInDwords);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  Status = Dev->PciIo->Io.Write (
+                  Dev->PciIo, EfiPciIoWidthFifoUint32, 0,
+                  MPT_REG_DOORBELL, ReqSizeInDwords, &Req);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  // Read reply through doorbell
+  // Each 32bit read produces 16bit of data
+  UINT16 *Reply16 = (UINT16 *) &Reply;
+  while (Reply16 != (UINT16 *) (&Reply + 1)) {
+    UINT32 Reply32;
+    Status = In32 (Dev, MPT_REG_DOORBELL, &Reply32);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    *Reply16++ = (UINT16) Reply32;
+  }
+
+  // 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
 //
@@ -328,6 +555,11 @@ MptScsiControllerStart (
   }
   PciAttributesChanged = TRUE;
 
+  Status = MptScsiInit (Dev);
+  if (EFI_ERROR (Status)) {
+    goto Done;
+  }
+
   Dev->PassThruMode.AdapterId = MAX_UINT32; // Host adapter channel, doesn't exist
   Dev->PassThruMode.Attributes =
     EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL
@@ -407,6 +639,8 @@ MptScsiControllerStop (
                   &gEfiExtScsiPassThruProtocolGuid,
                   &Dev->PassThru);
 
+  MptScsiReset (Dev);
+
   Dev->PciIo->Attributes (
                   Dev->PciIo,
                   EfiPciIoAttributeOperationEnable,
-- 
2.20.1

Re: [PATCH 11/14] OvmfPkg/MptScsiDxe: Initialize hardware

[Laszlo Ersek]

Hi Nikita,

I've jumped ahead a little bit to point out other style issues, so you
can address them at once in v2. These are again general comments, likely
applying to several patches in the series.

On 01/31/19 11:07, Nikita Leshenko wrote:

> +typedef 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;
> +} __attribute__((aligned(8), packed)) MPT_IO_CONTROLLER_INIT_REQUEST; // Align required by HW


(1) For packing, you need to say:

#pragma pack (1)
...
#pragma pack ()


(2) For enforcing an alignment of 8 bytes, I think you'll have to create
an outer union first, and place a UINT64 member into it, as one member.
The other member can be the structure you actually need. And, all
further variable definitions (with static or auto storage duration) will
have to occur through the union type. For dynamically allocated objects,
the maximum alignment is ensured anyway.

Perhaps other edk2 maintainers can give you a better tip for enforcing a
minimum alignment for local & global variables; I'm not aware of any
other portable method than the union.

> +STATIC
> +EFI_STATUS
> +MptDoorbell (
> +  IN MPT_SCSI_DEV       *Dev,
> +  IN UINT8              DoorbellFunc,
> +  IN UINT8              DoorbellArg
> +  )
> +{
> +  return Out32 (Dev, MPT_REG_DOORBELL, (DoorbellFunc << 24) | (DoorbellArg << 16));
> +}

(3) The DoorbellFunc variable is of type UINT8, which (in practice) is
"unsigned char". As the operand of the << operator, it is promoted to
"int" (because "int" can represent all values of "unsigned char", on all
the platforms that we care about). In practice, our "int" representation
is "31 value bits, 1 sign bit, no padding bits, and two's complement
representation". When you left shift a UINT8 value by 24 bits, it's
possible that you sign a nonzero bit into the sign bit -- more
precisely, the mathematical result of the bitwise left shift cannot be
represented in the signed int result. According to the specification of
the << operator, this is undefined behavior.

In other words, please cast DoorbellFunc to UINT32 before applying the
shift.

> +  MPT_IO_CONTROLLER_INIT_REQUEST Req = {
> +    .WhoInit = MPT_IOC_WHOINIT_ROM_BIOS,
> +    .Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT,
> +    .MaxDevices = 1,
> +    .MaxBuses = 1,
> +    .ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY),
> +  };

(4) Okay, so this uses two language features simultaneously that are not
permitted in edk2 code:

- declaration that is not at the top of a block,
- designated initializer.

You'll have to set these fields one by one (with assignments), or else
introduce a static constant object as a "template", and then set the
local variable with CopyMem().

(In edk2 you can't use structure assignment either, you can only assign
scalars. This limitation is not from the C language of course; it is
because otherwise some compilers cannot be prevented from generating
calls to intrinsics, which are not available in the firmware execution
environment.)

I guess I'll also mention that compound literals are forbidden too.
Basically, stick with C89.

Thanks
Laszlo

[PATCH 12/14] OvmfPkg/MptScsiDxe: Implement the PassThru method

[Nikita Leshenko]

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).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c      | 255 +++++++++++++++++++++++++++++-
 OvmfPkg/MptScsiDxe/MptScsiDxe.inf |   3 +
 OvmfPkg/OvmfPkg.dec               |   2 +
 3 files changed, 256 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index fbabeb22ca..a7dab0f71a 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -54,9 +54,15 @@
 #define MPT_MESSAGE_HDR_FUNCTION_IOC_INIT        0x02
 
 #define MPT_SG_ENTRY_TYPE_SIMPLE 0x01
+#define MPT_SG_ENTRY_SIMPLE_MAX_LENGTH ((1 << 24) - 1)
 
 #define MPT_IOC_WHOINIT_ROM_BIOS 0x02
 
+#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
+
 //
 // Runtime Structures
 //
@@ -110,6 +116,8 @@ 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;
@@ -136,6 +144,11 @@ typedef struct {
   UINT32    ResponseInfo;
 } __attribute__((aligned(8), packed)) MPT_SCSI_IO_ERROR_REPLY; // Align required by HW
 
+typedef struct {
+  MPT_SCSI_IO_REQUEST Header;
+  MPT_SG_ENTRY_SIMPLE Sg;
+} __attribute__((aligned(8), packed)) MPT_SCSI_REQUEST_WITH_SG; // Align required by HW
+
 #define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
 typedef struct {
   UINT32                          Signature;
@@ -143,6 +156,9 @@ typedef struct {
   EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
   EFI_PCI_IO_PROTOCOL             *PciIo;
   UINT64                          OriginalPciAttributes;
+  UINT32                          StallPerPollUsec;
+  MPT_SCSI_IO_ERROR_REPLY         IoErrorReply;
+  MPT_SCSI_REQUEST_WITH_SG        IoRequest;
 } MPT_SCSI_DEV;
 
 #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
@@ -228,6 +244,8 @@ MptScsiInit (
 {
   EFI_STATUS Status;
 
+  Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
+
   Status = MptScsiReset (Dev);
   if (EFI_ERROR (Status)) {
     return Status;
@@ -273,6 +291,193 @@ MptScsiInit (
     return Status;
   }
 
+  // Put one free reply frame on the reply queue, the hardware may use it to
+  // report an error to us.
+  ASSERT ((UINTN) &Dev->IoErrorReply <= MAX_UINT32);
+  Status = Out32 (Dev, MPT_REG_REP_Q, (UINT32)(UINTN) &Dev->IoErrorReply);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+MptScsiPopulateRequest (
+  IN UINT8                                          Target,
+  IN UINT64                                         Lun,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+  OUT MPT_SCSI_REQUEST_WITH_SG                      *Request
+  )
+{
+  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
+      Packet->CdbLength > sizeof (Request->Header.CDB)) {
+    return EFI_UNSUPPORTED;
+  }
+
+  if (Target > 0 || Lun > 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (Packet->InTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) {
+    Packet->InTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH;
+    return EFI_BAD_BUFFER_SIZE;
+  }
+  if (Packet->OutTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) {
+    Packet->OutTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH;
+    return EFI_BAD_BUFFER_SIZE;
+  }
+
+  ZeroMem (Request, sizeof (*Request));
+  Request->Header.TargetID = Target;
+  // It's 1 and not 0, for some reason...
+  Request->Header.LUN[1] = Lun;
+  Request->Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST;
+  Request->Header.MessageContext = 1; // Hardcoded, we handle one request at a time
+
+  Request->Header.CDBLength = Packet->CdbLength;
+  CopyMem (Request->Header.CDB, Packet->Cdb, Packet->CdbLength);
+
+  Request->Header.SenseBufferLength = Packet->SenseDataLength;
+  ASSERT ((UINTN) Packet->SenseData <= MAX_UINT32);
+  Request->Header.SenseBufferLowAddress = (UINT32)(UINTN) Packet->SenseData;
+
+  Request->Sg.EndOfList = 1;
+  Request->Sg.EndOfBuffer = 1;
+  Request->Sg.LastElement = 1;
+  Request->Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE;
+
+  switch (Packet->DataDirection)
+  {
+  case EFI_EXT_SCSI_DATA_DIRECTION_READ:
+    Request->Header.DataLength = Packet->InTransferLength;
+    Request->Sg.Length = Packet->InTransferLength;
+    ASSERT ((UINTN) Packet->InDataBuffer <= MAX_UINT32);
+    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->InDataBuffer;
+    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
+    break;
+  case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
+    Request->Header.DataLength = Packet->OutTransferLength;
+    Request->Sg.Length = Packet->OutTransferLength;
+    ASSERT ((UINTN) Packet->OutDataBuffer <= MAX_UINT32);
+    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->OutDataBuffer;
+    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
+    Request->Sg.BufferContainsData = 1;
+    break;
+  }
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+MptScsiSendRequest (
+  IN MPT_SCSI_DEV                                   *Dev,
+  IN MPT_SCSI_REQUEST_WITH_SG                       *Request,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
+  )
+{
+  EFI_STATUS Status;
+
+  // Make sure Request is fully written
+  MemoryFence ();
+
+  ASSERT ((UINTN) Request <= MAX_UINT32);
+  Status = Out32 (Dev, MPT_REG_REQ_Q, (UINT32)(UINTN) Request);
+  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;
+
+  for (;;) {
+    UINT32 Istatus;
+    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.
+  UINT32 EmptyReply;
+  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 MPT_SCSI_REQUEST_WITH_SG                       *Request,
+  IN UINT32                                         Reply,
+  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet
+  )
+{
+  if (Reply == Request->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 ((EFI_D_ERROR, "MptScsi request failed\n"));
+    // When reply MSB is set, it's an error frame.
+
+    switch (Dev->IoErrorReply.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
+    EFI_STATUS Status;
+    Status = Out32 (Dev, MPT_REG_REP_Q, (UINT32)(UINTN) &Dev->IoErrorReply);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+
+  } else {
+    DEBUG ((EFI_D_ERROR, "MptScsi unexpected reply\n"));
+    return EFI_DEVICE_ERROR;
+  }
+
   return EFI_SUCCESS;
 }
 
@@ -291,10 +496,52 @@ MptScsiPassThru (
   IN EFI_EVENT                                      Event     OPTIONAL
   )
 {
-  DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n",
-          Packet->DataDirection,
-          Packet->InTransferLength, Packet->OutTransferLength));
-  return EFI_UNSUPPORTED;
+  EFI_STATUS Status;
+  MPT_SCSI_DEV *Dev = MPT_SCSI_FROM_PASS_THRU (This);
+
+  // Noisy print, but useful when debugging this area
+  // DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n",
+  //         Packet->DataDirection,
+  //         Packet->InTransferLength, Packet->OutTransferLength));
+
+  Status = MptScsiPopulateRequest (*Target, Lun, Packet, &Dev->IoRequest);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = MptScsiSendRequest (Dev, &Dev->IoRequest, Packet);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+
+  UINT32 Reply;
+  Status = MptScsiGetReply (Dev, &Reply);
+  if (EFI_ERROR (Status)) {
+    goto Fatal;
+  }
+
+  Status = MptScsiHandleReply (Dev, &Dev->IoRequest, 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 ((EFI_D_ERROR, "MptScsi fatal error in scsi request\n"));
+  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
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 5d424606a5..5875d6c94b 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -40,3 +40,6 @@
 [Protocols]
   gEfiPciIoProtocolGuid                  ## TO_START
   gEfiExtScsiPassThruProtocolGuid        ## BY_START
+
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 7666297cf8..4ca98ff09f 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -122,6 +122,8 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize|0x0|UINT32|0x1a
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd|0x0|UINT32|0x1f
 
+  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x28
+
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
-- 
2.20.1

Re: [PATCH 12/14] OvmfPkg/MptScsiDxe: Implement the PassThru method

[Laszlo Ersek]

On 01/31/19 11:07, 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).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 255 +++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |   3 +
>  OvmfPkg/OvmfPkg.dec               |   2 +
>  3 files changed, 256 insertions(+), 4 deletions(-)

So, this is the patch where you pass pointers (DRAM addresses, as
expressed from the CPU's point of view) to the PCI device:

> +STATIC
> +EFI_STATUS
> +MptScsiPopulateRequest (
> +  IN UINT8                                          Target,
> +  IN UINT64                                         Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  OUT MPT_SCSI_REQUEST_WITH_SG                      *Request
> +  )
> +{
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
> +      Packet->CdbLength > sizeof (Request->Header.CDB)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  if (Target > 0 || Lun > 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Packet->InTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) {
> +    Packet->InTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH;
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +  if (Packet->OutTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) {
> +    Packet->OutTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH;
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  ZeroMem (Request, sizeof (*Request));
> +  Request->Header.TargetID = Target;
> +  // It's 1 and not 0, for some reason...
> +  Request->Header.LUN[1] = Lun;
> +  Request->Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST;
> +  Request->Header.MessageContext = 1; // Hardcoded, we handle one request at a time
> +
> +  Request->Header.CDBLength = Packet->CdbLength;
> +  CopyMem (Request->Header.CDB, Packet->Cdb, Packet->CdbLength);
> +
> +  Request->Header.SenseBufferLength = Packet->SenseDataLength;
> +  ASSERT ((UINTN) Packet->SenseData <= MAX_UINT32);
> +  Request->Header.SenseBufferLowAddress = (UINT32)(UINTN) Packet->SenseData;
> +
> +  Request->Sg.EndOfList = 1;
> +  Request->Sg.EndOfBuffer = 1;
> +  Request->Sg.LastElement = 1;
> +  Request->Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE;
> +
> +  switch (Packet->DataDirection)
> +  {
> +  case EFI_EXT_SCSI_DATA_DIRECTION_READ:
> +    Request->Header.DataLength = Packet->InTransferLength;
> +    Request->Sg.Length = Packet->InTransferLength;
> +    ASSERT ((UINTN) Packet->InDataBuffer <= MAX_UINT32);
> +    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->InDataBuffer;
> +    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
> +    break;
> +  case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
> +    Request->Header.DataLength = Packet->OutTransferLength;
> +    Request->Sg.Length = Packet->OutTransferLength;
> +    ASSERT ((UINTN) Packet->OutDataBuffer <= MAX_UINT32);
> +    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->OutDataBuffer;
> +    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
> +    Request->Sg.BufferContainsData = 1;
> +    break;
> +  }
> +
> +  return EFI_SUCCESS;
> +}

and

> +STATIC
> +EFI_STATUS
> +MptScsiSendRequest (
> +  IN MPT_SCSI_DEV                                   *Dev,
> +  IN MPT_SCSI_REQUEST_WITH_SG                       *Request,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  // Make sure Request is fully written
> +  MemoryFence ();
> +
> +  ASSERT ((UINTN) Request <= MAX_UINT32);
> +  Status = Out32 (Dev, MPT_REG_REQ_Q, (UINT32)(UINTN) Request);
> +  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;
> +}

and

>  
> @@ -291,10 +496,52 @@ MptScsiPassThru (
>    IN EFI_EVENT                                      Event     OPTIONAL
>    )
>  {
> -  DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n",
> -          Packet->DataDirection,
> -          Packet->InTransferLength, Packet->OutTransferLength));
> -  return EFI_UNSUPPORTED;
> +  EFI_STATUS Status;
> +  MPT_SCSI_DEV *Dev = MPT_SCSI_FROM_PASS_THRU (This);
> +
> +  // Noisy print, but useful when debugging this area
> +  // DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n",
> +  //         Packet->DataDirection,
> +  //         Packet->InTransferLength, Packet->OutTransferLength));
> +
> +  Status = MptScsiPopulateRequest (*Target, Lun, Packet, &Dev->IoRequest);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = MptScsiSendRequest (Dev, &Dev->IoRequest, Packet);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }

You will have to map each such memory transfer explicitly, with the
PciIo->Map(), PciIo->Unmap(), PciIo->AllocateBuffer() and
PciIo->FreeBuffer() functions. Otherwise, if there is an IOMMU in the
system, or the CPU view of memory addresses is not 1:1 identical with
the PCI device view of memory addresses for some other reason, then this
driver will fail.

And, this is a practical concern at this point, not a theoretical one.
If OVMF runs in an AMD SEV (Secure Encrypted Virtualization) guest, then
the PciIo Map / Unmap operations handle the memory decryption /
re-encryption that's necessary for QEMU to see the data in guest RAM.

In brief:

(1) Use PciIo->Map() with BusMasterRead for a single-shot transfer from
the CPU to the PCI device through DRAM.

(2) Use PciIo->Map() with BusMasterWrite for a single-shot transfer from
the PCI device to the CPU through DRAM.

(3) For bi-directional communication (such as request/response within
the same buffer), use PciIo->AllocateBuffer(), plus PciIo->Map() with
BusMasterCommonBuffer.

(4) If the Fusion MPT SCSI controller can handle 64-bit addresses
(*after* mapping; that is, in the device address space), then you might
want to set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in

  [PATCH 10/14] OvmfPkg/MptScsiDxe: Set and restore PCI attributes

but in a *separate* call from setting (EFI_PCI_IO_ATTRIBUTE_IO |
EFI_PCI_IO_ATTRIBUTE_BUS_MASTER). If setting DUAL_ADDRESS_CYCLE fails,
that in itself won't prevent the driver from working.

If the controller can only handle 32-bit device addresses (or you only
want to deal with 32-bit device addresses), then do not set
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE.


All of the above is described (much better) in the UEFI spec. See
EFI_PCI_IO_PROTOCOL | Description, in UEFI-2.7, pages 832-833 for
example (those are the page numbers printed on the pages, in reality
they are pages 902-903 in the PDF file).

Thanks!
Laszlo

[PATCH 13/14] OvmfPkg/MptScsiDxe: Report multiple targets

[Nikita Leshenko]

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.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c      | 27 ++++++++++++++++++++-------
 OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  1 +
 OvmfPkg/OvmfPkg.dec               |  3 +++
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index a7dab0f71a..0b888a1c56 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -157,6 +157,7 @@ typedef struct {
   EFI_PCI_IO_PROTOCOL             *PciIo;
   UINT64                          OriginalPciAttributes;
   UINT32                          StallPerPollUsec;
+  UINT8                           MaxTarget;
   MPT_SCSI_IO_ERROR_REPLY         IoErrorReply;
   MPT_SCSI_REQUEST_WITH_SG        IoRequest;
 } MPT_SCSI_DEV;
@@ -245,6 +246,7 @@ MptScsiInit (
   EFI_STATUS Status;
 
   Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
+  Dev->MaxTarget = PcdGet8 (PcdMptScsiMaxTargetLimit);
 
   Status = MptScsiReset (Dev);
   if (EFI_ERROR (Status)) {
@@ -254,7 +256,7 @@ MptScsiInit (
   MPT_IO_CONTROLLER_INIT_REQUEST Req = {
     .WhoInit = MPT_IOC_WHOINIT_ROM_BIOS,
     .Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT,
-    .MaxDevices = 1,
+    .MaxDevices = Dev->MaxTarget + 1,
     .MaxBuses = 1,
     .ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY),
   };
@@ -305,6 +307,7 @@ MptScsiInit (
 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,
@@ -316,7 +319,7 @@ MptScsiPopulateRequest (
     return EFI_UNSUPPORTED;
   }
 
-  if (Target > 0 || Lun > 0) {
+  if (Target > Dev->MaxTarget || Lun > 0) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -504,7 +507,7 @@ MptScsiPassThru (
   //         Packet->DataDirection,
   //         Packet->InTransferLength, Packet->OutTransferLength));
 
-  Status = MptScsiPopulateRequest (*Target, Lun, Packet, &Dev->IoRequest);
+  Status = MptScsiPopulateRequest (Dev, *Target, Lun, Packet, &Dev->IoRequest);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -568,16 +571,22 @@ MptScsiGetNextTargetLun (
   IN OUT UINT64                                     *Lun
   )
 {
+  MPT_SCSI_DEV *Dev = MPT_SCSI_FROM_PASS_THRU (This);
+
   DEBUG ((EFI_D_INFO, "MptScsiGetNextTargetLun %d %d\n", **Target, *Lun));
 
-  // Currently support only target 0 LUN 0, so hardcode it
+  // Currently support only LUN 0, so hardcode it
   if (!IsTargetInitialized (*Target)) {
     **Target = 0;
     *Lun = 0;
-    return EFI_SUCCESS;
+  } else if (**Target < Dev->MaxTarget) {
+    **Target += 1;
+    *Lun = 0;
   } else {
     return EFI_NOT_FOUND;
   }
+
+  return EFI_SUCCESS;
 }
 
 STATIC
@@ -588,15 +597,19 @@ MptScsiGetNextTarget (
   IN OUT UINT8                                     **Target
   )
 {
+  MPT_SCSI_DEV *Dev = MPT_SCSI_FROM_PASS_THRU (This);
+
   DEBUG ((EFI_D_INFO, "MptScsiGetNextTarget\n"));
 
-  // Currently support only target 0 LUN 0, so hardcode it
   if (!IsTargetInitialized (*Target)) {
     **Target = 0;
-    return EFI_SUCCESS;
+  } else if (**Target < Dev->MaxTarget) {
+    **Target += 1;
   } else {
     return EFI_NOT_FOUND;
   }
+
+  return EFI_SUCCESS;
 }
 
 STATIC
diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
index 5875d6c94b..3b6717c204 100644
--- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
+++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
@@ -43,3 +43,4 @@
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
+  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit ## CONSUMES
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4ca98ff09f..3f088eca96 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -123,6 +123,9 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd|0x0|UINT32|0x1f
 
   gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x28
+  ## Set the *inclusive* number of targets that MptScsi exposes for scan
+  #  by ScsiBusDxe.
+  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x29
 
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
-- 
2.20.1

[PATCH 14/14] OvmfPkg/MptScsiDxe: Support packets with pointers above 4G

[Nikita Leshenko]

Users of this device might pass data pointers which are above 4G, in which case
we can't pass the pointers directly to the controller because the descriptors
contain 32-bit pointers.

Instead of failing the request, use below-4G memory to support the request.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Aaron Young <aaron.young@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/MptScsiDxe/MptScsi.c | 50 +++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
index 0b888a1c56..9e803735d2 100644
--- a/OvmfPkg/MptScsiDxe/MptScsi.c
+++ b/OvmfPkg/MptScsiDxe/MptScsi.c
@@ -160,6 +160,9 @@ typedef struct {
   UINT8                           MaxTarget;
   MPT_SCSI_IO_ERROR_REPLY         IoErrorReply;
   MPT_SCSI_REQUEST_WITH_SG        IoRequest;
+
+  UINT8                           SenseBuffer[MAX_UINT8];
+  UINT8                           DataBuffer[0x2000];
 } MPT_SCSI_DEV;
 
 #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
@@ -323,6 +326,18 @@ MptScsiPopulateRequest (
     return EFI_INVALID_PARAMETER;
   }
 
+  // If the buffers are above 4G, make sure that we can buffer that data
+  if ((UINTN) Packet->InDataBuffer > MAX_UINT32 &&
+      Packet->InTransferLength > sizeof (Dev->DataBuffer)) {
+    Packet->InTransferLength = sizeof (Dev->DataBuffer);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+  if ((UINTN) Packet->OutDataBuffer > MAX_UINT32 &&
+      Packet->OutTransferLength > sizeof (Dev->DataBuffer)) {
+    Packet->OutTransferLength = sizeof (Dev->DataBuffer);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+
   if (Packet->InTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) {
     Packet->InTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH;
     return EFI_BAD_BUFFER_SIZE;
@@ -343,8 +358,14 @@ MptScsiPopulateRequest (
   CopyMem (Request->Header.CDB, Packet->Cdb, Packet->CdbLength);
 
   Request->Header.SenseBufferLength = Packet->SenseDataLength;
-  ASSERT ((UINTN) Packet->SenseData <= MAX_UINT32);
-  Request->Header.SenseBufferLowAddress = (UINT32)(UINTN) Packet->SenseData;
+  // If sense is above 4G, we can't pass it directly to controller
+  if ((UINTN) Packet->SenseData > MAX_UINT32) {
+    // Zero because the controller doesn't report the resulting sense data size
+    ZeroMem (&Dev->SenseBuffer, Packet->SenseDataLength);
+    Request->Header.SenseBufferLowAddress = (UINT32)(UINTN) &Dev->SenseBuffer;
+  } else {
+    Request->Header.SenseBufferLowAddress = (UINT32)(UINTN) Packet->SenseData;
+  }
 
   Request->Sg.EndOfList = 1;
   Request->Sg.EndOfBuffer = 1;
@@ -356,15 +377,24 @@ MptScsiPopulateRequest (
   case EFI_EXT_SCSI_DATA_DIRECTION_READ:
     Request->Header.DataLength = Packet->InTransferLength;
     Request->Sg.Length = Packet->InTransferLength;
-    ASSERT ((UINTN) Packet->InDataBuffer <= MAX_UINT32);
-    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->InDataBuffer;
+    // If buffer is above 4G, we can't pass it directly to controller
+    if ((UINTN) Packet->InDataBuffer > MAX_UINT32) {
+      Request->Sg.DataBufferAddress = (UINT32)(UINTN) &Dev->DataBuffer;
+    } else {
+      Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->InDataBuffer;
+    }
     Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
     break;
   case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
     Request->Header.DataLength = Packet->OutTransferLength;
     Request->Sg.Length = Packet->OutTransferLength;
-    ASSERT ((UINTN) Packet->OutDataBuffer <= MAX_UINT32);
-    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->OutDataBuffer;
+    // If buffer is above 4G, we can't pass it directly to controller
+    if ((UINTN) Packet->OutDataBuffer > MAX_UINT32) {
+      CopyMem (&Dev->DataBuffer, Packet->OutDataBuffer, Packet->OutTransferLength);
+      Request->Sg.DataBufferAddress = (UINT32)(UINTN) &Dev->DataBuffer;
+    } else {
+      Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->OutDataBuffer;
+    }
     Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
     Request->Sg.BufferContainsData = 1;
     break;
@@ -451,6 +481,14 @@ MptScsiHandleReply (
   OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet
   )
 {
+  if ((UINTN) Packet->SenseData > MAX_UINT32) {
+    CopyMem (Packet->SenseData, &Dev->SenseBuffer, Packet->SenseDataLength);
+  }
+  if ((UINTN) Packet->InDataBuffer > MAX_UINT32 &&
+      Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+    CopyMem (Packet->InDataBuffer, &Dev->DataBuffer, Packet->InTransferLength);
+  }
+
   if (Reply == Request->Header.MessageContext) {
     // Everything is good
     Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
-- 
2.20.1

Re: OvmfPkg: Support booting from Fusion-MPT SCSI controllers

[Laszlo Ersek]

On 01/31/19 11:07, 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.

In the future, please:
- CC maintainers / reviewers listed in "Maintainers.txt"
- please include the cumulative diffstat in the blurb.

Thanks
Laszlo

Re: OvmfPkg: Support booting from Fusion-MPT SCSI controllers

[Laszlo Ersek]

On 01/31/19 11:07, 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.

If you mark a scsi-cd or scsi-hd device with "bootindex=1" on the QEMU
command line, such that the device in question hangs off of the
Fusion-MPT SCSI controller, can you tell me what the corresponding entry
is in the "bootorder" fw_cfg file?

You can find that line in the OVMF debug log, if you build OVMF with the
DEBUG_VERBOSE bit (value 0x00400000) set in
"gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel". Look for:

> ConnectDevicesFromQemu: FwCfg:
> ...
> ConnectDevicesFromQemu: FwCfg: <end>

and

> SetBootOrderFromQemu: FwCfg:
> ...
> SetBootOrderFromQemu: FwCfg: <end>

I'm asking because you might have to extend
OvmfPkg/Library/QemuBootOrderLib too (namely function
TranslatePciOfwNodes()), if you want the "bootindex" property to be
honored by OVMF, for storage devices hanging off of this controller.

Thanks
Laszlo