| Message ID | 20230911103832.23596-1-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386: Re-introduce few KVM stubs for Clang debug builds | expand |
On Mon, 11 Sept 2023 at 06:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Since commits 3adce820cf..ef1cf6890f, When building on > a x86 host configured as: > > $ ./configure --cc=clang \ > --target-list=x86_64-linux-user,x86_64-softmmu \ > --enable-debug > > we get: > > [71/71] Linking target qemu-x86_64 > FAILED: qemu-x86_64 > /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `cpu_x86_cpuid': > cpu.c:(.text+0x1374): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `x86_cpu_filter_features': > cpu.c:(.text+0x81c2): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: cpu.c:(.text+0x81da): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: cpu.c:(.text+0x81f2): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: cpu.c:(.text+0x820a): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o:cpu.c:(.text+0x8225): more undefined references to `kvm_arch_get_supported_cpuid' follow > clang: error: linker command failed with exit code 1 (use -v to see invocation) > ninja: build stopped: subcommand failed. > > '--enable-debug' disables optimizations (CFLAGS=-O0). > > While at this (un)optimization level GCC eliminate the > following dead code: > > if (0 && foo()) { > ... > } > > Clang does not. Therefore restore a pair of stubs for > unoptimized Clang builds. > > Reported-by: Kevin Wolf <kwolf@redhat.com> > Fixes: 3adce820cf ("target/i386: Remove unused KVM stubs") > Fixes: ef1cf6890f ("target/i386: Allow elision of kvm_hv_vpindex_settable()") > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > target/i386/kvm/kvm_i386.h | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h > index 55d4e68c34..0b62ac628f 100644 > --- a/target/i386/kvm/kvm_i386.h > +++ b/target/i386/kvm/kvm_i386.h > @@ -32,7 +32,6 @@ > > bool kvm_has_smm(void); > bool kvm_enable_x2apic(void); > -bool kvm_hv_vpindex_settable(void); > bool kvm_has_pit_state2(void); > > bool kvm_enable_sgx_provisioning(KVMState *s); > @@ -41,8 +40,6 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp); > void kvm_arch_reset_vcpu(X86CPU *cs); > void kvm_arch_after_reset_vcpu(X86CPU *cpu); > void kvm_arch_do_init_vcpu(X86CPU *cs); > -uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, > - uint32_t index, int reg); > uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index); > > void kvm_set_max_apic_id(uint32_t max_apic_id); > @@ -60,6 +57,10 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value); > > bool kvm_has_x2apic_api(void); > bool kvm_has_waitpkg(void); > +bool kvm_hv_vpindex_settable(void); > + > +uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, > + uint32_t index, int reg); > > uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address); > void kvm_update_msi_routes_all(void *private, bool global, > @@ -76,6 +77,20 @@ typedef struct kvm_msr_handlers { > bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr, > QEMUWRMSRHandler *wrmsr); > > +#elif defined(__clang__) && !defined(__OPTIMIZE__) Another approach is a static library with a .o file containing the stubs so the linker only includes it in the executable if the compiler emitted the symbols. That way there is no need for defined(__clang__) && !defined(__OPTIMIZE__) and it will work with other compilers/optimization levels. It's more work to set up though. Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Mon, Sep 11, 2023 at 12:38:32PM +0200, Philippe Mathieu-Daudé wrote: > Since commits 3adce820cf..ef1cf6890f, When building on > a x86 host configured as: > > $ ./configure --cc=clang \ > --target-list=x86_64-linux-user,x86_64-softmmu \ > --enable-debug > > we get: > > [71/71] Linking target qemu-x86_64 > FAILED: qemu-x86_64 > /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `cpu_x86_cpuid': > cpu.c:(.text+0x1374): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `x86_cpu_filter_features': > cpu.c:(.text+0x81c2): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: cpu.c:(.text+0x81da): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: cpu.c:(.text+0x81f2): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: cpu.c:(.text+0x820a): undefined reference to `kvm_arch_get_supported_cpuid' > /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o:cpu.c:(.text+0x8225): more undefined references to `kvm_arch_get_supported_cpuid' follow > clang: error: linker command failed with exit code 1 (use -v to see invocation) > ninja: build stopped: subcommand failed. > > '--enable-debug' disables optimizations (CFLAGS=-O0). > > While at this (un)optimization level GCC eliminate the > following dead code: > > if (0 && foo()) { > ... > } > > Clang does not. Therefore restore a pair of stubs for > unoptimized Clang builds. > > Reported-by: Kevin Wolf <kwolf@redhat.com> > Fixes: 3adce820cf ("target/i386: Remove unused KVM stubs") > Fixes: ef1cf6890f ("target/i386: Allow elision of kvm_hv_vpindex_settable()") Why not just revert those two commits, since we now learned the rationale for them was incorrect ? > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > target/i386/kvm/kvm_i386.h | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) With regards, Daniel
11.09.2023 14:56, Stefan Hajnoczi: > Or instead of using linker behavior, maybe just change the #ifdef so it only applies when KVM is disabled. I didn't look at the code to see if this is > possible, but it would be nice to avoid the very specific #ifdef condition in this patch. Yeah, this is definitely preferrable to define it based on !kvm instead of !optimize. I mean the same thing when replied in the pullreq which broke things, - to base it on !kvm. But use something very similar to what Philippe did with the functions themselves (not with the condition). /mjt
On 11/9/23 14:32, Kevin Wolf wrote: > Am 11.09.2023 um 13:15 hat Stefan Hajnoczi geschrieben: >> On Mon, 11 Sept 2023 at 06:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>> >>> Since commits 3adce820cf..ef1cf6890f, When building on >>> a x86 host configured as: >>> >>> $ ./configure --cc=clang \ >>> --target-list=x86_64-linux-user,x86_64-softmmu \ >>> --enable-debug >>> >>> we get: >>> >>> [71/71] Linking target qemu-x86_64 >>> FAILED: qemu-x86_64 >>> /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `cpu_x86_cpuid': >>> cpu.c:(.text+0x1374): undefined reference to `kvm_arch_get_supported_cpuid' >>> /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `x86_cpu_filter_features': >>> cpu.c:(.text+0x81c2): undefined reference to `kvm_arch_get_supported_cpuid' >>> /usr/bin/ld: cpu.c:(.text+0x81da): undefined reference to `kvm_arch_get_supported_cpuid' >>> /usr/bin/ld: cpu.c:(.text+0x81f2): undefined reference to `kvm_arch_get_supported_cpuid' >>> /usr/bin/ld: cpu.c:(.text+0x820a): undefined reference to `kvm_arch_get_supported_cpuid' >>> /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o:cpu.c:(.text+0x8225): more undefined references to `kvm_arch_get_supported_cpuid' follow >>> clang: error: linker command failed with exit code 1 (use -v to see invocation) >>> ninja: build stopped: subcommand failed. >>> >>> '--enable-debug' disables optimizations (CFLAGS=-O0). >>> >>> While at this (un)optimization level GCC eliminate the >>> following dead code: >>> >>> if (0 && foo()) { >>> ... >>> } >>> >>> Clang does not. Therefore restore a pair of stubs for >>> unoptimized Clang builds. >>> >>> Reported-by: Kevin Wolf <kwolf@redhat.com> >>> Fixes: 3adce820cf ("target/i386: Remove unused KVM stubs") >>> Fixes: ef1cf6890f ("target/i386: Allow elision of kvm_hv_vpindex_settable()") >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> target/i386/kvm/kvm_i386.h | 21 ++++++++++++++++++--- >>> 1 file changed, 18 insertions(+), 3 deletions(-) >>> +#elif defined(__clang__) && !defined(__OPTIMIZE__) >> >> Another approach is a static library with a .o file containing the >> stubs so the linker only includes it in the executable if the compiler >> emitted the symbols. That way there is no need for defined(__clang__) >> && !defined(__OPTIMIZE__) and it will work with other >> compilers/optimization levels. It's more work to set up though. > > Isn't that exactly how it was before the stubs were removed? It would be > a simple revert of that commit. > > The approach with static inline functions defined only for a very > specific configuration looks a lot more fragile to me. In fact, I'm > surprised that it works because I think it requires that the header > isn't used in any files that are shared between user space and system > emulation - and naively cpu.c sounded like something that could be > shared. Looks like this patch only works because the linux-user target > uses a separate build of the same CPU emulation source file. > > So I think reverting the commit that removed the stubs would be much > more obvious. Yes, v2 reverts: https://lore.kernel.org/qemu-devel/20230911131507.24943-1-philmd@linaro.org/
diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h index 55d4e68c34..0b62ac628f 100644 --- a/target/i386/kvm/kvm_i386.h +++ b/target/i386/kvm/kvm_i386.h @@ -32,7 +32,6 @@ bool kvm_has_smm(void); bool kvm_enable_x2apic(void); -bool kvm_hv_vpindex_settable(void); bool kvm_has_pit_state2(void); bool kvm_enable_sgx_provisioning(KVMState *s); @@ -41,8 +40,6 @@ bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp); void kvm_arch_reset_vcpu(X86CPU *cs); void kvm_arch_after_reset_vcpu(X86CPU *cpu); void kvm_arch_do_init_vcpu(X86CPU *cs); -uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, - uint32_t index, int reg); uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index); void kvm_set_max_apic_id(uint32_t max_apic_id); @@ -60,6 +57,10 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value); bool kvm_has_x2apic_api(void); bool kvm_has_waitpkg(void); +bool kvm_hv_vpindex_settable(void); + +uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, + uint32_t index, int reg); uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address); void kvm_update_msi_routes_all(void *private, bool global, @@ -76,6 +77,20 @@ typedef struct kvm_msr_handlers { bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr, QEMUWRMSRHandler *wrmsr); +#elif defined(__clang__) && !defined(__OPTIMIZE__) + +static inline bool kvm_hv_vpindex_settable(void) +{ + g_assert_not_reached(); +} + +static inline uint32_t kvm_arch_get_supported_cpuid(KVMState *env, + uint32_t function, + uint32_t index, int reg) +{ + g_assert_not_reached(); +} + #endif /* CONFIG_KVM */ void kvm_pc_setup_irq_routing(bool pci_enabled);
Since commits 3adce820cf..ef1cf6890f, When building on a x86 host configured as: $ ./configure --cc=clang \ --target-list=x86_64-linux-user,x86_64-softmmu \ --enable-debug we get: [71/71] Linking target qemu-x86_64 FAILED: qemu-x86_64 /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `cpu_x86_cpuid': cpu.c:(.text+0x1374): undefined reference to `kvm_arch_get_supported_cpuid' /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o: in function `x86_cpu_filter_features': cpu.c:(.text+0x81c2): undefined reference to `kvm_arch_get_supported_cpuid' /usr/bin/ld: cpu.c:(.text+0x81da): undefined reference to `kvm_arch_get_supported_cpuid' /usr/bin/ld: cpu.c:(.text+0x81f2): undefined reference to `kvm_arch_get_supported_cpuid' /usr/bin/ld: cpu.c:(.text+0x820a): undefined reference to `kvm_arch_get_supported_cpuid' /usr/bin/ld: libqemu-x86_64-linux-user.fa.p/target_i386_cpu.c.o:cpu.c:(.text+0x8225): more undefined references to `kvm_arch_get_supported_cpuid' follow clang: error: linker command failed with exit code 1 (use -v to see invocation) ninja: build stopped: subcommand failed. '--enable-debug' disables optimizations (CFLAGS=-O0). While at this (un)optimization level GCC eliminate the following dead code: if (0 && foo()) { ... } Clang does not. Therefore restore a pair of stubs for unoptimized Clang builds. Reported-by: Kevin Wolf <kwolf@redhat.com> Fixes: 3adce820cf ("target/i386: Remove unused KVM stubs") Fixes: ef1cf6890f ("target/i386: Allow elision of kvm_hv_vpindex_settable()") Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- target/i386/kvm/kvm_i386.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)