qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes
@ 2024-09-17 16:00 Vitaly Kuznetsov
  2024-09-17 16:00 ` [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement Vitaly Kuznetsov
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-09-17 16:00 UTC (permalink / raw)
  To: qemu-devel, Paolo Bonzini

Changes since '[PATCH RESEND v3 0/3] i386: Fix Hyper-V Gen1 guests stuck 
on boot with 'hv-passthrough':

- Added "target/i386: Make sure SynIC state is really updated before KVM_RUN" 
  to the set.
 
This is a long pending collection of fixes for various Hyper-V related 
issues, mostly detected by tests. On top of that, the patchset updates
Hyper-V related documentation adding recommendations.

Vitaly Kuznetsov (4):
  target/i386: Fix conditional CONFIG_SYNDBG enablement
  target/i386: Exclude 'hv-syndbg' from 'hv-passthrough'
  target/i386: Make sure SynIC state is really updated before KVM_RUN
  docs/system: Add recommendations to Hyper-V enlightenments doc

 docs/system/i386/hyperv.rst | 43 +++++++++++++++++++++++++++++++++----
 target/i386/cpu.c           |  2 ++
 target/i386/kvm/hyperv.c    |  1 +
 target/i386/kvm/kvm.c       | 18 ++++++++++------
 4 files changed, 54 insertions(+), 10 deletions(-)

-- 
2.46.0



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement
  2024-09-17 16:00 [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
@ 2024-09-17 16:00 ` Vitaly Kuznetsov
  2024-11-14 10:46   ` Michael Tokarev
  2024-09-17 16:00 ` [PATCH RESEND v4 2/4] target/i386: Exclude 'hv-syndbg' from 'hv-passthrough' Vitaly Kuznetsov
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-09-17 16:00 UTC (permalink / raw)
  To: qemu-devel, Paolo Bonzini

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 85ef7452c04e..62d4fdfd599a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -8291,8 +8291,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 ada581c5d6ea..4009fcfd6b29 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1035,7 +1035,6 @@ static struct {
              .bits = HV_DEPRECATING_AEOI_RECOMMENDED}
         }
     },
