[PATCH] x86/time: Don't use EFI's GetTime call by default

[PATCH] x86/time: Don't use EFI's GetTime call by default

[Ross Lagerwall]

When EFI is used, don't use EFI's GetTime() to get the time, because it
is broken on many platforms. From Linux commit 7efe665903d0 ("rtc:
Disable EFI rtc for x86"):
"Disable it explicitly for x86 so that we don't give users false
hope that this driver will work - it won't, and your machine is likely
to crash."

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/time.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 30d52c4..28895c2 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -679,20 +679,28 @@ static void __get_cmos_time(struct rtc_time *rtc)
         rtc->year += 100;
 }
 
+/* EFI's GetTime() is frequently broken so don't use it by default. */
+#undef USE_EFI_GET_TIME
+
 static unsigned long get_cmos_time(void)
 {
-    unsigned long res, flags;
+#ifdef USE_EFI_GET_TIME
+    unsigned long res;
+#endif
+    unsigned long flags;
     struct rtc_time rtc;
     unsigned int seconds = 60;
     static bool_t __read_mostly cmos_rtc_probe;
     boolean_param("cmos-rtc-probe", cmos_rtc_probe);
 
+#ifdef USE_EFI_GET_TIME
     if ( efi_enabled )
     {
         res = efi_get_time();
         if ( res )
             return res;
     }
+#endif
 
     if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
         cmos_rtc_probe = 0;
-- 
2.4.3

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[David Vrabel]

On 01/12/15 16:57, Ross Lagerwall wrote:
> When EFI is used, don't use EFI's GetTime() to get the time, because it
> is broken on many platforms.
[...]
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -679,20 +679,28 @@ static void __get_cmos_time(struct rtc_time *rtc)
>          rtc->year += 100;
>  }
>  
> +/* EFI's GetTime() is frequently broken so don't use it by default. */
> +#undef USE_EFI_GET_TIME
> +
>  static unsigned long get_cmos_time(void)
>  {
> -    unsigned long res, flags;
> +#ifdef USE_EFI_GET_TIME
> +    unsigned long res;
> +#endif

You could move this res into the if ( efi_enabled ) below.

> +    unsigned long flags;
>      struct rtc_time rtc;
>      unsigned int seconds = 60;
>      static bool_t __read_mostly cmos_rtc_probe;
>      boolean_param("cmos-rtc-probe", cmos_rtc_probe);
>  
> +#ifdef USE_EFI_GET_TIME
>      if ( efi_enabled )
>      {
>          res = efi_get_time();
>          if ( res )
>              return res;
>      }
> +#endif

David

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Jan Beulich]

>>> On 01.12.15 at 17:57, <ross.lagerwall@citrix.com> wrote:
> When EFI is used, don't use EFI's GetTime() to get the time, because it
> is broken on many platforms. From Linux commit 7efe665903d0 ("rtc:
> Disable EFI rtc for x86"):
> "Disable it explicitly for x86 so that we don't give users false
> hope that this driver will work - it won't, and your machine is likely
> to crash."
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

NAK, since being conceptually wrong (and both of my systems work
fine). Vendors should get their firmware fixed, and by not using
runtime service functions we would give them even less reason to
do so. Until then we have "efi=no-rs".

Jan

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Andrew Cooper]

On 01/12/15 17:26, Jan Beulich wrote:
>>>> On 01.12.15 at 17:57, <ross.lagerwall@citrix.com> wrote:
>> When EFI is used, don't use EFI's GetTime() to get the time, because it
>> is broken on many platforms. From Linux commit 7efe665903d0 ("rtc:
>> Disable EFI rtc for x86"):
>> "Disable it explicitly for x86 so that we don't give users false
>> hope that this driver will work - it won't, and your machine is likely
>> to crash."
>>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> NAK, since being conceptually wrong (and both of my systems work
> fine). Vendors should get their firmware fixed, and by not using
> runtime service functions we would give them even less reason to
> do so. Until then we have "efi=no-rs".

This is completely unreasonable.

It is not conceptually wrong.  GetTime() is very well known completely
broken, especially after ExitBootServices(), to the point that every
other EFI implementation (including windows) completely avoids it.

The fact that your two systems don't crash immediately is curious, but
they are not a representative of systems in general.  Not a single
broadwell or skylake platform I have access to boots in EFI mode if
GetTime() is used (which include 4 different manufactures).

Vendors will not fix their firmware.  Disabling all runtime services is
not a reasonable alternative.

This is a firmware bug just like many others and needs to be worked
around by default like others.

Anything else is actively damaging to the Xen community.  People just
get frustrated when it doesn't work (especially if the problem has been
identify and a fix rejected upstream) and will move elsewhere instead.

Any situation where a command line override is required to make Xen boot
is a bug in Xen and should be fixed.  This is why we have __init time
quirks. it doesn't matter if we have some truly horrendous workarounds;
Xen needs to be able to boot by default wherever possible.

~Andrew

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Jan Beulich]

>>> On 01.12.15 at 20:26, <andrew.cooper3@citrix.com> wrote:
> On 01/12/15 17:26, Jan Beulich wrote:
>>>>> On 01.12.15 at 17:57, <ross.lagerwall@citrix.com> wrote:
>>> When EFI is used, don't use EFI's GetTime() to get the time, because it
>>> is broken on many platforms. From Linux commit 7efe665903d0 ("rtc:
>>> Disable EFI rtc for x86"):
>>> "Disable it explicitly for x86 so that we don't give users false
>>> hope that this driver will work - it won't, and your machine is likely
>>> to crash."
>>>
>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> NAK, since being conceptually wrong (and both of my systems work
>> fine). Vendors should get their firmware fixed, and by not using
>> runtime service functions we would give them even less reason to
>> do so. Until then we have "efi=no-rs".
> 
> This is completely unreasonable.
> 
> It is not conceptually wrong.

I'm sorry, but no. Otherwise you mean to state that specifications
don't even need to be written, since if people don't play by them,
workarounds will get implemented anyway.

>  GetTime() is very well known completely
> broken, especially after ExitBootServices(), to the point that every
> other EFI implementation (including windows) completely avoids it.

I'm not sure about the order of things. I started working with EFI on
IA64, where using runtime services is a must. Windows started
using EFI on IA64 too, so I'm pretty convinced they used GetTime()
there. Whether they didn't on x86 because x86 implementations
were broken, or whether x86 implementations end up broken
because Windows never used those functions is simply an unknown.

> The fact that your two systems don't crash immediately is curious, but
> they are not a representative of systems in general.  Not a single
> broadwell or skylake platform I have access to boots in EFI mode if
> GetTime() is used (which include 4 different manufactures).

The original x86 implementation we used almost 15 years back
(on top of systems not even supporting EFI, since there simply were
none back then) didn't have this problem either.

> Vendors will not fix their firmware.  Disabling all runtime services is
> not a reasonable alternative.

Then we should see about adding support for "efi=no-time".

> This is a firmware bug just like many others and needs to be worked
> around by default like others.

Except that you propose to work around it even when there is nothing
to be worked around. I wouldn't mind switching on the workaround
for known broken systems.

> Anything else is actively damaging to the Xen community.  People just
> get frustrated when it doesn't work (especially if the problem has been
> identify and a fix rejected upstream) and will move elsewhere instead.

I'd leave specifying (or switching on by source patching) default
workarounds to distros then. They know what their customers need,
or what systems they expect their stuff to be run on.

> Any situation where a command line override is required to make Xen boot
> is a bug in Xen and should be fixed.  This is why we have __init time
> quirks. it doesn't matter if we have some truly horrendous workarounds;
> Xen needs to be able to boot by default wherever possible.

But requiring command line overrides for other things that are
considered to not work by default is fine? I'm thinking of XenServer's
universal disabling of XSAVE here...

Jan

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Ian Campbell]

On Wed, 2015-12-02 at 02:33 -0700, Jan Beulich wrote:
> > > > On 01.12.15 at 20:26, <andrew.cooper3@citrix.com> wrote:
> > On 01/12/15 17:26, Jan Beulich wrote:
> > > > > > On 01.12.15 at 17:57, <ross.lagerwall@citrix.com> wrote:
> > > > When EFI is used, don't use EFI's GetTime() to get the time,
> > > > because it
> > > > is broken on many platforms. From Linux commit 7efe665903d0 ("rtc:
> > > > Disable EFI rtc for x86"):
> > > > "Disable it explicitly for x86 so that we don't give users false
> > > > hope that this driver will work - it won't, and your machine is
> > > > likely
> > > > to crash."
> > > > 
> > > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > NAK, since being conceptually wrong (and both of my systems work
> > > fine). Vendors should get their firmware fixed, and by not using
> > > runtime service functions we would give them even less reason to
> > > do so. Until then we have "efi=no-rs".
> > 
> > This is completely unreasonable.
> > 
> > It is not conceptually wrong.
> 
> I'm sorry, but no. Otherwise you mean to state that specifications
> don't even need to be written, since if people don't play by them,
> workarounds will get implemented anyway.

In an ideal world such specifications would indeed be worth the paper they
are written on. But the reality is that firmware implementations are often
complete rubbish and the Xen project has nowhere near the leverage needed
to actually get this fixed, even Linux lacks such leverage AFAICT.

In reality only Microsoft (and perhaps to a lesser extent Apple) have any
sort of ability to force things in this way (even so it is alleged in this
thread that even Windows avoids the GetTime call as too broken in the field
as well).

Given we have no leverage in this it seems to me that punishing our users
by ensuring that Xen won't work on the majority of EFI systems (based on
anecdotal evidence from both Andrew and yourself) is counter productive.
All it means is that at best we will get a continuous stream of bug reports
from such users, or worse those users will just silently disappear and use
something else which Just Works on their hardware and tell their friends
that Xen is broken on modern hardware.

There is only the smallest chance they will actually report a bug to their
firmware vendor and even if they do there is basically an infinitesimal
chance that the vendor will care in the slightest given that Windows
presumably boots and works on that system.

Realistically about all we can do is support efforts such as
    http://biosbits.org/
https://wiki.ubuntu.com/Kernel/Reference/fwts
https://01.org/linux-uefi-validation
and hope that they gain sufficient momentum, we certainly cannot effect any
kind of change in the x86 firmware world directly ourselves by holding
basic Xen functionality to ransom.

> >  GetTime() is very well known completely
> > broken, especially after ExitBootServices(), to the point that every
> > other EFI implementation (including windows) completely avoids it.
> 
> I'm not sure about the order of things. I started working with EFI on
> IA64, where using runtime services is a must. Windows started
> using EFI on IA64 too, so I'm pretty convinced they used GetTime()
> there. Whether they didn't on x86 because x86 implementations
> were broken, or whether x86 implementations end up broken
> because Windows never used those functions is simply an unknown.

I don't think it is relevant to use why x86 UEFI implementations are broken
in this way, just that it seems that the reality is that they mostly are.

> > Vendors will not fix their firmware.  Disabling all runtime services is
> > not a reasonable alternative.
> 
> Then we should see about adding support for "efi=no-time".

And based on what I'm reading in this thread about the reliability of the
time RS in the field it seems to me we should make it the default (on x86
at least) and provide efi=time to opt in.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Konrad Rzeszutek Wilk]

On Tue, Dec 08, 2015 at 11:02:16AM +0000, Ian Campbell wrote:
> On Wed, 2015-12-02 at 02:33 -0700, Jan Beulich wrote:
> > > > > On 01.12.15 at 20:26, <andrew.cooper3@citrix.com> wrote:
> > > On 01/12/15 17:26, Jan Beulich wrote:
> > > > > > > On 01.12.15 at 17:57, <ross.lagerwall@citrix.com> wrote:
> > > > > When EFI is used, don't use EFI's GetTime() to get the time,
> > > > > because it
> > > > > is broken on many platforms. From Linux commit 7efe665903d0 ("rtc:
> > > > > Disable EFI rtc for x86"):
> > > > > "Disable it explicitly for x86 so that we don't give users false
> > > > > hope that this driver will work - it won't, and your machine is
> > > > > likely
> > > > > to crash."
> > > > > 
> > > > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > > > NAK, since being conceptually wrong (and both of my systems work
> > > > fine). Vendors should get their firmware fixed, and by not using
> > > > runtime service functions we would give them even less reason to
> > > > do so. Until then we have "efi=no-rs".
> > > 
> > > This is completely unreasonable.
> > > 
> > > It is not conceptually wrong.
> > 
> > I'm sorry, but no. Otherwise you mean to state that specifications
> > don't even need to be written, since if people don't play by them,
> > workarounds will get implemented anyway.
> 
> In an ideal world such specifications would indeed be worth the paper they
> are written on. But the reality is that firmware implementations are often
> complete rubbish and the Xen project has nowhere near the leverage needed
> to actually get this fixed, even Linux lacks such leverage AFAICT.
> 
> In reality only Microsoft (and perhaps to a lesser extent Apple) have any
> sort of ability to force things in this way (even so it is alleged in this
> thread that even Windows avoids the GetTime call as too broken in the field
> as well).

We could confirm this right? We can run OVMF and instrument the 'GetTime' and
see what Windows does?
> 
> Given we have no leverage in this it seems to me that punishing our users
> by ensuring that Xen won't work on the majority of EFI systems (based on
> anecdotal evidence from both Andrew and yourself) is counter productive.
> All it means is that at best we will get a continuous stream of bug reports
> from such users, or worse those users will just silently disappear and use
> something else which Just Works on their hardware and tell their friends
> that Xen is broken on modern hardware.
> 
> There is only the smallest chance they will actually report a bug to their
> firmware vendor and even if they do there is basically an infinitesimal
> chance that the vendor will care in the slightest given that Windows
> presumably boots and works on that system.

