From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [RFC v3 3/6] xen/arm: Add save/restore support for ARM arch timer Date: Fri, 09 May 2014 00:02:52 +0100 Message-ID: <536C0D1C.6060406@citrix.com> References: <1399583908-21755-1-git-send-email-w1.huang@samsung.com> <1399583908-21755-4-git-send-email-w1.huang@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1399583908-21755-4-git-send-email-w1.huang@samsung.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Huang , xen-devel@lists.xen.org Cc: keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, julien.grall@linaro.org, tim@xen.org, jaeyong.yoo@samsung.com, jbeulich@suse.com, yjhyun.yoo@samsung.com List-Id: xen-devel@lists.xenproject.org On 08/05/2014 22:18, Wei Huang wrote: > This patch implements a save/resore support for ARM architecture > timer. > > Signed-off-by: Evgeny Fedotov > Signed-off-by: Wei Huang > --- > xen/arch/arm/vtimer.c | 90 ++++++++++++++++++++++++++++++++ > xen/include/public/arch-arm/hvm/save.h | 16 +++++- > 2 files changed, 105 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c > index b93153e..6576408 100644 > --- a/xen/arch/arm/vtimer.c > +++ b/xen/arch/arm/vtimer.c > @@ -21,6 +21,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -285,6 +286,95 @@ int vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr) > } > } > > +/* Save timer info to support save/restore */ > +static int hvm_timer_save(struct domain *d, hvm_domain_context_t *h) > +{ > + struct hvm_arm_timer ctxt; > + struct vcpu *v; > + struct vtimer *t; > + int i; unsigned int > + int rc = 0; > + > + /* Save the state of vtimer and ptimer */ > + for_each_vcpu( d, v ) > + { > + t = &v->arch.virt_timer; > + > + for ( i = 0; i < ARM_TIMER_TYPE_COUNT; i++ ) > + { > + ctxt.cval = t->cval; > + ctxt.ctl = t->ctl; > + > + switch ( i ) > + { > + case ARM_TIMER_TYPE_PHYS: > + ctxt.vtb_offset = d->arch.phys_timer_base.offset; > + ctxt.type = ARM_TIMER_TYPE_PHYS; > + break; > + case ARM_TIMER_TYPE_VIRT: > + ctxt.vtb_offset = d->arch.virt_timer_base.offset; > + ctxt.type = ARM_TIMER_TYPE_VIRT; > + default: > + rc = -EINVAL; > + break; This break is out of the switch, not out of the for loop, so you will still try to save the bogus entry. As you control i and want to save all timers, I suggest a BUG() instead; > + } > + > + if ( (rc = hvm_save_entry(TIMER, v->vcpu_id, h, &ctxt)) != 0 ) > + return rc; > + > + t = &v->arch.phys_timer; This updating of t looks suspect and fragile. This is a good approximation of the "for case" programming paradigm (http://thedailywtf.com/Comments/The_FOR-CASE_paradigm.aspx). There are only two timers and they refer to different named items inside struct domain. It would be clearer to remove the loop. > + } > + } > + > + return rc; > +} > + > +/* Restore timer info from context to support save/restore */ > +static int hvm_timer_load(struct domain *d, hvm_domain_context_t *h) > +{ > + int vcpuid; unsigned > + struct hvm_arm_timer ctxt; > + struct vcpu *v; > + struct vtimer *t = NULL; With this initialised here... > + int rc = 0; > + > + /* Which vcpu is this? */ > + vcpuid = hvm_load_instance(h); > + > + if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > + { > + dprintk(XENLOG_ERR, "HVM restore: dom%u has no vcpu%u\n", > + d->domain_id, vcpuid); > + return -EINVAL; > + } > + > + if ( hvm_load_entry(TIMER, h, &ctxt) != 0 ) > + return -EINVAL; > + > + switch ( ctxt.type ) > + { > + case ARM_TIMER_TYPE_PHYS: > + t = &v->arch.phys_timer; > + d->arch.phys_timer_base.offset = ctxt.vtb_offset; > + break; > + case ARM_TIMER_TYPE_VIRT: > + t = &v->arch.virt_timer; > + d->arch.virt_timer_base.offset = ctxt.vtb_offset; > + break; > + default: > + rc = -EINVAL; > + break; ... and this error handling, > + } > + > + t->cval = ctxt.cval; > + t->ctl = ctxt.ctl; > + t->v = v; this is going to end in tears. return -EINVAL from the default. > + > + return rc; > +} > + > +HVM_REGISTER_SAVE_RESTORE(TIMER, hvm_timer_save, hvm_timer_load, 2, > + HVMSR_PER_VCPU); > /* > * Local variables: > * mode: C > 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 */ > +#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; > + uint32_t ctl; > + uint64_t cval; > + uint32_t type; > +}; This is also going to have 32/64 alignment issues. ~Andrew > +DECLARE_HVM_SAVE_TYPE(TIMER, 4, struct hvm_arm_timer); > + > /* > * Largest type-code in use > */ > -#define HVM_SAVE_CODE_MAX 3 > +#define HVM_SAVE_CODE_MAX 4 > > #endif >