-#ifdef CONFIG_SYNDBG
     [HYPERV_FEAT_SYNDBG] = {
         .desc = "Enable synthetic kernel debugger channel (hv-syndbg)",
         .flags = {
@@ -1044,7 +1043,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 = {
@@ -1296,6 +1294,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;
@@ -3924,13 +3929,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.46.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RESEND v4 2/4] target/i386: Exclude 'hv-syndbg' from 'hv-passthrough'
  2024-09-17 16:00 [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
  2024-09-17 16:00 ` [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement Vitaly Kuznetsov
@ 2024-09-17 16:00 ` Vitaly Kuznetsov
  2024-09-17 16:00 ` [PATCH RESEND v4 3/4] target/i386: Make sure SynIC state is really updated before KVM_RUN Vitaly Kuznetsov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-09-17 16:00 UTC (permalink / raw)
  To: qemu-devel, Paolo Bonzini

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 4009fcfd6b29..cf0449968f66 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -913,6 +913,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)",
@@ -1041,7 +1042,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)",
@@ -1450,7 +1452,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.46.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RESEND v4 3/4] target/i386: Make sure SynIC state is really updated before KVM_RUN
  2024-09-17 16:00 [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
  2024-09-17 16:00 ` [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement Vitaly Kuznetsov
  2024-09-17 16:00 ` [PATCH RESEND v4 2/4] target/i386: Exclude 'hv-syndbg' from 'hv-passthrough' Vitaly Kuznetsov
@ 2024-09-17 16:00 ` Vitaly Kuznetsov
  2024-09-17 16:00 ` [PATCH RESEND v4 4/4] docs/system: Add recommendations to Hyper-V enlightenments doc Vitaly Kuznetsov
  2024-09-30 13:56 ` [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
  4 siblings, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-09-17 16:00 UTC (permalink / raw)
  To: qemu-devel, Paolo Bonzini

'hyperv_synic' test from KVM unittests was observed to be flaky on certain
hardware (hangs sometimes). Debugging shows that the problem happens in
hyperv_sint_route_new() when the test tries to set up a new SynIC
route. The function bails out on:

 if (!synic->sctl_enabled) {
         goto cleanup;
 }

but the test writes to HV_X64_MSR_SCONTROL just before it starts
establishing SINT routes. Further investigation shows that
synic_update() (called from async_synic_update()) happens after the SINT
setup attempt and not before. Apparently, the comment before
async_safe_run_on_cpu() in kvm_hv_handle_exit() does not correctly describe
the guarantees async_safe_run_on_cpu() gives. In particular, async worked
added to a CPU is actually processed from qemu_wait_io_event() which is not
always called before KVM_RUN, i.e. kvm_cpu_exec() checks whether an exit
request is pending for a CPU and if not, keeps running the vCPU until it
meets an exit it can't handle internally. Hyper-V specific MSR writes are
not automatically trigger an exit.

Fix the issue by simply raising an exit request for the vCPU where SynIC
update was queued. This is not a performance critical path as SynIC state
does not get updated so often (and async_safe_run_on_cpu() is a big hammer
anyways).

Reported-by: Jan Richter <jarichte@redhat.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/kvm/hyperv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c
index b94f12acc2c9..70b89cacf94b 100644
--- a/target/i386/kvm/hyperv.c
+++ b/target/i386/kvm/hyperv.c
@@ -80,6 +80,7 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
          * necessary because memory hierarchy is being changed
          */
         async_safe_run_on_cpu(CPU(cpu), async_synic_update, RUN_ON_CPU_NULL);
+        cpu_exit(CPU(cpu));
 
         return EXCP_INTERRUPT;
     case KVM_EXIT_HYPERV_HCALL: {
-- 
2.46.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH RESEND v4 4/4] docs/system: Add recommendations to Hyper-V enlightenments doc
  2024-09-17 16:00 [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
                   ` (2 preceding siblings ...)
  2024-09-17 16:00 ` [PATCH RESEND v4 3/4] target/i386: Make sure SynIC state is really updated before KVM_RUN Vitaly Kuznetsov
@ 2024-09-17 16:00 ` Vitaly Kuznetsov
  2024-09-30 13:56 ` [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
  4 siblings, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-09-17 16:00 UTC (permalink / raw)
  To: qemu-devel, Paolo Bonzini

While hyperv.rst already has all currently implemented Hyper-V
enlightenments documented, it may be unclear what is the recommended set to
achieve the best result. Add the corresponding section to the doc.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 docs/system/i386/hyperv.rst | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/docs/system/i386/hyperv.rst b/docs/system/i386/hyperv.rst
index 009947e39141..1c1de77feb65 100644
--- a/docs/system/i386/hyperv.rst
+++ b/docs/system/i386/hyperv.rst
@@ -283,6 +283,36 @@ Supplementary features
   feature alters this behavior and only allows the guest to use exposed Hyper-V
   enlightenments.
 
+Recommendations
+---------------
+
+To achieve the best performance of Windows and Hyper-V guests and unless there
+are any specific requirements (e.g. migration to older QEMU/KVM versions,
+emulating specific Hyper-V version, ...), it is recommended to enable all
+currently implemented Hyper-V enlightenments with the following exceptions:
+
+- ``hv-syndbg``, ``hv-passthrough``, ``hv-enforce-cpuid`` should not be enabled
+  in production configurations as these are debugging/development features.
+- ``hv-reset`` can be avoided as modern Hyper-V versions don't expose it.
+- ``hv-evmcs`` can (and should) be enabled on Intel CPUs only. While the feature
+  is only used in nested configurations (Hyper-V, WSL2), enabling it for regular
+  Windows guests should not have any negative effects.
+- ``hv-no-nonarch-coresharing`` must only be enabled if vCPUs are properly pinned
+  so no non-architectural core sharing is possible.
+- ``hv-vendor-id``, ``hv-version-id-build``, ``hv-version-id-major``,
+  ``hv-version-id-minor``, ``hv-version-id-spack``, ``hv-version-id-sbranch``,
+  ``hv-version-id-snumber`` can be left unchanged, guests are not supposed to
+  behave differently when different Hyper-V version is presented to them.
+- ``hv-crash`` must only be enabled if the crash information is consumed via
+  QAPI by higher levels of the virtualization stack. Enabling this feature
+  effectively prevents Windows from creating dumps upon crashes.
+- ``hv-reenlightenment`` can only be used on hardware which supports TSC
+  scaling or when guest migration is not needed.
+- ``hv-spinlocks`` should be set to e.g. 0xfff when host CPUs are overcommited
+  (meaning there are other scheduled tasks or guests) and can be left unchanged
+  from the default value (0xffffffff) otherwise.
+- ``hv-avic``/``hv-apicv`` should not be enabled if the hardware does not
+  support APIC virtualization (Intel APICv, AMD AVIC).
 
 Useful links
 ------------
-- 
2.46.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes
  2024-09-17 16:00 [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
                   ` (3 preceding siblings ...)
  2024-09-17 16:00 ` [PATCH RESEND v4 4/4] docs/system: Add recommendations to Hyper-V enlightenments doc Vitaly Kuznetsov
@ 2024-09-30 13:56 ` Vitaly Kuznetsov
  2024-10-14  9:04   ` Vitaly Kuznetsov
  4 siblings, 1 reply; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-09-30 13:56 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Changes since '[PATCH RESEND v3 0/3] i386: Fix Hyper-V Gen1 guests stuck 
> on boot with 'hv-passthrough':
>
> - Added "target/i386: Make sure SynIC state is really updated before KVM_RUN" 
>   to the set.
>  
> This is a long pending collection of fixes for various Hyper-V related 
> issues, mostly detected by tests. On top of that, the patchset updates
> Hyper-V related documentation adding recommendations.
>
> Vitaly Kuznetsov (4):
>   target/i386: Fix conditional CONFIG_SYNDBG enablement
>   target/i386: Exclude 'hv-syndbg' from 'hv-passthrough'
>   target/i386: Make sure SynIC state is really updated before KVM_RUN
>   docs/system: Add recommendations to Hyper-V enlightenments doc
>
>  docs/system/i386/hyperv.rst | 43 +++++++++++++++++++++++++++++++++----
>  target/i386/cpu.c           |  2 ++
>  target/i386/kvm/hyperv.c    |  1 +
>  target/i386/kvm/kvm.c       | 18 ++++++++++------
>  4 files changed, 54 insertions(+), 10 deletions(-)

Ping) Some of these patches are really getting old :-)

-- 
Vitaly



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes
  2024-09-30 13:56 ` [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
@ 2024-10-14  9:04   ` Vitaly Kuznetsov
  0 siblings, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-10-14  9:04 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>
>> Changes since '[PATCH RESEND v3 0/3] i386: Fix Hyper-V Gen1 guests stuck 
>> on boot with 'hv-passthrough':
>>
>> - Added "target/i386: Make sure SynIC state is really updated before KVM_RUN" 
>>   to the set.
>>  
>> This is a long pending collection of fixes for various Hyper-V related 
>> issues, mostly detected by tests. On top of that, the patchset updates
>> Hyper-V related documentation adding recommendations.
>>
>> Vitaly Kuznetsov (4):
>>   target/i386: Fix conditional CONFIG_SYNDBG enablement
>>   target/i386: Exclude 'hv-syndbg' from 'hv-passthrough'
>>   target/i386: Make sure SynIC state is really updated before KVM_RUN
>>   docs/system: Add recommendations to Hyper-V enlightenments doc
>>
>>  docs/system/i386/hyperv.rst | 43 +++++++++++++++++++++++++++++++++----
>>  target/i386/cpu.c           |  2 ++
>>  target/i386/kvm/hyperv.c    |  1 +
>>  target/i386/kvm/kvm.c       | 18 ++++++++++------
>>  4 files changed, 54 insertions(+), 10 deletions(-)
>
> Ping) Some of these patches are really getting old :-)

Ping)

-- 
Vitaly



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement
  2024-09-17 16:00 ` [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement Vitaly Kuznetsov
@ 2024-11-14 10:46   ` Michael Tokarev
  2024-11-14 11:33     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Tokarev @ 2024-11-14 10:46 UTC (permalink / raw)
  To: Vitaly Kuznetsov, qemu-devel, Paolo Bonzini

17.09.2024 19:00, 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>

> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index ada581c5d6ea..4009fcfd6b29 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
...
> @@ -3924,13 +3929,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

This change broke a minimal build:

$ ../configure --without-default-features --without-default-devices --target-list=x86_64-softmmu --enable-kvm
...
FAILED: qemu-system-x86_64
cc -m64 @qemu-system-x86_64.rsp
/usr/bin/ld: libqemu-x86_64-softmmu.a.p/target_i386_kvm_kvm.c.o: in function `kvm_put_msrs':
target/i386/kvm/kvm.c:4039:(.text+0x83ae): undefined reference to `hyperv_syndbg_query_options'
collect2: error: ld returned 1 exit status

Why this particular #ifdef has been removed?

Thanks,

/mjt


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement
  2024-11-14 10:46   ` Michael Tokarev
@ 2024-11-14 11:33     ` Vitaly Kuznetsov
  0 siblings, 0 replies; 9+ messages in thread
From: Vitaly Kuznetsov @ 2024-11-14 11:33 UTC (permalink / raw)
  To: Michael Tokarev, qemu-devel, Paolo Bonzini

Michael Tokarev <mjt@tls.msk.ru> writes:

> 17.09.2024 19:00, 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>
>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index ada581c5d6ea..4009fcfd6b29 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
> ...
>> @@ -3924,13 +3929,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
>
> This change broke a minimal build:

Sorry about that :-(

>
> $ ../configure --without-default-features --without-default-devices --target-list=x86_64-softmmu --enable-kvm
> ...
> FAILED: qemu-system-x86_64
> cc -m64 @qemu-system-x86_64.rsp
> /usr/bin/ld: libqemu-x86_64-softmmu.a.p/target_i386_kvm_kvm.c.o: in function `kvm_put_msrs':
> target/i386/kvm/kvm.c:4039:(.text+0x83ae): undefined reference to `hyperv_syndbg_query_options'
> collect2: error: ld returned 1 exit status
>
> Why this particular #ifdef has been removed?

The patch was on the list for over a year before it got accepted and I
completely forgot the details... Looking at it now and I don't believe
we need HV_X64_MSR_SYNDBG_OPTIONS at all when !CONFIG_SYNDBG so I guess
we can bring the ifdef back. Let me do some quick tests and I'll send a
patch.

-- 
Vitaly



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-11-14 11:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-17 16:00 [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
2024-09-17 16:00 ` [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement Vitaly Kuznetsov
2024-11-14 10:46   ` Michael Tokarev
2024-11-14 11:33     ` Vitaly Kuznetsov
2024-09-17 16:00 ` [PATCH RESEND v4 2/4] target/i386: Exclude 'hv-syndbg' from 'hv-passthrough' Vitaly Kuznetsov
2024-09-17 16:00 ` [PATCH RESEND v4 3/4] target/i386: Make sure SynIC state is really updated before KVM_RUN Vitaly Kuznetsov
2024-09-17 16:00 ` [PATCH RESEND v4 4/4] docs/system: Add recommendations to Hyper-V enlightenments doc Vitaly Kuznetsov
2024-09-30 13:56 ` [PATCH RESEND v4 0/4] target/i386: Various Hyper-V related fixes Vitaly Kuznetsov
2024-10-14  9:04   ` Vitaly Kuznetsov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).