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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

>> --- 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.

>> @@ -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).

Jan