[PATCH v2 0/7] OvmfXen: Set PcdFSBClock at runtime

[PATCH v2 0/7] 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-v2

Changes in v2:
- main change is to allow mapping of Xen pages outside of the RAM
  see patch: "OvmfPkg/XenPlatformPei: Map extra physical address"
- that new function allows to map the Xen shared info page (where we can find
  information about tsc frequency) at the highest physical address allowed.

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 (7):
  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/IndustryStandard: Introduce PageTable.h
  OvmfPkg/XenPlatformPei: Map extra physical address
  OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency
  OvmfPkg/OvmfXen: Set PcdFSBClock

 MdePkg/MdePkg.dec                             |   8 +-
 OvmfPkg/OvmfXen.dsc                           |   4 +-
 OvmfPkg/XenPlatformPei/XenPlatformPei.inf     |   4 +
 .../IndustryStandard/PageTable.h}             | 117 +-------
 OvmfPkg/Include/IndustryStandard/Xen/xen.h    |  17 +-
 .../BaseMemEncryptSevLib/X64/VirtualMemory.h  | 143 +---------
 OvmfPkg/XenPlatformPei/Platform.h             |  10 +
 OvmfPkg/XenPlatformPei/Platform.c             |   1 +
 OvmfPkg/XenPlatformPei/Xen.c                  | 252 ++++++++++++++++++
 OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm    |   2 +-
 10 files changed, 287 insertions(+), 271 deletions(-)
 copy OvmfPkg/{Library/BaseMemEncryptSevLib/X64/VirtualMemory.h => Include/IndustryStandard/PageTable.h} (60%)

-- 
Anthony PERARD

[PATCH v2 1/7] 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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

[PATCH v2 2/7] 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
---
CC: Bob Feng <bob.c.feng@intel.com>
CC: Michael D Kinney <michael.d.kinney@intel.com>
CC: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/MdePkg.dec | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 1d2637acc22a..f0d3b91fc635 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2257,10 +2257,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.
@@ -2364,5 +2360,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

[PATCH v2 3/7] 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    - fix case of Tsc* field

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

[PATCH v2 4/7] OvmfPkg/IndustryStandard: Introduce PageTable.h

[Anthony PERARD]

We are going to use the page table structure in yet another place,
collect the types and macro that can be used from another module
rather that making yet another copy.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
CC: Tom Lendacky <thomas.lendacky@amd.com>
CC: Brijesh Singh <brijesh.singh@amd.com>
---

Notes:
    v2:
    - new patch

 .../IndustryStandard/PageTable.h}             | 117 +-------------
 .../BaseMemEncryptSevLib/X64/VirtualMemory.h  | 143 +-----------------
 2 files changed, 5 insertions(+), 255 deletions(-)
 copy OvmfPkg/{Library/BaseMemEncryptSevLib/X64/VirtualMemory.h => Include/IndustryStandard/PageTable.h} (60%)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Include/IndustryStandard/PageTable.h
similarity index 60%
copy from OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
copy to OvmfPkg/Include/IndustryStandard/PageTable.h
index 996f94f07ebb..e3da4e8cf21c 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
+++ b/OvmfPkg/Include/IndustryStandard/PageTable.h
@@ -1,6 +1,6 @@
 /** @file
 
-  Virtual Memory Management Services to set or clear the memory encryption bit
+  x86_64 Page Tables structures
 
   Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>
@@ -11,17 +11,10 @@
 
 **/
 
-#ifndef __VIRTUAL_MEMORY__
-#define __VIRTUAL_MEMORY__
+#ifndef __PAGE_TABLE_H__
+#define __PAGE_TABLE_H__
 
-#include <Library/BaseLib.h>
-#include <Library/BaseMemoryLib.h>
-#include <Library/CacheMaintenanceLib.h>
-#include <Library/DebugLib.h>
-#include <Library/MemoryAllocationLib.h>
-#include <Uefi.h>
-
-#define SYS_CODE64_SEL 0x38
+#include <Base.h>
 
 #pragma pack(1)
 
@@ -165,106 +158,4 @@ typedef union {
 #define PTE_OFFSET(x)               ( (x >> 12) & PAGETABLE_ENTRY_MASK)
 #define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
 
-#define PAGE_TABLE_POOL_ALIGNMENT   BASE_2MB
-#define PAGE_TABLE_POOL_UNIT_SIZE   SIZE_2MB
-#define PAGE_TABLE_POOL_UNIT_PAGES  \
-  EFI_SIZE_TO_PAGES (PAGE_TABLE_POOL_UNIT_SIZE)
-#define PAGE_TABLE_POOL_ALIGN_MASK  \
-  (~(EFI_PHYSICAL_ADDRESS)(PAGE_TABLE_POOL_ALIGNMENT - 1))
-
-typedef struct {
-  VOID            *NextPool;
-  UINTN           Offset;
-  UINTN           FreePages;
-} PAGE_TABLE_POOL;
-
-/**
-  Return the pagetable memory encryption mask.
-
-  @return  The pagetable memory encryption mask.
-
-**/
-UINT64
-EFIAPI
-InternalGetMemEncryptionAddressMask (
-  VOID
-  );
-
-/**
-  This function clears memory encryption bit for the memory region specified by
-  PhysicalAddress and Length from the current page table context.
-
-  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
-                                      current CR3)
-  @param[in]  PhysicalAddress         The physical address that is the start
-                                      address of a memory region.
-  @param[in]  Length                  The length of memory region
-  @param[in]  Flush                   Flush the caches before applying the
-                                      encryption mask
-
-  @retval RETURN_SUCCESS              The attributes were cleared for the
-                                      memory region.
-  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
-  @retval RETURN_UNSUPPORTED          Clearing the memory encyrption attribute
-                                      is not supported
-**/
-RETURN_STATUS
-EFIAPI
-InternalMemEncryptSevSetMemoryDecrypted (
-  IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
-  IN  PHYSICAL_ADDRESS        PhysicalAddress,
-  IN  UINTN                   Length,
-  IN  BOOLEAN                 Flush
-  );
-
-/**
-  This function sets memory encryption bit for the memory region specified by
-  PhysicalAddress and Length from the current page table context.
-
-  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
-                                      current CR3)
-  @param[in]  PhysicalAddress         The physical address that is the start
-                                      address of a memory region.
-  @param[in]  Length                  The length of memory region
-  @param[in]  Flush                   Flush the caches before applying the
-                                      encryption mask
-
-  @retval RETURN_SUCCESS              The attributes were set for the memory
-                                      region.
-  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
-  @retval RETURN_UNSUPPORTED          Setting the memory encyrption attribute
-                                      is not supported
-**/
-RETURN_STATUS
-EFIAPI
-InternalMemEncryptSevSetMemoryEncrypted (
-  IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
-  IN  PHYSICAL_ADDRESS        PhysicalAddress,
-  IN  UINTN                   Length,
-  IN  BOOLEAN                 Flush
-  );
-
-/**
-  Returns the encryption state of the specified virtual address range.
-
-  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
-                                      current CR3)
-  @param[in]  BaseAddress             Base address to check
-  @param[in]  Length                  Length of virtual address range
-
-  @retval MemEncryptSevAddressRangeUnencrypted  Address range is mapped
-                                                unencrypted
-  @retval MemEncryptSevAddressRangeEncrypted    Address range is mapped
-                                                encrypted
-  @retval MemEncryptSevAddressRangeMixed        Address range is mapped mixed
-  @retval MemEncryptSevAddressRangeError        Address range is not mapped
-**/
-MEM_ENCRYPT_SEV_ADDRESS_RANGE_STATE
-EFIAPI
-InternalMemEncryptSevGetAddressRangeState (
-  IN PHYSICAL_ADDRESS         Cr3BaseAddress,
-  IN PHYSICAL_ADDRESS         BaseAddress,
-  IN UINTN                    Length
-  );
-
 #endif
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
index 996f94f07ebb..b621d811ca6f 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
@@ -20,151 +20,10 @@
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Uefi.h>
+#include <IndustryStandard/PageTable.h>
 
 #define SYS_CODE64_SEL 0x38
 
-#pragma pack(1)
-
-//
-// Page-Map Level-4 Offset (PML4) and
-// Page-Directory-Pointer Offset (PDPE) entries 4K & 2MB
-//
-
-typedef union {
-  struct {
-    UINT64  Present:1;                // 0 = Not present in memory,
-                                      //   1 = Present in memory
-    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
-    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
-    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
-                                      //   1 = Write-Through caching
-    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
-    UINT64  Accessed:1;               // 0 = Not accessed,
-                                      //   1 = Accessed (set by CPU)
-    UINT64  Reserved:1;               // Reserved
-    UINT64  MustBeZero:2;             // Must Be Zero
-    UINT64  Available:3;              // Available for use by system software
-    UINT64  PageTableBaseAddress:40;  // Page Table Base Address
-    UINT64  AvabilableHigh:11;        // Available for use by system software
-    UINT64  Nx:1;                     // No Execute bit
-  } Bits;
-  UINT64    Uint64;
-} PAGE_MAP_AND_DIRECTORY_POINTER;
-
-//
-// Page Table Entry 4KB
-//
-typedef union {
-  struct {
-    UINT64  Present:1;                // 0 = Not present in memory,
-                                      //   1 = Present in memory
-    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
-    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
-    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
-                                      //   1 = Write-Through caching
-    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
-    UINT64  Accessed:1;               // 0 = Not accessed,
-                                      //   1 = Accessed (set by CPU)
-    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by
-                                      //   processor on access to page
-    UINT64  PAT:1;                    //
-    UINT64  Global:1;                 // 0 = Not global page, 1 = global page
-                                      //   TLB not cleared on CR3 write
-    UINT64  Available:3;              // Available for use by system software
-    UINT64  PageTableBaseAddress:40;  // Page Table Base Address
-    UINT64  AvabilableHigh:11;        // Available for use by system software
-    UINT64  Nx:1;                     // 0 = Execute Code,
-                                      //   1 = No Code Execution
-  } Bits;
-  UINT64    Uint64;
-} PAGE_TABLE_4K_ENTRY;
-
-//
-// Page Table Entry 2MB
-//
-typedef union {
-  struct {
-    UINT64  Present:1;                // 0 = Not present in memory,
-                                      //   1 = Present in memory
-    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
-    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
-    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
-                                      //   1=Write-Through caching
-    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
-    UINT64  Accessed:1;               // 0 = Not accessed,
-                                      //   1 = Accessed (set by CPU)
-    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by
-                                      //   processor on access to page
-    UINT64  MustBe1:1;                // Must be 1
-    UINT64  Global:1;                 // 0 = Not global page, 1 = global page
-                                      //   TLB not cleared on CR3 write
-    UINT64  Available:3;              // Available for use by system software
-    UINT64  PAT:1;                    //
-    UINT64  MustBeZero:8;             // Must be zero;
-    UINT64  PageTableBaseAddress:31;  // Page Table Base Address
-    UINT64  AvabilableHigh:11;        // Available for use by system software
-    UINT64  Nx:1;                     // 0 = Execute Code,
-                                      //   1 = No Code Execution
-  } Bits;
-  UINT64    Uint64;
-} PAGE_TABLE_ENTRY;
-
-//
-// Page Table Entry 1GB
-//
-typedef union {
-  struct {
-    UINT64  Present:1;                // 0 = Not present in memory,
-                                      //   1 = Present in memory
-    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
-    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
-    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
-                                      //   1 = Write-Through caching
-    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
-    UINT64  Accessed:1;               // 0 = Not accessed,
-                                      //   1 = Accessed (set by CPU)
-    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by
-                                      //   processor on access to page
-    UINT64  MustBe1:1;                // Must be 1
-    UINT64  Global:1;                 // 0 = Not global page, 1 = global page
-                                      //   TLB not cleared on CR3 write
-    UINT64  Available:3;              // Available for use by system software
-    UINT64  PAT:1;                    //
-    UINT64  MustBeZero:17;            // Must be zero;
-    UINT64  PageTableBaseAddress:22;  // Page Table Base Address
-    UINT64  AvabilableHigh:11;        // Available for use by system software
-    UINT64  Nx:1;                     // 0 = Execute Code,
-                                      //   1 = No Code Execution
-  } Bits;
-  UINT64    Uint64;
-} PAGE_TABLE_1G_ENTRY;
-
-#pragma pack()
-
-#define IA32_PG_P                   BIT0
-#define IA32_PG_RW                  BIT1
-#define IA32_PG_PS                  BIT7
-
-#define PAGING_PAE_INDEX_MASK       0x1FF
-
-#define PAGING_4K_ADDRESS_MASK_64   0x000FFFFFFFFFF000ull
-#define PAGING_2M_ADDRESS_MASK_64   0x000FFFFFFFE00000ull
-#define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
-
-#define PAGING_L1_ADDRESS_SHIFT     12
-#define PAGING_L2_ADDRESS_SHIFT     21
-#define PAGING_L3_ADDRESS_SHIFT     30
-#define PAGING_L4_ADDRESS_SHIFT     39
-
-#define PAGING_PML4E_NUMBER         4
-
-#define PAGETABLE_ENTRY_MASK        ((1UL << 9) - 1)
-#define PML4_OFFSET(x)              ( (x >> 39) & PAGETABLE_ENTRY_MASK)
-#define PDP_OFFSET(x)               ( (x >> 30) & PAGETABLE_ENTRY_MASK)
-#define PDE_OFFSET(x)               ( (x >> 21) & PAGETABLE_ENTRY_MASK)
-#define PTE_OFFSET(x)               ( (x >> 12) & PAGETABLE_ENTRY_MASK)
-#define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
-
 #define PAGE_TABLE_POOL_ALIGNMENT   BASE_2MB
 #define PAGE_TABLE_POOL_UNIT_SIZE   SIZE_2MB
 #define PAGE_TABLE_POOL_UNIT_PAGES  \
-- 
Anthony PERARD

[PATCH v2 5/7] OvmfPkg/XenPlatformPei: Map extra physical address

[Anthony PERARD]

Some information available in a Xen guest can be mapped anywhere in
the physical address space and they don't need to be backed by RAM.
For example, the shared info page.

While it's easier to put those pages anywhere, it is better to avoid
mapping it where the RAM is. It might split a nice 1G guest page table
into 4k pages and thus reducing performance of the guest when it
access its memory. Also mapping a page like the shared info page and
then unmapping it or mapping it somewhere else would live a hole in
the RAM that the guest would propably not been able to use anymore.

So the patch introduce a new function which can be used to 1:1
mapping of guest physical memory above 4G during the PEI phase so we
can map the Xen shared pages outside of memory that can be used by
guest, and as high as possible.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v2:
    - new patch

 OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  1 +
 OvmfPkg/XenPlatformPei/Platform.h         |  5 ++
 OvmfPkg/XenPlatformPei/Xen.c              | 71 +++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
index 0ef77db92c03..8790d907d3ec 100644
--- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
+++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
@@ -66,6 +66,7 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
index 7661f4a8de0a..e70ca58078eb 100644
--- a/OvmfPkg/XenPlatformPei/Platform.h
+++ b/OvmfPkg/XenPlatformPei/Platform.h
@@ -127,6 +127,11 @@ XenGetE820Map (
   UINT32 *Count
   );
 
+EFI_STATUS
+PhysicalAddressIdentityMapping (
+  IN EFI_PHYSICAL_ADDRESS AddressToMap
+  );
+
 extern EFI_BOOT_MODE mBootMode;
 
 extern UINT8 mPhysMemAddressWidth;
diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index c41fecdc486e..b2a7d1c21dac 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -17,6 +17,8 @@
 //
 // The Library classes this module consumes
 //
+#include <Library/BaseMemoryLib.h>
+#include <Library/CpuLib.h>
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/MemoryAllocationLib.h>
@@ -25,6 +27,7 @@
 #include <IndustryStandard/E820.h>
 #include <Library/ResourcePublicationLib.h>
 #include <Library/MtrrLib.h>
+#include <IndustryStandard/PageTable.h>
 #include <IndustryStandard/Xen/arch-x86/hvm/start_info.h>
 #include <Library/XenHypercallLib.h>
 #include <IndustryStandard/Xen/memory.h>
@@ -386,3 +389,71 @@ InitializeXen (
 
   return EFI_SUCCESS;
 }
+
+EFI_STATUS
+PhysicalAddressIdentityMapping (
+  IN EFI_PHYSICAL_ADDRESS   AddressToMap
+  )
+{
+  INTN                            Index;
+  PAGE_MAP_AND_DIRECTORY_POINTER  *L4, *L3;
+  PAGE_TABLE_ENTRY                *PageTable;
+
+  DEBUG ((DEBUG_INFO, "Mapping 1:1 of address 0x%lx\n", (UINT64)AddressToMap));
+
+  // L4 / Top level Page Directory Pointers
+
+  L4 = (VOID*)(UINTN)PcdGet32 (PcdOvmfSecPageTablesBase);
+  Index = PML4_OFFSET (AddressToMap);
+
+  if (!L4[Index].Bits.Present) {
+    L3 = AllocatePages (1);
+    if (L3 == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    ZeroMem (L3, EFI_PAGE_SIZE);
+
+    L4[Index].Bits.ReadWrite = 1;
+    L4[Index].Bits.Accessed = 1;
+    L4[Index].Bits.PageTableBaseAddress = (EFI_PHYSICAL_ADDRESS)L3 >> 12;
+    L4[Index].Bits.Present = 1;
+  }
+
+  // L3 / Next level Page Directory Pointers
+
+  L3 = (VOID*)(EFI_PHYSICAL_ADDRESS)(L4[Index].Bits.PageTableBaseAddress << 12);
+  Index = PDP_OFFSET (AddressToMap);
+
+  if (!L3[Index].Bits.Present) {
+    PageTable = AllocatePages (1);
+    if (PageTable == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    ZeroMem (PageTable, EFI_PAGE_SIZE);
+
+    L3[Index].Bits.ReadWrite = 1;
+    L3[Index].Bits.Accessed = 1;
+    L3[Index].Bits.PageTableBaseAddress = (EFI_PHYSICAL_ADDRESS)PageTable >> 12;
+    L3[Index].Bits.Present = 1;
+  }
+
+  // L2 / Page Table Entries
+
+  PageTable = (VOID*)(EFI_PHYSICAL_ADDRESS)(L3[Index].Bits.PageTableBaseAddress << 12);
+  Index = PDE_OFFSET (AddressToMap);
+
+  if (!PageTable[Index].Bits.Present) {
+    PageTable[Index].Bits.ReadWrite = 1;
+    PageTable[Index].Bits.Accessed = 1;
+    PageTable[Index].Bits.Dirty = 1;
+    PageTable[Index].Bits.MustBe1 = 1;
+    PageTable[Index].Bits.PageTableBaseAddress = AddressToMap >> 21;
+    PageTable[Index].Bits.Present = 1;
+  }
+
+  CpuFlushTlb ();
+
+  return EFI_SUCCESS;
+}
-- 
Anthony PERARD

[PATCH v2 6/7] 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 shared_info page is map at the highest physical address allowed as
it doesn't need to be in the RAM, thus there's a call to update the
page table.

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

Notes:
    v2:
    - fix CamelCases
    - Use U64 multiplication and division helpers
    - don't read TscShift from the SharedInfo page again
    - change the location of the shared info page to be outside of the ram
    - check for overflow in XenDelay
    - check for overflow when calculating the calculating APIC frequency

 OvmfPkg/XenPlatformPei/XenPlatformPei.inf |   2 +
 OvmfPkg/XenPlatformPei/Platform.h         |   5 +
 OvmfPkg/XenPlatformPei/Platform.c         |   1 +
 OvmfPkg/XenPlatformPei/Xen.c              | 177 ++++++++++++++++++++++
 4 files changed, 185 insertions(+)

diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
index 8790d907d3ec..5732d2188871 100644
--- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
+++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
   DebugLib
   HobLib
   IoLib
+  LocalApicLib
   PciLib
   ResourcePublicationLib
   PeiServicesLib
@@ -59,6 +60,7 @@ [LibraryClasses]
   MtrrLib
   MemEncryptSevLib
   PcdLib
+  SafeIntLib
   XenHypercallLib
 
 [Pcd]
diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
index e70ca58078eb..77d88fc159d7 100644
--- a/OvmfPkg/XenPlatformPei/Platform.h
+++ b/OvmfPkg/XenPlatformPei/Platform.h
@@ -132,6 +132,11 @@ PhysicalAddressIdentityMapping (
   IN EFI_PHYSICAL_ADDRESS AddressToMap
   );
 
+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 b2a7d1c21dac..7524aaa11a29 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -21,8 +21,10 @@
 #include <Library/CpuLib.h>
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
+#include <Library/LocalApicLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PcdLib.h>
+#include <Library/SafeIntLib.h>
 #include <Guid/XenInfo.h>
 #include <IndustryStandard/E820.h>
 #include <Library/ResourcePublicationLib.h>
@@ -457,3 +459,178 @@ PhysicalAddressIdentityMapping (
 
   return EFI_SUCCESS;
 }
+
+STATIC
+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;
+}
+
+STATIC
+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 = DivU64x32 (LShiftU64 (1000000000ULL, 32), TscToSystemMultiplier);
+  if (TscShift >= 0) {
+      CpuFreq = RShiftU64 (CpuFreq, TscShift);
+  } else {
+      CpuFreq = LShiftU64 (CpuFreq, -TscShift);
+  }
+  return CpuFreq;
+}
+
+STATIC
+VOID
+XenDelay (
+  IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
+  IN UINT64             DelayNs
+  )
+{
+  UINT64        Tick;
+  UINT64        CpuFreq;
+  UINT64        Delay;
+  UINT64        DelayTick;
+  UINT64        NewTick;
+  RETURN_STATUS Status;
+
+  Tick = AsmReadTsc ();
+
+  CpuFreq = GetCpuFreq (VcpuTimeInfo);
+  Status = SafeUint64Mult (DelayNs, CpuFreq, &Delay);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR,
+        "XenDelay (%ld ns): delay too big in relation to CPU freq %ld Hz\n",
+        DelayNs, CpuFreq));
+    CpuDeadLoop ();
+  }
+
+  DelayTick = DivU64x32 (Delay, 1000000000UL);
+
+  NewTick = Tick + DelayTick;
+
+  //
+  // Check for overflow
+  //
+  if (NewTick < Tick) {
+    //
+    // Overflow, wait for TSC to also overflow
+    //
+    while (AsmReadTsc () >= Tick) {
+      CpuPause ();
+    }
+  }
+
+  while (AsmReadTsc () <= NewTick) {
+    CpuPause ();
+  }
+}
+
+
+/**
+  Calculate the frequency of the Local Apic Timer
+**/
+VOID
+CalibrateLapicTimer (
+  VOID
+  )
+{
+  XEN_SHARED_INFO       *SharedInfo;
+  XEN_VCPU_TIME_INFO    *VcpuTimeInfo;
+  UINT32                TimerTick, TimerTick2, DiffTimer;
+  UINT64                TscTick, TscTick2;
+  UINT64                Freq;
+  UINT64                Dividend;
+  EFI_STATUS            Status;
+
+
+  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);
+  Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR,
+        "Failed to add page table entry for Xen shared info page: %r\n",
+        Status));
+    return;
+  }
+
+  Status = MapSharedInfoPage (SharedInfo);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed to map Xen's shared info page: %r\n",
+        Status));
+    return;
+  }
+
+  VcpuTimeInfo = &SharedInfo->VcpuInfo[0].Time;
+
+  InitializeApicTimer (1, MAX_UINT32, TRUE, 0);
+  DisableApicTimerInterrupt ();
+
+  TimerTick = GetApicTimerCurrentCount ();
+  TscTick = AsmReadTsc ();
+  XenDelay (VcpuTimeInfo, 1000000ULL);
+  TimerTick2 = GetApicTimerCurrentCount ();
+  TscTick2 = AsmReadTsc ();
+
+
+  DiffTimer = TimerTick - TimerTick2;
+  Status = SafeUint64Mult (GetCpuFreq (VcpuTimeInfo), DiffTimer, &Dividend);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "overflow while calculating APIC frequency\n"));
+    DEBUG ((DEBUG_ERROR, "CPU freq: %ld Hz; APIC timer tick count for 1 ms: %d\n",
+        GetCpuFreq (VcpuTimeInfo), DiffTimer));
+    CpuDeadLoop ();
+  }
+
+  Freq = DivU64x64Remainder (Dividend, TscTick2 - TscTick, NULL);
+  DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
+
+  UnmapXenPage (SharedInfo);
+}
-- 
Anthony PERARD

[PATCH v2 7/7] 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>
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);
+  ASSERT_RETURN_ERROR (Status);
+
   UnmapXenPage (SharedInfo);
 }
-- 
Anthony PERARD

Re: [PATCH v2 0/7] OvmfXen: Set PcdFSBClock at runtime

[Laszlo Ersek]

On 03/25/21 16:47, Anthony PERARD wrote:
> Patch series available in this git branch:
> git://xenbits.xen.org/people/aperard/ovmf.git br.apic-timer-freq-v2

I'll get to this sometime in April, possibly after the SEV-SNP series.
That shouldn't discourage others from reviewing sooner, of course.

Thanks
Laszlo

> 
> Changes in v2:
> - main change is to allow mapping of Xen pages outside of the RAM
>   see patch: "OvmfPkg/XenPlatformPei: Map extra physical address"
> - that new function allows to map the Xen shared info page (where we can find
>   information about tsc frequency) at the highest physical address allowed.
> 
> 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 (7):
>   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/IndustryStandard: Introduce PageTable.h
>   OvmfPkg/XenPlatformPei: Map extra physical address
>   OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency
>   OvmfPkg/OvmfXen: Set PcdFSBClock
> 
>  MdePkg/MdePkg.dec                             |   8 +-
>  OvmfPkg/OvmfXen.dsc                           |   4 +-
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf     |   4 +
>  .../IndustryStandard/PageTable.h}             | 117 +-------
>  OvmfPkg/Include/IndustryStandard/Xen/xen.h    |  17 +-
>  .../BaseMemEncryptSevLib/X64/VirtualMemory.h  | 143 +---------
>  OvmfPkg/XenPlatformPei/Platform.h             |  10 +
>  OvmfPkg/XenPlatformPei/Platform.c             |   1 +
>  OvmfPkg/XenPlatformPei/Xen.c                  | 252 ++++++++++++++++++
>  OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm    |   2 +-
>  10 files changed, 287 insertions(+), 271 deletions(-)
>  copy OvmfPkg/{Library/BaseMemEncryptSevLib/X64/VirtualMemory.h => Include/IndustryStandard/PageTable.h} (60%)
> 

Re: [PATCH v2 4/7] OvmfPkg/IndustryStandard: Introduce PageTable.h

[Lendacky, Thomas]

On 3/25/21 10:47 AM, Anthony PERARD wrote:
> We are going to use the page table structure in yet another place,
> collect the types and macro that can be used from another module
> rather that making yet another copy.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

This begs the question of whether there should be only one version of this
header file, now. There are still copies in other places, but maybe that
can be a future cleanup? I'll leave that decision to Laszlo.

With one minor comment below, otherwise:

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: Brijesh Singh <brijesh.singh@amd.com>
> ---
> 
> Notes:
>     v2:
>     - new patch
> 
>  .../IndustryStandard/PageTable.h}             | 117 +-------------
>  .../BaseMemEncryptSevLib/X64/VirtualMemory.h  | 143 +-----------------
>  2 files changed, 5 insertions(+), 255 deletions(-)
>  copy OvmfPkg/{Library/BaseMemEncryptSevLib/X64/VirtualMemory.h => Include/IndustryStandard/PageTable.h} (60%)
> 

...

> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> index 996f94f07ebb..b621d811ca6f 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> @@ -20,151 +20,10 @@
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Uefi.h>
> +#include <IndustryStandard/PageTable.h>

Typically, these are preferred to be in sorted order.

Thanks,
Tom

>  
>  #define SYS_CODE64_SEL 0x38
>  

Re: [edk2-devel] [PATCH v2 4/7] OvmfPkg/IndustryStandard: Introduce PageTable.h

[Laszlo Ersek]

On 03/26/21 15:16, Lendacky, Thomas wrote:
> On 3/25/21 10:47 AM, Anthony PERARD wrote:
>> We are going to use the page table structure in yet another place,
>> collect the types and macro that can be used from another module
>> rather that making yet another copy.
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> This begs the question of whether there should be only one version of this
> header file, now. There are still copies in other places, but maybe that
> can be a future cleanup? I'll leave that decision to Laszlo.

Optimally the header file (a single header file) would exist solely in
MdePkg, but I'm OK with this patch too.

> 
> With one minor comment below, otherwise:
> 
> Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
>> ---
>> CC: Tom Lendacky <thomas.lendacky@amd.com>
>> CC: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>
>> Notes:
>>     v2:
>>     - new patch
>>
>>  .../IndustryStandard/PageTable.h}             | 117 +-------------
>>  .../BaseMemEncryptSevLib/X64/VirtualMemory.h  | 143 +-----------------
>>  2 files changed, 5 insertions(+), 255 deletions(-)
>>  copy OvmfPkg/{Library/BaseMemEncryptSevLib/X64/VirtualMemory.h => Include/IndustryStandard/PageTable.h} (60%)
>>
> 
> ...
> 
>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
>> index 996f94f07ebb..b621d811ca6f 100644
>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
>> @@ -20,151 +20,10 @@
>>  #include <Library/DebugLib.h>
>>  #include <Library/MemoryAllocationLib.h>
>>  #include <Uefi.h>
>> +#include <IndustryStandard/PageTable.h>
> 
> Typically, these are preferred to be in sorted order.

Exactly, thanks.

Laszlo

> 
> Thanks,
> Tom
> 
>>  
>>  #define SYS_CODE64_SEL 0x38
>>  
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 4/7] OvmfPkg/IndustryStandard: Introduce PageTable.h

[Laszlo Ersek]

On 03/25/21 16:47, Anthony PERARD via groups.io wrote:
> We are going to use the page table structure in yet another place,
> collect the types and macro that can be used from another module
> rather that making yet another copy.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> CC: Tom Lendacky <thomas.lendacky@amd.com>
> CC: Brijesh Singh <brijesh.singh@amd.com>
> ---
> 
> Notes:
>     v2:
>     - new patch
> 
>  .../IndustryStandard/PageTable.h}             | 117 +-------------
>  .../BaseMemEncryptSevLib/X64/VirtualMemory.h  | 143 +-----------------
>  2 files changed, 5 insertions(+), 255 deletions(-)
>  copy OvmfPkg/{Library/BaseMemEncryptSevLib/X64/VirtualMemory.h => Include/IndustryStandard/PageTable.h} (60%)
> 
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Include/IndustryStandard/PageTable.h
> similarity index 60%
> copy from OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> copy to OvmfPkg/Include/IndustryStandard/PageTable.h
> index 996f94f07ebb..e3da4e8cf21c 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> +++ b/OvmfPkg/Include/IndustryStandard/PageTable.h
> @@ -1,6 +1,6 @@
>  /** @file
>  
> -  Virtual Memory Management Services to set or clear the memory encryption bit
> +  x86_64 Page Tables structures
>  
>    Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>
> @@ -11,17 +11,10 @@
>  
>  **/
>  
> -#ifndef __VIRTUAL_MEMORY__
> -#define __VIRTUAL_MEMORY__
> +#ifndef __PAGE_TABLE_H__
> +#define __PAGE_TABLE_H__

