From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7665DC433DB for ; Tue, 23 Feb 2021 11:38:00 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id BFBEB64E22 for ; Tue, 23 Feb 2021 11:37:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BFBEB64E22 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:42684 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lEW0s-0004gy-Mg for qemu-devel@archiver.kernel.org; Tue, 23 Feb 2021 06:37:58 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:55824) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lEVzz-0004H7-1X for qemu-devel@nongnu.org; Tue, 23 Feb 2021 06:37:03 -0500 Received: from mx2.suse.de ([195.135.220.15]:52126) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lEVzx-00013O-3g for qemu-devel@nongnu.org; Tue, 23 Feb 2021 06:37:02 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 2F539AF0B; Tue, 23 Feb 2021 11:36:59 +0000 (UTC) Subject: Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG To: =?UTF-8?Q?Alex_Benn=c3=a9e?= References: <20210221092449.7545-1-cfontana@suse.de> <20210221092449.7545-35-cfontana@suse.de> <87v9ak5cz0.fsf@linaro.org> <03502e51-99f5-239d-42a6-e57892faa297@suse.de> <87wnuz3v0u.fsf@linaro.org> From: Claudio Fontana Message-ID: Date: Tue, 23 Feb 2021 12:36:57 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <87wnuz3v0u.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=195.135.220.15; envelope-from=cfontana@suse.de; helo=mx2.suse.de X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , Claudio Fontana , Richard Henderson , qemu-devel@nongnu.org, Roman Bolshakov , Paolo Bonzini , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Eduardo Habkost Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 2/23/21 12:01 PM, Alex Bennée wrote: > > Claudio Fontana writes: > >> On 2/22/21 4:34 PM, Alex Bennée wrote: >>> >>> Claudio Fontana writes: >>> >>>> From: Claudio Fontana >>>> >>>> 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 >>>> --- >>>> 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 > >> >> 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 > >