Re: [RFC v3 3/6] xen/arm: Add save/restore support for ARM arch timer

[Wei Huang <w1.huang@samsung.com>]

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.
>
>