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

["Laszlo Ersek" <lersek@redhat.com>]

On 03/16/20 16:00, 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
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>

(1) As stated earlier, I'll have to strip the feedback tags not given on
the list. Can you strip them for me in v2 please? (Nikita can re-ack on
the list, but I'd suggest waiting with that until the series stabilizes.)

> 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   | 24 ++++++++++++++++++++++++
>  OvmfPkg/PvScsiDxe/PvScsi.inf | 27 +++++++++++++++++++++++++++
>  8 files changed, 84 insertions(+)
>  create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.c
>  create mode 100644 OvmfPkg/PvScsiDxe/PvScsi.inf
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 19728f20b34e..79b8c58e54c3 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
> +!ifdef $(PVSCSI_ENABLE)
> +  OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +!endif
>    MdeModulePkg/Universal/WatchdogTimerDxe/WatchdogTimer.inf
>    MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/MonotonicCounterRuntimeDxe.inf
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf

(2) Please use the following directive instead:

!if $(PVSCSI_ENABLE) == TRUE

Because, passing "-D PVSCSI_ENABLE=FALSE" on the "build" utility's
command line would satisfy "!ifdef" (I've double-checked that with a
small ad-hoc patch right now). The "build" utility does not support an
"-U" option (for undefining a macro), like "cpp" does.

Note that this request is consistent with the existent !if and
!ifdef/!ifndef directives in the DSC files. The latter directive is only
applied to the following macros:

- FD_SIZE_1MB
- FD_SIZE_2MB
- FD_SIZE_4MB
- DEBUG_ON_SERIAL_PORT
- CSM_ENABLE

And none of those are DEFINE'd by default in the [Defines] section.
Therefore a "build" invocation can leave them undefined, or define them.
It never needs to "undo" them.

The boolean macros with default values assigned under [Defines] are
different -- we should allow "build" to disable them with a "-D
MACROBOOL=FALSE" option, and for that we should compare them against
TRUE (or FALSE) explicitly.

> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index 63607551ed75..f59a58bc2689 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

Yes, this is OK.

(3) However, please do not indent the conditional "INF" line. See other
!if directives in the same file.

(... Unfortunately, the SecureBootConfigDxe line, visible in the context
above, is precisely the one preexistent example that doesn't follow the
rule. :/ All the other !if's and !ifdef's do.)

> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 3c0c229e3a72..744f7eb05e12 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
> +!ifdef $(PVSCSI_ENABLE)
> +  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..5fd21ea1b2de 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

(4) In all of the above DSC and FDF files (Ia32 and Ia32X64), you refer
to "PvScsiDxe.inf". But the INF file is in fact called "PvScsi.inf". The
references are correct only in the X64 DSC and FDF files, below. The
32-bit PEI phase platforms no longer build with this patch:

build.py...
 : error 000E: File/directory not found in workspace
        .../OvmfPkg/PvScsiDxe/PvScsiDxe.inf

Please do not submit untested code for review. It is de-motivating to
find bugs by code inspection that could have been trivially caught in
testing.

I guess you had built the IA32 and IA32X64 platforms too, at some point,
but then you may have renamed the INF file, and forgot to update the
references, and to rebuild as well. It is best to always build all three
platforms (no matter how small the change), in a reasonably "complete"
configuration.

> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index f6c1d8d228c6..64ed3d5ec18e 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
> +!ifdef $(PVSCSI_ENABLE)
> +  OvmfPkg/PvScsiDxe/PvScsi.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..c155993dc16f 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/PvScsi.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..a3f704d60d77
> --- /dev/null
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -0,0 +1,24 @@
> +/** @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
> +
> +**/
> +
> +//
> +// Entry Point
> +//
> +
> +EFI_STATUS
> +EFIAPI
> +PvScsiEntryPoint (
> +  IN EFI_HANDLE       ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}

I'd #include <Uefi/UefiSpec.h> for EFI_SYSTEM_TABLE (and that #include
would bring in the rest of EFI_STATUS, EFI_HANDLE etc). But, I fully
agree that the C file already builds fine like this, due to the
auto-generated headers and whatnot, so it's OK.

> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.inf b/OvmfPkg/PvScsiDxe/PvScsi.inf
> new file mode 100644
> index 000000000000..093cc0171338
> --- /dev/null
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.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

Not consistent with the name of the INF file ("PvScsi.inf"), but it is
certainly not a deal breaker. It's OK if you don't want to bring those
in sync.

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

Thanks,
Laszlo