On 2/23/21 12:36 PM, Claudio Fontana wrote:
> On 2/23/21 12:01 PM, Alex Bennée wrote:
>>
>> Claudio Fontana <cfont...@suse.de> writes:
>>
>>> On 2/22/21 4:34 PM, Alex Bennée wrote:
>>>>
>>>> Claudio Fontana <cfont...@suse.de> writes:
>>>>
>>>>> From: Claudio Fontana <cfont...@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 <cfont...@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
>>
>>
>
