[PATCH] target/i386: Add kvm_get_one_msr helper

[PATCH] target/i386: Add kvm_get_one_msr helper

[Yang Weijiang]

When try to get one msr from KVM, I found there's no such kind of
existing interface while kvm_put_one_msr() is there. So here comes
the patch. It'll remove redundant preparation code before finally
call KVM_GET_MSRS IOCTL.

No functional change intended.

Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 target/i386/kvm/kvm.c | 48 ++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 2c8feb4a6f..c897dbaf60 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -135,6 +135,7 @@ static struct kvm_msr_list *kvm_feature_msrs;
 
 #define BUS_LOCK_SLICE_TIME 1000000000ULL /* ns */
 static RateLimit bus_lock_ratelimit_ctrl;
+static int kvm_get_one_msr(X86CPU *cpu, int index, uint64_t *value);
 
 int kvm_has_pit_state2(void)
 {
@@ -205,28 +206,21 @@ static int kvm_get_tsc(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
-    struct {
-        struct kvm_msrs info;
-        struct kvm_msr_entry entries[1];
-    } msr_data = {};
+    uint64_t value;
     int ret;
 
     if (env->tsc_valid) {
         return 0;
     }
 
-    memset(&msr_data, 0, sizeof(msr_data));
-    msr_data.info.nmsrs = 1;
-    msr_data.entries[0].index = MSR_IA32_TSC;
     env->tsc_valid = !runstate_is_running();
 
-    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
+    ret = kvm_get_one_msr(cpu, MSR_IA32_TSC, &value);
     if (ret < 0) {
         return ret;
     }
 
-    assert(ret == 1);
-    env->tsc = msr_data.entries[0].data;
+    env->tsc = value;
     return 0;
 }
 
@@ -1478,21 +1472,14 @@ static int hyperv_init_vcpu(X86CPU *cpu)
          * the kernel doesn't support setting vp_index; assert that its value
          * is in sync
          */
-        struct {
-            struct kvm_msrs info;
-            struct kvm_msr_entry entries[1];
-        } msr_data = {
-            .info.nmsrs = 1,
-            .entries[0].index = HV_X64_MSR_VP_INDEX,
-        };
-
-        ret = kvm_vcpu_ioctl(cs, KVM_GET_MSRS, &msr_data);
+        uint64_t value;
+
+        ret = kvm_get_one_msr(cpu, HV_X64_MSR_VP_INDEX, &value);
         if (ret < 0) {
             return ret;
         }
-        assert(ret == 1);
 
-        if (msr_data.entries[0].data != hyperv_vp_index(CPU(cpu))) {
+        if (value != hyperv_vp_index(CPU(cpu))) {
             error_report("kernel's vp_index != QEMU's vp_index");
             return -ENXIO;
         }
@@ -2734,6 +2721,25 @@ static int kvm_put_one_msr(X86CPU *cpu, int index, uint64_t value)
     return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
 }
 
+static int kvm_get_one_msr(X86CPU *cpu, int index, uint64_t *value)
+{
+    int ret;
+    struct {
+        struct kvm_msrs info;
+        struct kvm_msr_entry entries[1];
+    } msr_data = {
+        .info.nmsrs = 1,
+        .entries[0].index = index,
+    };
+
+    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
+    if (ret < 0) {
+        return ret;
+    }
+    assert(ret == 1);
+    *value = msr_data.entries[0].data;
+    return ret;
+}
 void kvm_put_apicbase(X86CPU *cpu, uint64_t value)
 {
     int ret;
-- 
2.27.0

Re: [PATCH] target/i386: Add kvm_get_one_msr helper

[Paolo Bonzini]

On 1/27/22 16:58, Yang Weijiang wrote:
> @@ -135,6 +135,7 @@ static struct kvm_msr_list *kvm_feature_msrs;
>   
>   #define BUS_LOCK_SLICE_TIME 1000000000ULL /* ns */
>   static RateLimit bus_lock_ratelimit_ctrl;
> +static int kvm_get_one_msr(X86CPU *cpu, int index, uint64_t *value);
>   
>   int kvm_has_pit_state2(void)
>   {

...

> @@ -2734,6 +2721,25 @@ static int kvm_put_one_msr(X86CPU *cpu, int index, uint64_t value)
>       return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
>   }
>   
> +static int kvm_get_one_msr(X86CPU *cpu, int index, uint64_t *value)
> +{
> +    int ret;
> +    struct {
> +        struct kvm_msrs info;
> +        struct kvm_msr_entry entries[1];
> +    } msr_data = {
> +        .info.nmsrs = 1,
> +        .entries[0].index = index,
> +    };
> +
> +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    assert(ret == 1);
> +    *value = msr_data.entries[0].data;
> +    return ret;
> +}
>   void kvm_put_apicbase(X86CPU *cpu, uint64_t value)
>   {
>       int ret;

The patch is a good idea, but you can put the function before the uses. 
  This way there will be no need for a forward declaration, either.

Thanks,

Paolo

Re: [PATCH] target/i386: Add kvm_get_one_msr helper

[Yang, Weijiang]

On 1/28/2022 6:55 PM, Paolo Bonzini wrote:
> On 1/27/22 16:58, Yang Weijiang wrote:
>> @@ -135,6 +135,7 @@ static struct kvm_msr_list *kvm_feature_msrs;
>>     #define BUS_LOCK_SLICE_TIME 1000000000ULL /* ns */
>>   static RateLimit bus_lock_ratelimit_ctrl;
>> +static int kvm_get_one_msr(X86CPU *cpu, int index, uint64_t *value);
>>     int kvm_has_pit_state2(void)
>>   {
>
> ...
>
>> @@ -2734,6 +2721,25 @@ static int kvm_put_one_msr(X86CPU *cpu, int 
>> index, uint64_t value)
>>       return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
>>   }
>>   +static int kvm_get_one_msr(X86CPU *cpu, int index, uint64_t *value)
>> +{
>> +    int ret;
>> +    struct {
>> +        struct kvm_msrs info;
>> +        struct kvm_msr_entry entries[1];
>> +    } msr_data = {
>> +        .info.nmsrs = 1,
>> +        .entries[0].index = index,
>> +    };
>> +
>> +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    assert(ret == 1);
>> +    *value = msr_data.entries[0].data;
>> +    return ret;
>> +}
>>   void kvm_put_apicbase(X86CPU *cpu, uint64_t value)
>>   {
>>       int ret;
>
> The patch is a good idea, but you can put the function before the 
> uses.  This way there will be no need for a forward declaration, either.

Thanks Paolo!

Is v2 version required for this?

>
> Thanks,
>
> Paolo