Re: [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed

[Xiao Guangrong <guangrong.xiao@gmail.com>]

On 05/03/2017 11:42 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> Currently, the timer is updated whenever RegA or RegB is written
>> even if the periodic timer related configuration is not changed
>>
>> This patch optimizes it slightly to make the update happen only
>> if its period or enable-status is changed, also later patches are
>> depend on this optimization
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> ---
>>   hw/timer/mc146818rtc.c | 31 +++++++++++++++++++++++++++++--
>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
>> index 4165450..749e206 100644
>> --- a/hw/timer/mc146818rtc.c
>> +++ b/hw/timer/mc146818rtc.c
>> @@ -387,10 +387,25 @@ static void rtc_update_timer(void *opaque)
>>       check_update_timer(s);
>>   }
>>   
>> +static bool rtc_periodic_timer_updated_by_regA(RTCState *s, uint64_t data)
>> +{
>> +    uint8_t orig = s->cmos_data[RTC_REG_A];
>> +
>> +    return (orig & 0x0f) != (data & 0x0f);
>> +}
>> +
>> +static bool rtc_periodic_timer_updated_by_regB(RTCState *s, uint64_t data)
>> +{
>> +    uint8_t orig = s->cmos_data[RTC_REG_B];
>> +
>> +    return (orig & REG_B_PIE) != (data & REG_B_PIE);
>> +}
>> +
>>   static void cmos_ioport_write(void *opaque, hwaddr addr,
>>                                 uint64_t data, unsigned size)
>>   {
>>       RTCState *s = opaque;
>> +    bool update_periodic_timer;
>>   
>>       if ((addr & 1) == 0) {
>>           s->cmos_index = data & 0x7f;
>> @@ -423,6 +438,8 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>               }
>>               break;
>>           case RTC_REG_A:
>> +            update_periodic_timer = rtc_periodic_timer_updated_by_regA(s, data);
> 
> I think you can change it to a...
> 
>>               if ((data & 0x60) == 0x60) {
>>                   if (rtc_running(s)) {
>>                       rtc_update_time(s);
>> @@ -445,10 +462,16 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>               /* UIP bit is read only */
> 
> ... inlined update_periodic_timer assignment here:
> 
>      update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_A_PERIOD;
> 
>>               s->cmos_data[RTC_REG_A] = (data & ~REG_A_UIP) |
>>                   (s->cmos_data[RTC_REG_A] & REG_A_UIP);
>> -            periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
>> +
>> +            if (update_periodic_timer) {
>> +                periodic_timer_update(s, qemu_clock_get_ns(rtc_clock));
>> +            }
>> +
>>               check_update_timer(s);
>>               break;
>>           case RTC_REG_B:
>> +            update_periodic_timer = rtc_periodic_timer_updated_by_regB(s, data);
>> +
>>               if (data & REG_B_SET) {
>>                   /* update cmos to when the rtc was stopping */
>>                   if (rtc_running(s)) {
>> @@ -475,7 +498,11 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>>                   qemu_irq_lower(s->irq);
>>               }
> 
> Same here:
> 
>      update_periodic_timer = (s->cmos_data[RTC_REG_A] ^ data) & REG_B_PIE;
> 

Okay, will do, thanks!