Re: [Xen-devel] [PATCH v2] x86: generic MSRs save/restore

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Jan Beulich wrote:
>>>> On 16.12.13 at 14:23, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
>> Jan Beulich wrote:
>>>>>> On 16.12.13 at 11:05, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>> wrote: 
>>>> Jan Beulich wrote:
>>>>>>>> On 16.12.13 at 10:53, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>>>> wrote:
>>>>>> @@ -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.
>>>>> 
>>>>> Right, because you ought to use CPU_MSR_CODE here, which the
>>>>> public header does define.
>>>> 
>>>> Isn't it ugly? not unified style w/ other typecodes.
>>>> Why not simply add declaration at Xen side?
>>> 
>>> We absolutely should not declare things that aren't correct.
>> 
>> You have had the declaration in the public header with variable size
>> data structure: struct hvm_msr {
>>     uint32_t count;
>>     struct hvm_one_msr {
>>         uint32_t index;
>>         uint32_t _rsvd;
>>         uint64_t val;
>>     } msr[1 /* variable size */];
>> };
>> Why can't declare DECLARE_HVM_SAVE_TYPE(CPU_MSR, CPU_MSR_CODE, struct
>> hvm_msr)? They are both variable size.
> 
> That's not the point. We should make it impossible for someone to
> erroneously use in particular HVM_SAVE_LENGTH() for variable
> size records.
> 
> Anyway - why don't you simply bring this up with your colleagues
> who introduced the XSAVE record? I'm really just following its
> model...
> 

They are my ex-colleagues :(

Anyway, I adjust msr dump tools side patch:

diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c
index 5a69245..8a4614d 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;
+    struct hvm_msr *p = (struct hvm_msr *)(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 CPU_MSR_CODE: dump_cpu_msr(); break;
         case HVM_SAVE_CODE(END): break;
         default:
             printf(" ** Don't understand type %u: skipping\n",


Thanks,
Jinsong


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel