Re: [PATCH] Provide HVM guest RTC change notification

[Ben Thomas <bthomas@virtualiron.com>]

Hi,

What's the status of this discussion/patch ? It would be nice to
get some closure on this. I know everyone's busy.

My understanding of the current concerns from Christian are (and
these are my interpretation of his comments):

- simply use a inequality test in time offsets to trigger setting
   the new time offset.

   My rationale for the as-submitted patch, was that there is an
   existing per-second mechanism that tries to emulate the set and
   uip bits and timing.  Time changes appear to happen during this
   per-second mechanism.  While it looks simple to just modify the
   time if the offset is detected to have changed, it would appear
   that this would negatively impact readers of the RTC and
   circumvent the work done by the per-second mechanism.  More
   simply, it wasn't clear to me that modifying the time wouldn't
   break guests unless the per-second mechanism was utilized.

- don't use the VIRQ mechanism for time-offset (RTC write)
   notifications.  Use an ioreq to qemu.

   The as-submitted patch used the VIRQ mechanism for a few
   reasons.  By building upon the domain event mechanism/virq,
   this low frequency event is handled in a fashion quite
   like other low-frequency domain events such as shutdown and
   crash. I was looking for a low-cost, low-impact, fire-and-forget
   means to signal an RTC change.

   The ioreq approach is interesting.  I'm uncertain about it for
   the following reasons, which some small additional explanation
   would help me with.  Adding an ioreq for time change seems quite
   out of character with the other ioreq events.  Additionally,
   qemu processing (and writing a store entry) are likely to be
   rather expensive.  Wouldn't this also cause undue delay in the
   guest awaiting the completion of the ioreq ?

It is quite possible that I have misunderstood Christian's comments.
If so, I'd love to better understand the issues. I really don't care
what the final implementation is, other than the usual expectations
of robustness, reasonable performance and clarity. Perhaps with a
bit more information, this can be cleared up and closed up.  I'd like
to get back to the capabilities that existed before the RTC code
was moved into the hypervisor.

Thanks !
-b




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