Re: [PATCH v2 4/4] x86/vRTC: support century field

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2026 10:07, Roger Pau Monné wrote:
> On Thu, Jul 02, 2026 at 11:31:14AM +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.
>>
>> In the new rtc_check(), besides checking the new fields also check the
>> pre-existing pad0 field.
>>
>> 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>
> 
> Acked-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>

Thanks.

>> Now that we extend struct hvm_hw_rtc, should we perhaps save not only the
>> century, but also its index?
> 
> Hm, possibly for correctness, albeit I think this is unlikely to cause
> issues.  Likely better done in a separate patch?

Possibly. Just that then we need to deal with two different cases of
backwards compatibility. (Not that I think that doing so would be overly
difficult, but still.)

>> Likely more sanity checking could be added to rtc_check(), but that's for
>> a separate patch imo.
>>
>> 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?
> 
> I don't know that much about the RTC TBH.
> 
>> And
>> isn't it excessive overhead to go through rtc_set_time() when the field
>> is updated while SET is clear?
> 
> I think this is done because we don't call rtc_set_time() when RTC_SET
> is activated in RTC_REG_B?  We would need to change the logic a bit.

I think you may have overlooked that my question was just for the day-of-
week field. When that field is changed, rtc_set_time() will call mktime(),
update_domain_wallclock_time(), and send_timeoffset_req() despite the
change not having any effect on the values used/calculated there. The only
actual change in that case is to s->current_tm.tm_wday.

> Is there a reason to propagate the changes to the DM even when SET is
> not active?

How does a DM come into play here? It's only accesses to non-time fields
of the CMOS which we forward to the DM.

>> @@ -760,6 +795,32 @@ static int cf_check rtc_save(struct vcpu
>>      return rc;
>>  }
>>  
>> +static int cf_check rtc_check(const struct domain *d, hvm_domain_context_t 
>> *h)
>> +{
>> +    const struct hvm_save_descriptor *desc =
>> +        (const struct hvm_save_descriptor *)&h->data[h->cur];
>> +    struct hvm_hw_rtc s;
>> +
>> +    if ( !has_vrtc(d) )
>> +        return -ENODEV;
>> +
>> +    if ( hvm_load_entry_zeroextend(RTC, h, &s) != 0 )
>> +        return -ENODATA;
>> +
>> +    if ( s.pad0 )
>> +        return -EINVAL;
>> +
>> +    for ( unsigned int i = 0; i < ARRAY_SIZE(s.pad1); ++i )
>> +        if ( s.pad1[i] )
>> +            return -EINVAL;
>> +
>> +    if ( desc->length >= endof_field(struct hvm_hw_rtc, century) &&
>> +         ((s.century & 0xf) >= 10 || (s.century >> 4) >= 10) )
>> +        return -EINVAL;
> 
> It would be nice to also set ->has_century here, but the s struct is
> a temporary stack allocation.

And deliberately so, as .check() hooks shouldn't alter state. (Hence
also why const struct domain * is passed in.)

>> @@ -793,12 +854,18 @@ static int cf_check rtc_load(struct doma
>>      check_update_timer(s);
>>      alarm_timer_update(s);
>>  
>> +    if ( !s->hw.century )
>> +    {
>> +        s->has_century = false;
>> +        s->hw.century = 0;
> 
> Isn't this last line pointless?  The if condition is !s->hw.century,
> and hence s->hw.century must be 0 here?

Oh, yes - insufficient pruning after the v2 adjustments.

Jan