[edk2-devel] [PATCH v3 09/35] OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU

Anthony PERARD posted 35 patches 5 years, 4 months ago
There is a newer version of this series
[edk2-devel] [PATCH v3 09/35] OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU
Posted by Anthony PERARD 5 years, 4 months ago
ACPI Timer does not work in a PVH guest, but local APIC works on both
PVH and HVM.

Note that the use of SecPeiDxeTimerLibCpu might be an issue with a
driver of type DXE_RUNTIME_DRIVER. I've attemptde to find out which of
the DXE_RUNTIME_DRIVER uses the TimerLib at runtime. I've done that by
replacing the TimerLib evaluation in
[LibraryClasses.common.DXE_RUNTIME_DRIVER] by a different one and
check every module that uses it (with the --report-file=report build
option).

ResetSystemRuntimeDxe is calling the TimerLib API at runtime to do the
operation "EfiResetCold", so this may never complete if the OS have
disabled the Local APIC Timer.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v3:
    - cleanup .dsc, leave only one TimerLib resolution
    - Added a note in the commit message regarding the use of the local apic
      by runtime drivers

 OvmfPkg/OvmfXen.dsc | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 9f79d455fa..6288394eb8 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.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

@@ -202,7 +202,6 @@ [LibraryClasses.common]
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf

 

 [LibraryClasses.common.SEC]

-  TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf

   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf

 !ifdef $(DEBUG_ON_SERIAL_PORT)

   DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf

@@ -281,7 +280,6 @@ [LibraryClasses.common.DXE_CORE]
 

 [LibraryClasses.common.DXE_RUNTIME_DRIVER]

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf

   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf

@@ -298,7 +296,6 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
 

 [LibraryClasses.common.UEFI_DRIVER]

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf

   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf

@@ -313,7 +310,6 @@ [LibraryClasses.common.UEFI_DRIVER]
 

 [LibraryClasses.common.DXE_DRIVER]

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf

   ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

@@ -337,7 +333,6 @@ [LibraryClasses.common.DXE_DRIVER]
 

 [LibraryClasses.common.UEFI_APPLICATION]

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

-  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf

   ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

-- 
Anthony PERARD


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43287): https://edk2.groups.io/g/devel/message/43287
Mute This Topic: https://groups.io/mt/32308573/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 09/35] OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU
Posted by Roger Pau Monné 5 years, 4 months ago
On Thu, Jul 04, 2019 at 03:42:07PM +0100, Anthony PERARD wrote:
> ACPI Timer does not work in a PVH guest, but local APIC works on both

This is not accurate. It's not that the ACPI timer doesn't work, it's
just that it's not present. The PM_TMR_BLK should be set to 0 to
indicate the lack of PM timer support, or else there's something
broken.

> PVH and HVM.
> 
> Note that the use of SecPeiDxeTimerLibCpu might be an issue with a
> driver of type DXE_RUNTIME_DRIVER. I've attemptde to find out which of
                                          ^ attempted
> the DXE_RUNTIME_DRIVER uses the TimerLib at runtime. I've done that by
> replacing the TimerLib evaluation in
> [LibraryClasses.common.DXE_RUNTIME_DRIVER] by a different one and
> check every module that uses it (with the --report-file=report build
  ^ checking
> option).
> 
> ResetSystemRuntimeDxe is calling the TimerLib API at runtime to do the
> operation "EfiResetCold", so this may never complete if the OS have
> disabled the Local APIC Timer.

Thanks, Roger.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43719): https://edk2.groups.io/g/devel/message/43719
Mute This Topic: https://groups.io/mt/32308573/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 09/35] OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU
Posted by Anthony PERARD 5 years, 3 months ago
On Mon, Jul 15, 2019 at 04:22:19PM +0200, Roger Pau Monné wrote:
> On Thu, Jul 04, 2019 at 03:42:07PM +0100, Anthony PERARD wrote:
> > ACPI Timer does not work in a PVH guest, but local APIC works on both
> 
> This is not accurate. It's not that the ACPI timer doesn't work, it's
> just that it's not present. The PM_TMR_BLK should be set to 0 to
> indicate the lack of PM timer support, or else there's something
> broken.

I'll reword that first sentence.

OVMF doesn't look at the PM_TMR_BLK value when initializing that timer,
it only looks at the PCI host bridge device ID because OVMF is built
with QEMU in mind and there are only two possibles choices, QEMU is
running with a piix or q35 machine type, I think.

> > PVH and HVM.
> > 
> > Note that the use of SecPeiDxeTimerLibCpu might be an issue with a
> > driver of type DXE_RUNTIME_DRIVER. I've attemptde to find out which of
>                                           ^ attempted
> > the DXE_RUNTIME_DRIVER uses the TimerLib at runtime. I've done that by
> > replacing the TimerLib evaluation in
> > [LibraryClasses.common.DXE_RUNTIME_DRIVER] by a different one and
> > check every module that uses it (with the --report-file=report build
>   ^ checking
> > option).
> > 
> > ResetSystemRuntimeDxe is calling the TimerLib API at runtime to do the
> > operation "EfiResetCold", so this may never complete if the OS have
> > disabled the Local APIC Timer.
> 
> Thanks, Roger.

Thanks,

-- 
Anthony PERARD

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44140): https://edk2.groups.io/g/devel/message/44140
Mute This Topic: https://groups.io/mt/32308573/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 09/35] OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU
Posted by Laszlo Ersek 5 years, 3 months ago
On 07/22/19 15:49, Anthony PERARD wrote:
> On Mon, Jul 15, 2019 at 04:22:19PM +0200, Roger Pau Monné wrote:
>> On Thu, Jul 04, 2019 at 03:42:07PM +0100, Anthony PERARD wrote:
>>> ACPI Timer does not work in a PVH guest, but local APIC works on both
>>
>> This is not accurate. It's not that the ACPI timer doesn't work, it's
>> just that it's not present. The PM_TMR_BLK should be set to 0 to
>> indicate the lack of PM timer support, or else there's something
>> broken.
> 
> I'll reword that first sentence.
> 
> OVMF doesn't look at the PM_TMR_BLK value when initializing that timer,
> it only looks at the PCI host bridge device ID because OVMF is built
> with QEMU in mind and there are only two possibles choices, QEMU is
> running with a piix or q35 machine type, I think.

We should split this statement in two. :)

OVMF doesn't look at ACPI payload because it is a design goal to keep
the guest firmware un-enlightened about such ACPI contents that arrive
from the hypervisor. Parsing ACPI in firmware always looks attractive
until someone actually writes the code, and then it always ends in
misery -- at the latest when people realize they have to parse AML.
Parsing ACPI is only feasible when you have a full-blown ACPICA (or
similar) subsystem, and edk2 doesn't. Therefore, OVMF looks at either
hardware, or specialized paravirt information channels such as fw_cfg
files, that are easy to parse by design.

Second, within the above design guidelines (i.e. "don't try to parse
ACPI", and "cook your paravirt info if you want the firmware to eat
it"), OVMF looks at such artifacts to steer its behavior for which
patches have been submitted & merged. OVMF is integrated with Xen to the
extent of patches merged from the Xen community. Thus, in my opinion,
"OVMF is built with QEMU in mind" is a stretch -- perhaps it is so in
*my* mind personally, but that's just me. We have designated reviewers
for Xen-related code, and this series certainly builds OVMF with Xen in
mind. :)

If we reworded the statement, e.g. as "the present code targets QEMU and
is unsuitable when running on Xen", then I would not object.

Thanks!
Laszlo

