From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wei Huang <w1.huang@samsung.com>
Subject: Re: [RFC v3 3/6] xen/arm: Add save/restore support for
	ARM arch timer
Date: Wed, 14 May 2014 14:04:27 -0500
Message-ID: <5373BE3B.9030202@samsung.com>
References: <1399583908-21755-1-git-send-email-w1.huang@samsung.com>
	<1399583908-21755-4-git-send-email-w1.huang@samsung.com>
	<1400066090.29366.31.camel@kazak.uk.xensource.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: <1400066090.29366.31.camel@kazak.uk.xensource.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: Ian Campbell <Ian.Campbell@citrix.com>
Cc: keir@xen.org, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, julien.grall@linaro.org, tim@xen.org, jaeyong.yoo@samsung.com, xen-devel@lists.xen.org, jbeulich@suse.com, ian.jackson@eu.citrix.com, yjhyun.yoo@samsung.com
List-Id: xen-devel@lists.xenproject.org

On 05/14/2014 06:14 AM, Ian Campbell wrote:
> On Thu, 2014-05-08 at 16:18 -0500, Wei Huang wrote:
>> This patch implements a save/resore support for ARM architecture
>
> "restore"
>
>> diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
>> index 421a6f6..8679bfd 100644
>> --- a/xen/include/public/arch-arm/hvm/save.h
>> +++ b/xen/include/public/arch-arm/hvm/save.h
>> @@ -72,10 +72,24 @@ struct hvm_arm_gich_v2
>>   };
>>   DECLARE_HVM_SAVE_TYPE(GICH_V2, 3, struct hvm_arm_gich_v2);
>>
>> +/* Two ARM timers (physical and virtual) are saved */
>
> Do you not need to save CNTFRQ and CNTKCTL?
>
>> +#define ARM_TIMER_TYPE_VIRT  0
>> +#define ARM_TIMER_TYPE_PHYS  1
>> +#define ARM_TIMER_TYPE_COUNT 2       /* total count */
>> +
>> +struct hvm_arm_timer
>> +{
>> +    uint64_t vtb_offset;
>
> As discussed elsewhere I don't think the offset is architectural state.
> This should be incorporated into the cval. Otherwise how does the
> receiver know what this is an offset from?
>
>> +    uint32_t ctl;
>> +    uint64_t cval;
>> +    uint32_t type;
>> +};
>> +DECLARE_HVM_SAVE_TYPE(TIMER, 4, struct hvm_arm_timer);
>
> Why not do
> DECLARE_HVM_SAVE_TYPE(VTIMER, 4, struct hvm_arm_timer)
> DECLARE_HVM_SAVE_TYPE(PTIMER, 5, struct hvm_arm_timer)
> and drop the type field?
>
> Or else define hvm_arm_timers with cntfeq, cntkctl and two sets of the
> ctl,cval in a single struct.
>
This is the preferred approach after discussing with Julien.
> Ian.
>
>