From mboxrd@z Thu Jan  1 00:00:00 1970
From: Don Slutz <dslutz@verizon.com>
Subject: Re: [PATCH v3 06/11] hvm/hpet: In hpet_save,
	call hpet_get_comparator.
Date: Fri, 25 Apr 2014 17:42:18 -0400
Message-ID: <535AD6BA.40907@terremark.com>
References: <1397756585-27091-1-git-send-email-dslutz@verizon.com>
	<1397756585-27091-7-git-send-email-dslutz@verizon.com>
	<5357F6B0020000780000BA79@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <5357F6B0020000780000BA79@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, Don Slutz <dslutz@verizon.com>
Cc: Keir Fraser <keir@xen.org>, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On 04/23/14 11:21, Jan Beulich wrote:
>>>> On 17.04.14 at 19:43, <dslutz@verizon.com> wrote:
>> --- a/xen/arch/x86/hvm/hpet.c
>> +++ b/xen/arch/x86/hvm/hpet.c
>> @@ -495,6 +495,7 @@ static int hpet_save(struct domain *d, hvm_domain_context_t *h)
>>   {
>>       HPETState *hp = domain_vhpet(d);
>>       int rc;
>> +    uint64_t guest_time;
>>   
>>       spin_lock(&hp->lock);
>>   
>> @@ -524,6 +525,12 @@ static int hpet_save(struct domain *d, hvm_domain_context_t *h)
>>           C(period[1]);
>>           C(period[2]);
>>   #undef C
>> +        /* read the comparator to get it updated so hpet_save will
>> +         * return the expected value. */
>> +        guest_time = hp->hpet.mc64 - hp->mc_offset;
> Wouldn't you be better off loading guest_time via guest_time_hpet()
> right after taking the lock, using the variable also in the assignment
> to mc64? I'm particularly suspecting an inconsistency in the
> !hpet_enabled() case (i.e. when mc64 doesn't get updated
> anymore). Iirc a later patch (in the earlier version of the series)
> makes the adjustment in hpet_get_comparator() also conditional
> upon the HPET being enabled, in which case the end effect would
> be identical (due to hpet_get_comparator() then becoming a nop for
> that case).

You are right.

> In any event, no matter that the immediately following comment is
> malformed too, please fix your comment style (and feel free to
> adjust the one below at once).

Will do.
    -Don Slutz

>> +        hpet_get_comparator(hp, 0, guest_time);
>> +        hpet_get_comparator(hp, 1, guest_time);
>> +        hpet_get_comparator(hp, 2, guest_time);
>>           /* save the 64 bit comparator in the 64 bit timer[n].cmp field
>>            * regardless of whether or not the timer is in 32 bit mode. */
>>           rec->timers[0].cmp = hp->hpet.comparator64[0];
> Jan
>