Re: [Qemu-devel] [PATCH 1/4] xen: introduce mc146818rtcxen

[Anthony Liguori <anthony@codemonkey.ws>]

On 11/18/2011 05:46 AM, Stefano Stabellini wrote:
> On Tue, 15 Nov 2011, Stefano Stabellini wrote:
>> On Tue, 15 Nov 2011, Anthony Liguori wrote:
>>> On 11/15/2011 08:51 AM, stefano.stabellini@eu.citrix.com wrote:
>>>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>>
>>>> Xen doesn't need full RTC emulation in Qemu because the RTC is already
>>>> emulated by the hypervisor. In particular we want to avoid the timers
>>>> initialization so that Qemu doesn't need to wake up needlessly.
>>>>
>>>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>>>
>>> Yuck.  There's got to be a better way to do this.
>>
>> Yeah, it is pretty ugly, I was hoping in some good suggestions to
>> improve this patch :)
>>
>>
>>> I think it would be better to name timers and then in Xen specific machine code,
>>> disable the RTC timers.
>>
>> Good idea!
>> I was thinking that I could implement an rtc_stop function in
>> mc146818rtc.c that stops and frees the timers.
>>
>> Now the problem is that from xen-all.c I cannot easily find the
>> ISADevice instance to pass to rtc_stop. Do you think it would be
>> reasonable to call rtc_stop from pc_basic_device_init, inside the same
>> if (!xen_available()) introduce by the next patch?
>>
>> Otherwise I could implement functions to walk the isa bus, similarly to
>> pci_for_each_device.
>>
>
> ping?

Thinking more about it, I think this entire line of thinking is wrong (including 
mine) :-)

The problem you're trying to solve is that the RTC fires two 1 second timers 
regardless of whether the guest is reading the wall clock time, right?  And 
since wall clock time is never read from the QEMU RTC in Xen, it's a huge waste?

The Right Solution would be to modify the RTC emulation such that it did a 
qemu_get_clock() during read of the CMOS registers in order to ensure the time 
was up to date (instead of using 1 second timers).

Then the timers wouldn't even exist anymore.

Regards,

Anthony Liguori

>
>
>> This is just an example:
>>
>> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
>> index 2aaca2f..568c540 100644
>> --- a/hw/mc146818rtc.c
>> +++ b/hw/mc146818rtc.c
>> @@ -667,6 +667,28 @@ ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
>>       return dev;
>>   }
>>
>> +void rtc_stop(ISADevice *dev)
>> +{
>> +    RTCState *s = DO_UPCAST(RTCState, dev, dev);
>> +
>> +    qemu_del_timer(s->periodic_timer);
>> +    qemu_del_timer(s->second_timer);
>> +    qemu_del_timer(s->second_timer2);
>> +#ifdef TARGET_I386
>> +    if (rtc_td_hack) {
>> +        qemu_del_timer(s->coalesced_timer);
>> +    }
>> +#endif
>> +    qemu_free_timer(s->periodic_timer);
>> +    qemu_free_timer(s->second_timer);
>> +    qemu_free_timer(s->second_timer2);
>> +#ifdef TARGET_I386
>> +    if (rtc_td_hack) {
>> +        qemu_free_timer(s->coalesced_timer);
>> +    }
>> +#endif
>> +}
>> +
>>   static ISADeviceInfo mc146818rtc_info = {
>>       .qdev.name     = "mc146818rtc",
>>       .qdev.size     = sizeof(RTCState),
>> diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
>> index 575968c..aa2b8ab 100644
>> --- a/hw/mc146818rtc.h
>> +++ b/hw/mc146818rtc.h
>> @@ -8,5 +8,6 @@
>>   ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
>>   void rtc_set_memory(ISADevice *dev, int addr, int val);
>>   void rtc_set_date(ISADevice *dev, const struct tm *tm);
>> +void rtc_stop(ISADevice *dev);
>>
>>   #endif /* !MC146818RTC_H */
>> diff --git a/hw/pc.c b/hw/pc.c
>> index a0ae981..d734f75 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -1145,6 +1145,8 @@ void pc_basic_device_init(qemu_irq *gsi,
>>
>>       if (!xen_available()) {
>>           pit = pit_init(0x40, 0);
>> +    } else {
>> +        rtc_stop(*rtc_state);
>>       }
>>       pcspk_init(pit);
>>
>>
>