[PATCH 0/2] i386: Fix Hyper-V Gen1 guests stuck on boot with 'hv-passthrough'

[PATCH 0/2] i386: Fix Hyper-V Gen1 guests stuck on boot with 'hv-passthrough'

[Vitaly Kuznetsov]

Hyper-V Gen1 guests are getting stuck on boot when 'hv-passthrough' is
used. While 'hv-passthrough' is a debug only feature, this significantly
limit its usefullness. While debugging the problem, I found that there are
two loosely connected issues:
- 'hv-passthrough' enables 'hv-syndbg' and this is undesired.
- 'hv-syndbg's support by KVM is detected incorrectly when !CONFIG_SYNDBG.

Fix both issues; exclude 'hv-syndbg' from 'hv-passthrough' and don't allow
to turn on 'hv-syndbg' for !CONFIG_SYNDBG builds. 

Vitaly Kuznetsov (2):
  i386: Fix conditional CONFIG_SYNDBG enablement
  i386: Exclude 'hv-syndbg' from 'hv-passthrough'

 docs/system/i386/hyperv.rst | 13 +++++++++----
 target/i386/cpu.c           |  2 ++
 target/i386/kvm/kvm.c       | 18 ++++++++++++------
 3 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.40.1

[PATCH 1/2] i386: Fix conditional CONFIG_SYNDBG enablement

[Vitaly Kuznetsov]

Putting HYPERV_FEAT_SYNDBG entry under "#ifdef CONFIG_SYNDBG" in
'kvm_hyperv_properties' array is wrong: as HYPERV_FEAT_SYNDBG is not
the highest feature number, the result is an empty (zeroed) entry in
the array (and not a skipped entry!). hyperv_feature_supported() is
designed to check that all CPUID bits are set but for a zeroed
feature in 'kvm_hyperv_properties' it returns 'true' so QEMU considers
HYPERV_FEAT_SYNDBG as always supported, regardless of whether KVM host
actually supports it.

To fix the issue, leave HYPERV_FEAT_SYNDBG's definition in
'kvm_hyperv_properties' array, there's nothing wrong in having it defined
even when 'CONFIG_SYNDBG' is not set. Instead, put "hv-syndbg" CPU property
under '#ifdef CONFIG_SYNDBG' to alter the existing behavior when the flag
is silently skipped in !CONFIG_SYNDBG builds.

Leave an 'assert' sentinel in hyperv_feature_supported() making sure there
are no 'holes' or improperly defined features in 'kvm_hyperv_properties'.

Fixes: d8701185f40c ("hw: hyperv: Initial commit for Synthetic Debugging device")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/cpu.c     |  2 ++
 target/i386/kvm/kvm.c | 11 +++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1242bd541a53..caa207849e9a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7564,8 +7564,10 @@ static Property x86_cpu_properties[] = {
                       HYPERV_FEAT_TLBFLUSH_DIRECT, 0),
     DEFINE_PROP_ON_OFF_AUTO("hv-no-nonarch-coresharing", X86CPU,
                             hyperv_no_nonarch_cs, ON_OFF_AUTO_OFF),
+#ifdef CONFIG_SYNDBG
     DEFINE_PROP_BIT64("hv-syndbg", X86CPU, hyperv_features,
                       HYPERV_FEAT_SYNDBG, 0),
+#endif
     DEFINE_PROP_BOOL("hv-passthrough", X86CPU, hyperv_passthrough, false),
     DEFINE_PROP_BOOL("hv-enforce-cpuid", X86CPU, hyperv_enforce_cpuid, false),
 
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index de531842f6b1..88c75f58f0a6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -989,7 +989,6 @@ static struct {
              .bits = HV_DEPRECATING_AEOI_RECOMMENDED}
         }
     },
-#ifdef CONFIG_SYNDBG
     [HYPERV_FEAT_SYNDBG] = {
         .desc = "Enable synthetic kernel debugger channel (hv-syndbg)",
         .flags = {
@@ -998,7 +997,6 @@ static struct {
         },
         .dependencies = BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_RELAXED)
     },
