|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] x86: generic MSRs save/restore
Jan Beulich wrote:
>>>> On 16.12.13 at 10:13, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>> Jan Beulich wrote:
>>>>>> On 16.12.13 at 09:39, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>> wrote:
>>>> Jan Beulich wrote:
>>>>>>>> On 16.12.13 at 04:01, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>>>> 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 <jbeulich@xxxxxxxx>
>>>>>>>
>>>>>>> --- 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
We need this declaration in the public header for compiling reason, then handle
variable size data specifically, like:
=================
diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c
index 5a69245..4b33450 100644
--- a/tools/misc/xen-hvmctx.c
+++ b/tools/misc/xen-hvmctx.c
@@ -399,6 +399,26 @@ static void dump_tsc_adjust(void)
printf(" TSC_ADJUST: tsc_adjust %" PRIx64 "\n", p.tsc_adjust);
}
+#define HVM_CPU_MSR_SIZE(cnt) offsetof(struct hvm_msr, msr[cnt])
+static void dump_cpu_msr(void)
+{
+ int i;
+ HVM_SAVE_TYPE(CPU_MSR) *p = (void *)(buf + off);
+
+ if ( len - off < HVM_CPU_MSR_SIZE(p->count) )
+ {
+ fprintf(stderr, "Error: need another %u bytes, only %u available",
+ (unsigned int)HVM_CPU_MSR_SIZE(p->count), len - off);
+ exit(1);
+ }
+
+ for ( i = 0; i < p->count; i++ )
+ printf(" CPU_MSR: msr %" PRIx32 " val %" PRIx64 "\n",
+ p->msr[i].index, p->msr[i].val);
+
+ off += HVM_CPU_MSR_SIZE(p->count);
+}
+
int main(int argc, char **argv)
{
int entry, domid;
@@ -467,6 +487,7 @@ int main(int argc, char **argv)
case HVM_SAVE_CODE(VIRIDIAN_VCPU): dump_viridian_vcpu(); break;
case HVM_SAVE_CODE(VMCE_VCPU): dump_vmce_vcpu(); break;
case HVM_SAVE_CODE(TSC_ADJUST): dump_tsc_adjust(); break;
+ case HVM_SAVE_CODE(CPU_MSR): dump_cpu_msr(); break;
case HVM_SAVE_CODE(END): break;
default:
printf(" ** Don't understand type %u: skipping\n",
=======================
w/o the declaration, case HVM_SAVE_CODE(CPU_MSR) compiling fail.
Thanks,
Jinsong
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |