Re: [PATCH] Provide HVM guest RTC change notification

[Ben Thomas <bthomas@virtualiron.com>]

Christian Limpach wrote:
> On 3/20/07, Ben Thomas <bthomas@virtualiron.com> wrote:
>> This patch restores the capabilities initially provided in
>> changeset 10010.  When the RTC code was moved into the hypervisor
>> (a good move), the control plane lost the ability to read the
>> current time offset as well as to receive notification of changes
>> to the RTC time base by a guest. This patch reintroduces these.
>>
>> Additionally, there is a small window at initialization time in
>> which the time offset may be set but not noticed or used. If
>> xc_domain_set_time_offset is called before the domain is started,
>> the offset won't be noticed until the next second update. An HVM
>> guest could read the RTC in this window and get an unintended
>> result. This patch closes the window.
> 
> Isn't the "s->time_offset_seconds !=
> s->pt.vcpu->domain->time_offset_seconds" condition an appropriate test
> whether you need to call rtc_copy_date?

It depends upon where you want this test. I didn't want it for
every access to the RTC registers, and something is needed to
set up the initial conditions.  A different approach would have
handled it when the RTC is set up, but that appears to be at
domain building time and it wasn't clear to me that there was a way
to pass the information without changing the domain builder API.
Given an existing mechanism to set the RTC time, it seemed an
interesting approach to use that and simply handle the initial
condition.

Changing the register access routines to unconditionally check for
a time offset change seemed too intrusive and at conflict
with the per-second change mechanism.  But, as long as it all works,
I'm open to anything.

The unlikely check for the accessed flag seemed a simple, low-cost
way to handle the initialization without unduly influencing other
portions of the mechanism.

>> The patch builds upon the existing change notification mechanism
>> provided by VIRQ_DOM_EXC. I stopped short of renaming the
>> releaseDomain watch to something like domainEvent as it wasn't
>> clear who might be relying upon the existing name. A patch for
>> that would be easy to create, though.
> 
> The domain exception virq is already quite overloaded as is and it
> causes xend to do a quite expensive scan of all the domain
> information.
> 
> I think using an ioreq to qemu would be a more appropriate mechanism
> to signal that the guest has changed the timeoffset.  The ioreq would
> include the delta by which the offset was changed.  qemu could then
> update the timeoffset stored in xenstore, since we would want to make
> the changed timeoffset persist across reboots.
 >    christian

I completely agree about the persistence, and that's the reason I
sent in the patch that resulted in c/s 10010.  I'm just trying to get
back to that level of capability.

The domain exception seemed like a reasonable approach. It may
be overloaded in the "number of things it can signal", but it
seems like a low-occurrence set of events.  And, the code checking
for that type of event easily accesses the additional flag.  Last,
but not least, using this particular virq completely removes all
qemu involvement.  Cutting back on qemu changes doesn't seem like
a bad side-effect.

Thanks,
-b



>> Last, the code introduced into qemu by 10010 is no longer really
>> connected to anything and could be removed. It's found in
>> tools/ioemu/patches/domain-timeoffset and can probably be excised
>> at some suitable point. Currently, it's relatively harmless.
>>
>>
>> Signed-off-by: Ben Thomas (ben@virtualiron.com)


-- 
------------------------------------------------------------------------
Ben Thomas                                         Virtual Iron Software
bthomas@virtualiron.com                            Tower 1, Floor 2
978-849-1214                                       900 Chelmsford Street
                                                    Lowell, MA 01851