|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 4/4] x86/vRTC: support century field
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>
> ---
> 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 think the checking is likely fine.
> 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?
>
> 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.
Is there a reason to propagate the changes to the DM even when SET is
not active?
> Perhaps we ought to also support alarm day/month features?
> ---
> v2: Don't re-purpose pad0 field of struct hvm_hw_rtc.
>
> --- 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 */
> +
> 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%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x, index
> 0x%02x\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
> @@ -482,16 +482,27 @@ static int rtc_ioport_write(void *opaque
> data &= 0x7f;
> s->hw.cmos_index = data;
> spin_unlock(&s->lock);
> - return (data < RTC_CMOS_SIZE);
> + return data < RTC_CMOS_SIZE || (s->has_century && data ==
> RTC_CENTURY);
> }
>
> - 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:
> + if ( s->has_century )
> + {
> + orig = s->hw.century;
> + break;
> + }
> + fallthrough;
> + 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 +518,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 +527,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);
> @@ -591,7 +606,16 @@ static void rtc_set_time(RTCState *s)
> tm->tm_wday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_WEEK]);
> tm->tm_mday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_MONTH]);
> tm->tm_mon = from_bcd(s, s->hw.cmos_data[RTC_MONTH]) - 1;
> - tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]) + 100;
> + tm->tm_year = from_bcd(s, s->hw.cmos_data[RTC_YEAR]);
> + if ( s->has_century )
> + {
> + unsigned int century = s->hw.century;
> +
> + BCD_TO_BIN(century);
> + tm->tm_year += century * 100 - epoch_year;
> + }
> + else
> + tm->tm_year += 100;
>
> after = mktime(get_year(tm->tm_year), tm->tm_mon + 1, tm->tm_mday,
> tm->tm_hour, tm->tm_min, tm->tm_sec);
> @@ -629,6 +653,12 @@ static void rtc_copy_date(RTCState *s)
> s->hw.cmos_data[RTC_DAY_OF_MONTH] = to_bcd(s, tm->tm_mday);
> s->hw.cmos_data[RTC_MONTH] = to_bcd(s, tm->tm_mon + 1);
> s->hw.cmos_data[RTC_YEAR] = to_bcd(s, tm->tm_year % 100);
> +
> + if ( s->has_century )
> + {
> + s->hw.century = get_year(tm->tm_year) / 100;
> + BIN_TO_BCD(s->hw.century);
> + }
> }
>
> static int update_in_progress(RTCState *s)
> @@ -663,13 +693,17 @@ static uint32_t rtc_ioport_read(RTCState
> case RTC_DAY_OF_MONTH:
> case RTC_MONTH:
> case RTC_YEAR:
> + case RTC_CENTURY:
> /* if not in set mode, adjust cmos before reading*/
> if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
> {
> s->current_tm = gmtime(get_localtime(d));
> rtc_copy_date(s);
> }
> - ret = s->hw.cmos_data[s->hw.cmos_index];
> + if ( s->hw.cmos_index != RTC_CENTURY )
> + ret = s->hw.cmos_data[s->hw.cmos_index];
> + else
> + ret = s->hw.century;
> break;
> case RTC_REG_A:
> ret = s->hw.cmos_data[s->hw.cmos_index];
> @@ -718,7 +752,8 @@ static int cf_check handle_rtc_io(
> *val = 0xff;
> return X86EMUL_OKAY;
> }
> - else if ( vrtc->hw.cmos_index < RTC_CMOS_SIZE )
> + else if ( vrtc->hw.cmos_index < RTC_CMOS_SIZE ||
> + (vrtc->has_century && vrtc->hw.cmos_index == RTC_CENTURY) )
> {
> *val = rtc_ioport_read(vrtc);
> return X86EMUL_OKAY;
> @@ -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.
> +
> + return 0;
> +}
> +
> /* Reload the hardware state from a saved domain */
> static int cf_check rtc_load(struct domain *d, hvm_domain_context_t *h)
> {
> @@ -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?
Thanks, Roger.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |