Re: [edk2] [PATCH RFC 11/14] OvmfPkg/XenOvmf: Adding XenTimerLocalApic

2017-01-05 Thread Laszlo Ersek 
On 12/08/16 16:33, Anthony PERARD wrote:
> And replacing the ACPI Timer by this one based on the local APIC.
> 
> ACPI Timer does not work in a PVH guest, but local APIC works on both
> PVH and HVM.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD 
> ---
>  OvmfPkg/XenOvmf.dsc |  18 +-
>  OvmfPkg/XenOvmf.fdf |   2 +-
>  OvmfPkg/XenTimerLocalApic/Timer.h   | 191 
>  OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c   | 386 
> 
>  OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf |  51 
>  5 files changed, 640 insertions(+), 8 deletions(-)
>  create mode 100644 OvmfPkg/XenTimerLocalApic/Timer.h
>  create mode 100644 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c
>  create mode 100644 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf

comments below:

> diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
> index ef32c33..31a2185 100644
> --- a/OvmfPkg/XenOvmf.dsc
> +++ b/OvmfPkg/XenOvmf.dsc
> @@ -81,7 +81,7 @@
>  
> 
>  [LibraryClasses]
>PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
> +  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
>BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
>BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
> @@ -175,7 +175,7 @@
>  !endif
>  
>  [LibraryClasses.common.SEC]
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> +  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
>  !ifdef $(DEBUG_ON_SERIAL_PORT)
>DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> @@ -251,7 +251,7 @@
>  
>  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> +  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -269,7 +269,7 @@
>  
>  [LibraryClasses.common.UEFI_DRIVER]
>PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> +  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -284,7 +284,7 @@
>  
>  [LibraryClasses.common.DXE_DRIVER]
>PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> +  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -314,7 +314,7 @@
>  
>  [LibraryClasses.common.UEFI_APPLICATION]
>PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> +  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

(1) I suggest to split this patch if possible. In the first half, the
TimerLib class resolutions should be flipped, with the argument given in
the second paragraph of the commit message.

The subject line for the first patch should be something like

  OvmfPkg/XenOvmf: use a TimerLib instance that depends only on the CPU

> @@ -546,7 +546,11 @@
>  !endif
>  
>MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
> -  PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf
> +  OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf

(2) The second patch should introduce the new DXE_DRIVER module:

  OvmfPkg/XenOvmf: introduce XenTimerDxe

The commit message should document that
"PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf" is replaced with a
Xen-specific EFI_TIMER_ARCH_PROTOCOL implementation.

(3) The new DXE_DRIVER module should be located under "OvmfPkg/XenTimerDxe".

> +  #{
> +  #
> +
> #LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> +  #}

(4) Please don't just comment out unnecessary stuff; it's better to
remove

[edk2] [PATCH RFC 11/14] OvmfPkg/XenOvmf: Adding XenTimerLocalApic

2016-12-08 Thread Anthony PERARD 
And replacing the ACPI Timer by this one based on the local APIC.

ACPI Timer does not work in a PVH guest, but local APIC works on both
PVH and HVM.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Anthony PERARD 
---
 OvmfPkg/XenOvmf.dsc |  18 +-
 OvmfPkg/XenOvmf.fdf |   2 +-
 OvmfPkg/XenTimerLocalApic/Timer.h   | 191 
 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c   | 386 
 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf |  51 
 5 files changed, 640 insertions(+), 8 deletions(-)
 create mode 100644 OvmfPkg/XenTimerLocalApic/Timer.h
 create mode 100644 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.c
 create mode 100644 OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf

diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
index ef32c33..31a2185 100644
--- a/OvmfPkg/XenOvmf.dsc
+++ b/OvmfPkg/XenOvmf.dsc
@@ -81,7 +81,7 @@
 

 [LibraryClasses]
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
-  TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
   PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
   BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
@@ -175,7 +175,7 @@
 !endif
 
 [LibraryClasses.common.SEC]
-  TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
+  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
 !ifdef $(DEBUG_ON_SERIAL_PORT)
   DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
@@ -251,7 +251,7 @@
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -269,7 +269,7 @@
 
 [LibraryClasses.common.UEFI_DRIVER]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
@@ -284,7 +284,7 @@
 
 [LibraryClasses.common.DXE_DRIVER]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
   
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -314,7 +314,7 @@
 
 [LibraryClasses.common.UEFI_APPLICATION]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
+  TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
   
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -546,7 +546,11 @@
 !endif
 
   MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
-  PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf
+  OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf
+  #{
+  #
+#LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
+  #}
   UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
   UefiCpuPkg/CpuDxe/CpuDxe.inf
   PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf
index c211f61..f6876d7 100644
--- a/OvmfPkg/XenOvmf.fdf
+++ b/OvmfPkg/XenOvmf.fdf
@@ -207,7 +207,7 @@ INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
 INF  MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
 INF  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
 INF  MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
-INF  PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf
+INF  OvmfPkg/XenTimerLocalApic/XenTimerLocalApic.inf
 INF  UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.inf
 INF  UefiCpuPkg/CpuDxe/CpuDxe.inf
 INF  PcAtChipsetPkg/8254TimerDxe/8254Timer.inf
diff --git a/OvmfPkg/XenTimerLocalApic/Timer.h 
b/OvmfPkg/XenTimerLocalApic/Timer.h
new file mode 100644
index 000..8d6f37c
--- /dev/null
+++ b/OvmfPkg/XenTimerLocalApic/Timer.h
@@ -0,0 +1,191 @@
+/** @file
+  Private data structures
+
+Copyright (c) 2005 - 2016, Intel Corporation. All