diff mbox

Use msr list to load and save msrs

Message ID 1256581565-20684-1-git-send-email-glommer@redhat.com
State New
Headers show

Commit Message

Glauber Costa Oct. 26, 2009, 6:26 p.m. UTC 
Since there is an ioctl that tells us which msrs are available,
use it. This saves us from the need of functions like has_star(),
lm_capable(), etc.

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Marcelo Tosatti <mtosatti@redhat.com>
CC: Avi Kivity <avi@redhat.com>
---
 target-i386/kvm.c |  200 ++++++++++++++++++++++++++---------------------------
 1 files changed, 98 insertions(+), 102 deletions(-)

Comments

Avi Kivity Oct. 27, 2009, 10:01 a.m. UTC | #1 
On 10/26/2009 08:26 PM, Glauber Costa wrote:
> +
> +    kvm_msr_list = kvm_get_msr_list(env);
> +    if (!kvm_msr_list) {
> +        printf("FAILED\n");
> +        return -1;
> +    }
> +
> +    msr_data.info.nmsrs = kvm_msr_list->nmsrs;
> +
> +    for (i = 0; i<  kvm_msr_list->nmsrs; i++) {
> +        uint64_t *data = kvm_get_msr_data_addr(env, kvm_msr_list->indices[i]);
> +        msrs[i].index = kvm_msr_list->indices[i];
> +        if (data != NULL) {
> +            msrs[i].data = *data;
> +        }
> +    }
>
>       return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
>
>    

Aren't you leaking the msr list structure?

Best to get it once during setup and reuse it later.
Glauber Costa Oct. 27, 2009, 10:19 a.m. UTC | #2 
On Tue, Oct 27, 2009 at 12:01:59PM +0200, Avi Kivity wrote:
> On 10/26/2009 08:26 PM, Glauber Costa wrote:
>> +
>> +    kvm_msr_list = kvm_get_msr_list(env);
>> +    if (!kvm_msr_list) {
>> +        printf("FAILED\n");
>> +        return -1;
>> +    }
>> +
>> +    msr_data.info.nmsrs = kvm_msr_list->nmsrs;
>> +
>> +    for (i = 0; i<  kvm_msr_list->nmsrs; i++) {
>> +        uint64_t *data = kvm_get_msr_data_addr(env, kvm_msr_list->indices[i]);
>> +        msrs[i].index = kvm_msr_list->indices[i];
>> +        if (data != NULL) {
>> +            msrs[i].data = *data;
>> +        }
>> +    }
>>
>>       return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
>>
>>    
>
> Aren't you leaking the msr list structure?
>
> Best to get it once during setup and reuse it later.

That's exactly what the function kvm_get_msr_list() does.
it allocs the structure the first time we use, and in subsequent
times, just return it.
Avi Kivity Oct. 27, 2009, 10:25 a.m. UTC | #3 
On 10/27/2009 12:19 PM, Glauber Costa wrote:
> On Tue, Oct 27, 2009 at 12:01:59PM +0200, Avi Kivity wrote:
>    
>> On 10/26/2009 08:26 PM, Glauber Costa wrote:
>>      
>>> +
>>> +    kvm_msr_list = kvm_get_msr_list(env);
>>> +    if (!kvm_msr_list) {
>>> +        printf("FAILED\n");
>>> +        return -1;
>>> +    }
>>> +
>>> +    msr_data.info.nmsrs = kvm_msr_list->nmsrs;
>>> +
>>> +    for (i = 0; i<   kvm_msr_list->nmsrs; i++) {
>>> +        uint64_t *data = kvm_get_msr_data_addr(env, kvm_msr_list->indices[i]);
>>> +        msrs[i].index = kvm_msr_list->indices[i];
>>> +        if (data != NULL) {
>>> +            msrs[i].data = *data;
>>> +        }
>>> +    }
>>>
>>>        return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
>>>
>>>
>>>        
>> Aren't you leaking the msr list structure?
>>
>> Best to get it once during setup and reuse it later.
>>      
> That's exactly what the function kvm_get_msr_list() does.
> it allocs the structure the first time we use, and in subsequent
> times, just return it.
>    

Oh, somehow I missed that.

(it's still better form to allocate on initialization, but that's not 
really a problem)
Glauber Costa Oct. 27, 2009, 10:32 a.m. UTC | #4 
On Tue, Oct 27, 2009 at 12:25:41PM +0200, Avi Kivity wrote:
> On 10/27/2009 12:19 PM, Glauber Costa wrote:
>> On Tue, Oct 27, 2009 at 12:01:59PM +0200, Avi Kivity wrote:
>>    
>>> On 10/26/2009 08:26 PM, Glauber Costa wrote:
>>>      
>>>> +
>>>> +    kvm_msr_list = kvm_get_msr_list(env);
>>>> +    if (!kvm_msr_list) {
>>>> +        printf("FAILED\n");
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    msr_data.info.nmsrs = kvm_msr_list->nmsrs;
>>>> +
>>>> +    for (i = 0; i<   kvm_msr_list->nmsrs; i++) {
>>>> +        uint64_t *data = kvm_get_msr_data_addr(env, kvm_msr_list->indices[i]);
>>>> +        msrs[i].index = kvm_msr_list->indices[i];
>>>> +        if (data != NULL) {
>>>> +            msrs[i].data = *data;
>>>> +        }
>>>> +    }
>>>>
>>>>        return kvm_vcpu_ioctl(env, KVM_SET_MSRS,&msr_data);
>>>>
>>>>
>>>>        
>>> Aren't you leaking the msr list structure?
>>>
>>> Best to get it once during setup and reuse it later.
>>>      
>> That's exactly what the function kvm_get_msr_list() does.
>> it allocs the structure the first time we use, and in subsequent
>> times, just return it.
>>    
>
> Oh, somehow I missed that.
>
> (it's still better form to allocate on initialization, but that's not  
> really a problem)
Yeah... there's no scenarion in which we don't need to allocate it, so it
is less confusing, indeed. I'll send that in v2 after people send in (hopefully)
more comments
diff mbox

Patch

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 7010999..488a3f5 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -221,49 +221,41 @@  int kvm_arch_init_vcpu(CPUState *env)
     return kvm_vcpu_ioctl(env, KVM_SET_CPUID2, &cpuid_data);
 }
 
-static int kvm_has_msr_star(CPUState *env)
+static struct kvm_msr_list *kvm_get_msr_list(CPUState *env)
 {
-    static int has_msr_star;
+    struct kvm_msr_list msr_list;
+    static struct kvm_msr_list *kvm_msr_list;
     int ret;
 
-    /* first time */
-    if (has_msr_star == 0) {        
-        struct kvm_msr_list msr_list, *kvm_msr_list;
+    if (kvm_msr_list)
+        return kvm_msr_list;
 
-        has_msr_star = -1;
+    /* Obtain MSR list from KVM.  These are the MSRs that we must
+     * save/restore */
+    msr_list.nmsrs = 0;
+    ret = kvm_ioctl(env->kvm_state, KVM_GET_MSR_INDEX_LIST, &msr_list);
+    if ((ret < 0) && (ret != -E2BIG)) {
+        printf("FAIL1 %d\n", ret);
+        return NULL;
+    }
 
-        /* Obtain MSR list from KVM.  These are the MSRs that we must
-         * save/restore */
-        msr_list.nmsrs = 0;
-        ret = kvm_ioctl(env->kvm_state, KVM_GET_MSR_INDEX_LIST, &msr_list);
-        if (ret < 0)
-            return 0;
-
-        /* Old kernel modules had a bug and could write beyond the provided
-           memory. Allocate at least a safe amount of 1K. */
-        kvm_msr_list = qemu_mallocz(MAX(1024, sizeof(msr_list) +
-                                              msr_list.nmsrs *
-                                              sizeof(msr_list.indices[0])));
-
-        kvm_msr_list->nmsrs = msr_list.nmsrs;
-        ret = kvm_ioctl(env->kvm_state, KVM_GET_MSR_INDEX_LIST, kvm_msr_list);
-        if (ret >= 0) {
-            int i;
-
-            for (i = 0; i < kvm_msr_list->nmsrs; i++) {
-                if (kvm_msr_list->indices[i] == MSR_STAR) {
-                    has_msr_star = 1;
-                    break;
-                }
-            }
-        }
+    /* Old kernel modules had a bug and could write beyond the provided
+       memory. Allocate at least a safe amount of 1K. */
+    kvm_msr_list = qemu_mallocz(MAX(1024, sizeof(msr_list) +
+                                          msr_list.nmsrs *
+                                          sizeof(msr_list.indices[0])));
+
+    kvm_msr_list->nmsrs = msr_list.nmsrs;
+    ret = kvm_ioctl(env->kvm_state, KVM_GET_MSR_INDEX_LIST, kvm_msr_list);
 
-        free(kvm_msr_list);
+    if (ret < 0) {
+        printf("FAIL2\n");
+        qemu_free(kvm_msr_list);
+        return NULL;
     }
 
-    if (has_msr_star == 1)
-        return 1;
-    return 0;
+    return kvm_msr_list;
+
 }
 
 int kvm_arch_init(KVMState *s, int smp_cpus)
@@ -455,13 +447,47 @@  static int kvm_put_sregs(CPUState *env)
     return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
 }
 
-static void kvm_msr_entry_set(struct kvm_msr_entry *entry,
-                              uint32_t index, uint64_t value)
+static uint64_t *kvm_get_msr_data_addr(CPUState *env, int index)
 {
-    entry->index = index;
-    entry->data = value;
+    uint64_t *addr;
+
+    switch (index) {
+    case MSR_IA32_SYSENTER_CS:
+        addr = (uint64_t *)&env->sysenter_cs;
+        break;
+    case MSR_IA32_SYSENTER_ESP:
+        addr = (uint64_t *)&env->sysenter_esp;
+        break;
+    case MSR_IA32_SYSENTER_EIP:
+        addr = (uint64_t *)&env->sysenter_eip;
+        break;
+    case MSR_STAR:
+        addr = &env->star;
+        break;
+#ifdef TARGET_X86_64
+    case MSR_CSTAR:
+        addr = &env->cstar;
+        break;
+    case MSR_KERNELGSBASE:
+        addr = &env->kernelgsbase;
+        break;
+    case MSR_FMASK:
+        addr = &env->fmask;
+        break;
+    case MSR_LSTAR:
+        addr = &env->lstar;
+        break;
+#endif
+    case MSR_IA32_TSC:
+        addr = &env->tsc;
+        break;
+    default:
+        addr = NULL;
+    }
+    return addr;
 }
 
