Re: [Xen-devel] [PATCH RFC XEN v1 08/14] xen: arm: Save and restore arch timer state.

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-12-16 at 18:05 +0000, Stefano Stabellini wrote:
> On Wed, 16 Dec 2015, Ian Campbell wrote:
> > On Wed, 2015-12-16 at 15:53 +0000, Stefano Stabellini wrote:
> > > On Wed, 9 Dec 2015, Ian Campbell wrote:
> > > > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > > ---
> > > > Âxen/arch/arm/vtimer.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 72
> > > > ++++++++++++++++++++++++++++++++++
> > > > Âxen/include/public/arch-arm/hvm/save.h | 15 ++++++-
> > > > Â2 files changed, 86 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > > > index 629feb4..9dfc699 100644
> > > > --- a/xen/arch/arm/vtimer.c
> > > > +++ b/xen/arch/arm/vtimer.c
> > > > @@ -22,6 +22,7 @@
> > > > Â#include <xen/timer.h>
> > > > Â#include <xen/sched.h>
> > > > Â#include <xen/perfc.h>
> > > > +#include <xen/hvm/save.h>
> > > > Â#include <asm/div64.h>
> > > > Â#include <asm/irq.h>
> > > > Â#include <asm/time.h>
> > > > @@ -355,6 +356,77 @@ int vtimer_emulate(struct cpu_user_regs *regs,
> > > > union hsr hsr)
> > > > ÂÂÂÂÂ}
> > > > Â}
> > > > Â
> > > > +static int timer_save_one(hvm_domain_context_t *h,
> > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct vcpu *v, struct vtimer *t,
> > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint type, uint64_t offset)
> > > > +{
> > > > +ÂÂÂstruct hvm_hw_timer ctxt;
> > > > +
> > > > +ÂÂÂctxt.cval = t->cval;
> > > > +ÂÂÂctxt.ctl = t->ctl;
> > > > +ÂÂÂctxt.vtb_offset = offset;
> > > > +ÂÂÂctxt.type = type;
> > > > +ÂÂÂif ( hvm_save_entry(A15_TIMER, v->vcpu_id, h, &ctxt) != 0 )
> > > > +ÂÂÂÂÂÂÂreturn 1;
> > > > +ÂÂÂreturn 0;
> > > > +}
> > > > +
> > > > +static int timer_save(struct domain *d, hvm_domain_context_t *h)
> > > > +{
> > > > +ÂÂÂÂstruct vcpu *v;
> > > > +
> > > > +ÂÂÂÂ/* Save the state of vtimer and ptimer */
> > > > +ÂÂÂÂfor_each_vcpu( d, v )
> > > > +ÂÂÂÂ{
> > > > +ÂÂÂÂÂÂÂÂtimer_save_one(h, v, &v->arch.phys_timer,
> > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂHVM_SAVE_TIMER_TYPE_PHYS, d-
> > > > > arch.phys_timer_base.offset);
> > > > +ÂÂÂÂÂÂÂÂtimer_save_one(h, v, &v->arch.virt_timer,
> > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂHVM_SAVE_TIMER_TYPE_VIRT, d-
> > > > > arch.virt_timer_base.offset);
> > > > +ÂÂÂÂ}
> > > 
> > > I don't think we should save phys_timer_base.offset and
> > > virt_timer_base.offset: they represent host specific offsets. I think
> > > we
> > > need to save:
> > > 
> > > phys_timer:
> > > ÂÂÂÂctxt.offset = NOW() - d->arch.phys_timer_base.offset;
> > > virt_timer:
> > > ÂÂÂÂctxt.offset = READ_SYSREG64(CNTPCT_EL0) - d-
> > > > arch.virt_timer_base.offset
> > 
> > Yes, I meant to come back to this (and the restore side) with a bigger
> > thinking cap on and forgot.
> > Â
> > > > +#define HVM_SAVE_TIMER_TYPE_VIRT 0
> > > > +#define HVM_SAVE_TIMER_TYPE_PHYS 1
> > > > +
> > > > +struct hvm_hw_timer
> > > > +{
> > > > +ÂÂÂÂuint64_t vtb_offset; /* XXX Should be abs time since guest
> > > > booted
> > > > */
> > > 
> > > I would call this simply "offset" with a comment
> > 
> > OK. The comment here was supposed to remind me about the bigger
> > thinking
> > cap. Oops.
> > 
> > > > +ÂÂÂÂuint32_t ctl;
> > > > +ÂÂÂÂuint64_t cval;
> > > > +ÂÂÂÂuint32_t type;
> > > 
> > > Isn't a uint32_t a bit too much for one bit?
> > > Don't we need to save and restore the PPIs too?
> > 
> > This is an ABI, so I wanted to avoid bit fields etc, I could make it a
> > uint8_t but then array alignment gets interesting. Really I guess this
> > is
> > actually a uint8_t and 3 bytes of padding.
> 
> Having to deal with types != from uint32_t and uint64_t is unavoidable.
> In fact other structs used in save/restore in other patches of this
> series already use uint8_t.

Right, I just don't really see a whole lot of point from an ABI point of
view right now, unless you are arguing against padding the struct to an 4
byte aligned size?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel