From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::d42; helo=mail-io1-xd42.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 671E1211B5090 for ; Wed, 9 Jan 2019 02:28:19 -0800 (PST) Received: by mail-io1-xd42.google.com with SMTP id v10so5575151ios.13 for ; Wed, 09 Jan 2019 02:28:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ymGDMwy4/J7+nXbvCA+JTDEloDF0x7nj2dKWgfVwY3Q=; b=E+Uc7JmE6drdzSkOQb+lHrbt9g+2LHCjm29wXieZqdj7yrp3i7agx4gOpv6ID98x58 F8jWUqDznyyziivBa1f9NsvKS8ACFMWAriurNRAOC43ryD8ygaovF7Aix0mwN092LwIL 0cN2rBmXabfXPbz74/Y6KjHB+z7mzaqOuY8GQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ymGDMwy4/J7+nXbvCA+JTDEloDF0x7nj2dKWgfVwY3Q=; b=f1PhiYRLHUdhX7ttNNKHpuFolBN4fq0LY0RcpJn3O5mysXgYbrD5Md3Sg+mfc9poVt 0/0/1KsiiDP8nGoySVcxAAoBvDD4wBmYqLvVCXuj01rXAqKjqpiTNmCFGQpjSj87PclP v2fgKKdJxwc0bhZVsW4B1dLD26IzpEBfzCt1yBoLu3dKtQXjil8eMuK2Nl+dHORj9vlH ZPuZYyhhnwwMT05p+jUGR7MUtO/kS5Fpu6rldEKpqlxhxS4U8T8Vx2bAIpDY7/9lZdMD 9GGIIFvPv8o/v+LDWlv6tFL4NWTyrnQHwIsq+onT0yfbt1cPoPWqsWYNkPWzTW5oY5oz xYXg== X-Gm-Message-State: AJcUukeDmKEnPUp1IgYzYZfk1KLmHNl+wbolRoRFM/46ZnbRis1ObSDV mtEaksNTDeuWJgr2GZJ/74cKmSIzUL1PKiz2CWxGEA== X-Google-Smtp-Source: ALg8bN4DlQMGSC1mafx8/MzuH1FAIJ2yOYmqHgfnGP4TrxV/NitSgQ8zD25vh1G4XDMBEo/eIWPBdKX47CdEheQkqj0= X-Received: by 2002:a5e:c206:: with SMTP id v6mr3609291iop.60.1547029698218; Wed, 09 Jan 2019 02:28:18 -0800 (PST) MIME-Version: 1.0 References: <20190103182825.32231-1-ard.biesheuvel@linaro.org> In-Reply-To: From: Ard Biesheuvel Date: Wed, 9 Jan 2019 11:28:07 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Michael D Kinney , Liming Gao , Jian J Wang , Hao Wu , Jagadeesh Ujja , Achin Gupta , Thomas Panakamattam Abraham , Sami Mujawar Subject: Re: [PATCH 0/6] implement standalone MM versions of the variable runtime drivers X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Jan 2019 10:28:19 -0000 Content-Type: text/plain; charset="UTF-8" On Wed, 9 Jan 2019 at 10:44, Laszlo Ersek wrote: > > On 01/03/19 19:28, Ard Biesheuvel wrote: > > This series proposed an alternative approach to the series sent out by > > Jagadeesh [0]. In particular, it gets rid of the InMm() calls and the > > special PCD, as well as some other if() conditionals. > > > > The primary difference is that this series defines and implements > > MmServicesTableLib in such a way that the traditional SMM drivers can > > use it as well. This is appropriate, considering that the PI spec has > > rebranded traditional SMM as one implementation of the generic MM > > framework. > > > > Patch #1 is based on Jagadeesh's patch, and introduces the > > MmServicesTableLib library class, but for all SMM flavours, not only > > for standalone MM. > > > > Patch #2 implements MmServicesTableLib for traditional SMM > > implementations. > > > > Patch #3 refactors FaultTolerantWriteDxe so that the parts of the SMM > > driver that invoke boot services are separated from the core SMM > > pieces. > > > > Patch #4 implements FaultTolerantWriteSmm for the standalone MM > > environment. > > > > Patches #5 and #6 do the same, respectively, for the variable runtime > > driver. > > > > This approach minimizes the delta, and thus the maintenance burden, > > between the traditional SMM and standalone MM drivers, while not > > resorting to runtime checks or other conditionals in the code to > > implement logic that should be decided at build time. > > > > Note that this series only covers part of the work contributed by > > Jagadeesh. This series focuses on the MdePkg and MdeModulePkg changes > > that affect shared code. > > > > Cc: Laszlo Ersek > > Cc: Leif Lindholm > > Cc: Michael D Kinney > > Cc: Liming Gao > > Cc: Jian J Wang > > Cc: Hao Wu > > Cc: Jagadeesh Ujja > > Cc: Achin Gupta > > Cc: Thomas Panakamattam Abraham > > Cc: Sami Mujawar > > I tried building this, on top of commit a53a888de8f5: > > build \ > -a IA32 \ > -p OvmfPkg/OvmfPkgIa32.dsc \ > -t GCC48 \ > -b NOOPT \ > -D HTTP_BOOT_ENABLE \ > -D NETWORK_IP6_ENABLE \ > -D SECURE_BOOT_ENABLE \ > -D SMM_REQUIRE \ > -D TLS_ENABLE \ > --cmd-len=65536 \ > --hash \ > --genfds-multi-thread > > but it failed with: > > > OvmfPkg/OvmfPkgIa32.dsc(...): error 4000: Instance of library class [MmServicesTableLib] is not found > > in [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf] [IA32] > > consumed by module [MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf] > > You did mention earlier that adding new lib class resolutions to several > x86 DSC files would be necessary, so this is not unexpected. Can you > please insert such a patch for OvmfPkg between patches #2 and #3? > Ah yes, of course. > I've looked at the current OVMF DSC files, and SmmServicesTableLib is > resolved for two module types, > > > [LibraryClasses.common.DXE_SMM_DRIVER] > > SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf > > > > [LibraryClasses.common.SMM_CORE] > > SmmServicesTableLib|MdeModulePkg/Library/PiSmmCoreSmmServicesTableLib/PiSmmCoreSmmServicesTableLib.inf > > I assume it should be enough, for this series, to update the > DXE_SMM_DRIVER resolution only, and to leave SMM_CORE alone. > > (Because, my understanding is that the current, x86 specific > > MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf > > module, of type SMM_CORE, will not be refactored; instead, it is > entirely supplanted -- in the affected platforms -- by the > > StandaloneMmPkg/Core/StandaloneMmCore.inf > > module, which is of type MM_CORE_STANDALONE.) > > But, it's still not clear to me (without trying) whether I should > resolve MmServicesTableLib for DXE_SMM_DRIVER in addition to > SmmServicesTableLib, or in its place. I'd prefer not experimenting with > this myself; I'd just like to apply the series, and build & test it. :) > At the moment, you will need both resolutions for DXE_SMM_DRIVERS globally. Based on the outcome of the review of this series, this is something we will need to clean up going forward, but currently, even the drivers that are updated to use MmServicesTableLib pull in libraries that depend on SmmServicesTableLib. This should be a rather straight-forward search/replace operation [famous last words], and I can commit to dedicating some of my time to getting this fixed throughout, at least to the point where modules that consume MmServicesTableLib no longer have to supply a resolution for SmmServicesTableLib as well. So, I will include a patch in the next revision of the series. In the mean time, the hunk below should suffice to complete your regression testing. --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -391,6 +391,7 @@ HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf + MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf !ifdef $(DEBUG_ON_SERIAL_PORT) DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf !else