From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web09.7829.1580315815758705166 for ; Wed, 29 Jan 2020 08:36:55 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=jA90gQUC; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580315812; 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=gLAD0GfjaQFBxJxSh1nOV6R/Tmkn0HodB9kRD1rATVw=; b=jA90gQUC7ouy1+Ua/7tQh1sGifZiDbRu7FZf2FlEv3EPFa+6CZPjOjTm3UDStPjpP0PJ2V vOM9N/me77G53dVrQFziiHJCCFyMbuIs5eegBo+jmqZDXcc1gFRCCjDdH6r+qosDRU0ZRW 83onLT7D06IUB5K1KAxpBQJ2UmKC4l4= 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-316-K4BvAp7bMve4CGSH1yYWag-1; Wed, 29 Jan 2020 11:36:34 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AA77C8D23F1; Wed, 29 Jan 2020 16:36:32 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-32.ams2.redhat.com [10.36.116.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4BFC190BC; Wed, 29 Jan 2020 16:36:30 +0000 (UTC) Subject: Re: [PATCH 5/5] OvmfPkg/OvmfXen: Set PcdFSBClock To: Anthony PERARD , devel@edk2.groups.io Cc: Michael D Kinney , Ard Biesheuvel , xen-devel@lists.xenproject.org, Liming Gao , Jordan Justen , Julien Grall References: <20200129121235.1814563-1-anthony.perard@citrix.com> <20200129121235.1814563-6-anthony.perard@citrix.com> From: "Laszlo Ersek" Message-ID: <096722ad-98a4-0cc4-e7d0-4bac4cb746c2@redhat.com> Date: Wed, 29 Jan 2020 17:36:25 +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: <20200129121235.1814563-6-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: K4BvAp7bMve4CGSH1yYWag-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 01/29/20 13:12, Anthony PERARD wrote: > Update gEfiMdePkgTokenSpaceGuid.PcdFSBClock so it can have the correct > value when SecPeiDxeTimerLibCpu start to use it for the APIC timer. > > Currently, nothing appear to use the value in PcdFSBClock before > XenPlatformPei had a chance to set it even though TimerLib is included > in modules runned before XenPlatformPei. > > XenPlatformPei doesn't use any of the functions that would use that > value. No other modules in the PEI phase seems to use the TimerLib > before PcdFSBClock is set. There are currently two other modules in > the PEI phase that needs the TimerLib: > - S3Resume2Pei, but only because LocalApicLib needs it, but nothing is > using the value from PcdFSBClock. > - CpuMpPei, but I believe it only runs after XenPlatformPei > > Before the PEI phase, there's the SEC phase, and SecMain needs > TimerLib because of LocalApicLib. And it initialise the APIC timers > for the debug agent. But I don't think any of the DebugLib that > OvmfXen could use are actually using the *Delay functions in TimerLib, > and so would not use the value from PcdFSBClock which would be > uninitialised. > > A simple runtime test showed that TimerLib doesn't use PcdFSBClock > value before it is set. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490 > Signed-off-by: Anthony PERARD > --- > OvmfPkg/OvmfXen.dsc | 4 +--- > OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 1 + > OvmfPkg/XenPlatformPei/Xen.c | 4 ++++ > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc > index 8c11efe9b709..190d7400c148 100644 > --- a/OvmfPkg/OvmfXen.dsc > +++ b/OvmfPkg/OvmfXen.dsc > @@ -442,9 +442,6 @@ [PcdsFixedAtBuild] > # Point to the MdeModulePkg/Application/UiApp/UiApp.inf > gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 } > > - ## Xen vlapic's frequence is 100 MHz > - gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000 > - > ################################################################################ > # > # Pcd Dynamic Section - list of all EDK II PCD Entries defined by this Platform > @@ -468,6 +465,7 @@ [PcdsDynamicDefault] > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0 > gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000 > > + gEfiMdePkgTokenSpaceGuid.PcdFSBClock (1) This syntax looks strange; I thought it was mandatory to provide a default value too. https://edk2-docs.gitbooks.io/edk-ii-dsc-specification/content/2_dsc_overview/28_pcd_sections.html --------- 2.8.3.1 PcdsDynamicDefault [...] The format for a boolean or numeric datum type PCD entry in this section is: PcdTokenSpaceGuidCName.PcdCName|Value --------- I'm not sure if the "build" utility accepts this intentionally, or by mistake. Can you simply keep the "|100000000" part too? Otherwise, I'm OK with the argument laid out in the commit message. (Thank you for the detailed commit message!) With (1) fixed: Reviewed-by: Laszlo Ersek Thanks! Laszlo > gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0 > > # Set video resolution for text setup. > diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > index 335a442538c2..177200f3b7e5 100644 > --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf > @@ -83,6 +83,7 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode > gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask > + gEfiMdePkgTokenSpaceGuid.PcdFSBClock > gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy > gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress > > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > index d6cdc9a8e31c..fc990462dccc 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -504,6 +504,10 @@ CalibrateLapicTimer ( > / (TscTick2 - TscTick); > DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq)); > > + ASSERT (Freq <= MAX_UINT32); > + Status = PcdSet32S (PcdFSBClock, Freq); > + ASSERT_RETURN_ERROR (Status); > + > UnmapXenPage (SharedInfo); > > Exit: >