xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: devel@edk2.groups.io, anthony.perard@citrix.com
Cc: xen-devel@lists.xenproject.org,
	Jordan Justen <jordan.l.justen@intel.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Julien Grall <julien@xen.org>
Subject: Re: [edk2-devel] [PATCH v2 7/7] OvmfPkg/OvmfXen: Set PcdFSBClock
Date: Wed, 7 Apr 2021 11:25:49 +0200	[thread overview]
Message-ID: <344af4b2-0ce4-23a9-7fde-bf1ee0343d27@redhat.com> (raw)
In-Reply-To: <20210325154713.670104-8-anthony.perard@citrix.com>

On 03/25/21 16:47, Anthony PERARD via groups.io 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.

(1) s/runned/run/

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

Effectively, yes.


* Assuming CpuMpPei were loaded / dispatched before XenPlatformPei, the
following would happen:

CpuMpPeimInit() [UefiCpuPkg/CpuMpPei/CpuMpPei.c] is the entry point
function of CpuMpPei, and all it does is register a notify for the
"memory discovered" PPI. The actual module initialization occurs in
MemoryDiscoveredPpiNotifyCallback().

The "memory discovered PPI" is produced as follows:

- XenPlatformPei calls PublishSystemMemory()

- XenPlatformPei exits

- the PEI Core relocates stuff (HOBs, PEIMs) to the permanent PEI
memory, and re-enters itself in the new area

- the PEI core installs the "memory discovered" PPI

- MemoryDiscoveredPpiNotifyCallback() is called.


* If XenPlatformPei is loaded / dispatched before CpuMpPei (and so the
"memory discovered" PPI exist at the time of CpuMpPei starting), then
MemoryDiscoveredPpiNotifyCallback() is immediately invoked in CpuMpPei,
when the PPI notify is registered.


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

OK.

> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v2:
>     - keep the default value of PcdFSBClock because that is part of the syntax.
> 
>  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 507029404f0b..faf3930ace04 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
> -
>    # We populate DXE IPL tables with 1G pages preferably on Xen
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
>  
> @@ -471,6 +468,7 @@ [PcdsDynamicDefault]
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size|0x800000000
>  
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0
>  
>    # Set video resolution for text setup.
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 5732d2188871..87dd4b24679a 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -85,6 +85,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 7524aaa11a29..a29b4e04086e 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -632,5 +632,9 @@ CalibrateLapicTimer (
>    Freq = DivU64x64Remainder (Dividend, TscTick2 - TscTick, NULL);
>    DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
>  
> +  ASSERT (Freq <= MAX_UINT32);
> +  Status = PcdSet32S (PcdFSBClock, Freq);

(2) Please use an explicit cast here: (UINT32)Freq; I'm concerned about
VS emitting a warning (despite the ASSERT()).

> +  ASSERT_RETURN_ERROR (Status);

(3) "Status" has type EFI_STATUS here; the assignment from PcdSet32S()
(RETURN_STATUS) is fine, but it's more idiomatic to use
ASSERT_EFI_ERROR(), given the type of "Status".

> +
>    UnmapXenPage (SharedInfo);
>  }
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo



  reply	other threads:[~2021-04-07  9:26 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-25 15:47 [PATCH v2 0/7] OvmfXen: Set PcdFSBClock at runtime Anthony PERARD
2021-03-25 15:47 ` [PATCH v2 1/7] OvmfPkg/XenResetVector: Silent a warning from nasm Anthony PERARD
2021-03-25 15:47 ` [PATCH v2 2/7] MdePkg: Allow PcdFSBClock to by Dynamic Anthony PERARD
2021-03-25 15:47 ` [PATCH v2 3/7] OvmfPkg/IndustryStandard/Xen: Apply EDK2 coding style to XEN_VCPU_TIME_INFO Anthony PERARD
2021-03-25 15:47 ` [PATCH v2 4/7] OvmfPkg/IndustryStandard: Introduce PageTable.h Anthony PERARD
2021-03-26 14:16   ` Tom Lendacky
2021-04-07  8:01     ` [edk2-devel] " Laszlo Ersek
2021-04-07  8:02   ` Laszlo Ersek
2021-04-07  8:04   ` Laszlo Ersek
2021-03-25 15:47 ` [PATCH v2 5/7] OvmfPkg/XenPlatformPei: Map extra physical address Anthony PERARD
2021-04-07  8:06   ` [edk2-devel] " Laszlo Ersek
2021-03-25 15:47 ` [PATCH v2 6/7] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency Anthony PERARD
2021-04-07  8:28   ` [edk2-devel] " Laszlo Ersek
2021-03-25 15:47 ` [PATCH v2 7/7] OvmfPkg/OvmfXen: Set PcdFSBClock Anthony PERARD
2021-04-07  9:25   ` Laszlo Ersek [this message]
2021-03-25 18:22 ` [PATCH v2 0/7] OvmfXen: Set PcdFSBClock at runtime Laszlo Ersek
2021-04-07  9:32 ` [edk2-devel] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=344af4b2-0ce4-23a9-7fde-bf1ee0343d27@redhat.com \
    --to=lersek@redhat.com \
    --cc=anthony.perard@citrix.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=devel@edk2.groups.io \
    --cc=jordan.l.justen@intel.com \
    --cc=julien@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [edk2-devel] [PATCH v2 7/7] OvmfPkg/OvmfXen: Set PcdFSBClock' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).