Re: [PATCH for-4.22 3/5] x86/vRTC: support century field

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, May 12, 2026 at 04:59:35PM +0200, Jan Beulich wrote:
> Both ROMBIOS and SeaBIOS (with CONFIG_QEMU=y, as we build it) blindly
> assume availability of this field (at its conventional index 0x32); OVMF
> at least has code to inspect FADT. Hence we ought to have supported it
> virtually forever.
> 
> As the index is beyond RTC_CMOS_SIZE, leverage the padding field in
> struct hvm_hw_rtc to hold its value. Update the field only when involved
> values are valid BCD century specifiers. Otherwise (for VMs migrated in
> from an older hypervisor) leave handling to the DM.
> 
> This makes the Linux rtc-cmos driver report y3k compatibility.
> 
> While extending xen-hvmctx.c:dump_rtc() also add RTC offset there.
> 
> Fixes: 4ca161214355 ("[HVM] Move RTC emulation into the hypervisor")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Am I overly paranoid with the checking of the field, considering that
> Xen 3.x post-dates year 2000 and hence all firmware nowadays usable guests
> have ever run with should have been aware of the field? Or am I, quite the
> opposite, still not strict enough?
> 
> I can't help the impression that this introduces a latency issue for
> the 2nd of gmtime()'s while() loops: We now allow years up into the 99th
> century, i.e. over 8000 years away from 1970. 8000 years are very roughly
> 2^^38 seconds, making for (again very roughly) 5 million iterations there.
> Did I get my math wrong, or do we need a prereq change to (vastly) reduce
> the number of iterations of that loop (e.g. along the lines of the other
> one, first going in 400 year steps)?

Hm, maybe we need to add some XTF testing for the RTC?  I'm slightly
worried how much time this could take, and since those calls are
serialized on the s->lock I wonder whether enough parallel accesses
from the guest could manage to trigger the watchdog?

> 
> Isn't day-of-week handling flawed? If the field is brought out of sync
> with the other values, shouldn't it stay respectively out-of-sync? And
> isn't it excessive overhead to go through rtc_set_time() when the field
> is updated while SET is clear?
> 
> Perhaps we ought to also support alarm day/month features?
> 
> --- a/tools/libacpi/static_tables.c
> +++ b/tools/libacpi/static_tables.c
> @@ -33,6 +33,8 @@ struct acpi_20_facs Facs = {
>  #define ACPI_PM_TMR_BLK_BIT_WIDTH           0x20
>  #define ACPI_PM_TMR_BLK_BIT_OFFSET          0x00
>  
> +#define CMOS_CENTURY 0x32 /* Conventional index used also without ACPI */

IMO this define (together with the RTC_CENTURY one below) need to be
in a public header so it can be consumed by both the hypervisor and
the toolstack.  Having two separate defines, one for the hypervisor,
and another for the toolstack will just create confusion.

> +
>  struct acpi_fadt Fadt = {
>      .header = {
>          .signature    = ACPI_FADT_SIGNATURE,
> @@ -88,7 +90,9 @@ struct acpi_fadt Fadt = {
>          .register_bit_width  = ACPI_PM_TMR_BLK_BIT_WIDTH,
>          .register_bit_offset = ACPI_PM_TMR_BLK_BIT_OFFSET,
>          .address             = ACPI_PM_TMR_BLK_ADDRESS_V1,
> -    }
> +    },
> +
> +    .century = CMOS_CENTURY,
>  };
>  
>  struct acpi_20_rsdt Rsdt = {
> --- a/tools/misc/xen-hvmctx.c
> +++ b/tools/misc/xen-hvmctx.c
> @@ -311,7 +311,7 @@ static void dump_rtc(void)
>      printf("              0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x, index 0x%2.2x\n",
>             r.cmos_data[8], r.cmos_data[9], r.cmos_data[10], r.cmos_data[11], 
>             r.cmos_data[12], r.cmos_data[13], r.cmos_index);
> -
> +    printf("         century 0x%02x  offset %"PRId64"\n", r.century, r.rtc_offset);
>  }
>  
>  static void dump_hpet(void)
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -47,6 +47,12 @@
>  #define epoch_year     1900
>  #define get_year(x)    ((x) + epoch_year)
>  
> +static inline bool is_century(unsigned int x)
> +{
> +    /* Constant below should match epoch_year above, just as BCD value. */
> +    return x >= 0x19 && (x & 0xf) < 10 && (x >> 4) < 10;
> +}
> +
>  enum rtc_mode {
>     rtc_mode_no_ack,
>     rtc_mode_strict
> @@ -482,16 +488,32 @@ static int rtc_ioport_write(void *opaque
>          data &= 0x7f;
>          s->hw.cmos_index = data;
>          spin_unlock(&s->lock);
> +        /* RTC_CENTURY always forwarded to DM. */
>          return (data < RTC_CMOS_SIZE);
>      }
>  
> -    if ( s->hw.cmos_index >= RTC_CMOS_SIZE )
> +    switch ( s->hw.cmos_index )
>      {
> +    case 0 ... RTC_CMOS_SIZE - 1:
> +        orig = s->hw.cmos_data[s->hw.cmos_index];
> +        break;
> +
> +    case RTC_CENTURY:
> +        orig = s->hw.century;
> +        if ( !is_century(orig) || !is_century(data) )

Is a real RTC strict in such a way, ie: will it refuse to set the
century value to < 19 (0x19)?  For example QEMU seems to be way more
relaxed, and allow any century value.

> +        {
> +            /* Prevent further use of the field. */
> +            s->hw.century = 0;
> +            spin_unlock(&s->lock);
> +            return 0;
> +        }
> +        break;
> +
> +    default:
>          spin_unlock(&s->lock);
>          return 0;
>      }
>  
> -    orig = s->hw.cmos_data[s->hw.cmos_index];
>      switch ( s->hw.cmos_index )
>      {
>      case RTC_SECONDS_ALARM:
> @@ -507,6 +529,7 @@ static int rtc_ioport_write(void *opaque
>      case RTC_DAY_OF_MONTH:
>      case RTC_MONTH:
>      case RTC_YEAR:
> +    case RTC_CENTURY:
>          /* if in set mode, just write the register */
>          if ( (s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
>              s->hw.cmos_data[s->hw.cmos_index] = data;
> @@ -515,7 +538,10 @@ static int rtc_ioport_write(void *opaque
>              /* Fetch the current time and update just this field. */
>              s->current_tm = gmtime(get_localtime(d));
>              rtc_copy_date(s);
> -            s->hw.cmos_data[s->hw.cmos_index] = data;
> +            if ( s->hw.cmos_index != RTC_CENTURY )
> +                s->hw.cmos_data[s->hw.cmos_index] = data;
> +            else
> +                s->hw.century = data;
>              rtc_set_time(s);
>          }
>          alarm_timer_update(s);

Don't you need to adjust the tail return of rtc_ioport_write() (below
the context here) to return 0 when s->hw.cmos_index == RTC_CENTURY, so
the set value is also propagated to the DM, and not only the index?

Thanks, Roger.