Xen devel - Mar 2012 - [PATCH 3/5] RTC: Add UIP(update in progress) check logic

The UIP(update in progress) is set when RTC is in updating. And the update cycle
begins 244us later after UIP is set.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>

diff -r 47cb862a07c2 -r edc35b026509 xen/arch/x86/hvm/rtc.c
--- a/xen/arch/x86/hvm/rtc.c    Mon Mar 05 14:39:07 2012 +0800
+++ b/xen/arch/x86/hvm/rtc.c    Mon Mar 05 14:39:41 2012 +0800
@@ -28,6 +28,8 @@
 #include <asm/hvm/support.h>
 #include <asm/current.h>

+#define USEC_PER_SEC    1000000UL
+
 #define domain_vrtc(x) (&(x)->arch.hvm_domain.pl_time.vrtc)
 #define vcpu_vrtc(x)   (domain_vrtc((x)->domain))
 #define vrtc_domain(x) (container_of((x), struct domain, \
@@ -239,6 +241,22 @@ static void rtc_copy_date(RTCState *s)
     s->hw.cmos_data[RTC_YEAR] = to_bcd(s, tm->tm_year % 100);
 }

+static int update_in_progress(RTCState *s)
+{
+    uint64_t guest_usec;
+    struct domain *d = vrtc_domain(s);
+
+    if (s->hw.cmos_data[RTC_REG_B] & RTC_SET)
+        return 0;
+
+    guest_usec = get_localtime_us(d);
+    /* UIP bit will be set at last 244us of every second. */
+    if ((guest_usec % USEC_PER_SEC) >= (USEC_PER_SEC - 244))
+        return 1;
+
+    return 0;
+}
+
 static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
 {
     int ret;
@@ -268,6 +286,8 @@ static uint32_t rtc_ioport_read(RTCState
         break;
     case RTC_REG_A:
         ret = s->hw.cmos_data[s->hw.cmos_index];
+        if (update_in_progress(s))
+            ret |= RTC_UIP;
         break;
     case RTC_REG_C:
         ret = s->hw.cmos_data[s->hw.cmos_index];
diff -r 47cb862a07c2 -r edc35b026509 xen/arch/x86/time.c
--- a/xen/arch/x86/time.c       Mon Mar 05 14:39:07 2012 +0800
+++ b/xen/arch/x86/time.c       Mon Mar 05 14:39:41 2012 +0800
@@ -1601,6 +1601,13 @@ unsigned long get_localtime(struct domai
         + d->time_offset_seconds;
 }

+/* Return millisecs after 00:00:00 localtime, 1 January, 1970. */
+uint64_t get_localtime_us(struct domain *d)
+{
+    return ((wc_sec + d->time_offset_seconds) * 1000000000ULL
+        + wc_nsec + NOW()) / 1000UL;
+}
+
 unsigned long get_sec(void)
 {
     return wc_sec + (wc_nsec + NOW()) / 1000000000ULL;
diff -r 47cb862a07c2 -r edc35b026509 xen/include/xen/time.h
--- a/xen/include/xen/time.h    Mon Mar 05 14:39:07 2012 +0800
+++ b/xen/include/xen/time.h    Mon Mar 05 14:39:41 2012 +0800
@@ -34,6 +34,7 @@ typedef s64 s_time_t;

 s_time_t get_s_time(void);
 unsigned long get_localtime(struct domain *d);
+uint64_t get_localtime_us(struct domain *d);

 struct tm {
     int     tm_sec;         /* seconds */

On Mon, 2012-03-05 at 03:25 -0500, Zhang, Yang Z wrote:
> The UIP(update in progress) is set when RTC is in updating. And the
> update cycle begins 244us later after UIP is set. 
Not arguing against this patch but OOI how important is it to emulate
this behaviour to this level of accuracy? On real hardware the UIP tells
you that the data may not be valid/consistent but I don''t think the
emulated RTC has that property (does it?). Do guest OSes rely on seeing
the UIP bit occasionally or have you seen a specific issue which caused
you to make this change?

Ian.

No comment on the rest of the series, but this caught my eye:

At 08:25 +0000 on 05 Mar (1330935936), Zhang, Yang Z
wrote:
> +/* Return millisecs after 00:00:00 localtime, 1 January, 1970. */
> +uint64_t get_localtime_us(struct domain *d)
> +{
> +    return ((wc_sec + d->time_offset_seconds) * 1000000000ULL
> +        + wc_nsec + NOW()) / 1000UL;
> +}
> +
The comment says milliseconds but the code does microseconds.  Which is
right?

Tim.

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Tuesday, March 06, 2012 4:38 AM
> To: Zhang, Yang Z
> Cc: xen-devel@lists.xensource.com; Jan Beulich
> Subject: Re: [Xen-devel] [PATCH 3/5] RTC: Add UIP(update in progress) check
> logic
> 
> No comment on the rest of the series, but this caught my eye:
> 
> At 08:25 +0000 on 05 Mar (1330935936), Zhang, Yang Z wrote:
> > +/* Return millisecs after 00:00:00 localtime, 1 January, 1970. */
> > +uint64_t get_localtime_us(struct domain *d) {
> > +    return ((wc_sec + d->time_offset_seconds) * 1000000000ULL
> > +        + wc_nsec + NOW()) / 1000UL;
> > +}
> > +
> 
> The comment says milliseconds but the code does microseconds.  Which is
> right?
A typo. It should be microseconds not miliseconds.

best regards
yang

Actually, in the draft patch, I also think it is unnecessary. And I
don''t find any real use mode to use RTC in this way.
But someone points out that the RTC datasheet allows us to check the update
cycle by UIP. So we need to support it.

best regards
yang

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
> Sent: Tuesday, March 06, 2012 2:36 AM
> To: Zhang, Yang Z
> Cc: xen-devel@lists.xensource.com; Jan Beulich
> Subject: Re: [Xen-devel] [PATCH 3/5] RTC: Add UIP(update in progress) check
> logic
> 
> On Mon, 2012-03-05 at 03:25 -0500, Zhang, Yang Z wrote:
> > The UIP(update in progress) is set when RTC is in updating. And the
> > update cycle begins 244us later after UIP is set.
> 
> Not arguing against this patch but OOI how important is it to emulate this
> behaviour to this level of accuracy? On real hardware the UIP tells you
that the
> data may not be valid/consistent but I don''t think the emulated
RTC has that
> property (does it?). Do guest OSes rely on seeing the UIP bit occasionally
or have
> you seen a specific issue which caused you to make this change?
> 
> Ian.

>>> On 06.03.12 at 01:26, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Actually, in the draft patch, I also think it is unnecessary. And I
don''t
> find any real use mode to use RTC in this way.
> But someone points out that the RTC datasheet allows us to check the update
> cycle by UIP. So we need to support it.
Doesn''t Xen itself make use of this (and would hence be affected [non-
fatally] when running nested)? See xen/arch/x86/time.c:get_cmos_time().

Jan
>> -----Original Message-----
>> From: Ian Campbell [mailto:Ian.Campbell@citrix.com]
>> Sent: Tuesday, March 06, 2012 2:36 AM
>> To: Zhang, Yang Z
>> Cc: xen-devel@lists.xensource.com; Jan Beulich
>> Subject: Re: [Xen-devel] [PATCH 3/5] RTC: Add UIP(update in progress)
check
>> logic
>> 
>> On Mon, 2012-03-05 at 03:25 -0500, Zhang, Yang Z wrote:
>> > The UIP(update in progress) is set when RTC is in updating. And
the
>> > update cycle begins 244us later after UIP is set.
>> 
>> Not arguing against this patch but OOI how important is it to emulate
this
>> behaviour to this level of accuracy? On real hardware the UIP tells you
that
> the
>> data may not be valid/consistent but I don''t think the
emulated RTC has that
>> property (does it?). Do guest OSes rely on seeing the UIP bit
occasionally
> or have
>> you seen a specific issue which caused you to make this change?
>> 
>> Ian.

On Tue, 2012-03-06 at 03:45 -0500, Jan Beulich wrote:
> >>> On 06.03.12 at 01:26, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> > Actually, in the draft patch, I also think it is unnecessary. And I
don''t
> > find any real use mode to use RTC in this way.
> > But someone points out that the RTC datasheet allows us to check the
update
> > cycle by UIP. So we need to support it.
> 
> Doesn''t Xen itself make use of this (and would hence be affected
[non-
> fatally] when running nested)? See xen/arch/x86/time.c:get_cmos_time().
Yes, looks like it. I imagine this is a pretty common idiom for reading
the RTC (why else would Xen do it that way...) so I think it is fair to
assume that other guest OSes also rely on the UIP bit.

Ian.