From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC v2 1/6] xen/arm: Save and restore support with hvm context hypercalls Date: Wed, 16 Apr 2014 10:48:00 +0100 Message-ID: <534E51D0.9020502@linaro.org> References: <1397595918-30419-1-git-send-email-w1.huang@samsung.com> <1397595918-30419-2-git-send-email-w1.huang@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1397595918-30419-2-git-send-email-w1.huang@samsung.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Huang , xen-devel@lists.xen.org Cc: Keir Fraser , ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, jaeyong.yoo@samsung.com, Jan Beulich , yjhyun.yoo@samsung.com List-Id: xen-devel@lists.xenproject.org Hello Wei, Thank you for the patch. You are modifying common code in this patch. I've add Jan and Keir. Also, I would create a separate patch for this code movement. On 15/04/14 22:05, Wei Huang wrote: > + > +HVM_REGISTER_SAVE_RESTORE(GIC, hvm_gic_save_ctxt, hvm_gic_load_ctxt, 1, > + HVMSR_PER_VCPU); With new support for different GIC, I would differentiate VGIC and GIC save/restore. Also can you append V2 in the name? GICv3 support will be added soon. [..] > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c > index 3d6a721..7c47eac 100644 > --- a/xen/arch/arm/vtimer.c > +++ b/xen/arch/arm/vtimer.c > @@ -21,6 +21,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -284,6 +285,76 @@ int vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr) > } > } > > +static int hvm_vtimer_save_ctxt(struct domain *d, hvm_domain_context_t *h) > +{ > + struct hvm_hw_timer ctxt; > + struct vcpu *v; > + struct vtimer *t; > + int i, ret = 0; > + > + /* Save the state of vtimer and ptimer */ > + for_each_vcpu( d, v ) > + { > + t = &v->arch.virt_timer; > + for ( i = 0; i < 2; i++ ) > + { Looping here is very confusing, what does mean 0? 1? I would create an helper with the content of this loop and call twice this helper with the correct value in parameter. Smth like: hvm_save_vtimer(TIMER_TYPE_PHYS, &v->arch.phys_timer); hvm_save_vtimer(TIMER_TYPE_VIRT, &v->arch.virt_timer); > + ctxt.cval = t->cval; > + ctxt.ctl = t->ctl; > + ctxt.vtb_offset = i ? d->arch.phys_timer_base.offset : > + d->arch.virt_timer_base.offset; > + ctxt.type = i ? TIMER_TYPE_PHYS : TIMER_TYPE_VIRT; > + > + if ( (ret = hvm_save_entry(TIMER, v->vcpu_id, h, &ctxt)) != 0 ) > + return ret; > + > + t = &v->arch.phys_timer; It will avoid this hackish line. > + } > + } > + > + return ret; > +} > + > +static int hvm_vtimer_load_ctxt(struct domain *d, hvm_domain_context_t *h) > +{ > + int vcpuid; > + struct hvm_hw_timer ctxt; > + struct vcpu *v; > + struct vtimer *t = NULL; > + > + /* Which vcpu is this? */ > + vcpuid = hvm_load_instance(h); > + > + if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > + { > + dprintk(XENLOG_G_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; > + > + if ( ctxt.type == TIMER_TYPE_VIRT ) > + { > + t = &v->arch.virt_timer; > + d->arch.virt_timer_base.offset = ctxt.vtb_offset; > + } > + else else if ( ctx.type == TIMER_TYPE_PHYS ) ? Then fail if the ctx.type is wrong. Even better I would use a switch case. > + { > + t = &v->arch.phys_timer; > + d->arch.phys_timer_base.offset = ctxt.vtb_offset; Saving {virt,phys}_timer_base.offset which are per-domain seems a waste of space and confusing. > + } > + > + t->cval = ctxt.cval; > + t->ctl = ctxt.ctl; > + t->v = v; > + > + return 0; > +} > + [..] > diff --git a/xen/include/asm-arm/hvm/support.h b/xen/include/asm-arm/hvm/support.h > new file mode 100644 > index 0000000..09f7cb8 > --- /dev/null > +++ b/xen/include/asm-arm/hvm/support.h > @@ -0,0 +1,29 @@ > +/* > + * asm-arm/hvm/support.h: HVM support routines used by ARM. > + * > + * Copyright (c) 2014, Samsung Electronics. > + * > + * 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 > +#include > +#include > +#include > +#include > + > +#endif /* __ASM_ARM_HVM_SUPPORT_H__ */ > diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h > index 75b8e65..f6ad258 100644 > --- a/xen/include/public/arch-arm/hvm/save.h > +++ b/xen/include/public/arch-arm/hvm/save.h > @@ -26,6 +26,136 @@ > #ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__ > #define __XEN_PUBLIC_HVM_SAVE_ARM_H__ > > +#define HVM_FILE_MAGIC 0x92385520 > +#define HVM_FILE_VERSION 0x00000001 > + > +struct hvm_save_header > +{ > + uint32_t magic; /* Must be HVM_FILE_MAGIC */ > + uint32_t version; /* File format version */ > + uint64_t changeset; /* Version of Xen that saved this file */ > + uint32_t cpuid; /* MIDR_EL1 on the saving machine */ > +}; > +DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header); > + > +struct vgic_rank I would rename into vgic_v2_rank. > +{ > + uint32_t ienable, iactive, ipend, pendsgi; > + uint32_t icfg[2]; > + uint32_t ipriority[8]; > + uint32_t itargets[8]; > +}; > + > +struct hvm_hw_gic I would rename into hvm_hw_vgic. > +{ > + uint32_t gic_hcr; > + uint32_t gic_vmcr; > + uint32_t gic_apr; > + uint32_t gic_lr[64]; > + uint64_t event_mask; > + uint64_t lr_mask; > + struct vgic_rank ppi_state; As said previously, I would separate GIC from VGIC save/restore. -- Julien Grall