Should be PAGE_TABLE_H_ nowadays (no leading underscore, and one
trailing underscore suffices).

Thanks
Laszlo

>  
> -#include <Library/BaseLib.h>
> -#include <Library/BaseMemoryLib.h>
> -#include <Library/CacheMaintenanceLib.h>
> -#include <Library/DebugLib.h>
> -#include <Library/MemoryAllocationLib.h>
> -#include <Uefi.h>
> -
> -#define SYS_CODE64_SEL 0x38
> +#include <Base.h>
>  
>  #pragma pack(1)
>  
> @@ -165,106 +158,4 @@ typedef union {
>  #define PTE_OFFSET(x)               ( (x >> 12) & PAGETABLE_ENTRY_MASK)
>  #define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
>  
> -#define PAGE_TABLE_POOL_ALIGNMENT   BASE_2MB
> -#define PAGE_TABLE_POOL_UNIT_SIZE   SIZE_2MB
> -#define PAGE_TABLE_POOL_UNIT_PAGES  \
> -  EFI_SIZE_TO_PAGES (PAGE_TABLE_POOL_UNIT_SIZE)
> -#define PAGE_TABLE_POOL_ALIGN_MASK  \
> -  (~(EFI_PHYSICAL_ADDRESS)(PAGE_TABLE_POOL_ALIGNMENT - 1))
> -
> -typedef struct {
> -  VOID            *NextPool;
> -  UINTN           Offset;
> -  UINTN           FreePages;
> -} PAGE_TABLE_POOL;
> -
> -/**
> -  Return the pagetable memory encryption mask.
> -
> -  @return  The pagetable memory encryption mask.
> -
> -**/
> -UINT64
> -EFIAPI
> -InternalGetMemEncryptionAddressMask (
> -  VOID
> -  );
> -
> -/**
> -  This function clears memory encryption bit for the memory region specified by
> -  PhysicalAddress and Length from the current page table context.
> -
> -  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
> -                                      current CR3)
> -  @param[in]  PhysicalAddress         The physical address that is the start
> -                                      address of a memory region.
> -  @param[in]  Length                  The length of memory region
> -  @param[in]  Flush                   Flush the caches before applying the
> -                                      encryption mask
> -
> -  @retval RETURN_SUCCESS              The attributes were cleared for the
> -                                      memory region.
> -  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
> -  @retval RETURN_UNSUPPORTED          Clearing the memory encyrption attribute
> -                                      is not supported
> -**/
> -RETURN_STATUS
> -EFIAPI
> -InternalMemEncryptSevSetMemoryDecrypted (
> -  IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
> -  IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> -  );
> -
> -/**
> -  This function sets memory encryption bit for the memory region specified by
> -  PhysicalAddress and Length from the current page table context.
> -
> -  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
> -                                      current CR3)
> -  @param[in]  PhysicalAddress         The physical address that is the start
> -                                      address of a memory region.
> -  @param[in]  Length                  The length of memory region
> -  @param[in]  Flush                   Flush the caches before applying the
> -                                      encryption mask
> -
> -  @retval RETURN_SUCCESS              The attributes were set for the memory
> -                                      region.
> -  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
> -  @retval RETURN_UNSUPPORTED          Setting the memory encyrption attribute
> -                                      is not supported
> -**/
> -RETURN_STATUS
> -EFIAPI
> -InternalMemEncryptSevSetMemoryEncrypted (
> -  IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
> -  IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> -  );
> -
> -/**
> -  Returns the encryption state of the specified virtual address range.
> -
> -  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
> -                                      current CR3)
> -  @param[in]  BaseAddress             Base address to check
> -  @param[in]  Length                  Length of virtual address range
> -
> -  @retval MemEncryptSevAddressRangeUnencrypted  Address range is mapped
> -                                                unencrypted
> -  @retval MemEncryptSevAddressRangeEncrypted    Address range is mapped
> -                                                encrypted
> -  @retval MemEncryptSevAddressRangeMixed        Address range is mapped mixed
> -  @retval MemEncryptSevAddressRangeError        Address range is not mapped
> -**/
> -MEM_ENCRYPT_SEV_ADDRESS_RANGE_STATE
> -EFIAPI
> -InternalMemEncryptSevGetAddressRangeState (
> -  IN PHYSICAL_ADDRESS         Cr3BaseAddress,
> -  IN PHYSICAL_ADDRESS         BaseAddress,
> -  IN UINTN                    Length
> -  );
> -
>  #endif
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> index 996f94f07ebb..b621d811ca6f 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> @@ -20,151 +20,10 @@
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Uefi.h>
> +#include <IndustryStandard/PageTable.h>
>  
>  #define SYS_CODE64_SEL 0x38
>  
> -#pragma pack(1)
> -
> -//
> -// Page-Map Level-4 Offset (PML4) and
> -// Page-Directory-Pointer Offset (PDPE) entries 4K & 2MB
> -//
> -
> -typedef union {
> -  struct {
> -    UINT64  Present:1;                // 0 = Not present in memory,
> -                                      //   1 = Present in memory
> -    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> -    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> -    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
> -                                      //   1 = Write-Through caching
> -    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> -    UINT64  Accessed:1;               // 0 = Not accessed,
> -                                      //   1 = Accessed (set by CPU)
> -    UINT64  Reserved:1;               // Reserved
> -    UINT64  MustBeZero:2;             // Must Be Zero
> -    UINT64  Available:3;              // Available for use by system software
> -    UINT64  PageTableBaseAddress:40;  // Page Table Base Address
> -    UINT64  AvabilableHigh:11;        // Available for use by system software
> -    UINT64  Nx:1;                     // No Execute bit
> -  } Bits;
> -  UINT64    Uint64;
> -} PAGE_MAP_AND_DIRECTORY_POINTER;
> -
> -//
> -// Page Table Entry 4KB
> -//
> -typedef union {
> -  struct {
> -    UINT64  Present:1;                // 0 = Not present in memory,
> -                                      //   1 = Present in memory
> -    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> -    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> -    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
> -                                      //   1 = Write-Through caching
> -    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> -    UINT64  Accessed:1;               // 0 = Not accessed,
> -                                      //   1 = Accessed (set by CPU)
> -    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by
> -                                      //   processor on access to page
> -    UINT64  PAT:1;                    //
> -    UINT64  Global:1;                 // 0 = Not global page, 1 = global page
> -                                      //   TLB not cleared on CR3 write
> -    UINT64  Available:3;              // Available for use by system software
> -    UINT64  PageTableBaseAddress:40;  // Page Table Base Address
> -    UINT64  AvabilableHigh:11;        // Available for use by system software
> -    UINT64  Nx:1;                     // 0 = Execute Code,
> -                                      //   1 = No Code Execution
> -  } Bits;
> -  UINT64    Uint64;
> -} PAGE_TABLE_4K_ENTRY;
> -
> -//
> -// Page Table Entry 2MB
> -//
> -typedef union {
> -  struct {
> -    UINT64  Present:1;                // 0 = Not present in memory,
> -                                      //   1 = Present in memory
> -    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> -    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> -    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
> -                                      //   1=Write-Through caching
> -    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> -    UINT64  Accessed:1;               // 0 = Not accessed,
> -                                      //   1 = Accessed (set by CPU)
> -    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by
> -                                      //   processor on access to page
> -    UINT64  MustBe1:1;                // Must be 1
> -    UINT64  Global:1;                 // 0 = Not global page, 1 = global page
> -                                      //   TLB not cleared on CR3 write
> -    UINT64  Available:3;              // Available for use by system software
> -    UINT64  PAT:1;                    //
> -    UINT64  MustBeZero:8;             // Must be zero;
> -    UINT64  PageTableBaseAddress:31;  // Page Table Base Address
> -    UINT64  AvabilableHigh:11;        // Available for use by system software
> -    UINT64  Nx:1;                     // 0 = Execute Code,
> -                                      //   1 = No Code Execution
> -  } Bits;
> -  UINT64    Uint64;
> -} PAGE_TABLE_ENTRY;
> -
> -//
> -// Page Table Entry 1GB
> -//
> -typedef union {
> -  struct {
> -    UINT64  Present:1;                // 0 = Not present in memory,
> -                                      //   1 = Present in memory
> -    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> -    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> -    UINT64  WriteThrough:1;           // 0 = Write-Back caching,
> -                                      //   1 = Write-Through caching
> -    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> -    UINT64  Accessed:1;               // 0 = Not accessed,
> -                                      //   1 = Accessed (set by CPU)
> -    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by
> -                                      //   processor on access to page
> -    UINT64  MustBe1:1;                // Must be 1
> -    UINT64  Global:1;                 // 0 = Not global page, 1 = global page
> -                                      //   TLB not cleared on CR3 write
> -    UINT64  Available:3;              // Available for use by system software
> -    UINT64  PAT:1;                    //
> -    UINT64  MustBeZero:17;            // Must be zero;
> -    UINT64  PageTableBaseAddress:22;  // Page Table Base Address
> -    UINT64  AvabilableHigh:11;        // Available for use by system software
> -    UINT64  Nx:1;                     // 0 = Execute Code,
> -                                      //   1 = No Code Execution
> -  } Bits;
> -  UINT64    Uint64;
> -} PAGE_TABLE_1G_ENTRY;
> -
> -#pragma pack()
> -
> -#define IA32_PG_P                   BIT0
> -#define IA32_PG_RW                  BIT1
> -#define IA32_PG_PS                  BIT7
> -
> -#define PAGING_PAE_INDEX_MASK       0x1FF
> -
> -#define PAGING_4K_ADDRESS_MASK_64   0x000FFFFFFFFFF000ull
> -#define PAGING_2M_ADDRESS_MASK_64   0x000FFFFFFFE00000ull
> -#define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
> -
> -#define PAGING_L1_ADDRESS_SHIFT     12
> -#define PAGING_L2_ADDRESS_SHIFT     21
> -#define PAGING_L3_ADDRESS_SHIFT     30
> -#define PAGING_L4_ADDRESS_SHIFT     39
> -
> -#define PAGING_PML4E_NUMBER         4
> -
> -#define PAGETABLE_ENTRY_MASK        ((1UL << 9) - 1)
> -#define PML4_OFFSET(x)              ( (x >> 39) & PAGETABLE_ENTRY_MASK)
> -#define PDP_OFFSET(x)               ( (x >> 30) & PAGETABLE_ENTRY_MASK)
> -#define PDE_OFFSET(x)               ( (x >> 21) & PAGETABLE_ENTRY_MASK)
> -#define PTE_OFFSET(x)               ( (x >> 12) & PAGETABLE_ENTRY_MASK)
> -#define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
> -
>  #define PAGE_TABLE_POOL_ALIGNMENT   BASE_2MB
>  #define PAGE_TABLE_POOL_UNIT_SIZE   SIZE_2MB
>  #define PAGE_TABLE_POOL_UNIT_PAGES  \
> 

Re: [edk2-devel] [PATCH v2 4/7] OvmfPkg/IndustryStandard: Introduce PageTable.h

[Laszlo Ersek]

On 03/25/21 16:47, Anthony PERARD via groups.io wrote:
> We are going to use the page table structure in yet another place,
> collect the types and macro that can be used from another module
> rather that making yet another copy.

s/rather that/rather than/

Re: [edk2-devel] [PATCH v2 5/7] OvmfPkg/XenPlatformPei: Map extra physical address

[Laszlo Ersek]

On 03/25/21 16:47, Anthony PERARD via groups.io wrote:
> Some information available in a Xen guest can be mapped anywhere in
> the physical address space and they don't need to be backed by RAM.
> For example, the shared info page.
> 
> While it's easier to put those pages anywhere, it is better to avoid
> mapping it where the RAM is. It might split a nice 1G guest page table
> into 4k pages and thus reducing performance of the guest when it
> access its memory. Also mapping a page like the shared info page and

s/access/accesses/

> then unmapping it or mapping it somewhere else would live a hole in

s/live/leave/

> the RAM that the guest would propably not been able to use anymore.

s/been/be/

> 
> So the patch introduce a new function which can be used to 1:1

s/introduce/introduces/

> mapping of guest physical memory above 4G during the PEI phase so we
> can map the Xen shared pages outside of memory that can be used by
> guest, and as high as possible.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     v2:
>     - new patch

With the typo fixes:

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

Thanks
Laszlo

> 
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |  1 +
>  OvmfPkg/XenPlatformPei/Platform.h         |  5 ++
>  OvmfPkg/XenPlatformPei/Xen.c              | 71 +++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 0ef77db92c03..8790d907d3ec 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -66,6 +66,7 @@ [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index 7661f4a8de0a..e70ca58078eb 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -127,6 +127,11 @@ XenGetE820Map (
>    UINT32 *Count
>    );
>  
> +EFI_STATUS
> +PhysicalAddressIdentityMapping (
> +  IN EFI_PHYSICAL_ADDRESS AddressToMap
> +  );
> +
>  extern EFI_BOOT_MODE mBootMode;
>  
>  extern UINT8 mPhysMemAddressWidth;
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index c41fecdc486e..b2a7d1c21dac 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -17,6 +17,8 @@
>  //
>  // The Library classes this module consumes
>  //
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/CpuLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
>  #include <Library/MemoryAllocationLib.h>
> @@ -25,6 +27,7 @@
>  #include <IndustryStandard/E820.h>
>  #include <Library/ResourcePublicationLib.h>
>  #include <Library/MtrrLib.h>
> +#include <IndustryStandard/PageTable.h>
>  #include <IndustryStandard/Xen/arch-x86/hvm/start_info.h>
>  #include <Library/XenHypercallLib.h>
>  #include <IndustryStandard/Xen/memory.h>
> @@ -386,3 +389,71 @@ InitializeXen (
>  
>    return EFI_SUCCESS;
>  }
> +
> +EFI_STATUS
> +PhysicalAddressIdentityMapping (
> +  IN EFI_PHYSICAL_ADDRESS   AddressToMap
> +  )
> +{
> +  INTN                            Index;
> +  PAGE_MAP_AND_DIRECTORY_POINTER  *L4, *L3;
> +  PAGE_TABLE_ENTRY                *PageTable;
> +
> +  DEBUG ((DEBUG_INFO, "Mapping 1:1 of address 0x%lx\n", (UINT64)AddressToMap));
> +
> +  // L4 / Top level Page Directory Pointers
> +
> +  L4 = (VOID*)(UINTN)PcdGet32 (PcdOvmfSecPageTablesBase);
> +  Index = PML4_OFFSET (AddressToMap);
> +
> +  if (!L4[Index].Bits.Present) {
> +    L3 = AllocatePages (1);
> +    if (L3 == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +
> +    ZeroMem (L3, EFI_PAGE_SIZE);
> +
> +    L4[Index].Bits.ReadWrite = 1;
> +    L4[Index].Bits.Accessed = 1;
> +    L4[Index].Bits.PageTableBaseAddress = (EFI_PHYSICAL_ADDRESS)L3 >> 12;
> +    L4[Index].Bits.Present = 1;
> +  }
> +
> +  // L3 / Next level Page Directory Pointers
> +
> +  L3 = (VOID*)(EFI_PHYSICAL_ADDRESS)(L4[Index].Bits.PageTableBaseAddress << 12);
> +  Index = PDP_OFFSET (AddressToMap);
> +
> +  if (!L3[Index].Bits.Present) {
> +    PageTable = AllocatePages (1);
> +    if (PageTable == NULL) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +
> +    ZeroMem (PageTable, EFI_PAGE_SIZE);
> +
> +    L3[Index].Bits.ReadWrite = 1;
> +    L3[Index].Bits.Accessed = 1;
> +    L3[Index].Bits.PageTableBaseAddress = (EFI_PHYSICAL_ADDRESS)PageTable >> 12;
> +    L3[Index].Bits.Present = 1;
> +  }
> +
> +  // L2 / Page Table Entries
> +
> +  PageTable = (VOID*)(EFI_PHYSICAL_ADDRESS)(L3[Index].Bits.PageTableBaseAddress << 12);
> +  Index = PDE_OFFSET (AddressToMap);
> +
> +  if (!PageTable[Index].Bits.Present) {
> +    PageTable[Index].Bits.ReadWrite = 1;
> +    PageTable[Index].Bits.Accessed = 1;
> +    PageTable[Index].Bits.Dirty = 1;
> +    PageTable[Index].Bits.MustBe1 = 1;
> +    PageTable[Index].Bits.PageTableBaseAddress = AddressToMap >> 21;
> +    PageTable[Index].Bits.Present = 1;
> +  }
> +
> +  CpuFlushTlb ();
> +
> +  return EFI_SUCCESS;
> +}
> 

Re: [edk2-devel] [PATCH v2 6/7] OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency

[Laszlo Ersek]

On 03/25/21 16:47, Anthony PERARD via groups.io 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 shared_info page is map at the highest physical address allowed as

(1) s/map/mapped/


> it doesn't need to be in the RAM, thus there's a call to update the
> page table.
> 
> 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>
> ---
> 
> Notes:
>     v2:
>     - fix CamelCases
>     - Use U64 multiplication and division helpers
>     - don't read TscShift from the SharedInfo page again
>     - change the location of the shared info page to be outside of the ram
>     - check for overflow in XenDelay
>     - check for overflow when calculating the calculating APIC frequency
> 
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf |   2 +
>  OvmfPkg/XenPlatformPei/Platform.h         |   5 +
>  OvmfPkg/XenPlatformPei/Platform.c         |   1 +
>  OvmfPkg/XenPlatformPei/Xen.c              | 177 ++++++++++++++++++++++
>  4 files changed, 185 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> index 8790d907d3ec..5732d2188871 100644
> --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
>    DebugLib
>    HobLib
>    IoLib
> +  LocalApicLib
>    PciLib
>    ResourcePublicationLib
>    PeiServicesLib
> @@ -59,6 +60,7 @@ [LibraryClasses]
>    MtrrLib
>    MemEncryptSevLib
>    PcdLib
> +  SafeIntLib
>    XenHypercallLib
>  
>  [Pcd]
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index e70ca58078eb..77d88fc159d7 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -132,6 +132,11 @@ PhysicalAddressIdentityMapping (
>    IN EFI_PHYSICAL_ADDRESS AddressToMap
>    );
>  
> +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 b2a7d1c21dac..7524aaa11a29 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -21,8 +21,10 @@
>  #include <Library/CpuLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/HobLib.h>
> +#include <Library/LocalApicLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/SafeIntLib.h>
>  #include <Guid/XenInfo.h>
>  #include <IndustryStandard/E820.h>
>  #include <Library/ResourcePublicationLib.h>
> @@ -457,3 +459,178 @@ PhysicalAddressIdentityMapping (
>  
>    return EFI_SUCCESS;
>  }
> +
> +STATIC
> +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;
> +}
> +
> +STATIC
> +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 = DivU64x32 (LShiftU64 (1000000000ULL, 32), TscToSystemMultiplier);
> +  if (TscShift >= 0) {
> +      CpuFreq = RShiftU64 (CpuFreq, TscShift);
> +  } else {
> +      CpuFreq = LShiftU64 (CpuFreq, -TscShift);
> +  }
> +  return CpuFreq;
> +}
> +
> +STATIC
> +VOID
> +XenDelay (
> +  IN XEN_VCPU_TIME_INFO *VcpuTimeInfo,
> +  IN UINT64             DelayNs
> +  )
> +{
> +  UINT64        Tick;
> +  UINT64        CpuFreq;
> +  UINT64        Delay;
> +  UINT64        DelayTick;
> +  UINT64        NewTick;
> +  RETURN_STATUS Status;
> +
> +  Tick = AsmReadTsc ();
> +
> +  CpuFreq = GetCpuFreq (VcpuTimeInfo);
> +  Status = SafeUint64Mult (DelayNs, CpuFreq, &Delay);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +        "XenDelay (%ld ns): delay too big in relation to CPU freq %ld Hz\n",
> +        DelayNs, CpuFreq));
> +    CpuDeadLoop ();
> +  }

