Re: [RFC v3 3/6] xen/arm: Add save/restore support for ARM arch timer

[Andrew Cooper <andrew.cooper3@citrix.com>]

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
>