[PATCH v7 0/9] i386: KVM: expand Hyper-V features early

[PATCH v7 0/9] i386: KVM: expand Hyper-V features early

[Vitaly Kuznetsov]

Changes since v6 [Eduardo]:
- First 14 patches of v6 made it upstream!
- New patches addressing feedback on the already merged patches added:
  i386: make hyperv_expand_features() return bool
  i386: hardcode supported eVMCS version to '1'
  i386: clarify 'hv-passthrough' behavior
  i386: avoid hardcoding '12' as 'hyperv_vendor_id' length
- Some comments expanded.
- HV_HYPERCALL_AVAILABLE setting moved to hyperv_fill_cpuids()

Note: Windows guest on QEMU are currently completely broken, see Claudio's
https://lore.kernel.org/kvm/20210529091313.16708-1-cfontana@suse.de/

The last two functional patches are inspired by 'Fine-grained access check
to Hyper-V hypercalls and MSRs' work for KVM:
https://lore.kernel.org/kvm/20210521095204.2161214-1-vkuznets@redhat.com/

Original description:

Upper layer tools like libvirt want to figure out which Hyper-V features are
supported by the underlying stack (QEMU/KVM) but currently they are unable to
do so. We have a nice 'hv_passthrough' CPU flag supported by QEMU but it has
no effect on e.g. QMP's 

query-cpu-model-expansion type=full model={"name":"host","props":{"hv-passthrough":true}}

command as we parse Hyper-V features after creating KVM vCPUs and not at
feature expansion time. To support the use-case we first need to make 
KVM_GET_SUPPORTED_HV_CPUID ioctl a system-wide ioctl as the existing
vCPU version can't be used that early. This is what KVM part does. With
that done, we can make early Hyper-V feature expansion (this series).

Vitaly Kuznetsov (9):
  i386: avoid hardcoding '12' as 'hyperv_vendor_id' length
  i386: clarify 'hv-passthrough' behavior
  i386: hardcode supported eVMCS version to '1'
  i386: make hyperv_expand_features() return bool
  i386: expand Hyper-V features during CPU feature expansion time
  i386: kill off hv_cpuid_check_and_set()
  i386: HV_HYPERCALL_AVAILABLE privilege bit is always needed
  i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS priviliges
  qtest/hyperv: Introduce a simple hyper-v test

 MAINTAINERS                    |   1 +
 docs/hyperv.txt                |   9 +-
 target/i386/cpu.c              |  13 +-
 target/i386/kvm/hyperv-proto.h |   6 +
 target/i386/kvm/kvm-stub.c     |   5 +
 target/i386/kvm/kvm.c          | 166 ++++++++++++------------
 target/i386/kvm/kvm_i386.h     |   1 +
 tests/qtest/hyperv-test.c      | 225 +++++++++++++++++++++++++++++++++
 tests/qtest/meson.build        |   3 +-
 9 files changed, 337 insertions(+), 92 deletions(-)
 create mode 100644 tests/qtest/hyperv-test.c

-- 
2.31.1

[PATCH v7 1/9] i386: avoid hardcoding '12' as 'hyperv_vendor_id' length

[Vitaly Kuznetsov]

While this is very unlikely to change, let's avoid hardcoding '12' as
'hyperv_vendor_id' length.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/cpu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fa282a07013f..b2d8e5713911 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6057,11 +6057,12 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
                                 &error_abort);
     }
     len = strlen(cpu->hyperv_vendor);
-    if (len > 12) {
-        warn_report("hv-vendor-id truncated to 12 characters");
-        len = 12;
+    if (len > sizeof(cpu->hyperv_vendor_id)) {
+        warn_report("hv-vendor-id truncated to %ld characters",
+                    sizeof(cpu->hyperv_vendor_id));
+        len = sizeof(cpu->hyperv_vendor_id);
     }
-    memset(cpu->hyperv_vendor_id, 0, 12);
+    memset(cpu->hyperv_vendor_id, 0, sizeof(cpu->hyperv_vendor_id));
     memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
 
     /* 'Hv#1' interface identification*/
-- 
2.31.1

Re: [PATCH v7 1/9] i386: avoid hardcoding '12' as 'hyperv_vendor_id' length

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:27PM +0200, Vitaly Kuznetsov wrote:
> While this is very unlikely to change, let's avoid hardcoding '12' as
> 'hyperv_vendor_id' length.
> 
> No functional change intended.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH v7 1/9] i386: avoid hardcoding '12' as 'hyperv_vendor_id' length

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:27PM +0200, Vitaly Kuznetsov wrote:
> While this is very unlikely to change, let's avoid hardcoding '12' as
> 'hyperv_vendor_id' length.
> 
> No functional change intended.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

This one breaks some build due to the printf format string.  See:
https://gitlab.com/ehabkost/qemu/-/jobs/1404084433

../target/i386/cpu.c: In function 'x86_cpu_hyperv_realize':
../target/i386/cpu.c:6049:50: error: format '%ld' expects argument of type 'long int', but argument 2 has type 'unsigned int' [-Werror=format=]
         warn_report("hv-vendor-id truncated to %ld characters",
                                                ~~^
                                                %d
                     sizeof(cpu->hyperv_vendor_id));


I'm removing it from the queue by now.

> ---
>  target/i386/cpu.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index fa282a07013f..b2d8e5713911 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6057,11 +6057,12 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
>                                  &error_abort);
>      }
>      len = strlen(cpu->hyperv_vendor);
> -    if (len > 12) {
> -        warn_report("hv-vendor-id truncated to 12 characters");
> -        len = 12;
> +    if (len > sizeof(cpu->hyperv_vendor_id)) {
> +        warn_report("hv-vendor-id truncated to %ld characters",
> +                    sizeof(cpu->hyperv_vendor_id));
> +        len = sizeof(cpu->hyperv_vendor_id);
>      }
> -    memset(cpu->hyperv_vendor_id, 0, 12);
> +    memset(cpu->hyperv_vendor_id, 0, sizeof(cpu->hyperv_vendor_id));
>      memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
>  
>      /* 'Hv#1' interface identification*/
> -- 
> 2.31.1
> 

-- 
Eduardo

[PATCH v7 2/9] i386: clarify 'hv-passthrough' behavior

[Vitaly Kuznetsov]

Clarify the fact that 'hv-passthrough' only enables features which are
already known to QEMU and that it overrides all other 'hv-*' settings.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 docs/hyperv.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/docs/hyperv.txt b/docs/hyperv.txt
index e53c581f4586..a51953daa833 100644
--- a/docs/hyperv.txt
+++ b/docs/hyperv.txt
@@ -209,8 +209,11 @@ In some cases (e.g. during development) it may make sense to use QEMU in
 'pass-through' mode and give Windows guests all enlightenments currently
 supported by KVM. This pass-through mode is enabled by "hv-passthrough" CPU
 flag.
-Note: enabling this flag effectively prevents migration as supported features
-may differ between target and destination.
+Note: "hv-passthrough" flag only enables enlightenments which are known to QEMU
+(have corresponding "hv-*" flag) and copies "hv-spinlocks="/"hv-vendor-id="
+values from KVM to QEMU. "hv-passthrough" overrides all other "hv-*" settings on
+the command line. Also, enabling this flag effectively prevents migration as the
+list of enabled enlightenments may differ between target and destination hosts.
 
 
 4. Useful links
-- 
2.31.1

Re: [PATCH v7 2/9] i386: clarify 'hv-passthrough' behavior

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:28PM +0200, Vitaly Kuznetsov wrote:
> Clarify the fact that 'hv-passthrough' only enables features which are
> already known to QEMU and that it overrides all other 'hv-*' settings.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[PATCH v7 3/9] i386: hardcode supported eVMCS version to '1'

[Vitaly Kuznetsov]

Currently, the only eVMCS version, supported by KVM (and described in TLFS)
is '1'. When Enlightened VMCS feature is enabled, QEMU takes the supported
eVMCS version range (from KVM_CAP_HYPERV_ENLIGHTENED_VMCS enablement) and
puts it to guest visible CPUIDs. When (and if) eVMCS ver.2 appears a
problem on migration is expected: it doesn't seem to be possible to migrate
from a host supporting eVMCS ver.2 to a host, which only support eVMCS
ver.1.

Hardcode eVMCS ver.1 as the result of 'hv-evmcs' enablement for now. Newer
eVMCS versions will have to have their own enablement options (e.g.
'hv-evmcs=2').

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 docs/hyperv.txt       |  2 +-
 target/i386/kvm/kvm.c | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/docs/hyperv.txt b/docs/hyperv.txt
index a51953daa833..000638a2fd38 100644
--- a/docs/hyperv.txt
+++ b/docs/hyperv.txt
@@ -170,7 +170,7 @@ Recommended: hv-frequencies
 3.16. hv-evmcs
 ===============
 The enlightenment is nested specific, it targets Hyper-V on KVM guests. When
-enabled, it provides Enlightened VMCS feature to the guest. The feature
+enabled, it provides Enlightened VMCS version 1 feature to the guest. The feature
 implements paravirtualized protocol between L0 (KVM) and L1 (Hyper-V)
 hypervisors making L2 exits to the hypervisor faster. The feature is Intel-only.
 Note: some virtualization features (e.g. Posted Interrupts) are disabled when
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index c676ee8b38a7..d57eede5dc81 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1490,13 +1490,19 @@ static int hyperv_init_vcpu(X86CPU *cpu)
         ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
                                   (uintptr_t)&evmcs_version);
 
-        if (ret < 0) {
-            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
-                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
+        /*
+         * KVM is required to support EVMCS ver.1. as that's what 'hv-evmcs'
+         * option sets. Note: we hardcode the maximum supported eVMCS version
+         * to '1' as well so 'hv-evmcs' feature is migratable even when (and if)
+         * ver.2 is implemented. A new option (e.g. 'hv-evmcs=2') will then have
+         * to be added.
+         */
+        if (ret < 0 || (uint8_t)evmcs_version > 1) {
+            error_report("Hyper-V %s verson 1 is not supported by kernel",
+                         kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
             return ret;
         }
-
-        cpu->hyperv_nested[0] = evmcs_version;
+        cpu->hyperv_nested[0] = (1 << 8) | 1;
     }
 
     return 0;
-- 
2.31.1

Re: [PATCH v7 3/9] i386: hardcode supported eVMCS version to '1'

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:29PM +0200, Vitaly Kuznetsov wrote:
> Currently, the only eVMCS version, supported by KVM (and described in TLFS)
> is '1'. When Enlightened VMCS feature is enabled, QEMU takes the supported
> eVMCS version range (from KVM_CAP_HYPERV_ENLIGHTENED_VMCS enablement) and
> puts it to guest visible CPUIDs. When (and if) eVMCS ver.2 appears a
> problem on migration is expected: it doesn't seem to be possible to migrate
> from a host supporting eVMCS ver.2 to a host, which only support eVMCS
> ver.1.

Isn't it possible and safe to expose eVMCS ver.1 to the guest on
a host that supports ver.2?

> 
> Hardcode eVMCS ver.1 as the result of 'hv-evmcs' enablement for now. Newer
> eVMCS versions will have to have their own enablement options (e.g.
> 'hv-evmcs=2').
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  docs/hyperv.txt       |  2 +-
>  target/i386/kvm/kvm.c | 16 +++++++++++-----
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> index a51953daa833..000638a2fd38 100644
> --- a/docs/hyperv.txt
> +++ b/docs/hyperv.txt
> @@ -170,7 +170,7 @@ Recommended: hv-frequencies
>  3.16. hv-evmcs
>  ===============
>  The enlightenment is nested specific, it targets Hyper-V on KVM guests. When
> -enabled, it provides Enlightened VMCS feature to the guest. The feature
> +enabled, it provides Enlightened VMCS version 1 feature to the guest. The feature
>  implements paravirtualized protocol between L0 (KVM) and L1 (Hyper-V)
>  hypervisors making L2 exits to the hypervisor faster. The feature is Intel-only.
>  Note: some virtualization features (e.g. Posted Interrupts) are disabled when
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index c676ee8b38a7..d57eede5dc81 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1490,13 +1490,19 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>          ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
>                                    (uintptr_t)&evmcs_version);
>  
> -        if (ret < 0) {
> -            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
> -                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
> +        /*
> +         * KVM is required to support EVMCS ver.1. as that's what 'hv-evmcs'
> +         * option sets. Note: we hardcode the maximum supported eVMCS version
> +         * to '1' as well so 'hv-evmcs' feature is migratable even when (and if)
> +         * ver.2 is implemented. A new option (e.g. 'hv-evmcs=2') will then have
> +         * to be added.
> +         */
> +        if (ret < 0 || (uint8_t)evmcs_version > 1) {

Wait, do you really want to get a fatal error every time, after a
kernel upgrade?

I was expecting this:

  vcpu_evmcs_version = 1; /* hardcoded, but can become configurable later */
  ...
  kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0, (uintptr_t)&supported_evmcs_version);
  if (ret < 0 || supported_evmcs_version < vcpu_evmcs_version) {
    error_setg(...);
    return;
  }
  cpu->hyperv_nested[0] = vcpu_evmcs_version;


> +            error_report("Hyper-V %s verson 1 is not supported by kernel",
> +                         kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
>              return ret;
>          }
> -
> -        cpu->hyperv_nested[0] = evmcs_version;
> +        cpu->hyperv_nested[0] = (1 << 8) | 1;
>      }
>  
>      return 0;
> -- 
> 2.31.1
> 

-- 
Eduardo

Re: [PATCH v7 3/9] i386: hardcode supported eVMCS version to '1'

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Jun 03, 2021 at 01:48:29PM +0200, Vitaly Kuznetsov wrote:
>> Currently, the only eVMCS version, supported by KVM (and described in TLFS)
>> is '1'. When Enlightened VMCS feature is enabled, QEMU takes the supported
>> eVMCS version range (from KVM_CAP_HYPERV_ENLIGHTENED_VMCS enablement) and
>> puts it to guest visible CPUIDs. When (and if) eVMCS ver.2 appears a
>> problem on migration is expected: it doesn't seem to be possible to migrate
>> from a host supporting eVMCS ver.2 to a host, which only support eVMCS
>> ver.1.
>
> Isn't it possible and safe to expose eVMCS ver.1 to the guest on
> a host that supports ver.2?

We expose the supported range, guest is free to use any eVMCS version in
the range (see below):

>
>> 
>> Hardcode eVMCS ver.1 as the result of 'hv-evmcs' enablement for now. Newer
>> eVMCS versions will have to have their own enablement options (e.g.
>> 'hv-evmcs=2').
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  docs/hyperv.txt       |  2 +-
>>  target/i386/kvm/kvm.c | 16 +++++++++++-----
>>  2 files changed, 12 insertions(+), 6 deletions(-)
>> 
>> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
>> index a51953daa833..000638a2fd38 100644
>> --- a/docs/hyperv.txt
>> +++ b/docs/hyperv.txt
>> @@ -170,7 +170,7 @@ Recommended: hv-frequencies
>>  3.16. hv-evmcs
>>  ===============
>>  The enlightenment is nested specific, it targets Hyper-V on KVM guests. When
>> -enabled, it provides Enlightened VMCS feature to the guest. The feature
>> +enabled, it provides Enlightened VMCS version 1 feature to the guest. The feature
>>  implements paravirtualized protocol between L0 (KVM) and L1 (Hyper-V)
>>  hypervisors making L2 exits to the hypervisor faster. The feature is Intel-only.
>>  Note: some virtualization features (e.g. Posted Interrupts) are disabled when
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index c676ee8b38a7..d57eede5dc81 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1490,13 +1490,19 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>>          ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
>>                                    (uintptr_t)&evmcs_version);
>>  
>> -        if (ret < 0) {
>> -            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
>> -                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
>> +        /*
>> +         * KVM is required to support EVMCS ver.1. as that's what 'hv-evmcs'
>> +         * option sets. Note: we hardcode the maximum supported eVMCS version
>> +         * to '1' as well so 'hv-evmcs' feature is migratable even when (and if)
>> +         * ver.2 is implemented. A new option (e.g. 'hv-evmcs=2') will then have
>> +         * to be added.
>> +         */
>> +        if (ret < 0 || (uint8_t)evmcs_version > 1) {
>
> Wait, do you really want to get a fatal error every time, after a
> kernel upgrade?
>

Here, evmcs_version (returned by kvm_vcpu_enable_cap()) represents a
*range* of supported eVMCS versions:

(evmcs_highest_supported_version << 8) | evmcs_lowest_supported_version

Currently, this is 0x101 [1..1] range.

The '(uint8_t)evmcs_version > 1' check here means 'eVMCS v1' is no
longer supported by KVM. This is not going to happen any time soon, but
I can imagine in 10 years or so we'll be dropping v1 so the range (in
theory) can be [10..2] -- which would mean eVMCS ver. 1 is NOT
supported. And we can't proceed then.

> I was expecting this:
>
>   vcpu_evmcs_version = 1; /* hardcoded, but can become configurable later */
>   ...
>   kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0, (uintptr_t)&supported_evmcs_version);
>   if (ret < 0 || supported_evmcs_version < vcpu_evmcs_version) {
>     error_setg(...);
>     return;
>   }
>   cpu->hyperv_nested[0] = vcpu_evmcs_version;
>
>
>> +            error_report("Hyper-V %s verson 1 is not supported by kernel",
>> +                         kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
>>              return ret;
>>          }
>> -
>> -        cpu->hyperv_nested[0] = evmcs_version;
>> +        cpu->hyperv_nested[0] = (1 << 8) | 1;
>>      }
>>  
>>      return 0;
>> -- 
>> 2.31.1
>> 

-- 
Vitaly

Re: [PATCH v7 3/9] i386: hardcode supported eVMCS version to '1'

[Eduardo Habkost]

On Fri, Jun 04, 2021 at 09:28:15AM +0200, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Jun 03, 2021 at 01:48:29PM +0200, Vitaly Kuznetsov wrote:
> >> Currently, the only eVMCS version, supported by KVM (and described in TLFS)
> >> is '1'. When Enlightened VMCS feature is enabled, QEMU takes the supported
> >> eVMCS version range (from KVM_CAP_HYPERV_ENLIGHTENED_VMCS enablement) and
> >> puts it to guest visible CPUIDs. When (and if) eVMCS ver.2 appears a
> >> problem on migration is expected: it doesn't seem to be possible to migrate
> >> from a host supporting eVMCS ver.2 to a host, which only support eVMCS
> >> ver.1.
> >
> > Isn't it possible and safe to expose eVMCS ver.1 to the guest on
> > a host that supports ver.2?
> 
> We expose the supported range, guest is free to use any eVMCS version in
> the range (see below):

Oh, I didn't notice the returned value was a range.

> 
> >
> >> 
> >> Hardcode eVMCS ver.1 as the result of 'hv-evmcs' enablement for now. Newer
> >> eVMCS versions will have to have their own enablement options (e.g.
> >> 'hv-evmcs=2').
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  docs/hyperv.txt       |  2 +-
> >>  target/i386/kvm/kvm.c | 16 +++++++++++-----
> >>  2 files changed, 12 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> >> index a51953daa833..000638a2fd38 100644
> >> --- a/docs/hyperv.txt
> >> +++ b/docs/hyperv.txt
> >> @@ -170,7 +170,7 @@ Recommended: hv-frequencies
> >>  3.16. hv-evmcs
> >>  ===============
> >>  The enlightenment is nested specific, it targets Hyper-V on KVM guests. When
> >> -enabled, it provides Enlightened VMCS feature to the guest. The feature
> >> +enabled, it provides Enlightened VMCS version 1 feature to the guest. The feature
> >>  implements paravirtualized protocol between L0 (KVM) and L1 (Hyper-V)
> >>  hypervisors making L2 exits to the hypervisor faster. The feature is Intel-only.
> >>  Note: some virtualization features (e.g. Posted Interrupts) are disabled when
> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> >> index c676ee8b38a7..d57eede5dc81 100644
> >> --- a/target/i386/kvm/kvm.c
> >> +++ b/target/i386/kvm/kvm.c
> >> @@ -1490,13 +1490,19 @@ static int hyperv_init_vcpu(X86CPU *cpu)
> >>          ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
> >>                                    (uintptr_t)&evmcs_version);
> >>  
> >> -        if (ret < 0) {
> >> -            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
> >> -                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
> >> +        /*
> >> +         * KVM is required to support EVMCS ver.1. as that's what 'hv-evmcs'
> >> +         * option sets. Note: we hardcode the maximum supported eVMCS version
> >> +         * to '1' as well so 'hv-evmcs' feature is migratable even when (and if)
> >> +         * ver.2 is implemented. A new option (e.g. 'hv-evmcs=2') will then have
> >> +         * to be added.
> >> +         */
> >> +        if (ret < 0 || (uint8_t)evmcs_version > 1) {
> >
> > Wait, do you really want to get a fatal error every time, after a
> > kernel upgrade?
> >
> 
> Here, evmcs_version (returned by kvm_vcpu_enable_cap()) represents a
> *range* of supported eVMCS versions:
> 
> (evmcs_highest_supported_version << 8) | evmcs_lowest_supported_version
> 
> Currently, this is 0x101 [1..1] range.
> 
> The '(uint8_t)evmcs_version > 1' check here means 'eVMCS v1' is no
> longer supported by KVM. This is not going to happen any time soon, but
> I can imagine in 10 years or so we'll be dropping v1 so the range (in
> theory) can be [10..2] -- which would mean eVMCS ver. 1 is NOT
> supported. And we can't proceed then.

Where is this documented?  The only reference to
KVM_CAP_HYPERV_ENLIGHTENED_VMCS I've found in linux/Documentation is this
single sentence:

| - HYPERV_CPUID_NESTED_FEATURES leaf and HV_X64_ENLIGHTENED_VMCS_RECOMMENDED
|  feature bit are only exposed when Enlightened VMCS was previously enabled
|  on the corresponding vCPU (KVM_CAP_HYPERV_ENLIGHTENED_VMCS).


> >>          ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
> >>                                    (uintptr_t)&evmcs_version);
> >>  
> >> -        if (ret < 0) {
> >> -            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
> >> -                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
> >> +        /*
> >> +         * KVM is required to support EVMCS ver.1. as that's what 'hv-evmcs'
> >> +         * option sets. Note: we hardcode the maximum supported eVMCS version
> >> +         * to '1' as well so 'hv-evmcs' feature is migratable even when (and if)
> >> +         * ver.2 is implemented. A new option (e.g. 'hv-evmcs=2') will then have
> >> +         * to be added.
> >> +         */
> >> +        if (ret < 0 || (uint8_t)evmcs_version > 1) {

Can we start with something that won't need to be rewritten after we change the
guest evmcs version range?  e.g.:

    static bool evmcs_version_supported(uint16_t version, uint32_t supported)
    {
        uint8_t min_ver = version;
        uint8_t max_ver = version >> 8;
        uint8_t min_supported = supported;
        uint8_t max_supported = supported >> 8;
        return (min_ver >= min_supported) && (max_ver <= max_supported);
    }
    ...
    #define DEFAULT_EVMCS_VERSION ((1 << 8) | 1)
    ...
    uint16_t evmcs_version = DEFAULT_EVMCS_VERSION;
    int ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
                              (uintptr_t)&supported_evmcs_version);
    if (ret < 0) {
        fprintf(...)
        return ret;
    }
    if (!evmcs_version_supported(evmcs_version, supported_evmcs_version)) {
        fprintf(...)
        return -ENOTSUP;
    }
    cpu->hyperv_nested[0] = evmcs_version;


> 
> > I was expecting this:
> >
> >   vcpu_evmcs_version = 1; /* hardcoded, but can become configurable later */
> >   ...
> >   kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0, (uintptr_t)&supported_evmcs_version);
> >   if (ret < 0 || supported_evmcs_version < vcpu_evmcs_version) {
> >     error_setg(...);
> >     return;
> >   }
> >   cpu->hyperv_nested[0] = vcpu_evmcs_version;
> >
> >
> >> +            error_report("Hyper-V %s verson 1 is not supported by kernel",
> >> +                         kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);

s/verson/version/

> >>              return ret;

What if ret is 0?

> >>          }
> >> -
> >> -        cpu->hyperv_nested[0] = evmcs_version;
> >> +        cpu->hyperv_nested[0] = (1 << 8) | 1;
> >>      }
> >>  
> >>      return 0;
> >> -- 
> >> 2.31.1
> >> 
> 
> -- 
> Vitaly
> 

-- 
Eduardo

Re: [PATCH v7 3/9] i386: hardcode supported eVMCS version to '1'

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Jun 04, 2021 at 09:28:15AM +0200, Vitaly Kuznetsov wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Thu, Jun 03, 2021 at 01:48:29PM +0200, Vitaly Kuznetsov wrote:
>> >> Currently, the only eVMCS version, supported by KVM (and described in TLFS)
>> >> is '1'. When Enlightened VMCS feature is enabled, QEMU takes the supported
>> >> eVMCS version range (from KVM_CAP_HYPERV_ENLIGHTENED_VMCS enablement) and
>> >> puts it to guest visible CPUIDs. When (and if) eVMCS ver.2 appears a
>> >> problem on migration is expected: it doesn't seem to be possible to migrate
>> >> from a host supporting eVMCS ver.2 to a host, which only support eVMCS
>> >> ver.1.
>> >
>> > Isn't it possible and safe to expose eVMCS ver.1 to the guest on
>> > a host that supports ver.2?
>> 
>> We expose the supported range, guest is free to use any eVMCS version in
>> the range (see below):
>
> Oh, I didn't notice the returned value was a range.
>
>> 
>> >
>> >> 
>> >> Hardcode eVMCS ver.1 as the result of 'hv-evmcs' enablement for now. Newer
>> >> eVMCS versions will have to have their own enablement options (e.g.
>> >> 'hv-evmcs=2').
>> >> 
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >> ---
>> >>  docs/hyperv.txt       |  2 +-
>> >>  target/i386/kvm/kvm.c | 16 +++++++++++-----
>> >>  2 files changed, 12 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
>> >> index a51953daa833..000638a2fd38 100644
>> >> --- a/docs/hyperv.txt
>> >> +++ b/docs/hyperv.txt
>> >> @@ -170,7 +170,7 @@ Recommended: hv-frequencies
>> >>  3.16. hv-evmcs
>> >>  ===============
>> >>  The enlightenment is nested specific, it targets Hyper-V on KVM guests. When
>> >> -enabled, it provides Enlightened VMCS feature to the guest. The feature
>> >> +enabled, it provides Enlightened VMCS version 1 feature to the guest. The feature
>> >>  implements paravirtualized protocol between L0 (KVM) and L1 (Hyper-V)
>> >>  hypervisors making L2 exits to the hypervisor faster. The feature is Intel-only.
>> >>  Note: some virtualization features (e.g. Posted Interrupts) are disabled when
>> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> >> index c676ee8b38a7..d57eede5dc81 100644
>> >> --- a/target/i386/kvm/kvm.c
>> >> +++ b/target/i386/kvm/kvm.c
>> >> @@ -1490,13 +1490,19 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>> >>          ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
>> >>                                    (uintptr_t)&evmcs_version);
>> >>  
>> >> -        if (ret < 0) {
>> >> -            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
>> >> -                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
>> >> +        /*
>> >> +         * KVM is required to support EVMCS ver.1. as that's what 'hv-evmcs'
>> >> +         * option sets. Note: we hardcode the maximum supported eVMCS version
>> >> +         * to '1' as well so 'hv-evmcs' feature is migratable even when (and if)
>> >> +         * ver.2 is implemented. A new option (e.g. 'hv-evmcs=2') will then have
>> >> +         * to be added.
>> >> +         */
>> >> +        if (ret < 0 || (uint8_t)evmcs_version > 1) {
>> >
>> > Wait, do you really want to get a fatal error every time, after a
>> > kernel upgrade?
>> >
>> 
>> Here, evmcs_version (returned by kvm_vcpu_enable_cap()) represents a
>> *range* of supported eVMCS versions:
>> 
>> (evmcs_highest_supported_version << 8) | evmcs_lowest_supported_version
>> 
>> Currently, this is 0x101 [1..1] range.
>> 
>> The '(uint8_t)evmcs_version > 1' check here means 'eVMCS v1' is no
>> longer supported by KVM. This is not going to happen any time soon, but
>> I can imagine in 10 years or so we'll be dropping v1 so the range (in
>> theory) can be [10..2] -- which would mean eVMCS ver. 1 is NOT
>> supported. And we can't proceed then.
>
> Where is this documented?  The only reference to
> KVM_CAP_HYPERV_ENLIGHTENED_VMCS I've found in linux/Documentation is this
> single sentence:
>
> | - HYPERV_CPUID_NESTED_FEATURES leaf and HV_X64_ENLIGHTENED_VMCS_RECOMMENDED
> |  feature bit are only exposed when Enlightened VMCS was previously enabled
> |  on the corresponding vCPU (KVM_CAP_HYPERV_ENLIGHTENED_VMCS).
>

It seems that KVM_CAP_HYPERV_ENLIGHTENED_VMCS's documentation is indeed
missing in KVM.

>
>> >>          ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
>> >>                                    (uintptr_t)&evmcs_version);
>> >>  
>> >> -        if (ret < 0) {
>> >> -            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
>> >> -                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
>> >> +        /*
>> >> +         * KVM is required to support EVMCS ver.1. as that's what 'hv-evmcs'
>> >> +         * option sets. Note: we hardcode the maximum supported eVMCS version
>> >> +         * to '1' as well so 'hv-evmcs' feature is migratable even when (and if)
>> >> +         * ver.2 is implemented. A new option (e.g. 'hv-evmcs=2') will then have
>> >> +         * to be added.
>> >> +         */
>> >> +        if (ret < 0 || (uint8_t)evmcs_version > 1) {
>
> Can we start with something that won't need to be rewritten after we change the
> guest evmcs version range?  e.g.:
>
>     static bool evmcs_version_supported(uint16_t version, uint32_t supported)
>     {
>         uint8_t min_ver = version;
>         uint8_t max_ver = version >> 8;
>         uint8_t min_supported = supported;
>         uint8_t max_supported = supported >> 8;
>         return (min_ver >= min_supported) && (max_ver <= max_supported);
>     }
>     ...
>     #define DEFAULT_EVMCS_VERSION ((1 << 8) | 1)
>     ...
>     uint16_t evmcs_version = DEFAULT_EVMCS_VERSION;
>     int ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
>                               (uintptr_t)&supported_evmcs_version);
>     if (ret < 0) {
>         fprintf(...)
>         return ret;
>     }
>     if (!evmcs_version_supported(evmcs_version, supported_evmcs_version)) {
>         fprintf(...)
>         return -ENOTSUP;
>     }
>     cpu->hyperv_nested[0] = evmcs_version;
>

Sure, I don't have anything against an attempt to make this future
proof. Will use this in v8, thanks!

-- 
Vitaly

[PATCH v7 4/9] i386: make hyperv_expand_features() return bool

[Vitaly Kuznetsov]

Return 'false' when hyperv_expand_features() sets an error.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/kvm/kvm.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index d57eede5dc81..a5f8553af921 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1217,12 +1217,12 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
  * of 'hv_passthrough' mode and fills the environment with all supported
  * Hyper-V features.
  */
-static void hyperv_expand_features(CPUState *cs, Error **errp)
+static bool hyperv_expand_features(CPUState *cs, Error **errp)
 {
     X86CPU *cpu = X86_CPU(cs);
 
     if (!hyperv_enabled(cpu))
-        return;
+        return true;
 
     if (cpu->hyperv_passthrough) {
         cpu->hyperv_vendor_id[0] =
@@ -1270,49 +1270,49 @@ static void hyperv_expand_features(CPUState *cs, Error **errp)
 
     /* Features */
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) {
-        return;
+        return false;
     }
     if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) {
-        return;
+        return false;
     }
 
     /* Additional dependencies not covered by kvm_hyperv_properties[] */
@@ -1322,7 +1322,10 @@ static void hyperv_expand_features(CPUState *cs, Error **errp)
         error_setg(errp, "Hyper-V %s requires Hyper-V %s",
                    kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc,
                    kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc);
+        return false;
     }
+
+    return true;
 }
 
 /*
@@ -1563,8 +1566,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
 
     /* Paravirtualization CPUIDs */
-    hyperv_expand_features(cs, &local_err);
-    if (local_err) {
+    if (!hyperv_expand_features(cs, &local_err)) {
         error_report_err(local_err);
         return -ENOSYS;
     }
-- 
2.31.1

Re: [PATCH v7 4/9] i386: make hyperv_expand_features() return bool

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:30PM +0200, Vitaly Kuznetsov wrote:
> Return 'false' when hyperv_expand_features() sets an error.
> 
> No functional change intended.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[PATCH v7 5/9] i386: expand Hyper-V features during CPU feature expansion time

[Vitaly Kuznetsov]

To make Hyper-V features appear in e.g. QMP query-cpu-model-expansion we
need to expand and set the corresponding CPUID leaves early. Modify
x86_cpu_get_supported_feature_word() to call newly intoduced Hyper-V
specific kvm_hv_get_supported_cpuid() instead of
kvm_arch_get_supported_cpuid(). We can't use kvm_arch_get_supported_cpuid()
as Hyper-V specific CPUID leaves intersect with KVM's.

Note, early expansion will only happen when KVM supports system wide
KVM_GET_SUPPORTED_HV_CPUID ioctl (KVM_CAP_SYS_HYPERV_CPUID).

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/cpu.c          |  4 ++++
 target/i386/kvm/kvm-stub.c |  5 +++++
 target/i386/kvm/kvm.c      | 24 ++++++++++++++++++++----
 target/i386/kvm/kvm_i386.h |  1 +
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b2d8e5713911..159b7aa8f073 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5990,6 +5990,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
     if (env->cpuid_xlevel2 == UINT32_MAX) {
         env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
     }
+
+    if (kvm_enabled()) {
+        kvm_hyperv_expand_features(cpu, errp);
+    }
 }
 
 /*
diff --git a/target/i386/kvm/kvm-stub.c b/target/i386/kvm/kvm-stub.c
index 92f49121b8fa..f6e7e4466e1a 100644
--- a/target/i386/kvm/kvm-stub.c
+++ b/target/i386/kvm/kvm-stub.c
@@ -39,3 +39,8 @@ bool kvm_hv_vpindex_settable(void)
 {
     return false;
 }
+
+bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
+{
+    abort();
+}
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index a5f8553af921..650fdd970d6e 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1217,13 +1217,22 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
  * of 'hv_passthrough' mode and fills the environment with all supported
  * Hyper-V features.
  */
-static bool hyperv_expand_features(CPUState *cs, Error **errp)
+bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
 {
-    X86CPU *cpu = X86_CPU(cs);
+    CPUState *cs = CPU(cpu);
 
     if (!hyperv_enabled(cpu))
         return true;
 
+    /*
+     * When kvm_hyperv_expand_features is called at CPU feature expansion
+     * time per-CPU kvm_state is not available yet so we can only proceed
+     * when KVM_CAP_SYS_HYPERV_CPUID is supported.
+     */
+    if (!cs->kvm_state &&
+        !kvm_check_extension(kvm_state, KVM_CAP_SYS_HYPERV_CPUID))
+        return true;
+
     if (cpu->hyperv_passthrough) {
         cpu->hyperv_vendor_id[0] =
             hv_cpuid_get_host(cs, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, R_EBX);
@@ -1565,8 +1574,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
 
     env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
 
-    /* Paravirtualization CPUIDs */
-    if (!hyperv_expand_features(cs, &local_err)) {
+    /*
+     * kvm_hyperv_expand_features() is called here for the second time in case
+     * KVM_CAP_SYS_HYPERV_CPUID is not supported. While we can't possibly handle
+     * 'query-cpu-model-expansion' in this case as we don't have a KVM vCPU to
+     * check which Hyper-V enlightenments are supported and which are not, we
+     * can still proceed and check/expand Hyper-V enlightenments here so legacy
+     * behavior is preserved.
+     */
+    if (!kvm_hyperv_expand_features(cpu, &local_err)) {
         error_report_err(local_err);
         return -ENOSYS;
     }
diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
index dc725083891c..54667b35f09c 100644
--- a/target/i386/kvm/kvm_i386.h
+++ b/target/i386/kvm/kvm_i386.h
@@ -47,6 +47,7 @@ bool kvm_has_x2apic_api(void);
 bool kvm_has_waitpkg(void);
 
 bool kvm_hv_vpindex_settable(void);
+bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp);
 
 uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
 
-- 
2.31.1

Re: [PATCH v7 5/9] i386: expand Hyper-V features during CPU feature expansion time

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:31PM +0200, Vitaly Kuznetsov wrote:
> To make Hyper-V features appear in e.g. QMP query-cpu-model-expansion we
> need to expand and set the corresponding CPUID leaves early. Modify
> x86_cpu_get_supported_feature_word() to call newly intoduced Hyper-V
> specific kvm_hv_get_supported_cpuid() instead of
> kvm_arch_get_supported_cpuid(). We can't use kvm_arch_get_supported_cpuid()
> as Hyper-V specific CPUID leaves intersect with KVM's.
> 
> Note, early expansion will only happen when KVM supports system wide
> KVM_GET_SUPPORTED_HV_CPUID ioctl (KVM_CAP_SYS_HYPERV_CPUID).
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  target/i386/cpu.c          |  4 ++++
>  target/i386/kvm/kvm-stub.c |  5 +++++
>  target/i386/kvm/kvm.c      | 24 ++++++++++++++++++++----
>  target/i386/kvm/kvm_i386.h |  1 +
>  4 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b2d8e5713911..159b7aa8f073 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5990,6 +5990,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>      if (env->cpuid_xlevel2 == UINT32_MAX) {
>          env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>      }
> +
> +    if (kvm_enabled()) {
> +        kvm_hyperv_expand_features(cpu, errp);
> +    }
>  }
>  
>  /*
> diff --git a/target/i386/kvm/kvm-stub.c b/target/i386/kvm/kvm-stub.c
> index 92f49121b8fa..f6e7e4466e1a 100644
> --- a/target/i386/kvm/kvm-stub.c
> +++ b/target/i386/kvm/kvm-stub.c
> @@ -39,3 +39,8 @@ bool kvm_hv_vpindex_settable(void)
>  {
>      return false;
>  }
> +
> +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
> +{
> +    abort();
> +}
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index a5f8553af921..650fdd970d6e 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1217,13 +1217,22 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
>   * of 'hv_passthrough' mode and fills the environment with all supported
>   * Hyper-V features.
>   */
> -static bool hyperv_expand_features(CPUState *cs, Error **errp)
> +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>  {
> -    X86CPU *cpu = X86_CPU(cs);
> +    CPUState *cs = CPU(cpu);
>  
>      if (!hyperv_enabled(cpu))
>          return true;
>  
> +    /*
> +     * When kvm_hyperv_expand_features is called at CPU feature expansion
> +     * time per-CPU kvm_state is not available yet so we can only proceed
> +     * when KVM_CAP_SYS_HYPERV_CPUID is supported.
> +     */
> +    if (!cs->kvm_state &&
> +        !kvm_check_extension(kvm_state, KVM_CAP_SYS_HYPERV_CPUID))
> +        return true;
> +
>      if (cpu->hyperv_passthrough) {
>          cpu->hyperv_vendor_id[0] =
>              hv_cpuid_get_host(cs, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, R_EBX);
> @@ -1565,8 +1574,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>      env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
>  
> -    /* Paravirtualization CPUIDs */
> -    if (!hyperv_expand_features(cs, &local_err)) {
> +    /*
> +     * kvm_hyperv_expand_features() is called here for the second time in case
> +     * KVM_CAP_SYS_HYPERV_CPUID is not supported. While we can't possibly handle
> +     * 'query-cpu-model-expansion' in this case as we don't have a KVM vCPU to
> +     * check which Hyper-V enlightenments are supported and which are not, we
> +     * can still proceed and check/expand Hyper-V enlightenments here so legacy
> +     * behavior is preserved.
> +     */

Issue we might need to solve later: how can management software
differentiate "Hyper-V enlightenments are not supported at all"
from "we can't tell you which Hyper-V enlightenments are
available because KVM_CAP_SYS_HYPERV_CPUID is unavailable"?


> +    if (!kvm_hyperv_expand_features(cpu, &local_err)) {
>          error_report_err(local_err);
>          return -ENOSYS;
>      }
> diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
> index dc725083891c..54667b35f09c 100644
> --- a/target/i386/kvm/kvm_i386.h
> +++ b/target/i386/kvm/kvm_i386.h
> @@ -47,6 +47,7 @@ bool kvm_has_x2apic_api(void);
>  bool kvm_has_waitpkg(void);
>  
>  bool kvm_hv_vpindex_settable(void);
> +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp);
>  
>  uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH v7 5/9] i386: expand Hyper-V features during CPU feature expansion time

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Jun 03, 2021 at 01:48:31PM +0200, Vitaly Kuznetsov wrote:
>> To make Hyper-V features appear in e.g. QMP query-cpu-model-expansion we
>> need to expand and set the corresponding CPUID leaves early. Modify
>> x86_cpu_get_supported_feature_word() to call newly intoduced Hyper-V
>> specific kvm_hv_get_supported_cpuid() instead of
>> kvm_arch_get_supported_cpuid(). We can't use kvm_arch_get_supported_cpuid()
>> as Hyper-V specific CPUID leaves intersect with KVM's.
>> 
>> Note, early expansion will only happen when KVM supports system wide
>> KVM_GET_SUPPORTED_HV_CPUID ioctl (KVM_CAP_SYS_HYPERV_CPUID).
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  target/i386/cpu.c          |  4 ++++
>>  target/i386/kvm/kvm-stub.c |  5 +++++
>>  target/i386/kvm/kvm.c      | 24 ++++++++++++++++++++----
>>  target/i386/kvm/kvm_i386.h |  1 +
>>  4 files changed, 30 insertions(+), 4 deletions(-)
>> 
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index b2d8e5713911..159b7aa8f073 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5990,6 +5990,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>>      if (env->cpuid_xlevel2 == UINT32_MAX) {
>>          env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>>      }
>> +
>> +    if (kvm_enabled()) {
>> +        kvm_hyperv_expand_features(cpu, errp);
>> +    }
>>  }
>>  
>>  /*
>> diff --git a/target/i386/kvm/kvm-stub.c b/target/i386/kvm/kvm-stub.c
>> index 92f49121b8fa..f6e7e4466e1a 100644
>> --- a/target/i386/kvm/kvm-stub.c
>> +++ b/target/i386/kvm/kvm-stub.c
>> @@ -39,3 +39,8 @@ bool kvm_hv_vpindex_settable(void)
>>  {
>>      return false;
>>  }
>> +
>> +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>> +{
>> +    abort();
>> +}
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index a5f8553af921..650fdd970d6e 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1217,13 +1217,22 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
>>   * of 'hv_passthrough' mode and fills the environment with all supported
>>   * Hyper-V features.
>>   */
>> -static bool hyperv_expand_features(CPUState *cs, Error **errp)
>> +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>>  {
>> -    X86CPU *cpu = X86_CPU(cs);
>> +    CPUState *cs = CPU(cpu);
>>  
>>      if (!hyperv_enabled(cpu))
>>          return true;
>>  
>> +    /*
>> +     * When kvm_hyperv_expand_features is called at CPU feature expansion
>> +     * time per-CPU kvm_state is not available yet so we can only proceed
>> +     * when KVM_CAP_SYS_HYPERV_CPUID is supported.
>> +     */
>> +    if (!cs->kvm_state &&
>> +        !kvm_check_extension(kvm_state, KVM_CAP_SYS_HYPERV_CPUID))
>> +        return true;
>> +
>>      if (cpu->hyperv_passthrough) {
>>          cpu->hyperv_vendor_id[0] =
>>              hv_cpuid_get_host(cs, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, R_EBX);
>> @@ -1565,8 +1574,15 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>  
>>      env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
>>  
>> -    /* Paravirtualization CPUIDs */
>> -    if (!hyperv_expand_features(cs, &local_err)) {
>> +    /*
>> +     * kvm_hyperv_expand_features() is called here for the second time in case
>> +     * KVM_CAP_SYS_HYPERV_CPUID is not supported. While we can't possibly handle
>> +     * 'query-cpu-model-expansion' in this case as we don't have a KVM vCPU to
>> +     * check which Hyper-V enlightenments are supported and which are not, we
>> +     * can still proceed and check/expand Hyper-V enlightenments here so legacy
>> +     * behavior is preserved.
>> +     */
>
> Issue we might need to solve later: how can management software
> differentiate "Hyper-V enlightenments are not supported at all"
> from "we can't tell you which Hyper-V enlightenments are
> available because KVM_CAP_SYS_HYPERV_CPUID is unavailable"?
>

In case we're not considering *very* old kernels, basic Hyper-V
enlightenments should always be available so if
'query-cpu-model-expansion' doesn't give you anything (like now,
pre-patch) then you treat this as 'QEMU can't tell me what's supported'
and not like 'no Hyper-V enlightenments at all'.

>
>> +    if (!kvm_hyperv_expand_features(cpu, &local_err)) {
>>          error_report_err(local_err);
>>          return -ENOSYS;
>>      }
>> diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
>> index dc725083891c..54667b35f09c 100644
>> --- a/target/i386/kvm/kvm_i386.h
>> +++ b/target/i386/kvm/kvm_i386.h
>> @@ -47,6 +47,7 @@ bool kvm_has_x2apic_api(void);
>>  bool kvm_has_waitpkg(void);
>>  
>>  bool kvm_hv_vpindex_settable(void);
>> +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp);
>>  
>>  uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address);
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks!

-- 
Vitaly

[PATCH v7 6/9] i386: kill off hv_cpuid_check_and_set()

[Vitaly Kuznetsov]

hv_cpuid_check_and_set() does too much:
- Checks if the feature is supported by KVM;
- Checks if all dependencies are enabled;
- Sets the feature bit in cpu->hyperv_features for 'passthrough' mode.

To reduce the complexity, move all the logic except for dependencies
check out of it. Also, in 'passthrough' mode we don't really need to
check dependencies because KVM is supposed to provide a consistent
set anyway.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/kvm/kvm.c | 104 +++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 68 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 650fdd970d6e..93f9b5a457d5 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1145,16 +1145,12 @@ static bool hyperv_feature_supported(CPUState *cs, int feature)
     return true;
 }
 
-static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
+/* Checks that all feature dependencies are enabled */
+static bool hv_feature_check_deps(X86CPU *cpu, int feature, Error **errp)
 {
-    X86CPU *cpu = X86_CPU(cs);
     uint64_t deps;
     int dep_feat;
 
-    if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) {
-        return 0;
-    }
-
     deps = kvm_hyperv_properties[feature].dependencies;
     while (deps) {
         dep_feat = ctz64(deps);
@@ -1162,26 +1158,12 @@ static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
             error_setg(errp, "Hyper-V %s requires Hyper-V %s",
                        kvm_hyperv_properties[feature].desc,
                        kvm_hyperv_properties[dep_feat].desc);
-            return 1;
+            return false;
         }
         deps &= ~(1ull << dep_feat);
     }
 
-    if (!hyperv_feature_supported(cs, feature)) {
-        if (hyperv_feat_enabled(cpu, feature)) {
-            error_setg(errp, "Hyper-V %s is not supported by kernel",
-                       kvm_hyperv_properties[feature].desc);
-            return 1;
-        } else {
-            return 0;
-        }
-    }
-
-    if (cpu->hyperv_passthrough) {
-        cpu->hyperv_features |= BIT(feature);
-    }
-
-    return 0;
+    return true;
 }
 
 static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
@@ -1220,6 +1202,8 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
 bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
 {
     CPUState *cs = CPU(cpu);
+    Error *local_err = NULL;
+    int feat;
 
     if (!hyperv_enabled(cpu))
         return true;
@@ -1275,53 +1259,37 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
 
         cpu->hyperv_spinlock_attempts =
             hv_cpuid_get_host(cs, HV_CPUID_ENLIGHTMENT_INFO, R_EBX);
-    }
 
-    /* Features */
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) {
-        return false;
-    }
-    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) {
-        return false;
+        /*
+         * Mark feature as enabled in 'cpu->hyperv_features' as
+         * hv_build_cpuid_leaf() uses this info to build guest CPUIDs.
+         */
+        for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
+            if (hyperv_feature_supported(cs, feat)) {
+                cpu->hyperv_features |= BIT(feat);
+            }
+        }
+    } else {
+        /* Check features availability and dependencies */
+        for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
+            /* If the feature was not requested skip it. */
+            if (!hyperv_feat_enabled(cpu, feat)) {
+                continue;
+            }
+
+            /* Check if the feature is supported by KVM */
+            if (!hyperv_feature_supported(cs, feat)) {
+                error_setg(errp, "Hyper-V %s is not supported by kernel",
+                           kvm_hyperv_properties[feat].desc);
+                return false;
+            }
+
+            /* Check dependencies */
+            if (!hv_feature_check_deps(cpu, feat, &local_err)) {
+                error_propagate(errp, local_err);
+                return false;
+            }
+        }
     }
 
     /* Additional dependencies not covered by kvm_hyperv_properties[] */
-- 
2.31.1

[PATCH v7 7/9] i386: HV_HYPERCALL_AVAILABLE privilege bit is always needed

[Vitaly Kuznetsov]

According to TLFS, Hyper-V guest is supposed to check
HV_HYPERCALL_AVAILABLE privilege bit before accessing
HV_X64_MSR_GUEST_OS_ID/HV_X64_MSR_HYPERCALL MSRs but at least some
Windows versions ignore that. As KVM is very permissive and allows
accessing these MSRs unconditionally, no issue is observed. We may,
however, want to tighten the checks eventually. Conforming to the
spec is probably also a good idea.

Enable HV_HYPERCALL_AVAILABLE bit unconditionally.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/kvm/kvm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 93f9b5a457d5..a3897d4d8788 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -810,8 +810,6 @@ static struct {
     [HYPERV_FEAT_RELAXED] = {
         .desc = "relaxed timing (hv-relaxed)",
         .flags = {
-            {.func = HV_CPUID_FEATURES, .reg = R_EAX,
-             .bits = HV_HYPERCALL_AVAILABLE},
             {.func = HV_CPUID_ENLIGHTMENT_INFO, .reg = R_EAX,
              .bits = HV_RELAXED_TIMING_RECOMMENDED}
         }
@@ -820,7 +818,7 @@ static struct {
         .desc = "virtual APIC (hv-vapic)",
         .flags = {
             {.func = HV_CPUID_FEATURES, .reg = R_EAX,
-             .bits = HV_HYPERCALL_AVAILABLE | HV_APIC_ACCESS_AVAILABLE},
+             .bits = HV_APIC_ACCESS_AVAILABLE},
             {.func = HV_CPUID_ENLIGHTMENT_INFO, .reg = R_EAX,
              .bits = HV_APIC_ACCESS_RECOMMENDED}
         }
@@ -829,8 +827,7 @@ static struct {
         .desc = "clocksources (hv-time)",
         .flags = {
             {.func = HV_CPUID_FEATURES, .reg = R_EAX,
-             .bits = HV_HYPERCALL_AVAILABLE | HV_TIME_REF_COUNT_AVAILABLE |
-             HV_REFERENCE_TSC_AVAILABLE}
+             .bits = HV_TIME_REF_COUNT_AVAILABLE | HV_REFERENCE_TSC_AVAILABLE}
         }
     },
     [HYPERV_FEAT_CRASH] = {
@@ -1343,6 +1340,9 @@ static int hyperv_fill_cpuids(CPUState *cs,
     c->ebx = hv_build_cpuid_leaf(cs, HV_CPUID_FEATURES, R_EBX);
     c->edx = hv_build_cpuid_leaf(cs, HV_CPUID_FEATURES, R_EDX);
 
+    /* Unconditionally required with any Hyper-V enlightenment */
+    c->eax |= HV_HYPERCALL_AVAILABLE;
+
     /* Not exposed by KVM but needed to make CPU hotplug in Windows work */
     c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
 
-- 
2.31.1

Re: [PATCH v7 7/9] i386: HV_HYPERCALL_AVAILABLE privilege bit is always needed

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:33PM +0200, Vitaly Kuznetsov wrote:
> According to TLFS, Hyper-V guest is supposed to check
> HV_HYPERCALL_AVAILABLE privilege bit before accessing
> HV_X64_MSR_GUEST_OS_ID/HV_X64_MSR_HYPERCALL MSRs but at least some
> Windows versions ignore that. As KVM is very permissive and allows
> accessing these MSRs unconditionally, no issue is observed. We may,
> however, want to tighten the checks eventually. Conforming to the
> spec is probably also a good idea.
> 
> Enable HV_HYPERCALL_AVAILABLE bit unconditionally.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[PATCH v7 8/9] i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS priviliges

[Vitaly Kuznetsov]

When Hyper-V SynIC is enabled, we may need to allow Windows guests to make
hypercalls (POST_MESSAGES/SIGNAL_EVENTS). No issue is currently observed
because KVM is very permissive, allowing these hypercalls regarding of
guest visible CPUid bits.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/kvm/hyperv-proto.h | 6 ++++++
 target/i386/kvm/kvm.c          | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/target/i386/kvm/hyperv-proto.h b/target/i386/kvm/hyperv-proto.h
index e30d64b4ade4..5fbb385cc136 100644
--- a/target/i386/kvm/hyperv-proto.h
+++ b/target/i386/kvm/hyperv-proto.h
@@ -38,6 +38,12 @@
 #define HV_ACCESS_FREQUENCY_MSRS     (1u << 11)
 #define HV_ACCESS_REENLIGHTENMENTS_CONTROL  (1u << 13)
 
+/*
+ * HV_CPUID_FEATURES.EBX bits
+ */
+#define HV_POST_MESSAGES             (1u << 4)
+#define HV_SIGNAL_EVENTS             (1u << 5)
+
 /*
  * HV_CPUID_FEATURES.EDX bits
  */
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index a3897d4d8788..6a32d43e6ec1 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1343,6 +1343,12 @@ static int hyperv_fill_cpuids(CPUState *cs,
     /* Unconditionally required with any Hyper-V enlightenment */
     c->eax |= HV_HYPERCALL_AVAILABLE;
 
+    /* SynIC and Vmbus devices require messages/signals hypercalls */
+    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) &&
+        !cpu->hyperv_synic_kvm_only) {
+        c->ebx |= HV_POST_MESSAGES | HV_SIGNAL_EVENTS;
+    }
+
     /* Not exposed by KVM but needed to make CPU hotplug in Windows work */
     c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
 
-- 
2.31.1

Re: [PATCH v7 8/9] i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS priviliges

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:34PM +0200, Vitaly Kuznetsov wrote:
> When Hyper-V SynIC is enabled, we may need to allow Windows guests to make
> hypercalls (POST_MESSAGES/SIGNAL_EVENTS). No issue is currently observed
> because KVM is very permissive, allowing these hypercalls regarding of
> guest visible CPUid bits.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  target/i386/kvm/hyperv-proto.h | 6 ++++++
>  target/i386/kvm/kvm.c          | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/target/i386/kvm/hyperv-proto.h b/target/i386/kvm/hyperv-proto.h
> index e30d64b4ade4..5fbb385cc136 100644
> --- a/target/i386/kvm/hyperv-proto.h
> +++ b/target/i386/kvm/hyperv-proto.h
> @@ -38,6 +38,12 @@
>  #define HV_ACCESS_FREQUENCY_MSRS     (1u << 11)
>  #define HV_ACCESS_REENLIGHTENMENTS_CONTROL  (1u << 13)
>  
> +/*
> + * HV_CPUID_FEATURES.EBX bits
> + */
> +#define HV_POST_MESSAGES             (1u << 4)
> +#define HV_SIGNAL_EVENTS             (1u << 5)
> +
>  /*
>   * HV_CPUID_FEATURES.EDX bits
>   */
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index a3897d4d8788..6a32d43e6ec1 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1343,6 +1343,12 @@ static int hyperv_fill_cpuids(CPUState *cs,
>      /* Unconditionally required with any Hyper-V enlightenment */
>      c->eax |= HV_HYPERCALL_AVAILABLE;
>  
> +    /* SynIC and Vmbus devices require messages/signals hypercalls */
> +    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) &&
> +        !cpu->hyperv_synic_kvm_only) {
> +        c->ebx |= HV_POST_MESSAGES | HV_SIGNAL_EVENTS;

Why exactly is the hyperv_synic_kvm_only check needed?

Is the hyperv_synic_kvm_only check the only reason this was done
here and not at kvm_hyperv_properties?


> +    }
> +
>      /* Not exposed by KVM but needed to make CPU hotplug in Windows work */
>      c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
>  
> -- 
> 2.31.1
> 

-- 
Eduardo

Re: [PATCH v7 8/9] i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS priviliges

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Jun 03, 2021 at 01:48:34PM +0200, Vitaly Kuznetsov wrote:
>> When Hyper-V SynIC is enabled, we may need to allow Windows guests to make
>> hypercalls (POST_MESSAGES/SIGNAL_EVENTS). No issue is currently observed
>> because KVM is very permissive, allowing these hypercalls regarding of
>> guest visible CPUid bits.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  target/i386/kvm/hyperv-proto.h | 6 ++++++
>>  target/i386/kvm/kvm.c          | 6 ++++++
>>  2 files changed, 12 insertions(+)
>> 
>> diff --git a/target/i386/kvm/hyperv-proto.h b/target/i386/kvm/hyperv-proto.h
>> index e30d64b4ade4..5fbb385cc136 100644
>> --- a/target/i386/kvm/hyperv-proto.h
>> +++ b/target/i386/kvm/hyperv-proto.h
>> @@ -38,6 +38,12 @@
>>  #define HV_ACCESS_FREQUENCY_MSRS     (1u << 11)
>>  #define HV_ACCESS_REENLIGHTENMENTS_CONTROL  (1u << 13)
>>  
>> +/*
>> + * HV_CPUID_FEATURES.EBX bits
>> + */
>> +#define HV_POST_MESSAGES             (1u << 4)
>> +#define HV_SIGNAL_EVENTS             (1u << 5)
>> +
>>  /*
>>   * HV_CPUID_FEATURES.EDX bits
>>   */
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index a3897d4d8788..6a32d43e6ec1 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1343,6 +1343,12 @@ static int hyperv_fill_cpuids(CPUState *cs,
>>      /* Unconditionally required with any Hyper-V enlightenment */
>>      c->eax |= HV_HYPERCALL_AVAILABLE;
>>  
>> +    /* SynIC and Vmbus devices require messages/signals hypercalls */
>> +    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) &&
>> +        !cpu->hyperv_synic_kvm_only) {
>> +        c->ebx |= HV_POST_MESSAGES | HV_SIGNAL_EVENTS;
>
> Why exactly is the hyperv_synic_kvm_only check needed?
>

'hyperv_synic_kvm_only' means SynIC is only used for in-KVM stimers and
in this case Post Messages / Signal Events hypercalls are not used. KVM
will also return an error to the guest directly (as it can't forward
them to QEMU). No need to expose HV_POST_MESSAGES | HV_SIGNAL_EVENTS.

> Is the hyperv_synic_kvm_only check the only reason this was done
> here and not at kvm_hyperv_properties?
>

Yes, basically.

>
>> +    }
>> +
>>      /* Not exposed by KVM but needed to make CPU hotplug in Windows work */
>>      c->edx |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
>>  
>> -- 
>> 2.31.1
>> 

-- 
Vitaly

Re: [PATCH v7 8/9] i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS priviliges

[Eduardo Habkost]

On Fri, Jun 04, 2021 at 09:35:27AM +0200, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Jun 03, 2021 at 01:48:34PM +0200, Vitaly Kuznetsov wrote:
> >> When Hyper-V SynIC is enabled, we may need to allow Windows guests to make
> >> hypercalls (POST_MESSAGES/SIGNAL_EVENTS). No issue is currently observed
> >> because KVM is very permissive, allowing these hypercalls regarding of
> >> guest visible CPUid bits.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  target/i386/kvm/hyperv-proto.h | 6 ++++++
> >>  target/i386/kvm/kvm.c          | 6 ++++++
> >>  2 files changed, 12 insertions(+)
> >> 
> >> diff --git a/target/i386/kvm/hyperv-proto.h b/target/i386/kvm/hyperv-proto.h
> >> index e30d64b4ade4..5fbb385cc136 100644
> >> --- a/target/i386/kvm/hyperv-proto.h
> >> +++ b/target/i386/kvm/hyperv-proto.h
> >> @@ -38,6 +38,12 @@
> >>  #define HV_ACCESS_FREQUENCY_MSRS     (1u << 11)
> >>  #define HV_ACCESS_REENLIGHTENMENTS_CONTROL  (1u << 13)
> >>  
> >> +/*
> >> + * HV_CPUID_FEATURES.EBX bits
> >> + */
> >> +#define HV_POST_MESSAGES             (1u << 4)
> >> +#define HV_SIGNAL_EVENTS             (1u << 5)
> >> +
> >>  /*
> >>   * HV_CPUID_FEATURES.EDX bits
> >>   */
> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> >> index a3897d4d8788..6a32d43e6ec1 100644
> >> --- a/target/i386/kvm/kvm.c
> >> +++ b/target/i386/kvm/kvm.c
> >> @@ -1343,6 +1343,12 @@ static int hyperv_fill_cpuids(CPUState *cs,
> >>      /* Unconditionally required with any Hyper-V enlightenment */
> >>      c->eax |= HV_HYPERCALL_AVAILABLE;
> >>  
> >> +    /* SynIC and Vmbus devices require messages/signals hypercalls */
> >> +    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) &&
> >> +        !cpu->hyperv_synic_kvm_only) {
> >> +        c->ebx |= HV_POST_MESSAGES | HV_SIGNAL_EVENTS;
> >
> > Why exactly is the hyperv_synic_kvm_only check needed?
> >
> 
> 'hyperv_synic_kvm_only' means SynIC is only used for in-KVM stimers and
> in this case Post Messages / Signal Events hypercalls are not used. KVM
> will also return an error to the guest directly (as it can't forward
> them to QEMU). No need to expose HV_POST_MESSAGES | HV_SIGNAL_EVENTS.
> 
> > Is the hyperv_synic_kvm_only check the only reason this was done
> > here and not at kvm_hyperv_properties?
> >
> 
> Yes, basically.

Thanks for the clarification!

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH v7 8/9] i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS priviliges

[Eric Blake]

On Thu, Jun 03, 2021 at 01:48:34PM +0200, Vitaly Kuznetsov wrote:

In the subject, s/priviliges/privileges/

> When Hyper-V SynIC is enabled, we may need to allow Windows guests to make
> hypercalls (POST_MESSAGES/SIGNAL_EVENTS). No issue is currently observed
> because KVM is very permissive, allowing these hypercalls regarding of
> guest visible CPUid bits.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH v7 9/9] qtest/hyperv: Introduce a simple hyper-v test

[Vitaly Kuznetsov]

For the beginning, just test 'hv-passthrough' and a couple of custom
Hyper-V  enlightenments configurations through QMP. Later, it would
be great to complement this by checking CPUID values from within the
guest.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 MAINTAINERS               |   1 +
 tests/qtest/hyperv-test.c | 225 ++++++++++++++++++++++++++++++++++++++
 tests/qtest/meson.build   |   3 +-
 3 files changed, 228 insertions(+), 1 deletion(-)
 create mode 100644 tests/qtest/hyperv-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5f55404f2fae..862720016b3a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1537,6 +1537,7 @@ F: hw/isa/apm.c
 F: include/hw/isa/apm.h
 F: tests/unit/test-x86-cpuid.c
 F: tests/qtest/test-x86-cpuid-compat.c
+F: tests/qtest/hyperv-test.c
 
 PC Chipset
 M: Michael S. Tsirkin <mst@redhat.com>
diff --git a/tests/qtest/hyperv-test.c b/tests/qtest/hyperv-test.c
new file mode 100644
index 000000000000..0be689548e18
--- /dev/null
+++ b/tests/qtest/hyperv-test.c
@@ -0,0 +1,225 @@
+/*
+ * Hyper-V emulation CPU feature test cases
+ *
+ * Copyright (c) 2021 Red Hat Inc.
+ * Authors:
+ *  Vitaly Kuznetsov <vkuznets@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include <linux/kvm.h>
+#include <sys/ioctl.h>
+
+#include "qemu/osdep.h"
+#include "qemu/bitops.h"
+#include "libqos/libqtest.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qjson.h"
+
+#define MACHINE_KVM "-machine pc-q35-5.2 -accel kvm "
+#define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
+                    "  'arguments': { 'type': 'full', "
+#define QUERY_TAIL  "}}"
+
+static bool kvm_enabled(QTestState *qts)
+{
+    QDict *resp, *qdict;
+    bool enabled;
+
+    resp = qtest_qmp(qts, "{ 'execute': 'query-kvm' }");
+    g_assert(qdict_haskey(resp, "return"));
+    qdict = qdict_get_qdict(resp, "return");
+    g_assert(qdict_haskey(qdict, "enabled"));
+    enabled = qdict_get_bool(qdict, "enabled");
+    qobject_unref(resp);
+
+    return enabled;
+}
+
+static bool kvm_has_sys_hyperv_cpuid(void)
+{
+    int fd = open("/dev/kvm", O_RDWR);
+    int ret;
+
+    g_assert(fd > 0);
+
+    ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_SYS_HYPERV_CPUID);
+
+    close(fd);
+
+    return ret > 0;
+}
+
+static QDict *do_query_no_props(QTestState *qts, const char *cpu_type)
+{
+    return qtest_qmp(qts, QUERY_HEAD "'model': { 'name': %s }"
+                          QUERY_TAIL, cpu_type);
+}
+
+static bool resp_has_props(QDict *resp)
+{
+    QDict *qdict;
+
+    g_assert(resp);
+
+    if (!qdict_haskey(resp, "return")) {
+        return false;
+    }
+    qdict = qdict_get_qdict(resp, "return");
+
+    if (!qdict_haskey(qdict, "model")) {
+        return false;
+    }
+    qdict = qdict_get_qdict(qdict, "model");
+
+    return qdict_haskey(qdict, "props");
+}
+
+static QDict *resp_get_props(QDict *resp)
+{
+    QDict *qdict;
+
+    g_assert(resp);
+    g_assert(resp_has_props(resp));
+
+    qdict = qdict_get_qdict(resp, "return");
+    qdict = qdict_get_qdict(qdict, "model");
+    qdict = qdict_get_qdict(qdict, "props");
+
+    return qdict;
+}
+
+static bool resp_get_feature(QDict *resp, const char *feature)
+{
+    QDict *props;
+
+    g_assert(resp);
+    g_assert(resp_has_props(resp));
+    props = resp_get_props(resp);
+    g_assert(qdict_get(props, feature));
+    return qdict_get_bool(props, feature);
+}
+
+#define assert_has_feature(qts, cpu_type, feature)                     \
+({                                                                     \
+    QDict *_resp = do_query_no_props(qts, cpu_type);                   \
+    g_assert(_resp);                                                   \
+    g_assert(resp_has_props(_resp));                                   \
+    g_assert(qdict_get(resp_get_props(_resp), feature));               \
+    qobject_unref(_resp);                                              \
+})
+
+#define resp_assert_feature(resp, feature, expected_value)             \
+({                                                                     \
+    QDict *_props;                                                     \
+                                                                       \
+    g_assert(_resp);                                                   \
+    g_assert(resp_has_props(_resp));                                   \
+    _props = resp_get_props(_resp);                                    \
+    g_assert(qdict_get(_props, feature));                              \
+    g_assert(qdict_get_bool(_props, feature) == (expected_value));     \
+})
+
+#define assert_feature(qts, cpu_type, feature, expected_value)         \
+({                                                                     \
+    QDict *_resp;                                                      \
+                                                                       \
+    _resp = do_query_no_props(qts, cpu_type);                          \
+    g_assert(_resp);                                                   \
+    resp_assert_feature(_resp, feature, expected_value);               \
+    qobject_unref(_resp);                                              \
+})
+
+#define assert_has_feature_enabled(qts, cpu_type, feature)             \
+    assert_feature(qts, cpu_type, feature, true)
+
+#define assert_has_feature_disabled(qts, cpu_type, feature)            \
+    assert_feature(qts, cpu_type, feature, false)
+
+static void test_assert_hyperv_all_but_evmcs(QTestState *qts)
+{
+    assert_has_feature_enabled(qts, "host", "hv-relaxed");
+    assert_has_feature_enabled(qts, "host", "hv-vapic");
+    assert_has_feature_enabled(qts, "host", "hv-vpindex");
+    assert_has_feature_enabled(qts, "host", "hv-runtime");
+    assert_has_feature_enabled(qts, "host", "hv-crash");
+    assert_has_feature_enabled(qts, "host", "hv-time");
+    assert_has_feature_enabled(qts, "host", "hv-synic");
+    assert_has_feature_enabled(qts, "host", "hv-stimer");
+    assert_has_feature_enabled(qts, "host", "hv-tlbflush");
+    assert_has_feature_enabled(qts, "host", "hv-ipi");
+    assert_has_feature_enabled(qts, "host", "hv-reset");
+    assert_has_feature_enabled(qts, "host", "hv-frequencies");
+    assert_has_feature_enabled(qts, "host", "hv-reenlightenment");
+    assert_has_feature_enabled(qts, "host", "hv-stimer-direct");
+}
+
+static void test_query_cpu_hv_all_but_evmcs(const void *data)
+{
+    QTestState *qts;
+
+    qts = qtest_init(MACHINE_KVM "-cpu host,hv-relaxed,hv-vapic,hv-vpindex,"
+                     "hv-runtime,hv-crash,hv-time,hv-synic,hv-stimer,"
+                     "hv-tlbflush,hv-ipi,hv-reset,hv-frequencies,"
+                     "hv-reenlightenment,hv-stimer-direct");
+
+    test_assert_hyperv_all_but_evmcs(qts);
+
+    qtest_quit(qts);
+}
+
+static void test_query_cpu_hv_custom(const void *data)
+{
+    QTestState *qts;
+
+    qts = qtest_init(MACHINE_KVM "-cpu host,hv-vpindex");
+
+    assert_has_feature_enabled(qts, "host", "hv-vpindex");
+    assert_has_feature_disabled(qts, "host", "hv-synic");
+
+    qtest_quit(qts);
+}
+
+static void test_query_cpu_hv_passthrough(const void *data)
+{
+    QTestState *qts;
+    QDict *resp;
+
+    qts = qtest_init(MACHINE_KVM "-cpu host,hv-passthrough");
+    if (!kvm_enabled(qts)) {
+        qtest_quit(qts);
+        return;
+    }
+
+    test_assert_hyperv_all_but_evmcs(qts);
+
+    resp = do_query_no_props(qts, "host");
+    if (resp_get_feature(resp, "vmx")) {
+        assert_has_feature_enabled(qts, "host", "hv-evmcs");
+    } else {
+        assert_has_feature_disabled(qts, "host", "hv-evmcs");
+    }
+
+    qtest_quit(qts);
+}
+
+int main(int argc, char **argv)
+{
+    const char *arch = qtest_get_arch();
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
+        qtest_add_data_func("/hyperv/hv-all-but-evmcs",
+                            NULL, test_query_cpu_hv_all_but_evmcs);
+        qtest_add_data_func("/hyperv/hv-custom",
+                            NULL, test_query_cpu_hv_custom);
+        if (kvm_has_sys_hyperv_cpuid()) {
+            qtest_add_data_func("/hyperv/hv-passthrough",
+                                NULL, test_query_cpu_hv_passthrough);
+       }
+    }
+
+    return g_test_run();
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c3a223a83d6a..958a88d0c8b4 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -83,7 +83,8 @@ qtests_i386 = \
    'vmgenid-test',
    'migration-test',
    'test-x86-cpuid-compat',
-   'numa-test']
+   'numa-test',
+   'hyperv-test']
 
 dbus_daemon = find_program('dbus-daemon', required: false)
 if dbus_daemon.found() and config_host.has_key('GDBUS_CODEGEN')
-- 
2.31.1

Re: [PATCH v7 9/9] qtest/hyperv: Introduce a simple hyper-v test

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:35PM +0200, Vitaly Kuznetsov wrote:
> For the beginning, just test 'hv-passthrough' and a couple of custom
> Hyper-V  enlightenments configurations through QMP. Later, it would
> be great to complement this by checking CPUID values from within the
> guest.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  MAINTAINERS               |   1 +
>  tests/qtest/hyperv-test.c | 225 ++++++++++++++++++++++++++++++++++++++
>  tests/qtest/meson.build   |   3 +-
>  3 files changed, 228 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qtest/hyperv-test.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f55404f2fae..862720016b3a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1537,6 +1537,7 @@ F: hw/isa/apm.c
>  F: include/hw/isa/apm.h
>  F: tests/unit/test-x86-cpuid.c
>  F: tests/qtest/test-x86-cpuid-compat.c
> +F: tests/qtest/hyperv-test.c

Maybe it makes sense to keep it here by now, but I believe we
should eventually create a section for hyperv in MAINTAINERS.

CCing Michael and Marcel, who are the people listed in this
MAINTAINERS section.


>  
>  PC Chipset
>  M: Michael S. Tsirkin <mst@redhat.com>
[...]
> +int main(int argc, char **argv)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {

Is this necessary when the test is already being added to
qtests_i386/qtests_x86_64 only?

> +        qtest_add_data_func("/hyperv/hv-all-but-evmcs",
> +                            NULL, test_query_cpu_hv_all_but_evmcs);
> +        qtest_add_data_func("/hyperv/hv-custom",
> +                            NULL, test_query_cpu_hv_custom);
> +        if (kvm_has_sys_hyperv_cpuid()) {
> +            qtest_add_data_func("/hyperv/hv-passthrough",
> +                                NULL, test_query_cpu_hv_passthrough);
> +       }
> +    }
> +
> +    return g_test_run();
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index c3a223a83d6a..958a88d0c8b4 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -83,7 +83,8 @@ qtests_i386 = \
>     'vmgenid-test',
>     'migration-test',
>     'test-x86-cpuid-compat',
> -   'numa-test']
> +   'numa-test',
> +   'hyperv-test']
>  
>  dbus_daemon = find_program('dbus-daemon', required: false)
>  if dbus_daemon.found() and config_host.has_key('GDBUS_CODEGEN')
> -- 
> 2.31.1
> 

-- 
Eduardo

Re: [PATCH v7 9/9] qtest/hyperv: Introduce a simple hyper-v test

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Jun 03, 2021 at 01:48:35PM +0200, Vitaly Kuznetsov wrote:
>> For the beginning, just test 'hv-passthrough' and a couple of custom
>> Hyper-V  enlightenments configurations through QMP. Later, it would
>> be great to complement this by checking CPUID values from within the
>> guest.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  MAINTAINERS               |   1 +
>>  tests/qtest/hyperv-test.c | 225 ++++++++++++++++++++++++++++++++++++++
>>  tests/qtest/meson.build   |   3 +-
>>  3 files changed, 228 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/qtest/hyperv-test.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5f55404f2fae..862720016b3a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1537,6 +1537,7 @@ F: hw/isa/apm.c
>>  F: include/hw/isa/apm.h
>>  F: tests/unit/test-x86-cpuid.c
>>  F: tests/qtest/test-x86-cpuid-compat.c
>> +F: tests/qtest/hyperv-test.c
>
> Maybe it makes sense to keep it here by now, but I believe we
> should eventually create a section for hyperv in MAINTAINERS.
>

If we are to create such a section, I'd like to volunteer myself as a
reviewer so I at least get Cced on the stuff.

> CCing Michael and Marcel, who are the people listed in this
> MAINTAINERS section.
>
>
>>  
>>  PC Chipset
>>  M: Michael S. Tsirkin <mst@redhat.com>
> [...]
>> +int main(int argc, char **argv)
>> +{
>> +    const char *arch = qtest_get_arch();
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
>
> Is this necessary when the test is already being added to
> qtests_i386/qtests_x86_64 only?
>

Bad copy-paste source I guess, the check is superfluous indeed.

>> +        qtest_add_data_func("/hyperv/hv-all-but-evmcs",
>> +                            NULL, test_query_cpu_hv_all_but_evmcs);
>> +        qtest_add_data_func("/hyperv/hv-custom",
>> +                            NULL, test_query_cpu_hv_custom);
>> +        if (kvm_has_sys_hyperv_cpuid()) {
>> +            qtest_add_data_func("/hyperv/hv-passthrough",
>> +                                NULL, test_query_cpu_hv_passthrough);
>> +       }
>> +    }
>> +
>> +    return g_test_run();
>> +}
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index c3a223a83d6a..958a88d0c8b4 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -83,7 +83,8 @@ qtests_i386 = \
>>     'vmgenid-test',
>>     'migration-test',
>>     'test-x86-cpuid-compat',
>> -   'numa-test']
>> +   'numa-test',
>> +   'hyperv-test']
>>  
>>  dbus_daemon = find_program('dbus-daemon', required: false)
>>  if dbus_daemon.found() and config_host.has_key('GDBUS_CODEGEN')
>> -- 
>> 2.31.1
>> 

-- 
Vitaly

Re: [PATCH v7 0/9] i386: KVM: expand Hyper-V features early

[Eduardo Habkost]

On Thu, Jun 03, 2021 at 01:48:26PM +0200, Vitaly Kuznetsov wrote:
> Changes since v6 [Eduardo]:
> - First 14 patches of v6 made it upstream!
> - New patches addressing feedback on the already merged patches added:
>   i386: make hyperv_expand_features() return bool
>   i386: hardcode supported eVMCS version to '1'
>   i386: clarify 'hv-passthrough' behavior
>   i386: avoid hardcoding '12' as 'hyperv_vendor_id' length
> - Some comments expanded.
> - HV_HYPERCALL_AVAILABLE setting moved to hyperv_fill_cpuids()
> 
> Note: Windows guest on QEMU are currently completely broken, see Claudio's
> https://lore.kernel.org/kvm/20210529091313.16708-1-cfontana@suse.de/
> 
> The last two functional patches are inspired by 'Fine-grained access check
> to Hyper-V hypercalls and MSRs' work for KVM:
> https://lore.kernel.org/kvm/20210521095204.2161214-1-vkuznets@redhat.com/
> 
> Original description:
> 
> Upper layer tools like libvirt want to figure out which Hyper-V features are
> supported by the underlying stack (QEMU/KVM) but currently they are unable to
> do so. We have a nice 'hv_passthrough' CPU flag supported by QEMU but it has
> no effect on e.g. QMP's 
> 
> query-cpu-model-expansion type=full model={"name":"host","props":{"hv-passthrough":true}}
> 
> command as we parse Hyper-V features after creating KVM vCPUs and not at
> feature expansion time. To support the use-case we first need to make 
> KVM_GET_SUPPORTED_HV_CPUID ioctl a system-wide ioctl as the existing
> vCPU version can't be used that early. This is what KVM part does. With
> that done, we can make early Hyper-V feature expansion (this series).

I'm finally queueing this.  Thanks, and sorry for the delay!

-- 
Eduardo