+
 static int kvm_put_msrs(CPUState *env)
 {
     struct {
@@ -469,22 +495,24 @@  static int kvm_put_msrs(CPUState *env)
         struct kvm_msr_entry entries[100];
     } msr_data;
     struct kvm_msr_entry *msrs = msr_data.entries;
-    int n = 0;
-
-    kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS, env->sysenter_cs);
-    kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp);
-    kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip);
-    if (kvm_has_msr_star(env))
-	kvm_msr_entry_set(&msrs[n++], MSR_STAR, env->star);
-    kvm_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc);
-#ifdef TARGET_X86_64
-    /* FIXME if lm capable */
-    kvm_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar);
-    kvm_msr_entry_set(&msrs[n++], MSR_KERNELGSBASE, env->kernelgsbase);
-    kvm_msr_entry_set(&msrs[n++], MSR_FMASK, env->fmask);
-    kvm_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar);
-#endif
-    msr_data.info.nmsrs = n;
+    struct kvm_msr_list *kvm_msr_list;
+    int i;
+
+    kvm_msr_list = kvm_get_msr_list(env);
+    if (!kvm_msr_list) {
+        printf("FAILED\n");
+        return -1;
+    }
+
+    msr_data.info.nmsrs = kvm_msr_list->nmsrs;
+
+    for (i = 0; i < kvm_msr_list->nmsrs; i++) {
+        uint64_t *data = kvm_get_msr_data_addr(env, kvm_msr_list->indices[i]);
+        msrs[i].index = kvm_msr_list->indices[i];
+        if (data != NULL) {
+            msrs[i].data = *data;
+        }
+    }
 
     return kvm_vcpu_ioctl(env, KVM_SET_MSRS, &msr_data);
 
@@ -601,58 +629,26 @@  static int kvm_get_msrs(CPUState *env)
         struct kvm_msr_entry entries[100];
     } msr_data;
     struct kvm_msr_entry *msrs = msr_data.entries;
-    int ret, i, n;
-
-    n = 0;
-    msrs[n++].index = MSR_IA32_SYSENTER_CS;
-    msrs[n++].index = MSR_IA32_SYSENTER_ESP;
-    msrs[n++].index = MSR_IA32_SYSENTER_EIP;
-    if (kvm_has_msr_star(env))
-	msrs[n++].index = MSR_STAR;
-    msrs[n++].index = MSR_IA32_TSC;
-#ifdef TARGET_X86_64
-    /* FIXME lm_capable_kernel */
-    msrs[n++].index = MSR_CSTAR;
-    msrs[n++].index = MSR_KERNELGSBASE;
-    msrs[n++].index = MSR_FMASK;
-    msrs[n++].index = MSR_LSTAR;
-#endif
-    msr_data.info.nmsrs = n;
+    int ret, i;
+    struct kvm_msr_list *kvm_msr_list;
+
+    kvm_msr_list = kvm_get_msr_list(env);
+    if (!kvm_msr_list)
+        return -1;
+
+    msr_data.info.nmsrs = kvm_msr_list->nmsrs;
+    for (i = 0; i < kvm_msr_list->nmsrs; i++) {
+        msrs[i].index = kvm_msr_list->indices[i];
+    }
+
     ret = kvm_vcpu_ioctl(env, KVM_GET_MSRS, &msr_data);
     if (ret < 0)
         return ret;
 
     for (i = 0; i < ret; i++) {
-        switch (msrs[i].index) {
-        case MSR_IA32_SYSENTER_CS:
-            env->sysenter_cs = msrs[i].data;
-            break;
-        case MSR_IA32_SYSENTER_ESP:
-            env->sysenter_esp = msrs[i].data;
-            break;
-        case MSR_IA32_SYSENTER_EIP:
-            env->sysenter_eip = msrs[i].data;
-            break;
-        case MSR_STAR:
-            env->star = msrs[i].data;
-            break;
-#ifdef TARGET_X86_64
-        case MSR_CSTAR:
-            env->cstar = msrs[i].data;
-            break;
-        case MSR_KERNELGSBASE:
-            env->kernelgsbase = msrs[i].data;
-            break;
-        case MSR_FMASK:
-            env->fmask = msrs[i].data;
-            break;
-        case MSR_LSTAR:
-            env->lstar = msrs[i].data;
-            break;
-#endif
-        case MSR_IA32_TSC:
-            env->tsc = msrs[i].data;
-            break;
+        uint64_t *data = kvm_get_msr_data_addr(env, msrs[i].index);
+        if (data) {
+            *data = msrs[i].data;
         }
     }