(2) I suggest moving the ASSERT_EFI_ERROR() into the EFI_ERROR() branch,
namely between the DEBUG message and the CpuDeadLoop() call.

Applies to the rest of the ASSERT_EFI_ERROR() invocations in this patch.


(3) DelayNs and CpuFreq are of type UINT64, they should be logged with
%lu, not %ld.


(4) The indentation of the DEBUG macro arguments is incorrect; should be
2 spaces rather than 4.

Applies to the rest of the DEBUG() invocations in this patch.

> +
> +  DelayTick = DivU64x32 (Delay, 1000000000UL);

(5) The UL suffix on the constant is wasteful / misleading IMO;
DivU64x32 clearly takes a UINT32 as second parameter ("Divisor").


> +
> +  NewTick = Tick + DelayTick;
> +
> +  //
> +  // Check for overflow
> +  //
> +  if (NewTick < Tick) {
> +    //
> +    // Overflow, wait for TSC to also overflow
> +    //
> +    while (AsmReadTsc () >= Tick) {
> +      CpuPause ();
> +    }
> +  }
> +
> +  while (AsmReadTsc () <= NewTick) {
> +    CpuPause ();
> +  }
> +}
> +
> +
> +/**
> +  Calculate the frequency of the Local Apic Timer
> +**/
> +VOID
> +CalibrateLapicTimer (
> +  VOID
> +  )
> +{
> +  XEN_SHARED_INFO       *SharedInfo;
> +  XEN_VCPU_TIME_INFO    *VcpuTimeInfo;
> +  UINT32                TimerTick, TimerTick2, DiffTimer;
> +  UINT64                TscTick, TscTick2;
> +  UINT64                Freq;
> +  UINT64                Dividend;
> +  EFI_STATUS            Status;
> +
> +
> +  SharedInfo = (VOID*)((1ULL << mPhysMemAddressWidth) - EFI_PAGE_SIZE);

(I'm keeping in mind that this code is X64 only.)

> +  Status = PhysicalAddressIdentityMapping ((EFI_PHYSICAL_ADDRESS)SharedInfo);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +        "Failed to add page table entry for Xen shared info page: %r\n",
> +        Status));
> +    return;
> +  }
> +
> +  Status = MapSharedInfoPage (SharedInfo);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to map Xen's shared info page: %r\n",
> +        Status));
> +    return;
> +  }
> +
> +  VcpuTimeInfo = &SharedInfo->VcpuInfo[0].Time;
> +
> +  InitializeApicTimer (1, MAX_UINT32, TRUE, 0);
> +  DisableApicTimerInterrupt ();
> +
> +  TimerTick = GetApicTimerCurrentCount ();
> +  TscTick = AsmReadTsc ();
> +  XenDelay (VcpuTimeInfo, 1000000ULL);
> +  TimerTick2 = GetApicTimerCurrentCount ();
> +  TscTick2 = AsmReadTsc ();
> +
> +
> +  DiffTimer = TimerTick - TimerTick2;
> +  Status = SafeUint64Mult (GetCpuFreq (VcpuTimeInfo), DiffTimer, &Dividend);
> +  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "overflow while calculating APIC frequency\n"));
> +    DEBUG ((DEBUG_ERROR, "CPU freq: %ld Hz; APIC timer tick count for 1 ms: %d\n",
> +        GetCpuFreq (VcpuTimeInfo), DiffTimer));

