diff mbox series

[v2,09/31] OvmfPkg/XenOvmf: use a TimerLib instance that depends only on the CPU

Message ID 20190409110844.14746-10-anthony.perard@citrix.com (mailing list archive)
State New, archived
Headers show
Series Specific platform to run OVMF in Xen PVH and HVM guests | expand

Commit Message

Anthony PERARD April 9, 2019, 11:08 a.m. UTC 
ACPI Timer does not work in a PVH guest, but local APIC works on both
PVH and HVM.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 OvmfPkg/XenOvmf.dsc | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Laszlo Ersek April 11, 2019, 11:25 a.m. UTC | #1 
On 04/09/19 13:08, Anthony PERARD wrote:
> ACPI Timer does not work in a PVH guest, but local APIC works on both
> PVH and HVM.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenOvmf.dsc | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
> index 7b8a1fdf6b..cc51bac3be 100644
> --- a/OvmfPkg/XenOvmf.dsc
> +++ b/OvmfPkg/XenOvmf.dsc
> @@ -104,7 +104,7 @@ [SkuIds]
>  ################################################################################
>  [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
> @@ -205,7 +205,7 @@ [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>  
>  [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
> @@ -284,7 +284,7 @@ [LibraryClasses.common.DXE_CORE]
>  
>  [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
> @@ -301,7 +301,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>  
>  [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
> @@ -316,7 +316,7 @@ [LibraryClasses.common.UEFI_DRIVER]
>  
>  [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
> @@ -344,7 +344,7 @@ [LibraryClasses.common.DXE_DRIVER]
>  
>  [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 simplifying this patch by:
- deleting all TimerLib resolutions except the one under
  [LibraryClasses],
- updating that one (remaining) resolution to SecPeiDxeTimerLibCpu.inf.

With that:

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

Thanks
Laszlo
Laszlo Ersek April 11, 2019, 11:33 a.m. UTC | #2 
On 04/11/19 13:25, Laszlo Ersek wrote:
> On 04/09/19 13:08, Anthony PERARD wrote:
>> ACPI Timer does not work in a PVH guest, but local APIC works on both
>> PVH and HVM.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  OvmfPkg/XenOvmf.dsc | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
>> index 7b8a1fdf6b..cc51bac3be 100644
>> --- a/OvmfPkg/XenOvmf.dsc
>> +++ b/OvmfPkg/XenOvmf.dsc
>> @@ -104,7 +104,7 @@ [SkuIds]
>>  ################################################################################
>>  [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
>> @@ -205,7 +205,7 @@ [LibraryClasses.common]
>>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>>
>>  [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
>> @@ -284,7 +284,7 @@ [LibraryClasses.common.DXE_CORE]
>>
>>  [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
>> @@ -301,7 +301,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>>
>>  [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
>> @@ -316,7 +316,7 @@ [LibraryClasses.common.UEFI_DRIVER]
>>
>>  [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
>> @@ -344,7 +344,7 @@ [LibraryClasses.common.DXE_DRIVER]
>>
>>  [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 simplifying this patch by:
> - deleting all TimerLib resolutions except the one under
>   [LibraryClasses],
> - updating that one (remaining) resolution to SecPeiDxeTimerLibCpu.inf.
>
> With that:
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>

... BTW, this is the first time I hear about

  MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf

so now I've gone ahead and read the documentation in that INF file.
(It's really nice documentation.)

It says,

# Note: A driver of type DXE_RUNTIME_DRIVER and DXE_SMM_DRIVER can use this TimerLib
#  in their initialization without any issues. They only have to be careful in
#  the implementation of runtime services and SMI handlers.
#  Because CPU Local APIC and ITC could be programmed by OS, it cannot be
#  used by SMM drivers and runtime drivers, ACPI timer is recommended for SMM
#  drivers and runtime drivers.

Now, Xen doesn't use SMM, and runtime drivers can be penetrated by the
OS anyway, so I'm not concerned about security here. It's just that you
could run into unexpected behavior with runtime drivers, as long as Xen
allows the guest OS to program the hardware like hinted above.

Obviously I haven't checked the runtime drivers included by this new
platform for their consumption of TimerLib. The patch does modify
[LibraryClasses.common.DXE_RUNTIME_DRIVER], so it's likely prudent for
you to build the platform with "--report-file=blah.report", and check
all runtime drivers. For each that consumes TimerLib, I suggest also
checking if they call a TimerLib API at runtime, not just at
initialization.

If it turns out that the above is not only theoretical, I think the
commit message might deserve a note about the fact, but I'm OK to ACK
the patch without such a note too.

Thanks
Laszlo
diff mbox series

Patch

diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc
index 7b8a1fdf6b..cc51bac3be 100644
--- a/OvmfPkg/XenOvmf.dsc
+++ b/OvmfPkg/XenOvmf.dsc
@@ -104,7 +104,7 @@  [SkuIds]
 ################################################################################
 [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
@@ -205,7 +205,7 @@  [LibraryClasses.common]
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
 
 [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
@@ -284,7 +284,7 @@  [LibraryClasses.common.DXE_CORE]
 
 [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
@@ -301,7 +301,7 @@  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
 
 [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
@@ -316,7 +316,7 @@  [LibraryClasses.common.UEFI_DRIVER]
 
 [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
@@ -344,7 +344,7 @@  [LibraryClasses.common.DXE_DRIVER]
 
 [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