-#endif
     [HYPERV_FEAT_MSR_BITMAP] = {
         .desc = "enlightened MSR-Bitmap (hv-emsr-bitmap)",
         .flags = {
@@ -1250,6 +1248,13 @@ static bool hyperv_feature_supported(CPUState *cs, int feature)
     uint32_t func, bits;
     int i, reg;
 
+    /*
+     * kvm_hyperv_properties needs to define at least one CPUID flag which
+     * must be used to detect the feature, it's hard to say whether it is
+     * supported or not otherwise.
+     */
+    assert(kvm_hyperv_properties[feature].flags[0].func);
+
     for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) {
 
         func = kvm_hyperv_properties[feature].flags[i].func;
@@ -3474,13 +3479,11 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
                 kvm_msr_entry_add(cpu, HV_X64_MSR_TSC_EMULATION_STATUS,
                                   env->msr_hv_tsc_emulation_status);
             }
-#ifdef CONFIG_SYNDBG
             if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNDBG) &&
                 has_msr_hv_syndbg_options) {
                 kvm_msr_entry_add(cpu, HV_X64_MSR_SYNDBG_OPTIONS,
                                   hyperv_syndbg_query_options());
             }
-#endif
         }
         if (hyperv_feat_enabled(cpu, HYPERV_FEAT_VAPIC)) {
             kvm_msr_entry_add(cpu, HV_X64_MSR_APIC_ASSIST_PAGE,
-- 
2.40.1

[PATCH 2/2] i386: Exclude 'hv-syndbg' from 'hv-passthrough'

[Vitaly Kuznetsov]

Windows with Hyper-V role enabled doesn't boot with 'hv-passthrough' when
no debugger is configured, this significantly limits the usefulness of the
feature as there's no support for subtracting Hyper-V features from CPU
flags at this moment (e.g. "-cpu host,hv-passthrough,-hv-syndbg" does not
work). While this is also theoretically fixable, 'hv-syndbg' is likely
very special and unneeded in the default set. Genuine Hyper-V doesn't seem
to enable it either.

Introduce 'skip_passthrough' flag to 'kvm_hyperv_properties' and use it as
one-off to skip 'hv-syndbg' when enabling features in 'hv-passthrough'
mode. Note, "-cpu host,hv-passthrough,hv-syndbg" can still be used if
needed.

As both 'hv-passthrough' and 'hv-syndbg' are debug features, the change
should not have any effect on production environments.

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

diff --git a/docs/system/i386/hyperv.rst b/docs/system/i386/hyperv.rst
index 2505dc4c86e0..009947e39141 100644
--- a/docs/system/i386/hyperv.rst
+++ b/docs/system/i386/hyperv.rst
@@ -262,14 +262,19 @@ Supplementary features
 ``hv-passthrough``
   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.
+  supported by KVM.
 
   Note: ``hv-passthrough`` flag only enables enlightenments which are known to QEMU
   (have corresponding 'hv-' flag) and copies ``hv-spinlocks`` and ``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.
+  the command line.
+
+  Note: ``hv-passthrough`` does not enable ``hv-syndbg`` which can prevent certain
+  Windows guests from booting when used without proper configuration. If needed,
+  ``hv-syndbg`` can be enabled additionally.
+
+  Note: ``hv-passthrough`` effectively prevents migration as the list of enabled
+  enlightenments may differ between target and destination hosts.
 
 ``hv-enforce-cpuid``
   By default, KVM allows the guest to use all currently supported Hyper-V
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 88c75f58f0a6..fbaaacf9877c 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -867,6 +867,7 @@ static struct {
         uint32_t bits;
     } flags[2];
     uint64_t dependencies;
+    bool skip_passthrough;
 } kvm_hyperv_properties[] = {
     [HYPERV_FEAT_RELAXED] = {
         .desc = "relaxed timing (hv-relaxed)",
@@ -995,7 +996,8 @@ static struct {
             {.func = HV_CPUID_FEATURES, .reg = R_EDX,
              .bits = HV_FEATURE_DEBUG_MSRS_AVAILABLE}
         },
-        .dependencies = BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_RELAXED)
+        .dependencies = BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_RELAXED),
+        .skip_passthrough = true,
     },
     [HYPERV_FEAT_MSR_BITMAP] = {
         .desc = "enlightened MSR-Bitmap (hv-emsr-bitmap)",
@@ -1404,7 +1406,8 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
          * 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)) {
+            if (hyperv_feature_supported(cs, feat) &&
+                !kvm_hyperv_properties[feat].skip_passthrough) {
                 cpu->hyperv_features |= BIT(feat);
             }
         }
-- 
2.40.1

Re: [PATCH 0/2] i386: Fix Hyper-V Gen1 guests stuck on boot with 'hv-passthrough'

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Hyper-V Gen1 guests are getting stuck on boot when 'hv-passthrough' is
> used. While 'hv-passthrough' is a debug only feature, this significantly
> limit its usefullness. While debugging the problem, I found that there are
> two loosely connected issues:
> - 'hv-passthrough' enables 'hv-syndbg' and this is undesired.
> - 'hv-syndbg's support by KVM is detected incorrectly when !CONFIG_SYNDBG.
>
> Fix both issues; exclude 'hv-syndbg' from 'hv-passthrough' and don't allow
> to turn on 'hv-syndbg' for !CONFIG_SYNDBG builds. 
>
> Vitaly Kuznetsov (2):
>   i386: Fix conditional CONFIG_SYNDBG enablement
>   i386: Exclude 'hv-syndbg' from 'hv-passthrough'
>
>  docs/system/i386/hyperv.rst | 13 +++++++++----
>  target/i386/cpu.c           |  2 ++
>  target/i386/kvm/kvm.c       | 18 ++++++++++++------
>  3 files changed, 23 insertions(+), 10 deletions(-)

Ping)

-- 
Vitaly

Re: [PATCH 0/2] i386: Fix Hyper-V Gen1 guests stuck on boot with 'hv-passthrough'

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>
>> Hyper-V Gen1 guests are getting stuck on boot when 'hv-passthrough' is
>> used. While 'hv-passthrough' is a debug only feature, this significantly
>> limit its usefullness. While debugging the problem, I found that there are
>> two loosely connected issues:
>> - 'hv-passthrough' enables 'hv-syndbg' and this is undesired.
>> - 'hv-syndbg's support by KVM is detected incorrectly when !CONFIG_SYNDBG.
>>
>> Fix both issues; exclude 'hv-syndbg' from 'hv-passthrough' and don't allow
>> to turn on 'hv-syndbg' for !CONFIG_SYNDBG builds. 
>>
>> Vitaly Kuznetsov (2):
>>   i386: Fix conditional CONFIG_SYNDBG enablement
>>   i386: Exclude 'hv-syndbg' from 'hv-passthrough'
>>
>>  docs/system/i386/hyperv.rst | 13 +++++++++----
>>  target/i386/cpu.c           |  2 ++
>>  target/i386/kvm/kvm.c       | 18 ++++++++++++------
>>  3 files changed, 23 insertions(+), 10 deletions(-)

Monthly ping)

-- 
Vitaly

Re: [PATCH 0/2] i386: Fix Hyper-V Gen1 guests stuck on boot with 'hv-passthrough'

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>
>> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>>
>>> Hyper-V Gen1 guests are getting stuck on boot when 'hv-passthrough' is
>>> used. While 'hv-passthrough' is a debug only feature, this significantly
>>> limit its usefullness. While debugging the problem, I found that there are
>>> two loosely connected issues:
>>> - 'hv-passthrough' enables 'hv-syndbg' and this is undesired.
>>> - 'hv-syndbg's support by KVM is detected incorrectly when !CONFIG_SYNDBG.
>>>
>>> Fix both issues; exclude 'hv-syndbg' from 'hv-passthrough' and don't allow
>>> to turn on 'hv-syndbg' for !CONFIG_SYNDBG builds. 
>>>
>>> Vitaly Kuznetsov (2):
>>>   i386: Fix conditional CONFIG_SYNDBG enablement
>>>   i386: Exclude 'hv-syndbg' from 'hv-passthrough'
>>>
>>>  docs/system/i386/hyperv.rst | 13 +++++++++----
>>>  target/i386/cpu.c           |  2 ++
>>>  target/i386/kvm/kvm.c       | 18 ++++++++++++------
>>>  3 files changed, 23 insertions(+), 10 deletions(-)
>
> Monthly ping)

Turns out these patches were never merged and honestly I forgot about
them myself. Will resend shortly.

-- 
Vitaly