From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 11 Apr 2019 04:33:35 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7CFAB307D97F; Thu, 11 Apr 2019 11:33:33 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-225.rdu2.redhat.com [10.10.120.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id DBC765C8BC; Thu, 11 Apr 2019 11:33:30 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 09/31] OvmfPkg/XenOvmf: use a TimerLib instance that depends only on the CPU From: "Laszlo Ersek" To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Jordan Justen , Ard Biesheuvel , Julien Grall , xen-devel@lists.xenproject.org References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-10-anthony.perard@citrix.com> <7dbe25f9-841c-6e98-f961-29e5f4505725@redhat.com> Message-ID: Date: Thu, 11 Apr 2019 13:33:24 +0200 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: <7dbe25f9-841c-6e98-f961-29e5f4505725@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Thu, 11 Apr 2019 11:33:34 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/11/19 13:25, Laszlo Ersek wrote: > On 04/09/19 13:08, Anthony PERARD wrote: >> ACPI Timer does not work in a PVH guest, but local APIC works on both >> PVH and HVM. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Anthony PERARD >> --- >> OvmfPkg/XenOvmf.dsc | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc >> index 7b8a1fdf6b..cc51bac3be 100644 >> --- a/OvmfPkg/XenOvmf.dsc >> +++ b/OvmfPkg/XenOvmf.dsc >> @@ -104,7 +104,7 @@ [SkuIds] >> ################################################################################ >> [LibraryClasses] >> PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf >> - TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf >> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf >> PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf >> BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf >> BaseLib|MdePkg/Library/BaseLib/BaseLib.inf >> @@ -205,7 +205,7 @@ [LibraryClasses.common] >> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf >> >> [LibraryClasses.common.SEC] >> - TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf >> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf >> QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf >> !ifdef $(DEBUG_ON_SERIAL_PORT) >> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf >> @@ -284,7 +284,7 @@ [LibraryClasses.common.DXE_CORE] >> >> [LibraryClasses.common.DXE_RUNTIME_DRIVER] >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf >> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf >> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf >> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf >> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf >> @@ -301,7 +301,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER] >> >> [LibraryClasses.common.UEFI_DRIVER] >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf >> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf >> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf >> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf >> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf >> @@ -316,7 +316,7 @@ [LibraryClasses.common.UEFI_DRIVER] >> >> [LibraryClasses.common.DXE_DRIVER] >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf >> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf >> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf >> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf >> @@ -344,7 +344,7 @@ [LibraryClasses.common.DXE_DRIVER] >> >> [LibraryClasses.common.UEFI_APPLICATION] >> PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf >> - TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf >> + TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf >> HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf >> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf >> > > (1) I suggest simplifying this patch by: > - deleting all TimerLib resolutions except the one under > [LibraryClasses], > - updating that one (remaining) resolution to SecPeiDxeTimerLibCpu.inf. > > With that: > > Acked-by: Laszlo Ersek ... BTW, this is the first time I hear about MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf so now I've gone ahead and read the documentation in that INF file. (It's really nice documentation.) It says, # Note: A driver of type DXE_RUNTIME_DRIVER and DXE_SMM_DRIVER can use this TimerLib # in their initialization without any issues. They only have to be careful in # the implementation of runtime services and SMI handlers. # Because CPU Local APIC and ITC could be programmed by OS, it cannot be # used by SMM drivers and runtime drivers, ACPI timer is recommended for SMM # drivers and runtime drivers. Now, Xen doesn't use SMM, and runtime drivers can be penetrated by the OS anyway, so I'm not concerned about security here. It's just that you could run into unexpected behavior with runtime drivers, as long as Xen allows the guest OS to program the hardware like hinted above. Obviously I haven't checked the runtime drivers included by this new platform for their consumption of TimerLib. The patch does modify [LibraryClasses.common.DXE_RUNTIME_DRIVER], so it's likely prudent for you to build the platform with "--report-file=blah.report", and check all runtime drivers. For each that consumes TimerLib, I suggest also checking if they call a TimerLib API at runtime, not just at initialization. If it turns out that the above is not only theoretical, I think the commit message might deserve a note about the fact, but I'm OK to ACK the patch without such a note too. Thanks Laszlo