Xen devel - May 2013 - [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

From: David Vrabel <david.vrabel@citrix.com>

If NTP is used in dom0 and it is synchronized to its clock source,
then the kernel will periodically synchronize the Xen wallclock with
the system time.  Updates to the Xen wallclock do not persist across
reboots, so also synchronize the CMOS RTC (as on bare metal).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/time.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index a1947ac..4656165 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -14,6 +14,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/math64.h>
 #include <linux/gfp.h>
+#include <linux/mc146818rtc.h>
 
 #include <asm/pvclock.h>
 #include <asm/xen/hypervisor.h>
@@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
 static int xen_set_wallclock(const struct timespec *now)
 {
 	struct xen_platform_op op;
+	int ret;
 
 	/* do nothing for domU */
 	if (!xen_initial_domain())
 		return -1;
 
+	/* Set the Xen wallclock. */
 	op.cmd = XENPF_settime;
 	op.u.settime.secs = now->tv_sec;
 	op.u.settime.nsecs = now->tv_nsec;
 	op.u.settime.system_time = xen_clocksource_read();
 
-	return HYPERVISOR_dom0_op(&op);
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return ret;
+
+	/* Set the hardware RTC. */
+	return mach_set_rtc_mmss(now);
+
 }
 
 static struct clocksource xen_clocksource __read_mostly = {
-- 
1.7.2.5

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> If NTP is used in dom0 and it is synchronized to its clock source,
> then the kernel will periodically synchronize the Xen wallclock with
> the system time.  Updates to the Xen wallclock do not persist across
> reboots, so also synchronize the CMOS RTC (as on bare metal).
Sorry again, not getting this one either.

So normally in this case we''re using the Xen wallclock as the
underlying
source for the persistent_clock here, my understanding is we use this 
instead of the standard cmos, because we get benefits of using the 
hypervisor''s sense of time instead of the bare hardware, and allows for
virtualization of the persistent clock.

But the problem is that even if Dom0 tries to set the xen persistent 
clock, it doesn''t actually update anything in the underlying hardware?
So here you instead try to sync the underlying hardware cmos from the 
same Xen dom0 environment?

Honestly, it seems a little strange to me. If you''re running as dom0, 
why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
its virtualizing? This seems to mess with the proper virtualization 
layering.

I''d appreciate some more details and maybe some feedback from other xen
folks here to make sure this really makes sense.

thanks
-john

>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   11 ++++++++++-
>   1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..4656165 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/mc146818rtc.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>   static int xen_set_wallclock(const struct timespec *now)
>   {
>   	struct xen_platform_op op;
> +	int ret;
>   
>   	/* do nothing for domU */
>   	if (!xen_initial_domain())
>   		return -1;
>   
> +	/* Set the Xen wallclock. */
>   	op.cmd = XENPF_settime;
>   	op.u.settime.secs = now->tv_sec;
>   	op.u.settime.nsecs = now->tv_nsec;
>   	op.u.settime.system_time = xen_clocksource_read();
>   
> -	return HYPERVISOR_dom0_op(&op);
> +	ret = HYPERVISOR_dom0_op(&op);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the hardware RTC. */
> +	return mach_set_rtc_mmss(now);
> +
>   }
>   
>   static struct clocksource xen_clocksource __read_mostly = {

>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org>
wrote:
> Honestly, it seems a little strange to me. If you''re running as
dom0,
> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
> its virtualizing? This seems to mess with the proper virtualization 
> layering.
Thy hypervisor tries to control as little system and peripheral devices
as possible, and the CMOS (including the clock) is among those not
controlled by it, but by Dom0.

Jan

On 14/05/13 01:52, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
> 
> Sorry again, not getting this one either.
> 
> So normally in this case we''re using the Xen wallclock as the
underlying
> source for the persistent_clock here, my understanding is we use this 
> instead of the standard cmos, because we get benefits of using the 
> hypervisor''s sense of time instead of the bare hardware, and
allows for
> virtualization of the persistent clock.
> 
> But the problem is that even if Dom0 tries to set the xen persistent 
> clock, it doesn''t actually update anything in the underlying
hardware?
> So here you instead try to sync the underlying hardware cmos from the 
> same Xen dom0 environment?
Yes.
> Honestly, it seems a little strange to me. If you''re running as
dom0,
> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
> its virtualizing? This seems to mess with the proper virtualization 
> layering.
As Jan says the hypervisor only drives a minimal set of hardware,
everything else is made accessible to dom0 for it to control.

I think this makes sense as it allows us to reuse the existing RTC
drivers etc. in the Linux kernel, instead of having to reimplement them
in the hypervisor.

David

On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 02:52, John Stultz
<john.stultz@linaro.org> wrote:
>> Honestly, it seems a little strange to me. If you''re running
as dom0,
>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos
>> its virtualizing? This seems to mess with the proper virtualization
>> layering.
> Thy hypervisor tries to control as little system and peripheral devices
> as possible, and the CMOS (including the clock) is among those not
> controlled by it, but by Dom0.
Huh. So what does calling HYPERVISOR_dom0_op do then?

Either way, I don''t have the context to insightfully review this, so 
I''ll want this to get some Acked-bys from additional Xen folks before I
queue it.

thanks
-john

>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org>
wrote:
> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>> On 14.05.13 at 02:52, John Stultz
<john.stultz@linaro.org> wrote:
>>> Honestly, it seems a little strange to me. If you''re
running as dom0,
>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the
cmos
>>> its virtualizing? This seems to mess with the proper virtualization
>>> layering.
>> Thy hypervisor tries to control as little system and peripheral devices
>> as possible, and the CMOS (including the clock) is among those not
>> controlled by it, but by Dom0.
> 
> Huh. So what does calling HYPERVISOR_dom0_op do then?
Here it merely tells the hypervisor that the wall clock changed (so
it can propagate this on to DomU-s).

Jan

On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 17:59, John Stultz
<john.stultz@linaro.org> wrote:
>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>> On 14.05.13 at 02:52, John Stultz
<john.stultz@linaro.org> wrote:
>>>> Honestly, it seems a little strange to me. If you''re
running as dom0,
>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set
the cmos
>>>> its virtualizing? This seems to mess with the proper
virtualization
>>>> layering.
>>> Thy hypervisor tries to control as little system and peripheral
devices
>>> as possible, and the CMOS (including the clock) is among those not
>>> controlled by it, but by Dom0.
>> Huh. So what does calling HYPERVISOR_dom0_op do then?
> Here it merely tells the hypervisor that the wall clock changed (so
> it can propagate this on to DomU-s).
Ok, I appreciate the explanation. Still waiting for acks on this one.

thanks
-john

John Stultz <john.stultz@linaro.org> wrote:
>On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>>> On 14.05.13 at 17:59, John Stultz
<john.stultz@linaro.org> wrote:
>>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>>> On 14.05.13 at 02:52, John Stultz
<john.stultz@linaro.org>
>wrote:
>>>>> Honestly, it seems a little strange to me. If
you''re running as
>dom0,
>>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to
set the
>cmos
>>>>> its virtualizing? This seems to mess with the proper
>virtualization
>>>>> layering.
>>>> Thy hypervisor tries to control as little system and peripheral
>devices
>>>> as possible, and the CMOS (including the clock) is among those
not
>>>> controlled by it, but by Dom0.
>>> Huh. So what does calling HYPERVISOR_dom0_op do then?
>> Here it merely tells the hypervisor that the wall clock changed (so
>> it can propagate this on to DomU-s).
>
>Ok, I appreciate the explanation. Still waiting for acks on this one.
>
>thanks
>-john
Acked-by: Konrad Rzeszutek Wilk

Or Reviewed-by. Or John if you would like I can carry this in my branch for
Linus.

Thought it might make sense to add the comment from Jan about the CMOS and the
follow up explanation.
-- 
Sent from my Android phone. Please excuse my brevity.

On 05/14/2013 09:24 AM, Konrad Rzeszutek Wilk wrote:
> John Stultz <john.stultz@linaro.org> wrote:
>
>> On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>>>> On 14.05.13 at 17:59, John Stultz
<john.stultz@linaro.org> wrote:
>>>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>>>> On 14.05.13 at 02:52, John Stultz
<john.stultz@linaro.org>
>> wrote:
>>>>>> Honestly, it seems a little strange to me. If
you''re running as
>> dom0,
>>>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor
to set the
>> cmos
>>>>>> its virtualizing? This seems to mess with the proper
>> virtualization
>>>>>> layering.
>>>>> Thy hypervisor tries to control as little system and
peripheral
>> devices
>>>>> as possible, and the CMOS (including the clock) is among
those not
>>>>> controlled by it, but by Dom0.
>>>> Huh. So what does calling HYPERVISOR_dom0_op do then?
>>> Here it merely tells the hypervisor that the wall clock changed (so
>>> it can propagate this on to DomU-s).
>> Ok, I appreciate the explanation. Still waiting for acks on this one.
>>
>> thanks
>> -john
> Acked-by: Konrad Rzeszutek Wilk
>
> Or Reviewed-by. Or John if you would like I can carry this in my branch for
Linus.
>
> Thought it might make sense to add the comment from Jan about the CMOS and
the follow up explanation.
I''m fine queuing it, but yea, the commit will need some more context.

thanks
-john

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> If NTP is used in dom0 and it is synchronized to its clock source,
> then the kernel will periodically synchronize the Xen wallclock with
> the system time.  Updates to the Xen wallclock do not persist across
> reboots, so also synchronize the CMOS RTC (as on bare metal).
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   11 ++++++++++-
>   1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..4656165 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/mc146818rtc.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>   static int xen_set_wallclock(const struct timespec *now)
>   {
>   	struct xen_platform_op op;
> +	int ret;
>   
>   	/* do nothing for domU */
>   	if (!xen_initial_domain())
>   		return -1;
>   
> +	/* Set the Xen wallclock. */
>   	op.cmd = XENPF_settime;
>   	op.u.settime.secs = now->tv_sec;
>   	op.u.settime.nsecs = now->tv_nsec;
>   	op.u.settime.system_time = xen_clocksource_read();
>   
> -	return HYPERVISOR_dom0_op(&op);
> +	ret = HYPERVISOR_dom0_op(&op);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the hardware RTC. */
> +	return mach_set_rtc_mmss(now);
Sorry, just noticed one more thing while applying this. Do all Xen 
systems run on hardware that has the conventional CMOS clock?

What happens if the bare-metal needs to use efi_set_rtc() or 
vrtc_set_mmss() ?

thanks
-john

On 14/05/13 18:24, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/xen/time.c |   11 ++++++++++-
>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>> index a1947ac..4656165 100644
>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/kernel_stat.h>
>>   #include <linux/math64.h>
>>   #include <linux/gfp.h>
>> +#include <linux/mc146818rtc.h>
>>   
>>   #include <asm/pvclock.h>
>>   #include <asm/xen/hypervisor.h>
>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec
*now)
>>   static int xen_set_wallclock(const struct timespec *now)
>>   {
>>   	struct xen_platform_op op;
>> +	int ret;
>>   
>>   	/* do nothing for domU */
>>   	if (!xen_initial_domain())
>>   		return -1;
>>   
>> +	/* Set the Xen wallclock. */
>>   	op.cmd = XENPF_settime;
>>   	op.u.settime.secs = now->tv_sec;
>>   	op.u.settime.nsecs = now->tv_nsec;
>>   	op.u.settime.system_time = xen_clocksource_read();
>>   
>> -	return HYPERVISOR_dom0_op(&op);
>> +	ret = HYPERVISOR_dom0_op(&op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set the hardware RTC. */
>> +	return mach_set_rtc_mmss(now);
> 
> Sorry, just noticed one more thing while applying this. Do all Xen 
> systems run on hardware that has the conventional CMOS clock?
> 
> What happens if the bare-metal needs to use efi_set_rtc() or 
> vrtc_set_mmss() ?
Ug. I hadn''t considered that.  I''ll have think some more on
this.

David

On 05/14/2013 11:00 AM, David Vrabel wrote:
> On 14/05/13 18:24, John Stultz wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> If NTP is used in dom0 and it is synchronized to its clock source,
>>> then the kernel will periodically synchronize the Xen wallclock
with
>>> the system time.  Updates to the Xen wallclock do not persist
across
>>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>> ---
>>>    arch/x86/xen/time.c |   11 ++++++++++-
>>>    1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>>> index a1947ac..4656165 100644
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/kernel_stat.h>
>>>    #include <linux/math64.h>
>>>    #include <linux/gfp.h>
>>> +#include <linux/mc146818rtc.h>
>>>    
>>>    #include <asm/pvclock.h>
>>>    #include <asm/xen/hypervisor.h>
>>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec
*now)
>>>    static int xen_set_wallclock(const struct timespec *now)
>>>    {
>>>    	struct xen_platform_op op;
>>> +	int ret;
>>>    
>>>    	/* do nothing for domU */
>>>    	if (!xen_initial_domain())
>>>    		return -1;
>>>    
>>> +	/* Set the Xen wallclock. */
>>>    	op.cmd = XENPF_settime;
>>>    	op.u.settime.secs = now->tv_sec;
>>>    	op.u.settime.nsecs = now->tv_nsec;
>>>    	op.u.settime.system_time = xen_clocksource_read();
>>>    
>>> -	return HYPERVISOR_dom0_op(&op);
>>> +	ret = HYPERVISOR_dom0_op(&op);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Set the hardware RTC. */
>>> +	return mach_set_rtc_mmss(now);
>> Sorry, just noticed one more thing while applying this. Do all Xen
>> systems run on hardware that has the conventional CMOS clock?
>>
>> What happens if the bare-metal needs to use efi_set_rtc() or
>> vrtc_set_mmss() ?
> Ug. I hadn''t considered that.  I''ll have think some more
on this.
Ok. I''ll look forward to a future revision then and not queue this one.


Just FYI, here''s what I rewrote the commit message to:

     x86/xen: Sync the CMOS RTC as well as the Xen wallclock

     Adjustments to Xen''s persistent_clock via
update_persistent_clock()
     don''t actually persist, as the xen_set_walltime() just notifies
     other domN guests that it has been updated, and does not modify
     the underlying CMOS clock.

     Thus, this patch modifies xen_set_wallclock() so it will set
     the underlying CMOS clock when called from dom0, ensuring the
     persistent_clock will be correct on the next hardware boot.


I feel its a bit more clear, but feel free to tweak it as you see fit.

thanks
-john

>>> On 14.05.13 at 19:24, John Stultz <john.stultz@linaro.org>
wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/xen/time.c |   11 ++++++++++-
>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>> index a1947ac..4656165 100644
>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/kernel_stat.h>
>>   #include <linux/math64.h>
>>   #include <linux/gfp.h>
>> +#include <linux/mc146818rtc.h>
>>   
>>   #include <asm/pvclock.h>
>>   #include <asm/xen/hypervisor.h>
>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec
*now)
>>   static int xen_set_wallclock(const struct timespec *now)
>>   {
>>   	struct xen_platform_op op;
>> +	int ret;
>>   
>>   	/* do nothing for domU */
>>   	if (!xen_initial_domain())
>>   		return -1;
>>   
>> +	/* Set the Xen wallclock. */
>>   	op.cmd = XENPF_settime;
>>   	op.u.settime.secs = now->tv_sec;
>>   	op.u.settime.nsecs = now->tv_nsec;
>>   	op.u.settime.system_time = xen_clocksource_read();
>>   
>> -	return HYPERVISOR_dom0_op(&op);
>> +	ret = HYPERVISOR_dom0_op(&op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set the hardware RTC. */
>> +	return mach_set_rtc_mmss(now);
> 
> Sorry, just noticed one more thing while applying this. Do all Xen 
> systems run on hardware that has the conventional CMOS clock?
> 
> What happens if the bare-metal needs to use efi_set_rtc() or 
> vrtc_set_mmss() ?
There''s no EFI support in the upstream kernel yet when running
on Xen (the code that''s in the kernel can''t be used, as Dom0
can''t directly invoke EFI runtime service functions).

And I don''t think Xen is in any way prepared to run on
Moorestown, which afaict is where vrtc_set_mmss() comes into
the picture.

Jan

On 05/15/2013 01:19 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 19:24, John Stultz
<john.stultz@linaro.org> wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> If NTP is used in dom0 and it is synchronized to its clock source,
>>> then the kernel will periodically synchronize the Xen wallclock
with
>>> the system time.  Updates to the Xen wallclock do not persist
across
>>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>> ---
>>>    arch/x86/xen/time.c |   11 ++++++++++-
>>>    1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>>> index a1947ac..4656165 100644
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/kernel_stat.h>
>>>    #include <linux/math64.h>
>>>    #include <linux/gfp.h>
>>> +#include <linux/mc146818rtc.h>
>>>    
>>>    #include <asm/pvclock.h>
>>>    #include <asm/xen/hypervisor.h>
>>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec
*now)
>>>    static int xen_set_wallclock(const struct timespec *now)
>>>    {
>>>    	struct xen_platform_op op;
>>> +	int ret;
>>>    
>>>    	/* do nothing for domU */
>>>    	if (!xen_initial_domain())
>>>    		return -1;
>>>    
>>> +	/* Set the Xen wallclock. */
>>>    	op.cmd = XENPF_settime;
>>>    	op.u.settime.secs = now->tv_sec;
>>>    	op.u.settime.nsecs = now->tv_nsec;
>>>    	op.u.settime.system_time = xen_clocksource_read();
>>>    
>>> -	return HYPERVISOR_dom0_op(&op);
>>> +	ret = HYPERVISOR_dom0_op(&op);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Set the hardware RTC. */
>>> +	return mach_set_rtc_mmss(now);
>> Sorry, just noticed one more thing while applying this. Do all Xen
>> systems run on hardware that has the conventional CMOS clock?
>>
>> What happens if the bare-metal needs to use efi_set_rtc() or
>> vrtc_set_mmss() ?
> There''s no EFI support in the upstream kernel yet when running
> on Xen (the code that''s in the kernel can''t be used, as
Dom0
> can''t directly invoke EFI runtime service functions).
>
> And I don''t think Xen is in any way prepared to run on
> Moorestown, which afaict is where vrtc_set_mmss() comes into
> the picture.
Ok.

David: Would you resubmit this patch, adding this additional context in 
a comment?

thanks
-john