Re: [PATCH v7 2/2] x86/time: prefer CMOS over EFI_GET_TIME

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Sep 13, 2024 at 09:59:07AM +0200, Roger Pau Monne wrote:
> The EFI_GET_TIME implementation is well known to be broken for many firmware
> implementations, for Xen the result on such implementations are:
> 
> ----[ Xen-4.19-unstable  x86_64  debug=y  Tainted:   C    ]----
> CPU:    0
> RIP:    e008:[<0000000062ccfa70>] 0000000062ccfa70
> [...]
> Xen call trace:
>    [<0000000062ccfa70>] R 0000000062ccfa70
>    [<00000000732e9a3f>] S 00000000732e9a3f
>    [<ffff82d04034f34f>] F arch/x86/time.c#get_cmos_time+0x1b3/0x26e
>    [<ffff82d04045926f>] F init_xen_time+0x28/0xa4
>    [<ffff82d040454bc4>] F __start_xen+0x1ee7/0x2578
>    [<ffff82d040203334>] F __high_start+0x94/0xa0
> 
> Pagetable walk from 0000000062ccfa70:
>  L4[0x000] = 000000207ef1c063 ffffffffffffffff
>  L3[0x001] = 000000005d6c0063 ffffffffffffffff
>  L2[0x116] = 8000000062c001e3 ffffffffffffffff (PSE)
> 
> ****************************************
> Panic on CPU 0:
> FATAL PAGE FAULT
> [error_code=0011]
> Faulting linear address: 0000000062ccfa70
> ****************************************
> 
> Swap the preference to default to CMOS first, and EFI later, in an attempt to
> use EFI_GET_TIME as a last resort option only.  Note that Linux for example
> doesn't allow calling the get_time method, and instead provides a dummy 
> handler
> that unconditionally returns EFI_UNSUPPORTED on x86-64.
> 
> Such change in the preferences requires some re-arranging of the function
> logic, so that panic messages with workaround suggestions are suitably 
> printed.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Since this changes behavior for running on EFI,
Acked-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

> ---
> Changes since v2:
>  - Updated to match previous changes.
> ---
>  xen/arch/x86/time.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index e4751684951e..b86e4d58b40c 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1592,14 +1592,14 @@ static void __init probe_wallclock(void)
>          wallclock_source = WALLCLOCK_XEN;
>          return;
>      }
> -    if ( efi_enabled(EFI_RS) && efi_get_time() )
> +    if ( cmos_rtc_probe() )
>      {
> -        wallclock_source = WALLCLOCK_EFI;
> +        wallclock_source = WALLCLOCK_CMOS;
>          return;
>      }
> -    if ( cmos_rtc_probe() )
> +    if ( efi_enabled(EFI_RS) && efi_get_time() )
>      {
> -        wallclock_source = WALLCLOCK_CMOS;
> +        wallclock_source = WALLCLOCK_EFI;
>          return;
>      }
>  
> -- 
> 2.46.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature