Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana <cfontana@suse.de>]

On 2/23/21 12:36 PM, Claudio Fontana wrote:
> On 2/23/21 12:01 PM, Alex Bennée wrote:
>>
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> On 2/22/21 4:34 PM, Alex Bennée wrote:
>>>>
>>>> Claudio Fontana <cfontana@suse.de> writes:
>>>>
>>>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>>>
>>>>> KVM has its own cpu->kvm_vtime.
>>>>>
>>>>> Adjust cpu vmstate by putting unused fields instead of the
>>>>> VMSTATE_TIMER_PTR when TCG is not available.
>>>>>
>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>> ---
>>>>>  target/arm/cpu.c     | 4 +++-
>>>>>  target/arm/machine.c | 5 +++++
>>>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>>>> index 1d81a1e7ac..b929109054 100644
>>>>> --- a/target/arm/cpu.c
>>>>> +++ b/target/arm/cpu.c
>>>>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>>>          }
>>>>>      }
>>>>>  
>>>>> +#ifdef CONFIG_TCG
>>>>>      {
>>>>>          uint64_t scale;
>>>>>  
>>>>> @@ -1347,7 +1348,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>>>          cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
>>>>>                                                    arm_gt_hvtimer_cb, cpu);
>>>>>      }
>>>>> -#endif
>>>>> +#endif /* CONFIG_TCG */
>>>>> +#endif /* !CONFIG_USER_ONLY */
>>>>>  
>>>>>      cpu_exec_realizefn(cs, &local_err);
>>>>>      if (local_err != NULL) {
>>>>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>>>>> index 666ef329ef..13d7c6d930 100644
>>>>> --- a/target/arm/machine.c
>>>>> +++ b/target/arm/machine.c
>>>>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>>>>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>>>>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>>>>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>>>>> +#ifdef CONFIG_TCG
>>>>>          VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
>>>>>          VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
>>>>> +#else
>>>>> +        VMSTATE_UNUSED(sizeof(QEMUTimer *)),
>>>>> +        VMSTATE_UNUSED(sizeof(QEMUTimer *)),
>>>>> +#endif /* CONFIG_TCG */
>>>>
>>>> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
>>>> just expose expired time but QEMUTimer has more in it than that. Paolo
>>>
>>>
>>> I am not sure I follow can you state more precisely where the issue could be?
>>>
>>> it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
>>> it ends up in VMSTATE_POINTER where a single pointer is assigned;
>>
>> Does it? I thought it ended up with the .expire_time (int64_t) which
>> will be bigger than sizeof(QemuTimer *) on a 32 bit system.
> 
> Ok I understand what you mean. Lets see:
> 
> Looking at vmstate.h,
> 
> #define VMSTATE_TIMER_PTR(_f, _s)                                         \
>     VMSTATE_TIMER_PTR_V(_f, _s, 0)
> 
> #define VMSTATE_TIMER_PTR_V(_f, _s, _v)                                   \
>     VMSTATE_POINTER(_f, _s, _v, vmstate_info_timer, QEMUTimer *)
> 
> #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
>     .name       = (stringify(_field)),                               \
>     .version_id = (_version),                                        \
>     .info       = &(_info),                                          \
>     .size       = sizeof(_type),                                     \
>     .flags      = VMS_SINGLE|VMS_POINTER,                            \
>     .offset     = vmstate_offset_value(_state, _field, _type),       \
> }
> 
> so here we get the vmstate field definition.
> 
> .size is fine, as it is sizeof(QEMUTimer *).
> 
> .info, is &vmstate_info_timer, migration/savevm.c:
> 
> const VMStateInfo vmstate_info_timer = {
>     .name = "timer",
>     .get  = get_timer,
>     .put  = put_timer,
> };
> 
> void timer_put(QEMUFile *f, QEMUTimer *ts)
> {
>     uint64_t expire_time;
> 
>     expire_time = timer_expire_time_ns(ts);
>     qemu_put_be64(f, expire_time);
> }
> 
> void timer_get(QEMUFile *f, QEMUTimer *ts)
> {
>     uint64_t expire_time;
> 
>     expire_time = qemu_get_be64(f);
>     if (expire_time != -1) {
>         timer_mod_ns(ts, expire_time);
>     } else {
>         timer_del(ts);
>     }
> }
> 
> ---
> 
> And the migration code does: (migration/vmstate.c):
> 
> int vmstate_save_state_v() {
>   ...
>   ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
>   ...
> }
> 
> which puts a BE64 in the QEMUFile *f (see timer_put above).
> 
> The load code in the same file does:
> 
> int vmstate_load_state() {
>   ...
>   ret = field->info->get(f, curr_elem, size, field);
>   ...
> }
> 
> which reads a BE64 from the QEMUFile *f (see timer_get above).
> 
> Would be "fine" from the field sizes perspective (the .size of the field type, and the value of the BE64),
> 
> but it's the calculations done in timer_get and timer_put which are scary, as they dereference the timer pointer.
> 
> 
> Should we actually have a check for null pointer in vmstate.c?
> 
> We _do_ have one in vmstate_save_state_v and vmstate_load_state, but it is actually active only for VMS_ARRAY_OF_POINTER.
> Why? Why not also do the same (write the null pointer and not following it) for normal VMS_POINTER ?
> 
> int vmstate_save_state_v() {
>  ...
>                 if (!curr_elem && size) {
>                     /* if null pointer write placeholder and do not follow */
>                     assert(field->flags & VMS_ARRAY_OF_POINTER);
>                     ret = vmstate_info_nullptr.put(f, curr_elem, size, NULL,
>                                                    NULL);
> 
>  ...
> 
> 
> int vmstate_load_state() {
> 
> ...
>                 if (!curr_elem && size) {
>                     /* if null pointer check placeholder and do not follow */
>                     assert(field->flags & VMS_ARRAY_OF_POINTER);
>                     ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
> ...
> 
> }
> 
> 
> This is worthwhile investigating further, any other ideas?
> 
> Thanks,
> 
> Claudio
> 
> 

Btw here it would be good to be able to rely on the existing tests,
do we have full coverage of these compatibility situations?

According to make check it's all a-ok, but... is the testing coverage insufficient
for these VMSTATE compatibility issues?

Ciao,

Claudio


>>
>>>
>>> so if we don't use gt_timer at all (as is the case with !CONFIG_TCG), we just
>>> need to ensure that an unused number is there to assign, migrating from old to new version?
>>>
>>>
>>>> suggested a straight VMSTATE_UNUSED(8) on IRC but I wonder if it would
>>>> be better to have a VMSTATE_UNUSED_TIMER?
>>>>
>>>> I don't think there is an impact for Xen because I'm fairly certain
>>>> migration isn't a thing we do - but I'll double check.
>>>>
>>>
>>> Thanks Alex, that would be helpful,
>>> if Xen uses gt_timer in any way I would not want to unwillingly break
>>> it.
>>
>> Not for ARM no, currently there is no ARM specific machine emulated by
>> QEMU for Xen. All ARM guests are PV guests.
>>
>>>
>>> Thanks,
>>>
>>> Claudio
>>
>>
>