(6) Please use %lu and %u for formatting the UINT64 retval of
GetCpuFreq(), and the UINT32 variable DiffTimer, respectively.

All trivial feedback of course, so:

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

Thanks
Laszlo

> +    CpuDeadLoop ();
> +  }
> +
> +  Freq = DivU64x64Remainder (Dividend, TscTick2 - TscTick, NULL);
> +  DEBUG ((DEBUG_INFO, "APIC Freq % 8lu Hz\n", Freq));
> +
> +  UnmapXenPage (SharedInfo);
> +}
> 

Re: [edk2-devel] [PATCH v2 7/7] OvmfPkg/OvmfXen: Set PcdFSBClock

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH v2 0/7] OvmfXen: Set PcdFSBClock at runtime

[Laszlo Ersek]

On 03/25/21 16:47, Anthony PERARD via groups.io wrote:
> Patch series available in this git branch:
> git://xenbits.xen.org/people/aperard/ovmf.git br.apic-timer-freq-v2
> 
> Changes in v2:
> - main change is to allow mapping of Xen pages outside of the RAM
>   see patch: "OvmfPkg/XenPlatformPei: Map extra physical address"
> - that new function allows to map the Xen shared info page (where we can find
>   information about tsc frequency) at the highest physical address allowed.

Before posting v3 (which I expect I'll merge), please submit a
github.com pull request as well, for your v3 topic branch. Such a PR
will never be merged, but it's good for kicking off CI (automated
builds) on your patches. If any one of the CI plugins fails on your
series, then you getting those results directly is better than me
encountering them first during an actual merge attempt.

