[PATCH 0/5] OvmfXen: Set PcdFSBClock at runtime

[PATCH 0/5] OvmfXen: Set PcdFSBClock at runtime

[Anthony PERARD]

Patch series available in this git branch:
git://xenbits.xen.org/people/aperard/ovmf.git br.apic-timer-freq-v1

Hi,

OvmfXen uses the APIC timer, but with an hard-coded frequency that may change
as pointed out here:
  https://edk2.groups.io/g/devel/message/45185
  <20190808134423.ybqg3qkpw5ucfzk4@Air-de-Roger>

This series changes that so the frequency is calculated at runtime.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490

There is also one cleanup patch that has nothing to do with the rest.

Cheers,

Anthony PERARD (5):
  OvmfPkg/XenResetVector: Silent a warning from nasm
  MdePkg: Allow PcdFSBClock to by Dynamic
  OvmfPkg/IndustryStandard/Xen: Apply EDK2 coding style to
    XEN_VCPU_TIME_INFO
  OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency
  OvmfPkg/OvmfXen: Set PcdFSBClock

 MdePkg/MdePkg.dec                          |   8 +-
 OvmfPkg/OvmfXen.dsc                        |   4 +-
 OvmfPkg/XenPlatformPei/XenPlatformPei.inf  |   2 +
 OvmfPkg/Include/IndustryStandard/Xen/xen.h |  17 +--
 OvmfPkg/XenPlatformPei/Platform.h          |   5 +
 OvmfPkg/XenPlatformPei/Platform.c          |   1 +
 OvmfPkg/XenPlatformPei/Xen.c               | 127 +++++++++++++++++++++
 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm |   2 +-
 8 files changed, 150 insertions(+), 16 deletions(-)

-- 
Anthony PERARD

[PATCH 1/5] OvmfPkg/XenResetVector: Silent a warning from nasm

[Anthony PERARD]

To avoid nasm generating a warning, replace the macro by the value
expected to be stored in eax.
  Ia32/XenPVHMain.asm:76: warning: dword data exceeds bounds

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
index 2df0f12e18cb..c761e9d30729 100644
--- a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
+++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
@@ -73,7 +73,7 @@ xenPVHMain:
     ;
     ; parameter for Flat32SearchForBfvBase
     ;
-    mov     eax, ADDR_OF(fourGigabytes)
+    mov     eax, 0   ; ADDR_OF(fourGigabytes)
     add     eax, edx ; add delta
 
     ;
-- 
Anthony PERARD

Re: [PATCH 1/5] OvmfPkg/XenResetVector: Silent a warning from nasm

[Laszlo Ersek]

On 01/29/20 13:12, Anthony PERARD wrote:
> To avoid nasm generating a warning, replace the macro by the value
> expected to be stored in eax.
>   Ia32/XenPVHMain.asm:76: warning: dword data exceeds bounds
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> index 2df0f12e18cb..c761e9d30729 100644
> --- a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> @@ -73,7 +73,7 @@ xenPVHMain:
>      ;
>      ; parameter for Flat32SearchForBfvBase
>      ;
> -    mov     eax, ADDR_OF(fourGigabytes)
> +    mov     eax, 0   ; ADDR_OF(fourGigabytes)
>      add     eax, edx ; add delta
>  
>      ;
> 

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

[PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic

[Anthony PERARD]

We are going to want to change the value of PcdFSBClock at run time in
OvmfXen, so move it to the PcdsDynamic section.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
CC: Bob Feng <bob.c.feng@intel.com>
CC: Liming Gao <liming.gao@intel.com>
---
 MdePkg/MdePkg.dec | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index d022cc5e3ef2..8f5a48346e50 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2194,10 +2194,6 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   # @ValidList  0x80000001 | 8, 16, 32
   gEfiMdePkgTokenSpaceGuid.PcdPort80DataWidth|8|UINT8|0x0000002d
 
-  ## This value is used to configure X86 Processor FSB clock.
-  # @Prompt FSB Clock.
-  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
-
   ## The maximum printable number of characters. UefLib functions: AsciiPrint(), AsciiErrorPrint(),
   #  PrintXY(), AsciiPrintXY(), Print(), ErrorPrint() base on this PCD value to print characters.
   # @Prompt Maximum Printable Number of Characters.
@@ -2297,5 +2293,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   # @Prompt Boot Timeout (s)
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
 
+  ## This value is used to configure X86 Processor FSB clock.
+  # @Prompt FSB Clock.
+  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
+
 [UserExtensions.TianoCore."ExtraFiles"]
   MdePkgExtra.uni
-- 
Anthony PERARD

Re: [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic

[Laszlo Ersek]

On 01/29/20 13:12, Anthony PERARD wrote:
> We are going to want to change the value of PcdFSBClock at run time in
> OvmfXen, so move it to the PcdsDynamic section.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> CC: Bob Feng <bob.c.feng@intel.com>
> CC: Liming Gao <liming.gao@intel.com>
> ---
>  MdePkg/MdePkg.dec | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index d022cc5e3ef2..8f5a48346e50 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2194,10 +2194,6 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>    # @ValidList  0x80000001 | 8, 16, 32
>    gEfiMdePkgTokenSpaceGuid.PcdPort80DataWidth|8|UINT8|0x0000002d
>  
> -  ## This value is used to configure X86 Processor FSB clock.
> -  # @Prompt FSB Clock.
> -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> -
>    ## The maximum printable number of characters. UefLib functions: AsciiPrint(), AsciiErrorPrint(),
>    #  PrintXY(), AsciiPrintXY(), Print(), ErrorPrint() base on this PCD value to print characters.
>    # @Prompt Maximum Printable Number of Characters.
> @@ -2297,5 +2293,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    # @Prompt Boot Timeout (s)
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
>  
> +  ## This value is used to configure X86 Processor FSB clock.
> +  # @Prompt FSB Clock.
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> +
>  [UserExtensions.TianoCore."ExtraFiles"]
>    MdePkgExtra.uni
> 

Looks good to me:

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

Mike or Liming will have to ACK.

Thanks!
Laszlo

Re: [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic

[Liming Gao]

Anthony:
  This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take some time and cause the inaccurate time delay. Have you measured its impact?

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 30, 2020 12:11 AM
> To: Anthony PERARD <anthony.perard@citrix.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; xen-devel@lists.xenproject.org;
> Gao, Liming <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Feng, Bob C
> <bob.c.feng@intel.com>
> Subject: Re: [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
> 
> On 01/29/20 13:12, Anthony PERARD wrote:
> > We are going to want to change the value of PcdFSBClock at run time in
> > OvmfXen, so move it to the PcdsDynamic section.
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > CC: Bob Feng <bob.c.feng@intel.com>
> > CC: Liming Gao <liming.gao@intel.com>
> > ---
> >  MdePkg/MdePkg.dec | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index d022cc5e3ef2..8f5a48346e50 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -2194,10 +2194,6 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
> >    # @ValidList  0x80000001 | 8, 16, 32
> >    gEfiMdePkgTokenSpaceGuid.PcdPort80DataWidth|8|UINT8|0x0000002d
> >
> > -  ## This value is used to configure X86 Processor FSB clock.
> > -  # @Prompt FSB Clock.
> > -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> > -
> >    ## The maximum printable number of characters. UefLib functions: AsciiPrint(), AsciiErrorPrint(),
> >    #  PrintXY(), AsciiPrintXY(), Print(), ErrorPrint() base on this PCD value to print characters.
> >    # @Prompt Maximum Printable Number of Characters.
> > @@ -2297,5 +2293,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> >    # @Prompt Boot Timeout (s)
> >    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
> >
> > +  ## This value is used to configure X86 Processor FSB clock.
> > +  # @Prompt FSB Clock.
> > +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> > +
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    MdePkgExtra.uni
> >
> 
> Looks good to me:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Mike or Liming will have to ACK.
> 
> Thanks!
> Laszlo

Re: [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic

[Anthony PERARD]

On Mon, Feb 03, 2020 at 01:34:55AM +0000, Gao, Liming wrote:
> Anthony:
>   This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take some time and cause the inaccurate time delay. Have you measured its impact?

No, I haven't. But I don't think it matter in a Xen guest, the APIC timer is
emulated anyway, so reading from a register of the APIC is going to be
slower than getting the value from the PCD services, I think.
(Hopefully, I'm not too wrong.)

But I'll give it at measuring the difference, it would be interesting to
know.

Thanks,

-- 
Anthony PERARD

Re: [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic

[Anthony PERARD]

On Mon, Feb 03, 2020 at 03:34:07PM +0000, Anthony PERARD wrote:
> On Mon, Feb 03, 2020 at 01:34:55AM +0000, Gao, Liming wrote:
> > Anthony:
> >   This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take some time and cause the inaccurate time delay. Have you measured its impact?
> 
> No, I haven't. But I don't think it matter in a Xen guest, the APIC timer is
> emulated anyway, so reading from a register of the APIC is going to be
> slower than getting the value from the PCD services, I think.
> (Hopefully, I'm not too wrong.)
> 
> But I'll give it at measuring the difference, it would be interesting to
> know.

Now that I've given a try, having the value as Dynamic doesn't change
anything in a Xen guest.

On my test machine, simply running GetPerformanceCounter (); takes
between 10000 ns and 20000 ns. Reading the dynamic value from PCD on the
other hand takes about 350ns. (10ns if it's static.)

When I run NanoSecondDelay() with different values, I have:
  - with static pcd:
           63894 ns to delay by 1 ns
           66611 ns to delay by 10 ns
           43927 ns to delay by 100 ns
           71367 ns to delay by 1000 ns
           55881 ns to delay by 10000 ns
          147716 ns to delay by 100000 ns
         1048335 ns to delay by 1000000 ns
        10041179 ns to delay by 10000000 ns
  - with a dynamic pcd:
           40949 ns to delay by 1 ns
           84832 ns to delay by 10 ns
           82745 ns to delay by 100 ns
           59848 ns to delay by 1000 ns
           52647 ns to delay by 10000 ns
          137051 ns to delay by 100000 ns
         1042492 ns to delay by 1000000 ns
        10036306 ns to delay by 10000000 ns

So, the kind of PCD used for PcdFSBClock on Xen (with OvmfXen) doesn't
really matter.

Anyway, thanks for the feedback.

-- 
Anthony PERARD

Re: [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic

[Liming Gao]

Thanks for your data. Seemly, those data is acceptable on OvmfXen. For this patch, Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Anthony PERARD
> Sent: Tuesday, February 4, 2020 1:26 AM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; xen-devel@lists.xenproject.org; Justen, Jordan L <jordan.l.justen@intel.com>; Julien Grall
> <julien@xen.org>; Feng, Bob C <bob.c.feng@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
> 
> On Mon, Feb 03, 2020 at 03:34:07PM +0000, Anthony PERARD wrote:
> > On Mon, Feb 03, 2020 at 01:34:55AM +0000, Gao, Liming wrote:
> > > Anthony:
> > >   This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take
> some time and cause the inaccurate time delay. Have you measured its impact?
> >
> > No, I haven't. But I don't think it matter in a Xen guest, the APIC timer is
> > emulated anyway, so reading from a register of the APIC is going to be
> > slower than getting the value from the PCD services, I think.
> > (Hopefully, I'm not too wrong.)
> >
> > But I'll give it at measuring the difference, it would be interesting to
> > know.
> 
> Now that I've given a try, having the value as Dynamic doesn't change
> anything in a Xen guest.
> 
> On my test machine, simply running GetPerformanceCounter (); takes
> between 10000 ns and 20000 ns. Reading the dynamic value from PCD on the
> other hand takes about 350ns. (10ns if it's static.)
> 
> When I run NanoSecondDelay() with different values, I have:
>   - with static pcd:
>            63894 ns to delay by 1 ns
>            66611 ns to delay by 10 ns
>            43927 ns to delay by 100 ns
>            71367 ns to delay by 1000 ns
>            55881 ns to delay by 10000 ns
>           147716 ns to delay by 100000 ns
>          1048335 ns to delay by 1000000 ns
>         10041179 ns to delay by 10000000 ns
>   - with a dynamic pcd:
>            40949 ns to delay by 1 ns
>            84832 ns to delay by 10 ns
>            82745 ns to delay by 100 ns
>            59848 ns to delay by 1000 ns
>            52647 ns to delay by 10000 ns
>           137051 ns to delay by 100000 ns
>          1042492 ns to delay by 1000000 ns
>         10036306 ns to delay by 10000000 ns
> 
> So, the kind of PCD used for PcdFSBClock on Xen (with OvmfXen) doesn't
> really matter.
> 
> Anyway, thanks for the feedback.
> 
> --
> Anthony PERARD
> 
> 

[PATCH 3/5] OvmfPkg/IndustryStandard/Xen: Apply EDK2 coding style to XEN_VCPU_TIME_INFO

[Anthony PERARD]

We are going to use new fields from the Xen headers. Apply the EDK2
coding style so that the code that is going to use it doesn't look out
of place.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 OvmfPkg/Include/IndustryStandard/Xen/xen.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/Include/IndustryStandard/Xen/xen.h b/OvmfPkg/Include/IndustryStandard/Xen/xen.h
index e55d93263285..ac9155089902 100644
--- a/OvmfPkg/Include/IndustryStandard/Xen/xen.h
+++ b/OvmfPkg/Include/IndustryStandard/Xen/xen.h
@@ -183,10 +183,10 @@ struct vcpu_time_info {
      * The correct way to interact with the version number is similar to
      * Linux's seqlock: see the implementations of read_seqbegin/read_seqretry.
      */
-    UINT32 version;
+    UINT32 Version;
     UINT32 pad0;
-    UINT64 tsc_timestamp;   /* TSC at last update of time vals.  */
-    UINT64 system_time;     /* Time, in nanosecs, since boot.    */
+    UINT64 TSCTimestamp;   /* TSC at last update of time vals.  */
+    UINT64 SystemTime;     /* Time, in nanosecs, since boot.    */
     /*
      * Current system time:
      *   system_time +
@@ -194,11 +194,11 @@ struct vcpu_time_info {
      * CPU frequency (Hz):
      *   ((10^9 << 32) / tsc_to_system_mul) >> tsc_shift
      */
-    UINT32 tsc_to_system_mul;
-    INT8   tsc_shift;
+    UINT32 TSCToSystemMultiplier;
+    INT8   TSCShift;
     INT8   pad1[3];
 }; /* 32 bytes */
-typedef struct vcpu_time_info vcpu_time_info_t;
+typedef struct vcpu_time_info XEN_VCPU_TIME_INFO;
 
 struct vcpu_info {
     /*
@@ -234,7 +234,7 @@ struct vcpu_info {
 #endif /* XEN_HAVE_PV_UPCALL_MASK */
     xen_ulong_t evtchn_pending_sel;
     struct arch_vcpu_info arch;
-    struct vcpu_time_info time;
+    struct vcpu_time_info Time;
 }; /* 64 bytes (x86) */
 #ifndef __XEN__
 typedef struct vcpu_info vcpu_info_t;
@@ -250,7 +250,7 @@ typedef struct vcpu_info vcpu_info_t;
  * of this structure remaining constant.
  */
 struct shared_info {
-    struct vcpu_info vcpu_info[XEN_LEGACY_MAX_VCPUS];
+    struct vcpu_info VcpuInfo[XEN_LEGACY_MAX_VCPUS];
 
     /*
      * A domain can create "event channels" on which it can send and receive
@@ -299,6 +299,7 @@ struct shared_info {
 };
 #ifndef __XEN__
 typedef struct shared_info shared_info_t;
+typedef struct shared_info XEN_SHARED_INFO;
 #endif
 
 /* Turn a plain number into a C UINTN constant. */
-- 
Anthony PERARD

Re: [PATCH 3/5] OvmfPkg/IndustryStandard/Xen: Apply EDK2 coding style to XEN_VCPU_TIME_INFO

[Laszlo Ersek]

On 01/29/20 13:12, Anthony PERARD wrote:
> We are going to use new fields from the Xen headers. Apply the EDK2
> coding style so that the code that is going to use it doesn't look out
> of place.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/Include/IndustryStandard/Xen/xen.h | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

This is highly appreciated. Comments below:

> 
> diff --git a/OvmfPkg/Include/IndustryStandard/Xen/xen.h b/OvmfPkg/Include/IndustryStandard/Xen/xen.h
> index e55d93263285..ac9155089902 100644
> --- a/OvmfPkg/Include/IndustryStandard/Xen/xen.h
> +++ b/OvmfPkg/Include/IndustryStandard/Xen/xen.h
> @@ -183,10 +183,10 @@ struct vcpu_time_info {
>       * The correct way to interact with the version number is similar to
>       * Linux's seqlock: see the implementations of read_seqbegin/read_seqretry.
>       */
> -    UINT32 version;
> +    UINT32 Version;
>      UINT32 pad0;
> -    UINT64 tsc_timestamp;   /* TSC at last update of time vals.  */
> -    UINT64 system_time;     /* Time, in nanosecs, since boot.    */
> +    UINT64 TSCTimestamp;   /* TSC at last update of time vals.  */

(1) Should be "TscTimestamp". Acronyms are de-capitalized when composed
into longer identifiers, to maintain a consistent CamelCase.

> +    UINT64 SystemTime;     /* Time, in nanosecs, since boot.    */
>      /*
>       * Current system time:
>       *   system_time +
> @@ -194,11 +194,11 @@ struct vcpu_time_info {
>       * CPU frequency (Hz):
>       *   ((10^9 << 32) / tsc_to_system_mul) >> tsc_shift
>       */
> -    UINT32 tsc_to_system_mul;
> -    INT8   tsc_shift;
> +    UINT32 TSCToSystemMultiplier;
> +    INT8   TSCShift;

(2) Ditto (both fields).

>      INT8   pad1[3];
>  }; /* 32 bytes */
> -typedef struct vcpu_time_info vcpu_time_info_t;
> +typedef struct vcpu_time_info XEN_VCPU_TIME_INFO;
>  
>  struct vcpu_info {
>      /*
> @@ -234,7 +234,7 @@ struct vcpu_info {
>  #endif /* XEN_HAVE_PV_UPCALL_MASK */
>      xen_ulong_t evtchn_pending_sel;
>      struct arch_vcpu_info arch;
> -    struct vcpu_time_info time;
> +    struct vcpu_time_info Time;
>  }; /* 64 bytes (x86) */
>  #ifndef __XEN__
>  typedef struct vcpu_info vcpu_info_t;
> @@ -250,7 +250,7 @@ typedef struct vcpu_info vcpu_info_t;
>   * of this structure remaining constant.
>   */
>  struct shared_info {
> -    struct vcpu_info vcpu_info[XEN_LEGACY_MAX_VCPUS];
> +    struct vcpu_info VcpuInfo[XEN_LEGACY_MAX_VCPUS];

Yes, this is a good example. "Vcpu" and not "VCPU" or "VCpu".

>  
>      /*
>       * A domain can create "event channels" on which it can send and receive
> @@ -299,6 +299,7 @@ struct shared_info {
>  };
>  #ifndef __XEN__
>  typedef struct shared_info shared_info_t;
> +typedef struct shared_info XEN_SHARED_INFO;
>  #endif
>  
>  /* Turn a plain number into a C UINTN constant. */
> 

Assuming the OVMF platforms continue to build at this stage into the
series, and provided that (1) and (2) are fixed:

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

Re: [PATCH 3/5] OvmfPkg/IndustryStandard/Xen: Apply EDK2 coding style to XEN_VCPU_TIME_INFO

[Anthony PERARD]

On Wed, Jan 29, 2020 at 05:14:35PM +0100, Laszlo Ersek wrote:
> On 01/29/20 13:12, Anthony PERARD wrote:
> Assuming the OVMF platforms continue to build at this stage into the
> series, and provided that (1) and (2) are fixed:

I'll fix (1) and (2).

I've build tests both OvmfXen and OvmfPkgX64, and I've grep for some of
those field, and the struct name, so I think ArmVirtXen will also
continue to build.

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

Thanks,

-- 
Anthony PERARD

[PATCH 4/5] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

[Anthony PERARD]

Calculate the frequency of the APIC timer that Xen provides.

Even though the frequency is currently hard-coded, it isn't part of
the public ABI that Xen provides and thus may change at any time. OVMF
needs to determine the frequency by an other mean.

Fortunately, Xen provides a way to determines the frequency of the
TSC, so we can use TSC to calibrate the frequency of the APIC timer.
That information is found in the shared_info page which we map and
unmap once done (XenBusDxe is going to map the page somewhere else).

The calculated frequency is only logged in this patch, it will be used
in a following patch.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 OvmfPkg/XenPlatformPei/XenPlatformPei.inf |   1 +
 OvmfPkg/XenPlatformPei/Platform.h         |   5 +
 OvmfPkg/XenPlatformPei/Platform.c         |   1 +
 OvmfPkg/XenPlatformPei/Xen.c              | 123 ++++++++++++++++++++++
 4 files changed, 130 insertions(+)

diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
index 0ef77db92c03..335a442538c2 100644
--- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
+++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
   DebugLib
   HobLib
   IoLib
+  LocalApicLib
   PciLib
   ResourcePublicationLib
   PeiServicesLib
diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
index 7661f4a8de0a..97e482a065f0 100644
--- a/OvmfPkg/XenPlatformPei/Platform.h
+++ b/OvmfPkg/XenPlatformPei/Platform.h
@@ -127,6 +127,11 @@ XenGetE820Map (
   UINT32 *Count
   );
 
+VOID
+CalibrateLapicTimer (
+  VOID
+  );
+
 extern EFI_BOOT_MODE mBootMode;
 
 extern UINT8 mPhysMemAddressWidth;
diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
index 717fd0ab1a45..e9511eb40c62 100644
--- a/OvmfPkg/XenPlatformPei/Platform.c
+++ b/OvmfPkg/XenPlatformPei/Platform.c
@@ -448,6 +448,7 @@ InitializeXenPlatform (
   InitializeRamRegions ();
 
   InitializeXen ();
+  CalibrateLapicTimer ();
 
   if (mBootMode != BOOT_ON_S3_RESUME) {
     ReserveEmuVariableNvStore ();
diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index c41fecdc486e..d6cdc9a8e31c 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -19,6 +19,7 @@
 //
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
+#include <Library/LocalApicLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PcdLib.h>
 #include <Guid/XenInfo.h>
@@ -386,3 +387,125 @@ InitializeXen (
 
   return EFI_SUCCESS;
 }
+
+
+EFI_STATUS
+MapSharedInfoPage (
+  IN VOID *PagePtr
+  )
+{
+  xen_add_to_physmap_t  Parameters;
+  INTN                  ReturnCode;
+
+  Parameters.domid = DOMID_SELF;
+  Parameters.space = XENMAPSPACE_shared_info;
+  Parameters.idx = 0;
+  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;
+  ReturnCode = XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameters);
+  if (ReturnCode != 0) {
+    return EFI_NO_MAPPING;
+  }
+  return EFI_SUCCESS;
+}
+
+VOID
+UnmapXenPage (
+  IN VOID *PagePtr
+  )
+{
+  xen_remove_from_physmap_t Parameters;
+  INTN                      ReturnCode;
+
+  Parameters.domid = DOMID_SELF;
+  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;
+  ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);
+  ASSERT (ReturnCode == 0);
+}
+
+
+STATIC
+UINT64
+GetCPUFreq (
+  IN XEN_VCPU_TIME_INFO *VcpuTime
+  )
+{
+  UINT32 Version;
+  UINT32 TSCToSystemMultiplier;
+  INT8   TSCShift;
+  UINT64 CPUFreq;
+
+  do {
+    Version = VcpuTime->Version;
+    MemoryFence ();
+    TSCToSystemMultiplier = VcpuTime->TSCToSystemMultiplier;
+    TSCShift = VcpuTime->TSCShift;
+    MemoryFence ();
+  } while (((Version & 1) != 0) && (Version != VcpuTime->Version));
+
+  CPUFreq = (1000000000ULL << 32) / TSCToSystemMultiplier;
+  if (TSCShift >= 0) {
+      CPUFreq >>= VcpuTime->TSCShift;
+  } else {
+      CPUFreq <<= -VcpuTime->TSCShift;
+  }
+  return CPUFreq;
+}
+
+VOID
+XenDelay (
+  IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
+  IN UINT64             DelayNs
+  )
+{
+  UINT64 Tick;
+
+  Tick = AsmReadTsc ();
+  Tick += (DelayNs * GetCPUFreq (VcpuTimeInfo)) / 1000000000ULL;
+  while (AsmReadTsc() <= Tick) {
+    CpuPause();
+  }
+}
+
+
+/**
+  Calculate the frequency of the Local Apic Timer
+**/
+VOID
+CalibrateLapicTimer (
+  VOID
+  )
+{
+  XEN_SHARED_INFO       *SharedInfo;
+  XEN_VCPU_TIME_INFO    *VcpuTimeInfo;
+  UINT32                TimerTick, TimerTick2;
+  UINT64                TscTick, TscTick2;
+  UINT64                Freq;
+  EFI_STATUS            Status;
+
+  SharedInfo = AllocatePages (1);
+  Status = MapSharedInfoPage (SharedInfo);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    goto Exit;
+  }
+
+  VcpuTimeInfo = &SharedInfo->VcpuInfo[0].Time;
+
+  InitializeApicTimer (1, MAX_UINT32, TRUE, 0);
+  DisableApicTimerInterrupt ();
+
+  TimerTick = GetApicTimerCurrentCount ();
+  TscTick = AsmReadTsc ();
+  XenDelay (VcpuTimeInfo, 1000000ULL);
+  TimerTick2 = GetApicTimerCurrentCount ();
+  TscTick2 = AsmReadTsc ();
+
+  Freq = (GetCPUFreq (VcpuTimeInfo) * (TimerTick - TimerTick2))
+    / (TscTick2 - TscTick);
+  DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
+
+  UnmapXenPage (SharedInfo);
+
+Exit:
+  FreePages (SharedInfo, 1);
+}
-- 
Anthony PERARD

Re: [PATCH 4/5] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

[Laszlo Ersek]

On 01/29/20 13:12, Anthony PERARD wrote:
> Calculate the frequency of the APIC timer that Xen provides.
> 
> Even though the frequency is currently hard-coded, it isn't part of
> the public ABI that Xen provides and thus may change at any time. OVMF
> needs to determine the frequency by an other mean.
> 
> Fortunately, Xen provides a way to determines the frequency of the
> TSC, so we can use TSC to calibrate the frequency of the APIC timer.
> That information is found in the shared_info page which we map and
> unmap once done (XenBusDxe is going to map the page somewhere else).
> 
> The calculated frequency is only logged in this patch, it will be used
> in a following patch.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |   1 +
>  OvmfPkg/XenPlatformPei/Platform.h         |   5 +
>  OvmfPkg/XenPlatformPei/Platform.c         |   1 +
>  OvmfPkg/XenPlatformPei/Xen.c              | 123 ++++++++++++++++++++++
>  4 files changed, 130 insertions(+)

I'll review this superficially; it should be approved by someone from
xen-devel:

> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 0ef77db92c03..335a442538c2 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
>    DebugLib
>    HobLib
>    IoLib
> +  LocalApicLib
>    PciLib
>    ResourcePublicationLib
>    PeiServicesLib
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index 7661f4a8de0a..97e482a065f0 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -127,6 +127,11 @@ XenGetE820Map (
>    UINT32 *Count
>    );
>  
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  );
> +
>  extern EFI_BOOT_MODE mBootMode;
>  
>  extern UINT8 mPhysMemAddressWidth;
> diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
> index 717fd0ab1a45..e9511eb40c62 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.c
> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> @@ -448,6 +448,7 @@ InitializeXenPlatform (
>    InitializeRamRegions ();
>  
>    InitializeXen ();
> +  CalibrateLapicTimer ();
>  
>    if (mBootMode != BOOT_ON_S3_RESUME) {
>      ReserveEmuVariableNvStore ();
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index c41fecdc486e..d6cdc9a8e31c 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -19,6 +19,7 @@
>  //
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
> +#include <Library/LocalApicLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
>  #include <Guid/XenInfo.h>
> @@ -386,3 +387,125 @@ InitializeXen (
>  
>    return EFI_SUCCESS;
>  }
> +
> +
> +EFI_STATUS
> +MapSharedInfoPage (
> +  IN VOID *PagePtr
> +  )
> +{
> +  xen_add_to_physmap_t  Parameters;
> +  INTN                  ReturnCode;
> +
> +  Parameters.domid = DOMID_SELF;
> +  Parameters.space = XENMAPSPACE_shared_info;
> +  Parameters.idx = 0;
> +  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;

(1) Please remove the space character from "(UINTN) PagePtr". Inserting
a space character is a bad and confusing habit in edk2, because it masks
the fact that the cast operator has one of the strongest bindings in the
C language. So I try to keep it out of OvmfPkg and ArmVirtPkg.

> +  ReturnCode = XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameters);
> +  if (ReturnCode != 0) {
> +    return EFI_NO_MAPPING;
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +VOID
> +UnmapXenPage (
> +  IN VOID *PagePtr
> +  )
> +{
> +  xen_remove_from_physmap_t Parameters;
> +  INTN                      ReturnCode;
> +
> +  Parameters.domid = DOMID_SELF;
> +  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;

(2) Ditto.

> +  ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);
> +  ASSERT (ReturnCode == 0);
> +}
> +
> +
> +STATIC
> +UINT64
> +GetCPUFreq (
> +  IN XEN_VCPU_TIME_INFO *VcpuTime
> +  )
> +{
> +  UINT32 Version;
> +  UINT32 TSCToSystemMultiplier;
> +  INT8   TSCShift;
> +  UINT64 CPUFreq;
> +
> +  do {
> +    Version = VcpuTime->Version;
> +    MemoryFence ();
> +    TSCToSystemMultiplier = VcpuTime->TSCToSystemMultiplier;
> +    TSCShift = VcpuTime->TSCShift;
> +    MemoryFence ();
> +  } while (((Version & 1) != 0) && (Version != VcpuTime->Version));
> +
> +  CPUFreq = (1000000000ULL << 32) / TSCToSystemMultiplier;

(3) I understand that OvmfXen is X64, and so this code will not be built
for IA32 in practice. Still, for sticking with the coding style, it's
better to use LShiftU64() here, and then DivU64x32().

> +  if (TSCShift >= 0) {
> +      CPUFreq >>= VcpuTime->TSCShift;
> +  } else {
> +      CPUFreq <<= -VcpuTime->TSCShift;
> +  }

(4) Please use LShiftU64() and RShiftU64().

(5) I think there's a typo here: you just fished out "TscShift" from the
shared info page; we should used that cached value, and not access the
shared info page again. Is that right?

> +  return CPUFreq;
> +}
> +
> +VOID
> +XenDelay (
> +  IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
> +  IN UINT64             DelayNs
> +  )
> +{
> +  UINT64 Tick;
> +
> +  Tick = AsmReadTsc ();
> +  Tick += (DelayNs * GetCPUFreq (VcpuTimeInfo)) / 1000000000ULL;

(6) Please use MultU64x64() and DivU64x32(). (1,000,000,000 fits in a
UINT32.)

> +  while (AsmReadTsc() <= Tick) {
> +    CpuPause();
> +  }
> +}
> +
> +
> +/**
> +  Calculate the frequency of the Local Apic Timer
> +**/
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  )
> +{
> +  XEN_SHARED_INFO       *SharedInfo;
> +  XEN_VCPU_TIME_INFO    *VcpuTimeInfo;
> +  UINT32                TimerTick, TimerTick2;
> +  UINT64                TscTick, TscTick2;
> +  UINT64                Freq;
> +  EFI_STATUS            Status;
> +
> +  SharedInfo = AllocatePages (1);

(7) Can you check if this succeeds?

> +  Status = MapSharedInfoPage (SharedInfo);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    goto Exit;
> +  }
> +
> +  VcpuTimeInfo = &SharedInfo->VcpuInfo[0].Time;
> +
> +  InitializeApicTimer (1, MAX_UINT32, TRUE, 0);
> +  DisableApicTimerInterrupt ();
> +
> +  TimerTick = GetApicTimerCurrentCount ();
> +  TscTick = AsmReadTsc ();
> +  XenDelay (VcpuTimeInfo, 1000000ULL);
> +  TimerTick2 = GetApicTimerCurrentCount ();
> +  TscTick2 = AsmReadTsc ();
> +
> +  Freq = (GetCPUFreq (VcpuTimeInfo) * (TimerTick - TimerTick2))
> +    / (TscTick2 - TscTick);

(8) Please use the above-mentioned U64 multiplication and division helpers.

(9) In case we are concerned about U64 overflows anywhere in this patch:
SafeIntLib has range-checked functions, for example SafeUint64Mult().

Thanks!
Laszlo

> +  DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
> +
> +  UnmapXenPage (SharedInfo);
> +
> +Exit:
> +  FreePages (SharedInfo, 1);
> +}
> 

Re: [PATCH 4/5] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

[Roger Pau Monné]

On Wed, Jan 29, 2020 at 12:12:34PM +0000, Anthony PERARD wrote:
> Calculate the frequency of the APIC timer that Xen provides.
> 
> Even though the frequency is currently hard-coded, it isn't part of
> the public ABI that Xen provides and thus may change at any time. OVMF
> needs to determine the frequency by an other mean.
> 
> Fortunately, Xen provides a way to determines the frequency of the
> TSC, so we can use TSC to calibrate the frequency of the APIC timer.
> That information is found in the shared_info page which we map and
> unmap once done (XenBusDxe is going to map the page somewhere else).
> 
> The calculated frequency is only logged in this patch, it will be used
> in a following patch.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks! Some comments below on the implementation.

> ---
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |   1 +
>  OvmfPkg/XenPlatformPei/Platform.h         |   5 +
>  OvmfPkg/XenPlatformPei/Platform.c         |   1 +
>  OvmfPkg/XenPlatformPei/Xen.c              | 123 ++++++++++++++++++++++
>  4 files changed, 130 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 0ef77db92c03..335a442538c2 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
>    DebugLib
>    HobLib
>    IoLib
> +  LocalApicLib
>    PciLib
>    ResourcePublicationLib
>    PeiServicesLib
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index 7661f4a8de0a..97e482a065f0 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -127,6 +127,11 @@ XenGetE820Map (
>    UINT32 *Count
>    );
>  
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  );
> +
>  extern EFI_BOOT_MODE mBootMode;
>  
>  extern UINT8 mPhysMemAddressWidth;
> diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c
> index 717fd0ab1a45..e9511eb40c62 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.c
> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> @@ -448,6 +448,7 @@ InitializeXenPlatform (
>    InitializeRamRegions ();
>  
>    InitializeXen ();
> +  CalibrateLapicTimer ();
>  
>    if (mBootMode != BOOT_ON_S3_RESUME) {
>      ReserveEmuVariableNvStore ();
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index c41fecdc486e..d6cdc9a8e31c 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -19,6 +19,7 @@
>  //
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
> +#include <Library/LocalApicLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
>  #include <Guid/XenInfo.h>
> @@ -386,3 +387,125 @@ InitializeXen (
>  
>    return EFI_SUCCESS;
>  }
> +
> +
> +EFI_STATUS
> +MapSharedInfoPage (
> +  IN VOID *PagePtr
> +  )
> +{
> +  xen_add_to_physmap_t  Parameters;
> +  INTN                  ReturnCode;
> +
> +  Parameters.domid = DOMID_SELF;
> +  Parameters.space = XENMAPSPACE_shared_info;
> +  Parameters.idx = 0;
> +  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;
> +  ReturnCode = XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameters);
> +  if (ReturnCode != 0) {
> +    return EFI_NO_MAPPING;
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +VOID
> +UnmapXenPage (
> +  IN VOID *PagePtr
> +  )
> +{
> +  xen_remove_from_physmap_t Parameters;
> +  INTN                      ReturnCode;
> +
> +  Parameters.domid = DOMID_SELF;
> +  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;
> +  ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);

I'm afraid this will leave a hole in the p2m, and hence freeing the
page back to OVMF is wrong (I assume this is what FreePages does in
CalibrateLapicTimer), as the physical address would be unpopulated
after the call to XENMEM_remove_from_physmap.

> +  ASSERT (ReturnCode == 0);
> +}
> +
> +
> +STATIC
> +UINT64
> +GetCPUFreq (
> +  IN XEN_VCPU_TIME_INFO *VcpuTime
> +  )
> +{
> +  UINT32 Version;
> +  UINT32 TSCToSystemMultiplier;
> +  INT8   TSCShift;
> +  UINT64 CPUFreq;
> +
> +  do {
> +    Version = VcpuTime->Version;
> +    MemoryFence ();
> +    TSCToSystemMultiplier = VcpuTime->TSCToSystemMultiplier;
> +    TSCShift = VcpuTime->TSCShift;
> +    MemoryFence ();
> +  } while (((Version & 1) != 0) && (Version != VcpuTime->Version));
> +
> +  CPUFreq = (1000000000ULL << 32) / TSCToSystemMultiplier;
> +  if (TSCShift >= 0) {
> +      CPUFreq >>= VcpuTime->TSCShift;
> +  } else {
> +      CPUFreq <<= -VcpuTime->TSCShift;
> +  }
> +  return CPUFreq;
> +}
> +
> +VOID
> +XenDelay (
> +  IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
> +  IN UINT64             DelayNs
> +  )
> +{
> +  UINT64 Tick;
> +
> +  Tick = AsmReadTsc ();
> +  Tick += (DelayNs * GetCPUFreq (VcpuTimeInfo)) / 1000000000ULL;
> +  while (AsmReadTsc() <= Tick) {
> +    CpuPause();
> +  }
> +}
> +
> +
> +/**
> +  Calculate the frequency of the Local Apic Timer
> +**/
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  )
> +{
> +  XEN_SHARED_INFO       *SharedInfo;
> +  XEN_VCPU_TIME_INFO    *VcpuTimeInfo;
> +  UINT32                TimerTick, TimerTick2;
> +  UINT64                TscTick, TscTick2;
> +  UINT64                Freq;
> +  EFI_STATUS            Status;
> +
> +  SharedInfo = AllocatePages (1);

Hm, it's not the best approach to use a regular memory page to map the
shared info: mapping it is very likely to cause superpage shattering,
and once unmapped it leaves a hole in the p2m.

As a reference you could map the shared info page at maximum physical
address allowed (after checking it's not populated) like Wei is doing
for the Xen on HyperV work.

Roger.

Re: [PATCH 4/5] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

[Anthony PERARD]

On Thu, Jan 30, 2020 at 10:12:51AM +0100, Roger Pau Monné wrote:
> On Wed, Jan 29, 2020 at 12:12:34PM +0000, Anthony PERARD wrote:
> > +  Parameters.domid = DOMID_SELF;
> > +  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;
> > +  ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);
> 
> I'm afraid this will leave a hole in the p2m, and hence freeing the
> page back to OVMF is wrong (I assume this is what FreePages does in
> CalibrateLapicTimer), as the physical address would be unpopulated
> after the call to XENMEM_remove_from_physmap.

I guess there's more refactoring to do in OVMF, there are other's of
this kind of call, mostly in the PV drivers, XenBusDxe.

> > +
> > +  SharedInfo = AllocatePages (1);
> 
> Hm, it's not the best approach to use a regular memory page to map the
> shared info: mapping it is very likely to cause superpage shattering,
> and once unmapped it leaves a hole in the p2m.

:-(

> As a reference you could map the shared info page at maximum physical
> address allowed (after checking it's not populated) like Wei is doing
> for the Xen on HyperV work.

I'll look at what can be done with OVMF.

Is there some kind of information that Xen could give? Or is it all
information that OVMF should keep track of? Or if Xen only have
XENMEM_memory_map, then OVMF already have this information.

Thanks,

-- 
Anthony PERARD

Re: [Xen-devel] [PATCH 4/5] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

[Andrew Cooper]

On 30/01/2020 12:44, Anthony PERARD wrote:
> On Thu, Jan 30, 2020 at 10:12:51AM +0100, Roger Pau Monné wrote:
>> On Wed, Jan 29, 2020 at 12:12:34PM +0000, Anthony PERARD wrote:
>>> +  Parameters.domid = DOMID_SELF;
>>> +  Parameters.gpfn = (UINTN) PagePtr >> EFI_PAGE_SHIFT;
>>> +  ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);
>> I'm afraid this will leave a hole in the p2m, and hence freeing the
>> page back to OVMF is wrong (I assume this is what FreePages does in
>> CalibrateLapicTimer), as the physical address would be unpopulated
>> after the call to XENMEM_remove_from_physmap.
> I guess there's more refactoring to do in OVMF, there are other's of
> this kind of call, mostly in the PV drivers, XenBusDxe.
>
>>> +
>>> +  SharedInfo = AllocatePages (1);
>> Hm, it's not the best approach to use a regular memory page to map the
>> shared info: mapping it is very likely to cause superpage shattering,
>> and once unmapped it leaves a hole in the p2m.
> :-(
>
>> As a reference you could map the shared info page at maximum physical
>> address allowed (after checking it's not populated) like Wei is doing
>> for the Xen on HyperV work.
> I'll look at what can be done with OVMF.
>
> Is there some kind of information that Xen could give? Or is it all
> information that OVMF should keep track of? Or if Xen only have
> XENMEM_memory_map, then OVMF already have this information.

We need to actually tackle the memory problem, and provide something
correct via XENMEM_memory_map (/similar).

So far, noone has actually started to try fixing the problem.  Perhaps
now is a good enough kick to get started.

~Andrew

[PATCH 5/5] OvmfPkg/OvmfXen: Set PcdFSBClock

[Anthony PERARD]

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 <anthony.perard@citrix.com>
---
 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
   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:
-- 
Anthony PERARD

Re: [PATCH 5/5] OvmfPkg/OvmfXen: Set PcdFSBClock

[Laszlo Ersek]

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 <anthony.perard@citrix.com>
> ---
>  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 <lersek@redhat.com>

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