[Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov]

It was found that QMP users of QEMU (e.g. libvirt) may need
HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In
particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in
HV_CPUID_ENLIGHTMENT_INFO.EAX.

HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience
(we don't need to export it from hyperv_handle_properties() and as
future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and
direct virtual flush features.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/cpu.c | 30 +++++++++++++++++
 target/i386/cpu.h |  2 ++
 target/i386/kvm.c | 85 +++++++++++++++++++++++++----------------------
 3 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f81d35e1f9..8306670e09 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -980,6 +980,36 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         },
         .cpuid = { .eax = 0x40000003, .reg = R_EDX, },
     },
+    [FEAT_HV_RECOMM_EAX] = {
+        .type = CPUID_FEATURE_WORD,
+        .feat_names = {
+            NULL /* hv_recommend_pv_as_switch */,
+            NULL /* hv_recommend_pv_tlbflush_local */,
+            NULL /* hv_recommend_pv_tlbflush_remote */,
+            NULL /* hv_recommend_msr_apic_access */,
+            NULL /* hv_recommend_msr_reset */,
+            NULL /* hv_recommend_relaxed_timing */,
+            NULL /* hv_recommend_dma_remapping */,
+            NULL /* hv_recommend_int_remapping */,
+            NULL /* hv_recommend_x2apic_msrs */,
+            NULL /* hv_recommend_autoeoi_deprecation */,
+            NULL /* hv_recommend_pv_ipi */,
+            NULL /* hv_recommend_ex_hypercalls */,
+            NULL /* hv_hypervisor_is_nested */,
+            NULL /* hv_recommend_int_mbec */,
+            NULL /* hv_recommend_evmcs */,
+            NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+            NULL, NULL, NULL, NULL,
+        },
+        .cpuid = { .eax = 0x40000004, .reg = R_EAX, },
+    },
+    [FEAT_HV_NESTED_EAX] = {
+        .type = CPUID_FEATURE_WORD,
+        .cpuid = { .eax = 0x4000000A, .reg = R_EAX, },
+    },
     [FEAT_SVM] = {
         .type = CPUID_FEATURE_WORD,
         .feat_names = {
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 9c52d0cbeb..dd881510ac 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -497,6 +497,8 @@ typedef enum FeatureWord {
     FEAT_HYPERV_EAX,    /* CPUID[4000_0003].EAX */
     FEAT_HYPERV_EBX,    /* CPUID[4000_0003].EBX */
     FEAT_HYPERV_EDX,    /* CPUID[4000_0003].EDX */
+    FEAT_HV_RECOMM_EAX, /* CPUID[4000_0004].EAX */
+    FEAT_HV_NESTED_EAX, /* CPUID[4000_000A].EAX */
     FEAT_SVM,           /* CPUID[8000_000A].EDX */
     FEAT_XSAVE,         /* CPUID[EAX=0xd,ECX=1].EAX */
     FEAT_6_EAX,         /* CPUID[6].EAX */
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f524e7d929..b4d2b40a40 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -797,6 +797,48 @@ static int hyperv_handle_properties(CPUState *cs)
         }
         env->features[FEAT_HYPERV_EAX] |= HV_SYNTIMERS_AVAILABLE;
     }
+    if (cpu->hyperv_relaxed_timing) {
+        env->features[FEAT_HV_RECOMM_EAX] |= HV_RELAXED_TIMING_RECOMMENDED;
+    }
+    if (cpu->hyperv_vapic) {
+        env->features[FEAT_HV_RECOMM_EAX] |= HV_APIC_ACCESS_RECOMMENDED;
+    }
+    if (cpu->hyperv_tlbflush) {
+        if (kvm_check_extension(cs->kvm_state,
+                                KVM_CAP_HYPERV_TLBFLUSH) <= 0) {
+            fprintf(stderr, "Hyper-V TLB flush support "
+                    "(requested by 'hv-tlbflush' cpu flag) "
+                    " is not supported by kernel\n");
+            return -ENOSYS;
+        }
+        env->features[FEAT_HV_RECOMM_EAX] |= HV_REMOTE_TLB_FLUSH_RECOMMENDED;
+        env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
+    }
+    if (cpu->hyperv_ipi) {
+        if (kvm_check_extension(cs->kvm_state,
+                                KVM_CAP_HYPERV_SEND_IPI) <= 0) {
+            fprintf(stderr, "Hyper-V IPI send support "
+                    "(requested by 'hv-ipi' cpu flag) "
+                    " is not supported by kernel\n");
+            return -ENOSYS;
+        }
+        env->features[FEAT_HV_RECOMM_EAX] |= HV_CLUSTER_IPI_RECOMMENDED;
+        env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
+    }
+    if (cpu->hyperv_evmcs) {
+        uint16_t evmcs_version;
+
+        if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
+                                (uintptr_t)&evmcs_version)) {
+            fprintf(stderr, "Hyper-V Enlightened VMCS "
+                    "(requested by 'hv-evmcs' cpu flag) "
+                    "is not supported by kernel\n");
+            return -ENOSYS;
+        }
+        env->features[FEAT_HV_RECOMM_EAX] |= HV_ENLIGHTENED_VMCS_RECOMMENDED;
+        env->features[FEAT_HV_NESTED_EAX] = evmcs_version;
+    }
+
     return 0;
 }
 
