From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2] x86: generic MSRs save/restore Date: Mon, 16 Dec 2013 09:41:34 +0000 Message-ID: <52AED8DE020000780010D8AE@nat28.tlf.novell.com> References: <529F47D2020000780010A0E4@nat28.tlf.novell.com> <52AB2165020000780010D068@nat28.tlf.novell.com> <52AEC238020000780010D789@nat28.tlf.novell.com> <52AECD48020000780010D81B@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 16.12.13 at 10:13, "Liu, Jinsong" wrote: > Jan Beulich wrote: >>>>> On 16.12.13 at 09:39, "Liu, Jinsong" wrote: >>> Jan Beulich wrote: >>>>>>> On 16.12.13 at 04:01, "Liu, Jinsong" >>>>>>> wrote: >>>>> Jan Beulich wrote: >>>>>> This patch introduces a generic MSRs save/restore mechanism, so >>>>>> that in the future new MSRs save/restore could be added w/ >>>>>> smaller change than the full blown addition of a new save/restore >>>>>> type. >>>>>> >>>>>> Signed-off-by: Jan Beulich >>>>>> >>>>>> --- a/xen/arch/x86/hvm/hvm.c >>>>>> +++ b/xen/arch/x86/hvm/hvm.c >>>>>> @@ -1127,10 +1127,117 @@ 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; >>>>>> + >>>>>> + 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; >>>>>> + >>>>>> + if ( ctxt->count ) >>>>>> + h->cur += HVM_CPU_MSR_SIZE(ctxt->count); + >>>>>> else + h->cur -= sizeof(struct hvm_save_descriptor); + >>>>>> } + + return 0; >>>>>> +} >>>>>> + >>>>>> +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; + 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; >>>>>> + } >>>>>> + >>>>>> + for ( i = 0; i < ctxt->count; ++i ) >>>>>> + if ( ctxt->msr[i]._rsvd ) >>>>>> + return -EOPNOTSUPP; >>>>>> + /* Checking finished */ >>>>>> + >>>>>> + if ( hvm_funcs.load_msr ) >>>>>> + err = hvm_funcs.load_msr(v, ctxt); >>>>>> + >>>>>> + for ( i = 0; !err && i < ctxt->count; ++i ) >>>>>> + { >>>>>> + switch ( ctxt->msr[i].index ) >>>>>> + { >>>>>> + default: >>>>>> + if ( !ctxt->msr[i]._rsvd ) >>>>>> + err = -ENXIO; >>>>>> + break; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return err; >>>>>> +} >>>>>> + >>>>>> +/* We need variable length data chunks for XSAVE area and MSRs, >>>>>> hence + * a custom declaration rather than >>>>>> HVM_REGISTER_SAVE_RESTORE. */ -static int __init >>>>>> __hvm_register_CPU_XSAVE_save_and_restore(void) +static int __init >>>>>> hvm_register_CPU_save_and_restore(void) { >>>>>> hvm_register_savevm(CPU_XSAVE_CODE, >>>>>> "CPU_XSAVE", @@ -1139,9 +1246,22 @@ static int __init >>>>>> __hvm_register_CPU_XSA >>>>>> HVM_CPU_XSAVE_SIZE(xfeature_mask) + >>>>>> sizeof(struct hvm_save_descriptor), >>>>>> HVMSR_PER_VCPU); + + if ( hvm_funcs.init_msr ) >>>>>> + msr_count_max += hvm_funcs.init_msr(); >>>>>> + >>>>>> + if ( msr_count_max ) >>>>>> + hvm_register_savevm(CPU_MSR_CODE, >>>>>> + "CPU_MSR", >>>>>> + hvm_save_cpu_msrs, >>>>>> + hvm_load_cpu_msrs, >>>>>> + HVM_CPU_MSR_SIZE(msr_count_max) + >>>>>> + sizeof(struct >>>>>> hvm_save_descriptor), + >>>>>> HVMSR_PER_VCPU); + >>>>>> return 0; >>>>>> } >>>>>> -__initcall(__hvm_register_CPU_XSAVE_save_and_restore); >>>>>> +__initcall(hvm_register_CPU_save_and_restore); >>>>>> >>>>>> int hvm_vcpu_initialise(struct vcpu *v) >>>>>> { >>>>>> --- a/xen/include/asm-x86/hvm/hvm.h >>>>>> +++ b/xen/include/asm-x86/hvm/hvm.h >>>>>> @@ -109,6 +109,10 @@ struct hvm_function_table { >>>>>> void (*save_cpu_ctxt)(struct vcpu *v, struct hvm_hw_cpu >>>>>> *ctxt); int (*load_cpu_ctxt)(struct vcpu *v, struct >>>>>> hvm_hw_cpu *ctxt); >>>>>> >>>>>> + unsigned int (*init_msr)(void); >>>>>> + void (*save_msr)(struct vcpu *, struct hvm_msr *); >>>>>> + int (*load_msr)(struct vcpu *, struct hvm_msr *); + >>>>>> /* Examine specifics of the guest state. */ >>>>>> unsigned int (*get_interrupt_shadow)(struct vcpu *v); >>>>>> void (*set_interrupt_shadow)(struct vcpu *v, unsigned int >>>>>> intr_shadow); --- 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 20 >>>>>> + >>>>> >>>>> +DECLARE_HVM_SAVE_TYPE(CPU_MSR, CPU_MSR_CODE, struct hvm_msr); >>>>> +msr dump patch at tools/misc/xen-hvmctx.c >>>> >>>> Sorry, I don't follow what this is to mean. If that other patch of >>>> yours needs adjustment, then this surely doesn't belong here. >>>> >>> >>> OK, I will adjust tools side msr dump patch. >>> >>> DECLARE_HVM_SAVE_TYPE(CPU_MSR, CPU_MSR_CODE, struct hvm_msr) is for >>> msr dump tools side patch, so if you don't add it here I can use a >>> distinct patch to add it. >> >> The main thing is - this does _not_ belong in the public header, just >> like the XSAVE one has no such declaration there. > > XSAVE didn't get dumped at xen-hvmctx.c, it just forget (?) to do so, hence > it didn't declare at Xen side. But that doesn't change the fact that having the declaration in the public header is wrong for variable size save records. Jan