Re: [PATCH 1/1] OvmfPkg: fix PcdFSBClock

["Laszlo Ersek" <lersek@redhat.com>]

On 05/23/22 15:45, Gerd Hoffmann wrote:
> kvm FSB clock is 1GHz, not 100 MHz.  Timings are off by factor 10.
> Fix all affected build configurations.  Not changed: Microvm and
> Cloudhw (they have already have the correct value), and Xen (has
> no fixed frequency, the PCD is configured at runtime by platform
> initialization code).
>
> Fixes: c37cbc030d96 ("OvmfPkg: Switch timer in build time for OvmfPkg")

Consider adding "Fixes: 44a53a3bdd9c7" too, on a separate line; commit
44a53a3bdd9c ("OvmfPkg: Introduce IntelTdxX64 for TDVF Config-B",
2022-04-02) copied the wrong setting when creating
"OvmfPkg/IntelTdx/IntelTdxX64.dsc".

(Obviously it needs no repost.)

> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc     | 2 +-
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc | 2 +-
>  OvmfPkg/OvmfPkgIa32.dsc          | 2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc       | 2 +-
>  OvmfPkg/OvmfPkgX64.dsc           | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)

Using "OvmfPkg/OvmfPkgX64.dsc":

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

By the way: "kvm FSB clock is 1GHz" -- where is that constant set in
KVM?

... is it "target/i386/kvm/kvm.c" in QEMU:

> /* From arch/x86/kvm/lapic.h */
> #define KVM_APIC_BUS_CYCLE_NS       1
> #define KVM_APIC_BUS_FREQUENCY      (1000000000ULL / KVM_APIC_BUS_CYCLE_NS)

FWIW, APIC_BUS_CYCLE_NS=1 goes back to historical KVM commit
97222cc83163 ("KVM: Emulate local APIC in kernel", 2007-10-13). The
commit does not say anything about this particular choice. (Maybe I
should look at the QEMU source from 2007 -- perhaps the KVM commit was
inspired by QEMU practice back then. And now we've come full circle: the
definitive constant lives in the kernel, which is where QEMU is taking
it from...)

... I think these macros are pretty difficult to find, unless one knows
already what they're looking for!

BTW is there a chance that TCG uses a different frequency?

Thanks!
Laszlo

>
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index bead9722eab8..fc1fdb2e2297 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -578,7 +578,7 @@ [PcdsDynamicDefault]
>
>  !include OvmfPkg/OvmfTpmPcds.dsc.inc
>
> -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|1000000000
>
>  [PcdsDynamicHii]
>  !include OvmfPkg/OvmfTpmPcdsHii.dsc.inc
> diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> index 00bc1255bc4e..dd8d446f4a56 100644
> --- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> +++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> @@ -511,7 +511,7 @@ [PcdsDynamicDefault]
>    # Set ConfidentialComputing defaults
>    gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0
>
> -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|1000000000
>
>  ################################################################################
>  #
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index c16a840fff16..a9841cbfc3ca 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -651,7 +651,7 @@ [PcdsDynamicDefault]
>    gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0
>
>  !if $(CSM_ENABLE) == FALSE
> -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|1000000000
>  !endif
>
>  [PcdsDynamicHii]
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index d3a80cb56892..f7949780fa38 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -659,7 +659,7 @@ [PcdsDynamicDefault]
>    gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0
>
>  !if $(CSM_ENABLE) == FALSE
> -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|1000000000
>  !endif
>
>  [PcdsDynamicDefault.X64]
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 7b3d48aac430..1448f925b782 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -684,7 +684,7 @@ [PcdsDynamicDefault]
>    gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0
>
>  !if $(CSM_ENABLE) == FALSE
> -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|100000000
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|1000000000
>  !endif
>
>  [PcdsDynamicHii]
>