target/i386: Various Hyper-V related fixes

[PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement

Posted by Vitaly Kuznetsov 1 year, 2 months ago

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

Re: [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement

Posted by Michael Tokarev 1 year ago

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

Re: [PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement

Posted by Vitaly Kuznetsov 1 year ago

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

[PATCH RESEND v4 1/4] target/i386: Fix conditional CONFIG_SYNDBG enablement
[PATCH RESEND v4 2/4] target/i386: Exclude 'hv-syndbg' from 'hv-passthrough'
[PATCH RESEND v4 3/4] target/i386: Make sure SynIC state is really updated before KVM_RUN
[PATCH RESEND v4 4/4] docs/system: Add recommendations to Hyper-V enlightenments doc