>>> PVH and HVM.
>>>
>>> Note that the use of SecPeiDxeTimerLibCpu might be an issue with a
>>> driver of type DXE_RUNTIME_DRIVER. I've attemptde to find out which of
>>                                           ^ attempted
>>> the DXE_RUNTIME_DRIVER uses the TimerLib at runtime. I've done that by
>>> replacing the TimerLib evaluation in
>>> [LibraryClasses.common.DXE_RUNTIME_DRIVER] by a different one and
>>> check every module that uses it (with the --report-file=report build
>>   ^ checking
>>> option).
>>>
>>> ResetSystemRuntimeDxe is calling the TimerLib API at runtime to do the
>>> operation "EfiResetCold", so this may never complete if the OS have
>>> disabled the Local APIC Timer.
>>
>> Thanks, Roger.
> 
> Thanks,
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44148): https://edk2.groups.io/g/devel/message/44148
Mute This Topic: https://groups.io/mt/32308573/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 09/35] OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU
Posted by Roger Pau Monné 5 years, 3 months ago
On Mon, Jul 22, 2019 at 09:28:20PM +0200, Laszlo Ersek wrote:
> On 07/22/19 15:49, Anthony PERARD wrote:
> > On Mon, Jul 15, 2019 at 04:22:19PM +0200, Roger Pau Monné wrote:
> >> On Thu, Jul 04, 2019 at 03:42:07PM +0100, Anthony PERARD wrote:
> >>> ACPI Timer does not work in a PVH guest, but local APIC works on both
> >>
> >> This is not accurate. It's not that the ACPI timer doesn't work, it's
> >> just that it's not present. The PM_TMR_BLK should be set to 0 to
> >> indicate the lack of PM timer support, or else there's something
> >> broken.
> > 
> > I'll reword that first sentence.
> > 
> > OVMF doesn't look at the PM_TMR_BLK value when initializing that timer,
> > it only looks at the PCI host bridge device ID because OVMF is built
> > with QEMU in mind and there are only two possibles choices, QEMU is
> > running with a piix or q35 machine type, I think.
> 
> We should split this statement in two. :)
> 
> OVMF doesn't look at ACPI payload because it is a design goal to keep
> the guest firmware un-enlightened about such ACPI contents that arrive
> from the hypervisor. Parsing ACPI in firmware always looks attractive
> until someone actually writes the code, and then it always ends in
> misery -- at the latest when people realize they have to parse AML.
> Parsing ACPI is only feasible when you have a full-blown ACPICA (or
> similar) subsystem, and edk2 doesn't. Therefore, OVMF looks at either
> hardware, or specialized paravirt information channels such as fw_cfg
> files, that are easy to parse by design.

IMO passing information using such side-channels always looks
attractive at first sight, until you realize at some point later that
you just have ended up with a completely custom interface that
duplicates ACPI. Having that said, Xen manages to get most of what it
needs from static ACPI tables, but I'm not sure if OVMF could manage
to do so also.

Xen has quite a lot of baggage here, like using xenstore/xenbus
instead of PCI, or custom 'start info pages' instead of ACPI, which we
are currently trying to partially move away from when possible.

Thanks, Roger.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44232): https://edk2.groups.io/g/devel/message/44232
Mute This Topic: https://groups.io/mt/32308573/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 09/35] OvmfPkg/OvmfXen: use a TimerLib instance that depends only on the CPU
Posted by Laszlo Ersek 5 years, 4 months ago
On 07/04/19 16:42, Anthony PERARD wrote:
> ACPI Timer does not work in a PVH guest, but local APIC works on both
> PVH and HVM.
> 
> Note that the use of SecPeiDxeTimerLibCpu might be an issue with a
> driver of type DXE_RUNTIME_DRIVER. I've attemptde to find out which of
> the DXE_RUNTIME_DRIVER uses the TimerLib at runtime. I've done that by
> replacing the TimerLib evaluation in
> [LibraryClasses.common.DXE_RUNTIME_DRIVER] by a different one and
> check every module that uses it (with the --report-file=report build
> option).
> 
> ResetSystemRuntimeDxe is calling the TimerLib API at runtime to do the
> operation "EfiResetCold", so this may never complete if the OS have
> disabled the Local APIC Timer.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     v3:
>     - cleanup .dsc, leave only one TimerLib resolution
>     - Added a note in the commit message regarding the use of the local apic
>       by runtime drivers

Good note -- yes, the warning in
"MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf" applies.
I guess Xen setups can live with that potential problem, for now.

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

Thanks
Laszlo

> 
>  OvmfPkg/OvmfXen.dsc | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 9f79d455fa..6288394eb8 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.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
> @@ -202,7 +202,6 @@ [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
>  
>  [LibraryClasses.common.SEC]
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
>  !ifdef $(DEBUG_ON_SERIAL_PORT)
>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> @@ -281,7 +280,6 @@ [LibraryClasses.common.DXE_CORE]
>  
>  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>    DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -298,7 +296,6 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>  
>  [LibraryClasses.common.UEFI_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>    DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
> @@ -313,7 +310,6 @@ [LibraryClasses.common.UEFI_DRIVER]
>  
>  [LibraryClasses.common.DXE_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>    ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> @@ -337,7 +333,6 @@ [LibraryClasses.common.DXE_DRIVER]
>  
>  [LibraryClasses.common.UEFI_APPLICATION]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> -  TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>    ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43372): https://edk2.groups.io/g/devel/message/43372
Mute This Topic: https://groups.io/mt/32308573/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-