[PATCH 1/1] OvmfPkg: fix PcdFSBClock

[PATCH 1/1] OvmfPkg: fix PcdFSBClock

[Gerd Hoffmann]

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")
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(-)

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

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

[Gerd Hoffmann]

On Mon, May 23, 2022 at 03:45:04PM +0200, 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")

Cc'ing gaoliming.

This fixes a regression and should go into 220205

thanks,
  Gerd

回复: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock

[gaoliming]

Gerd:
  I am OK to merge this bug fix for stable tag 202205 once this patch passes
code review. 

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Gerd
> Hoffmann
> 发送时间: 2022年5月24日 14:29
> 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn
> 抄送: Pawel Polawski <ppolawsk@redhat.com>; Oliver Steffen
> <osteffen@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> 主题: Re: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock
> 
> On Mon, May 23, 2022 at 03:45:04PM +0200, 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")
> 
> Cc'ing gaoliming.
> 
> This fixes a regression and should go into 220205
> 
> thanks,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock

[Yao, Jiewen]

Agree to merge to stable tag. Thanks for the reminder.

Reviewed-by: Jiewen Yao@intel.com

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> Sent: Wednesday, May 25, 2022 9:43 AM
> To: devel@edk2.groups.io; kraxel@redhat.com
> Cc: 'Pawel Polawski' <ppolawsk@redhat.com>; 'Oliver Steffen'
> <osteffen@redhat.com>; 'Laszlo Ersek' <lersek@redhat.com>
> Subject: 回复: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock
> 
> Gerd:
>   I am OK to merge this bug fix for stable tag 202205 once this patch passes
> code review.
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Gerd
> > Hoffmann
> > 发送时间: 2022年5月24日 14:29
> > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn
> > 抄送: Pawel Polawski <ppolawsk@redhat.com>; Oliver Steffen
> > <osteffen@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> > 主题: Re: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock
> >
> > On Mon, May 23, 2022 at 03:45:04PM +0200, 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")
> >
> > Cc'ing gaoliming.
> >
> > This fixes a regression and should go into 220205
> >
> > thanks,
> >   Gerd
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock

[Yao, Jiewen]

Merged https://github.com/tianocore/edk2/pull/2916

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, May 25, 2022 11:00 AM
> To: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>;
> kraxel@redhat.com
> Cc: 'Pawel Polawski' <ppolawsk@redhat.com>; 'Oliver Steffen'
> <osteffen@redhat.com>; 'Laszlo Ersek' <lersek@redhat.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock
> 
> Agree to merge to stable tag. Thanks for the reminder.
> 
> Reviewed-by: Jiewen Yao@intel.com
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> > Sent: Wednesday, May 25, 2022 9:43 AM
> > To: devel@edk2.groups.io; kraxel@redhat.com
> > Cc: 'Pawel Polawski' <ppolawsk@redhat.com>; 'Oliver Steffen'
> > <osteffen@redhat.com>; 'Laszlo Ersek' <lersek@redhat.com>
> > Subject: 回复: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock
> >
> > Gerd:
> >   I am OK to merge this bug fix for stable tag 202205 once this patch passes
> > code review.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Gerd
> > > Hoffmann
> > > 发送时间: 2022年5月24日 14:29
> > > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn
> > > 抄送: Pawel Polawski <ppolawsk@redhat.com>; Oliver Steffen
> > > <osteffen@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> > > 主题: Re: [edk2-devel] [PATCH 1/1] OvmfPkg: fix PcdFSBClock
> > >
> > > On Mon, May 23, 2022 at 03:45:04PM +0200, 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")
> > >
> > > Cc'ing gaoliming.
> > >
> > > This fixes a regression and should go into 220205
> > >
> > > thanks,
> > >   Gerd
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> > 
> >

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

[Laszlo Ersek]

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

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

[Gerd Hoffmann]

  Hi,

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

I know ;)

Did the same a while back, looking for a way to figure at runtime
what the frequency is (simliar to xen), only to find that it is not
needed on kvm because frequency is fixed.

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

Never noticed a difference.  Unlikely I'd say.  Maybe kvm uses 1GHz
because tcg does after all.  Didn't check the code though.

take care,
  Gerd