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

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

On Wed, May 13, 2026 at 04:58:57PM +0200, Jan Beulich wrote:
> On 13.05.2026 16:24, Roger Pau Monné wrote:
> > 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@xxxxxxxx>
> >> ---
> >> 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?
> 
> I'm not really up to making an XTF test, I guess. However, as you look to
> share my concern, I'll add a prereq patch adjusting gmtime().
> 
> >> --- 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.
> 
> I first thought I'd do it like this, but (a) this isn't a value Xen
> defines (hence the comments in both places) and (b) I'm not entirely
> happy with such a(n) (ab)use of the public headers (yes, we have other
> such examples there, which I also don't really like).

Yeah, it's not great, but it's better than having the same value
defined in two different files, and having to keep them in-sync for
the CMOS century field to work correctly?

> >> --- 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.
> 
> I can switch to rejecting merely 0. Unlike centuries in the future, it
> didn't look very useful to me to permit anything below 19. Please clarify
> which way you prefer it.

QEMU seems to tolerate everything, so I lean towards tolerating
everything that's not 0.  That's solely based on what QEMU does, which
I think it's likely to be (quite) widely tested.

> >> @@ -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?
> 
> I don't think so. The case of us not handling RTC_CENTURY is dealt with
> earlier in the function. Whereas when we handle the field, we don't want
> to forward (like for all the other RTC fields).

Right, so then you also want to adjust the top part of
rtc_ioport_write() to not propagate the write to the 0x70 IO port when
data is RTC_CENTURY?  Otherwise you propagate the write to port 0x70,
but not the read/write to port 0x71?

Thanks, Roger.