For the above, I think you'll need to open a github.com account (in case
you don't have one yet). Running CI locally is possible, but it's so
much work (both setting up and executing) that I recommend just using a
github.com PR for CI's sake.

Thanks
Laszlo


> 
> 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 (7):
>   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/IndustryStandard: Introduce PageTable.h
>   OvmfPkg/XenPlatformPei: Map extra physical address
>   OvmfPkg/XenPlatformPei: Calibrate APIC timer frequency
>   OvmfPkg/OvmfXen: Set PcdFSBClock
> 
>  MdePkg/MdePkg.dec                             |   8 +-
>  OvmfPkg/OvmfXen.dsc                           |   4 +-
>  OvmfPkg/XenPlatformPei/XenPlatformPei.inf     |   4 +
>  .../IndustryStandard/PageTable.h}             | 117 +-------
>  OvmfPkg/Include/IndustryStandard/Xen/xen.h    |  17 +-
>  .../BaseMemEncryptSevLib/X64/VirtualMemory.h  | 143 +---------
>  OvmfPkg/XenPlatformPei/Platform.h             |  10 +
>  OvmfPkg/XenPlatformPei/Platform.c             |   1 +
>  OvmfPkg/XenPlatformPei/Xen.c                  | 252 ++++++++++++++++++
>  OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm    |   2 +-
>  10 files changed, 287 insertions(+), 271 deletions(-)
>  copy OvmfPkg/{Library/BaseMemEncryptSevLib/X64/VirtualMemory.h => Include/IndustryStandard/PageTable.h} (60%)
> 

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

[ray_linn]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Thu, Mar 25, 2021 at 11:47 PM, Anthony PERARD wrote:

> 
> - ## This value is used to configure X86 Processor FSB clock.
> - # @Prompt FSB Clock.
> - gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c

hi, Sir

This change caused the OVMF failed to load in QEMU when compiled with "SOURCE_DEBUG_ENABLE" as True.

the verbose message of QEMU shows:

> 
> ASSERT [SecMain]
> d:\myedk2\edk2\MdePkg\Library\BasePcdLibNull\PcdLib.c(95):
> ((BOOLEAN)(0==1))

The failure point is  PcdGet32 (PcdFSBClock) of debugtimer.c,  I  rolled back above change, then issue disappeared.

[-- Attachment #2: Type: text/html, Size: 694 bytes --]

回复: [edk2-devel] [PATCH v2 2/7] MdePkg: Allow PcdFSBClock to by Dynamic

[gaoliming]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

Ray:

 The problem is that PcdFSBClock is configured as Dynamic on OVMF. 

 

Anthony:

 Have you any suggestion for this problem?

 

Thanks

Liming

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 ray_linn@hotmail.com
发送时间: 2022年7月7日 12:13
收件人: Anthony PERARD <anthony.perard@citrix.com>; devel@edk2.groups.io
主题: Re: [edk2-devel] [PATCH v2 2/7] MdePkg: Allow PcdFSBClock to by Dynamic

 

On Thu, Mar 25, 2021 at 11:47 PM, Anthony PERARD wrote:

- ## This value is used to configure X86 Processor FSB clock.
- # @Prompt FSB Clock.
- gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c

hi, Sir

This change caused the OVMF failed to load in QEMU when compiled with "SOURCE_DEBUG_ENABLE" as True.

the verbose message of QEMU shows:

ASSERT [SecMain] d:\myedk2\edk2\MdePkg\Library\BasePcdLibNull\PcdLib.c(95): ((BOOLEAN)(0==1))

The failure point is  PcdGet32 (PcdFSBClock) of debugtimer.c,  I  rolled back above change, then issue disappeared.






[-- Attachment #2: Type: text/html, Size: 5235 bytes --]

Re: 回复: [edk2-devel] [PATCH v2 2/7] MdePkg: Allow PcdFSBClock to by Dynamic

[Anthony PERARD]

On Fri, Jul 08, 2022 at 10:14:35AM +0800, gaoliming wrote:
> Ray:
>  The problem is that PcdFSBClock is configured as Dynamic on OVMF. 

> Anthony:
>  Have you any suggestion for this problem?
> 
> Liming
> 
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 ray_linn@hotmail.com
> 发送时间: 2022年7月7日 12:13
> 收件人: Anthony PERARD <anthony.perard@citrix.com>; devel@edk2.groups.io
> 主题: Re: [edk2-devel] [PATCH v2 2/7] MdePkg: Allow PcdFSBClock to by Dynamic
> 
> On Thu, Mar 25, 2021 at 11:47 PM, Anthony PERARD wrote:
> 
> - ## This value is used to configure X86 Processor FSB clock.
> - # @Prompt FSB Clock.
> - gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> 
> hi, Sir
> 
> This change caused the OVMF failed to load in QEMU when compiled with "SOURCE_DEBUG_ENABLE" as True.
> 
> the verbose message of QEMU shows:
> 
> ASSERT [SecMain] d:\myedk2\edk2\MdePkg\Library\BasePcdLibNull\PcdLib.c(95): ((BOOLEAN)(0==1))
> 
> The failure point is  PcdGet32 (PcdFSBClock) of debugtimer.c,  I  rolled back above change, then issue disappeared.

What if you revert c37cbc030d96 ("OvmfPkg: Switch timer in build time
for OvmfPkg") instead? That commit seems to introduce PcdFSBClock as
dynamic in OvmfPkg*.dsc. But revert isn't going to be possible so
instead you could move "PcdFSBClock" to the [PcdsFixedAtBuild] sections
of all "OvmfPkg*.dsc". The dynamic nature of the pcd was only meant to
be used by OvmfXen, and no other platforms.

Cheers,

-- 
Anthony PERARD