Plus I have had no luck actually finding a way to report this.
Take for example my Lenovo hardware which has EFI holes everywhere..

> 
> Realistically about all we can do is support efforts such as
>     http://biosbits.org/
> https://wiki.ubuntu.com/Kernel/Reference/fwts
> https://01.org/linux-uefi-validation
> and hope that they gain sufficient momentum, we certainly cannot effect any
> kind of change in the x86 firmware world directly ourselves by holding
> basic Xen functionality to ransom.
> 
> > >  GetTime() is very well known completely
> > > broken, especially after ExitBootServices(), to the point that every
> > > other EFI implementation (including windows) completely avoids it.
> > 
> > I'm not sure about the order of things. I started working with EFI on
> > IA64, where using runtime services is a must. Windows started
> > using EFI on IA64 too, so I'm pretty convinced they used GetTime()
> > there. Whether they didn't on x86 because x86 implementations
> > because Windows never used those functions is simply an unknown.
> 
> I don't think it is relevant to use why x86 UEFI implementations are broken
> in this way, just that it seems that the reality is that they mostly are.

At least on Lenovo equipment they seem to assume a lot of things
after ExitBootServicers - especially as we don't follow the same mechanism
of switching to virtual addresses.

(And Lenovo EFI code will blindly use virtual addresses after ExitBootServices
which is why hack allows me to run Xen on Lenovo equipment).
> 
> > > Vendors will not fix their firmware.  Disabling all runtime services is
> > > not a reasonable alternative.
> > 
> > Then we should see about adding support for "efi=no-time".
> 
> And based on what I'm reading in this thread about the reliability of the
> time RS in the field it seems to me we should make it the default (on x86
> at least) and provide efi=time to opt in.
> 
> Ian.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Jan Beulich]

>>> On 14.12.15 at 17:53, <konrad.wilk@oracle.com> wrote:
> On Tue, Dec 08, 2015 at 11:02:16AM +0000, Ian Campbell wrote:
>> In reality only Microsoft (and perhaps to a lesser extent Apple) have any
>> sort of ability to force things in this way (even so it is alleged in this
>> thread that even Windows avoids the GetTime call as too broken in the field
>> as well).
> 
> We could confirm this right? We can run OVMF and instrument the 'GetTime' 
> and
> see what Windows does?

I have no doubt MS doesn't use it. I believe though that it's broken
in many case because they don't, not that they don't because it's
broken.

Jan

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Jan Beulich]

>>> On 08.12.15 at 12:02, <ian.campbell@citrix.com> wrote:
> On Wed, 2015-12-02 at 02:33 -0700, Jan Beulich wrote:
>> Then we should see about adding support for "efi=no-time".
> 
> And based on what I'm reading in this thread about the reliability of the
> time RS in the field it seems to me we should make it the default (on x86
> at least) and provide efi=time to opt in.

Which would get us to actively violate at least early versions of the
spec (the current one doesn't appear to be as strict anymore). I'm
fine making it easy to work around the issue, but I'm against us doing
the wrong thing by default.

Apart from that, Ross, the patch you provided breaks on systems
without CMOS clock (e.g. so called legacy free ones). This definitely
can't be a compile time thing.

Jan

Re: [PATCH] x86/time: Don't use EFI's GetTime call by default

[Jan Beulich]

>>> On 15.12.15 at 15:50, <konrad.wilk@oracle.com> wrote:
> On Tue, Dec 15, 2015 at 12:19:45AM -0700, Jan Beulich wrote:
>> >>> On 08.12.15 at 12:02, <ian.campbell@citrix.com> wrote:
>> > On Wed, 2015-12-02 at 02:33 -0700, Jan Beulich wrote:
>> >> Then we should see about adding support for "efi=no-time".
>> > 
>> > And based on what I'm reading in this thread about the reliability of the
>> > time RS in the field it seems to me we should make it the default (on x86
>> > at least) and provide efi=time to opt in.
>> 
>> Which would get us to actively violate at least early versions of the
>> spec (the current one doesn't appear to be as strict anymore). I'm
>> fine making it easy to work around the issue, but I'm against us doing
>> the wrong thing by default.
> 
> Are you OK with DMI quirks?

Sure, why not?

Jan