[PATCH 0/5] x86/time: CMOS RTC century byte

[PATCH 0/5] x86/time: CMOS RTC century byte

[Jan Beulich]

I'm surprised that we didn't consume this thus far, and that we got away with
also not emulating it for HVM guests.

The 1st and 3rd patches are imo candidates for 4.22, whereas the others (more
or less associated cleanup) likely aren't.

1: x86/time: use RTC century byte when available
2: x86/time: move BCD_TO_BIN() uses
3: x86/vRTC: support century field
4: x86/vRTC: use available macros for BCD <-> BIN conversion
5: tools/xen-hvmctx: shorten various format strings a little

Jan

[PATCH for-4.22 1/5] x86/time: use RTC century byte when available

[Jan Beulich]

Without this the present logic will misbehave from 2070 onwards.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1257,6 +1257,7 @@ struct rtc_time {
 static bool __get_cmos_time(struct rtc_time *rtc)
 {
     s_time_t start, t1, t2;
+    unsigned int century = 0;
     unsigned long flags;
 
     spin_lock_irqsave(&rtc_lock, flags);
@@ -1280,6 +1281,8 @@ static bool __get_cmos_time(struct rtc_t
     rtc->day  = CMOS_READ(RTC_DAY_OF_MONTH);
     rtc->mon  = CMOS_READ(RTC_MONTH);
     rtc->year = CMOS_READ(RTC_YEAR);
+    if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
+        century = CMOS_READ(acpi_gbl_FADT.century);
     
     if ( RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) )
     {
@@ -1293,7 +1296,12 @@ static bool __get_cmos_time(struct rtc_t
 
     spin_unlock_irqrestore(&rtc_lock, flags);
 
-    if ( (rtc->year += 1900) < 1970 )
+    if ( century )
+    {
+        BCD_TO_BIN(century);
+        rtc->year += century * 100;
+    }
+    else if ( (rtc->year += 1900) < 1970 )
         rtc->year += 100;
 
     return t1 <= SECONDS(1) && t2 < MILLISECS(3);

[PATCH 2/5] x86/time: move BCD_TO_BIN() uses

[Jan Beulich]

... outside of __get_cmos_time()'s locked region. There's no need to hold
the lock for these computations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
How come RTC_ALWAYS_BCD is compile-time constant 1? And then even with an
inverted comment? Looks like we've inherited this from Linux, and even in
Linus'es current tree it's still this same way. Yet all half-way recent
chipsets I'm aware of properly implement the DM bit in reg B. Might this
be another 32-bit leftover?

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1257,6 +1257,7 @@ struct rtc_time {
 static bool __get_cmos_time(struct rtc_time *rtc)
 {
     s_time_t start, t1, t2;
+    bool bcd;
     unsigned int century = 0;
     unsigned long flags;
 
@@ -1283,8 +1284,12 @@ static bool __get_cmos_time(struct rtc_t
     rtc->year = CMOS_READ(RTC_YEAR);
     if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
         century = CMOS_READ(acpi_gbl_FADT.century);
-    
-    if ( RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) )
+
+    bcd = RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY);
+
+    spin_unlock_irqrestore(&rtc_lock, flags);
+
+    if ( bcd )
     {
         BCD_TO_BIN(rtc->sec);
         BCD_TO_BIN(rtc->min);
@@ -1294,8 +1299,6 @@ static bool __get_cmos_time(struct rtc_t
         BCD_TO_BIN(rtc->year);
     }
 
-    spin_unlock_irqrestore(&rtc_lock, flags);
-
     if ( century )
     {
         BCD_TO_BIN(century);

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

[Jan Beulich]

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

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 */
+
 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) )
+        {
+            /* 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);
@@ -591,7 +617,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 ( is_century(s->hw.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 +664,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 ( is_century(s->hw.century) )
+    {
+        s->hw.century = get_year(tm->tm_year) / 100;
+        BIN_TO_BCD(s->hw.century);
+    }
 }
 
 static int update_in_progress(RTCState *s)
@@ -656,6 +697,13 @@ static uint32_t rtc_ioport_read(RTCState
 
     switch ( s->hw.cmos_index )
     {
+    case RTC_CENTURY:
+        if ( !is_century(s->hw.century) )
+        {
+            ret = UINT32_MAX;
+            break;
+        }
+        fallthrough;
     case RTC_SECONDS:
     case RTC_MINUTES:
     case RTC_HOURS:
@@ -669,7 +717,10 @@ static uint32_t rtc_ioport_read(RTCState
             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,10 +769,12 @@ 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->hw.cmos_index == RTC_CENTURY )
     {
         *val = rtc_ioport_read(vrtc);
-        return X86EMUL_OKAY;
+        if ( *val != UINT32_MAX )
+            return X86EMUL_OKAY;
     }
 
     return X86EMUL_UNHANDLEABLE;
@@ -873,6 +926,8 @@ void rtc_init(struct domain *d)
     s->hw.cmos_data[RTC_REG_C] = 0;
     s->hw.cmos_data[RTC_REG_D] = RTC_VRT;
 
+    s->hw.century = 0x20; /* Arbitrary initial value satisfying is_century() */
+
     s->current_tm = gmtime(get_localtime(d));
     s->start_time = NOW();
 
--- a/xen/arch/x86/include/asm/mc146818rtc.h
+++ b/xen/arch/x86/include/asm/mc146818rtc.h
@@ -37,6 +37,9 @@ bool is_cmos_port(unsigned int port, uns
 #define RTC_REG_C               12
 #define RTC_REG_D               13
 
+/* Conventional index used without (and typically also with) ACPI. */
+#define RTC_CENTURY             0x32
+
 /**********************************************************************
  * register details
  **********************************************************************/
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -485,7 +485,7 @@ struct hvm_hw_rtc {
     uint8_t cmos_data[RTC_CMOS_SIZE];
     /* Index register for 2-part operations */
     uint8_t cmos_index;
-    uint8_t pad0;
+    uint8_t century;
     /* RTC offset from host time */
     int64_t rtc_offset;
 };

[PATCH 4/5] x86/vRTC: use available macros for BCD <-> BIN conversion

[Jan Beulich]

There's no need to open-code these. No functional change intended, even if
the | changes to + in to_bcd().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -253,18 +253,18 @@ static void cf_check rtc_update_timer2(v
 
 static unsigned int to_bcd(const RTCState *s, unsigned int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
-        return a;
+    if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY) )
+        BIN_TO_BCD(a);
 
-    return ((a / 10) << 4) | (a % 10);
+    return a;
 }
 
 static unsigned int from_bcd(const RTCState *s, unsigned int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
-        return a;
+    if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY) )
+        BCD_TO_BIN(a);
 
-    return ((a >> 4) * 10) + (a & 0x0f);
+    return a;
 }
 
 /*

[PATCH 5/5] tools/xen-hvmctx: shorten various format strings a little

[Jan Beulich]

%4.4x and alike format specifiers can be expressed shorter as %04x or, as
e.g. dump_ioapic() has it, %.4x.

In dump_fpu()'s XMM register dumping, also move away from showing bogus
xmm03 and alike. The proper register name is xmm3 for that particular
example.

Also strip trailing whitespace from lines touched.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/misc/xen-hvmctx.c
+++ b/tools/misc/xen-hvmctx.c
@@ -107,26 +107,26 @@ static void dump_fpu(void *p)
     struct fpu_regs *r = p;
     int i;
 
-    printf("    FPU:    fcw 0x%4.4x fsw 0x%4.4x\n"  
-           "            ftw 0x%2.2x (0x%2.2x) fop 0x%4.4x\n"
-           "          fpuip 0x%16.16"PRIx64" fpudp 0x%16.16"PRIx64"\n"
-           "          mxcsr 0x%8.8lx mask 0x%8.8lx\n",
+    printf("    FPU:    fcw 0x%04x fsw 0x%04x\n"
+           "            ftw 0x%02x (0x%02x) fop 0x%04x\n"
+           "          fpuip 0x%016"PRIx64" fpudp 0x%016"PRIx64"\n"
+           "          mxcsr 0x%08lx mask 0x%08lx\n",
            (unsigned)r->fcw, (unsigned)r->fsw, 
            (unsigned)r->ftw, (unsigned)r->res0, (unsigned)r->fop, 
            r->fpuip, r->fpudp, 
            (unsigned long)r->mxcsr, (unsigned long)r->mxcsr_mask);
 
     for ( i = 0 ; i < 8 ; i++ ) 
-        printf("            mm%i 0x%4.4x%16.16"PRIx64" (0x%4.4x%4.4x%4.4x)\n",
+        printf("            mm%i 0x%04x%016"PRIx64" (0x%04x%04x%04x)\n",
                i, r->mm[i].hi, r->mm[i].lo,
                r->mm[i].pad[2], r->mm[i].pad[1], r->mm[i].pad[0]);
 
     for ( i = 0 ; i < 16 ; i++ ) 
-        printf("          xmm%2.2i 0x%16.16"PRIx64"%16.16"PRIx64"\n",
+        printf("          xmm%-2i 0x%016"PRIx64"%016"PRIx64"\n",
                i, r->xmm[i].hi, r->xmm[i].lo);
     
     for ( i = 0 ; i < 6 ; i++ ) 
-        printf("               (0x%16.16"PRIx64"%16.16"PRIx64")\n",
+        printf("               (0x%016"PRIx64"%016"PRIx64")\n",
                r->res1[2*i+1], r->res1[2*i]);
 }
 
@@ -134,20 +134,20 @@ static void dump_cpu(void)
 {
     HVM_SAVE_TYPE(CPU) c;
     READ(c);
-    printf("    CPU:    rax 0x%16.16llx     rbx 0x%16.16llx\n"
-           "            rcx 0x%16.16llx     rdx 0x%16.16llx\n"
-           "            rbp 0x%16.16llx     rsi 0x%16.16llx\n"
-           "            rdi 0x%16.16llx     rsp 0x%16.16llx\n"
-           "             r8 0x%16.16llx      r9 0x%16.16llx\n"
-           "            r10 0x%16.16llx     r11 0x%16.16llx\n"
-           "            r12 0x%16.16llx     r13 0x%16.16llx\n"
-           "            r14 0x%16.16llx     r15 0x%16.16llx\n"
-           "            rip 0x%16.16llx  rflags 0x%16.16llx\n"
-           "            cr0 0x%16.16llx     cr2 0x%16.16llx\n"
-           "            cr3 0x%16.16llx     cr4 0x%16.16llx\n"
-           "            dr0 0x%16.16llx     dr1 0x%16.16llx\n"
-           "            dr2 0x%16.16llx     dr3 0x%16.16llx\n"
-           "            dr6 0x%16.16llx     dr7 0x%16.16llx\n"
+    printf("    CPU:    rax 0x%016llx     rbx 0x%016llx\n"
+           "            rcx 0x%016llx     rdx 0x%016llx\n"
+           "            rbp 0x%016llx     rsi 0x%016llx\n"
+           "            rdi 0x%016llx     rsp 0x%016llx\n"
+           "             r8 0x%016llx      r9 0x%016llx\n"
+           "            r10 0x%016llx     r11 0x%016llx\n"
+           "            r12 0x%016llx     r13 0x%016llx\n"
+           "            r14 0x%016llx     r15 0x%016llx\n"
+           "            rip 0x%016llx  rflags 0x%016llx\n"
+           "            cr0 0x%016llx     cr2 0x%016llx\n"
+           "            cr3 0x%016llx     cr4 0x%016llx\n"
+           "            dr0 0x%016llx     dr1 0x%016llx\n"
+           "            dr2 0x%016llx     dr3 0x%016llx\n"
+           "            dr6 0x%016llx     dr7 0x%016llx\n"
            "             cs %#6.4" PRIx32 " (%#18.8" PRIx64 " + %#10.8" PRIx32 " / %#7.4" PRIx32 ")\n"
            "             es %#6.4" PRIx32 " (%#18.8" PRIx64 " + %#10.8" PRIx32 " / %#7.4" PRIx32 ")\n"
            "             ds %#6.4" PRIx32 " (%#18.8" PRIx64 " + %#10.8" PRIx32 " / %#7.4" PRIx32 ")\n"
@@ -158,12 +158,12 @@ static void dump_cpu(void)
            "           ldtr %#6.4" PRIx32 " (%#18.8" PRIx64 " + %#10.4" PRIx32 " / %#7.4" PRIx32 ")\n"
            "           idtr        (%#18.8" PRIx64 " + %#10.4" PRIx32 ")\n"
            "           gdtr        (%#18.8" PRIx64 " + %#10.4" PRIx32 ")\n"
-           "    sysenter cs 0x%8.8llx  eip 0x%16.16llx  esp 0x%16.16llx\n"
+           "    sysenter cs 0x%08llx  eip 0x%016llx  esp 0x%016llx\n"
            "      shadow gs %#18.16" PRIx64 "   efer %#18.8" PRIx64 "\n"
            "          lstar %#18.16" PRIx64 "  cstar %#18.16" PRIx64 "\n"
            "           star %#18.16" PRIx64 " sfmask %#18.8" PRIx64 "\n"
-           "            tsc 0x%16.16llx\n"
-           "          event 0x%8.8lx error 0x%8.8lx\n",
+           "            tsc 0x%016llx\n"
+           "          event 0x%08lx error 0x%08lx\n",
            (unsigned long long) c.rax, (unsigned long long) c.rbx,
            (unsigned long long) c.rcx, (unsigned long long) c.rdx,
            (unsigned long long) c.rbp, (unsigned long long) c.rsi,
@@ -260,7 +260,7 @@ static void dump_pci_irq(void)
 {
     HVM_SAVE_TYPE(PCI_IRQ) i;
     READ(i);
-    printf("    PCI IRQs: 0x%16.16llx%16.16llx\n", 
+    printf("    PCI IRQs: 0x%016llx%016llx\n",
            (unsigned long long) i.pad[0], (unsigned long long) i.pad[1]);
 }
 
@@ -268,7 +268,7 @@ static void dump_isa_irq(void)
 {
     HVM_SAVE_TYPE(ISA_IRQ) i;
     READ(i);
-    printf("    ISA IRQs: 0x%4.4llx\n", 
+    printf("    ISA IRQs: 0x%04llx\n",
            (unsigned long long) i.pad[0]);
 }
 
@@ -305,10 +305,10 @@ static void dump_rtc(void)
 {
     HVM_SAVE_TYPE(RTC) r;
     READ(r);
-    printf("    RTC: regs 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x\n",
+    printf("    RTC: regs 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x\n",
            r.cmos_data[0], r.cmos_data[1], r.cmos_data[2], r.cmos_data[3], 
            r.cmos_data[4], r.cmos_data[5], r.cmos_data[6], r.cmos_data[7]);
-    printf("              0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x 0x%2.2x, index 0x%2.2x\n",
+    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);

Re: [PATCH for-4.22 1/5] x86/time: use RTC century byte when available

[Roger Pau Monné]

On Tue, May 12, 2026 at 04:58:43PM +0200, Jan Beulich wrote:
> Without this the present logic will misbehave from 2070 onwards.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1257,6 +1257,7 @@ struct rtc_time {
>  static bool __get_cmos_time(struct rtc_time *rtc)
>  {
>      s_time_t start, t1, t2;
> +    unsigned int century = 0;
>      unsigned long flags;
>  
>      spin_lock_irqsave(&rtc_lock, flags);
> @@ -1280,6 +1281,8 @@ static bool __get_cmos_time(struct rtc_t
>      rtc->day  = CMOS_READ(RTC_DAY_OF_MONTH);
>      rtc->mon  = CMOS_READ(RTC_MONTH);
>      rtc->year = CMOS_READ(RTC_YEAR);
> +    if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
> +        century = CMOS_READ(acpi_gbl_FADT.century);
>      
>      if ( RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) )
>      {
> @@ -1293,7 +1296,12 @@ static bool __get_cmos_time(struct rtc_t
>  
>      spin_unlock_irqrestore(&rtc_lock, flags);
>  
> -    if ( (rtc->year += 1900) < 1970 )
> +    if ( century )
> +    {
> +        BCD_TO_BIN(century);

Don't you need to move the BCD_TO_BIN() translation with the rest, so
it's not done unconditionally?

Thanks, Roger.

Re: [PATCH 2/5] x86/time: move BCD_TO_BIN() uses

[Roger Pau Monné]

On Tue, May 12, 2026 at 04:59:03PM +0200, Jan Beulich wrote:
> ... outside of __get_cmos_time()'s locked region. There's no need to hold
> the lock for these computations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

I had the same thought about moving the conversion out of the locked
region when reviewing the previous patch.

As noted in the previous patch, we should move the conversion of the
century field with the rest?

> ---
> How come RTC_ALWAYS_BCD is compile-time constant 1? And then even with an
> inverted comment? Looks like we've inherited this from Linux, and even in
> Linus'es current tree it's still this same way. Yet all half-way recent
> chipsets I'm aware of properly implement the DM bit in reg B. Might this
> be another 32-bit leftover?

*shrugs* I don't know.  Seems like Linux is still doing it, so it's
likely safer for us to continue doing it also?  We had no reports of
it being problematic, albeit one could argue it would be best to
prevent such reports by doing the right thing.

Thanks, Roger.

Re: [PATCH for-4.22 1/5] x86/time: use RTC century byte when available

[Jan Beulich]

On 13.05.2026 10:49, Roger Pau Monné wrote:
> On Tue, May 12, 2026 at 04:58:43PM +0200, Jan Beulich wrote:
>> @@ -1280,6 +1281,8 @@ static bool __get_cmos_time(struct rtc_t
>>      rtc->day  = CMOS_READ(RTC_DAY_OF_MONTH);
>>      rtc->mon  = CMOS_READ(RTC_MONTH);
>>      rtc->year = CMOS_READ(RTC_YEAR);
>> +    if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
>> +        century = CMOS_READ(acpi_gbl_FADT.century);
>>      
>>      if ( RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) )
>>      {
>> @@ -1293,7 +1296,12 @@ static bool __get_cmos_time(struct rtc_t
>>  
>>      spin_unlock_irqrestore(&rtc_lock, flags);
>>  
>> -    if ( (rtc->year += 1900) < 1970 )
>> +    if ( century )
>> +    {
>> +        BCD_TO_BIN(century);
> 
> Don't you need to move the BCD_TO_BIN() translation with the rest, so
> it's not done unconditionally?

No, the century field is always BCD.

Jan

Re: [PATCH 2/5] x86/time: move BCD_TO_BIN() uses

[Jan Beulich]

On 13.05.2026 10:56, Roger Pau Monné wrote:
> On Tue, May 12, 2026 at 04:59:03PM +0200, Jan Beulich wrote:
>> ... outside of __get_cmos_time()'s locked region. There's no need to hold
>> the lock for these computations.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I had the same thought about moving the conversion out of the locked
> region when reviewing the previous patch.
> 
> As noted in the previous patch, we should move the conversion of the
> century field with the rest?

As said there, no, I don't think so.

>> ---
>> How come RTC_ALWAYS_BCD is compile-time constant 1? And then even with an
>> inverted comment? Looks like we've inherited this from Linux, and even in
>> Linus'es current tree it's still this same way. Yet all half-way recent
>> chipsets I'm aware of properly implement the DM bit in reg B. Might this
>> be another 32-bit leftover?
> 
> *shrugs* I don't know.  Seems like Linux is still doing it, so it's
> likely safer for us to continue doing it also?  We had no reports of
> it being problematic, albeit one could argue it would be best to
> prevent such reports by doing the right thing.

That's my point. If we did this as specified, we'd unbreak systems with the
DM bit set correctly, but we'd break (hypothetical) systems with it bogusly
set. Like with a few other fixes, perhaps we should correct it, but provide
a command line option to restore old behavior?

Jan

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

[Roger Pau Monné]

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.

Re: [PATCH 4/5] x86/vRTC: use available macros for BCD <-> BIN conversion

[Roger Pau Monné]

On Tue, May 12, 2026 at 05:00:04PM +0200, Jan Beulich wrote:
> There's no need to open-code these. No functional change intended, even if
> the | changes to + in to_bcd().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH for-4.22 1/5] x86/time: use RTC century byte when available

[Roger Pau Monné]

On Wed, May 13, 2026 at 12:36:26PM +0200, Jan Beulich wrote:
> On 13.05.2026 10:49, Roger Pau Monné wrote:
> > On Tue, May 12, 2026 at 04:58:43PM +0200, Jan Beulich wrote:
> >> @@ -1280,6 +1281,8 @@ static bool __get_cmos_time(struct rtc_t
> >>      rtc->day  = CMOS_READ(RTC_DAY_OF_MONTH);
> >>      rtc->mon  = CMOS_READ(RTC_MONTH);
> >>      rtc->year = CMOS_READ(RTC_YEAR);
> >> +    if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
> >> +        century = CMOS_READ(acpi_gbl_FADT.century);
> >>      
> >>      if ( RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) )
> >>      {
> >> @@ -1293,7 +1296,12 @@ static bool __get_cmos_time(struct rtc_t
> >>  
> >>      spin_unlock_irqrestore(&rtc_lock, flags);
> >>  
> >> -    if ( (rtc->year += 1900) < 1970 )
> >> +    if ( century )
> >> +    {
> >> +        BCD_TO_BIN(century);
> > 
> > Don't you need to move the BCD_TO_BIN() translation with the rest, so
> > it's not done unconditionally?
> 
> No, the century field is always BCD.

Hm, then I guess Linux needs adjusting, as mc146818_set_time() only
converts the century to the BCD format conditionally on the control
register or RTC_ALWAYS_BCD.

I've found several sources only that as you mention also claim the
century value is unconditionally in BCD format.

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/5] x86/time: move BCD_TO_BIN() uses

[Roger Pau Monné]

On Wed, May 13, 2026 at 12:39:46PM +0200, Jan Beulich wrote:
> On 13.05.2026 10:56, Roger Pau Monné wrote:
> > On Tue, May 12, 2026 at 04:59:03PM +0200, Jan Beulich wrote:
> >> ... outside of __get_cmos_time()'s locked region. There's no need to hold
> >> the lock for these computations.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > I had the same thought about moving the conversion out of the locked
> > region when reviewing the previous patch.
> > 
> > As noted in the previous patch, we should move the conversion of the
> > century field with the rest?
> 
> As said there, no, I don't think so.
> 
> >> ---
> >> How come RTC_ALWAYS_BCD is compile-time constant 1? And then even with an
> >> inverted comment? Looks like we've inherited this from Linux, and even in
> >> Linus'es current tree it's still this same way. Yet all half-way recent
> >> chipsets I'm aware of properly implement the DM bit in reg B. Might this
> >> be another 32-bit leftover?
> > 
> > *shrugs* I don't know.  Seems like Linux is still doing it, so it's
> > likely safer for us to continue doing it also?  We had no reports of
> > it being problematic, albeit one could argue it would be best to
> > prevent such reports by doing the right thing.
> 
> That's my point. If we did this as specified, we'd unbreak systems with the
> DM bit set correctly, but we'd break (hypothetical) systems with it bogusly
> set. Like with a few other fixes, perhaps we should correct it, but provide
> a command line option to restore old behavior?

Possibly, but I would do after 4.22 has branched, just in case.

One thing I've noticed, is that Xen don't attempts to set
RTC_DM_BINARY in REG_B, shouldn't it try to set the bit when probing
for the CMOS?  Since it assumes BCD mode it should at least try to set
it?

Thanks, Roger.

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

[Jan Beulich]

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@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?

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

Re: [PATCH 5/5] tools/xen-hvmctx: shorten various format strings a little

[Roger Pau Monné]

On Tue, May 12, 2026 at 05:00:43PM +0200, Jan Beulich wrote:
> %4.4x and alike format specifiers can be expressed shorter as %04x or, as
> e.g. dump_ioapic() has it, %.4x.
> 
> In dump_fpu()'s XMM register dumping, also move away from showing bogus
> xmm03 and alike. The proper register name is xmm3 for that particular
> example.
> 
> Also strip trailing whitespace from lines touched.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH for-4.22 1/5] x86/time: use RTC century byte when available

[Jan Beulich]

On 13.05.2026 16:51, Roger Pau Monné wrote:
> On Wed, May 13, 2026 at 12:36:26PM +0200, Jan Beulich wrote:
>> On 13.05.2026 10:49, Roger Pau Monné wrote:
>>> On Tue, May 12, 2026 at 04:58:43PM +0200, Jan Beulich wrote:
>>>> @@ -1280,6 +1281,8 @@ static bool __get_cmos_time(struct rtc_t
>>>>      rtc->day  = CMOS_READ(RTC_DAY_OF_MONTH);
>>>>      rtc->mon  = CMOS_READ(RTC_MONTH);
>>>>      rtc->year = CMOS_READ(RTC_YEAR);
>>>> +    if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
>>>> +        century = CMOS_READ(acpi_gbl_FADT.century);
>>>>      
>>>>      if ( RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) )
>>>>      {
>>>> @@ -1293,7 +1296,12 @@ static bool __get_cmos_time(struct rtc_t
>>>>  
>>>>      spin_unlock_irqrestore(&rtc_lock, flags);
>>>>  
>>>> -    if ( (rtc->year += 1900) < 1970 )
>>>> +    if ( century )
>>>> +    {
>>>> +        BCD_TO_BIN(century);
>>>
>>> Don't you need to move the BCD_TO_BIN() translation with the rest, so
>>> it's not done unconditionally?
>>
>> No, the century field is always BCD.
> 
> Hm, then I guess Linux needs adjusting, as mc146818_set_time() only
> converts the century to the BCD format conditionally on the control
> register or RTC_ALWAYS_BCD.

Hmm, indeed.

Btw, while looking there I also noticed this

#ifdef CONFIG_ACPI
	if (p.century > 19)
		time->tm_year += (p.century - 19) * 100;
#endif

(relevant to a remark you gave on a later patch in this series).

> I've found several sources only that as you mention also claim the
> century value is unconditionally in BCD format.
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Jan

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

[Roger Pau Monné]

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

Re: [PATCH 2/5] x86/time: move BCD_TO_BIN() uses

[Jan Beulich]

On 13.05.2026 16:58, Roger Pau Monné wrote:
> On Wed, May 13, 2026 at 12:39:46PM +0200, Jan Beulich wrote:
>> On 13.05.2026 10:56, Roger Pau Monné wrote:
>>> On Tue, May 12, 2026 at 04:59:03PM +0200, Jan Beulich wrote:
>>>> ---
>>>> How come RTC_ALWAYS_BCD is compile-time constant 1? And then even with an
>>>> inverted comment? Looks like we've inherited this from Linux, and even in
>>>> Linus'es current tree it's still this same way. Yet all half-way recent
>>>> chipsets I'm aware of properly implement the DM bit in reg B. Might this
>>>> be another 32-bit leftover?
>>>
>>> *shrugs* I don't know.  Seems like Linux is still doing it, so it's
>>> likely safer for us to continue doing it also?  We had no reports of
>>> it being problematic, albeit one could argue it would be best to
>>> prevent such reports by doing the right thing.
>>
>> That's my point. If we did this as specified, we'd unbreak systems with the
>> DM bit set correctly, but we'd break (hypothetical) systems with it bogusly
>> set. Like with a few other fixes, perhaps we should correct it, but provide
>> a command line option to restore old behavior?
> 
> Possibly, but I would do after 4.22 has branched, just in case.

Of course.

> One thing I've noticed, is that Xen don't attempts to set
> RTC_DM_BINARY in REG_B, shouldn't it try to set the bit when probing
> for the CMOS?  Since it assumes BCD mode it should at least try to set
> it?

For one - don't you mean "clear it"? But then - no, that bit is purely
informational aiui. Changing it won't alter what the date/time registers
hold (only how they're updated). Hence by fiddling with it we'd corrupt
information (breaking OSes which properly respect the bit).

Jan

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

[Jan Beulich]

On 13.05.2026 17:14, Roger Pau Monné wrote:
> 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:
>>>> --- 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?

As the values come from the outside, they necessarily need to stay the
way they are (and hence implicitly in sync). If we meant to announce
another value to guests in the FADT we produce (breaking non-ACPI
guests), we then couldn't use RTC_CENTURY in hvm/rtc.c anyway. Instead
we'd have to track and migrate the index to use.

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

Will do.

>>>> @@ -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?

I can't, as whether to forward depends on the data subsequently written.
Propagating the index "just in case" is the only workable model that I
can think of. And as guests can do any number of successive port 70
writes, the DM needs to cope with this anyway.

Jan

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

[Jan Beulich]

On 13.05.2026 16:58, 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@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?
> 
> 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().

While making such a patch, I noticed my flaw in the description above: That
loop walks in granularity of years, so can't have more than about 10k
iterations. Shortening the processing by first going in 400-year steps may
still be worthwhile, but doesn't look to be strictly required.

Jan

Re: [PATCH 2/5] x86/time: move BCD_TO_BIN() uses

[Roger Pau Monné]

On Wed, May 13, 2026 at 05:15:41PM +0200, Jan Beulich wrote:
> On 13.05.2026 16:58, Roger Pau Monné wrote:
> > On Wed, May 13, 2026 at 12:39:46PM +0200, Jan Beulich wrote:
> >> On 13.05.2026 10:56, Roger Pau Monné wrote:
> >>> On Tue, May 12, 2026 at 04:59:03PM +0200, Jan Beulich wrote:
> >>>> ---
> >>>> How come RTC_ALWAYS_BCD is compile-time constant 1? And then even with an
> >>>> inverted comment? Looks like we've inherited this from Linux, and even in
> >>>> Linus'es current tree it's still this same way. Yet all half-way recent
> >>>> chipsets I'm aware of properly implement the DM bit in reg B. Might this
> >>>> be another 32-bit leftover?
> >>>
> >>> *shrugs* I don't know.  Seems like Linux is still doing it, so it's
> >>> likely safer for us to continue doing it also?  We had no reports of
> >>> it being problematic, albeit one could argue it would be best to
> >>> prevent such reports by doing the right thing.
> >>
> >> That's my point. If we did this as specified, we'd unbreak systems with the
> >> DM bit set correctly, but we'd break (hypothetical) systems with it bogusly
> >> set. Like with a few other fixes, perhaps we should correct it, but provide
> >> a command line option to restore old behavior?
> > 
> > Possibly, but I would do after 4.22 has branched, just in case.
> 
> Of course.
> 
> > One thing I've noticed, is that Xen don't attempts to set
> > RTC_DM_BINARY in REG_B, shouldn't it try to set the bit when probing
> > for the CMOS?  Since it assumes BCD mode it should at least try to set
> > it?
> 
> For one - don't you mean "clear it"? But then - no, that bit is purely
> informational aiui. Changing it won't alter what the date/time registers
> hold (only how they're updated). Hence by fiddling with it we'd corrupt
> information (breaking OSes which properly respect the bit).

Yes, sorry, clear it.  The (possibly very outdated) specification I
have contains:

DM – The data mode (DM) bit indicates whether time
and calendar updates are to use binary or BCD formats. The
DM bit is written by the processor program and maybe read
by the program, but is not modified by any internal functions
or RESET. A "1" in DM signifies binary data, while a "0" in
DM specifies binary-coded-decimal (BCD) data.

To me the "DM bit is written by the processor program" reads as if it
could be set by the OS, but maybe that just means the bit is writable,
but it doesn't affect the format of the field really.

Thanks, Roger.

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

[Roger Pau Monné]

On Wed, May 13, 2026 at 05:24:13PM +0200, Jan Beulich wrote:
> On 13.05.2026 17:14, Roger Pau Monné wrote:
> > 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:
> >>>> --- 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?
> 
> As the values come from the outside, they necessarily need to stay the
> way they are (and hence implicitly in sync). If we meant to announce
> another value to guests in the FADT we produce (breaking non-ACPI
> guests), we then couldn't use RTC_CENTURY in hvm/rtc.c anyway. Instead
> we'd have to track and migrate the index to use.

Hm, OK, so the value is well-known enough to be considered part of the
standard CMOS index layout, I guess that's fair enough.

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

We need to keep 0 as the sentinel invalid value, because that's the
content of the pad field in the structure when not supported.
Otherwise we could just use 0xff (or any other invalid) BCD value.

> >>>> @@ -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?
> 
> I can't, as whether to forward depends on the data subsequently written.
> Propagating the index "just in case" is the only workable model that I
> can think of. And as guests can do any number of successive port 70
> writes, the DM needs to cope with this anyway.

Hm, I see, the newly written value to port 0x71 is taken into account
to decide whether to forward to the DM or not.

I wonder: would it be simpler to extend the size of the hvm_hw_rtc
structure so that Xen can detect whether the incoming VM has support
for the century field, and then avoid having to play heuristics
with the value itself?  We would know ahead of starting the guest
whether RTC_CENTURY is supposed to be handled by Xen or forwarded to
the DM, and we won't be limited to use 0 as the sentinel value for not
exposing RTC_CENTURY.

Thanks, Roger.

Re: [PATCH for-4.22 1/5] x86/time: use RTC century byte when available

[Oleksii Kurochko]

On 5/12/26 4:58 PM, Jan Beulich wrote:
> Without this the present logic will misbehave from 2070 onwards.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Generally it looks like 4.22 won't be used in 2070 or higher (I am 
curious do we have similar use cases now that very old Xen version is 
used nowadays?) but the patch looks pretty straightforward:
  Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1257,6 +1257,7 @@ struct rtc_time {
>   static bool __get_cmos_time(struct rtc_time *rtc)
>   {
>       s_time_t start, t1, t2;
> +    unsigned int century = 0;
>       unsigned long flags;
>   
>       spin_lock_irqsave(&rtc_lock, flags);
> @@ -1280,6 +1281,8 @@ static bool __get_cmos_time(struct rtc_t
>       rtc->day  = CMOS_READ(RTC_DAY_OF_MONTH);
>       rtc->mon  = CMOS_READ(RTC_MONTH);
>       rtc->year = CMOS_READ(RTC_YEAR);
> +    if ( acpi_gbl_FADT.century && acpi_gbl_FADT.century < 0x80 )
> +        century = CMOS_READ(acpi_gbl_FADT.century);
>       
>       if ( RTC_ALWAYS_BCD || !(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) )
>       {
> @@ -1293,7 +1296,12 @@ static bool __get_cmos_time(struct rtc_t
>   
>       spin_unlock_irqrestore(&rtc_lock, flags);
>   
> -    if ( (rtc->year += 1900) < 1970 )
> +    if ( century )
> +    {
> +        BCD_TO_BIN(century);
> +        rtc->year += century * 100;
> +    }
> +    else if ( (rtc->year += 1900) < 1970 )
>           rtc->year += 100;
>   
>       return t1 <= SECONDS(1) && t2 < MILLISECS(3);
> 

Re: [PATCH 2/5] x86/time: move BCD_TO_BIN() uses

[Jan Beulich]

On 13.05.2026 21:08, Roger Pau Monné wrote:
> On Wed, May 13, 2026 at 05:15:41PM +0200, Jan Beulich wrote:
>> On 13.05.2026 16:58, Roger Pau Monné wrote:
>>> On Wed, May 13, 2026 at 12:39:46PM +0200, Jan Beulich wrote:
>>>> On 13.05.2026 10:56, Roger Pau Monné wrote:
>>>>> On Tue, May 12, 2026 at 04:59:03PM +0200, Jan Beulich wrote:
>>>>>> ---
>>>>>> How come RTC_ALWAYS_BCD is compile-time constant 1? And then even with an
>>>>>> inverted comment? Looks like we've inherited this from Linux, and even in
>>>>>> Linus'es current tree it's still this same way. Yet all half-way recent
>>>>>> chipsets I'm aware of properly implement the DM bit in reg B. Might this
>>>>>> be another 32-bit leftover?
>>>>>
>>>>> *shrugs* I don't know.  Seems like Linux is still doing it, so it's
>>>>> likely safer for us to continue doing it also?  We had no reports of
>>>>> it being problematic, albeit one could argue it would be best to
>>>>> prevent such reports by doing the right thing.
>>>>
>>>> That's my point. If we did this as specified, we'd unbreak systems with the
>>>> DM bit set correctly, but we'd break (hypothetical) systems with it bogusly
>>>> set. Like with a few other fixes, perhaps we should correct it, but provide
>>>> a command line option to restore old behavior?
>>>
>>> Possibly, but I would do after 4.22 has branched, just in case.
>>
>> Of course.
>>
>>> One thing I've noticed, is that Xen don't attempts to set
>>> RTC_DM_BINARY in REG_B, shouldn't it try to set the bit when probing
>>> for the CMOS?  Since it assumes BCD mode it should at least try to set
>>> it?
>>
>> For one - don't you mean "clear it"? But then - no, that bit is purely
>> informational aiui. Changing it won't alter what the date/time registers
>> hold (only how they're updated). Hence by fiddling with it we'd corrupt
>> information (breaking OSes which properly respect the bit).
> 
> Yes, sorry, clear it.  The (possibly very outdated) specification I
> have contains:
> 
> DM – The data mode (DM) bit indicates whether time
> and calendar updates are to use binary or BCD formats. The
> DM bit is written by the processor program and maybe read
> by the program, but is not modified by any internal functions
> or RESET. A "1" in DM signifies binary data, while a "0" in
> DM specifies binary-coded-decimal (BCD) data.
> 
> To me the "DM bit is written by the processor program" reads as if it
> could be set by the OS, but maybe that just means the bit is writable,
> but it doesn't affect the format of the field really.

Well, it does affect the format of the field, but not right when the bit
is written. Aiui it will have an effect when the next update cycle runs,
as then the right kind of arithmetic (BCD or binary) need to be applied
by the chip (or what was a separate chip back at the time when those
specs were written). (The other "written by the processor program" of
course applies when date/time are fully updated. At that point the OS
can pick DM to its liking, and it would then better store the date/time
fields in the respective format.)

Jan

Re: [PATCH for-4.22 1/5] x86/time: use RTC century byte when available

[Jan Beulich]

On 14.05.2026 09:27, Oleksii Kurochko wrote:
> On 5/12/26 4:58 PM, Jan Beulich wrote:
>> Without this the present logic will misbehave from 2070 onwards.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Generally it looks like 4.22 won't be used in 2070 or higher (I am 
> curious do we have similar use cases now that very old Xen version is 
> used nowadays?) but the patch looks pretty straightforward:
>   Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks. The concern towards 4.22 isn't year 2070, but that we're over 25
years late in doing this work (field introduction which was associated
with Y2K iirc).

Jan

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

[Jan Beulich]

On 13.05.2026 21:40, Roger Pau Monné wrote:
> On Wed, May 13, 2026 at 05:24:13PM +0200, Jan Beulich wrote:
>> On 13.05.2026 17:14, Roger Pau Monné wrote:
>>> 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:
>>>>>> @@ -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?
>>
>> I can't, as whether to forward depends on the data subsequently written.
>> Propagating the index "just in case" is the only workable model that I
>> can think of. And as guests can do any number of successive port 70
>> writes, the DM needs to cope with this anyway.
> 
> Hm, I see, the newly written value to port 0x71 is taken into account
> to decide whether to forward to the DM or not.
> 
> I wonder: would it be simpler to extend the size of the hvm_hw_rtc
> structure so that Xen can detect whether the incoming VM has support
> for the century field, and then avoid having to play heuristics
> with the value itself?

It would surely be possible (and we may need to do so anyway for the alarm
date/month fields), but I wanted to get away without doing so here. And it
seemed pretty reasonable to leverage the padding field for this.

>  We would know ahead of starting the guest
> whether RTC_CENTURY is supposed to be handled by Xen or forwarded to
> the DM, and we won't be limited to use 0 as the sentinel value for not
> exposing RTC_CENTURY.

These are the positive aspects. The negative one is that "backwards"
migration would break with the larger record size. Whereas with the padding
field used, it won't: The guest likely won't notice that the value at 0x32
isn't updated anymore, i.e. stays at value 20. Somebody would need to
artificially change time across century boundaries for this to become
noticeable. (Note that this aspect is different for alarm date/month: If
either of those fields was in use, backwards migration indeed needs to
fail.)

Jan

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

[Roger Pau Monné]

On Fri, May 15, 2026 at 08:52:03AM +0200, Jan Beulich wrote:
> On 13.05.2026 21:40, Roger Pau Monné wrote:
> > On Wed, May 13, 2026 at 05:24:13PM +0200, Jan Beulich wrote:
> >> On 13.05.2026 17:14, Roger Pau Monné wrote:
> >>> 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:
> >>>>>> @@ -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?
> >>
> >> I can't, as whether to forward depends on the data subsequently written.
> >> Propagating the index "just in case" is the only workable model that I
> >> can think of. And as guests can do any number of successive port 70
> >> writes, the DM needs to cope with this anyway.
> > 
> > Hm, I see, the newly written value to port 0x71 is taken into account
> > to decide whether to forward to the DM or not.
> > 
> > I wonder: would it be simpler to extend the size of the hvm_hw_rtc
> > structure so that Xen can detect whether the incoming VM has support
> > for the century field, and then avoid having to play heuristics
> > with the value itself?
> 
> It would surely be possible (and we may need to do so anyway for the alarm
> date/month fields), but I wanted to get away without doing so here. And it
> seemed pretty reasonable to leverage the padding field for this.
> 
> >  We would know ahead of starting the guest
> > whether RTC_CENTURY is supposed to be handled by Xen or forwarded to
> > the DM, and we won't be limited to use 0 as the sentinel value for not
> > exposing RTC_CENTURY.
> 
> These are the positive aspects. The negative one is that "backwards"
> migration would break with the larger record size. Whereas with the padding
> field used, it won't: The guest likely won't notice that the value at 0x32
> isn't updated anymore, i.e. stays at value 20.

Sorry, maybe I'm confused, but when migrating backwards the access
won't be handled by Xen anymore, and hence we don't know what it would
return, it's up to the catch-all device model.  I don't think it's
safe to migrate backwards, as the century value won't be preserved
correctly.  IOW: we might want to actively prevent such scenario?

Thanks, Roger.

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

[Jan Beulich]

On 15.05.2026 10:42, Roger Pau Monné wrote:
> On Fri, May 15, 2026 at 08:52:03AM +0200, Jan Beulich wrote:
>> On 13.05.2026 21:40, Roger Pau Monné wrote:
>>> On Wed, May 13, 2026 at 05:24:13PM +0200, Jan Beulich wrote:
>>>> On 13.05.2026 17:14, Roger Pau Monné wrote:
>>>>> 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:
>>>>>>>> @@ -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?
>>>>
>>>> I can't, as whether to forward depends on the data subsequently written.
>>>> Propagating the index "just in case" is the only workable model that I
>>>> can think of. And as guests can do any number of successive port 70
>>>> writes, the DM needs to cope with this anyway.
>>>
>>> Hm, I see, the newly written value to port 0x71 is taken into account
>>> to decide whether to forward to the DM or not.
>>>
>>> I wonder: would it be simpler to extend the size of the hvm_hw_rtc
>>> structure so that Xen can detect whether the incoming VM has support
>>> for the century field, and then avoid having to play heuristics
>>> with the value itself?
>>
>> It would surely be possible (and we may need to do so anyway for the alarm
>> date/month fields), but I wanted to get away without doing so here. And it
>> seemed pretty reasonable to leverage the padding field for this.
>>
>>>  We would know ahead of starting the guest
>>> whether RTC_CENTURY is supposed to be handled by Xen or forwarded to
>>> the DM, and we won't be limited to use 0 as the sentinel value for not
>>> exposing RTC_CENTURY.
>>
>> These are the positive aspects. The negative one is that "backwards"
>> migration would break with the larger record size. Whereas with the padding
>> field used, it won't: The guest likely won't notice that the value at 0x32
>> isn't updated anymore, i.e. stays at value 20.
> 
> Sorry, maybe I'm confused, but when migrating backwards the access
> won't be handled by Xen anymore, and hence we don't know what it would
> return, it's up to the catch-all device model.

Oh, you're right of course. The value returned would be whatever the DM
deems appropriate.

Jan

Re: [PATCH 5/5] tools/xen-hvmctx: shorten various format strings a little

[Anthony PERARD]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On Tue, May 12, 2026 at 05:00:43PM +0200, Jan Beulich wrote:
> %4.4x and alike format specifiers can be expressed shorter as %04x or, as
> e.g. dump_ioapic() has it, %.4x.
> 
> In dump_fpu()'s XMM register dumping, also move away from showing bogus
> xmm03 and alike. The proper register name is xmm3 for that particular
> example.
> 
> Also strip trailing whitespace from lines touched.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech