Add kvm_get_supported_feature_msrs() to get supported MSR feature index list.
Add kvm_arch_get_supported_msr_feature() to get each MSR features value.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 include/sysemu/kvm.h |  2 ++
 target/i386/cpu.c    |  7 ++---
 target/i386/kvm.c    | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 0b64b8e..97d8d9d 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
 
 uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
+uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
+
 
 void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a252c26..0160e97 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3670,7 +3670,7 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only)
 {
     FeatureWordInfo *wi = &feature_word_info[w];
-    uint32_t r;
+    uint32_t r = 0;
 
     if (kvm_enabled()) {
         switch (wi->type) {
@@ -3679,8 +3679,9 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                 wi->cpuid.ecx,
                                                 wi->cpuid.reg);
             break;
-        default:
-            r = 0;
+        case MSR_FEATURE_WORD:
+            r = kvm_arch_get_supported_msr_feature(kvm_state,
+                        wi->msr.index);
             break;
         }
     } else if (hvf_enabled()) {
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 0b2a07d..bfd8088 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -107,6 +107,7 @@ static int has_pit_state2;
 static bool has_msr_mcg_ext_ctl;
 
 static struct kvm_cpuid2 *cpuid_cache;
+static struct kvm_msr_list *kvm_feature_msrs;
 
 int kvm_has_pit_state2(void)
 {
@@ -420,6 +421,33 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
     return ret;
 }
 
+uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index)
+{
+    struct {
+        struct kvm_msrs info;
+        struct kvm_msr_entry entries[1];
+    } msr_data;
+    uint32_t ret;
+
+    if (kvm_feature_msrs == NULL) { /*ARCH doesn't support feature MSRs*/
+        return 0;
+    }
+
+    msr_data.info.nmsrs = 1;
+    msr_data.entries[0].index = index;
+
+    ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data);
+
+    if (ret != 1) {
+        fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n",
+            index, strerror(-ret));
+        exit(1);
+    }
+
+    return msr_data.entries[0].data;
+}
+
+
 typedef struct HWPoisonPage {
     ram_addr_t ram_addr;
     QLIST_ENTRY(HWPoisonPage) list;
@@ -1239,6 +1267,45 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu)
     }
 }
 
+static int kvm_get_supported_feature_msrs(KVMState *s)
+{
+    int ret = 0;
+
+    if (kvm_feature_msrs != NULL) {
+        return 0;
+    }
+
+    if (!kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES)) {
+        return -1;
+    }
+
+    struct kvm_msr_list msr_list;
+
+    msr_list.nmsrs = 0;
+    ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list);
+    if (ret < 0 && ret != -E2BIG) {
+        return ret;
+    }
+
+    assert(msr_list.nmsrs > 0);
+    kvm_feature_msrs = (struct kvm_msr_list *) \
+        g_malloc0(sizeof(msr_list) +
+                 msr_list.nmsrs * sizeof(msr_list.indices[0]));
+
+    kvm_feature_msrs->nmsrs = msr_list.nmsrs;
+    ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, kvm_feature_msrs);
+
+    if (ret < 0) {
+        fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n",
+            strerror(-ret));
+        g_free(kvm_feature_msrs);
+        kvm_feature_msrs = NULL;
+        return ret;
+    }
+
+    return 0;
+}
+
 static int kvm_get_supported_msrs(KVMState *s)
 {
     static int kvm_supported_msrs;
@@ -1392,6 +1459,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         return ret;
     }
 
+    ret = kvm_get_supported_feature_msrs(s);
+    if (ret < 0) { /*if MSR based features aren't supported, ignore it.*/
+        warn_report("Get supported feature MSRs failed.");
+    }
+
     uname(&utsname);
     lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
 
-- 
1.8.3.1

Hi,

Thanks for the patch and sorry for taking so long to review it.

On Sun, Sep 02, 2018 at 07:46:06PM +0800, Robert Hoo wrote:
> Add kvm_get_supported_feature_msrs() to get supported MSR feature index list.
> Add kvm_arch_get_supported_msr_feature() to get each MSR features value.
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
>  include/sysemu/kvm.h |  2 ++
>  target/i386/cpu.c    |  7 ++---
>  target/i386/kvm.c    | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 0b64b8e..97d8d9d 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
>  
>  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
>                                        uint32_t index, int reg);
> +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index);
> +
>  
>  void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>  
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a252c26..0160e97 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3670,7 +3670,7 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>                                                     bool migratable_only)
>  {
>      FeatureWordInfo *wi = &feature_word_info[w];
> -    uint32_t r;
> +    uint32_t r = 0;
>  
>      if (kvm_enabled()) {
>          switch (wi->type) {
> @@ -3679,8 +3679,9 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
>                                                  wi->cpuid.ecx,
>                                                  wi->cpuid.reg);
>              break;
> -        default:
> -            r = 0;
> +        case MSR_FEATURE_WORD:
> +            r = kvm_arch_get_supported_msr_feature(kvm_state,
> +                        wi->msr.index);

If you move this patch before patch 1/3, this hunk could be part
of patch 1/3.

>              break;
>          }
>      } else if (hvf_enabled()) {
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 0b2a07d..bfd8088 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -107,6 +107,7 @@ static int has_pit_state2;
>  static bool has_msr_mcg_ext_ctl;
>  
>  static struct kvm_cpuid2 *cpuid_cache;
> +static struct kvm_msr_list *kvm_feature_msrs;
>  
>  int kvm_has_pit_state2(void)
>  {
> @@ -420,6 +421,33 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>      return ret;
>  }
>  
> +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index)
> +{
> +    struct {
> +        struct kvm_msrs info;
> +        struct kvm_msr_entry entries[1];
> +    } msr_data;
> +    uint32_t ret;
> +
> +    if (kvm_feature_msrs == NULL) { /*ARCH doesn't support feature MSRs*/

Nit: normally comments have spaces after "/*" and before "*/".

Also: what do you mean by "ARCH"?  Do you mean "host kernel"?


> +        return 0;
> +    }
> +
> +    msr_data.info.nmsrs = 1;
> +    msr_data.entries[0].index = index;
> +
> +    ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data);
> +
> +    if (ret != 1) {

If the MSR is not supported by the host kernel, it must not be a
fatal error.  We should just return 0 on that case.

Probably the best way to ensure that is to check if the MSR is
listed on kvm_feature_msrs before calling KVM_GET_MSRS (and
return 0 if the MSR is not on the list).


> +        fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n",
> +            index, strerror(-ret));

Please use error_report() instead of fprintf(stderr).

> +        exit(1);

I'm unsure if exit(1) is the best option here, but at least this
is consistent with error handling kvm_arch_get_supported_cpuid().


> +    }
> +
> +    return msr_data.entries[0].data;
> +}
> +
> +
>  typedef struct HWPoisonPage {
>      ram_addr_t ram_addr;
>      QLIST_ENTRY(HWPoisonPage) list;
> @@ -1239,6 +1267,45 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu)
>      }
>  }
>  
> +static int kvm_get_supported_feature_msrs(KVMState *s)
> +{
> +    int ret = 0;
> +
> +    if (kvm_feature_msrs != NULL) {
> +        return 0;
> +    }
> +
> +    if (!kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES)) {
> +        return -1;

There's nothing wrong with not supporting
KVM_CAP_GET_MSR_FEATURES.  Why not return 0?


> +    }
> +
> +    struct kvm_msr_list msr_list;
> +
> +    msr_list.nmsrs = 0;
> +    ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list);
> +    if (ret < 0 && ret != -E2BIG) {

You print an error to stderr if (ret < 0) below, but don't print
anything here.  Seems inconsistent.

> +        return ret;
> +    }
> +
> +    assert(msr_list.nmsrs > 0);
> +    kvm_feature_msrs = (struct kvm_msr_list *) \
> +        g_malloc0(sizeof(msr_list) +
> +                 msr_list.nmsrs * sizeof(msr_list.indices[0]));
> +
> +    kvm_feature_msrs->nmsrs = msr_list.nmsrs;
> +    ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, kvm_feature_msrs);

kvm_arch_get_supported_msr_feature() is only checking if
kvm_feature_msrs is NULL, and nothing else.

What exactly is the point of calling
KVM_GET_MSR_FEATURE_INDEX_LIST and saving data at
kvm_feature_msrs, if no other code is ever looking at the
returned data?


> +
> +    if (ret < 0) {
> +        fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n",
> +            strerror(-ret));

Please use error_report() instead of fprintf(stderr).

> +        g_free(kvm_feature_msrs);
> +        kvm_feature_msrs = NULL;
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
>  static int kvm_get_supported_msrs(KVMState *s)
>  {
>      static int kvm_supported_msrs;
> @@ -1392,6 +1459,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>          return ret;
>      }
>  
> +    ret = kvm_get_supported_feature_msrs(s);
> +    if (ret < 0) { /*if MSR based features aren't supported, ignore it.*/
> +        warn_report("Get supported feature MSRs failed.");

We must not print a warning only because KVM_CAP_GET_MSR_FEATURES
isn't supported by the host kernel.

If KVM_GET_MSR_FEATURE_INDEX_LIST fails, on the other hand, we
probably should make it a fatal error and not a warning.

> +    }
> +
>      uname(&utsname);
>      lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
>  
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

On Thu, 2018-09-20 at 00:07 -0300, Eduardo Habkost wrote:
> Hi,
> 
> Thanks for the patch and sorry for taking so long to review it.

Never mind. I understand you're really busy. :-)
> 
> On Sun, Sep 02, 2018 at 07:46:06PM +0800, Robert Hoo wrote:
> > Add kvm_get_supported_feature_msrs() to get supported MSR feature
> > index list.
> > Add kvm_arch_get_supported_msr_feature() to get each MSR features
> > value.
> > 
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > ---
> >  include/sysemu/kvm.h |  2 ++
> >  target/i386/cpu.c    |  7 ++---
> >  target/i386/kvm.c    | 72
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 78 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 0b64b8e..97d8d9d 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s,
> > unsigned int extension);
> >  
> >  uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t
> > function,
> >                                        uint32_t index, int reg);
> > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t
> > index);
> > +
> >  
> >  void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
> >  
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index a252c26..0160e97 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -3670,7 +3670,7 @@ static uint32_t
> > x86_cpu_get_supported_feature_word(FeatureWord w,
> >                                                     bool
> > migratable_only)
> >  {
> >      FeatureWordInfo *wi = &feature_word_info[w];
> > -    uint32_t r;
> > +    uint32_t r = 0;
> >  
> >      if (kvm_enabled()) {
> >          switch (wi->type) {
> > @@ -3679,8 +3679,9 @@ static uint32_t
> > x86_cpu_get_supported_feature_word(FeatureWord w,
> >                                                  wi->cpuid.ecx,
> >                                                  wi->cpuid.reg);
> >              break;
> > -        default:
> > -            r = 0;
> > +        case MSR_FEATURE_WORD:
> > +            r = kvm_arch_get_supported_msr_feature(kvm_state,
> > +                        wi->msr.index);
> 
> If you move this patch before patch 1/3, this hunk could be part
> of patch 1/3.
> 
I'm afraid that if I moved this hunk, because of the dependency, I
would have to move the definition of
kvm_arch_get_supported_msr_feature() to patch 1/3, then in turn, it
used kvm_feature_msrs, I've to put its definition and initialization
function kvm_get_supported_feature_msrs() to patch 1/3 as well. Then
actually, this makes patch 1/3 and 2/3 merged into 1. Would you like me
to do so?

> >              break;
> >          }
> >      } else if (hvf_enabled()) {
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index 0b2a07d..bfd8088 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -107,6 +107,7 @@ static int has_pit_state2;
> >  static bool has_msr_mcg_ext_ctl;
> >  
> >  static struct kvm_cpuid2 *cpuid_cache;
> > +static struct kvm_msr_list *kvm_feature_msrs;
> >  
> >  int kvm_has_pit_state2(void)
> >  {
> > @@ -420,6 +421,33 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState
> > *s, uint32_t function,
> >      return ret;
> >  }
> >  
> > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t
> > index)
> > +{
> > +    struct {
> > +        struct kvm_msrs info;
> > +        struct kvm_msr_entry entries[1];
> > +    } msr_data;
> > +    uint32_t ret;
> > +
> > +    if (kvm_feature_msrs == NULL) { /*ARCH doesn't support feature
> > MSRs*/
> 
> Nit: normally comments have spaces after "/*" and before "*/".
> 
> Also: what do you mean by "ARCH"?  Do you mean "host kernel"?
> 
Going to say "Host".

> 
> > +        return 0;
> > +    }
> > +
> > +    msr_data.info.nmsrs = 1;
> > +    msr_data.entries[0].index = index;
> > +
> > +    ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data);
> > +
> > +    if (ret != 1) {
> 
> If the MSR is not supported by the host kernel, it must not be a
> fatal error.  We should just return 0 on that case.
> 
> Probably the best way to ensure that is to check if the MSR is
> listed on kvm_feature_msrs before calling KVM_GET_MSRS (and
> return 0 if the MSR is not on the list).

Yes. Will do in this way in v5. 
> 
> 
> > +        fprintf(stderr, "KVM get MSR (index=0x%x) feature failed,
> > %s\n",
> > +            index, strerror(-ret));
> 
> Please use error_report() instead of fprintf(stderr).
> 
> > +        exit(1);
> 
> I'm unsure if exit(1) is the best option here, but at least this
> is consistent with error handling kvm_arch_get_supported_cpuid().
> 
> 
> > +    }
> > +
> > +    return msr_data.entries[0].data;
> > +}
> > +
> > +
> >  typedef struct HWPoisonPage {
> >      ram_addr_t ram_addr;
> >      QLIST_ENTRY(HWPoisonPage) list;
> > @@ -1239,6 +1267,45 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu)
> >      }
> >  }
> >  
> > +static int kvm_get_supported_feature_msrs(KVMState *s)
> > +{
> > +    int ret = 0;
> > +
> > +    if (kvm_feature_msrs != NULL) {
> > +        return 0;
> > +    }
> > +
> > +    if (!kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES)) {
> > +        return -1;
> 
> There's nothing wrong with not supporting
> KVM_CAP_GET_MSR_FEATURES.  Why not return 0?
> 
OK
> 
> > +    }
> > +
> > +    struct kvm_msr_list msr_list;
> > +
> > +    msr_list.nmsrs = 0;
> > +    ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list);
> > +    if (ret < 0 && ret != -E2BIG) {
> 
> You print an error to stderr if (ret < 0) below, but don't print
> anything here.  Seems inconsistent.

Going to be consistent.
> 
> > +        return ret;
> > +    }
> > +
> > +    assert(msr_list.nmsrs > 0);
> > +    kvm_feature_msrs = (struct kvm_msr_list *) \
> > +        g_malloc0(sizeof(msr_list) +
> > +                 msr_list.nmsrs * sizeof(msr_list.indices[0]));
> > +
> > +    kvm_feature_msrs->nmsrs = msr_list.nmsrs;
> > +    ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST,
> > kvm_feature_msrs);
> 
> kvm_arch_get_supported_msr_feature() is only checking if
> kvm_feature_msrs is NULL, and nothing else.
> 
> What exactly is the point of calling
> KVM_GET_MSR_FEATURE_INDEX_LIST and saving data at
> kvm_feature_msrs, if no other code is ever looking at the
> returned data?
> 
Now above change make it useful for checking requested feature MSR's
validity.
> 
> > +
> > +    if (ret < 0) {
> > +        fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n",
> > +            strerror(-ret));
> 
> Please use error_report() instead of fprintf(stderr).

