Re: [edk2-devel] [PATCH v2 09/31] OvmfPkg/XenOvmf: use a TimerLib instance that depends only on the CPU

["Laszlo Ersek" <lersek@redhat.com>]

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