From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE. Date: Sat, 15 Feb 2014 00:02:15 +0000 Message-ID: <52FEAE87.9070601@citrix.com> References: <1392407521-19884-1-git-send-email-andrew.cooper3@citrix.com> <1392407521-19884-2-git-send-email-andrew.cooper3@citrix.com> <20140214230312.GA33715@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140214230312.GA33715@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: george.dunlap@eu.citrix.com, roger.pau@citrix.com, keir@xen.org, JBeulich@suse.com, Xen-devel List-Id: xen-devel@lists.xenproject.org On 14/02/2014 23:03, Tim Deegan wrote: > At 19:51 +0000 on 14 Feb (1392403919), Andrew Cooper wrote: >> From: Tim Deegan >> >> If the guest has not asked for interrupts, don't run the vpt timer >> to generate them. This is a prerequisite for a patch to simplify how >> the vpt interacts with the RTC, and also gets rid of a timer series in >> Xen in a case where it's unlikely to be needed. >> >> Instead, calculate the correct value for REG_C.PF whenever REG_C is >> read or PIE is enabled. This allow a guest to poll for the PF bit >> while not asking for actual timer interrupts. Such a guest would no >> longer get the benefit of the vpt's timer modes. >> >> Signed-off-by: Tim Deegan >> Signed-off-by: Andrew Cooper >> >> --- >> >> Changes in v2: >> * Reduce scope of `now` in rtc_timer_update() >> * Merge PIE logic in REG_B write >> * Tightly couple setting s->period with creating/destroying timers, so the >> timer change gets properly recreated when the guest sets REG_B.PIE > Thanks for sorting out that bug, but I think in this version the !PIE > case won't work. check_for_pf_ticks() uses s->period to figure out > whether to set PF, so it needs to be set whenever the REG_A selector > is configured, even if the timer's not running. > > It looks like always setting s->period == 0 just before the call to > rtc_timer_update in the REG_B write (i.e. not just in the !PIE case) > would DTRT, but is that what you tried earlier? I tried the "careful clobbering" which resulted in the fragment below, and altering the position of s->period in rtc_timer_update() to match, but that does indeed lead to issues of s->period being 0 if !PIE. I clearly should have been less careful. > > Er, that is, here: > >> rtc_update_irq(s); >> - if ( (data & RTC_PIE) && !(orig & RTC_PIE) ) >> + if ( (data ^ orig) & RTC_PIE ) >> + { >> + if ( !(data & RTC_PIE) ) >> + { >> + destroy_periodic_time(&s->pt); >> + s->period = 0; >> + } >> rtc_timer_update(s); >> + } >> if ( (data ^ orig) & RTC_SET ) > do this: > >> + if ( (data ^ orig) & RTC_PIE ) >> + { >> + destroy_periodic_time(&s->pt); >> + s->period = 0; >> rtc_timer_update(s); >> + } > Tim. ... and undo the changes in rtc_timer_update(). ~Andrew