Re: [Xen-devel] [PATCH RESEND v5 1/6] xen/arm: Implement hvm save and restore

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-11-08 at 16:50 +0900, Jaeyong Yoo wrote:

If these get resent again please can you CC the ARM maintainers. 
$ git grep -A5 ARM MAINTAINERS
MAINTAINERS:ARM (W/ VIRTUALISATION EXTENSIONS) ARCHITECTURE
MAINTAINERS-M:  Ian Campbell <ian.campbell@xxxxxxxxxx>
MAINTAINERS-M:  Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
MAINTAINERS-M:  Tim Deegan <tim@xxxxxxx>
MAINTAINERS-S:  Supported
MAINTAINERS-L:  xen-devel@xxxxxxxxxxxxx

> From: Evgeny Fedotov <e.fedotov@xxxxxxxxxxx>
> 
> Implement save/restore of hvm context hypercall.
> In hvm context save/restore, we save gic, timer and vfp registers.
> 
> Changes from v4: Save vcpu registers within hvm context, and purge
> the save-vcpu-register patch.

The inter-version changelog should go after the main commit message and
the S-o-b separated by a "---" on a line by itself. This way it is not
included in the final commit.
[...]
> +static int vgic_irq_rank_save(struct vgic_rank *ext,
> +                               struct vgic_irq_rank *rank)
> +{
> +    spin_lock(&rank->lock);

This is probably wise, but the domain must be paused at this point,
right?

> +    /* Some of VGIC registers are not used yet, it is for a future usage */
> +    /* IENABLE, IACTIVE, IPEND,  PENDSGI registers */
> +    ext->ienable = rank->ienable;
> +    ext->iactive = rank->iactive;
> +    ext->ipend = rank->ipend;
> +    ext->pendsgi = rank->pendsgi;
> +    /* ICFG */
> +    ext->icfg[0] = rank->icfg[0];
> +    ext->icfg[1] = rank->icfg[1];
> +    /* IPRIORITY */
> +    if ( sizeof(rank->ipriority) != sizeof (ext->ipriority) )
> +    {
> +        dprintk(XENLOG_G_ERR, "hvm_hw_gic: check ipriority dumping space\n");

What makes ipriority (and itargets below) special enough to warrant such
a check compared with all the other fields?

Could these checks be BUILD_BUG_ON?

> +        return -EINVAL;
> +    }
> +    memcpy(ext->ipriority, rank->ipriority, sizeof(rank->ipriority));
> +    /* ITARGETS */
> +    if ( sizeof(rank->itargets) != sizeof (ext->itargets) )
> +    {
> +        dprintk(XENLOG_G_ERR, "hvm_hw_gic: check itargets dumping space\n");
> +        return -EINVAL;
> +    }
> +    memcpy(ext->itargets, rank->itargets, sizeof(rank->itargets));
> +    spin_unlock(&rank->lock);
> +    return 0;
> +}
> +
> +static int gic_save(struct domain *d, hvm_domain_context_t *h)
> +{
> +    struct hvm_hw_gic ctxt;
> +    struct vcpu *v;
> +
> +    /* Save the state of GICs */
> +    for_each_vcpu( d, v )

Where is the GICD state saved then?

> +    {
> +        ctxt.gic_hcr = v->arch.gic_hcr;
> +        ctxt.gic_vmcr = v->arch.gic_vmcr;
> +        ctxt.gic_apr = v->arch.gic_apr;
> +
> +        /* Save list registers and masks */
> +        /* (it is not necessary to save/restore them, but LR state can have
> +         * influence on downtime after Live Migration (to be tested)

What does this mean?

I'm in two minds about whether LR counts as architectural state, since
it is hypervisor side and not guest facing. I'd have thought that the LR
content could be reconstructed from the pending and active bitmasks.

> +         */
> +        if ( sizeof(v->arch.gic_lr) > sizeof (ctxt.gic_lr) )
> +        {
> +             dprintk(XENLOG_G_ERR, "hvm_hw_gic: increase LR dumping 
> space\n");
> +             return -EINVAL;
> +        }
> +        memcpy(ctxt.gic_lr, v->arch.gic_lr, sizeof(v->arch.gic_lr));
> +        ctxt.lr_mask = v->arch.lr_mask;
> +        ctxt.event_mask = v->arch.event_mask;

These last two in particular make me think that saving the LRs is wrong.

> +        /* Save PPI states (per-CPU) */
> +        /* It is necessary if SMP enabled */
> +        if ( vgic_irq_rank_save(&ctxt.ppi_state,  
> &v->arch.vgic.private_irqs) )
> +            return 1;
> +
> +        if ( hvm_save_entry(GIC, v->vcpu_id, h, &ctxt) != 0 )
> +            return 1;
> +    }
> +    return 0;
> +}
> +
> +static int gic_load(struct domain *d, hvm_domain_context_t *h)
> +{
> +    int vcpuid;
> +    struct hvm_hw_gic ctxt;
> +    struct vcpu *v;
> [...]
> +    return 0;
> +}
> +
> +HVM_REGISTER_SAVE_RESTORE(GIC, gic_save, gic_load, 1, HVMSR_PER_VCPU);
> +
> [...]

> +#ifdef CONFIG_ARM_32
> +        ctxt.mair0 = v->arch.mair0;
> +        ctxt.mair1 = v->arch.mair1;
> +#else
> +        ctxt.mair0 = v->arch.mair;
> +#endif
> +        /* Control Registers */
> +        ctxt.actlr = v->arch.actlr;
> +        ctxt.sctlr = v->arch.sctlr;
> +        ctxt.cpacr = v->arch.cpacr;
> [...]

> +        ctxt.x0 = c.x0;

Would it make sense to include a field of type vcpu_guest_core_regs in
the save struct instead of this big list?
[...]
> +        #ifdef CONFIG_ARM_32

#ifdef in the first columns please.

> +                ctxt.spsr_fiq = c.spsr_fiq;
> +                ctxt.spsr_irq = c.spsr_irq;
> +                ctxt.spsr_und = c.spsr_und;
> +                ctxt.spsr_abt = c.spsr_abt;

and don't indent these as if they were inside a block

You had this right when you saved the MAIR registers above.

> +        #endif
> +        #ifdef CONFIG_ARM_64

or #else if you prefer.

> +                ctxt.sp_el0 = c.sp_el0;
> +                ctxt.sp_el1 = c.sp_el1;
> +                ctxt.elr_el1 = c.elr_el1;
> +        #endif
> +
> +        /* check VFP state size before dumping */
> +        if ( sizeof(v->arch.vfp) > sizeof (ctxt.vfp) )
> +        {
> +            dprintk(XENLOG_G_ERR, "hvm_hw_cpu: increase VFP dumping 
> space\n");
> +            return -EINVAL;
> +        }
> +        memcpy((void*) &ctxt.vfp, (void*) &v->arch.vfp, sizeof(v->arch.vfp));
> +
> +        ctxt.pause_flags = v->pause_flags;

x86 has at the top of the loop:
        /* We don't need to save state for a vcpu that is down; the restore 
         * code will leave it down if there is nothing saved. */
        if ( test_bit(_VPF_down, &v->pause_flags) ) 
            continue;

which I think is preferable to this.

> diff --git a/xen/include/asm-arm/hvm/support.h 
> b/xen/include/asm-arm/hvm/support.h
> new file mode 100644
> index 0000000..8311f2f
> --- /dev/null
> +++ b/xen/include/asm-arm/hvm/support.h
> @@ -0,0 +1,29 @@
> +/*
> + * support.h: HVM support routines used by ARMv7 VE.

Not just on Vexpress nor just on v7 I expect?

> + *
> + * Copyright (c) 2012, Citrix Systems

Really?

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; if not, write to the Free Software Foundation, Inc., 59 
> Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + */
> +
> +#ifndef __ASM_ARM_HVM_SUPPORT_H__
> +#define __ASM_ARM_HVM_SUPPORT_H__
> +
> +#include <xen/types.h>
> +#include <public/hvm/ioreq.h>
> +#include <xen/sched.h>
> +#include <xen/hvm/save.h>
> +#include <asm/processor.h>

Just some #includes here?

Ian,


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