Re: [PATCH 1/6] xen/arm: Save and restore support for hvm context hypercall

[Julien Grall <julien.grall@linaro.org>]

Hi Wei,

Thank you for the patch.

On 04/10/2014 05:48 PM, Wei Huang wrote:

[..]

>      case XEN_DOMCTL_cacheflush:
>      {
>          unsigned long s = domctl->u.cacheflush.start_pfn;
>          unsigned long e = s + domctl->u.cacheflush.nr_pfns;
>  
>          if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
> -            return -EINVAL;
> +            ret = -EINVAL;
>  
>          if ( e < s )
> -            return -EINVAL;
> +            ret = -EINVAL;
>  
> -        return p2m_cache_flush(d, s, e);
> +        ret = p2m_cache_flush(d, s, e);
>      }

Your change in XEN_DOMCTL_cacheflush is wrong. The code should not
continue if the sanity check has failed.

>  void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 471c4cd..bfe38f4 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -7,14 +7,15 @@
>  
>  #include <xsm/xsm.h>
>  
> +#include <xen/hvm/save.h>
>  #include <public/xen.h>
>  #include <public/hvm/params.h>
>  #include <public/hvm/hvm_op.h>
>  
>  #include <asm/hypercall.h>
> +#include <asm/gic.h>
>  
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> -
>  {
>      long rc = 0;
>  
> @@ -65,3 +66,505 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      return rc;
>  }
> +
> +static int vgic_irq_rank_save(struct vcpu *v,
> +                              struct vgic_rank *ext,
> +                              struct vgic_irq_rank *rank)

I would prefer if you move all theses functions of this file in each
specific file (timer in time.c, vgic in vgic.c,...).

It would be easier when GICv3 and new stuff will be supported.

> +{
> +    spin_lock(&rank->lock);
> +
> +    /* 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");
> +        return -EINVAL;
> +    }

You don't need to check it at runtime. BUILG_BUG_ON is enough here.


> +    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;
> +    }

Same here. And actually everywhere you have the pattern "if (sizeof(a)
op sizeof (b))" in this file.

[..]

> +HVM_REGISTER_SAVE_RESTORE(A15_TIMER, timer_save, timer_load, 2, HVMSR_PER_VCPU);

The timer structure is not A15 specific. Can you rename it in timer?

[..]

> +void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr)
> +{
> +    hdr->cpuid = READ_SYSREG32(MIDR_EL1);

You can directly use current_cpu_data.midr.bits;

> +}
> +
> +int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr)
> +{
> +    uint32_t cpuid;
> +
> +    if ( hdr->magic != HVM_FILE_MAGIC )
> +    {
> +        printk(XENLOG_G_ERR "HVM%d restore: bad magic number %#"PRIx32"\n",
> +               d->domain_id, hdr->magic);
> +        return -1;
> +    }
> +
> +    if ( hdr->version != HVM_FILE_VERSION )
> +    {
> +        printk(XENLOG_G_ERR "HVM%d restore: unsupported version %u\n",
> +               d->domain_id, hdr->version);
> +        return -1;
> +    }
> +
> +    cpuid = READ_SYSREG32(MIDR_EL1);

Same here.

> +    if ( hdr->cpuid != cpuid )
> +    {
> +        printk(XENLOG_G_INFO "HVM%d restore: VM saved on one CPU "
> +               "(%#"PRIx32") and restored on another (%#"PRIx32").\n",
> +               d->domain_id, hdr->cpuid, cpuid);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 3683ae3..714a3c4 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -64,6 +64,8 @@ subdir-$(CONFIG_COMPAT) += compat
>  
>  subdir-$(x86_64) += hvm
>  
> +subdir-$(CONFIG_ARM) += hvm
> +

I think you can directly merge with the line above and use:

subdir-y += hvm

Regards,

-- 
Julien Grall