From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v3 11/11] hvm/hpet: handle 1st period special Date: Thu, 01 May 2014 16:19:02 -0400 Message-ID: <5362AC36.70609@terremark.com> References: <1397756585-27091-1-git-send-email-dslutz@verizon.com> <1397756585-27091-12-git-send-email-dslutz@verizon.com> <535A7205020000780000C510@nat28.tlf.novell.com> <20140501103111.GA77240@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140501103111.GA77240@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 , Jan Beulich Cc: Keir Fraser , Don Slutz , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 05/01/14 06:31, Tim Deegan wrote: > At 13:32 +0100 on 25 Apr (1398429157), Jan Beulich wrote: >>>>> On 17.04.14 at 19:43, wrote: >>> v3: >>> Better commit message. >>> More setting of first_mc64 & first_enabled when needed. >>> Switch to bool_t. >>> >>> xen/arch/x86/hvm/hpet.c | 83 ++++++++++++++++++++++++++++++++++++------- >>> xen/include/asm-x86/hvm/vpt.h | 2 ++ >>> 2 files changed, 73 insertions(+), 12 deletions(-) >> I still can't help myself thinking that this must be achievable with less >> special casing - I just can't imagine that hardware also has this huge >> an amount of special case logic just for the first period. > +1. I didn't follow the explanation on v1 very clearly, so perhaps > you can correct my understanding, but I _think_ the issue you're > describing is: > > when we read the comparator, we try to wind it forward from its > current value towards the main counter value, by adding as many > periods as will fit. Yes. > If the guest sets the comparator >1 period away, we'll incorrectly > warp the comparator to some foolish value as soon as we read it. Yes. The "foolish value" falls within the bounds of a period. > So, you're adding mechanism to detect the first interrupt after a > comparator set (or when the main counter is being started, or on hpet > load -- both of those rely on 'first_enabled' being essentialy > harmless if set when it's _not_ the first interrupt, which suggests > that maybe that's not the best name for the flag). It is not harmless if it is set when _not_ in the first interval (before first interrupt). Since it is used by hpet_save during migration, the correct values need to be saved. I do not care about the name. You are missing the case when the guest sets the main counter. > Couldn't it be fixed more simply by detecting comparator values in the > past in hpet_get_comparator()? This statement only works using 64-bit arithmetic for the main counter never 63 changing by more then 2 . (Which is a boundary case that should not happen in my life time.) Without patch #11 (this patch) and without patch #8 (Use signed divide...) and adding: commit 8c450bed530b13c3f3d18b9dafb3d1b1d69ac1f2 Author: Don Slutz Date: Thu May 1 14:02:47 2014 -0400 hvm/hpet: Detect comparator values in the past Signed-off-by: Don Slutz diff --git a/xen/arch/x86/hvm/hpet.c b/xen/arch/x86/hvm/hpet.c index 4e4da20..d93dd69 100644 --- a/xen/arch/x86/hvm/hpet.c +++ b/xen/arch/x86/hvm/hpet.c @@ -98,10 +98,12 @@ static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn, uint64_t period = h->hpet.period[tn]; if (period) { - elapsed = hpet_read_maincounter(h, guest_time) + - period - comparator; - comparator += (elapsed / period) * period; - h->hpet.comparator64[tn] = comparator; + elapsed = hpet_read_maincounter(h, guest_time) - comparator; + if ( (int64_t)elapsed >= 0 ) + { + comparator += ((elapsed + period) / period) * period; + h->hpet.comparator64[tn] = comparator; + } } } Which I think was what you mean. It looks to be "ok" (ignoring the boundary case). So should I send it as v4? -Don Slutz > Tim.