| Message ID | 20240917160051.2637594-2-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: Various Hyper-V related fixes | expand |
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
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.
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,
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(-)