[PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller

[PATCH v2 00/17] OvmfPkg: Support booting from VMware PVSCSI controller

[Liran Alon]

Hi,

This series adds driver support for VMware PVSCSI controller.

This controller is supported by VMware and QEMU. This work is part of
the more general agenda of enhancing OVMF boot device support to have
feature parity with SeaBIOS (Which supports booting from VMware PVSCSI).

I pushed a copy of these (v2) patches to https://github.com/nikital/edk2/tree/pvscsi5
The v1 patches can be found at https://github.com/nikital/edk2/tree/pvscsi4

Regards,
-Liran

v1->v2:
* Removed Nikita’s Reviewed-By tags. [Laszlo]
* Renamed PvScsi.inf to PvScsiDxe.inf and fixed references from all DSC files. [Laszlo]
* Changed “!ifdef $(PVSCSI_ENABLE)” in DSC files to “!if $(PVSCSI_ENABLE) == TRUE”. [Laszlo]
* Fix Identation in various places. [Laszlo]
* Added “#include <Uefi/UefiSpec.h>” for EFI_SYSTEM_TABLE. [Laszlo]
* Fix various typos. [Laszlo]
* Made “STATIC” on same line of object delcerations. [Laszlo]
* Added Laszlo’s Reviewed-by tags on some patches. [Laszlo]
* Added missing spaces before “(“ on various function calls. [Laszlo]
* Added PvScsi.h header file to INF [Sources] section. [Laszlo]
* Changed [Protocols] section in INF file to be lexicographically sorted. [Laszlo]
* Changed [PCDs] section in INF file to be lexigraphically sorted. [Laszlo]
* Fixed function comments blocks to be “/** **/” instead of “//” style. [Laszlo]
* Changed PvScsiGetTargetLun() to ZeroMem() all target bytes except first one. [Laszlo]
* Replaced “IOSpace” with “MMIO-Space” in comments. [Laszlo]
* Changed enums to match EDK2 coding convention. [Laszlo]
* Use PCI_BAR_IDX0 instead of hard-coded 0. [Laszlo]
* Use EFI_PAGES_TO_SIZE() instead of manually multiplying with EFI_PAGE_SIZE. [Laszlo]
* Use RShiftU64() to shift UINT64 vars. [Laszlo]
* Changed ReqNumEntries var to UINT32 and shift to use “1U <<” instead of “1 <<”. [Laszlo]
* Changed condition on flag (In PvScsiWaitForRequestCompletion()) to be a boolean expression. [Laszlo]
* Replaced “FakeHostAdapterError” label with a utility function. [Laszlo]
* Added debug message to PvScsiExitBoot() to assist debugging. [Laszlo]
* Fixed resource management to make each function either completely succeed or completely fail and free all resources. [Laszlo]
* Changed PvScsiWriteCmdDesc() to use EfiPciIoWidthFifoUint32. [Laszlo]
* Changed PvScsiWriteCmdDesc() prototype to make clear it descriptor must be an array of words. [Laszlo]

[PATCH v2 01/17] OvmfPkg/PvScsiDxe: Create empty driver

[Liran Alon]

In preparation for support booting from PvScsi devices, create a
basic scaffolding for a driver.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/OvmfPkgIa32.dsc         |  8 ++++++++
 OvmfPkg/OvmfPkgIa32.fdf         |  3 +++
 OvmfPkg/OvmfPkgIa32X64.dsc      |  8 ++++++++
 OvmfPkg/OvmfPkgIa32X64.fdf      |  3 +++
 OvmfPkg/OvmfPkgX64.dsc          |  8 ++++++++
 OvmfPkg/OvmfPkgX64.fdf          |  3 +++
 OvmfPkg/PvScsiDxe/PvScsi.c      | 26 ++++++++++++++++++++++++++
 OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 27 +++++++++++++++++++++++++++
 8 files changed, 86 insertions(+)
 create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.c
 create mode 100644 OvmfPkg/PvScsiDxe/PvScsiDxe.inf

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 19728f20b34e..af985b4f7826 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -44,6 +44,11 @@
 
 !include NetworkPkg/NetworkDefines.dsc.inc
 
+  #
+  # Device drivers
+  #
+  DEFINE PVSCSI_ENABLE           = TRUE
+
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
   # one of the supported values, in place of any of the convenience macros, is
@@ -718,6 +723,9 @@
   OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
   OvmfPkg/XenBusDxe/XenBusDxe.inf
   OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+  OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
   MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
   MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
   MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index 63607551ed75..a442e133b952 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -227,6 +227,9 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
 INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
 INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
 INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+INF  OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   INF  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 3c0c229e3a72..267a83bf86e2 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -44,6 +44,11 @@
 
 !include NetworkPkg/NetworkDefines.dsc.inc
 
+  #
+  # Device drivers
+  #
+  DEFINE PVSCSI_ENABLE           = TRUE
+
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
   # one of the supported values, in place of any of the convenience macros, is
@@ -731,6 +736,9 @@
   OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
   OvmfPkg/XenBusDxe/XenBusDxe.inf
   OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+  OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
   MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
   MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
   MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index 0488e5d95ffe..5fddaac2b0cb 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -228,6 +228,9 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
 INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
 INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
 INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+INF  OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   INF  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index f6c1d8d228c6..98cc2a955c3e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -44,6 +44,11 @@
 
 !include NetworkPkg/NetworkDefines.dsc.inc
 
+  #
+  # Device drivers
+  #
+  DEFINE PVSCSI_ENABLE           = TRUE
+
   #
   # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
   # one of the supported values, in place of any of the convenience macros, is
@@ -729,6 +734,9 @@
   OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
   OvmfPkg/XenBusDxe/XenBusDxe.inf
   OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+  OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
   MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
   MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
   MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 0488e5d95ffe..5fddaac2b0cb 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -228,6 +228,9 @@ INF  OvmfPkg/VirtioRngDxe/VirtioRng.inf
 INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf
 INF  OvmfPkg/XenBusDxe/XenBusDxe.inf
 INF  OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf
+!if $(PVSCSI_ENABLE) == TRUE
+INF  OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+!endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   INF  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
new file mode 100644
index 000000000000..1ae4de9869c1
--- /dev/null
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -0,0 +1,26 @@
+/** @file
+
+  This driver produces Extended SCSI Pass Thru Protocol instances for
+  pvscsi devices.
+
+  Copyright (C) 2020, Oracle and/or its affiliates.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi/UefiSpec.h>
+
+//
+// Entry Point
+//
+
+EFI_STATUS
+EFIAPI
+PvScsiEntryPoint (
+  IN EFI_HANDLE       ImageHandle,
+  IN EFI_SYSTEM_TABLE *SystemTable
+  )
+{
+  return EFI_UNSUPPORTED;
+}
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
new file mode 100644
index 000000000000..093cc0171338
--- /dev/null
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -0,0 +1,27 @@
+## @file
+#
+# This driver produces Extended SCSI Pass Thru Protocol instances for
+# pvscsi devices.
+#
+# Copyright (C) 2020, Oracle and/or its affiliates.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.29
+  BASE_NAME                      = PvScsiDxe
+  FILE_GUID                      = 30346B14-1580-4781-879D-BA0C55AE9BB2
+  MODULE_TYPE                    = UEFI_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = PvScsiEntryPoint
+
+[Sources]
+  PvScsi.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  UefiDriverEntryPoint
-- 
2.20.1

[PATCH v2 02/17] OvmfPkg/PvScsiDxe: Install DriverBinding protocol

[Liran Alon]

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

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c      | 66 ++++++++++++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsiDxe.inf |  1 +
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 1ae4de9869c1..77b28b326784 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -9,8 +9,65 @@
 
 **/
 
+#include <Library/UefiLib.h>
 #include <Uefi/UefiSpec.h>
 
+//
+// Higher versions will be used before lower, 0x10-0xffffffef is the version
+// range for IHV (Indie Hardware Vendors)
+//
+#define PVSCSI_BINDING_VERSION      0x10
+
+//
+// Driver Binding
+//
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiDriverBindingSupported (
+  IN EFI_DRIVER_BINDING_PROTOCOL *This,
+  IN EFI_HANDLE                  ControllerHandle,
+  IN EFI_DEVICE_PATH_PROTOCOL    *RemainingDevicePath OPTIONAL
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiDriverBindingStart (
+  IN EFI_DRIVER_BINDING_PROTOCOL *This,
+  IN EFI_HANDLE                  ControllerHandle,
+  IN EFI_DEVICE_PATH_PROTOCOL    *RemainingDevicePath OPTIONAL
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiDriverBindingStop (
+  IN EFI_DRIVER_BINDING_PROTOCOL *This,
+  IN EFI_HANDLE                  ControllerHandle,
+  IN UINTN                       NumberOfChildren,
+  IN EFI_HANDLE                  *ChildHandleBuffer
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC EFI_DRIVER_BINDING_PROTOCOL mPvScsiDriverBinding = {
+  &PvScsiDriverBindingSupported,
+  &PvScsiDriverBindingStart,
+  &PvScsiDriverBindingStop,
+  PVSCSI_BINDING_VERSION,
+  NULL, // ImageHandle, filled by EfiLibInstallDriverBindingComponentName2()
+  NULL  // DriverBindingHandle, filled as well
+};
+
 //
 // Entry Point
 //
@@ -22,5 +79,12 @@ PvScsiEntryPoint (
   IN EFI_SYSTEM_TABLE *SystemTable
   )
 {
-  return EFI_UNSUPPORTED;
+  return EfiLibInstallDriverBindingComponentName2 (
+           ImageHandle,
+           SystemTable,
+           &mPvScsiDriverBinding,
+           ImageHandle,
+           NULL, // TODO Component name
+           NULL  // TODO Component name
+           );
 }
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index 093cc0171338..d1d0e963f96d 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -25,3 +25,4 @@
 
 [LibraryClasses]
   UefiDriverEntryPoint
+  UefiLib
-- 
2.20.1

[PATCH v2 03/17] OvmfPkg/PvScsiDxe: Report name of driver

[Liran Alon]

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

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 59 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 77b28b326784..51b03f709040 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -68,6 +68,61 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL mPvScsiDriverBinding = {
   NULL  // DriverBindingHandle, filled as well
 };
 
+//
+// Component Name
+//
+
+STATIC EFI_UNICODE_STRING_TABLE mDriverNameTable[] = {
+  { "eng;en", L"PVSCSI Host Driver" },
+  { NULL,     NULL                  }
+};
+
+STATIC EFI_COMPONENT_NAME_PROTOCOL mComponentName;
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetDriverName (
+  IN  EFI_COMPONENT_NAME_PROTOCOL *This,
+  IN  CHAR8                       *Language,
+  OUT CHAR16                      **DriverName
+  )
+{
+  return LookupUnicodeString2 (
+           Language,
+           This->SupportedLanguages,
+           mDriverNameTable,
+           DriverName,
+           (BOOLEAN)(This == &mComponentName) // Iso639Language
+           );
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetDeviceName (
+  IN  EFI_COMPONENT_NAME_PROTOCOL *This,
+  IN  EFI_HANDLE                  DeviceHandle,
+  IN  EFI_HANDLE                  ChildHandle,
+  IN  CHAR8                       *Language,
+  OUT CHAR16                      **ControllerName
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC EFI_COMPONENT_NAME_PROTOCOL mComponentName = {
+  &PvScsiGetDriverName,
+  &PvScsiGetDeviceName,
+  "eng" // SupportedLanguages, ISO 639-2 language codes
+};
+
+STATIC EFI_COMPONENT_NAME2_PROTOCOL mComponentName2 = {
+  (EFI_COMPONENT_NAME2_GET_DRIVER_NAME)     &PvScsiGetDriverName,
+  (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) &PvScsiGetDeviceName,
+  "en" // SupportedLanguages, RFC 4646 language codes
+};
+
 //
 // Entry Point
 //
@@ -84,7 +139,7 @@ PvScsiEntryPoint (
            SystemTable,
            &mPvScsiDriverBinding,
            ImageHandle,
-           NULL, // TODO Component name
-           NULL  // TODO Component name
+           &mComponentName,
+           &mComponentName2
            );
 }
-- 
2.20.1

[PATCH v2 04/17] OvmfPkg/PvScsiDxe: Probe PCI devices and look for PvScsi

[Liran Alon]

PvScsiControllerSupported() is called on handles passed in
by the ConnectController() boot service and if the handle is the
PVSCSI controller, the function would return success. A success
return value will attach our driver to the device.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/Include/IndustryStandard/PvScsi.h | 21 ++++++++++
 OvmfPkg/PvScsiDxe/PvScsi.c                | 49 ++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsiDxe.inf           |  5 +++
 3 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 OvmfPkg/Include/IndustryStandard/PvScsi.h

diff --git a/OvmfPkg/Include/IndustryStandard/PvScsi.h b/OvmfPkg/Include/IndustryStandard/PvScsi.h
new file mode 100644
index 000000000000..004c0af84989
--- /dev/null
+++ b/OvmfPkg/Include/IndustryStandard/PvScsi.h
@@ -0,0 +1,21 @@
+/** @file
+
+  VMware PVSCSI Device specific type and macro definitions.
+
+  Copyright (C) 2020, Oracle and/or its affiliates.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PVSCSI_H_
+#define __PVSCSI_H_
+
+//
+// Device offsets and constants
+//
+
+#define PCI_VENDOR_ID_VMWARE            (0x15ad)
+#define PCI_DEVICE_ID_VMWARE_PVSCSI     (0x07c0)
+
+#endif // __PVSCSI_H_
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 51b03f709040..9923a31d25d7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -9,7 +9,11 @@
 
 **/
 
+#include <IndustryStandard/Pci.h>
+#include <IndustryStandard/PvScsi.h>
+#include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
+#include <Protocol/PciIo.h>
 #include <Uefi/UefiSpec.h>
 
 //
@@ -31,7 +35,50 @@ PvScsiDriverBindingSupported (
   IN EFI_DEVICE_PATH_PROTOCOL    *RemainingDevicePath OPTIONAL
   )
 {
-  return EFI_UNSUPPORTED;
+  EFI_STATUS          Status;
+  EFI_PCI_IO_PROTOCOL *PciIo;
+  PCI_TYPE00          Pci;
+
+  Status = gBS->OpenProtocol (
+                  ControllerHandle,
+                  &gEfiPciIoProtocolGuid,
+                  (VOID **)&PciIo,
+                  This->DriverBindingHandle,
+                  ControllerHandle,
+                  EFI_OPEN_PROTOCOL_BY_DRIVER
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = PciIo->Pci.Read (
+                        PciIo,
+                        EfiPciIoWidthUint32,
+                        0,
+                        sizeof (Pci) / sizeof (UINT32),
+                        &Pci
+                        );
+  if (EFI_ERROR (Status)) {
+    goto Done;
+  }
+
+  if ((Pci.Hdr.VendorId != PCI_VENDOR_ID_VMWARE) ||
+      (Pci.Hdr.DeviceId != PCI_DEVICE_ID_VMWARE_PVSCSI)) {
+    Status = EFI_UNSUPPORTED;
+    goto Done;
+  }
+
+  Status = EFI_SUCCESS;
+
+Done:
+  gBS->CloseProtocol (
+         ControllerHandle,
+         &gEfiPciIoProtocolGuid,
+         This->DriverBindingHandle,
+         ControllerHandle
+         );
+
+  return Status;
 }
 
 STATIC
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index d1d0e963f96d..c1f0663832ed 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -22,7 +22,12 @@
 
 [Packages]
   MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
+  UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiLib
+
+[Protocols]
+  gEfiPciIoProtocolGuid        ## TO_START
-- 
2.20.1

[PATCH v2 05/17] OvmfPkg/PvScsiDxe: Install stubbed EXT_SCSI_PASS_THRU

[Liran Alon]

Support dynamic insertion and removal of the protocol.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c      | 209 +++++++++++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsi.h      |  29 +++++
 OvmfPkg/PvScsiDxe/PvScsiDxe.inf |   6 +-
 3 files changed, 241 insertions(+), 3 deletions(-)
 create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.h

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 9923a31d25d7..04c08036b799 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -11,17 +11,156 @@
 
 #include <IndustryStandard/Pci.h>
 #include <IndustryStandard/PvScsi.h>
+#include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
 #include <Protocol/PciIo.h>
 #include <Uefi/UefiSpec.h>
 
+#include "PvScsi.h"
+
 //
 // Higher versions will be used before lower, 0x10-0xffffffef is the version
 // range for IHV (Indie Hardware Vendors)
 //
 #define PVSCSI_BINDING_VERSION      0x10
 
+//
+// Ext SCSI Pass Thru
+//
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiPassThru (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
+  IN UINT8                                          *Target,
+  IN UINT64                                         Lun,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+  IN EFI_EVENT                                      Event    OPTIONAL
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetNextTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
+  IN OUT UINT8                                      **Target,
+  IN OUT UINT64                                     *Lun
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiBuildDevicePath (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
+  IN UINT8                                          *Target,
+  IN UINT64                                         Lun,
+  IN OUT EFI_DEVICE_PATH_PROTOCOL                   **DevicePath
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
+  IN EFI_DEVICE_PATH_PROTOCOL                       *DevicePath,
+  OUT UINT8                                         **Target,
+  OUT UINT64                                        *Lun
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiResetChannel (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiResetTargetLun (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
+  IN UINT8                                          *Target,
+  IN UINT64                                         Lun
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+PvScsiGetNextTarget (
+  IN EFI_EXT_SCSI_PASS_THRU_PROTOCOL                *This,
+  IN OUT UINT8                                      **Target
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+STATIC
+EFI_STATUS
+PvScsiInit (
+  IN OUT PVSCSI_DEV *Dev
+  )
+{
+  //
+  // Populate the exported interface's attributes
+  //
+  Dev->PassThru.Mode             = &Dev->PassThruMode;
+  Dev->PassThru.PassThru         = &PvScsiPassThru;
+  Dev->PassThru.GetNextTargetLun = &PvScsiGetNextTargetLun;
+  Dev->PassThru.BuildDevicePath  = &PvScsiBuildDevicePath;
+  Dev->PassThru.GetTargetLun     = &PvScsiGetTargetLun;
+  Dev->PassThru.ResetChannel     = &PvScsiResetChannel;
+  Dev->PassThru.ResetTargetLun   = &PvScsiResetTargetLun;
+  Dev->PassThru.GetNextTarget    = &PvScsiGetNextTarget;
+
+  //
+  // AdapterId is a target for which no handle will be created during bus scan.
+  // Prevent any conflict with real devices.
+  //
+  Dev->PassThruMode.AdapterId = MAX_UINT32;
+
+  //
+  // Set both physical and logical attributes for non-RAID SCSI channel
+  //
+  Dev->PassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL |
+                                 EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
+
+  //
+  // No restriction on transfer buffer alignment
+  //
+  Dev->PassThruMode.IoAlign = 0;
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+VOID
+PvScsiUninit (
+  IN OUT PVSCSI_DEV *Dev
+  )
+{
+  // Currently nothing to do here
+}
+
 //
 // Driver Binding
 //
@@ -90,7 +229,42 @@ PvScsiDriverBindingStart (
   IN EFI_DEVICE_PATH_PROTOCOL    *RemainingDevicePath OPTIONAL
   )
 {
-  return EFI_UNSUPPORTED;
+  PVSCSI_DEV *Dev;
+  EFI_STATUS Status;
+
+  Dev = (PVSCSI_DEV *) AllocateZeroPool (sizeof (*Dev));
+  if (Dev == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  Status = PvScsiInit (Dev);
+  if (EFI_ERROR (Status)) {
+    goto FreePvScsi;
+  }
+
+  //
+  // Setup complete, attempt to export the driver instance's PassThru interface
+  //
+  Dev->Signature = PVSCSI_SIG;
+  Status = gBS->InstallProtocolInterface (
+                  &ControllerHandle,
+                  &gEfiExtScsiPassThruProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  &Dev->PassThru
+                  );
+  if (EFI_ERROR (Status)) {
+    goto UninitDev;
+  }
+
+  return EFI_SUCCESS;
+
+UninitDev:
+  PvScsiUninit (Dev);
+
+FreePvScsi:
+  FreePool (Dev);
+
+  return Status;
 }
 
 STATIC
@@ -103,7 +277,38 @@ PvScsiDriverBindingStop (
   IN EFI_HANDLE                  *ChildHandleBuffer
   )
 {
-  return EFI_UNSUPPORTED;
+  EFI_STATUS                      Status;
+  EFI_EXT_SCSI_PASS_THRU_PROTOCOL *PassThru;
+  PVSCSI_DEV                      *Dev;
+
+  Status = gBS->OpenProtocol (
+                  ControllerHandle,
+                  &gEfiExtScsiPassThruProtocolGuid,
+                  (VOID **)&PassThru,
+                  This->DriverBindingHandle,
+                  ControllerHandle,
+                  EFI_OPEN_PROTOCOL_GET_PROTOCOL // Lookup only
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Dev = PVSCSI_FROM_PASS_THRU (PassThru);
+
+  Status = gBS->UninstallProtocolInterface (
+                  ControllerHandle,
+                  &gEfiExtScsiPassThruProtocolGuid,
+                  &Dev->PassThru
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  PvScsiUninit (Dev);
+
+  FreePool (Dev);
+
+  return EFI_SUCCESS;
 }
 
 STATIC EFI_DRIVER_BINDING_PROTOCOL mPvScsiDriverBinding = {
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
new file mode 100644
index 000000000000..3940b4c20019
--- /dev/null
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -0,0 +1,29 @@
+/** @file
+
+  Internal definitions for the PVSCSI driver, which produces Extended SCSI
+  Pass Thru Protocol instances for pvscsi devices.
+
+  Copyright (C) 2020, Oracle and/or its affiliates.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PVSCSI_DXE_H_
+#define __PVSCSI_DXE_H_
+
+#include <Library/DebugLib.h>
+#include <Protocol/ScsiPassThruExt.h>
+
+#define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
+
+typedef struct {
+  UINT32                          Signature;
+  EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
+  EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
+} PVSCSI_DEV;
+
+#define PVSCSI_FROM_PASS_THRU(PassThruPointer) \
+  CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG)
+
+#endif // __PVSCSI_DXE_H_
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index c1f0663832ed..f4d452c6c3d2 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -19,15 +19,19 @@
 
 [Sources]
   PvScsi.c
+  PvScsi.h
 
 [Packages]
   MdePkg/MdePkg.dec
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
+  DebugLib
+  MemoryAllocationLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiLib
 
 [Protocols]
-  gEfiPciIoProtocolGuid        ## TO_START
+  gEfiExtScsiPassThruProtocolGuid   ## BY_START
+  gEfiPciIoProtocolGuid             ## TO_START
-- 
2.20.1

[PATCH v2 06/17] OvmfPkg/PvScsiDxe: Report the number of targets and LUNs

[Liran Alon]

Implement EXT_SCSI_PASS_THRU.GetNextTarget() and
EXT_SCSI_PASS_THRU.GetNextTargetLun().

ScsiBusDxe scans all MaxTarget * MaxLun possible devices.
This can take unnecessarily long for large number of targets.
To deal with this, VirtioScsiDxe has defined PCDs to limit the
MaxTarget & MaxLun to desired values which gives sufficient
performance. It is very important in virtio-scsi as it can have
very big MaxTarget & MaxLun.
Even though a common PVSCSI device has a default MaxTarget=64 and
MaxLun=0, we implement similar mechanism as virtio-scsi for completeness.
This may be useful in the future when PVSCSI will have bigger values
for MaxTarget and MaxLun.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/OvmfPkg.dec             |   9 +++
 OvmfPkg/PvScsiDxe/PvScsi.c      | 122 +++++++++++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsi.h      |   2 +
 OvmfPkg/PvScsiDxe/PvScsiDxe.inf |   5 ++
 4 files changed, 136 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4c5b6511cb97..a04aee5c2cd4 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -121,6 +121,15 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxTargetLimit|31|UINT16|6
   gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxLunLimit|7|UINT32|7
 
+  ## Sets the *inclusive* number of targets and LUNs that PvScsi exposes for
+  #  scan by ScsiBusDxe.
+  #  As specified above for VirtioScsi, ScsiBusDxe scans all MaxTarget * MaxLun
+  #  possible devices, which can take extremely long. Thus, the below constants
+  #  are used so that scanning the number of devices given by their product
+  #  is still acceptably fast.
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x36
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x37
+
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 04c08036b799..7f51ada19a1a 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -11,6 +11,7 @@
 
 #include <IndustryStandard/Pci.h>
 #include <IndustryStandard/PvScsi.h>
+#include <Library/BaseMemoryLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
@@ -25,6 +26,30 @@
 //
 #define PVSCSI_BINDING_VERSION      0x10
 
+//
+// Ext SCSI Pass Thru utilities
+//
+
+/**
+  Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
+  EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
+**/
+STATIC
+BOOLEAN
+IsTargetInitialized (
+  IN UINT8                                          *Target
+  )
+{
+  UINTN Idx;
+
+  for (Idx = 0; Idx < TARGET_MAX_BYTES; ++Idx) {
+    if (Target[Idx] != 0xFF) {
+      return TRUE;
+    }
+  }
+  return FALSE;
+}
+
 //
 // Ext SCSI Pass Thru
 //
@@ -52,7 +77,54 @@ PvScsiGetNextTargetLun (
   IN OUT UINT64                                     *Lun
   )
 {
-  return EFI_UNSUPPORTED;
+  UINT8      *TargetPtr;
+  UINT8      LastTarget;
+  PVSCSI_DEV *Dev;
+
+  if (Target == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // The Target input parameter is unnecessarily a pointer-to-pointer
+  //
+  TargetPtr = *Target;
+
+  //
+  // If target not initialized, return first target & LUN
+  //
+  if (!IsTargetInitialized (TargetPtr)) {
+    ZeroMem (TargetPtr, TARGET_MAX_BYTES);
+    *Lun = 0;
+    return EFI_SUCCESS;
+  }
+
+  //
+  // We only use first byte of target identifer
+  //
+  LastTarget = *TargetPtr;
+
+  //
+  // Increment (target, LUN) pair if valid on input
+  //
+  Dev = PVSCSI_FROM_PASS_THRU (This);
+  if (LastTarget > Dev->MaxTarget || *Lun > Dev->MaxLun) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (*Lun < Dev->MaxLun) {
+    ++*Lun;
+    return EFI_SUCCESS;
+  }
+
+  if (LastTarget < Dev->MaxTarget) {
+    *Lun = 0;
+    ++LastTarget;
+    *TargetPtr = LastTarget;
+    return EFI_SUCCESS;
+  }
+
+  return EFI_NOT_FOUND;
 }
 
 STATIC
@@ -111,7 +183,47 @@ PvScsiGetNextTarget (
   IN OUT UINT8                                      **Target
   )
 {
-  return EFI_UNSUPPORTED;
+  UINT8      *TargetPtr;
+  UINT8      LastTarget;
+  PVSCSI_DEV *Dev;
+
+  if (Target == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // The Target input parameter is unnecessarily a pointer-to-pointer
+  //
+  TargetPtr = *Target;
+
+  //
+  // If target not initialized, return first target
+  //
+  if (!IsTargetInitialized (TargetPtr)) {
+    ZeroMem (TargetPtr, TARGET_MAX_BYTES);
+    return EFI_SUCCESS;
+  }
+
+  //
+  // We only use first byte of target identifer
+  //
+  LastTarget = *TargetPtr;
+
+  //
+  // Increment target if valid on input
+  //
+  Dev = PVSCSI_FROM_PASS_THRU (This);
+  if (LastTarget > Dev->MaxTarget) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (LastTarget < Dev->MaxTarget) {
+    ++LastTarget;
+    *TargetPtr = LastTarget;
+    return EFI_SUCCESS;
+  }
+
+  return EFI_NOT_FOUND;
 }
 
 STATIC
@@ -120,6 +232,12 @@ PvScsiInit (
   IN OUT PVSCSI_DEV *Dev
   )
 {
+  //
+  // Init configuration
+  //
+  Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
+  Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
+
   //
   // Populate the exported interface's attributes
   //
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 3940b4c20019..dd3e0c68e6da 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -19,6 +19,8 @@
 
 typedef struct {
   UINT32                          Signature;
+  UINT8                           MaxTarget;
+  UINT8                           MaxLun;
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
   EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
 } PVSCSI_DEV;
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index f4d452c6c3d2..fcffc90d46c8 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -26,6 +26,7 @@
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
+  BaseMemoryLib
   DebugLib
   MemoryAllocationLib
   UefiBootServicesTableLib
@@ -35,3 +36,7 @@
 [Protocols]
   gEfiExtScsiPassThruProtocolGuid   ## BY_START
   gEfiPciIoProtocolGuid             ## TO_START
+
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit       ## CONSUMES
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit    ## CONSUMES
-- 
2.20.1

[PATCH v2 07/17] OvmfPkg/PvScsiDxe: Translate Target & LUN to/from DevicePath

[Liran Alon]

Implement EXT_SCSI_PASS_THRU.BuildDevicePath() and
EXT_SCSI_PASS_THRU.GetTargetLun().

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 61 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 7f51ada19a1a..76fc1eb910f2 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -137,7 +137,38 @@ PvScsiBuildDevicePath (
   IN OUT EFI_DEVICE_PATH_PROTOCOL                   **DevicePath
   )
 {
-  return EFI_UNSUPPORTED;
+  UINT8             TargetValue;
+  PVSCSI_DEV        *Dev;
+  SCSI_DEVICE_PATH  *ScsiDevicePath;
+
+  if (DevicePath == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // We only use first byte of target identifer
+  //
+  TargetValue = *Target;
+
+  Dev = PVSCSI_FROM_PASS_THRU (This);
+  if (TargetValue > Dev->MaxTarget || Lun > Dev->MaxLun) {
+    return EFI_NOT_FOUND;
+  }
+
+  ScsiDevicePath = AllocatePool (sizeof (*ScsiDevicePath));
+  if (ScsiDevicePath == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  ScsiDevicePath->Header.Type      = MESSAGING_DEVICE_PATH;
+  ScsiDevicePath->Header.SubType   = MSG_SCSI_DP;
+  ScsiDevicePath->Header.Length[0] = (UINT8)sizeof (*ScsiDevicePath);
+  ScsiDevicePath->Header.Length[1] = (UINT8)(sizeof (*ScsiDevicePath) >> 8);
+  ScsiDevicePath->Pun              = TargetValue;
+  ScsiDevicePath->Lun              = (UINT16)Lun;
+
+  *DevicePath = &ScsiDevicePath->Header;
+  return EFI_SUCCESS;
 }
 
 STATIC
@@ -150,7 +181,33 @@ PvScsiGetTargetLun (
   OUT UINT64                                        *Lun
   )
 {
-  return EFI_UNSUPPORTED;
+  SCSI_DEVICE_PATH *ScsiDevicePath;
+  PVSCSI_DEV       *Dev;
+
+  if (DevicePath == NULL || Target == NULL || *Target == NULL || Lun == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (DevicePath->Type    != MESSAGING_DEVICE_PATH ||
+      DevicePath->SubType != MSG_SCSI_DP) {
+    return EFI_UNSUPPORTED;
+  }
+
+  ScsiDevicePath = (SCSI_DEVICE_PATH *)DevicePath;
+  Dev = PVSCSI_FROM_PASS_THRU (This);
+  if (ScsiDevicePath->Pun > Dev->MaxTarget ||
+      ScsiDevicePath->Lun > Dev->MaxLun) {
+    return EFI_NOT_FOUND;
+  }
+
+  //
+  // We only use first byte of target identifer
+  //
+  **Target = (UINT8)ScsiDevicePath->Pun;
+  ZeroMem (*Target + 1, TARGET_MAX_BYTES - 1);
+  *Lun = ScsiDevicePath->Lun;
+
+  return EFI_SUCCESS;
 }
 
 STATIC
-- 
2.20.1

[PATCH v2 08/17] OvmfPkg/PvScsiDxe: Open PciIo protocol for later use

[Liran Alon]

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

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 29 ++++++++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsi.h |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 76fc1eb910f2..e0380d729b3c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -412,11 +412,23 @@ PvScsiDriverBindingStart (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  Status = PvScsiInit (Dev);
+  Status = gBS->OpenProtocol (
+                  ControllerHandle,
+                  &gEfiPciIoProtocolGuid,
+                  (VOID **)&Dev->PciIo,
+                  This->DriverBindingHandle,
+                  ControllerHandle,
+                  EFI_OPEN_PROTOCOL_BY_DRIVER
+                  );
   if (EFI_ERROR (Status)) {
     goto FreePvScsi;
   }
 
+  Status = PvScsiInit (Dev);
+  if (EFI_ERROR (Status)) {
+    goto ClosePciIo;
+  }
+
   //
   // Setup complete, attempt to export the driver instance's PassThru interface
   //
@@ -436,6 +448,14 @@ PvScsiDriverBindingStart (
 UninitDev:
   PvScsiUninit (Dev);
 
+ClosePciIo:
+  gBS->CloseProtocol (
+         ControllerHandle,
+         &gEfiPciIoProtocolGuid,
+         This->DriverBindingHandle,
+         ControllerHandle
+         );
+
 FreePvScsi:
   FreePool (Dev);
 
@@ -481,6 +501,13 @@ PvScsiDriverBindingStop (
 
   PvScsiUninit (Dev);
 
+  gBS->CloseProtocol (
+         ControllerHandle,
+         &gEfiPciIoProtocolGuid,
+         This->DriverBindingHandle,
+         ControllerHandle
+         );
+
   FreePool (Dev);
 
   return EFI_SUCCESS;
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index dd3e0c68e6da..e1e5ae18ebf2 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -19,6 +19,7 @@
 
 typedef struct {
   UINT32                          Signature;
+  EFI_PCI_IO_PROTOCOL             *PciIo;
   UINT8                           MaxTarget;
   UINT8                           MaxLun;
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
-- 
2.20.1

[PATCH v2 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit

[Liran Alon]

This commit doesn't change semantics.
It is done as a preparation for future commits which will modify
PCI attributes.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 54 +++++++++++++++++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsi.h |  1 +
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index e0380d729b3c..5566b4cce467 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -283,18 +283,70 @@ PvScsiGetNextTarget (
   return EFI_NOT_FOUND;
 }
 
+STATIC
+EFI_STATUS
+PvScsiSetPciAttributes (
+  IN OUT PVSCSI_DEV *Dev
+  )
+{
+  EFI_STATUS Status;
+
+  //
+  // Backup original PCI Attributes
+  //
+  Status = Dev->PciIo->Attributes (
+                         Dev->PciIo,
+                         EfiPciIoAttributeOperationGet,
+                         0,
+                         &Dev->OriginalPciAttributes
+                         );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // TODO: Change PCI Attributes
+  //
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+VOID
+PvScsiRestorePciAttributes (
+  IN PVSCSI_DEV *Dev
+  )
+{
+  Dev->PciIo->Attributes (
+                Dev->PciIo,
+                EfiPciIoAttributeOperationSet,
+                Dev->OriginalPciAttributes,
+                NULL
+                );
+}
+
 STATIC
 EFI_STATUS
 PvScsiInit (
   IN OUT PVSCSI_DEV *Dev
   )
 {
+  EFI_STATUS Status;
+
   //
   // Init configuration
   //
   Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
   Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
 
+  //
+  // Set PCI Attributes
+  //
+  Status = PvScsiSetPciAttributes (Dev);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   //
   // Populate the exported interface's attributes
   //
@@ -333,7 +385,7 @@ PvScsiUninit (
   IN OUT PVSCSI_DEV *Dev
   )
 {
-  // Currently nothing to do here
+  PvScsiRestorePciAttributes (Dev);
 }
 
 //
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index e1e5ae18ebf2..5f611dbbc98c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -20,6 +20,7 @@
 typedef struct {
   UINT32                          Signature;
   EFI_PCI_IO_PROTOCOL             *PciIo;
+  UINT64                          OriginalPciAttributes;
   UINT8                           MaxTarget;
   UINT8                           MaxLun;
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
-- 
2.20.1

[PATCH v2 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes

[Liran Alon]

Enable MMIO-Space & Bus-Mastering PCI attributes when device is started.
Note that original PCI attributes are restored when device is stopped.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 5566b4cce467..531bed4e5ab7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -305,8 +305,18 @@ PvScsiSetPciAttributes (
   }
 
   //
-  // TODO: Change PCI Attributes
+  // Enable MMIO-Space & Bus-Mastering
   //
+  Status = Dev->PciIo->Attributes (
+                         Dev->PciIo,
+                         EfiPciIoAttributeOperationEnable,
+                         (EFI_PCI_IO_ATTRIBUTE_MEMORY |
+                          EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
+                         NULL
+                         );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
 
   return EFI_SUCCESS;
 }
-- 
2.20.1

[PATCH v2 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants

[Liran Alon]

These definitions will be used by the following commits to complete the
implementation of PVSCSI device driver.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/Include/IndustryStandard/PvScsi.h | 165 ++++++++++++++++++++++
 1 file changed, 165 insertions(+)

diff --git a/OvmfPkg/Include/IndustryStandard/PvScsi.h b/OvmfPkg/Include/IndustryStandard/PvScsi.h
index 004c0af84989..a4d6634f3ba0 100644
--- a/OvmfPkg/Include/IndustryStandard/PvScsi.h
+++ b/OvmfPkg/Include/IndustryStandard/PvScsi.h
@@ -18,4 +18,169 @@
 #define PCI_VENDOR_ID_VMWARE            (0x15ad)
 #define PCI_DEVICE_ID_VMWARE_PVSCSI     (0x07c0)
 
+//
+// CDB (Command Descriptor Block) with size above this constant
+// should be considered out-of-band
+//
+#define PVSCSI_CDB_MAX_SIZE         (16)
+
+typedef enum {
+  PvScsiRegOffsetCommand           =    0x0,
+  PvScsiRegOffsetCommandData       =    0x4,
+  PvScsiRegOffsetCommandStatus     =    0x8,
+  PvScsiRegOffsetLastSts0          =  0x100,
+  PvScsiRegOffsetLastSts1          =  0x104,
+  PvScsiRegOffsetLastSts2          =  0x108,
+  PvScsiRegOffsetLastSts3          =  0x10c,
+  PvScsiRegOffsetIntrStatus        = 0x100c,
+  PvScsiRegOffsetIntrMask          = 0x2010,
+  PvScsiRegOffsetKickNonRwIo       = 0x3014,
+  PvScsiRegOffsetDebug             = 0x3018,
+  PvScsiRegOffsetKickRwIo          = 0x4018,
+} PVSCSI_BAR0_OFFSETS;
+
+//
+// Define Interrupt-Status register flags
+//
+#define PVSCSI_INTR_CMPL_0      BIT0
+#define PVSCSI_INTR_CMPL_1      BIT1
+#define PVSCSI_INTR_CMPL_MASK   (PVSCSI_INTR_CMPL_0 | PVSCSI_INTR_CMPL_1)
+
+typedef enum {
+  PvScsiCmdFirst               = 0,
+  PvScsiCmdAdapterReset        = 1,
+  PvScsiCmdIssueScsi           = 2,
+  PvScsiCmdSetupRings          = 3,
+  PvScsiCmdResetBus            = 4,
+  PvScsiCmdResetDevice         = 5,
+  PvScsiCmdAbortCmd            = 6,
+  PvScsiCmdConfig              = 7,
+  PvScsiCmdSetupMsgRing        = 8,
+  PvScsiCmdDeviceUnplug        = 9,
+  PvScsiCmdLast                = 10
+} PVSCSI_COMMANDS;
+
+#define PVSCSI_SETUP_RINGS_MAX_NUM_PAGES    (32)
+
+#pragma pack (1)
+typedef struct {
+  UINT32 ReqRingNumPages;
+  UINT32 CmpRingNumPages;
+  UINT64 RingsStatePPN;
+  UINT64 ReqRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
+  UINT64 CmpRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
+} PVSCSI_CMD_DESC_SETUP_RINGS;
+#pragma pack ()
+
+#define PVSCSI_MAX_CMD_DATA_WORDS   \
+  (sizeof (PVSCSI_CMD_DESC_SETUP_RINGS) / sizeof (UINT32))
+
+#pragma pack (1)
+typedef struct {
+  UINT32 ReqProdIdx;
+  UINT32 ReqConsIdx;
+  UINT32 ReqNumEntriesLog2;
+
+  UINT32 CmpProdIdx;
+  UINT32 CmpConsIdx;
+  UINT32 CmpNumEntriesLog2;
+
+  UINT8  Pad[104];
+
+  UINT32 MsgProdIdx;
+  UINT32 MsgConsIdx;
+  UINT32 MsgNumEntriesLog2;
+} PVSCSI_RINGS_STATE;
+#pragma pack ()
+
+//
+// Define PVSCSI request descriptor tags
+//
+#define PVSCSI_SIMPLE_QUEUE_TAG            (0x20)
+
+//
+// Define PVSCSI request descriptor flags
+//
+#define PVSCSI_FLAG_CMD_WITH_SG_LIST       BIT0
+#define PVSCSI_FLAG_CMD_OUT_OF_BAND_CDB    BIT1
+#define PVSCSI_FLAG_CMD_DIR_NONE           BIT2
+#define PVSCSI_FLAG_CMD_DIR_TOHOST         BIT3
+#define PVSCSI_FLAG_CMD_DIR_TODEVICE       BIT4
+
+#pragma pack (1)
+typedef struct {
+  UINT64 Context;
+  UINT64 DataAddr;
+  UINT64 DataLen;
+  UINT64 SenseAddr;
+  UINT32 SenseLen;
+  UINT32 Flags;
+  UINT8  Cdb[16];
+  UINT8  CdbLen;
+  UINT8  Lun[8];
+  UINT8  Tag;
+  UINT8  Bus;
+  UINT8  Target;
+  UINT8  VcpuHint;
+  UINT8  Unused[59];
+} PVSCSI_RING_REQ_DESC;
+#pragma pack ()
+
+//
+// Host adapter status/error codes
+//
+typedef enum {
+  PvScsiBtStatSuccess       = 0x00,  // CCB complete normally with no errors
+  PvScsiBtStatLinkedCommandCompleted         = 0x0a,
+  PvScsiBtStatLinkedCommandCompletedWithFlag = 0x0b,
+  PvScsiBtStatDataUnderrun  = 0x0c,
+  PvScsiBtStatSelTimeout    = 0x11,  // SCSI selection timeout
+  PvScsiBtStatDatarun       = 0x12,  // Data overrun/underrun
+  PvScsiBtStatBusFree       = 0x13,  // Unexpected bus free
+  PvScsiBtStatInvPhase      = 0x14,  //
+                                     // Invalid bus phase or sequence requested
+                                     // by target
+                                     //
+  PvScsiBtStatLunMismatch   = 0x17,  //
+                                     // Linked CCB has different LUN from first
+                                     // CCB
+                                     //
+  PvScsiBtStatSensFailed    = 0x1b,  // Auto request sense failed
+  PvScsiBtStatTagReject     = 0x1c,  //
+                                     // SCSI II tagged queueing message rejected
+                                     // by target
+                                     //
+  PvScsiBtStatBadMsg        = 0x1d,  //
+                                     // Unsupported message received by the host
+                                     // adapter
+                                     //
+  PvScsiBtStatHaHardware    = 0x20,  // Host adapter hardware failed
+  PvScsiBtStatNoResponse    = 0x21,  //
+                                     // Target did not respond to SCSI ATN sent
+                                     // a SCSI RST
+                                     //
+  PvScsiBtStatSentRst       = 0x22,  // Host adapter asserted a SCSI RST
+  PvScsiBtStatRecvRst       = 0x23,  // Other SCSI devices asserted a SCSI RST
+  PvScsiBtStatDisconnect    = 0x24,  //
+                                     // Target device reconnected improperly
+                                     // (w/o tag)
+                                     //
+  PvScsiBtStatBusReset      = 0x25,  // Host adapter issued BUS device reset
+  PvScsiBtStatAbortQueue    = 0x26,  // Abort queue generated
+  PvScsiBtStatHaSoftware    = 0x27,  // Host adapter software error
+  PvScsiBtStatHaTimeout     = 0x30,  // Host adapter hardware timeout error
+  PvScsiBtStatScsiParity    = 0x34,  // SCSI parity error detected
+} PVSCSI_HOST_BUS_ADAPTER_STATUS;
+
+#pragma pack (1)
+typedef struct {
+  UINT64 Context;
+  UINT64 DataLen;
+  UINT32 SenseLen;
+  UINT16 HostStatus;
+  UINT16 ScsiStatus;
+  UINT32 Pad[2];
+} PVSCSI_RING_CMP_DESC;
+#pragma pack ()
+
 #endif // __PVSCSI_H_
-- 
2.20.1

[PATCH v2 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init

[Liran Alon]

The following commits will complete the implementation of
device initialization.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 91 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 531bed4e5ab7..831a78cc18c7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -30,6 +30,84 @@
 // Ext SCSI Pass Thru utilities
 //
 
+/**
+  Writes a 32-bit value into BAR0 using MMIO
+**/
+STATIC
+EFI_STATUS
+PvScsiMmioWrite32 (
+  IN CONST PVSCSI_DEV   *Dev,
+  IN UINT64             Offset,
+  IN UINT32             Value
+  )
+{
+  return Dev->PciIo->Mem.Write (
+                           Dev->PciIo,
+                           EfiPciIoWidthUint32,
+                           PCI_BAR_IDX0,
+                           Offset,
+                           1,   // Count
+                           &Value
+                           );
+}
+
+/**
+  Writes multiple words of data into BAR0 using MMIO
+**/
+STATIC
+EFI_STATUS
+PvScsiMmioWrite32Multiple (
+  IN CONST PVSCSI_DEV   *Dev,
+  IN UINT64             Offset,
+  IN UINTN              Count,
+  IN UINT32             *Words
+  )
+{
+  return Dev->PciIo->Mem.Write (
+                           Dev->PciIo,
+                           EfiPciIoWidthFifoUint32,
+                           PCI_BAR_IDX0,
+                           Offset,
+                           Count,
+                           Words
+                           );
+}
+
+/**
+  Send PVSCSI command to device
+**/
+STATIC
+EFI_STATUS
+PvScsiWriteCmdDesc (
+  IN CONST PVSCSI_DEV   *Dev,
+  IN UINT32             Cmd,
+  IN UINT32             *DescWords,
+  IN UINTN              DescWordsCount
+  )
+{
+  EFI_STATUS Status;
+
+  if (DescWordsCount > PVSCSI_MAX_CMD_DATA_WORDS) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetCommand, Cmd);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (DescWordsCount > 0) {
+    return PvScsiMmioWrite32Multiple (
+             Dev,
+             PvScsiRegOffsetCommandData,
+             DescWordsCount,
+             DescWords
+             );
+  }
+
+  return EFI_SUCCESS;
+}
+
 /**
   Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
   EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
@@ -357,6 +435,14 @@ PvScsiInit (
     return Status;
   }
 
+  //
+  // Reset adapter
+  //
+  Status = PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
+  if (EFI_ERROR (Status)) {
+    goto RestorePciAttributes;
+  }
+
   //
   // Populate the exported interface's attributes
   //
@@ -387,6 +473,11 @@ PvScsiInit (
   Dev->PassThruMode.IoAlign = 0;
 
   return EFI_SUCCESS;
+
+RestorePciAttributes:
+  PvScsiRestorePciAttributes (Dev);
+
+  return Status;
 }
 
 STATIC
-- 
2.20.1

[PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings

[Liran Alon]

These rings are shared memory buffers between host and device in which
a cyclic buffer is managed to send request descriptors from host to
device and receive completion descriptors from device to host.

Note that because device may be constrained by IOMMU or guest may be run
under AMD SEV, we make sure to map these rings to device by using
PciIo->Map().

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 269 +++++++++++++++++++++++++++++++++++++
 OvmfPkg/PvScsiDxe/PvScsi.h |  17 +++
 2 files changed, 286 insertions(+)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 831a78cc18c7..59863f83c60c 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -16,6 +16,7 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
 #include <Protocol/PciIo.h>
+#include <Protocol/PciRootBridgeIo.h>
 #include <Uefi/UefiSpec.h>
 
 #include "PvScsi.h"
@@ -413,6 +414,264 @@ PvScsiRestorePciAttributes (
                 );
 }
 
+STATIC
+EFI_STATUS
+PvScsiAllocatePages (
+  IN PVSCSI_DEV     *Dev,
+  IN UINTN          Pages,
+  IN OUT VOID       **HostAddress
+  )
+{
+  return Dev->PciIo->AllocateBuffer (
+                       Dev->PciIo,
+                       AllocateAnyPages,
+                       EfiBootServicesData,
+                       Pages,
+                       HostAddress,
+                       EFI_PCI_ATTRIBUTE_MEMORY_CACHED
+                       );
+}
+
+STATIC
+VOID
+PvScsiFreePages (
+  IN PVSCSI_DEV     *Dev,
+  IN UINTN          Pages,
+  IN VOID           *HostAddress
+  )
+{
+  Dev->PciIo->FreeBuffer (
+                Dev->PciIo,
+                Pages,
+                HostAddress
+                );
+}
+
+STATIC
+EFI_STATUS
+PvScsiMapBuffer (
+  IN PVSCSI_DEV                     *Dev,
+  IN EFI_PCI_IO_PROTOCOL_OPERATION  PciIoOperation,
+  IN VOID                           *HostAddress,
+  IN UINTN                          NumberOfBytes,
+  OUT PVSCSI_DMA_DESC               *DmaDesc
+  )
+{
+  EFI_STATUS Status;
+  UINTN      BytesMapped;
+
+  BytesMapped = NumberOfBytes;
+  Status = Dev->PciIo->Map (
+                         Dev->PciIo,
+                         PciIoOperation,
+                         HostAddress,
+                         &BytesMapped,
+                         &DmaDesc->DeviceAddress,
+                         &DmaDesc->Mapping
+                         );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (BytesMapped != NumberOfBytes) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto Unmap;
+  }
+
+  return EFI_SUCCESS;
+
+Unmap:
+  Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+
+  return Status;
+}
+
+STATIC
+VOID
+PvScsiUnmapBuffer (
+  IN PVSCSI_DEV                 *Dev,
+  IN OUT PVSCSI_DMA_DESC        *DmaDesc)
+{
+  Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
+}
+
+STATIC
+EFI_STATUS
+PvScsiAllocateSharedPages (
+  IN PVSCSI_DEV                     *Dev,
+  IN UINTN                          Pages,
+  IN EFI_PCI_IO_PROTOCOL_OPERATION  PciIoOperation,
+  OUT VOID                          **HostAddress,
+  OUT PVSCSI_DMA_DESC               *DmaDesc
+  )
+{
+  EFI_STATUS Status;
+
+  Status = PvScsiAllocatePages (Dev, Pages, HostAddress);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = PvScsiMapBuffer (
+             Dev,
+             PciIoOperation,
+             *HostAddress,
+             EFI_PAGES_TO_SIZE (Pages),
+             DmaDesc
+             );
+  if (EFI_ERROR (Status)) {
+    goto FreePages;
+  }
+
+  return EFI_SUCCESS;
+
+FreePages:
+  PvScsiFreePages (Dev, Pages, *HostAddress);
+
+  return Status;
+}
+
+STATIC
+VOID
+PvScsiFreeSharedPages (
+  IN PVSCSI_DEV                     *Dev,
+  IN UINTN                          Pages,
+  IN OUT VOID                       *HostAddress,
+  IN OUT PVSCSI_DMA_DESC            *DmaDesc
+  )
+{
+  PvScsiUnmapBuffer (Dev, DmaDesc);
+  PvScsiFreePages (Dev, Pages, HostAddress);
+}
+
+STATIC
+EFI_STATUS
+PvScsiInitRings (
+  IN OUT PVSCSI_DEV *Dev
+  )
+{
+  EFI_STATUS                  Status;
+  PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
+
+  Status = PvScsiAllocateSharedPages (
+             Dev,
+             1,
+             EfiPciIoOperationBusMasterCommonBuffer,
+             (VOID **)&Dev->RingDesc.RingState,
+             &Dev->RingDesc.RingStateDmaDesc
+             );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
+
+  Status = PvScsiAllocateSharedPages (
+             Dev,
+             1,
+             EfiPciIoOperationBusMasterCommonBuffer,
+             (VOID **)&Dev->RingDesc.RingReqs,
+             &Dev->RingDesc.RingReqsDmaDesc
+             );
+  if (EFI_ERROR (Status)) {
+    goto FreeRingState;
+  }
+  ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
+
+  Status = PvScsiAllocateSharedPages (
+             Dev,
+             1,
+             EfiPciIoOperationBusMasterCommonBuffer,
+             (VOID **)&Dev->RingDesc.RingCmps,
+             &Dev->RingDesc.RingCmpsDmaDesc
+             );
+  if (EFI_ERROR (Status)) {
+    goto FreeRingReqs;
+  }
+  ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
+
+  ZeroMem (&Cmd, sizeof (Cmd));
+  Cmd.ReqRingNumPages = 1;
+  Cmd.CmpRingNumPages = 1;
+  Cmd.RingsStatePPN = RShiftU64 (
+                        (UINT64)Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
+                        EFI_PAGE_SHIFT
+                        );
+  Cmd.ReqRingPPNs[0] = RShiftU64 (
+                         (UINT64)Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
+                         EFI_PAGE_SHIFT
+                         );
+  Cmd.CmpRingPPNs[0] = RShiftU64 (
+                         (UINT64)Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
+                         EFI_PAGE_SHIFT
+                         );
+
+  Status = PvScsiWriteCmdDesc (
+             Dev,
+             PvScsiCmdSetupRings,
+             (UINT32 *)&Cmd,
+             sizeof (Cmd) / sizeof (UINT32)
+             );
+  if (EFI_ERROR (Status)) {
+    goto FreeRingCmps;
+  }
+
+  return EFI_SUCCESS;
+
+FreeRingCmps:
+  PvScsiFreeSharedPages (
+    Dev,
+    1,
+    Dev->RingDesc.RingCmps,
+    &Dev->RingDesc.RingCmpsDmaDesc
+    );
+
+FreeRingReqs:
+  PvScsiFreeSharedPages (
+    Dev,
+    1,
+    Dev->RingDesc.RingReqs,
+    &Dev->RingDesc.RingReqsDmaDesc
+    );
+
+FreeRingState:
+  PvScsiFreeSharedPages (
+    Dev,
+    1,
+    Dev->RingDesc.RingState,
+    &Dev->RingDesc.RingStateDmaDesc
+    );
+
+  return Status;
+}
+
+STATIC
+VOID
+PvScsiFreeRings (
+  IN OUT PVSCSI_DEV *Dev
+  )
+{
+  PvScsiFreeSharedPages (
+    Dev,
+    1,
+    Dev->RingDesc.RingCmps,
+    &Dev->RingDesc.RingCmpsDmaDesc
+    );
+
+  PvScsiFreeSharedPages (
+    Dev,
+    1,
+    Dev->RingDesc.RingReqs,
+    &Dev->RingDesc.RingReqsDmaDesc
+    );
+
+  PvScsiFreeSharedPages (
+    Dev,
+    1,
+    Dev->RingDesc.RingState,
+    &Dev->RingDesc.RingStateDmaDesc
+    );
+}
+
 STATIC
 EFI_STATUS
 PvScsiInit (
@@ -443,6 +702,14 @@ PvScsiInit (
     goto RestorePciAttributes;
   }
 
+  //
+  // Init PVSCSI rings
+  //
+  Status = PvScsiInitRings (Dev);
+  if (EFI_ERROR (Status)) {
+    goto RestorePciAttributes;
+  }
+
   //
   // Populate the exported interface's attributes
   //
@@ -486,6 +753,8 @@ PvScsiUninit (
   IN OUT PVSCSI_DEV *Dev
   )
 {
+  PvScsiFreeRings (Dev);
+
   PvScsiRestorePciAttributes (Dev);
 }
 
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 5f611dbbc98c..6d23b6e1eccf 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -15,12 +15,29 @@
 #include <Library/DebugLib.h>
 #include <Protocol/ScsiPassThruExt.h>
 
+typedef struct {
+  EFI_PHYSICAL_ADDRESS DeviceAddress;
+  VOID                 *Mapping;
+} PVSCSI_DMA_DESC;
+
+typedef struct {
+  PVSCSI_RINGS_STATE   *RingState;
+  PVSCSI_DMA_DESC      RingStateDmaDesc;
+
+  PVSCSI_RING_REQ_DESC *RingReqs;
+  PVSCSI_DMA_DESC      RingReqsDmaDesc;
+
+  PVSCSI_RING_CMP_DESC *RingCmps;
+  PVSCSI_DMA_DESC      RingCmpsDmaDesc;
+} PVSCSI_RING_DESC;
+
 #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
 
 typedef struct {
   UINT32                          Signature;
   EFI_PCI_IO_PROTOCOL             *PciIo;
   UINT64                          OriginalPciAttributes;
+  PVSCSI_RING_DESC                RingDesc;
   UINT8                           MaxTarget;
   UINT8                           MaxLun;
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
-- 
2.20.1

[PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

[Liran Alon]

In case device is constrained by IOMMU or guest is running under AMD SEV,
input/output buffers provided to device (DataBuffer and SenseData) needs
to be explicitly mapped to device by PciIo->Map().

To avoid the overhead of mapping/unmapping the DataBuffer and SenseData
to the device for every SCSI requst (And to simplify code), introduce a
single DMA communication buffer that will be mapped to device on
initialization. When a SCSI request needs to be sent to device, the
DataBuffer and SenseData will be copied from/to the DMA communication
buffer as required. This will be done by the following commits.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 27 +++++++++++++++++++++++++++
 OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++
 2 files changed, 37 insertions(+)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 59863f83c60c..928984099520 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -710,6 +710,20 @@ PvScsiInit (
     goto RestorePciAttributes;
   }
 
+  //
+  // Allocate DMA communication buffer
+  //
+  Status = PvScsiAllocateSharedPages (
+             Dev,
+             EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
+             EfiPciIoOperationBusMasterCommonBuffer,
+             (VOID **)&Dev->DmaBuf,
+             &Dev->DmaBufDmaDesc
+             );
+  if (EFI_ERROR (Status)) {
+    goto FreeRings;
+  }
+
   //
   // Populate the exported interface's attributes
   //
@@ -741,6 +755,9 @@ PvScsiInit (
 
   return EFI_SUCCESS;
 
+FreeRings:
+  PvScsiFreeRings (Dev);
+
 RestorePciAttributes:
   PvScsiRestorePciAttributes (Dev);
 
@@ -753,6 +770,16 @@ PvScsiUninit (
   IN OUT PVSCSI_DEV *Dev
   )
 {
+  //
+  // Free DMA communication buffer
+  //
+  PvScsiFreeSharedPages (
+    Dev,
+    EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
+    (VOID **)&Dev->DmaBuf,
+    &Dev->DmaBufDmaDesc
+    );
+
   PvScsiFreeRings (Dev);
 
   PvScsiRestorePciAttributes (Dev);
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 6d23b6e1eccf..7f91d70fec79 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -31,6 +31,11 @@ typedef struct {
   PVSCSI_DMA_DESC      RingCmpsDmaDesc;
 } PVSCSI_RING_DESC;
 
+typedef struct {
+  UINT8     SenseData[MAX_UINT8];
+  UINT8     Data[0x2000];
+} PVSCSI_DMA_BUFFER;
+
 #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
 
 typedef struct {
@@ -38,6 +43,8 @@ typedef struct {
   EFI_PCI_IO_PROTOCOL             *PciIo;
   UINT64                          OriginalPciAttributes;
   PVSCSI_RING_DESC                RingDesc;
+  PVSCSI_DMA_BUFFER               *DmaBuf;
+  PVSCSI_DMA_DESC                 DmaBufDmaDesc;
   UINT8                           MaxTarget;
   UINT8                           MaxLun;
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
@@ -47,4 +54,7 @@ typedef struct {
 #define PVSCSI_FROM_PASS_THRU(PassThruPointer) \
   CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG)
 
+#define PVSCSI_DMA_BUF_DEV_ADDR(Dev, MemberName) \
+  (Dev->DmaBufDmaDesc.DeviceAddress + OFFSET_OF(PVSCSI_DMA_BUFFER, MemberName))
+
 #endif // __PVSCSI_DXE_H_
-- 
2.20.1

[PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Liran Alon]

Implement EXT_SCSI_PASS_THRU.PassThru().

Machines should be able to boot after this commit.
Tested with Ubuntu 16.04 guest.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/OvmfPkg.dec             |   6 +
 OvmfPkg/PvScsiDxe/PvScsi.c      | 442 +++++++++++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsi.h      |   1 +
 OvmfPkg/PvScsiDxe/PvScsiDxe.inf |   5 +-
 4 files changed, 451 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index a04aee5c2cd4..ff49ec0e9e6a 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -130,6 +130,12 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x36
   gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x37
 
+  ## After PvScsiDxe sends a SCSI request to the device, it waits for
+  #  the request completion in a polling loop.
+  #  This constant defines how many micro-seconds to wait between each
+  #  polling loop iteration.
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38
+
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 928984099520..de4122e39a81 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -31,6 +31,27 @@
 // Ext SCSI Pass Thru utilities
 //
 
+/**
+  Reads a 32-bit value into BAR0 using MMIO
+**/
+STATIC
+EFI_STATUS
+PvScsiMmioRead32 (
+  IN CONST PVSCSI_DEV   *Dev,
+  IN UINT64             Offset,
+  OUT UINT32            *Value
+  )
+{
+  return Dev->PciIo->Mem.Read (
+                           Dev->PciIo,
+                           EfiPciIoWidthUint32,
+                           PCI_BAR_IDX0,
+                           Offset,
+                           1,   // Count
+                           Value
+                           );
+}
+
 /**
   Writes a 32-bit value into BAR0 using MMIO
 **/
@@ -109,6 +130,352 @@ PvScsiWriteCmdDesc (
   return EFI_SUCCESS;
 }
 
+/**
+  Returns if PVSCSI request ring is full
+**/
+STATIC
+BOOLEAN
+PvScsiIsReqRingFull (
+  IN CONST PVSCSI_DEV   *Dev
+  )
+{
+  PVSCSI_RINGS_STATE *RingsState;
+  UINT32             ReqNumEntries;
+
+  RingsState = Dev->RingDesc.RingState;
+  ReqNumEntries = 1U << RingsState->ReqNumEntriesLog2;
+  return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >= ReqNumEntries;
+}
+
+/**
+  Returns pointer to current request descriptor to produce
+**/
+STATIC
+PVSCSI_RING_REQ_DESC *
+PvScsiGetCurrentRequest (
+  IN CONST PVSCSI_DEV   *Dev
+  )
+{
+  PVSCSI_RINGS_STATE *RingState;
+  UINT32             ReqNumEntries;
+
+  RingState = Dev->RingDesc.RingState;
+  ReqNumEntries = 1U << RingState->ReqNumEntriesLog2;
+  return Dev->RingDesc.RingReqs +
+         (RingState->ReqProdIdx & (ReqNumEntries - 1));
+}
+
+/**
+  Returns pointer to current completion descriptor to consume
+**/
+STATIC
+PVSCSI_RING_CMP_DESC *
+PvScsiGetCurrentResponse (
+  IN CONST PVSCSI_DEV   *Dev
+  )
+{
+  PVSCSI_RINGS_STATE *RingState;
+  UINT32             CmpNumEntries;
+
+  RingState = Dev->RingDesc.RingState;
+  CmpNumEntries = 1U << RingState->CmpNumEntriesLog2;
+  return Dev->RingDesc.RingCmps +
+         (RingState->CmpConsIdx & (CmpNumEntries - 1));
+}
+
+/**
+  Wait for device to signal completion of submitted requests
+**/
+STATIC
+EFI_STATUS
+PvScsiWaitForRequestCompletion (
+  IN CONST PVSCSI_DEV   *Dev
+  )
+{
+  EFI_STATUS Status;
+  UINT32     IntrStatus;
+
+  //
+  // Note: We don't yet support Timeout according to
+  // EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.Timeout.
+  //
+  // This is consistent with some other Scsi PassThru drivers
+  // such as VirtioScsi.
+  //
+  for (;;) {
+    Status = PvScsiMmioRead32 (Dev, PvScsiRegOffsetIntrStatus, &IntrStatus);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+
+    //
+    // PVSCSI_INTR_CMPL_MASK is set if device completed submitted requests
+    //
+    if ((IntrStatus & PVSCSI_INTR_CMPL_MASK) != 0) {
+        break;
+    }
+
+    gBS->Stall (Dev->WaitForCmpStallInUsecs);
+  }
+
+  //
+  // Acknowledge PVSCSI_INTR_CMPL_MASK in device interrupt-status register
+  //
+  return PvScsiMmioWrite32 (
+           Dev,
+           PvScsiRegOffsetIntrStatus,
+           PVSCSI_INTR_CMPL_MASK
+           );
+}
+
+/**
+  Populate a PVSCSI request descriptor from the Extended SCSI Pass Thru
+  Protocol packet.
+**/
+STATIC
+EFI_STATUS
+PopulateRequest (
+  IN CONST PVSCSI_DEV                               *Dev,
+  IN UINT8                                          *Target,
+  IN UINT64                                         Lun,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+  OUT PVSCSI_RING_REQ_DESC                          *Request
+  )
+{
+  UINT8 TargetValue;
+
+  //
+  // We only use first byte of target identifer
+  //
+  TargetValue = *Target;
+
+  //
+  // Check for unsupported requests
+  //
+  if (
+      //
+      // Bidirectional transfer was requested
+      //
+      (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
+      (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
+      //
+      // Command Descriptor Block bigger than this constant should be considered
+      // out-of-band. We currently don't support these CDBs.
+      //
+      (Packet->CdbLength > PVSCSI_CDB_MAX_SIZE)
+      ) {
+
+    //
+    // This error code doesn't require updates to the Packet output fields
+    //
+    return EFI_UNSUPPORTED;
+  }
+
+  //
+  // Check for invalid parameters
+  //
+  if (
+      //
+      // Addressed invalid device
+      //
+      (TargetValue > Dev->MaxTarget) || (Lun > Dev->MaxLun) ||
+      //
+      // Invalid direction (there doesn't seem to be a macro for the "no data
+      // transferred" "direction", eg. for TEST UNIT READY)
+      //
+      (Packet->DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
+      //
+      // Trying to receive, but destination pointer is NULL, or contradicting
+      // transfer direction
+      //
+      ((Packet->InTransferLength > 0) &&
+       ((Packet->InDataBuffer == NULL) ||
+        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE)
+        )
+       ) ||
+      //
+      // Trying to send, but source pointer is NULL, or contradicting
+      // transfer direction
+      //
+      ((Packet->OutTransferLength > 0) &&
+       ((Packet->OutDataBuffer == NULL) ||
+        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ)
+        )
+       )
+      ) {
+
+    //
+    // This error code doesn't require updates to the Packet output fields
+    //
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // Check for input/output buffer too large for DMA communication buffer
+  //
+  if (Packet->InTransferLength > sizeof (Dev->DmaBuf->Data)) {
+    Packet->InTransferLength = sizeof (Dev->DmaBuf->Data);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+  if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) {
+    Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+
+  //
+  // Encode PVSCSI request
+  //
+  ZeroMem (Request, sizeof (*Request));
+
+  Request->Bus = 0;
+  Request->Target = TargetValue;
+  //
+  // This cast is safe as MaxLun is defined as UINT8
+  //
+  Request->Lun[1] = (UINT8)Lun;
+  Request->SenseLen = Packet->SenseDataLength;
+  Request->SenseAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, SenseData);
+  Request->CdbLen = Packet->CdbLength;
+  CopyMem (Request->Cdb, Packet->Cdb, Packet->CdbLength);
+  Request->VcpuHint = 0;
+  Request->Tag = PVSCSI_SIMPLE_QUEUE_TAG;
+  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+    Request->Flags = PVSCSI_FLAG_CMD_DIR_TOHOST;
+    Request->DataLen = Packet->InTransferLength;
+  } else {
+    Request->Flags = PVSCSI_FLAG_CMD_DIR_TODEVICE;
+    Request->DataLen = Packet->OutTransferLength;
+    CopyMem (
+      Dev->DmaBuf->Data,
+      Packet->OutDataBuffer,
+      Packet->OutTransferLength
+      );
+  }
+  Request->DataAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, Data);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Handle the PVSCSI device response:
+  - Copy returned data from DMA communication buffer.
+  - Update fields in Extended SCSI Pass Thru Protocol packet as required.
+  - Translate response code to EFI status code and host adapter status.
+**/
+STATIC
+EFI_STATUS
+HandleResponse (
+  IN PVSCSI_DEV                                     *Dev,
+  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
+  IN CONST PVSCSI_RING_CMP_DESC                     *Response
+  )
+{
+  //
+  // Check if device returned sense data
+  //
+  if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
+    //
+    // Fix SenseDataLength to amount of data returned
+    //
+    if (Packet->SenseDataLength > Response->SenseLen) {
+      Packet->SenseDataLength = (UINT8)Response->SenseLen;
+    }
+    //
+    // Copy sense data from DMA communication buffer
+    //
+    CopyMem (
+      Packet->SenseData,
+      Dev->DmaBuf->SenseData,
+      Packet->SenseDataLength
+      );
+  } else {
+    //
+    // Signal no sense data returned
+    //
+    Packet->SenseDataLength = 0;
+  }
+
+  //
+  // Copy device output from DMA communication buffer
+  //
+  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+    CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
+  }
+
+  //
+  // Report target status
+  //
+  Packet->TargetStatus = Response->ScsiStatus;
+
+  //
+  // Host adapter status and function return value depend on
+  // device response's host status
+  //
+  switch (Response->HostStatus) {
+    case PvScsiBtStatSuccess:
+    case PvScsiBtStatLinkedCommandCompleted:
+    case PvScsiBtStatLinkedCommandCompletedWithFlag:
+      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
+      return EFI_SUCCESS;
+
+    case PvScsiBtStatSelTimeout:
+      Packet->HostAdapterStatus =
+                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
+      return EFI_TIMEOUT;
+
+    case PvScsiBtStatDatarun:
+    case PvScsiBtStatDataUnderrun:
+      //
+      // Report residual data in overrun/underrun
+      //
+      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+        Packet->InTransferLength = Response->DataLen;
+      } else {
+        Packet->OutTransferLength = Response->DataLen;
+      }
+      Packet->HostAdapterStatus =
+                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
+      return EFI_BAD_BUFFER_SIZE;
+
+    case PvScsiBtStatBusFree:
+      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE;
+      break;
+
+    case PvScsiBtStatInvPhase:
+      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR;
+      break;
+
+    case PvScsiBtStatSensFailed:
+      Packet->HostAdapterStatus =
+                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED;
+      break;
+
+    case PvScsiBtStatTagReject:
+    case PvScsiBtStatBadMsg:
+      Packet->HostAdapterStatus =
+          EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT;
+      break;
+
+    case PvScsiBtStatBusReset:
+      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
+      break;
+
+    case PvScsiBtStatHaTimeout:
+      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
+      return EFI_TIMEOUT;
+
+    case PvScsiBtStatScsiParity:
+      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
+      break;
+
+    default:
+      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+      break;
+  }
+
+  return EFI_DEVICE_ERROR;
+}
+
 /**
   Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
   EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
@@ -129,6 +496,23 @@ IsTargetInitialized (
   return FALSE;
 }
 
+/**
+  Create a fake host adapter error
+**/
+STATIC
+EFI_STATUS
+ReportHostAdapterError (
+  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
+  )
+{
+  Packet->InTransferLength = 0;
+  Packet->OutTransferLength = 0;
+  Packet->SenseDataLength = 0;
+  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
+  Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
+  return EFI_DEVICE_ERROR;
+}
+
 //
 // Ext SCSI Pass Thru
 //
@@ -144,7 +528,62 @@ PvScsiPassThru (
   IN EFI_EVENT                                      Event    OPTIONAL
   )
 {
-  return EFI_UNSUPPORTED;
+  PVSCSI_DEV            *Dev;
+  EFI_STATUS            Status;
+  PVSCSI_RING_REQ_DESC *Request;
+  PVSCSI_RING_CMP_DESC *Response;
+
+  Dev = PVSCSI_FROM_PASS_THRU (This);
+
+  if (PvScsiIsReqRingFull (Dev)) {
+    return EFI_NOT_READY;
+  }
+
+  Request = PvScsiGetCurrentRequest (Dev);
+
+  Status = PopulateRequest (Dev, Target, Lun, Packet, Request);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // Writes to Request must be globally visible before making request
+  // available to device
+  //
+  MemoryFence ();
+  Dev->RingDesc.RingState->ReqProdIdx++;
+
+  Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetKickRwIo, 0);
+  if (EFI_ERROR (Status)) {
+    //
+    // If kicking the host fails, we must fake a host adapter error.
+    // EFI_NOT_READY would save us the effort, but it would also suggest that
+    // the caller retry.
+    //
+    return ReportHostAdapterError (Packet);
+  }
+
+  Status = PvScsiWaitForRequestCompletion (Dev);
+  if (EFI_ERROR (Status)) {
+    //
+    // If waiting for request completion fails, we must fake a host adapter
+    // error. EFI_NOT_READY would save us the effort, but it would also suggest
+    // that the caller retry.
+    //
+    return ReportHostAdapterError (Packet);
+  }
+
+  Response = PvScsiGetCurrentResponse (Dev);
+  Status = HandleResponse (Dev, Packet, Response);
+
+  //
+  // Reads from response must complete before releasing completion entry
+  // to device
+  //
+  MemoryFence ();
+  Dev->RingDesc.RingState->CmpConsIdx++;
+
+  return Status;
 }
 
 STATIC
@@ -685,6 +1124,7 @@ PvScsiInit (
   //
   Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
   Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
+  Dev->WaitForCmpStallInUsecs = PcdGet32 (PcdPvScsiWaitForCmpStallInUsecs);
 
   //
   // Set PCI Attributes
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 7f91d70fec79..08e876b75930 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -47,6 +47,7 @@ typedef struct {
   PVSCSI_DMA_DESC                 DmaBufDmaDesc;
   UINT8                           MaxTarget;
   UINT8                           MaxLun;
+  UINTN                           WaitForCmpStallInUsecs;
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
   EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
 } PVSCSI_DEV;
diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
index fcffc90d46c8..ae6d8753623f 100644
--- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
+++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
@@ -38,5 +38,6 @@
   gEfiPciIoProtocolGuid             ## TO_START
 
 [Pcd]
-  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit       ## CONSUMES
-  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit    ## CONSUMES
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit               ## CONSUMES
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit            ## CONSUMES
+  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs    ## CONSUMES
-- 
2.20.1

[PATCH v2 16/17] OvmfPkg/PvScsiDxe: Reset device on ExitBootServices()

[Liran Alon]

This causes the device to forget about the request/completion rings.
We allocated said rings in EfiBootServicesData type memory, and code
executing after ExitBootServices() is permitted to overwrite it.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 43 +++++++++++++++++++++++++++++++++++++-
 OvmfPkg/PvScsiDxe/PvScsi.h |  1 +
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index de4122e39a81..42a18494efb7 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -1225,6 +1225,31 @@ PvScsiUninit (
   PvScsiRestorePciAttributes (Dev);
 }
 
+/**
+  Event notification called by ExitBootServices()
+**/
+STATIC
+VOID
+EFIAPI
+PvScsiExitBoot (
+  IN  EFI_EVENT Event,
+  IN  VOID      *Context
+  )
+{
+  PVSCSI_DEV *Dev;
+
+  Dev = Context;
+  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
+
+  //
+  // Reset the device. This causes the device to forget about the
+  // request/completion rings. We allocated said rings in EfiBootServicesData
+  // type memory, and code executing after ExitBootServices() is permitted to
+  // overwrite it.
+  //
+  PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
+}
+
 //
 // Driver Binding
 //
@@ -1318,6 +1343,17 @@ PvScsiDriverBindingStart (
     goto ClosePciIo;
   }
 
+  Status = gBS->CreateEvent (
+                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
+                  TPL_CALLBACK,
+                  &PvScsiExitBoot,
+                  Dev,
+                  &Dev->ExitBoot
+                  );
+  if (EFI_ERROR (Status)) {
+    goto UninitDev;
+  }
+
   //
   // Setup complete, attempt to export the driver instance's PassThru interface
   //
@@ -1329,11 +1365,14 @@ PvScsiDriverBindingStart (
                   &Dev->PassThru
                   );
   if (EFI_ERROR (Status)) {
-    goto UninitDev;
+    goto CloseExitBoot;
   }
 
   return EFI_SUCCESS;
 
+CloseExitBoot:
+  gBS->CloseEvent (Dev->ExitBoot);
+
 UninitDev:
   PvScsiUninit (Dev);
 
@@ -1388,6 +1427,8 @@ PvScsiDriverBindingStop (
     return Status;
   }
 
+  gBS->CloseEvent (Dev->ExitBoot);
+
   PvScsiUninit (Dev);
 
   gBS->CloseProtocol (
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 08e876b75930..e68a7dedf71f 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -41,6 +41,7 @@ typedef struct {
 typedef struct {
   UINT32                          Signature;
   EFI_PCI_IO_PROTOCOL             *PciIo;
+  EFI_EVENT                       ExitBoot;
   UINT64                          OriginalPciAttributes;
   PVSCSI_RING_DESC                RingDesc;
   PVSCSI_DMA_BUFFER               *DmaBuf;
-- 
2.20.1

[PATCH v2 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses

[Liran Alon]

Enable PCI dual-address cycle attribute to signal device
supports 64-bit DMA addresses.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 42a18494efb7..3aea5c7f8eda 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -836,6 +836,29 @@ PvScsiSetPciAttributes (
     return Status;
   }
 
+  //
+  // Signal device supports 64-bit DMA addresses
+  //
+  Status = Dev->PciIo->Attributes (
+                         Dev->PciIo,
+                         EfiPciIoAttributeOperationEnable,
+                         EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
+                         NULL
+                         );
+  if (EFI_ERROR (Status)) {
+    //
+    // Warn user that device will only be using 32-bit DMA addresses.
+    //
+    // Note that this does not prevent the device/driver from working
+    // and therefore we only warn and continue as usual.
+    //
+    DEBUG ((
+      DEBUG_WARN,
+      "%a: failed to enable 64-bit DMA addresses\n",
+      __FUNCTION__
+      ));
+  }
+
   return EFI_SUCCESS;
 }
 
-- 
2.20.1

Re: [edk2-devel] [PATCH v2 01/17] OvmfPkg/PvScsiDxe: Create empty driver

[Laszlo Ersek]

On 03/25/20 17:09, Liran Alon wrote:
> In preparation for support booting from PvScsi devices, create a
> basic scaffolding for a driver.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc         |  8 ++++++++
>  OvmfPkg/OvmfPkgIa32.fdf         |  3 +++
>  OvmfPkg/OvmfPkgIa32X64.dsc      |  8 ++++++++
>  OvmfPkg/OvmfPkgIa32X64.fdf      |  3 +++
>  OvmfPkg/OvmfPkgX64.dsc          |  8 ++++++++
>  OvmfPkg/OvmfPkgX64.fdf          |  3 +++
>  OvmfPkg/PvScsiDxe/PvScsi.c      | 26 ++++++++++++++++++++++++++
>  OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 27 +++++++++++++++++++++++++++
>  8 files changed, 86 insertions(+)
>  create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.c
>  create mode 100644 OvmfPkg/PvScsiDxe/PvScsiDxe.inf

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

Re: [edk2-devel] [PATCH v2 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit

[Laszlo Ersek]

On 03/25/20 17:09, Liran Alon wrote:
> This commit doesn't change semantics.
> It is done as a preparation for future commits which will modify
> PCI attributes.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 54 +++++++++++++++++++++++++++++++++++++-
>  OvmfPkg/PvScsiDxe/PvScsi.h |  1 +
>  2 files changed, 54 insertions(+), 1 deletion(-)

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

Thanks,
Laszlo

> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index e0380d729b3c..5566b4cce467 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -283,18 +283,70 @@ PvScsiGetNextTarget (
>    return EFI_NOT_FOUND;
>  }
>  
> +STATIC
> +EFI_STATUS
> +PvScsiSetPciAttributes (
> +  IN OUT PVSCSI_DEV *Dev
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  //
> +  // Backup original PCI Attributes
> +  //
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationGet,
> +                         0,
> +                         &Dev->OriginalPciAttributes
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // TODO: Change PCI Attributes
> +  //
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +VOID
> +PvScsiRestorePciAttributes (
> +  IN PVSCSI_DEV *Dev
> +  )
> +{
> +  Dev->PciIo->Attributes (
> +                Dev->PciIo,
> +                EfiPciIoAttributeOperationSet,
> +                Dev->OriginalPciAttributes,
> +                NULL
> +                );
> +}
> +
>  STATIC
>  EFI_STATUS
>  PvScsiInit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  EFI_STATUS Status;
> +
>    //
>    // Init configuration
>    //
>    Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
>    Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
>  
> +  //
> +  // Set PCI Attributes
> +  //
> +  Status = PvScsiSetPciAttributes (Dev);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>    //
>    // Populate the exported interface's attributes
>    //
> @@ -333,7 +385,7 @@ PvScsiUninit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> -  // Currently nothing to do here
> +  PvScsiRestorePciAttributes (Dev);
>  }
>  
>  //
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index e1e5ae18ebf2..5f611dbbc98c 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -20,6 +20,7 @@
>  typedef struct {
>    UINT32                          Signature;
>    EFI_PCI_IO_PROTOCOL             *PciIo;
> +  UINT64                          OriginalPciAttributes;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> 

Re: [PATCH v2 10/17] OvmfPkg/PvScsiDxe: Enable MMIO-Space & Bus-Mastering in PCI attributes

[Laszlo Ersek]

On 03/25/20 17:09, Liran Alon wrote:
> Enable MMIO-Space & Bus-Mastering PCI attributes when device is started.
> Note that original PCI attributes are restored when device is stopped.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 5566b4cce467..531bed4e5ab7 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -305,8 +305,18 @@ PvScsiSetPciAttributes (
>    }
>  
>    //
> -  // TODO: Change PCI Attributes
> +  // Enable MMIO-Space & Bus-Mastering
>    //
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationEnable,
> +                         (EFI_PCI_IO_ATTRIBUTE_MEMORY |
> +                          EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
> +                         NULL
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
>  
>    return EFI_SUCCESS;
>  }
> 

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

Re: [edk2-devel] [PATCH v2 11/17] OvmfPkg/PvScsiDxe: Define device interface structures and constants

[Laszlo Ersek]

On 03/25/20 17:09, Liran Alon wrote:
> These definitions will be used by the following commits to complete the
> implementation of PVSCSI device driver.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/Include/IndustryStandard/PvScsi.h | 165 ++++++++++++++++++++++
>  1 file changed, 165 insertions(+)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/PvScsi.h b/OvmfPkg/Include/IndustryStandard/PvScsi.h
> index 004c0af84989..a4d6634f3ba0 100644
> --- a/OvmfPkg/Include/IndustryStandard/PvScsi.h
> +++ b/OvmfPkg/Include/IndustryStandard/PvScsi.h
> @@ -18,4 +18,169 @@
>  #define PCI_VENDOR_ID_VMWARE            (0x15ad)
>  #define PCI_DEVICE_ID_VMWARE_PVSCSI     (0x07c0)
>  
> +//
> +// CDB (Command Descriptor Block) with size above this constant
> +// should be considered out-of-band
> +//
> +#define PVSCSI_CDB_MAX_SIZE         (16)
> +
> +typedef enum {
> +  PvScsiRegOffsetCommand           =    0x0,
> +  PvScsiRegOffsetCommandData       =    0x4,
> +  PvScsiRegOffsetCommandStatus     =    0x8,
> +  PvScsiRegOffsetLastSts0          =  0x100,
> +  PvScsiRegOffsetLastSts1          =  0x104,
> +  PvScsiRegOffsetLastSts2          =  0x108,
> +  PvScsiRegOffsetLastSts3          =  0x10c,
> +  PvScsiRegOffsetIntrStatus        = 0x100c,
> +  PvScsiRegOffsetIntrMask          = 0x2010,
> +  PvScsiRegOffsetKickNonRwIo       = 0x3014,
> +  PvScsiRegOffsetDebug             = 0x3018,
> +  PvScsiRegOffsetKickRwIo          = 0x4018,
> +} PVSCSI_BAR0_OFFSETS;
> +
> +//
> +// Define Interrupt-Status register flags
> +//
> +#define PVSCSI_INTR_CMPL_0      BIT0
> +#define PVSCSI_INTR_CMPL_1      BIT1
> +#define PVSCSI_INTR_CMPL_MASK   (PVSCSI_INTR_CMPL_0 | PVSCSI_INTR_CMPL_1)
> +
> +typedef enum {
> +  PvScsiCmdFirst               = 0,
> +  PvScsiCmdAdapterReset        = 1,
> +  PvScsiCmdIssueScsi           = 2,
> +  PvScsiCmdSetupRings          = 3,
> +  PvScsiCmdResetBus            = 4,
> +  PvScsiCmdResetDevice         = 5,
> +  PvScsiCmdAbortCmd            = 6,
> +  PvScsiCmdConfig              = 7,
> +  PvScsiCmdSetupMsgRing        = 8,
> +  PvScsiCmdDeviceUnplug        = 9,
> +  PvScsiCmdLast                = 10
> +} PVSCSI_COMMANDS;
> +
> +#define PVSCSI_SETUP_RINGS_MAX_NUM_PAGES    (32)
> +
> +#pragma pack (1)
> +typedef struct {
> +  UINT32 ReqRingNumPages;
> +  UINT32 CmpRingNumPages;
> +  UINT64 RingsStatePPN;
> +  UINT64 ReqRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
> +  UINT64 CmpRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
> +} PVSCSI_CMD_DESC_SETUP_RINGS;
> +#pragma pack ()
> +
> +#define PVSCSI_MAX_CMD_DATA_WORDS   \
> +  (sizeof (PVSCSI_CMD_DESC_SETUP_RINGS) / sizeof (UINT32))
> +
> +#pragma pack (1)
> +typedef struct {
> +  UINT32 ReqProdIdx;
> +  UINT32 ReqConsIdx;
> +  UINT32 ReqNumEntriesLog2;
> +
> +  UINT32 CmpProdIdx;
> +  UINT32 CmpConsIdx;
> +  UINT32 CmpNumEntriesLog2;
> +
> +  UINT8  Pad[104];
> +
> +  UINT32 MsgProdIdx;
> +  UINT32 MsgConsIdx;
> +  UINT32 MsgNumEntriesLog2;
> +} PVSCSI_RINGS_STATE;
> +#pragma pack ()
> +
> +//
> +// Define PVSCSI request descriptor tags
> +//
> +#define PVSCSI_SIMPLE_QUEUE_TAG            (0x20)
> +
> +//
> +// Define PVSCSI request descriptor flags
> +//
> +#define PVSCSI_FLAG_CMD_WITH_SG_LIST       BIT0
> +#define PVSCSI_FLAG_CMD_OUT_OF_BAND_CDB    BIT1
> +#define PVSCSI_FLAG_CMD_DIR_NONE           BIT2
> +#define PVSCSI_FLAG_CMD_DIR_TOHOST         BIT3
> +#define PVSCSI_FLAG_CMD_DIR_TODEVICE       BIT4
> +
> +#pragma pack (1)
> +typedef struct {
> +  UINT64 Context;
> +  UINT64 DataAddr;
> +  UINT64 DataLen;
> +  UINT64 SenseAddr;
> +  UINT32 SenseLen;
> +  UINT32 Flags;
> +  UINT8  Cdb[16];
> +  UINT8  CdbLen;
> +  UINT8  Lun[8];
> +  UINT8  Tag;
> +  UINT8  Bus;
> +  UINT8  Target;
> +  UINT8  VcpuHint;
> +  UINT8  Unused[59];
> +} PVSCSI_RING_REQ_DESC;
> +#pragma pack ()
> +
> +//
> +// Host adapter status/error codes
> +//
> +typedef enum {
> +  PvScsiBtStatSuccess       = 0x00,  // CCB complete normally with no errors
> +  PvScsiBtStatLinkedCommandCompleted         = 0x0a,
> +  PvScsiBtStatLinkedCommandCompletedWithFlag = 0x0b,
> +  PvScsiBtStatDataUnderrun  = 0x0c,
> +  PvScsiBtStatSelTimeout    = 0x11,  // SCSI selection timeout
> +  PvScsiBtStatDatarun       = 0x12,  // Data overrun/underrun
> +  PvScsiBtStatBusFree       = 0x13,  // Unexpected bus free
> +  PvScsiBtStatInvPhase      = 0x14,  //
> +                                     // Invalid bus phase or sequence requested
> +                                     // by target
> +                                     //
> +  PvScsiBtStatLunMismatch   = 0x17,  //
> +                                     // Linked CCB has different LUN from first
> +                                     // CCB
> +                                     //
> +  PvScsiBtStatSensFailed    = 0x1b,  // Auto request sense failed
> +  PvScsiBtStatTagReject     = 0x1c,  //
> +                                     // SCSI II tagged queueing message rejected
> +                                     // by target
> +                                     //
> +  PvScsiBtStatBadMsg        = 0x1d,  //
> +                                     // Unsupported message received by the host
> +                                     // adapter
> +                                     //
> +  PvScsiBtStatHaHardware    = 0x20,  // Host adapter hardware failed
> +  PvScsiBtStatNoResponse    = 0x21,  //
> +                                     // Target did not respond to SCSI ATN sent
> +                                     // a SCSI RST
> +                                     //
> +  PvScsiBtStatSentRst       = 0x22,  // Host adapter asserted a SCSI RST
> +  PvScsiBtStatRecvRst       = 0x23,  // Other SCSI devices asserted a SCSI RST
> +  PvScsiBtStatDisconnect    = 0x24,  //
> +                                     // Target device reconnected improperly
> +                                     // (w/o tag)
> +                                     //
> +  PvScsiBtStatBusReset      = 0x25,  // Host adapter issued BUS device reset
> +  PvScsiBtStatAbortQueue    = 0x26,  // Abort queue generated
> +  PvScsiBtStatHaSoftware    = 0x27,  // Host adapter software error
> +  PvScsiBtStatHaTimeout     = 0x30,  // Host adapter hardware timeout error
> +  PvScsiBtStatScsiParity    = 0x34,  // SCSI parity error detected
> +} PVSCSI_HOST_BUS_ADAPTER_STATUS;
> +
> +#pragma pack (1)
> +typedef struct {
> +  UINT64 Context;
> +  UINT64 DataLen;
> +  UINT32 SenseLen;
> +  UINT16 HostStatus;
> +  UINT16 ScsiStatus;
> +  UINT32 Pad[2];
> +} PVSCSI_RING_CMP_DESC;
> +#pragma pack ()
> +
>  #endif // __PVSCSI_H_
> 

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

(Not an R-b because I haven't validated the various structures and
constants against the device model. But stylistically, this patch can
certainly be merged, now.)

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v2 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init

[Laszlo Ersek]

On 03/25/20 17:10, Liran Alon wrote:
> The following commits will complete the implementation of
> device initialization.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 91 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 531bed4e5ab7..831a78cc18c7 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -30,6 +30,84 @@
>  // Ext SCSI Pass Thru utilities
>  //
>  
> +/**
> +  Writes a 32-bit value into BAR0 using MMIO
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiMmioWrite32 (
> +  IN CONST PVSCSI_DEV   *Dev,
> +  IN UINT64             Offset,
> +  IN UINT32             Value
> +  )
> +{
> +  return Dev->PciIo->Mem.Write (
> +                           Dev->PciIo,
> +                           EfiPciIoWidthUint32,
> +                           PCI_BAR_IDX0,
> +                           Offset,
> +                           1,   // Count
> +                           &Value
> +                           );
> +}
> +
> +/**
> +  Writes multiple words of data into BAR0 using MMIO
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiMmioWrite32Multiple (
> +  IN CONST PVSCSI_DEV   *Dev,
> +  IN UINT64             Offset,
> +  IN UINTN              Count,
> +  IN UINT32             *Words
> +  )
> +{
> +  return Dev->PciIo->Mem.Write (
> +                           Dev->PciIo,
> +                           EfiPciIoWidthFifoUint32,
> +                           PCI_BAR_IDX0,
> +                           Offset,
> +                           Count,
> +                           Words
> +                           );
> +}
> +
> +/**
> +  Send PVSCSI command to device
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiWriteCmdDesc (
> +  IN CONST PVSCSI_DEV   *Dev,
> +  IN UINT32             Cmd,
> +  IN UINT32             *DescWords,
> +  IN UINTN              DescWordsCount
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  if (DescWordsCount > PVSCSI_MAX_CMD_DATA_WORDS) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetCommand, Cmd);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (DescWordsCount > 0) {
> +    return PvScsiMmioWrite32Multiple (
> +             Dev,
> +             PvScsiRegOffsetCommandData,
> +             DescWordsCount,
> +             DescWords
> +             );
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
>    EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -357,6 +435,14 @@ PvScsiInit (
>      return Status;
>    }
>  
> +  //
> +  // Reset adapter
> +  //
> +  Status = PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);
> +  if (EFI_ERROR (Status)) {
> +    goto RestorePciAttributes;
> +  }
> +
>    //
>    // Populate the exported interface's attributes
>    //
> @@ -387,6 +473,11 @@ PvScsiInit (
>    Dev->PassThruMode.IoAlign = 0;
>  
>    return EFI_SUCCESS;
> +
> +RestorePciAttributes:
> +  PvScsiRestorePciAttributes (Dev);
> +
> +  return Status;
>  }
>  
>  STATIC
> 

Nice.

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

Re: [PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings

[Laszlo Ersek]

On 03/25/20 17:10, Liran Alon wrote:
> These rings are shared memory buffers between host and device in which
> a cyclic buffer is managed to send request descriptors from host to
> device and receive completion descriptors from device to host.
>
> Note that because device may be constrained by IOMMU or guest may be run
> under AMD SEV, we make sure to map these rings to device by using
> PciIo->Map().
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 269 +++++++++++++++++++++++++++++++++++++
>  OvmfPkg/PvScsiDxe/PvScsi.h |  17 +++
>  2 files changed, 286 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 831a78cc18c7..59863f83c60c 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -16,6 +16,7 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
>  #include <Uefi/UefiSpec.h>
>
>  #include "PvScsi.h"
> @@ -413,6 +414,264 @@ PvScsiRestorePciAttributes (
>                  );
>  }
>
> +STATIC
> +EFI_STATUS
> +PvScsiAllocatePages (
> +  IN PVSCSI_DEV     *Dev,
> +  IN UINTN          Pages,
> +  IN OUT VOID       **HostAddress
> +  )
> +{
> +  return Dev->PciIo->AllocateBuffer (
> +                       Dev->PciIo,
> +                       AllocateAnyPages,
> +                       EfiBootServicesData,
> +                       Pages,
> +                       HostAddress,
> +                       EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> +                       );
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreePages (
> +  IN PVSCSI_DEV     *Dev,
> +  IN UINTN          Pages,
> +  IN VOID           *HostAddress
> +  )
> +{
> +  Dev->PciIo->FreeBuffer (
> +                Dev->PciIo,
> +                Pages,
> +                HostAddress
> +                );
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiMapBuffer (
> +  IN PVSCSI_DEV                     *Dev,
> +  IN EFI_PCI_IO_PROTOCOL_OPERATION  PciIoOperation,
> +  IN VOID                           *HostAddress,
> +  IN UINTN                          NumberOfBytes,
> +  OUT PVSCSI_DMA_DESC               *DmaDesc
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINTN      BytesMapped;
> +
> +  BytesMapped = NumberOfBytes;
> +  Status = Dev->PciIo->Map (
> +                         Dev->PciIo,
> +                         PciIoOperation,
> +                         HostAddress,
> +                         &BytesMapped,
> +                         &DmaDesc->DeviceAddress,
> +                         &DmaDesc->Mapping
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (BytesMapped != NumberOfBytes) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto Unmap;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +Unmap:
> +  Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiUnmapBuffer (
> +  IN PVSCSI_DEV                 *Dev,
> +  IN OUT PVSCSI_DMA_DESC        *DmaDesc)
> +{
> +  Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiAllocateSharedPages (
> +  IN PVSCSI_DEV                     *Dev,
> +  IN UINTN                          Pages,
> +  IN EFI_PCI_IO_PROTOCOL_OPERATION  PciIoOperation,
> +  OUT VOID                          **HostAddress,
> +  OUT PVSCSI_DMA_DESC               *DmaDesc
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  Status = PvScsiAllocatePages (Dev, Pages, HostAddress);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = PvScsiMapBuffer (
> +             Dev,
> +             PciIoOperation,
> +             *HostAddress,
> +             EFI_PAGES_TO_SIZE (Pages),
> +             DmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreePages;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +FreePages:
> +  PvScsiFreePages (Dev, Pages, *HostAddress);
> +
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeSharedPages (
> +  IN PVSCSI_DEV                     *Dev,
> +  IN UINTN                          Pages,
> +  IN OUT VOID                       *HostAddress,
> +  IN OUT PVSCSI_DMA_DESC            *DmaDesc
> +  )
> +{
> +  PvScsiUnmapBuffer (Dev, DmaDesc);
> +  PvScsiFreePages (Dev, Pages, HostAddress);
> +}

(1) From all of the above functions, please keep only two:
PvScsiAllocateSharedPages() and PvScsiFreeSharedPages(). The rest of the
helper functions should be flattened into them.

This is not a "frivolous" request -- it will make more sense once you
read my next request.

(2) Please eliminate the "PciIoOperation" parameter from the
PvScsiAllocateSharedPages() prototype, and open-code
"EfiPciIoOperationBusMasterCommonBuffer" in its place, in the
(flattened) implementation of the function.


Rationale: the buffer should be allocated separately with the
PciIo->AllocateBuffer() API *if and only if* a common buffer operation
is being set up. In other cases (one-shot bus master read, one-shot bus
master write), *only* the Map() operation should be used, and then Map()
and Unmap() will handle the bounce-buffering internally.

The functions above are misleading because they suggest they handle all
three bus master operations -- but for the one-shot BM read and write
operations, the PvScsiAllocateSharedPages() / PvScsiFreeSharedPages()
implementations are not correct.

Now, given that the call sites only set up common buffer operations, the
ultimately exercised logic is OK. And therefore the solution is not to
generalize the implementation uselessly (to BM read and BM write
operations too), but to tighten the interfaces such that they precisely
reflect the actual intended use case.

Hence, please open-code the common buffer operation internally to
PvScsiAllocateSharedPages(), and also stop exposing the separate
allocation / freeing / mapping / unmapping interfaces. The call sites
need nothing else than (a) "give me a completely mapped range for common
buffer operation" and (b) "release that".

Here's the (untested, not even compiled) implementation I propose:

> STATIC
> EFI_STATUS
> PvScsiAllocateSharedPages (
>   IN PVSCSI_DEV       *Dev,
>   IN UINTN            Pages,
>   OUT VOID            **HostAddress,
>   OUT PVSCSI_DMA_DESC *DmaDesc
>   )
> {
>   EFI_STATUS Status;
>   UINTN      NumberOfBytes;
>
>   Status = Dev->PciIo->AllocateBuffer (
>                          Dev->PciIo,
>                          AllocateAnyPages,
>                          EfiBootServicesData,
>                          Pages,
>                          HostAddress,
>                          EFI_PCI_ATTRIBUTE_MEMORY_CACHED
>                          );
>   if (EFI_ERROR (Status)) {
>     return Status;
>   }
>
>   NumberOfBytes = EFI_PAGES_TO_SIZE (Pages);
>   Status = Dev->PciIo->Map (
>                          Dev->PciIo,
>                          EfiPciIoOperationBusMasterCommonBuffer,
>                          *HostAddress,
>                          &NumberOfBytes,
>                          &DmaDesc->DeviceAddress,
>                          &DmaDesc->Mapping
>                          );
>   if (EFI_ERROR (Status)) {
>     goto FreeBuffer;
>   }
>
>   if (NumberOfBytes != EFI_PAGES_TO_SIZE (Pages)) {
>     Status = EFI_OUT_OF_RESOURCES;
>     goto Unmap;
>   }
>
>   return EFI_SUCCESS;
>
> Unmap:
>   Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
>
> FreeBuffer:
>   Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, *HostAddress);
>
>   return Status;
> }
>
> STATIC
> VOID
> PvScsiFreeSharedPages (
>   IN PVSCSI_DEV      *Dev,
>   IN UINTN           Pages,
>   IN VOID            *HostAddress,
>   IN PVSCSI_DMA_DESC *DmaDesc
>   )
> {
>   Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
>   Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, HostAddress);
> }

Back to your patch:

On 03/25/20 17:10, Liran Alon wrote:
> +
> +STATIC
> +EFI_STATUS
> +PvScsiInitRings (
> +  IN OUT PVSCSI_DEV *Dev
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
> +
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             1,
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->RingDesc.RingState,
> +             &Dev->RingDesc.RingStateDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
> +
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             1,
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->RingDesc.RingReqs,
> +             &Dev->RingDesc.RingReqsDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeRingState;
> +  }
> +  ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
> +
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             1,
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->RingDesc.RingCmps,
> +             &Dev->RingDesc.RingCmpsDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeRingReqs;
> +  }
> +  ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
> +
> +  ZeroMem (&Cmd, sizeof (Cmd));
> +  Cmd.ReqRingNumPages = 1;
> +  Cmd.CmpRingNumPages = 1;
> +  Cmd.RingsStatePPN = RShiftU64 (
> +                        (UINT64)Dev->RingDesc.RingStateDmaDesc.DeviceAddress,
> +                        EFI_PAGE_SHIFT
> +                        );
> +  Cmd.ReqRingPPNs[0] = RShiftU64 (
> +                         (UINT64)Dev->RingDesc.RingReqsDmaDesc.DeviceAddress,
> +                         EFI_PAGE_SHIFT
> +                         );
> +  Cmd.CmpRingPPNs[0] = RShiftU64 (
> +                         (UINT64)Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress,
> +                         EFI_PAGE_SHIFT
> +                         );

(3) Please drop the explicit UINT64 casts -- for brevity --,
EFI_PHYSICAL_ADDRESS is just a different name (a typedef) for UINT64, by
spec. (See in the EFI_BOOT_SERVICES.AllocatePages() chapter in the UEFI
spec.)

(4) Please #include <Library/BaseLib.h> in the C file, and also list
BaseLib under [LibraryClasses] in the INF file. RShiftU64() comes from
there.

> +
> +  Status = PvScsiWriteCmdDesc (
> +             Dev,
> +             PvScsiCmdSetupRings,
> +             (UINT32 *)&Cmd,

(5) So I guess this is where the "union trick" will come, in v3.

> +             sizeof (Cmd) / sizeof (UINT32)

(6) Can you please try inserting, before the PvScsiWriteCmdDesc() call:

  STATIC_ASSERT (sizeof Cmd % sizeof (UINT32) == 0, "invalid Cmd size");

> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeRingCmps;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +FreeRingCmps:
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingCmps,
> +    &Dev->RingDesc.RingCmpsDmaDesc
> +    );
> +
> +FreeRingReqs:
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingReqs,
> +    &Dev->RingDesc.RingReqsDmaDesc
> +    );
> +
> +FreeRingState:
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingState,
> +    &Dev->RingDesc.RingStateDmaDesc
> +    );
> +
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeRings (
> +  IN OUT PVSCSI_DEV *Dev
> +  )
> +{
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingCmps,
> +    &Dev->RingDesc.RingCmpsDmaDesc
> +    );
> +
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingReqs,
> +    &Dev->RingDesc.RingReqsDmaDesc
> +    );
> +
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    Dev->RingDesc.RingState,
> +    &Dev->RingDesc.RingStateDmaDesc
> +    );
> +}
> +

(7) This function is almost correct.

Note that your last *such* action in PvScsiInitRings() that impacts any
resource life cycle is the *exposure* of the rings to the device model
(the hypervisor).

Therefore, after PvScsiInitRings() succeeds, then on any subsequently
occurring error path, you first need to make the device model forget
about the rings, before you release the rings. A hypervisor-side
reference to guest memory is just another resource you "allocate", so
you need to drop it too, in reverse order of construction.

In brief, please add the following to the top of the PvScsiFreeRings()
function:

  PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0);

(Consider, for example, what would happen *otherwise*, if the DmaBuf
allocation failed -- in the next patch -- in the PvScsiInit() function.

There, you jump to "FreeRings" correctly, and release the rings with
PvScsiFreeRings() -- but the hypervisor still remembers them!)

>  STATIC
>  EFI_STATUS
>  PvScsiInit (
> @@ -443,6 +702,14 @@ PvScsiInit (
>      goto RestorePciAttributes;
>    }
>
> +  //
> +  // Init PVSCSI rings
> +  //
> +  Status = PvScsiInitRings (Dev);
> +  if (EFI_ERROR (Status)) {
> +    goto RestorePciAttributes;
> +  }
> +
>    //
>    // Populate the exported interface's attributes
>    //
> @@ -486,6 +753,8 @@ PvScsiUninit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  PvScsiFreeRings (Dev);
> +
>    PvScsiRestorePciAttributes (Dev);
>  }
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 5f611dbbc98c..6d23b6e1eccf 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -15,12 +15,29 @@
>  #include <Library/DebugLib.h>
>  #include <Protocol/ScsiPassThruExt.h>
>
> +typedef struct {
> +  EFI_PHYSICAL_ADDRESS DeviceAddress;
> +  VOID                 *Mapping;
> +} PVSCSI_DMA_DESC;
> +
> +typedef struct {
> +  PVSCSI_RINGS_STATE   *RingState;
> +  PVSCSI_DMA_DESC      RingStateDmaDesc;
> +
> +  PVSCSI_RING_REQ_DESC *RingReqs;
> +  PVSCSI_DMA_DESC      RingReqsDmaDesc;
> +
> +  PVSCSI_RING_CMP_DESC *RingCmps;
> +  PVSCSI_DMA_DESC      RingCmpsDmaDesc;
> +} PVSCSI_RING_DESC;
> +
>  #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>
>  typedef struct {
>    UINT32                          Signature;
>    EFI_PCI_IO_PROTOCOL             *PciIo;
>    UINT64                          OriginalPciAttributes;
> +  PVSCSI_RING_DESC                RingDesc;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>

These look fine.

(8) A general request regarding your git setup: please run the
"BaseTools/Scripts/SetupGit.py" script in your edk2 clone.

The reason I'm asking for that right now is that the script will point
the "diff.orderFile" setting to "BaseTools/Conf/diff.order". And that
will make git-format-patch, on your end, place the header file changes
ahead of the C file changes, in the generated emails.

It helps reviewers greatly if they can first see the declarative changes
(such as new types, new structure members, and so on), before they face
the code using the new constructs.

The script in question configures a bunch of other cool stuff too that
boosts reviewer productivity.

Thanks,
Laszlo

Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

[Laszlo Ersek]

On 03/25/20 17:10, Liran Alon wrote:
> In case device is constrained by IOMMU or guest is running under AMD SEV,
> input/output buffers provided to device (DataBuffer and SenseData) needs
> to be explicitly mapped to device by PciIo->Map().
> 
> To avoid the overhead of mapping/unmapping the DataBuffer and SenseData
> to the device for every SCSI requst (And to simplify code), introduce a

(1) s/And/and/

> single DMA communication buffer that will be mapped to device on
> initialization. When a SCSI request needs to be sent to device, the
> DataBuffer and SenseData will be copied from/to the DMA communication
> buffer as required. This will be done by the following commits.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 27 +++++++++++++++++++++++++++
>  OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 59863f83c60c..928984099520 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -710,6 +710,20 @@ PvScsiInit (
>      goto RestorePciAttributes;
>    }
>  
> +  //
> +  // Allocate DMA communication buffer
> +  //
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->DmaBuf,
> +             &Dev->DmaBufDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeRings;
> +  }
> +
>    //
>    // Populate the exported interface's attributes
>    //
> @@ -741,6 +755,9 @@ PvScsiInit (
>  
>    return EFI_SUCCESS;
>  
> +FreeRings:
> +  PvScsiFreeRings (Dev);
> +
>  RestorePciAttributes:
>    PvScsiRestorePciAttributes (Dev);
>  
> @@ -753,6 +770,16 @@ PvScsiUninit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  //
> +  // Free DMA communication buffer
> +  //
> +  PvScsiFreeSharedPages (

(2) From peeking ahead at the next patch, the following seems possible:

- we send a data transfer request to the device model, passing some
pointers into the DMA communication buffer to the hypervisor

- PvScsiWaitForRequestCompletion() fails (for whatever reason), and so
we can't be sure whether the device is completely done with the buffer
that we exposed to it.

Therefore, please *prepend* a Reset operation to this
PvScsiFreeSharedPages() call as well, at the top of PvScsiUninit().

(Note: we could be tempted to somehow "centralize" all of these Reset
operations into a single spot. Bad idea. We are revoking the device's
access rights to different resources, so the revocation operations will
show up in different spots. It's a mere circumstance that the
revocations all happen to be Reset operations.)

I might be paranoid of course -- I just feel that maybe-superfluous
reset operations on error paths are much better than silently corrupted
guest memory and/or disk contents.

> +    Dev,
> +    EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> +    (VOID **)&Dev->DmaBuf,

(3) Copy-paste typo: you should only pass "Dev->DmaBuf".

The compiler doesn't catch this for you because PvScsiFreeSharedPages()
takes a "VOID *HostAddress" here, and (VOID **) -- like all other
pointer-to-object types -- converts to (VOID *) silently.

> +    &Dev->DmaBufDmaDesc
> +    );
> +
>    PvScsiFreeRings (Dev);
>  
>    PvScsiRestorePciAttributes (Dev);
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 6d23b6e1eccf..7f91d70fec79 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -31,6 +31,11 @@ typedef struct {
>    PVSCSI_DMA_DESC      RingCmpsDmaDesc;
>  } PVSCSI_RING_DESC;
>  
> +typedef struct {
> +  UINT8     SenseData[MAX_UINT8];

(4) Is the maximum possible size of the sense data specified somewhere?
If so, it would be nice to document it with a comment at least.

> +  UINT8     Data[0x2000];

(5) Same here. From peeking at the next patch, we seem to be choosing
this size arbitrarily.

If it works for you in all relevant boot scenarios, I'm OK with it, but
we should be clear that this value is arbitrarily chosen. No need for a
#define, but a comment would be nice.

(6) Should we declare this structure as packed?

> +} PVSCSI_DMA_BUFFER;
> +
>  #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>  
>  typedef struct {
> @@ -38,6 +43,8 @@ typedef struct {
>    EFI_PCI_IO_PROTOCOL             *PciIo;
>    UINT64                          OriginalPciAttributes;
>    PVSCSI_RING_DESC                RingDesc;
> +  PVSCSI_DMA_BUFFER               *DmaBuf;
> +  PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> @@ -47,4 +54,7 @@ typedef struct {
>  #define PVSCSI_FROM_PASS_THRU(PassThruPointer) \
>    CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG)
>  
> +#define PVSCSI_DMA_BUF_DEV_ADDR(Dev, MemberName) \
> +  (Dev->DmaBufDmaDesc.DeviceAddress + OFFSET_OF(PVSCSI_DMA_BUFFER, MemberName))
> +
>  #endif // __PVSCSI_DXE_H_
> 

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v2 17/17] OvmfPkg/PvScsiDxe: Enable device 64-bit DMA addresses

[Laszlo Ersek]

On 03/25/20 17:10, Liran Alon wrote:
> Enable PCI dual-address cycle attribute to signal device
> supports 64-bit DMA addresses.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 42a18494efb7..3aea5c7f8eda 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -836,6 +836,29 @@ PvScsiSetPciAttributes (
>      return Status;
>    }
>  
> +  //
> +  // Signal device supports 64-bit DMA addresses
> +  //
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationEnable,
> +                         EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
> +                         NULL
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // Warn user that device will only be using 32-bit DMA addresses.
> +    //
> +    // Note that this does not prevent the device/driver from working
> +    // and therefore we only warn and continue as usual.
> +    //
> +    DEBUG ((
> +      DEBUG_WARN,
> +      "%a: failed to enable 64-bit DMA addresses\n",
> +      __FUNCTION__
> +      ));
> +  }
> +
>    return EFI_SUCCESS;
>  }
>  
> 

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

Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

[Liran Alon]

On 27/03/2020 0:17, Laszlo Ersek wrote:
> On 03/25/20 17:10, Liran Alon wrote:
>>     PvScsiRestorePciAttributes (Dev);
>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>> index 6d23b6e1eccf..7f91d70fec79 100644
>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>> @@ -31,6 +31,11 @@ typedef struct {
>>     PVSCSI_DMA_DESC      RingCmpsDmaDesc;
>>   } PVSCSI_RING_DESC;
>>   
>> +typedef struct {
>> +  UINT8     SenseData[MAX_UINT8];
> (4) Is the maximum possible size of the sense data specified somewhere?
> If so, it would be nice to document it with a comment at least.
This is a good point. Thanks for pointing it out.
Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1 Descriptor 
format sense data overview specifies:
"The additional sense length shall be less than or equal to 244 (i.e., 
limiting the total length of the sense data to 252 bytes)"

i.e. The max possible size of sense data is 252.
As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed defining:

#define SCSI_SENSE_BUF_SIZE 252

This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi 
sense buf size to 252") which says in it's commit message:
"According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."

Interestingly, this QEMU commit changed the value of SCSI_SENSE_BUF_SIZE 
from 96 to 252.
But then I found this in EDK2 OvmfPkg/Include/IndustryStandard/VirtioScsi.h:

//
// We expect these maximum sizes from the host. Also we force the 
CdbLength and
// SenseDataLength parameters of 
EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
// to exceed these limits. See UEFI 2.3.1 errata C 14.7.
//
#define VIRTIO_SCSI_CDB_SIZE   32
#define VIRTIO_SCSI_SENSE_SIZE 96

Which seems to be wrong...

It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to EDK2 
MdePkg/Include/IndustryStandard/Scsi.h.
And then use that from my PvScsi driver.

I can also submit a separate patch (Not part of this series) to remove 
this VIRTIO_SCSI_SENSE_SIZE constant.
Note that VirtioScsi doesn't have a technical issue here because it 
provides this max sense length to the device in VirtioScsiInit():

Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);

So if it's OK by you, I think I will just add a patch to this series 
defining SCSI_MAX_SENSE_SIZE in MdePkg/Include/IndustryStandard/Scsi.h,
and then rely on that when defining SenseData in PVSCSI_DMA_BUFFER.

>> +  UINT8     Data[0x2000];
> (5) Same here. From peeking at the next patch, we seem to be choosing
> this size arbitrarily.
This is indeed arbitrary...
>
> If it works for you in all relevant boot scenarios, I'm OK with it, but
> we should be clear that this value is arbitrarily chosen. No need for a
> #define, but a comment would be nice.

OK. Will just add a comment such as:

//
// This size is arbitrarily chosen,
// It seems to be sufficient for all I/O requests sent through 
EFI_SCSI_PASS_THRU_PROTOCOL.PassThru().
//

If you wish to have something else, please say so. :)

>
> (6) Should we declare this structure as packed?
There is no such requirement as it's not a wire-format or anything like 
that.
The only rational of defining it as packed is to avoid mapping 
unnecessary pages to device.
But this structure is less than a page-size anyway. So I think it's fine.

If you still think it should be marked as packed, I can change this.

Thanks!
-Liran

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Laszlo Ersek]

On 03/25/20 17:10, Liran Alon wrote:
> Implement EXT_SCSI_PASS_THRU.PassThru().
>
> Machines should be able to boot after this commit.
> Tested with Ubuntu 16.04 guest.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/OvmfPkg.dec             |   6 +
>  OvmfPkg/PvScsiDxe/PvScsi.c      | 442 +++++++++++++++++++++++++++++++-
>  OvmfPkg/PvScsiDxe/PvScsi.h      |   1 +
>  OvmfPkg/PvScsiDxe/PvScsiDxe.inf |   5 +-
>  4 files changed, 451 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index a04aee5c2cd4..ff49ec0e9e6a 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -130,6 +130,12 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x36
>    gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x37
>
> +  ## After PvScsiDxe sends a SCSI request to the device, it waits for
> +  #  the request completion in a polling loop.
> +  #  This constant defines how many micro-seconds to wait between each
> +  #  polling loop iteration.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38
> +
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 928984099520..de4122e39a81 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -31,6 +31,27 @@
>  // Ext SCSI Pass Thru utilities
>  //
>
> +/**
> +  Reads a 32-bit value into BAR0 using MMIO
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiMmioRead32 (
> +  IN CONST PVSCSI_DEV   *Dev,
> +  IN UINT64             Offset,
> +  OUT UINT32            *Value
> +  )
> +{
> +  return Dev->PciIo->Mem.Read (
> +                           Dev->PciIo,
> +                           EfiPciIoWidthUint32,
> +                           PCI_BAR_IDX0,
> +                           Offset,
> +                           1,   // Count
> +                           Value
> +                           );
> +}
> +
>  /**
>    Writes a 32-bit value into BAR0 using MMIO
>  **/
> @@ -109,6 +130,352 @@ PvScsiWriteCmdDesc (
>    return EFI_SUCCESS;
>  }
>
> +/**
> +  Returns if PVSCSI request ring is full
> +**/
> +STATIC
> +BOOLEAN
> +PvScsiIsReqRingFull (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingsState;
> +  UINT32             ReqNumEntries;
> +
> +  RingsState = Dev->RingDesc.RingState;
> +  ReqNumEntries = 1U << RingsState->ReqNumEntriesLog2;
> +  return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >= ReqNumEntries;
> +}

(Just some thoughts, not a request for changing the code.)

Normally I prefer accessing buffers shared with the device though
volatile-qualified  pointers.

Meaning, in this case, that every "PCI host" pointer (i.e., each pointer
that is associated with a PVSCSI_DMA_DESC) would have to be
volatile-qualified. In particular:

- in patch#13, PVSCSI_RING_DESC would have to be updated like this:

> typedef struct {
>   volatile PVSCSI_RINGS_STATE   *RingState;
>   PVSCSI_DMA_DESC               RingStateDmaDesc;
>
>   volatile PVSCSI_RING_REQ_DESC *RingReqs;
>   PVSCSI_DMA_DESC               RingReqsDmaDesc;
>
>   volatile PVSCSI_RING_CMP_DESC *RingCmps;
>   PVSCSI_DMA_DESC               RingCmpsDmaDesc;
> } PVSCSI_RING_DESC;

- in patch#14, PVSCSI_DEV would change as follows:

> typedef struct {
>   UINT32                          Signature;
>   EFI_PCI_IO_PROTOCOL             *PciIo;
>   EFI_EVENT                       ExitBoot;
>   UINT64                          OriginalPciAttributes;
>   PVSCSI_RING_DESC                RingDesc;
>   volatile PVSCSI_DMA_BUFFER      *DmaBuf;
>   PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>   UINT8                           MaxTarget;
>   UINT8                           MaxLun;
>   UINTN                           WaitForCmpStallInUsecs;
>   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>   EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
> } PVSCSI_DEV;

After these changes, the compiler would (justifiedly) flag a bunch of
code locations casting away the volatile qualification -- for example,
in the above function, in the assignment to the "RingsState" local
variable.

Clearly, most of these compilation errors would have to be fixed (not
suppressed), because they would be valid. Meaning:

- you'd have to volatile-qualify the "RingsState" local variable in all
  of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(),
  PvScsiWaitForRequestCompletion();

- you'd also have to volatile-qualify the return types of
  PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion();

- you'd have to update PopulateRequest() and HandleResponse() too; and
  the most annoying part of that would be that you could no longer use
  CopyMem() and ZeroMem() -- because those functions take
  pointer-to-void parameters, rather than pointer-to-volatile-void ones.

(FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype
-- it would be OK to cast away volatile in those calls, as we wouldn't
dereference the pointers in that case.)

So... the reason I'm not actually requesting these
volatile-qualifications is that (a) your use of MemoryFence() seems
mostly OK, and (b) the UEFI Driver Writer's guide recommends *either*
volatile *or* MemoryFence(). Of course using both techniques at the same
time is not a problem -- and in code I write I actually like to use both
at the same time --, but just one suffices too. (See section 4.2.6
"Memory ordering" in the DWG.)

The reason I'm writing this up here is because I want the "record" (the
mailing list archive) to show that we have considered this topic
explicitly.

Back to your patch:

On 03/25/20 17:10, Liran Alon wrote:
> +
> +/**
> +  Returns pointer to current request descriptor to produce
> +**/
> +STATIC
> +PVSCSI_RING_REQ_DESC *
> +PvScsiGetCurrentRequest (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingState;
> +  UINT32             ReqNumEntries;
> +
> +  RingState = Dev->RingDesc.RingState;
> +  ReqNumEntries = 1U << RingState->ReqNumEntriesLog2;
> +  return Dev->RingDesc.RingReqs +
> +         (RingState->ReqProdIdx & (ReqNumEntries - 1));
> +}
> +
> +/**
> +  Returns pointer to current completion descriptor to consume
> +**/
> +STATIC
> +PVSCSI_RING_CMP_DESC *
> +PvScsiGetCurrentResponse (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingState;
> +  UINT32             CmpNumEntries;
> +
> +  RingState = Dev->RingDesc.RingState;
> +  CmpNumEntries = 1U << RingState->CmpNumEntriesLog2;
> +  return Dev->RingDesc.RingCmps +
> +         (RingState->CmpConsIdx & (CmpNumEntries - 1));
> +}
> +
> +/**
> +  Wait for device to signal completion of submitted requests
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiWaitForRequestCompletion (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINT32     IntrStatus;
> +
> +  //
> +  // Note: We don't yet support Timeout according to
> +  // EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.Timeout.
> +  //
> +  // This is consistent with some other Scsi PassThru drivers
> +  // such as VirtioScsi.
> +  //
> +  for (;;) {
> +    Status = PvScsiMmioRead32 (Dev, PvScsiRegOffsetIntrStatus, &IntrStatus);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    //
> +    // PVSCSI_INTR_CMPL_MASK is set if device completed submitted requests
> +    //
> +    if ((IntrStatus & PVSCSI_INTR_CMPL_MASK) != 0) {
> +        break;

(1) Wrong indentation.

> +    }
> +
> +    gBS->Stall (Dev->WaitForCmpStallInUsecs);
> +  }
> +
> +  //
> +  // Acknowledge PVSCSI_INTR_CMPL_MASK in device interrupt-status register
> +  //
> +  return PvScsiMmioWrite32 (
> +           Dev,
> +           PvScsiRegOffsetIntrStatus,
> +           PVSCSI_INTR_CMPL_MASK
> +           );
> +}
> +
> +/**
> +  Populate a PVSCSI request descriptor from the Extended SCSI Pass Thru
> +  Protocol packet.
> +**/
> +STATIC
> +EFI_STATUS
> +PopulateRequest (
> +  IN CONST PVSCSI_DEV                               *Dev,
> +  IN UINT8                                          *Target,
> +  IN UINT64                                         Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  OUT PVSCSI_RING_REQ_DESC                          *Request
> +  )
> +{
> +  UINT8 TargetValue;
> +
> +  //
> +  // We only use first byte of target identifer
> +  //
> +  TargetValue = *Target;
> +
> +  //
> +  // Check for unsupported requests
> +  //
> +  if (
> +      //
> +      // Bidirectional transfer was requested
> +      //
> +      (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
> +      (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
> +      //
> +      // Command Descriptor Block bigger than this constant should be considered
> +      // out-of-band. We currently don't support these CDBs.
> +      //
> +      (Packet->CdbLength > PVSCSI_CDB_MAX_SIZE)
> +      ) {
> +
> +    //
> +    // This error code doesn't require updates to the Packet output fields
> +    //
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // Check for invalid parameters
> +  //
> +  if (
> +      //
> +      // Addressed invalid device
> +      //
> +      (TargetValue > Dev->MaxTarget) || (Lun > Dev->MaxLun) ||
> +      //
> +      // Invalid direction (there doesn't seem to be a macro for the "no data
> +      // transferred" "direction", eg. for TEST UNIT READY)
> +      //
> +      (Packet->DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
> +      //
> +      // Trying to receive, but destination pointer is NULL, or contradicting
> +      // transfer direction
> +      //
> +      ((Packet->InTransferLength > 0) &&
> +       ((Packet->InDataBuffer == NULL) ||
> +        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE)
> +        )
> +       ) ||
> +      //
> +      // Trying to send, but source pointer is NULL, or contradicting
> +      // transfer direction
> +      //
> +      ((Packet->OutTransferLength > 0) &&
> +       ((Packet->OutDataBuffer == NULL) ||
> +        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ)
> +        )
> +       )
> +      ) {
> +
> +    //
> +    // This error code doesn't require updates to the Packet output fields
> +    //
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Check for input/output buffer too large for DMA communication buffer
> +  //
> +  if (Packet->InTransferLength > sizeof (Dev->DmaBuf->Data)) {
> +    Packet->InTransferLength = sizeof (Dev->DmaBuf->Data);
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +  if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) {
> +    Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data);
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  //
> +  // Encode PVSCSI request
> +  //
> +  ZeroMem (Request, sizeof (*Request));
> +
> +  Request->Bus = 0;
> +  Request->Target = TargetValue;
> +  //
> +  // This cast is safe as MaxLun is defined as UINT8
> +  //
> +  Request->Lun[1] = (UINT8)Lun;
> +  Request->SenseLen = Packet->SenseDataLength;

Ah, *now* I understand why you chose MAX_UINT8 as the size of
"PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has
type UINT8, and this way you guarantee that the SCSI client's
"Packet->SenseDataLength" will always fit in the DMA buffer.

Good solution, but it *absolutely* needs to be documented in patch#14
("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact,
see my question (4) under patch#14.

(2) Also, please add a comment here that a "Dev->DmaBuf->SenseData"
overflow is not possible due to "Packet->SenseDataLength" having type
UINT8.

This would be a comment in the same vein as the "MaxLun" reference just
above -- I find *that* comment very helpful, too.

> +  Request->SenseAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, SenseData);

(3) Please drop the (UINT64) cast -- see my earlier comment about
EFI_PHYSICAL_ADDRESS being a typedef to UINT64.

> +  Request->CdbLen = Packet->CdbLength;
> +  CopyMem (Request->Cdb, Packet->Cdb, Packet->CdbLength);

Right, this is safe, due to the PVSCSI_CDB_MAX_SIZE near the top of the
function (PVSCSI_CDB_MAX_SIZE is 16, and "sizeof
PVSCSI_RING_REQ_DESC.Cdb" is also 16).

> +  Request->VcpuHint = 0;
> +  Request->Tag = PVSCSI_SIMPLE_QUEUE_TAG;
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    Request->Flags = PVSCSI_FLAG_CMD_DIR_TOHOST;
> +    Request->DataLen = Packet->InTransferLength;
> +  } else {
> +    Request->Flags = PVSCSI_FLAG_CMD_DIR_TODEVICE;
> +    Request->DataLen = Packet->OutTransferLength;
> +    CopyMem (
> +      Dev->DmaBuf->Data,
> +      Packet->OutDataBuffer,
> +      Packet->OutTransferLength
> +      );
> +  }
> +  Request->DataAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, Data);

(4) Same as (3).

> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Handle the PVSCSI device response:
> +  - Copy returned data from DMA communication buffer.
> +  - Update fields in Extended SCSI Pass Thru Protocol packet as required.
> +  - Translate response code to EFI status code and host adapter status.
> +**/
> +STATIC
> +EFI_STATUS
> +HandleResponse (
> +  IN PVSCSI_DEV                                     *Dev,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN CONST PVSCSI_RING_CMP_DESC                     *Response
> +  )
> +{
> +  //
> +  // Check if device returned sense data
> +  //
> +  if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
> +    //
> +    // Fix SenseDataLength to amount of data returned
> +    //
> +    if (Packet->SenseDataLength > Response->SenseLen) {
> +      Packet->SenseDataLength = (UINT8)Response->SenseLen;
> +    }
> +    //
> +    // Copy sense data from DMA communication buffer
> +    //
> +    CopyMem (
> +      Packet->SenseData,
> +      Dev->DmaBuf->SenseData,
> +      Packet->SenseDataLength
> +      );
> +  } else {
> +    //
> +    // Signal no sense data returned
> +    //
> +    Packet->SenseDataLength = 0;
> +  }
> +
> +  //
> +  // Copy device output from DMA communication buffer
> +  //
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
> +  }

I'm unfamilar with the PVSCSI device model, but I think this is not
general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that
short reads are possible at least in theory.

(5) If a short read occurs (Response->DataLen <
Packet->InTransferLength), then we should adjust
"Packet->InTransferLength", and also copy that many bytes only.

(6) I think it would be prudent to update "Packet->OutTransferLength"
too, for short writes.

> +
> +  //
> +  // Report target status
> +  //
> +  Packet->TargetStatus = Response->ScsiStatus;
> +
> +  //
> +  // Host adapter status and function return value depend on
> +  // device response's host status
> +  //
> +  switch (Response->HostStatus) {
> +    case PvScsiBtStatSuccess:
> +    case PvScsiBtStatLinkedCommandCompleted:
> +    case PvScsiBtStatLinkedCommandCompletedWithFlag:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +      return EFI_SUCCESS;
> +
> +    case PvScsiBtStatSelTimeout:
> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> +      return EFI_TIMEOUT;
> +
> +    case PvScsiBtStatDatarun:
> +    case PvScsiBtStatDataUnderrun:
> +      //
> +      // Report residual data in overrun/underrun
> +      //
> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +        Packet->InTransferLength = Response->DataLen;
> +      } else {
> +        Packet->OutTransferLength = Response->DataLen;
> +      }

OK, if we are sure that (a) the device will always report short
reads/writes like this, and that (b) the above assignments will never
cause InTransferLength / OutTransferLength to *grow*, then the
InTransferLength / OutTransferLength adjustments are sufficiently
covered.

Still:

(7) Please cast DataLen to UINT32, otherwise Visual Studio might whine.

(8) The CopyMem() call above should not copy garbage (at the tail).

Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
then I would prefer if:

- we always updated InTransferLength / OutTransferLength (regardless of
"Response->HostStatus"),

- and we only used these case labels (PvScsiBtStatDatarun /
PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".

> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> +      return EFI_BAD_BUFFER_SIZE;

I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
But that's not the case here -- we do have a partially completed
transfer.

(9) Thus I feel we should use a "break" here.

> +
> +    case PvScsiBtStatBusFree:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE;
> +      break;
> +
> +    case PvScsiBtStatInvPhase:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR;
> +      break;
> +
> +    case PvScsiBtStatSensFailed:
> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED;
> +      break;
> +
> +    case PvScsiBtStatTagReject:
> +    case PvScsiBtStatBadMsg:
> +      Packet->HostAdapterStatus =
> +          EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT;
> +      break;
> +
> +    case PvScsiBtStatBusReset:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
> +      break;
> +
> +    case PvScsiBtStatHaTimeout:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
> +      return EFI_TIMEOUT;
> +
> +    case PvScsiBtStatScsiParity:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
> +      break;
> +
> +    default:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +      break;
> +  }
> +
> +  return EFI_DEVICE_ERROR;
> +}
> +
>  /**
>    Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
>    EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -129,6 +496,23 @@ IsTargetInitialized (
>    return FALSE;
>  }
>
> +/**
> +  Create a fake host adapter error
> +**/
> +STATIC
> +EFI_STATUS
> +ReportHostAdapterError (
> +  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  Packet->InTransferLength = 0;
> +  Packet->OutTransferLength = 0;
> +  Packet->SenseDataLength = 0;
> +  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +  Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +  return EFI_DEVICE_ERROR;
> +}
> +
>  //
>  // Ext SCSI Pass Thru
>  //
> @@ -144,7 +528,62 @@ PvScsiPassThru (
>    IN EFI_EVENT                                      Event    OPTIONAL
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  PVSCSI_DEV            *Dev;
> +  EFI_STATUS            Status;
> +  PVSCSI_RING_REQ_DESC *Request;
> +  PVSCSI_RING_CMP_DESC *Response;
> +
> +  Dev = PVSCSI_FROM_PASS_THRU (This);
> +
> +  if (PvScsiIsReqRingFull (Dev)) {
> +    return EFI_NOT_READY;
> +  }
> +
> +  Request = PvScsiGetCurrentRequest (Dev);
> +
> +  Status = PopulateRequest (Dev, Target, Lun, Packet, Request);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // Writes to Request must be globally visible before making request
> +  // available to device
> +  //
> +  MemoryFence ();
> +  Dev->RingDesc.RingState->ReqProdIdx++;
> +

(10) Please insert another MemoryFence () here.

> +  Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetKickRwIo, 0);
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // If kicking the host fails, we must fake a host adapter error.
> +    // EFI_NOT_READY would save us the effort, but it would also suggest that
> +    // the caller retry.
> +    //
> +    return ReportHostAdapterError (Packet);
> +  }
> +
> +  Status = PvScsiWaitForRequestCompletion (Dev);
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // If waiting for request completion fails, we must fake a host adapter
> +    // error. EFI_NOT_READY would save us the effort, but it would also suggest
> +    // that the caller retry.
> +    //
> +    return ReportHostAdapterError (Packet);
> +  }
> +

(11) Please insert a MemoryFence() here.

> +  Response = PvScsiGetCurrentResponse (Dev);
> +  Status = HandleResponse (Dev, Packet, Response);
> +
> +  //
> +  // Reads from response must complete before releasing completion entry
> +  // to device
> +  //
> +  MemoryFence ();
> +  Dev->RingDesc.RingState->CmpConsIdx++;
> +
> +  return Status;
>  }
>
>  STATIC
> @@ -685,6 +1124,7 @@ PvScsiInit (
>    //
>    Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
>    Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
> +  Dev->WaitForCmpStallInUsecs = PcdGet32 (PcdPvScsiWaitForCmpStallInUsecs);
>
>    //
>    // Set PCI Attributes
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 7f91d70fec79..08e876b75930 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -47,6 +47,7 @@ typedef struct {
>    PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
> +  UINTN                           WaitForCmpStallInUsecs;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>  } PVSCSI_DEV;
> diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> index fcffc90d46c8..ae6d8753623f 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> @@ -38,5 +38,6 @@
>    gEfiPciIoProtocolGuid             ## TO_START
>
>  [Pcd]
> -  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit       ## CONSUMES
> -  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit    ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit               ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit            ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs    ## CONSUMES
>

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Liran Alon]

On 27/03/2020 14:26, Laszlo Ersek wrote:
> On 03/25/20 17:10, Liran Alon wrote:
>> +/**
>> +  Returns if PVSCSI request ring is full
>> +**/
>> +STATIC
>> +BOOLEAN
>> +PvScsiIsReqRingFull (
>> +  IN CONST PVSCSI_DEV   *Dev
>> +  )
>> +{
>> +  PVSCSI_RINGS_STATE *RingsState;
>> +  UINT32             ReqNumEntries;
>> +
>> +  RingsState = Dev->RingDesc.RingState;
>> +  ReqNumEntries = 1U << RingsState->ReqNumEntriesLog2;
>> +  return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >= ReqNumEntries;
>> +}
> (Just some thoughts, not a request for changing the code.)
>
> Normally I prefer accessing buffers shared with the device though
> volatile-qualified  pointers.
>
> Meaning, in this case, that every "PCI host" pointer (i.e., each pointer
> that is associated with a PVSCSI_DMA_DESC) would have to be
> volatile-qualified. In particular:
>
> - in patch#13, PVSCSI_RING_DESC would have to be updated like this:
>
>> typedef struct {
>>    volatile PVSCSI_RINGS_STATE   *RingState;
>>    PVSCSI_DMA_DESC               RingStateDmaDesc;
>>
>>    volatile PVSCSI_RING_REQ_DESC *RingReqs;
>>    PVSCSI_DMA_DESC               RingReqsDmaDesc;
>>
>>    volatile PVSCSI_RING_CMP_DESC *RingCmps;
>>    PVSCSI_DMA_DESC               RingCmpsDmaDesc;
>> } PVSCSI_RING_DESC;
> - in patch#14, PVSCSI_DEV would change as follows:
>
>> typedef struct {
>>    UINT32                          Signature;
>>    EFI_PCI_IO_PROTOCOL             *PciIo;
>>    EFI_EVENT                       ExitBoot;
>>    UINT64                          OriginalPciAttributes;
>>    PVSCSI_RING_DESC                RingDesc;
>>    volatile PVSCSI_DMA_BUFFER      *DmaBuf;
>>    PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>>    UINT8                           MaxTarget;
>>    UINT8                           MaxLun;
>>    UINTN                           WaitForCmpStallInUsecs;
>>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>> } PVSCSI_DEV;
> After these changes, the compiler would (justifiedly) flag a bunch of
> code locations casting away the volatile qualification -- for example,
> in the above function, in the assignment to the "RingsState" local
> variable.
>
> Clearly, most of these compilation errors would have to be fixed (not
> suppressed), because they would be valid. Meaning:
>
> - you'd have to volatile-qualify the "RingsState" local variable in all
>    of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(),
>    PvScsiWaitForRequestCompletion();
>
> - you'd also have to volatile-qualify the return types of
>    PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion();
>
> - you'd have to update PopulateRequest() and HandleResponse() too; and
>    the most annoying part of that would be that you could no longer use
>    CopyMem() and ZeroMem() -- because those functions take
>    pointer-to-void parameters, rather than pointer-to-volatile-void ones.
>
> (FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype
> -- it would be OK to cast away volatile in those calls, as we wouldn't
> dereference the pointers in that case.)
>
> So... the reason I'm not actually requesting these
> volatile-qualifications is that (a) your use of MemoryFence() seems
> mostly OK, and (b) the UEFI Driver Writer's guide recommends *either*
> volatile *or* MemoryFence(). Of course using both techniques at the same
> time is not a problem -- and in code I write I actually like to use both
> at the same time --, but just one suffices too. (See section 4.2.6
> "Memory ordering" in the DWG.)
>
> The reason I'm writing this up here is because I want the "record" (the
> mailing list archive) to show that we have considered this topic
> explicitly.
I prefer to remain with only memory fences if that's OK by you. As the 
code is written now.
As it's allows for potential compiler optimization and leads to more 
readable code in my opinion.
> Back to your patch:
>
> On 03/25/20 17:10, Liran Alon wrote:
>> +  //
>> +  // This cast is safe as MaxLun is defined as UINT8
>> +  //
>> +  Request->Lun[1] = (UINT8)Lun;
>> +  Request->SenseLen = Packet->SenseDataLength;
> Ah, *now* I understand why you chose MAX_UINT8 as the size of
> "PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has
> type UINT8, and this way you guarantee that the SCSI client's
> "Packet->SenseDataLength" will always fit in the DMA buffer.
>
> Good solution, but it *absolutely* needs to be documented in patch#14
> ("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact,
> see my question (4) under patch#14.
Please read the response I have written you to your patch#14 review.
Where I suggest we define a constant in IndustryStandard/Scsi.h for the 
limit of the total length of SenseData that is defined to be 252 
according to SCSI specification.
>
> (2) Also, please add a comment here that a "Dev->DmaBuf->SenseData"
> overflow is not possible due to "Packet->SenseDataLength" having type
> UINT8.
>
> This would be a comment in the same vein as the "MaxLun" reference just
> above -- I find *that* comment very helpful, too.
OK.
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> +  Handle the PVSCSI device response:
>> +  - Copy returned data from DMA communication buffer.
>> +  - Update fields in Extended SCSI Pass Thru Protocol packet as required.
>> +  - Translate response code to EFI status code and host adapter status.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +HandleResponse (
>> +  IN PVSCSI_DEV                                     *Dev,
>> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
>> +  IN CONST PVSCSI_RING_CMP_DESC                     *Response
>> +  )
>> +{
>> +  //
>> +  // Check if device returned sense data
>> +  //
>> +  if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
>> +    //
>> +    // Fix SenseDataLength to amount of data returned
>> +    //
>> +    if (Packet->SenseDataLength > Response->SenseLen) {
>> +      Packet->SenseDataLength = (UINT8)Response->SenseLen;
>> +    }
>> +    //
>> +    // Copy sense data from DMA communication buffer
>> +    //
>> +    CopyMem (
>> +      Packet->SenseData,
>> +      Dev->DmaBuf->SenseData,
>> +      Packet->SenseDataLength
>> +      );
>> +  } else {
>> +    //
>> +    // Signal no sense data returned
>> +    //
>> +    Packet->SenseDataLength = 0;
>> +  }
>> +
>> +  //
>> +  // Copy device output from DMA communication buffer
>> +  //
>> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>> +    CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
>> +  }
> I'm unfamilar with the PVSCSI device model, but I think this is not
> general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that
> short reads are possible at least in theory.
>
> (5) If a short read occurs (Response->DataLen <
> Packet->InTransferLength), then we should adjust
> "Packet->InTransferLength", and also copy that many bytes only.
>
> (6) I think it would be prudent to update "Packet->OutTransferLength"
> too, for short writes.
As you can see below, this is done in case device return 
Response->HostStatus as either PvScsiBtStatDatarun or 
PvScsiBtStatDataUnderrun.
>
>> +
>> +  //
>> +  // Report target status
>> +  //
>> +  Packet->TargetStatus = Response->ScsiStatus;
>> +
>> +  //
>> +  // Host adapter status and function return value depend on
>> +  // device response's host status
>> +  //
>> +  switch (Response->HostStatus) {
>> +    case PvScsiBtStatSuccess:
>> +    case PvScsiBtStatLinkedCommandCompleted:
>> +    case PvScsiBtStatLinkedCommandCompletedWithFlag:
>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
>> +      return EFI_SUCCESS;
>> +
>> +    case PvScsiBtStatSelTimeout:
>> +      Packet->HostAdapterStatus =
>> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>> +      return EFI_TIMEOUT;
>> +
>> +    case PvScsiBtStatDatarun:
>> +    case :
>> +      //
>> +      // Report residual data in overrun/underrun
>> +      //
>> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>> +        Packet->InTransferLength = Response->DataLen;
>> +      } else {
>> +        Packet->OutTransferLength = Response->DataLen;
>> +      }
> OK, if we are sure that (a) the device will always report short
> reads/writes like this, and that (b) the above assignments will never
> cause InTransferLength / OutTransferLength to *grow*, then the
> InTransferLength / OutTransferLength adjustments are sufficiently
> covered.
I believe both of these are indeed true.
Even though that current QEMU VMware PVSCSI device emulation code have a 
bug that it never sets this in pvscsi_command_complete() when it does 
set BTSTAT_DATARUN...
> Still:
>
> (8) The CopyMem() call above should not copy garbage (at the tail).
I don't think it matters. We don't guarantee anything on the content in 
Packet->InDataBuffer beyond Packet->InTransferLength.
I think the code is simpler how it is currently written.
>
> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
I don't think this is the case.
> then I would prefer if:
>
> - we always updated InTransferLength / OutTransferLength (regardless of
> "Response->HostStatus"),
>
> - and we only used these case labels (PvScsiBtStatDatarun /
> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
>
>> +      Packet->HostAdapterStatus =
>> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>> +      return EFI_BAD_BUFFER_SIZE;
> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
> But that's not the case here -- we do have a partially completed
> transfer.

Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU 
in MdePkg/Include/Protocol/ScsiPassThru.h:

   @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was 
executed, but the
                                     entire DataBuffer could not be 
transferred.
                                     The actual number of bytes 
transferred is returned
                                     in TransferLength. See 
HostAdapterStatus,
                                     TargetStatus, SenseDataLength, and 
SenseData in
                                     that order for additional status 
information.

So I don't know who to believe... It does seem to me that this 
documentation in the code makes more sense
and then my current code is correct. What do you think?

>
> (9) Thus I feel we should use a "break" here.
>
>> +
>> +    case PvScsiBtStatBusFree:
>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE;
>> +      break;
>> +
>> +    case PvScsiBtStatInvPhase:
>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR;
>> +      break;
>> +
>> +    case PvScsiBtStatSensFailed:
>> +      Packet->HostAdapterStatus =
>> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED;
>> +      break;
>> +
>> +    case PvScsiBtStatTagReject:
>> +    case PvScsiBtStatBadMsg:
>> +      Packet->HostAdapterStatus =
>> +          EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT;
>> +      break;
>> +
>> +    case PvScsiBtStatBusReset:
>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
>> +      break;
>> +
>> +    case PvScsiBtStatHaTimeout:
>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
>> +      return EFI_TIMEOUT;
>> +
>> +    case PvScsiBtStatScsiParity:
>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
>> +      break;
>> +
>> +    default:
>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
>> +      break;
>> +  }
>> +
>> +  return EFI_DEVICE_ERROR;
>> +}
>> +
>>
>>   //
>>   // Ext SCSI Pass Thru
>>   //
>> @@ -144,7 +528,62 @@ PvScsiPassThru (
>>     IN EFI_EVENT                                      Event    OPTIONAL
>>     )
>>   {
>> -  return EFI_UNSUPPORTED;
>> +  PVSCSI_DEV            *Dev;
>> +  EFI_STATUS            Status;
>> +  PVSCSI_RING_REQ_DESC *Request;
>> +  PVSCSI_RING_CMP_DESC *Response;
>> +
>> +  Dev = PVSCSI_FROM_PASS_THRU (This);
>> +
>> +  if (PvScsiIsReqRingFull (Dev)) {
>> +    return EFI_NOT_READY;
>> +  }
>> +
>> +  Request = PvScsiGetCurrentRequest (Dev);
>> +
>> +  Status = PopulateRequest (Dev, Target, Lun, Packet, Request);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  //
>> +  // Writes to Request must be globally visible before making request
>> +  // available to device
>> +  //
>> +  MemoryFence ();
>> +  Dev->RingDesc.RingState->ReqProdIdx++;
>> +
> (10) Please insert another MemoryFence () here.

That would be unnecessary and wrong.

The MemoryFence() here is used to make sure the request is globally 
visible before the update to the producer-index. As in any 
circular-buffer implementation.
There is no need for an additional MemoryFence() here.

Note that the MMIO access below is guaranteed to be globally visible 
only after the write to the producer-index.
If EDK2 MMIO accessors wouldn't have guaranteed this, you would have a 
very broken code base...
Similar to why Linux MMIO accessors (e.g. writel()) macros guarantee these.

For example, see how MdePkg/Library/BaseIoLibIntrinsic/IoLib.c 
MmioWrite32() internally calls MemoryFence() before and after MMIO 
access itself.

>
>> +  Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetKickRwIo, 0);
>> +  if (EFI_ERROR (Status)) {
>> +    //
>> +    // If kicking the host fails, we must fake a host adapter error.
>> +    // EFI_NOT_READY would save us the effort, but it would also suggest that
>> +    // the caller retry.
>> +    //
>> +    return ReportHostAdapterError (Packet);
>> +  }
>> +
>> +  Status = PvScsiWaitForRequestCompletion (Dev);
>> +  if (EFI_ERROR (Status)) {
>> +    //
>> +    // If waiting for request completion fails, we must fake a host adapter
>> +    // error. EFI_NOT_READY would save us the effort, but it would also suggest
>> +    // that the caller retry.
>> +    //
>> +    return ReportHostAdapterError (Packet);
>> +  }
>> +
> (11) Please insert a MemoryFence() here.

Why is a MemoryFence() needed here? I don't think that's true.

PvScsiWaitForRequestCompletion() ends with an MMIO write which is 
guaranteed to be a memory fence.
Thus, there is no need for a MemoryFence() here (to serve as a rmb()) to 
make sure the completion-descriptor is globally visible.

Thanks,

-Liran

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Liran Alon]

On 27/03/2020 16:04, Liran Alon wrote:
>
> On 27/03/2020 14:26, Laszlo Ersek wrote:
>> On 03/25/20 17:10, Liran Alon wrote:
>>
>>> +
>>> +  //
>>> +  // Report target status
>>> +  //
>>> +  Packet->TargetStatus = Response->ScsiStatus;
>>> +
>>> +  //
>>> +  // Host adapter status and function return value depend on
>>> +  // device response's host status
>>> +  //
>>> +  switch (Response->HostStatus) {
>>> +    case PvScsiBtStatSuccess:
>>> +    case PvScsiBtStatLinkedCommandCompleted:
>>> +    case PvScsiBtStatLinkedCommandCompletedWithFlag:
>>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
>>> +      return EFI_SUCCESS;
>>> +
>>> +    case PvScsiBtStatSelTimeout:
>>> +      Packet->HostAdapterStatus =
>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>>> +      return EFI_TIMEOUT;
>>> +
>>> +    case PvScsiBtStatDatarun:
>>> +    case :
>>> +      //
>>> +      // Report residual data in overrun/underrun
>>> +      //
>>> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>> +        Packet->InTransferLength = Response->DataLen;
>>> +      } else {
>>> +        Packet->OutTransferLength = Response->DataLen;
>>> +      }
>> OK, if we are sure that (a) the device will always report short
>> reads/writes like this, and that (b) the above assignments will never
>> cause InTransferLength / OutTransferLength to *grow*, then the
>> InTransferLength / OutTransferLength adjustments are sufficiently
>> covered.
> I believe both of these are indeed true.
> Even though that current QEMU VMware PVSCSI device emulation code have 
> a bug that it never sets this in pvscsi_command_complete() when it 
> does set BTSTAT_DATARUN...
>> Still:
>>
>> (8) The CopyMem() call above should not copy garbage (at the tail).
> I don't think it matters. We don't guarantee anything on the content 
> in Packet->InDataBuffer beyond Packet->InTransferLength.
> I think the code is simpler how it is currently written.
>>
>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
> I don't think this is the case.
>> then I would prefer if:
>>
>> - we always updated InTransferLength / OutTransferLength (regardless of
>> "Response->HostStatus"),
>>
>> - and we only used these case labels (PvScsiBtStatDatarun /
>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".

Regarding all the above:
You can also see that Linux PVSCSI driver (drivers/scsi/vmw_pvscsi.c) 
reads the "DataLen" field only in case the "HostStatus" is 
BTSTAT_DATARUN or BTSTAT_DATA_UNDERRUN.
As I have done in my driver. In the lack of more detailed device 
specification (As PVSCSI is a proprietary VMware PV device), I prefer to 
remain with my implementation which seems
to be guaranteed to be safe and working. Please tell me if you think 
otherwise.

-Liran

Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

[Laszlo Ersek]

On 03/27/20 01:05, Liran Alon wrote:
>
> On 27/03/2020 0:17, Laszlo Ersek wrote:
>> On 03/25/20 17:10, Liran Alon wrote:
>>>     PvScsiRestorePciAttributes (Dev);
>>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>>> index 6d23b6e1eccf..7f91d70fec79 100644
>>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>>> @@ -31,6 +31,11 @@ typedef struct {
>>>     PVSCSI_DMA_DESC      RingCmpsDmaDesc;
>>>   } PVSCSI_RING_DESC;
>>>   +typedef struct {
>>> +  UINT8     SenseData[MAX_UINT8];
>> (4) Is the maximum possible size of the sense data specified
>> somewhere? If so, it would be nice to document it with a comment at
>> least.
> This is a good point. Thanks for pointing it out.
> Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1
> Descriptor format sense data overview specifies:
> "The additional sense length shall be less than or equal to 244 (i.e.,
> limiting the total length of the sense data to 252 bytes)"
>
> i.e. The max possible size of sense data is 252.
> As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed
> defining:
>
> #define SCSI_SENSE_BUF_SIZE 252
>
> This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
> sense buf size to 252") which says in it's commit message:
> "According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
>
> Interestingly, this QEMU commit changed the value of
> SCSI_SENSE_BUF_SIZE from 96 to 252.
> But then I found this in EDK2
> OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
>
> //
> // We expect these maximum sizes from the host. Also we force the
> CdbLength and
> // SenseDataLength parameters of
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
> // to exceed these limits. See UEFI 2.3.1 errata C 14.7.
> //
> #define VIRTIO_SCSI_CDB_SIZE   32
> #define VIRTIO_SCSI_SENSE_SIZE 96
>
> Which seems to be wrong...

No, these macros / limits are valid. They match the virtio specification
(both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size"
configuration values that the virtio-scsi device is required to expose
by default.

While the driver could theoretically ask for larger sizes, these values
have never caused a problem in practice. I don't see a reason to change
the constants (let alone to complicate the code).

>
> It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to
> EDK2 MdePkg/Include/IndustryStandard/Scsi.h.
> And then use that from my PvScsi driver.
>
> I can also submit a separate patch (Not part of this series) to remove
> this VIRTIO_SCSI_SENSE_SIZE constant.

Please don't; there's no reason to change the VirtioScsiDxe driver.

> Note that VirtioScsi doesn't have a technical issue here because it
> provides this max sense length to the device in VirtioScsiInit():
>
> Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);

Right, and even those config writes are mostly documentation / "just to
be sure" oriented. Because, there's also a comment in those parts:

  //
  // We expect these maximum sizes from the host. Since they are
  // guest-negotiable, ask for them rather than just checking them.
  //

>
> So if it's OK by you, I think I will just add a patch to this series
> defining SCSI_MAX_SENSE_SIZE in
> MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when
> defining SenseData in PVSCSI_DMA_BUFFER.

My experience tells me that making any OvmfPkg change dependent on new
patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...)
slows down the upstreaming of the OvmfPkg change significantly.

Therefore, I would strongly advise against this ordering of changes.

Today, upon reviewing patch#15, I commented again on the "SenseData"
array size (see under point (2) in my patch#15 feedback). Accordingly,
for now, please stick with the existent "SenseData" array declaration,
just add a comment. I expect / hope to merge your v3 posting.

(Side comment: I would also like to ask Nikita to R-b the full final
patch set on the list, once I'm ready to merge the series, because I'd
like to testify to Nikita's review effort in the upstream git history.)

Then, with the series merged, you can propose a separate patch series (2
patches), later -- introducing the new constant in MdePkg, plus putting
the new constant to use in the (then-upstream) PvScsi feature. No matter
how long that is delayed, the main feature will have been merged.

>
>>> +  UINT8     Data[0x2000];
>> (5) Same here. From peeking at the next patch, we seem to be choosing
>> this size arbitrarily.
> This is indeed arbitrary...
>>
>> If it works for you in all relevant boot scenarios, I'm OK with it,
>> but we should be clear that this value is arbitrarily chosen. No need
>> for a #define, but a comment would be nice.
>
> OK. Will just add a comment such as:
>
> //
> // This size is arbitrarily chosen,
> // It seems to be sufficient for all I/O requests sent through
> EFI_SCSI_PASS_THRU_PROTOCOL.PassThru().
> //
>
> If you wish to have something else, please say so. :)

This comment looks nice, I'd only append: "for common boot scenarios".

>
>>
>> (6) Should we declare this structure as packed?
> There is no such requirement as it's not a wire-format or anything
> like that.

My thinking was that we shouldn't have any "padding" in a structure
that's exposed to a device.

But, admittedly, due to rounding up the allocation to page size anyway,
we're virtually guaranteed to have unused space at the end. Possibly
having some padding in the middle makes no difference, so you are right:
no need for packing this.

> The only rational of defining it as packed is to avoid mapping
> unnecessary pages to device.
> But this structure is less than a page-size anyway. So I think it's
> fine.

I agree with your general point, but I'd like to comment on the size
independently (just for the record): the PVSCSI_DMA_BUFFER structure is
not smaller than a page. It is slightly larger than two pages -- it
contains at least (255 + 2 * 4096) bytes. Again, that doesn't change the
main point.

>
> If you still think it should be marked as packed, I can change this.

No, I agree it need not be packed.

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Laszlo Ersek]

On 03/27/20 14:04, Liran Alon wrote:
> 
> On 27/03/2020 14:26, Laszlo Ersek wrote:
>> On 03/25/20 17:10, Liran Alon wrote:
>>> +/**
>>> +  Returns if PVSCSI request ring is full
>>> +**/
>>> +STATIC
>>> +BOOLEAN
>>> +PvScsiIsReqRingFull (
>>> +  IN CONST PVSCSI_DEV   *Dev
>>> +  )
>>> +{
>>> +  PVSCSI_RINGS_STATE *RingsState;
>>> +  UINT32             ReqNumEntries;
>>> +
>>> +  RingsState = Dev->RingDesc.RingState;
>>> +  ReqNumEntries = 1U << RingsState->ReqNumEntriesLog2;
>>> +  return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >=
>>> ReqNumEntries;
>>> +}
>> (Just some thoughts, not a request for changing the code.)
>>
>> Normally I prefer accessing buffers shared with the device though
>> volatile-qualified  pointers.
>>
>> Meaning, in this case, that every "PCI host" pointer (i.e., each pointer
>> that is associated with a PVSCSI_DMA_DESC) would have to be
>> volatile-qualified. In particular:
>>
>> - in patch#13, PVSCSI_RING_DESC would have to be updated like this:
>>
>>> typedef struct {
>>>    volatile PVSCSI_RINGS_STATE   *RingState;
>>>    PVSCSI_DMA_DESC               RingStateDmaDesc;
>>>
>>>    volatile PVSCSI_RING_REQ_DESC *RingReqs;
>>>    PVSCSI_DMA_DESC               RingReqsDmaDesc;
>>>
>>>    volatile PVSCSI_RING_CMP_DESC *RingCmps;
>>>    PVSCSI_DMA_DESC               RingCmpsDmaDesc;
>>> } PVSCSI_RING_DESC;
>> - in patch#14, PVSCSI_DEV would change as follows:
>>
>>> typedef struct {
>>>    UINT32                          Signature;
>>>    EFI_PCI_IO_PROTOCOL             *PciIo;
>>>    EFI_EVENT                       ExitBoot;
>>>    UINT64                          OriginalPciAttributes;
>>>    PVSCSI_RING_DESC                RingDesc;
>>>    volatile PVSCSI_DMA_BUFFER      *DmaBuf;
>>>    PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>>>    UINT8                           MaxTarget;
>>>    UINT8                           MaxLun;
>>>    UINTN                           WaitForCmpStallInUsecs;
>>>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>>>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>>> } PVSCSI_DEV;
>> After these changes, the compiler would (justifiedly) flag a bunch of
>> code locations casting away the volatile qualification -- for example,
>> in the above function, in the assignment to the "RingsState" local
>> variable.
>>
>> Clearly, most of these compilation errors would have to be fixed (not
>> suppressed), because they would be valid. Meaning:
>>
>> - you'd have to volatile-qualify the "RingsState" local variable in all
>>    of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(),
>>    PvScsiWaitForRequestCompletion();
>>
>> - you'd also have to volatile-qualify the return types of
>>    PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion();
>>
>> - you'd have to update PopulateRequest() and HandleResponse() too; and
>>    the most annoying part of that would be that you could no longer use
>>    CopyMem() and ZeroMem() -- because those functions take
>>    pointer-to-void parameters, rather than pointer-to-volatile-void ones.
>>
>> (FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype
>> -- it would be OK to cast away volatile in those calls, as we wouldn't
>> dereference the pointers in that case.)
>>
>> So... the reason I'm not actually requesting these
>> volatile-qualifications is that (a) your use of MemoryFence() seems
>> mostly OK, and (b) the UEFI Driver Writer's guide recommends *either*
>> volatile *or* MemoryFence(). Of course using both techniques at the same
>> time is not a problem -- and in code I write I actually like to use both
>> at the same time --, but just one suffices too. (See section 4.2.6
>> "Memory ordering" in the DWG.)
>>
>> The reason I'm writing this up here is because I want the "record" (the
>> mailing list archive) to show that we have considered this topic
>> explicitly.

> I prefer to remain with only memory fences if that's OK by you.

Yes, that's fine.

> As the code is written now.
> As it's allows for potential compiler optimization and leads to more
> readable code in my opinion.

The UEFI Driver Writer's Guide makes the same argument -- it favors
explicit MemoryFence()s over volatile. So your suggestion is entirely
valid and I agree with it.

>> Back to your patch:
>>
>> On 03/25/20 17:10, Liran Alon wrote:
>>> +  //
>>> +  // This cast is safe as MaxLun is defined as UINT8
>>> +  //
>>> +  Request->Lun[1] = (UINT8)Lun;
>>> +  Request->SenseLen = Packet->SenseDataLength;
>> Ah, *now* I understand why you chose MAX_UINT8 as the size of
>> "PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has
>> type UINT8, and this way you guarantee that the SCSI client's
>> "Packet->SenseDataLength" will always fit in the DMA buffer.
>>
>> Good solution, but it *absolutely* needs to be documented in patch#14
>> ("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact,
>> see my question (4) under patch#14.

> Please read the response I have written you to your patch#14 review.
> Where I suggest we define a constant in IndustryStandard/Scsi.h for the
> limit of the total length of SenseData that is defined to be 252
> according to SCSI specification.

MdePkg macro is good, but it should be decoupled from this series.

>>
>> (2) Also, please add a comment here that a "Dev->DmaBuf->SenseData"
>> overflow is not possible due to "Packet->SenseDataLength" having type
>> UINT8.
>>
>> This would be a comment in the same vein as the "MaxLun" reference just
>> above -- I find *that* comment very helpful, too.
> OK.
>>> +
>>> +  return EFI_SUCCESS;
>>> +}
>>> +
>>> +/**
>>> +  Handle the PVSCSI device response:
>>> +  - Copy returned data from DMA communication buffer.
>>> +  - Update fields in Extended SCSI Pass Thru Protocol packet as
>>> required.
>>> +  - Translate response code to EFI status code and host adapter status.
>>> +**/
>>> +STATIC
>>> +EFI_STATUS
>>> +HandleResponse (
>>> +  IN PVSCSI_DEV                                     *Dev,
>>> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
>>> +  IN CONST PVSCSI_RING_CMP_DESC                     *Response
>>> +  )
>>> +{
>>> +  //
>>> +  // Check if device returned sense data
>>> +  //
>>> +  if (Response->ScsiStatus ==
>>> EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
>>> +    //
>>> +    // Fix SenseDataLength to amount of data returned
>>> +    //
>>> +    if (Packet->SenseDataLength > Response->SenseLen) {
>>> +      Packet->SenseDataLength = (UINT8)Response->SenseLen;
>>> +    }
>>> +    //
>>> +    // Copy sense data from DMA communication buffer
>>> +    //
>>> +    CopyMem (
>>> +      Packet->SenseData,
>>> +      Dev->DmaBuf->SenseData,
>>> +      Packet->SenseDataLength
>>> +      );
>>> +  } else {
>>> +    //
>>> +    // Signal no sense data returned
>>> +    //
>>> +    Packet->SenseDataLength = 0;
>>> +  }
>>> +
>>> +  //
>>> +  // Copy device output from DMA communication buffer
>>> +  //
>>> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>> +    CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data,
>>> Packet->InTransferLength);
>>> +  }
>> I'm unfamilar with the PVSCSI device model, but I think this is not
>> general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that
>> short reads are possible at least in theory.
>>
>> (5) If a short read occurs (Response->DataLen <
>> Packet->InTransferLength), then we should adjust
>> "Packet->InTransferLength", and also copy that many bytes only.
>>
>> (6) I think it would be prudent to update "Packet->OutTransferLength"
>> too, for short writes.
> As you can see below, this is done in case device return
> Response->HostStatus as either PvScsiBtStatDatarun or
> PvScsiBtStatDataUnderrun.
>>
>>> +
>>> +  //
>>> +  // Report target status
>>> +  //
>>> +  Packet->TargetStatus = Response->ScsiStatus;
>>> +
>>> +  //
>>> +  // Host adapter status and function return value depend on
>>> +  // device response's host status
>>> +  //
>>> +  switch (Response->HostStatus) {
>>> +    case PvScsiBtStatSuccess:
>>> +    case PvScsiBtStatLinkedCommandCompleted:
>>> +    case PvScsiBtStatLinkedCommandCompletedWithFlag:
>>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
>>> +      return EFI_SUCCESS;
>>> +
>>> +    case PvScsiBtStatSelTimeout:
>>> +      Packet->HostAdapterStatus =
>>> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>>> +      return EFI_TIMEOUT;
>>> +
>>> +    case PvScsiBtStatDatarun:
>>> +    case :
>>> +      //
>>> +      // Report residual data in overrun/underrun
>>> +      //
>>> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>> +        Packet->InTransferLength = Response->DataLen;
>>> +      } else {
>>> +        Packet->OutTransferLength = Response->DataLen;
>>> +      }
>> OK, if we are sure that (a) the device will always report short
>> reads/writes like this, and that (b) the above assignments will never
>> cause InTransferLength / OutTransferLength to *grow*, then the
>> InTransferLength / OutTransferLength adjustments are sufficiently
>> covered.
> I believe both of these are indeed true.
> Even though that current QEMU VMware PVSCSI device emulation code have a
> bug that it never sets this in pvscsi_command_complete() when it does
> set BTSTAT_DATARUN...
>> Still:
>>
>> (8) The CopyMem() call above should not copy garbage (at the tail).
> I don't think it matters. We don't guarantee anything on the content in
> Packet->InDataBuffer beyond Packet->InTransferLength.
> I think the code is simpler how it is currently written.

I'm not convinced, but this is not a question I feel very strongly
about. I OK to go with your preference.

>>
>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
> I don't think this is the case.
>> then I would prefer if:
>>
>> - we always updated InTransferLength / OutTransferLength (regardless of
>> "Response->HostStatus"),
>>
>> - and we only used these case labels (PvScsiBtStatDatarun /
>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
>>
>>> +      Packet->HostAdapterStatus =
>>> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>> +      return EFI_BAD_BUFFER_SIZE;
>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>> But that's not the case here -- we do have a partially completed
>> transfer.
> 
> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
> in MdePkg/Include/Protocol/ScsiPassThru.h:
> 
>   @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was
> executed, but the
>                                     entire DataBuffer could not be
> transferred.
>                                     The actual number of bytes
> transferred is returned
>                                     in TransferLength. See
> HostAdapterStatus,
>                                     TargetStatus, SenseDataLength, and
> SenseData in
>                                     that order for additional status
> information.
> 
> So I don't know who to believe... It does seem to me that this
> documentation in the code makes more sense
> and then my current code is correct. What do you think?

You are looking at the wrong protocol header file. The top of this
header file bears the comment

  SCSI Pass Through protocol as defined in EFI 1.1.

and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
only refers to Mantis ticket 845
<https://mantis.uefi.org/mantis/view.php?id=845> with subject
"EFI_SCSI_PASS_THRU_PROTOCOL replacement".

Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
"MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
SCSI Request Packet was not executed").

> 
>>
>> (9) Thus I feel we should use a "break" here.
>>
>>> +
>>> +    case PvScsiBtStatBusFree:
>>> +      Packet->HostAdapterStatus =
>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE;
>>> +      break;
>>> +
>>> +    case PvScsiBtStatInvPhase:
>>> +      Packet->HostAdapterStatus =
>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR;
>>> +      break;
>>> +
>>> +    case PvScsiBtStatSensFailed:
>>> +      Packet->HostAdapterStatus =
>>> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED;
>>> +      break;
>>> +
>>> +    case PvScsiBtStatTagReject:
>>> +    case PvScsiBtStatBadMsg:
>>> +      Packet->HostAdapterStatus =
>>> +          EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT;
>>> +      break;
>>> +
>>> +    case PvScsiBtStatBusReset:
>>> +      Packet->HostAdapterStatus =
>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
>>> +      break;
>>> +
>>> +    case PvScsiBtStatHaTimeout:
>>> +      Packet->HostAdapterStatus =
>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
>>> +      return EFI_TIMEOUT;
>>> +
>>> +    case PvScsiBtStatScsiParity:
>>> +      Packet->HostAdapterStatus =
>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
>>> +      break;
>>> +
>>> +    default:
>>> +      Packet->HostAdapterStatus =
>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
>>> +      break;
>>> +  }
>>> +
>>> +  return EFI_DEVICE_ERROR;
>>> +}
>>> +
>>>
>>>   //
>>>   // Ext SCSI Pass Thru
>>>   //
>>> @@ -144,7 +528,62 @@ PvScsiPassThru (
>>>     IN EFI_EVENT                                      Event    OPTIONAL
>>>     )
>>>   {
>>> -  return EFI_UNSUPPORTED;
>>> +  PVSCSI_DEV            *Dev;
>>> +  EFI_STATUS            Status;
>>> +  PVSCSI_RING_REQ_DESC *Request;
>>> +  PVSCSI_RING_CMP_DESC *Response;
>>> +
>>> +  Dev = PVSCSI_FROM_PASS_THRU (This);
>>> +
>>> +  if (PvScsiIsReqRingFull (Dev)) {
>>> +    return EFI_NOT_READY;
>>> +  }
>>> +
>>> +  Request = PvScsiGetCurrentRequest (Dev);
>>> +
>>> +  Status = PopulateRequest (Dev, Target, Lun, Packet, Request);
>>> +  if (EFI_ERROR (Status)) {
>>> +    return Status;
>>> +  }
>>> +
>>> +  //
>>> +  // Writes to Request must be globally visible before making request
>>> +  // available to device
>>> +  //
>>> +  MemoryFence ();
>>> +  Dev->RingDesc.RingState->ReqProdIdx++;
>>> +
>> (10) Please insert another MemoryFence () here.
> 
> That would be unnecessary and wrong.
> 
> The MemoryFence() here is used to make sure the request is globally
> visible before the update to the producer-index.

I agree.

> As in any
> circular-buffer implementation.
> There is no need for an additional MemoryFence() here.
> 
> Note that the MMIO access below is guaranteed to be globally visible
> only after the write to the producer-index.

Yes, that was the goal of my suggestion. What guarantees it?

> If EDK2 MMIO accessors wouldn't have guaranteed this, you would have a
> very broken code base...
> Similar to why Linux MMIO accessors (e.g. writel()) macros guarantee these.
> 
> For example, see how MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> MmioWrite32() internally calls MemoryFence() before and after MMIO
> access itself.

So basically you are saying that I proposed the right thing, except
there is no need to spell it out here, because the MMIO accessor
primitives already cover that internally :)

I admit that I have not been aware of the internal fences!

(And given that there is a specific commit in the git history to push
the fences into the source file you mention, namely 9de780dcd6208, I do
think my suggestion was not "wrong", only unnecessary.)

I do agree that the MemoryFence() need not be added in this spot. Thanks
for making me aware of the internal fences!

> 
>>
>>> +  Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetKickRwIo, 0);
>>> +  if (EFI_ERROR (Status)) {
>>> +    //
>>> +    // If kicking the host fails, we must fake a host adapter error.
>>> +    // EFI_NOT_READY would save us the effort, but it would also
>>> suggest that
>>> +    // the caller retry.
>>> +    //
>>> +    return ReportHostAdapterError (Packet);
>>> +  }
>>> +
>>> +  Status = PvScsiWaitForRequestCompletion (Dev);
>>> +  if (EFI_ERROR (Status)) {
>>> +    //
>>> +    // If waiting for request completion fails, we must fake a host
>>> adapter
>>> +    // error. EFI_NOT_READY would save us the effort, but it would
>>> also suggest
>>> +    // that the caller retry.
>>> +    //
>>> +    return ReportHostAdapterError (Packet);
>>> +  }
>>> +
>> (11) Please insert a MemoryFence() here.
> 
> Why is a MemoryFence() needed here? I don't think that's true.
> 
> PvScsiWaitForRequestCompletion() ends with an MMIO write which is
> guaranteed to be a memory fence.

Yes, I see that now. My point was that a fence needed to *occur* here. I
didn't realize it was already covered, internally.

> Thus, there is no need for a MemoryFence() here (to serve as a rmb()) to
> make sure the completion-descriptor is globally visible.

Agreed.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Laszlo Ersek]

On 03/27/20 14:20, Liran Alon wrote:
> 
> On 27/03/2020 16:04, Liran Alon wrote:
>>
>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>> On 03/25/20 17:10, Liran Alon wrote:
>>>
>>>> +
>>>> +  //
>>>> +  // Report target status
>>>> +  //
>>>> +  Packet->TargetStatus = Response->ScsiStatus;
>>>> +
>>>> +  //
>>>> +  // Host adapter status and function return value depend on
>>>> +  // device response's host status
>>>> +  //
>>>> +  switch (Response->HostStatus) {
>>>> +    case PvScsiBtStatSuccess:
>>>> +    case PvScsiBtStatLinkedCommandCompleted:
>>>> +    case PvScsiBtStatLinkedCommandCompletedWithFlag:
>>>> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
>>>> +      return EFI_SUCCESS;
>>>> +
>>>> +    case PvScsiBtStatSelTimeout:
>>>> +      Packet->HostAdapterStatus =
>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
>>>> +      return EFI_TIMEOUT;
>>>> +
>>>> +    case PvScsiBtStatDatarun:
>>>> +    case :
>>>> +      //
>>>> +      // Report residual data in overrun/underrun
>>>> +      //
>>>> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
>>>> +        Packet->InTransferLength = Response->DataLen;
>>>> +      } else {
>>>> +        Packet->OutTransferLength = Response->DataLen;
>>>> +      }
>>> OK, if we are sure that (a) the device will always report short
>>> reads/writes like this, and that (b) the above assignments will never
>>> cause InTransferLength / OutTransferLength to *grow*, then the
>>> InTransferLength / OutTransferLength adjustments are sufficiently
>>> covered.
>> I believe both of these are indeed true.
>> Even though that current QEMU VMware PVSCSI device emulation code have
>> a bug that it never sets this in pvscsi_command_complete() when it
>> does set BTSTAT_DATARUN...
>>> Still:
>>>
>>> (8) The CopyMem() call above should not copy garbage (at the tail).
>> I don't think it matters. We don't guarantee anything on the content
>> in Packet->InDataBuffer beyond Packet->InTransferLength.
>> I think the code is simpler how it is currently written.
>>>
>>> Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
>> I don't think this is the case.
>>> then I would prefer if:
>>>
>>> - we always updated InTransferLength / OutTransferLength (regardless of
>>> "Response->HostStatus"),
>>>
>>> - and we only used these case labels (PvScsiBtStatDatarun /
>>> PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".
> 
> Regarding all the above:
> You can also see that Linux PVSCSI driver (drivers/scsi/vmw_pvscsi.c)
> reads the "DataLen" field only in case the "HostStatus" is
> BTSTAT_DATARUN or BTSTAT_DATA_UNDERRUN.
> As I have done in my driver. In the lack of more detailed device
> specification (As PVSCSI is a proprietary VMware PV device), I prefer to
> remain with my implementation which seems
> to be guaranteed to be safe and working. Please tell me if you think
> otherwise.

No, I'm fine with your reasoning.

Thanks,
Laszlo

Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

[Liran Alon]

On 27/03/2020 16:35, Laszlo Ersek wrote:
> On 03/27/20 01:05, Liran Alon wrote:
>> On 27/03/2020 0:17, Laszlo Ersek wrote:
>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>      PvScsiRestorePciAttributes (Dev);
>>>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>> index 6d23b6e1eccf..7f91d70fec79 100644
>>>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>>>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>> @@ -31,6 +31,11 @@ typedef struct {
>>>>      PVSCSI_DMA_DESC      RingCmpsDmaDesc;
>>>>    } PVSCSI_RING_DESC;
>>>>    +typedef struct {
>>>> +  UINT8     SenseData[MAX_UINT8];
>>> (4) Is the maximum possible size of the sense data specified
>>> somewhere? If so, it would be nice to document it with a comment at
>>> least.
>> This is a good point. Thanks for pointing it out.
>> Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1
>> Descriptor format sense data overview specifies:
>> "The additional sense length shall be less than or equal to 244 (i.e.,
>> limiting the total length of the sense data to 252 bytes)"
>>
>> i.e. The max possible size of sense data is 252.
>> As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed
>> defining:
>>
>> #define SCSI_SENSE_BUF_SIZE 252
>>
>> This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
>> sense buf size to 252") which says in it's commit message:
>> "According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
>>
>> Interestingly, this QEMU commit changed the value of
>> SCSI_SENSE_BUF_SIZE from 96 to 252.
>> But then I found this in EDK2
>> OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
>>
>> //
>> // We expect these maximum sizes from the host. Also we force the
>> CdbLength and
>> // SenseDataLength parameters of
>> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
>> // to exceed these limits. See UEFI 2.3.1 errata C 14.7.
>> //
>> #define VIRTIO_SCSI_CDB_SIZE   32
>> #define VIRTIO_SCSI_SENSE_SIZE 96
>>
>> Which seems to be wrong...
> No, these macros / limits are valid. They match the virtio specification
> (both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size"
> configuration values that the virtio-scsi device is required to expose
> by default.
>
> While the driver could theoretically ask for larger sizes, these values
> have never caused a problem in practice. I don't see a reason to change
> the constants (let alone to complicate the code).
>
>> It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to
>> EDK2 MdePkg/Include/IndustryStandard/Scsi.h.
>> And then use that from my PvScsi driver.
>>
>> I can also submit a separate patch (Not part of this series) to remove
>> this VIRTIO_SCSI_SENSE_SIZE constant.
> Please don't; there's no reason to change the VirtioScsiDxe driver.
>
>> Note that VirtioScsi doesn't have a technical issue here because it
>> provides this max sense length to the device in VirtioScsiInit():
>>
>> Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
> Right, and even those config writes are mostly documentation / "just to
> be sure" oriented. Because, there's also a comment in those parts:
>
>    //
>    // We expect these maximum sizes from the host. Since they are
>    // guest-negotiable, ask for them rather than just checking them.
>    //
>
>> So if it's OK by you, I think I will just add a patch to this series
>> defining SCSI_MAX_SENSE_SIZE in
>> MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when
>> defining SenseData in PVSCSI_DMA_BUFFER.
> My experience tells me that making any OvmfPkg change dependent on new
> patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...)
> slows down the upstreaming of the OvmfPkg change significantly.
>
> Therefore, I would strongly advise against this ordering of changes.
>
> Today, upon reviewing patch#15, I commented again on the "SenseData"
> array size (see under point (2) in my patch#15 feedback). Accordingly,
> for now, please stick with the existent "SenseData" array declaration,
> just add a comment. I expect / hope to merge your v3 posting.
As an alternative, I can also define SenseData to be of size 252 (which 
I would #define as a constant in PvScsi.h),
and explain that it is the limit of total length of SenseData according 
to SCSI specification.

And indeed leave for a separate patch-series to move this PvScsi.h 
private constant and put it in the generic
IndustryStandard/Scsi.h header file.

I think it's nicer than just defining SenseData to be MAX_UINT8 because 
of the field size in PassThru packet.
But I will of course do as you prefer. Just let me know your choice :)

>
> (Side comment: I would also like to ask Nikita to R-b the full final
> patch set on the list, once I'm ready to merge the series, because I'd
> like to testify to Nikita's review effort in the upstream git history.)

Ok. I will let him know to review my v3 submission upstream.

-Liran

>
> Then, with the series merged, you can propose a separate patch series (2
> patches), later -- introducing the new constant in MdePkg, plus putting
> the new constant to use in the (then-upstream) PvScsi feature. No matter
> how long that is delayed, the main feature will have been merged.
>

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Liran Alon]

On 28/03/2020 0:05, Laszlo Ersek wrote:
> On 03/27/20 14:04, Liran Alon wrote:
>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>> On 03/25/20 17:10, Liran Alon wrote:
>>>> +      Packet->HostAdapterStatus =
>>>> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>> +      return EFI_BAD_BUFFER_SIZE;
>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>> But that's not the case here -- we do have a partially completed
>>> transfer.
>> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>
>>    @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was
>> executed, but the
>>                                      entire DataBuffer could not be
>> transferred.
>>                                      The actual number of bytes
>> transferred is returned
>>                                      in TransferLength. See
>> HostAdapterStatus,
>>                                      TargetStatus, SenseDataLength, and
>> SenseData in
>>                                      that order for additional status
>> information.
>>
>> So I don't know who to believe... It does seem to me that this
>> documentation in the code makes more sense
>> and then my current code is correct. What do you think?
> You are looking at the wrong protocol header file.
Oops. You are right.
> The top of this
> header file bears the comment
>
>    SCSI Pass Through protocol as defined in EFI 1.1.
>
> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
> only refers to Mantis ticket 845
> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$ > with subject
> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>
> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
> SCSI Request Packet was not executed").

Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and 
ScsiPassThruExt.h header file. It doesn't make sense.
In the new header file, there is no way for PassThru() to indicate to 
caller that a partial data-transfer have occurred.
Which seems to be a quite standard functionality. Almost all SCSI 
devices are able to return information on partial data-transfer completions.

Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete() 
implementation, we can see how it handles partial data-transfer completions.
It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual 
accordingly.
Looking at EDK2 VirtioScsi driver, function ParseResponse(), this will 
be handled by updating Packet->InTransferLength and 
Packet->OutTransferLength based on Response->Residual,
but then setting Packet->HostAdapterStatus to 
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
(In contrast to your suggestion for me to return EFI_DEVICE_ERROR in 
this case).

The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS 
specifies:

   @retval EFI_SUCCESS           The SCSI Request Packet was sent by the 
host. For bi-directional
                                 commands, InTransferLength bytes were 
transferred from
                                 InDataBuffer. For write and 
bi-directional commands,
                                 OutTransferLength bytes were transferred by
                                 OutDataBuffer.

But it seems to reference the InTransferLength and OutTransferLength 
*before* the call to PassThru().

In contrast to old header file, ScsiPassThru.h, which for 
EFI_BAD_BUFFER_SIZE specifies explicitly:

  @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was 
executed, but the
                                     entire DataBuffer could not be 
transferred.
                                     The actual number of bytes 
transferred is returned
                                     in TransferLength. See 
HostAdapterStatus,
                                     TargetStatus, SenseDataLength, and 
SenseData in
                                     that order for additional status 
information.

In addition, looking at EDK2 iSCSI ScsiPassThru driver 
(NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use 
EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
(Which matches the new header file ScsiPassThruExt.h and not the old 
ScsiPassThru.h).
If you would have a look at how IScsiOnScsiRspRcvd() handles 
SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW bits, 
it seems to always
set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating 
Packet->InTransferLength / Packet->OutTransferLength.

What do you think? Do you have any further insights? This is quite 
confusing to me.

-Liran

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Liran Alon]

On 28/03/2020 1:04, Liran Alon wrote:
>
> On 28/03/2020 0:05, Laszlo Ersek wrote:
>> On 03/27/20 14:04, Liran Alon wrote:
>>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>> +      Packet->HostAdapterStatus =
>>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>>> +      return EFI_BAD_BUFFER_SIZE;
>>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI 
>>>> spec,
>>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>>> But that's not the case here -- we do have a partially completed
>>>> transfer.
>>> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
>>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>>
>>>    @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was
>>> executed, but the
>>>                                      entire DataBuffer could not be
>>> transferred.
>>>                                      The actual number of bytes
>>> transferred is returned
>>>                                      in TransferLength. See
>>> HostAdapterStatus,
>>>                                      TargetStatus, SenseDataLength, and
>>> SenseData in
>>>                                      that order for additional status
>>> information.
>>>
>>> So I don't know who to believe... It does seem to me that this
>>> documentation in the code makes more sense
>>> and then my current code is correct. What do you think?
>> You are looking at the wrong protocol header file.
> Oops. You are right.
>> The top of this
>> header file bears the comment
>>
>>    SCSI Pass Through protocol as defined in EFI 1.1.
>>
>> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
>> only refers to Mantis ticket 845
>> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$ 
>> > with subject
>> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>>
>> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
>> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
>> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
>> SCSI Request Packet was not executed").
>
> Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and 
> ScsiPassThruExt.h header file. It doesn't make sense.
> In the new header file, there is no way for PassThru() to indicate to 
> caller that a partial data-transfer have occurred.
> Which seems to be a quite standard functionality. Almost all SCSI 
> devices are able to return information on partial data-transfer 
> completions.
>
> Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete() 
> implementation, we can see how it handles partial data-transfer 
> completions.
> It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual 
> accordingly.
> Looking at EDK2 VirtioScsi driver, function ParseResponse(), this will 
> be handled by updating Packet->InTransferLength and 
> Packet->OutTransferLength based on Response->Residual,
> but then setting Packet->HostAdapterStatus to 
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
> (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in 
> this case).
>
> The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS 
> specifies:
>
>   @retval EFI_SUCCESS           The SCSI Request Packet was sent by 
> the host. For bi-directional
>                                 commands, InTransferLength bytes were 
> transferred from
>                                 InDataBuffer. For write and 
> bi-directional commands,
>                                 OutTransferLength bytes were 
> transferred by
>                                 OutDataBuffer.
>
> But it seems to reference the InTransferLength and OutTransferLength 
> *before* the call to PassThru().
>
> In contrast to old header file, ScsiPassThru.h, which for 
> EFI_BAD_BUFFER_SIZE specifies explicitly:
>
>  @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was 
> executed, but the
>                                     entire DataBuffer could not be 
> transferred.
>                                     The actual number of bytes 
> transferred is returned
>                                     in TransferLength. See 
> HostAdapterStatus,
>                                     TargetStatus, SenseDataLength, and 
> SenseData in
>                                     that order for additional status 
> information.
>
> In addition, looking at EDK2 iSCSI ScsiPassThru driver 
> (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use 
> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
> (Which matches the new header file ScsiPassThruExt.h and not the old 
> ScsiPassThru.h).
> If you would have a look at how IScsiOnScsiRspRcvd() handles 
> SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW 
> bits, it seems to always
> set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating 
> Packet->InTransferLength / Packet->OutTransferLength.
>
> What do you think? Do you have any further insights? This is quite 
> confusing to me.
>
> -Liran
>
Furthermore, I have later found ScsiExecuteSCSICommand() in 
MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls the 
PassThru() method.
Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is 
relevant to us), one can see it just simply executes the PassThru() 
device and returns.
Examining ScsiExecuteSCSICommand() documentation specifies for 
EFI_BAD_BUFFER_SIZE:

   @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
                               but the entire DataBuffer could not be 
transferred.
                               The actual number of bytes transferred is 
returned
                               in TransferLength. See HostAdapterStatus,
                               TargetStatus, SenseDataLength, and 
SenseData in
                               that order for additional status information.

Which again seems to align to what I have done in my PvScsi driver...

-Liran

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Liran Alon]

On 28/03/2020 1:17, Liran Alon wrote:
>
> On 28/03/2020 1:04, Liran Alon wrote:
>>
>> On 28/03/2020 0:05, Laszlo Ersek wrote:
>>> On 03/27/20 14:04, Liran Alon wrote:
>>>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>>> + Packet->HostAdapterStatus =
>>>>>> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>>>> +      return EFI_BAD_BUFFER_SIZE;
>>>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI 
>>>>> spec,
>>>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>>>> But that's not the case here -- we do have a partially completed
>>>>> transfer.
>>>> Hmm... According to the documentation above 
>>>> EFI_SCSI_PASS_THRU_PASSTHRU
>>>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>>>
>>>>    @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was
>>>> executed, but the
>>>>                                      entire DataBuffer could not be
>>>> transferred.
>>>>                                      The actual number of bytes
>>>> transferred is returned
>>>>                                      in TransferLength. See
>>>> HostAdapterStatus,
>>>>                                      TargetStatus, SenseDataLength, 
>>>> and
>>>> SenseData in
>>>>                                      that order for additional status
>>>> information.
>>>>
>>>> So I don't know who to believe... It does seem to me that this
>>>> documentation in the code makes more sense
>>>> and then my current code is correct. What do you think?
>>> You are looking at the wrong protocol header file.
>> Oops. You are right.
>>> The top of this
>>> header file bears the comment
>>>
>>>    SCSI Pass Through protocol as defined in EFI 1.1.
>>>
>>> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
>>> only refers to Mantis ticket 845
>>> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$ 
>>> > with subject
>>> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>>>
>>> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
>>> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
>>> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
>>> SCSI Request Packet was not executed").
>>
>> Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and 
>> ScsiPassThruExt.h header file. It doesn't make sense.
>> In the new header file, there is no way for PassThru() to indicate to 
>> caller that a partial data-transfer have occurred.
>> Which seems to be a quite standard functionality. Almost all SCSI 
>> devices are able to return information on partial data-transfer 
>> completions.
>>
>> Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete() 
>> implementation, we can see how it handles partial data-transfer 
>> completions.
>> It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual 
>> accordingly.
>> Looking at EDK2 VirtioScsi driver, function ParseResponse(), this 
>> will be handled by updating Packet->InTransferLength and 
>> Packet->OutTransferLength based on Response->Residual,
>> but then setting Packet->HostAdapterStatus to 
>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
>> (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in 
>> this case).
>>
>> The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS 
>> specifies:
>>
>>   @retval EFI_SUCCESS           The SCSI Request Packet was sent by 
>> the host. For bi-directional
>>                                 commands, InTransferLength bytes were 
>> transferred from
>>                                 InDataBuffer. For write and 
>> bi-directional commands,
>>                                 OutTransferLength bytes were 
>> transferred by
>>                                 OutDataBuffer.
>>
>> But it seems to reference the InTransferLength and OutTransferLength 
>> *before* the call to PassThru().
>>
>> In contrast to old header file, ScsiPassThru.h, which for 
>> EFI_BAD_BUFFER_SIZE specifies explicitly:
>>
>>  @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was 
>> executed, but the
>>                                     entire DataBuffer could not be 
>> transferred.
>>                                     The actual number of bytes 
>> transferred is returned
>>                                     in TransferLength. See 
>> HostAdapterStatus,
>>                                     TargetStatus, SenseDataLength, 
>> and SenseData in
>>                                     that order for additional status 
>> information.
>>
>> In addition, looking at EDK2 iSCSI ScsiPassThru driver 
>> (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use 
>> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
>> (Which matches the new header file ScsiPassThruExt.h and not the old 
>> ScsiPassThru.h).
>> If you would have a look at how IScsiOnScsiRspRcvd() handles 
>> SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW 
>> bits, it seems to always
>> set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating 
>> Packet->InTransferLength / Packet->OutTransferLength.
>>
>> What do you think? Do you have any further insights? This is quite 
>> confusing to me.
>>
>> -Liran
>>
> Furthermore, I have later found ScsiExecuteSCSICommand() in 
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls 
> the PassThru() method.
> Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is 
> relevant to us), one can see it just simply executes the PassThru() 
> device and returns.
> Examining ScsiExecuteSCSICommand() documentation specifies for 
> EFI_BAD_BUFFER_SIZE:
>
>   @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
>                               but the entire DataBuffer could not be 
> transferred.
>                               The actual number of bytes transferred 
> is returned
>                               in TransferLength. See HostAdapterStatus,
>                               TargetStatus, SenseDataLength, and 
> SenseData in
>                               that order for additional status 
> information.
>
> Which again seems to align to what I have done in my PvScsi driver...
>
> -Liran
>
Sorry for the spam but I think I eventually figured it out.

The call-chain in EDK2 looks like this:
ScsiDiskWriteSectors() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
ScsiDiskWrite16() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
ScsiWrite16Command() (MdePkg/Library/UefiScsiLib/UefiScsiLib.c)
ScsiExecuteSCSICommand() (MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c)
ExtScsiPassThru->PassThru()

It can be seen that:
* ScsiExecuteSCSICommand() just passes Packet to 
ExtScsiPassThru->PassThru() and returns it's return value.
* ScsiWrite16Command() always updates DataLength with 
Packet->OutTransferLength and returns ScsiExecuteSCSICommand() return value.
* In ScsiDiskWrite16():
   ** If EFI_BAD_BUFFER_SIZE is returned, NeedRetry is set to TRUE and 
EFI_DEVICE_ERROR is returned.
   ** Otherwise, if EFI_SUCCESS is returned but HostAdapterStatus is set 
to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, then again 
NeedRetry is set to TRUE and EFI_DEVICE_ERROR is returned.
   ** Otherwise, just return EFI_SUCCESS to caller.
* In ScsiDiskWriteSectors() we finally see the logic we were looking for:
   ** If ScsiDiskWrite16() returned EFI_SUCCESS, then we break out of 
retry loop and indeed take into account ByteCount. (Which was updated 
from Packet->OutTransferLength).
   ** Otherwise, (e.g. EFI_BAD_BUFFER_SIZE was returned), if NeedRetry 
is set to TRUE, then next request retry is updated to be done with 
ByteCount (Which was updated from Packet->OutTransferLength).

So it seems the UEFI-2.8 spec is right and a lot of EDK2 function 
documentation is out-of-date.

Therefore, I will update my code accordingly. i.e.:
1) Change PvScsi.c PopulateRequest() such that in case TransferLength is 
too big for DMA communication buffer Data field, we update 
TransferLength and return EFI_BAD_BUFFER_SIZE.
     But in addition, for completion, we should also set 
HostAdapterStatus to 
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and TargetStatus 
to EFI_EXT_SCSI_STATUS_TARGET_GOOD
     and SenseDataLength to 0 as done in VirtioScsi.c PopulateRequest() 
in case it returns EFI_BAD_BUFFER_SIZE.
2) Change PvScsi.c HandleResponse() such that:
   ** If device returned PvScsiBtStatDataUnderrun: Update TransferLength 
with Response->DataLen, set HostAdapterStatus to 
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return 
EFI_SUCCESS.
   ** If device returned PvScsiBtStatDatarun: Just set HostAdapterStatus 
to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return 
EFI_SUCCESS.

Regards,
-Liran

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Laszlo Ersek]

On 03/27/20 23:04, Liran Alon wrote:
> 
> On 28/03/2020 0:05, Laszlo Ersek wrote:
>> On 03/27/20 14:04, Liran Alon wrote:
>>> On 27/03/2020 14:26, Laszlo Ersek wrote:
>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>> +      Packet->HostAdapterStatus =
>>>>> +               
>>>>> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
>>>>> +      return EFI_BAD_BUFFER_SIZE;
>>>> I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI
>>>> spec,
>>>> EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
>>>> But that's not the case here -- we do have a partially completed
>>>> transfer.
>>> Hmm... According to the documentation above EFI_SCSI_PASS_THRU_PASSTHRU
>>> in MdePkg/Include/Protocol/ScsiPassThru.h:
>>>
>>>    @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was
>>> executed, but the
>>>                                      entire DataBuffer could not be
>>> transferred.
>>>                                      The actual number of bytes
>>> transferred is returned
>>>                                      in TransferLength. See
>>> HostAdapterStatus,
>>>                                      TargetStatus, SenseDataLength, and
>>> SenseData in
>>>                                      that order for additional status
>>> information.
>>>
>>> So I don't know who to believe... It does seem to me that this
>>> documentation in the code makes more sense
>>> and then my current code is correct. What do you think?
>> You are looking at the wrong protocol header file.
> Oops. You are right.
>> The top of this
>> header file bears the comment
>>
>>    SCSI Pass Through protocol as defined in EFI 1.1.
>>
>> and the UEFI-2.8 spec does not define EFI_SCSI_PASS_THRU_PROTOCOL; it
>> only refers to Mantis ticket 845
>> <https://urldefense.com/v3/__https://mantis.uefi.org/mantis/view.php?id=845__;!!GqivPVa7Brio!M5o2BC3kQvh7gEgQh98Kb7YJeFCM_ajknF8iRXjDer3Ir8hUy6dHOReS12oCRvE$
>> > with subject
>> "EFI_SCSI_PASS_THRU_PROTOCOL replacement".
>>
>> Instead, please consult EFI_EXT_SCSI_PASS_THRU_PASSTHRU in
>> "MdePkg/Include/Protocol/ScsiPassThruExt.h". There, the
>> EFI_BAD_BUFFER_SIZE return value conforms to the UEFI 2.8 spec ("The
>> SCSI Request Packet was not executed").
> 
> Honestly, I'm not sure if this is not a bug in UEFI-2.8 spec and
> ScsiPassThruExt.h header file. It doesn't make sense.

I agree there is a bug in the UEFI spec (but I do think the intent is
mostly clear). More below.

> In the new header file, there is no way for PassThru() to indicate to
> caller that a partial data-transfer have occurred.
> Which seems to be a quite standard functionality. Almost all SCSI
> devices are able to return information on partial data-transfer
> completions.

My understanding from the spec is that EFI_BAD_BUFFER_SIZE is for the
case when the driver can determine up-front -- without talking to the
device for the specific request's sake -- that the request is over-sized.

Otherwise (= it makes sense to submit the request to the device), a
short transfer is normal / expected. If there is no other issue with the
transfer, then InTransferLength / OutTransferLength should be updated on
output, and EFI_SUCCESS should be returned.

> 
> Looking at QEMU's virtio-scsi.c virtio_scsi_command_complete()
> implementation, we can see how it handles partial data-transfer
> completions.
> It sets Response->Response to VIRTIO_SCSI_S_OK and Response->Residual
> accordingly.
> Looking at EDK2 VirtioScsi driver, function ParseResponse(), this will
> be handled by updating Packet->InTransferLength and
> Packet->OutTransferLength based on Response->Residual,
> but then setting Packet->HostAdapterStatus to
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK and returning EFI_SUCCESS.
> (In contrast to your suggestion for me to return EFI_DEVICE_ERROR in
> this case).

This is what I wrote, verbatim:

'(9) Thus I feel we should use a "break" here.'

Note: "feel".

VirtioScsiDxe returns EFI_SUCCESS (and modifies
InTransferLength/OutTransferLength, per "residual") when the device
reports VIRTIO_SCSI_S_OK. That's a top-level success report from the device.

PVSCSI does not return a top-level success code in this case
(apparently). I have never before worked with PVSCSI, and I don't know
what exactly its return codes stand for. If "PvScsiBtStatDatarun" and
"PvScsiBtStatDataUnderrun" mean "normal partial transfer", then I fully
agree with you that EFI_DEVICE_ERROR would not be correct.

> The documentation of PassThru in ScsiPassThruExt.h for EFI_SUCCESS
> specifies:
> 
>   @retval EFI_SUCCESS           The SCSI Request Packet was sent by the
> host. For bi-directional
>                                 commands, InTransferLength bytes were
> transferred from
>                                 InDataBuffer. For write and
> bi-directional commands,
>                                 OutTransferLength bytes were transferred by
>                                 OutDataBuffer.
> 
> But it seems to reference the InTransferLength and OutTransferLength
> *before* the call to PassThru().

If you look at the individual "InTransferLength" and "OutTransferLength"
parameter (i.e., Packet member) specifications, in the spec (not the
header file), they do clarify "on input" vs. "on output". They make
sense to me, in connection with the EFI_SUCCESS return status.

(There *is* a bug, IMO, in the documentation of EFI_BAD_BUFFER_SIZE, in
the spec.)

Quote:

----v----
InTransferLength  On Input, the size, in bytes, of InDataBuffer. On
                  output, the number of bytes transferred between the
                  SCSI controller and the SCSI device. If
                  InTransferLength is larger than the SCSI controller
                  can handle, no data will be transferred,
                  InTransferLength will be updated to contain the number
                  of bytes that the SCSI controller is able to transfer,
                  and EFI_BAD_BUFFER_SIZE will be returned.

OutTransferLength On Input, the size, in bytes of OutDataBuffer. On
                  Output, the Number of bytes transferred between SCSI
                  Controller and the SCSI device. If OutTransferLength
                  is larger than the SCSI controller can handle, no data
                  will be transferred, OutTransferLength will be updated
                  to contain the number of bytes that the SCSI
                  controller is able to transfer, and
                  EFI_BAD_BUFFER_SIZE will be returned.

If the data buffer described by InDataBuffer and InTransferLength is too
big to be transferred in a single command, then no data is transferred
and EFI_BAD_BUFFER_SIZE is returned. The number of bytes that can be
transferred in a single command are returned in InTransferLength.

If the data buffer described by OutDataBuffer and OutTransferLength is
too big to be transferred in a single command, then no data is
transferred and EFI_BAD_BUFFER_SIZE is returned. The number of bytes
that can be transferred in a single command are returned in
OutTransferLength.

EFI_SUCCESS       The SCSI Request Packet was sent by the host. For
                  bi-directional commands, InTransferLength bytes were
                  transferred from InDataBuffer. For write and
                  bi-directional commands, OutTransferLength bytes were
                  transferred by OutDataBuffer. See HostAdapterStatus,
                  TargetStatus, SenseDataLength, and SenseData in that
                  order for additional status information.

EFI_BAD_BUFFER_SIZE The SCSI Request Packet was not executed. The number
                    of bytes that could be transferred is returned in
                    InTransferLength. For write and bi-directional
                    commands, OutTransferLength bytes were transferred
                    by OutDataBuffer. See HostAdapterStatus,
                    TargetStatus, and in that order for additional
                    status information.
----^----

The spec bug (IMO) is the penultimate sentence of the
"EFI_BAD_BUFFER_SIZE" documentation -- "were transferred" seems to make
no sense, given that "The SCSI Request Packet was not executed".

Regarding why VirtioScsiDxe sets HostAdapterStatus to
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK when reporting an (otherwise
successful) short transfer: I can list two reasons.

(1) Paolo had reviewed this code at least on two separate occasions --
once in October 2012 and another time in September 2015.

(2) To me, EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN seemed
much closer to VIRTIO_SCSI_S_OVERRUN, which -- per virtio spec -- stands
for something very different:

  VIRTIO_SCSI_S_OVERRUN if the content of the CDB (such as the
                        allocation length, parameter length or transfer
                        size) requires more data than is available in
                        the datain and dataout buffers.

Given that this part of VirtioScsiDxe has not caused any problems in the
past 7.5 years (it goes back to commit 37078a63b1911), i.e. setting
EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK on a partial (but otherwise OK)
transfer, I would like to change it (to what?) only if:

- Paolo recommends that too,

- there is an actual industry standard or spec that indicates the
current host status is wrong.


To summarize my (updated) opinion on your handling of
PvScsiBtStatDatarun / PvScsiBtStatDataUnderrun:

- I don't know what the HostAdapterStatus should be (on output), or
whether you should report EFI_SUCCESS vs. EFI_DEVICE_ERROR. (If those
PVSCSI response codes stand for "normal, just short, transfer", then I
would suggest to follow VirtioScsiDxe's handling -- but I don't insist
at all.)

- Either way, I am quite convinced that returning EFI_BAD_BUFFER_SIZE is
wrong, because the packet *was* submitted to the device.

> In contrast to old header file, ScsiPassThru.h, which for
> EFI_BAD_BUFFER_SIZE specifies explicitly:
> 
>  @retval EFI_BAD_BUFFER_SIZE       The SCSI Request Packet was executed,
> but the
>                                     entire DataBuffer could not be
> transferred.
>                                     The actual number of bytes
> transferred is returned
>                                     in TransferLength. See
> HostAdapterStatus,
>                                     TargetStatus, SenseDataLength, and
> SenseData in
>                                     that order for additional status
> information.
> 
> In addition, looking at EDK2 iSCSI ScsiPassThru driver
> (NetworkPkg/IScsiDxe/IScsiExtScsiPassThru.c), one can see it use
> EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET
> (Which matches the new header file ScsiPassThruExt.h and not the old
> ScsiPassThru.h).
> If you would have a look at how IScsiOnScsiRspRcvd() handles
> SCSI_RSP_PDU_FLAG_BI_READ_OVERFLOW and SCSI_RSP_PDU_FLAG_OVERFLOW bits,
> it seems to always
> set Status to EFI_BAD_BUFFER_SIZE as I have done. After updating
> Packet->InTransferLength / Packet->OutTransferLength.

Great example!

And yes, I do feel that returning EFI_BAD_BUFFER_SIZE, after updating
InTransferLength / OutTransferLength from a field called
"ResidualCount", is wrong. "ResidualCount" is coming back in a response
structure, and its name ("residual") indicates the device transfer was
actually initiated on the remote end.

To me EFI_BAD_BUFFER_SIZE just has a different "spirit". It generally
means "I couldn't begin to act upon the request due to the wrong buffer
size. Adjust the buffer size, and try again".

A partial / short transfer is different from that.

> What do you think? Do you have any further insights? This is quite
> confusing to me.

I agree that it's very confusing.

I'm not sure if this is covered by the UEFI SCT (self conformance test).

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Laszlo Ersek]

On 03/27/20 23:17, Liran Alon wrote:

> Furthermore, I have later found ScsiExecuteSCSICommand() in
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c to be the one that calls the
> PassThru() method.
> Looking at it's "if (ScsiIoDevice->ExtScsiSupport)" branch (Which is
> relevant to us), one can see it just simply executes the PassThru()
> device and returns.
> Examining ScsiExecuteSCSICommand() documentation specifies for
> EFI_BAD_BUFFER_SIZE:
> 
>   @retval EFI_BAD_BUFFER_SIZE The SCSI Request Packet was executed,
>                               but the entire DataBuffer could not be
> transferred.
>                               The actual number of bytes transferred is
> returned
>                               in TransferLength. See HostAdapterStatus,
>                               TargetStatus, SenseDataLength, and
> SenseData in
>                               that order for additional status information.
> 

(1) The commit that added the "ScsiIoDevice->ExtScsiSupport" branch to ScsiExecuteSCSICommand() was 70c94b3b6ddf ("Porting R8's PI-enabled ScsiBus driver", 2007-07-02).

$ git show -U100 70c94b3b6ddf

At that time, the ScsiExecuteSCSICommand() function had the following documentation:

     EFI_SUCCESS           - The SCSI Request Packet was sent by the host 
                             successfully, and TransferLength bytes were 
                             transferred to/from DataBuffer.See 
                             HostAdapterStatus, TargetStatus, 
                             SenseDataLength, and SenseData in that order
                             for additional status information.
     EFI_WARN_BUFFER_TOO_SMALL - The SCSI Request Packet was executed, 
                             but the entire DataBuffer could not be transferred.
                             The actual number of bytes transferred is returned
                             in TransferLength. See HostAdapterStatus, 
                             TargetStatus, SenseDataLength, and SenseData in 
                             that order for additional status information.

No "EFI_BAD_BUFFER_SIZE".

So commit 70c94b3b6ddf was not entirely correct, because after it, the ScsiExecuteSCSICommand() could return EFI_BAD_BUFFER_SIZE (propagating it from ScsiIoDevice->ExtScsiPassThru->PassThru()), but the function's documentation was not updated.

(At the same commit, "MdePkg/Include/Protocol/ScsiPassThruExt.h" already specified EFI_BAD_BUFFER_SIZE as "The SCSI Request Packet was not executed" -- that had come from commit d1f950002362, "Checked in the Protocols introduced in UEFI/PI.", 2007-06-19.)


(2) In commit f36d6e669c97 (2007-09-20), the leading comment block on ScsiExecuteSCSICommand() was updated. The EFI_WARN_BUFFER_TOO_SMALL retval disappeared, and the incorrect EFI_BAD_BUFFER_SIZE language ("The SCSI Request Packet was executed") appeared. (Incorrect for the "EXT" passthru, anyway.)

Interestingly, in this commit message, we see:

    3. Correctify some return status to sync with newest Uefi Spec 2.1

However, in the UEFI 2.0 spec <http://www.uefi.org/sites/default/files/resources/UEFI_Specification_2_and_Errata_Sept16_08.pdf>, EFI_BAD_BUFFER_SIZE is already defined as "The SCSI Request Packet was not executed".

So I think it's a bug in edk2 (at least a documentation bug).

Thanks
Laszlo

Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

[Laszlo Ersek]

On 03/28/20 20:18, Liran Alon wrote:

> Sorry for the spam but I think I eventually figured it out.

It's not spam; thank you for the thorough investigation!

> The call-chain in EDK2 looks like this:
> ScsiDiskWriteSectors() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
> ScsiDiskWrite16() (MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c)
> ScsiWrite16Command() (MdePkg/Library/UefiScsiLib/UefiScsiLib.c)
> ScsiExecuteSCSICommand() (MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBus.c)
> ExtScsiPassThru->PassThru()
> 
> It can be seen that:
> * ScsiExecuteSCSICommand() just passes Packet to
> ExtScsiPassThru->PassThru() and returns it's return value.
> * ScsiWrite16Command() always updates DataLength with
> Packet->OutTransferLength and returns ScsiExecuteSCSICommand() return
> value.
> * In ScsiDiskWrite16():
>   ** If EFI_BAD_BUFFER_SIZE is returned, NeedRetry is set to TRUE and
> EFI_DEVICE_ERROR is returned.
>   ** Otherwise, if EFI_SUCCESS is returned but HostAdapterStatus is set
> to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN, then again
> NeedRetry is set to TRUE and EFI_DEVICE_ERROR is returned.
>   ** Otherwise, just return EFI_SUCCESS to caller.
> * In ScsiDiskWriteSectors() we finally see the logic we were looking for:
>   ** If ScsiDiskWrite16() returned EFI_SUCCESS, then we break out of
> retry loop and indeed take into account ByteCount. (Which was updated
> from Packet->OutTransferLength).
>   ** Otherwise, (e.g. EFI_BAD_BUFFER_SIZE was returned), if NeedRetry is
> set to TRUE, then next request retry is updated to be done with
> ByteCount (Which was updated from Packet->OutTransferLength).
> 
> So it seems the UEFI-2.8 spec is right and a lot of EDK2 function
> documentation is out-of-date.

On a tangent... Since you mention ScsiDiskWriteSectors(), let me mention commit 5abc2a70da4f ("MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers", 2015-09-10), and the related mailing list discussion (this is what I referred to before, when I mentioned that Paolo had looked at the VirtioScsiDxe code twice -- this is the second occasion, from September 2015):

http://mid.mail-archive.com/1441390936-27763-1-git-send-email-lersek@redhat.com

> Therefore, I will update my code accordingly. i.e.:
> 1) Change PvScsi.c PopulateRequest() such that in case TransferLength is
> too big for DMA communication buffer Data field, we update
> TransferLength and return EFI_BAD_BUFFER_SIZE.

Sounds OK.

>     But in addition, for completion, we should also set
> HostAdapterStatus to
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and TargetStatus
> to EFI_EXT_SCSI_STATUS_TARGET_GOOD
>     and SenseDataLength to 0 as done in VirtioScsi.c PopulateRequest()
> in case it returns EFI_BAD_BUFFER_SIZE.

OK, thanks.

> 2) Change PvScsi.c HandleResponse() such that:
>   ** If device returned PvScsiBtStatDataUnderrun: Update TransferLength
> with Response->DataLen, set HostAdapterStatus to
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
> EFI_SUCCESS.

OK.

>   ** If device returned PvScsiBtStatDatarun: Just set HostAdapterStatus
> to EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN and return
> EFI_SUCCESS.

Again I'm not familiar with PvScsiBtStatDataUnderrun / PvScsiBtStatDatarun (let alone their differences); I'm OK with this too.

Thanks!
Laszlo

Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

[Laszlo Ersek]

On 03/27/20 22:31, Liran Alon wrote:
> 
> On 27/03/2020 16:35, Laszlo Ersek wrote:
>> On 03/27/20 01:05, Liran Alon wrote:
>>> On 27/03/2020 0:17, Laszlo Ersek wrote:
>>>> On 03/25/20 17:10, Liran Alon wrote:
>>>>>      PvScsiRestorePciAttributes (Dev);
>>>>> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>>> index 6d23b6e1eccf..7f91d70fec79 100644
>>>>> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
>>>>> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
>>>>> @@ -31,6 +31,11 @@ typedef struct {
>>>>>      PVSCSI_DMA_DESC      RingCmpsDmaDesc;
>>>>>    } PVSCSI_RING_DESC;
>>>>>    +typedef struct {
>>>>> +  UINT8     SenseData[MAX_UINT8];
>>>> (4) Is the maximum possible size of the sense data specified
>>>> somewhere? If so, it would be nice to document it with a comment at
>>>> least.
>>> This is a good point. Thanks for pointing it out.
>>> Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1
>>> Descriptor format sense data overview specifies:
>>> "The additional sense length shall be less than or equal to 244 (i.e.,
>>> limiting the total length of the sense data to 252 bytes)"
>>>
>>> i.e. The max possible size of sense data is 252.
>>> As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed
>>> defining:
>>>
>>> #define SCSI_SENSE_BUF_SIZE 252
>>>
>>> This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
>>> sense buf size to 252") which says in it's commit message:
>>> "According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
>>>
>>> Interestingly, this QEMU commit changed the value of
>>> SCSI_SENSE_BUF_SIZE from 96 to 252.
>>> But then I found this in EDK2
>>> OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
>>>
>>> //
>>> // We expect these maximum sizes from the host. Also we force the
>>> CdbLength and
>>> // SenseDataLength parameters of
>>> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
>>> // to exceed these limits. See UEFI 2.3.1 errata C 14.7.
>>> //
>>> #define VIRTIO_SCSI_CDB_SIZE   32
>>> #define VIRTIO_SCSI_SENSE_SIZE 96
>>>
>>> Which seems to be wrong...
>> No, these macros / limits are valid. They match the virtio specification
>> (both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size"
>> configuration values that the virtio-scsi device is required to expose
>> by default.
>>
>> While the driver could theoretically ask for larger sizes, these values
>> have never caused a problem in practice. I don't see a reason to change
>> the constants (let alone to complicate the code).
>>
>>> It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to
>>> EDK2 MdePkg/Include/IndustryStandard/Scsi.h.
>>> And then use that from my PvScsi driver.
>>>
>>> I can also submit a separate patch (Not part of this series) to remove
>>> this VIRTIO_SCSI_SENSE_SIZE constant.
>> Please don't; there's no reason to change the VirtioScsiDxe driver.
>>
>>> Note that VirtioScsi doesn't have a technical issue here because it
>>> provides this max sense length to the device in VirtioScsiInit():
>>>
>>> Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
>> Right, and even those config writes are mostly documentation / "just to
>> be sure" oriented. Because, there's also a comment in those parts:
>>
>>    //
>>    // We expect these maximum sizes from the host. Since they are
>>    // guest-negotiable, ask for them rather than just checking them.
>>    //
>>
>>> So if it's OK by you, I think I will just add a patch to this series
>>> defining SCSI_MAX_SENSE_SIZE in
>>> MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when
>>> defining SenseData in PVSCSI_DMA_BUFFER.
>> My experience tells me that making any OvmfPkg change dependent on new
>> patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...)
>> slows down the upstreaming of the OvmfPkg change significantly.
>>
>> Therefore, I would strongly advise against this ordering of changes.
>>
>> Today, upon reviewing patch#15, I commented again on the "SenseData"
>> array size (see under point (2) in my patch#15 feedback). Accordingly,
>> for now, please stick with the existent "SenseData" array declaration,
>> just add a comment. I expect / hope to merge your v3 posting.
> As an alternative, I can also define SenseData to be of size 252 (which
> I would #define as a constant in PvScsi.h),
> and explain that it is the limit of total length of SenseData according
> to SCSI specification.
> 
> And indeed leave for a separate patch-series to move this PvScsi.h
> private constant and put it in the generic
> IndustryStandard/Scsi.h header file.
> 
> I think it's nicer than just defining SenseData to be MAX_UINT8 because
> of the field size in PassThru packet.
> But I will of course do as you prefer. Just let me know your choice :)

A new PvScsi.h-internal macro, with replacement text "252", sounds nice.

But in that case, you'll have to check (or trim) the
"EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.SenseDataLength" field value
on input.

I'm fine either way -- my main point is that we shouldn't report more
sense room to the device than what we have available in the DMA buffer.
If the caller passes in a SenseDataLength that's larger than 252 (such
as 254, for example), and our DMA buffer field is 252 bytes large, then
we should give 252 to the device, and report back to the caller
accordingly (= no more than 252 bytes).

Thanks,
Laszlo

> 
>>
>> (Side comment: I would also like to ask Nikita to R-b the full final
>> patch set on the list, once I'm ready to merge the series, because I'd
>> like to testify to Nikita's review effort in the upstream git history.)
> 
> Ok. I will let him know to review my v3 submission upstream.
> 
> -Liran
> 
>>
>> Then, with the series merged, you can propose a separate patch series (2
>> patches), later -- introducing the new constant in MdePkg, plus putting
>> the new constant to use in the (then-upstream) PvScsi feature. No matter
>> how long that is delayed, the main feature will have been merged.
>>
>