From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web10.3606.1585048539838004121 for ; Tue, 24 Mar 2020 04:15:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=d5V9Gw/T; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585048538; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YPlAG+yXLINF48YOB/5VcvRxQnKtqOkVVBo/0AvswKQ=; b=d5V9Gw/T+xuvCL0UoxdIa4YcbLkMBGHklzXawE669PXgTsGHptx3IgBAPP6Iw1PqtN+tmO YHqc9emjiHYB+YSXmb5xaMcOEuAl101HsJ4rk3no6jmiZpVifP0lPv9J+fmyLA/0bKOvdP aSV0WLlek0ODNBMrtOfrHkCwqJBOv8c= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-402-cIchqQecOaWWaEX82umgeg-1; Tue, 24 Mar 2020 07:15:33 -0400 X-MC-Unique: cIchqQecOaWWaEX82umgeg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D1699DB66; Tue, 24 Mar 2020 11:15:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-139.ams2.redhat.com [10.36.115.139]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2F22290766; Tue, 24 Mar 2020 11:15:30 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 01/17] OvmfPkg/PvScsiDxe: Create empty driver To: devel@edk2.groups.io, liran.alon@oracle.com Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200316150113.104630-1-liran.alon@oracle.com> <20200316150113.104630-2-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 24 Mar 2020 12:15:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200316150113.104630-2-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 (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 > --- > 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 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