Re: [PATCH v2 1/4] x86/time: CMOS RTC may run in binary mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jul 02, 2026 at 11:29:11AM +0200, Jan Beulich wrote:
> Indicating it would always use BCD mode is just wrong (and then the
> comment there said the opposite). All halfway recent (and really all 64-
> bit capable) systems having a CMOS RTC should properly indicate the mode
> in control register B.
> 
> Make use of the flag, but provide a fallback mechanism in case people run
> into systems not matching the above assumption. Additionally, when binary
> mode is indicated and when "cmos-rtc-probe" is in use (but "cmos-rtc-bcd"
> isn't), probe whether the clock really runs in binary mode. (This probing,
> sadly, can take up to 10 seconds.)
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: New.
> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -339,6 +339,14 @@ parameter to "stable:socket".
>  Specify the event count threshold for raising Corrected Machine Check
>  Interrupts.  Specifying zero disables CMCI handling.
>  
> +### cmos-rtc-bcd (x86)
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Flag to indicate the CMOS Real Time Clock uses BCD mode irrespective of
> +control register B indicating binary mode.
> +

Likely too late for it now, but I get the feeling we should have
introduced a cmos option, with rtc-bcd and rtc-probe as boolean sub
options:

cmos = [ rtc-probe, rtc-bcd ]

>  ### cmos-rtc-probe (x86)
>  > `= <boolean>`
>  
> --- a/xen/arch/x86/include/asm/mc146818rtc.h
> +++ b/xen/arch/x86/include/asm/mc146818rtc.h
> @@ -96,7 +96,6 @@ bool is_cmos_port(unsigned int port, uns
>  
>  #ifndef RTC_PORT
>  #define RTC_PORT(x)  (0x70 + (x))
> -#define RTC_ALWAYS_BCD       1       /* RTC operates in binary mode */
>  #endif
>  
>  /*
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1250,6 +1250,9 @@ mktime (unsigned int year, unsigned int
>          )*60 + sec; /* finally seconds */
>  }
>  
> +static bool __ro_after_init opt_cmos_rtc_bcd;
> +boolean_param("cmos-rtc-bcd", opt_cmos_rtc_bcd);
> +
>  struct rtc_time {
>      unsigned int year, mon, day, hour, min, sec;
>  };
> @@ -1285,7 +1288,7 @@ static bool __get_cmos_time(struct rtc_t
>      if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
>          century = CMOS_READ(acpi_gbl_FADT.century);
>  
> -    bcd = RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY);
> +    bcd = opt_cmos_rtc_bcd || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY);
>  
>      spin_unlock_irqrestore(&rtc_lock, flags);
>  
> @@ -1353,6 +1356,48 @@ static bool __init cmos_rtc_probe(void)
>      return false;
>  }
>  
> +static inline bool __init attr_const is_bcd(unsigned int x)
> +{
> +    return (x & 0xf) < 10 && (x >> 4) < 10;
> +}
> +
> +static void __init cmos_rtc_probe_bcd(void)
> +{
> +    bool bcd;
> +    unsigned long flags;
> +
> +    if ( opt_cmos_rtc_bcd )
> +        return;
> +
> +    spin_lock_irqsave(&rtc_lock, flags);
> +    bcd = !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY);
> +    spin_unlock_irqrestore(&rtc_lock, flags);
> +
> +    if ( bcd )
> +        return;
> +
> +    for ( unsigned int seclo = 0; ; )
> +    {
> +        struct rtc_time rtc;
> +
> +        if ( !__get_cmos_time(&rtc) ||
> +             !is_bcd(rtc.sec) ||
> +             !is_bcd(rtc.min) ||
> +             !is_bcd(rtc.hour) ||
> +             !is_bcd(rtc.day) ||
> +             !is_bcd(rtc.mon) )
> +            return;
> +
> +        if ( seclo > (rtc.sec & 0xf) )
> +            break;
> +
> +        seclo = rtc.sec & 0xf;

Is there a risk of this loop triggering the watchdog, and hence we
should process softirqs in the loop? (or otherwise have some kind of
hard loop stop after certain iterations / time)

Oh, I now see the mention in the commit message and also note this is
done ahead of SMP and also ahead of the watchdog being enabled, hence
it can't trigger the watchdog.

Thanks, Roger.