| Message ID | 1399583908-21755-4-git-send-email-w1.huang@samsung.com |
|---|---|
| State | New |
| Headers | show |
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 <e.fedotov@samsung.com> > Signed-off-by: Wei Huang <w1.huang@samsung.com> > --- > 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 <xen/lib.h> > #include <xen/timer.h> > #include <xen/sched.h> > +#include <xen/hvm/save.h> > #include <asm/irq.h> > #include <asm/time.h> > #include <asm/gic.h> > @@ -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 >
Hi Wei, Thank you for the patch. On 08/05/14 22:18, Wei Huang wrote: > + 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; It seems you forgot to address some of my comments on the timer save/restore. Saving {virt,phys}_timer_base.offset per VCPU rather than per-domain seems a waste of space and confusing. Furthermore, this offset is used to get a relative offset from the Xen timer counter. Migrating the guess to another server will invalidate the value. Regards,
On 09/05/14 00:02, Andrew Cooper wrote: >> + } >> + >> + 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. I agree with Andrew. I've already made a similar comment on v2... Regards,
On Sun, 2014-05-11 at 09:58 +0100, Julien Grall wrote: > Hi Wei, > > Thank you for the patch. > > On 08/05/14 22:18, Wei Huang wrote: > > + 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; > > It seems you forgot to address some of my comments on the timer > save/restore. > > Saving {virt,phys}_timer_base.offset per VCPU rather than per-domain > seems a waste of space and confusing. Is it 100% inconceivable that two vcpus might some day have different timers? > Furthermore, this offset is used to get a relative offset from the Xen > timer counter. Migrating the guess to another server will invalidate the > value. The correct architectural state to migrate is the time stamp as the guest sees it, i.e. with the offset already applied. The receiving end would then need to recalculate the correct offset based on its local timer count. Ian.
On 05/12/2014 09:35 AM, Ian Campbell wrote: > On Sun, 2014-05-11 at 09:58 +0100, Julien Grall wrote: >> Hi Wei, >> >> Thank you for the patch. >> >> On 08/05/14 22:18, Wei Huang wrote: >>> + 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; >> >> It seems you forgot to address some of my comments on the timer >> save/restore. >> >> Saving {virt,phys}_timer_base.offset per VCPU rather than per-domain >> seems a waste of space and confusing. > > Is it 100% inconceivable that two vcpus might some day have different > timers? By timers, did you mean did different boot offset? AFAIU, the timer counter (CNTP) is common to every {p,v{CPUs and contains the number of ticks from the start time. If some days we want to have two vcpus with different offfset, then we can extend the format. Regards,
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. Ian.
On 05/14/2014 12:14 PM, 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? CNTFRQ is set by the platform and can't change for any guest. If we migrate to a platform with a different frequency, then the guest should cope with it. IHMO, CNTKCTL should be saved/restored in guest core registers. >> +#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? Careful, phystimer.vtb_offset is in nanosecond and virttimer.vtb_offset is in ticks. Regards,
On Wed, 2014-05-14 at 13:13 +0100, Julien Grall wrote: > On 05/14/2014 12:14 PM, 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? > > CNTFRQ is set by the platform and can't change for any guest. If we > migrate to a platform with a different frequency, then the guest should > cope with it. I doubt that (guest coping) will be the case in reality. CNTFRQ should be saved so that the target platform can either reject the restore or take steps to emulate the original state. In the short term this probably means reject. > IHMO, CNTKCTL should be saved/restored in guest core registers. I don't see why when there is a timer specific struct. Ian.
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. > >
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 <xen/lib.h> #include <xen/timer.h> #include <xen/sched.h> +#include <xen/hvm/save.h> #include <asm/irq.h> #include <asm/time.h> #include <asm/gic.h> @@ -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; + 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; + } + + if ( (rc = hvm_save_entry(TIMER, v->vcpu_id, h, &ctxt)) != 0 ) + return rc; + + t = &v->arch.phys_timer; + } + } + + 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; + struct hvm_arm_timer ctxt; + struct vcpu *v; + struct vtimer *t = NULL; + 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; + } + + t->cval = ctxt.cval; + t->ctl = ctxt.ctl; + t->v = v; + + 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; +}; +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