@@ -869,7 +911,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
     uint32_t unused;
     struct kvm_cpuid_entry2 *c;
     uint32_t signature[3];
-    uint16_t evmcs_version;
     int kvm_base = KVM_CPUID_SIGNATURE;
     int r;
     Error *local_err = NULL;
@@ -944,44 +985,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
         c = &cpuid_data.entries[cpuid_i++];
         c->function = HV_CPUID_ENLIGHTMENT_INFO;
-        if (cpu->hyperv_relaxed_timing) {
-            c->eax |= HV_RELAXED_TIMING_RECOMMENDED;
-        }
-        if (cpu->hyperv_vapic) {
-            c->eax |= HV_APIC_ACCESS_RECOMMENDED;
-        }
-        if (cpu->hyperv_tlbflush) {
-            if (kvm_check_extension(cs->kvm_state,
-                                    KVM_CAP_HYPERV_TLBFLUSH) <= 0) {
-                fprintf(stderr, "Hyper-V TLB flush support "
-                        "(requested by 'hv-tlbflush' cpu flag) "
-                        " is not supported by kernel\n");
-                return -ENOSYS;
-            }
-            c->eax |= HV_REMOTE_TLB_FLUSH_RECOMMENDED;
-            c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
-        }
-        if (cpu->hyperv_ipi) {
-            if (kvm_check_extension(cs->kvm_state,
-                                    KVM_CAP_HYPERV_SEND_IPI) <= 0) {
-                fprintf(stderr, "Hyper-V IPI send support "
-                        "(requested by 'hv-ipi' cpu flag) "
-                        " is not supported by kernel\n");
-                return -ENOSYS;
-            }
-            c->eax |= HV_CLUSTER_IPI_RECOMMENDED;
-            c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
-        }
-        if (cpu->hyperv_evmcs) {
-            if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
-                                    (uintptr_t)&evmcs_version)) {
-                fprintf(stderr, "Hyper-V Enlightened VMCS "
-                        "(requested by 'hv-evmcs' cpu flag) "
-                        "is not supported by kernel\n");
-                return -ENOSYS;
-            }
-            c->eax |= HV_ENLIGHTENED_VMCS_RECOMMENDED;
-        }
+
+        c->eax = env->features[FEAT_HV_RECOMM_EAX];
         c->ebx = cpu->hyperv_spinlock_attempts;
 
         c = &cpuid_data.entries[cpuid_i++];
@@ -1005,7 +1010,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
             c = &cpuid_data.entries[cpuid_i++];
             c->function = HV_CPUID_NESTED_FEATURES;
-            c->eax = evmcs_version;
+            c->eax = env->features[FEAT_HV_NESTED_EAX];
         }
     }
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Roman Kagan]

On Mon, Nov 26, 2018 at 02:59:58PM +0100, Vitaly Kuznetsov wrote:
> It was found that QMP users of QEMU (e.g. libvirt) may need
> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In
> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in
> HV_CPUID_ENLIGHTMENT_INFO.EAX.
> 
> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience
> (we don't need to export it from hyperv_handle_properties() and as
> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and
> direct virtual flush features.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  target/i386/cpu.c | 30 +++++++++++++++++
>  target/i386/cpu.h |  2 ++
>  target/i386/kvm.c | 85 +++++++++++++++++++++++++----------------------
>  3 files changed, 77 insertions(+), 40 deletions(-)

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Paolo Bonzini]

On 26/11/18 14:59, Vitaly Kuznetsov wrote:
> It was found that QMP users of QEMU (e.g. libvirt) may need
> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In
> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in
> HV_CPUID_ENLIGHTMENT_INFO.EAX.
> 
> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience
> (we don't need to export it from hyperv_handle_properties() and as
> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and
> direct virtual flush features.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Can you add a comment to feature_word_info, explaining why the
feat_names are not set?

Thanks,

Paolo

> ---
>  target/i386/cpu.c | 30 +++++++++++++++++
>  target/i386/cpu.h |  2 ++
>  target/i386/kvm.c | 85 +++++++++++++++++++++++++----------------------
>  3 files changed, 77 insertions(+), 40 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index f81d35e1f9..8306670e09 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -980,6 +980,36 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>          },
>          .cpuid = { .eax = 0x40000003, .reg = R_EDX, },
>      },
> +    [FEAT_HV_RECOMM_EAX] = {
> +        .type = CPUID_FEATURE_WORD,
> +        .feat_names = {
> +            NULL /* hv_recommend_pv_as_switch */,
> +            NULL /* hv_recommend_pv_tlbflush_local */,
> +            NULL /* hv_recommend_pv_tlbflush_remote */,
> +            NULL /* hv_recommend_msr_apic_access */,
> +            NULL /* hv_recommend_msr_reset */,
> +            NULL /* hv_recommend_relaxed_timing */,
> +            NULL /* hv_recommend_dma_remapping */,
> +            NULL /* hv_recommend_int_remapping */,
> +            NULL /* hv_recommend_x2apic_msrs */,
> +            NULL /* hv_recommend_autoeoi_deprecation */,
> +            NULL /* hv_recommend_pv_ipi */,
> +            NULL /* hv_recommend_ex_hypercalls */,
> +            NULL /* hv_hypervisor_is_nested */,
> +            NULL /* hv_recommend_int_mbec */,
> +            NULL /* hv_recommend_evmcs */,
> +            NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +            NULL, NULL, NULL, NULL,
> +        },
> +        .cpuid = { .eax = 0x40000004, .reg = R_EAX, },
> +    },
> +    [FEAT_HV_NESTED_EAX] = {
> +        .type = CPUID_FEATURE_WORD,
> +        .cpuid = { .eax = 0x4000000A, .reg = R_EAX, },
> +    },
>      [FEAT_SVM] = {
>          .type = CPUID_FEATURE_WORD,
>          .feat_names = {
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 9c52d0cbeb..dd881510ac 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -497,6 +497,8 @@ typedef enum FeatureWord {
>      FEAT_HYPERV_EAX,    /* CPUID[4000_0003].EAX */
>      FEAT_HYPERV_EBX,    /* CPUID[4000_0003].EBX */
>      FEAT_HYPERV_EDX,    /* CPUID[4000_0003].EDX */
> +    FEAT_HV_RECOMM_EAX, /* CPUID[4000_0004].EAX */
> +    FEAT_HV_NESTED_EAX, /* CPUID[4000_000A].EAX */
>      FEAT_SVM,           /* CPUID[8000_000A].EDX */
>      FEAT_XSAVE,         /* CPUID[EAX=0xd,ECX=1].EAX */
>      FEAT_6_EAX,         /* CPUID[6].EAX */
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index f524e7d929..b4d2b40a40 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -797,6 +797,48 @@ static int hyperv_handle_properties(CPUState *cs)
>          }
>          env->features[FEAT_HYPERV_EAX] |= HV_SYNTIMERS_AVAILABLE;
>      }
> +    if (cpu->hyperv_relaxed_timing) {
> +        env->features[FEAT_HV_RECOMM_EAX] |= HV_RELAXED_TIMING_RECOMMENDED;
> +    }
> +    if (cpu->hyperv_vapic) {
> +        env->features[FEAT_HV_RECOMM_EAX] |= HV_APIC_ACCESS_RECOMMENDED;
> +    }
> +    if (cpu->hyperv_tlbflush) {
> +        if (kvm_check_extension(cs->kvm_state,
> +                                KVM_CAP_HYPERV_TLBFLUSH) <= 0) {
> +            fprintf(stderr, "Hyper-V TLB flush support "
> +                    "(requested by 'hv-tlbflush' cpu flag) "
> +                    " is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
> +        env->features[FEAT_HV_RECOMM_EAX] |= HV_REMOTE_TLB_FLUSH_RECOMMENDED;
> +        env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
> +    }
> +    if (cpu->hyperv_ipi) {
> +        if (kvm_check_extension(cs->kvm_state,
> +                                KVM_CAP_HYPERV_SEND_IPI) <= 0) {
> +            fprintf(stderr, "Hyper-V IPI send support "
> +                    "(requested by 'hv-ipi' cpu flag) "
> +                    " is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
> +        env->features[FEAT_HV_RECOMM_EAX] |= HV_CLUSTER_IPI_RECOMMENDED;
> +        env->features[FEAT_HV_RECOMM_EAX] |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
> +    }
> +    if (cpu->hyperv_evmcs) {
> +        uint16_t evmcs_version;
> +
> +        if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
> +                                (uintptr_t)&evmcs_version)) {
> +            fprintf(stderr, "Hyper-V Enlightened VMCS "
> +                    "(requested by 'hv-evmcs' cpu flag) "
> +                    "is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
> +        env->features[FEAT_HV_RECOMM_EAX] |= HV_ENLIGHTENED_VMCS_RECOMMENDED;
> +        env->features[FEAT_HV_NESTED_EAX] = evmcs_version;
> +    }
> +
>      return 0;
>  }
>  
> @@ -869,7 +911,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      uint32_t unused;
>      struct kvm_cpuid_entry2 *c;
>      uint32_t signature[3];
> -    uint16_t evmcs_version;
>      int kvm_base = KVM_CPUID_SIGNATURE;
>      int r;
>      Error *local_err = NULL;
> @@ -944,44 +985,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>          c = &cpuid_data.entries[cpuid_i++];
>          c->function = HV_CPUID_ENLIGHTMENT_INFO;
> -        if (cpu->hyperv_relaxed_timing) {
> -            c->eax |= HV_RELAXED_TIMING_RECOMMENDED;
> -        }
> -        if (cpu->hyperv_vapic) {
> -            c->eax |= HV_APIC_ACCESS_RECOMMENDED;
> -        }
> -        if (cpu->hyperv_tlbflush) {
> -            if (kvm_check_extension(cs->kvm_state,
> -                                    KVM_CAP_HYPERV_TLBFLUSH) <= 0) {
> -                fprintf(stderr, "Hyper-V TLB flush support "
> -                        "(requested by 'hv-tlbflush' cpu flag) "
> -                        " is not supported by kernel\n");
> -                return -ENOSYS;
> -            }
> -            c->eax |= HV_REMOTE_TLB_FLUSH_RECOMMENDED;
> -            c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
> -        }
> -        if (cpu->hyperv_ipi) {
> -            if (kvm_check_extension(cs->kvm_state,
> -                                    KVM_CAP_HYPERV_SEND_IPI) <= 0) {
> -                fprintf(stderr, "Hyper-V IPI send support "
> -                        "(requested by 'hv-ipi' cpu flag) "
> -                        " is not supported by kernel\n");
> -                return -ENOSYS;
> -            }
> -            c->eax |= HV_CLUSTER_IPI_RECOMMENDED;
> -            c->eax |= HV_EX_PROCESSOR_MASKS_RECOMMENDED;
> -        }
> -        if (cpu->hyperv_evmcs) {
> -            if (kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
> -                                    (uintptr_t)&evmcs_version)) {
> -                fprintf(stderr, "Hyper-V Enlightened VMCS "
> -                        "(requested by 'hv-evmcs' cpu flag) "
> -                        "is not supported by kernel\n");
> -                return -ENOSYS;
> -            }
> -            c->eax |= HV_ENLIGHTENED_VMCS_RECOMMENDED;
> -        }
> +
> +        c->eax = env->features[FEAT_HV_RECOMM_EAX];
>          c->ebx = cpu->hyperv_spinlock_attempts;
>  
>          c = &cpuid_data.entries[cpuid_i++];
> @@ -1005,7 +1010,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>              c = &cpuid_data.entries[cpuid_i++];
>              c->function = HV_CPUID_NESTED_FEATURES;
> -            c->eax = evmcs_version;
> +            c->eax = env->features[FEAT_HV_NESTED_EAX];
>          }
>      }
>  
> 

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 26/11/18 14:59, Vitaly Kuznetsov wrote:
>> It was found that QMP users of QEMU (e.g. libvirt) may need
>> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In
>> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in
>> HV_CPUID_ENLIGHTMENT_INFO.EAX.
>> 
>> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience
>> (we don't need to export it from hyperv_handle_properties() and as
>> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and
>> direct virtual flush features.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Can you add a comment to feature_word_info, explaining why the
> feat_names are not set?

I had to do some code archeology to make sure I understand, I think it
goes back to

http://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06579.html

So the comment (probably added before FEAT_HYPERV_EAX definition) would
be

".feat_names are commented out for Hyper-V enlightenments because we
don't want to have two different ways for enabling them on QEMU command
line. Some features (e.g. "hyperv_time", "hyperv_vapic", ...) require
enabling several feature bits simultaneously, exposing these bits
individually may just confuse guests."

Would do?

-- 
Vitaly

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Eduardo Habkost]

On Thu, Nov 29, 2018 at 12:51:55PM +0100, Vitaly Kuznetsov wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 26/11/18 14:59, Vitaly Kuznetsov wrote:
> >> It was found that QMP users of QEMU (e.g. libvirt) may need
> >> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In
> >> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in
> >> HV_CPUID_ENLIGHTMENT_INFO.EAX.
> >> 
> >> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience
> >> (we don't need to export it from hyperv_handle_properties() and as
> >> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and
> >> direct virtual flush features.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >
> > Can you add a comment to feature_word_info, explaining why the
> > feat_names are not set?
> 
> I had to do some code archeology to make sure I understand, I think it
> goes back to
> 
> http://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06579.html
> 
> So the comment (probably added before FEAT_HYPERV_EAX definition) would
> be
> 
> ".feat_names are commented out for Hyper-V enlightenments because we
> don't want to have two different ways for enabling them on QEMU command
> line. Some features (e.g. "hyperv_time", "hyperv_vapic", ...) require
> enabling several feature bits simultaneously, exposing these bits
> individually may just confuse guests."
> 
> Would do?

That's an accurate description.

But note that we might still be able to move the existing
"hyperv_*" features to feature_word_info[].feat_names.  We just
need to keep the same semantics (e.g. enable
HV_HYPERCALL_AVAILABLE automatically when some features are set).

Maybe we can make some of the feature properties read-only.  This
way we can give them meaningful names for debugging and error
messages, even if we don't want to make them configurable
directly.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Nov 29, 2018 at 12:51:55PM +0100, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 26/11/18 14:59, Vitaly Kuznetsov wrote:
>> >> It was found that QMP users of QEMU (e.g. libvirt) may need
>> >> HV_CPUID_ENLIGHTMENT_INFO.EAX/HV_CPUID_NESTED_FEATURES.EAX information. In
>> >> particular, 'hv_tlbflush' and 'hv_evmcs' enlightenments are only exposed in
>> >> HV_CPUID_ENLIGHTMENT_INFO.EAX.
>> >> 
>> >> HV_CPUID_NESTED_FEATURES.EAX is exposed for two reasons: convenience
>> >> (we don't need to export it from hyperv_handle_properties() and as
>> >> future-proof for Enlightened MSR-Bitmap, PV EPT invalidation and
>> >> direct virtual flush features.
>> >> 
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >
>> > Can you add a comment to feature_word_info, explaining why the
>> > feat_names are not set?
>> 
>> I had to do some code archeology to make sure I understand, I think it
>> goes back to
>> 
>> http://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06579.html
>> 
>> So the comment (probably added before FEAT_HYPERV_EAX definition) would
>> be
>> 
>> ".feat_names are commented out for Hyper-V enlightenments because we
>> don't want to have two different ways for enabling them on QEMU command
>> line. Some features (e.g. "hyperv_time", "hyperv_vapic", ...) require
>> enabling several feature bits simultaneously, exposing these bits
>> individually may just confuse guests."
>> 
>> Would do?
>
> That's an accurate description.
>

Thanks, I'll send v2 out with it.

> But note that we might still be able to move the existing
> "hyperv_*" features to feature_word_info[].feat_names.  We just
> need to keep the same semantics (e.g. enable
> HV_HYPERCALL_AVAILABLE automatically when some features are set).
>
> Maybe we can make some of the feature properties read-only.  This
> way we can give them meaningful names for debugging and error
> messages, even if we don't want to make them configurable
> directly.

I'd suggest (if there are no objections of course) we do this separately
from this patch. Some time ago when merging direct mode stimers for KVM
Paolo suggested we stop adding capabilities to KVM for each individulat
feature and replace them with something like KVM_GET_SUPPORTED_HV_CPUID
ioctl returning all Hyper-V related feature words. When this is done we
can reconsider how Qemu discoveres Hyper-V related KVM features and as
part of this work we can take a closer look at feature words and
feat_names.

-- 
Vitaly

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Eduardo Habkost]

On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
[...]
> > But note that we might still be able to move the existing
> > "hyperv_*" features to feature_word_info[].feat_names.  We just
> > need to keep the same semantics (e.g. enable
> > HV_HYPERCALL_AVAILABLE automatically when some features are set).
> >
> > Maybe we can make some of the feature properties read-only.  This
> > way we can give them meaningful names for debugging and error
> > messages, even if we don't want to make them configurable
> > directly.
> 
> I'd suggest (if there are no objections of course) we do this separately
> from this patch. [...]

Agreed.

>            [...] Some time ago when merging direct mode stimers for KVM
> Paolo suggested we stop adding capabilities to KVM for each individulat
> feature and replace them with something like KVM_GET_SUPPORTED_HV_CPUID
> ioctl returning all Hyper-V related feature words. When this is done we
> can reconsider how Qemu discoveres Hyper-V related KVM features and as
> part of this work we can take a closer look at feature words and
> feat_names.

Why a separate ioctl instead of extending GET_SUPPORTED_CPUID?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

>>            [...] Some time ago when merging direct mode stimers for KVM
>> Paolo suggested we stop adding capabilities to KVM for each individulat
>> feature and replace them with something like KVM_GET_SUPPORTED_HV_CPUID
>> ioctl returning all Hyper-V related feature words. When this is done we
>> can reconsider how Qemu discoveres Hyper-V related KVM features and as
>> part of this work we can take a closer look at feature words and
>> feat_names.
>
> Why a separate ioctl instead of extending GET_SUPPORTED_CPUID?

Unfortunatelly both KVM and Hyper-V use feature leaves 0x40000000,
0x40000001 (so it's up to the userspace - qemu in our case - what to
expose to the guest) and GET_SUPPORTED_CPUID already returns KVM's. Not
sure this can be changed (to e.g. returning these leaves twice with
different flags) without breaking userspace. New ioctl is safer.

-- 
Vitaly

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
> [...]
>> > But note that we might still be able to move the existing
>> > "hyperv_*" features to feature_word_info[].feat_names.  We just
>> > need to keep the same semantics (e.g. enable
>> > HV_HYPERCALL_AVAILABLE automatically when some features are set).
>> >
>> > Maybe we can make some of the feature properties read-only.  This
>> > way we can give them meaningful names for debugging and error
>> > messages, even if we don't want to make them configurable
>> > directly.
>> 
>> I'd suggest (if there are no objections of course) we do this separately
>> from this patch. [...]
>
> Agreed.
>

Paolo, Eduardo,

in case there are no concerns here, could you please pick this patch up?
Thanks!

-- 
Vitaly

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Eduardo Habkost]

On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> > [...]
> >> > But note that we might still be able to move the existing
> >> > "hyperv_*" features to feature_word_info[].feat_names.  We just
> >> > need to keep the same semantics (e.g. enable
> >> > HV_HYPERCALL_AVAILABLE automatically when some features are set).
> >> >
> >> > Maybe we can make some of the feature properties read-only.  This
> >> > way we can give them meaningful names for debugging and error
> >> > messages, even if we don't want to make them configurable
> >> > directly.
> >> 
> >> I'd suggest (if there are no objections of course) we do this separately
> >> from this patch. [...]
> >
> > Agreed.
> >
> 
> Paolo, Eduardo,
> 
> in case there are no concerns here, could you please pick this patch up?
> Thanks!

Queued, thanks!

Can you please send the comment you wrote about feat_names as a
follow-up patch?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> > [...]
>> >> > But note that we might still be able to move the existing
>> >> > "hyperv_*" features to feature_word_info[].feat_names.  We just
>> >> > need to keep the same semantics (e.g. enable
>> >> > HV_HYPERCALL_AVAILABLE automatically when some features are set).
>> >> >
>> >> > Maybe we can make some of the feature properties read-only.  This
>> >> > way we can give them meaningful names for debugging and error
>> >> > messages, even if we don't want to make them configurable
>> >> > directly.
>> >> 
>> >> I'd suggest (if there are no objections of course) we do this separately
>> >> from this patch. [...]
>> >
>> > Agreed.
>> >
>> 
>> Paolo, Eduardo,
>> 
>> in case there are no concerns here, could you please pick this patch up?
>> Thanks!
>
> Queued, thanks!
>
> Can you please send the comment you wrote about feat_names as a
> follow-up patch?

Oops, sorry, I just realized I promissed to send out v2 with it and
aparently never did. Will send out a follow-up patch shortly. Thanks!

-- 
Vitaly

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Eduardo Habkost <ehabkost@redhat.com> writes:
>
>> On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote:
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>> 
>>> > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote:
>>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>>> > [...]
>>> >> > But note that we might still be able to move the existing
>>> >> > "hyperv_*" features to feature_word_info[].feat_names.  We just
>>> >> > need to keep the same semantics (e.g. enable
>>> >> > HV_HYPERCALL_AVAILABLE automatically when some features are set).
>>> >> >
>>> >> > Maybe we can make some of the feature properties read-only.  This
>>> >> > way we can give them meaningful names for debugging and error
>>> >> > messages, even if we don't want to make them configurable
>>> >> > directly.
>>> >> 
>>> >> I'd suggest (if there are no objections of course) we do this separately
>>> >> from this patch. [...]
>>> >
>>> > Agreed.
>>> >
>>> 
>>> Paolo, Eduardo,
>>> 
>>> in case there are no concerns here, could you please pick this patch up?
>>> Thanks!
>>
>> Queued, thanks!
>>
>> Can you please send the comment you wrote about feat_names as a
>> follow-up patch?
>
> Oops, sorry, I just realized I promissed to send out v2 with it and
> aparently never did. Will send out a follow-up patch shortly. Thanks!

Hey Eduardo,

any news about the fate of this patch?

(Correcting myself: there was v2 with the comment included:
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00355.html

but as I sent the follow-up patch you requested separately too:
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05463.html
)

-- 
Vitaly

Re: [Qemu-devel] [PATCH] i386/kvm: expose HV_CPUID_ENLIGHTMENT_INFO.EAX and HV_CPUID_NESTED_FEATURES.EAX as feature words

[Eduardo Habkost]

On Mon, Jan 14, 2019 at 11:54:30AM +0100, Vitaly Kuznetsov wrote:
> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> 
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >
> >> On Wed, Dec 19, 2018 at 06:25:06PM +0100, Vitaly Kuznetsov wrote:
> >>> Eduardo Habkost <ehabkost@redhat.com> writes:
> >>> 
> >>> > On Mon, Dec 03, 2018 at 03:17:06PM +0100, Vitaly Kuznetsov wrote:
> >>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >>> > [...]
> >>> >> > But note that we might still be able to move the existing
> >>> >> > "hyperv_*" features to feature_word_info[].feat_names.  We just
> >>> >> > need to keep the same semantics (e.g. enable
> >>> >> > HV_HYPERCALL_AVAILABLE automatically when some features are set).
> >>> >> >
> >>> >> > Maybe we can make some of the feature properties read-only.  This
> >>> >> > way we can give them meaningful names for debugging and error
> >>> >> > messages, even if we don't want to make them configurable
> >>> >> > directly.
> >>> >> 
> >>> >> I'd suggest (if there are no objections of course) we do this separately
> >>> >> from this patch. [...]
> >>> >
> >>> > Agreed.
> >>> >
> >>> 
> >>> Paolo, Eduardo,
> >>> 
> >>> in case there are no concerns here, could you please pick this patch up?
> >>> Thanks!
> >>
> >> Queued, thanks!
> >>
> >> Can you please send the comment you wrote about feat_names as a
> >> follow-up patch?
> >
> > Oops, sorry, I just realized I promissed to send out v2 with it and
> > aparently never did. Will send out a follow-up patch shortly. Thanks!
> 
> Hey Eduardo,
> 
> any news about the fate of this patch?
> 
> (Correcting myself: there was v2 with the comment included:
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00355.html
> 
> but as I sent the follow-up patch you requested separately too:
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05463.html
> )

Patch was queued but I took too long to send a pull request,
sorry.  Pull request is being sent right now.

-- 
Eduardo