From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v5 4/7] X86: generic MSRs save/restore Date: Fri, 13 Dec 2013 09:44:55 +0000 Message-ID: <52AAE527020000780010CE4C@nat28.tlf.novell.com> References: <529F47D2020000780010A0E4@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jinsong Liu Cc: Andrew Cooper , "xen-devel@lists.xen.org" , "keir@xen.org" , "Ian.Campbell@citrix.com" , "haoxudong.hao@gmail.com" List-Id: xen-devel@lists.xenproject.org >>> On 13.12.13 at 08:50, "Liu, Jinsong" wrote: Please get your reply quoting fixed. > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1127,10 +1127,110 @@ static int hvm_load_cpu_xsave_states(str > return 0; > } > > -/* We need variable length data chunk for xsave area, hence customized > - * declaration other than HVM_REGISTER_SAVE_RESTORE. > +#define HVM_CPU_MSR_SIZE(cnt) offsetof(struct hvm_msr, msr[cnt]) > +static unsigned int __read_mostly msr_count_max; > + > +static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h) > +{ > + struct vcpu *v; > + > + if ( !msr_count_max ) > + return 0; > > ASSERT( msr_count_mas ); ? Right. Left from before the registration was made conditional. > + > + for_each_vcpu ( d, v ) > + { > + struct hvm_msr *ctxt; > + unsigned int i; > + > + if ( _hvm_init_entry(h, CPU_MSR_CODE, v->vcpu_id, > + HVM_CPU_MSR_SIZE(msr_count_max)) ) > + return 1; > + ctxt = (struct hvm_msr *)&h->data[h->cur]; > + ctxt->count = 0; > + > + if ( hvm_funcs.save_msr ) > + hvm_funcs.save_msr(v, ctxt); > + > + for ( i = 0; i < ctxt->count; ++i ) > + ctxt->msr[i]._rsvd = 0; > > What _rsvd for? seems not used at the patches. See XSA-77 and http://lists.xenproject.org/archives/html/xen-devel/2013-12/msg01522.html - avoid leaking hypervisor data. > +static int hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h) > +{ > + unsigned int i, vcpuid = hvm_load_instance(h); > + struct vcpu *v; > + const struct hvm_save_descriptor *desc; > + const struct hvm_msr *ctxt; > + int err = 0; > + > + if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > + { > + dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vcpu%u\n", > + d->domain_id, vcpuid); > + return -EINVAL; > + } > + > + /* Customized checking for entry since our entry is of variable length */ > + desc = (struct hvm_save_descriptor *)&h->data[h->cur]; > + if ( sizeof (*desc) > h->size - h->cur) > + { > + printk(XENLOG_G_WARNING > + "HVM%d.%d restore: not enough data left to read MSR descriptor\n", > + d->domain_id, vcpuid); > + return -ENODATA; > + } > + if ( desc->length + sizeof (*desc) > h->size - h->cur) > + { > + printk(XENLOG_G_WARNING > + "HVM%d.%d restore: not enough data left to read %u MSR bytes\n", > + d->domain_id, vcpuid, desc->length); > + return -ENODATA; > + } > + if ( desc->length < HVM_CPU_MSR_SIZE(1) ) > + { > + printk(XENLOG_G_WARNING > + "HVM%d.%d restore mismatch: MSR length %u < %zu\n", > + d->domain_id, vcpuid, desc->length, HVM_CPU_MSR_SIZE(1)); > + return -EINVAL; > + } > + > + h->cur += sizeof(*desc); > + ctxt = (struct hvm_msr *)&h->data[h->cur]; > + h->cur += desc->length; > + > + if ( desc->length != HVM_CPU_MSR_SIZE(ctxt->count) ) > + { > + printk(XENLOG_G_WARNING > + "HVM%d.%d restore mismatch: MSR length %u != %zu\n", > + d->domain_id, vcpuid, desc->length, > + HVM_CPU_MSR_SIZE(ctxt->count)); > + return -EOPNOTSUPP; > + } > + /* Checking finished */ > + > + for ( i = 0; i < ctxt->count; ++i ) > + { > + if ( hvm_funcs.load_msr ) > + err = hvm_funcs.load_msr(v, &ctxt->msr[i]); > + if ( err ) > + break; > + } > > Is it for vmx/svm specific msrs, or, will extend to generic msrs? > If these patches are generic save/load mechanism supporting generic msrs and > vmx/sve specific msrs, it need update here. In which way? But I'll re-write this anyway, so that we don't need to handle MSRs one by one (leveraging the _rsvd field). > --- a/xen/include/public/arch-x86/hvm/save.h > +++ b/xen/include/public/arch-x86/hvm/save.h > @@ -592,9 +592,21 @@ struct hvm_tsc_adjust { > > DECLARE_HVM_SAVE_TYPE(TSC_ADJUST, 19, struct hvm_tsc_adjust); > > + > +struct hvm_msr { > + uint32_t count; > + struct hvm_one_msr { > + uint32_t index; > + uint32_t _rsvd; > + uint64_t val; > + } msr[1 /* variable size */]; > +}; > + > +#define CPU_MSR_CODE 16 > + > > It's incorrect, conflict with CPU_XSAVE_CODE and panic when > hvm_register_savevm(CPU_MSR_CODE, ...) > Why not 20? Copy-n-paste mistake. Thanks for spotting. Jan