OK
> 
> > +        g_free(kvm_feature_msrs);
> > +        kvm_feature_msrs = NULL;
> > +        return ret;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int kvm_get_supported_msrs(KVMState *s)
> >  {
> >      static int kvm_supported_msrs;
> > @@ -1392,6 +1459,11 @@ int kvm_arch_init(MachineState *ms, KVMState
> > *s)
> >          return ret;
> >      }
> >  
> > +    ret = kvm_get_supported_feature_msrs(s);
> > +    if (ret < 0) { /*if MSR based features aren't supported,
> > ignore it.*/
> > +        warn_report("Get supported feature MSRs failed.");
> 
> We must not print a warning only because KVM_CAP_GET_MSR_FEATURES
> isn't supported by the host kernel.
> 
> If KVM_GET_MSR_FEATURE_INDEX_LIST fails, on the other hand, we
> probably should make it a fatal error and not a warning.
> 
OK, remove the check.

> > +    }
> > +
> >      uname(&utsname);
> >      lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
> >  
> > -- 
> > 1.8.3.1
> > 
> > 
> 
> 

On Thu, Sep 20, 2018 at 03:45:42PM +0800, Robert Hoo wrote:
[...]
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index a252c26..0160e97 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -3670,7 +3670,7 @@ static uint32_t
> > > x86_cpu_get_supported_feature_word(FeatureWord w,
> > >                                                     bool
> > > migratable_only)
> > >  {
> > >      FeatureWordInfo *wi = &feature_word_info[w];
> > > -    uint32_t r;
> > > +    uint32_t r = 0;
> > >  
> > >      if (kvm_enabled()) {
> > >          switch (wi->type) {
> > > @@ -3679,8 +3679,9 @@ static uint32_t
> > > x86_cpu_get_supported_feature_word(FeatureWord w,
> > >                                                  wi->cpuid.ecx,
> > >                                                  wi->cpuid.reg);
> > >              break;
> > > -        default:
> > > -            r = 0;
> > > +        case MSR_FEATURE_WORD:
> > > +            r = kvm_arch_get_supported_msr_feature(kvm_state,
> > > +                        wi->msr.index);
> > 
> > If you move this patch before patch 1/3, this hunk could be part
> > of patch 1/3.
> > 
> I'm afraid that if I moved this hunk, because of the dependency, I
> would have to move the definition of
> kvm_arch_get_supported_msr_feature() to patch 1/3, then in turn, it
> used kvm_feature_msrs, I've to put its definition and initialization
> function kvm_get_supported_feature_msrs() to patch 1/3 as well. Then
> actually, this makes patch 1/3 and 2/3 merged into 1. Would you like me
> to do so?

I don't get it.  Why would you need to move the definition of
kvm_arch_get_supported_cpuid() to patch 1/3, if we change the
patch order and apply this patch before patch 1/3?

-- 
Eduardo

On Thu, 2018-09-20 at 14:22 -0300, Eduardo Habkost wrote:
> On Thu, Sep 20, 2018 at 03:45:42PM +0800, Robert Hoo wrote:
> [...]
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index a252c26..0160e97 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -3670,7 +3670,7 @@ static uint32_t
> > > > x86_cpu_get_supported_feature_word(FeatureWord w,
> > > >                                                     bool
> > > > migratable_only)
> > > >  {
> > > >      FeatureWordInfo *wi = &feature_word_info[w];
> > > > -    uint32_t r;
> > > > +    uint32_t r = 0;
> > > >  
> > > >      if (kvm_enabled()) {
> > > >          switch (wi->type) {
> > > > @@ -3679,8 +3679,9 @@ static uint32_t
> > > > x86_cpu_get_supported_feature_word(FeatureWord w,
> > > >                                                  wi->cpuid.ecx,
> > > >                                                  wi-
> > > > >cpuid.reg);
> > > >              break;
> > > > -        default:
> > > > -            r = 0;
> > > > +        case MSR_FEATURE_WORD:
> > > > +            r = kvm_arch_get_supported_msr_feature(kvm_state,
> > > > +                        wi->msr.index);
> > > 
> > > If you move this patch before patch 1/3, this hunk could be part
> > > of patch 1/3.
> > > 
> > 
> > I'm afraid that if I moved this hunk, because of the dependency, I
> > would have to move the definition of
> > kvm_arch_get_supported_msr_feature() to patch 1/3, then in turn, it
> > used kvm_feature_msrs, I've to put its definition and
> > initialization
> > function kvm_get_supported_feature_msrs() to patch 1/3 as well.
> > Then
> > actually, this makes patch 1/3 and 2/3 merged into 1. Would you
> > like me
> > to do so?
> 
> I don't get it.  Why would you need to move the definition of
> kvm_arch_get_supported_cpuid() to patch 1/3, if we change the
> patch order and apply this patch before patch 1/3?
> 
OK, so you mean simply switch the 1/3 and 2/3 order, not move some
hunks of 2/3 into 1/3. Am I right?

On Fri, Sep 21, 2018 at 08:28:24AM +0800, Robert Hoo wrote:
> On Thu, 2018-09-20 at 14:22 -0300, Eduardo Habkost wrote:
> > On Thu, Sep 20, 2018 at 03:45:42PM +0800, Robert Hoo wrote:
> > [...]
> > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > > index a252c26..0160e97 100644
> > > > > --- a/target/i386/cpu.c
> > > > > +++ b/target/i386/cpu.c
> > > > > @@ -3670,7 +3670,7 @@ static uint32_t
> > > > > x86_cpu_get_supported_feature_word(FeatureWord w,
> > > > >                                                     bool
> > > > > migratable_only)
> > > > >  {
> > > > >      FeatureWordInfo *wi = &feature_word_info[w];
> > > > > -    uint32_t r;
> > > > > +    uint32_t r = 0;
> > > > >  
> > > > >      if (kvm_enabled()) {
> > > > >          switch (wi->type) {
> > > > > @@ -3679,8 +3679,9 @@ static uint32_t
> > > > > x86_cpu_get_supported_feature_word(FeatureWord w,
> > > > >                                                  wi->cpuid.ecx,
> > > > >                                                  wi-
> > > > > >cpuid.reg);
> > > > >              break;
> > > > > -        default:
> > > > > -            r = 0;
> > > > > +        case MSR_FEATURE_WORD:
> > > > > +            r = kvm_arch_get_supported_msr_feature(kvm_state,
> > > > > +                        wi->msr.index);
> > > > 
> > > > If you move this patch before patch 1/3, this hunk could be part
> > > > of patch 1/3.
> > > > 
> > > 
> > > I'm afraid that if I moved this hunk, because of the dependency, I
> > > would have to move the definition of
> > > kvm_arch_get_supported_msr_feature() to patch 1/3, then in turn, it
> > > used kvm_feature_msrs, I've to put its definition and
> > > initialization
> > > function kvm_get_supported_feature_msrs() to patch 1/3 as well.
> > > Then
> > > actually, this makes patch 1/3 and 2/3 merged into 1. Would you
> > > like me
> > > to do so?
> > 
> > I don't get it.  Why would you need to move the definition of
> > kvm_arch_get_supported_cpuid() to patch 1/3, if we change the
> > patch order and apply this patch before patch 1/3?
> > 
> OK, so you mean simply switch the 1/3 and 2/3 order, not move some
> hunks of 2/3 into 1/3. Am I right?

Exactly.

-- 
Eduardo