[v4,35/35] OvmfPkg/OvmfXen: use RealTimeClockRuntimeDxe from EmbeddedPkg

Commit Message

Anthony PERARD July 29, 2019, 3:39 p.m. UTC

A Xen PVH guest doesn't have a RTC that OVMF would expect, so
PcatRealTimeClockRuntimeDxe fails to initialize and prevent the
firmware from finish to boot. To prevent that, we will use
XenRealTimeClockLib which simply always return the same time.
This will work on both Xen PVH and HVM guests.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
Signed-off-by: Anthony PERARD <[e-mail]>
Acked-by: Laszlo Ersek <[e-mail]>
---

Notes:
    v3:
    - moved RealTimeClockLib|*/XenRealTimeClockLib.inf to the global
      [LibraryClasses]

 OvmfPkg/OvmfXen.dsc | 3 ++-
 OvmfPkg/OvmfXen.fdf | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

Comments

Roger Pau Monné Aug. 7, 2019, 4:09 p.m. UTC | #1

On Mon, Jul 29, 2019 at 04:39:44PM +0100, Anthony PERARD wrote:
> A Xen PVH guest doesn't have a RTC that OVMF would expect, so
> PcatRealTimeClockRuntimeDxe fails to initialize and prevent the
> firmware from finish to boot. To prevent that, we will use
> XenRealTimeClockLib which simply always return the same time.
> This will work on both Xen PVH and HVM guests.

Not that this is not correct, but what's the point in requiring a
clock if it can be faked by always returning the same value?

It seems like it's usage is not really required, and could indeed be
dropped altogether?

Alternatively, there's the PV clock which is available to PVH guests
and will return a proper time.

Thanks, Roger.

Anthony PERARD Aug. 8, 2019, 2:03 p.m. UTC | #2

On Wed, Aug 07, 2019 at 06:09:57PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:44PM +0100, Anthony PERARD wrote:
> > A Xen PVH guest doesn't have a RTC that OVMF would expect, so
> > PcatRealTimeClockRuntimeDxe fails to initialize and prevent the
> > firmware from finish to boot. To prevent that, we will use
> > XenRealTimeClockLib which simply always return the same time.
> > This will work on both Xen PVH and HVM guests.
> 
> Not that this is not correct, but what's the point in requiring a
> clock if it can be faked by always returning the same value?

It's not a clock that is required, it is a library that implements
RealTimeClockLib. Something needs it, so it is provided, even if it is
only to display the "current time".

> It seems like it's usage is not really required, and could indeed be
> dropped altogether?

Way to much work to drop it. Also, I don't work to fork OVMF.

The ARM implementation of OVMF for Xen does the same thing and simply
always return the same value.

> Alternatively, there's the PV clock which is available to PVH guests
> and will return a proper time.

We might need to do that one day I guess, but right now it is just a
nice to have.

Thanks,

Roger Pau Monné Aug. 8, 2019, 3:19 p.m. UTC | #3

On Thu, Aug 08, 2019 at 03:03:48PM +0100, Anthony PERARD wrote:
> On Wed, Aug 07, 2019 at 06:09:57PM +0200, Roger Pau Monné wrote:
> > On Mon, Jul 29, 2019 at 04:39:44PM +0100, Anthony PERARD wrote:
> > > A Xen PVH guest doesn't have a RTC that OVMF would expect, so
> > > PcatRealTimeClockRuntimeDxe fails to initialize and prevent the
> > > firmware from finish to boot. To prevent that, we will use
> > > XenRealTimeClockLib which simply always return the same time.
> > > This will work on both Xen PVH and HVM guests.
> > 
> > Not that this is not correct, but what's the point in requiring a
> > clock if it can be faked by always returning the same value?
> 
> It's not a clock that is required, it is a library that implements
> RealTimeClockLib. Something needs it, so it is provided, even if it is
> only to display the "current time".
> 
> > It seems like it's usage is not really required, and could indeed be
> > dropped altogether?
> 
> Way to much work to drop it. Also, I don't work to fork OVMF.
> 
> The ARM implementation of OVMF for Xen does the same thing and simply
> always return the same value.
> 
> > Alternatively, there's the PV clock which is available to PVH guests
> > and will return a proper time.
> 
> We might need to do that one day I guess, but right now it is just a
> nice to have.

Ack, thanks for the explanation.

Roger.

Patch

diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 5e07b37279..5a31f75f05 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -199,6 +199,7 @@  [LibraryClasses]
 
   Tcg2PhysicalPresenceLib|OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
+  RealTimeClockLib|OvmfPkg/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf
 
 [LibraryClasses.common]
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -564,7 +565,7 @@  [Components]
   }
   MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
   MdeModulePkg/Universal/Metronome/Metronome.inf
-  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf
+  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
   MdeModulePkg/Universal/DriverHealthManagerDxe/DriverHealthManagerDxe.inf
   MdeModulePkg/Universal/BdsDxe/BdsDxe.inf {
     <LibraryClasses>
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index 517a492f14..e6e9e184ef 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -307,7 +307,7 @@  [FV.DXEFV]
 INF  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
 INF  MdeModulePkg/Universal/ResetSystemRuntimeDxe/ResetSystemRuntimeDxe.inf
 INF  MdeModulePkg/Universal/Metronome/Metronome.inf
-INF  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf
+INF  EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf
 
 INF  OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf
 INF  OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf