| Message ID | 20241113144923.41225-1-phil@philjordan.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds | expand |
On 11/13/24 15:49, Phil Dennis-Jordan wrote: > It appears that existing call sites for the kvm_enable_x2apic() > function rely on the compiler eliding the calls during optimisation > when building with KVM disabled, or on platforms other than Linux, > where that function is declared but not defined. > > This fragile reliance recently broke down when commit b12cb38 added > a new call site which apparently failed to be optimised away when > building QEMU on macOS with clang, resulting in a link error. That's weird, can you check the preprocessor output? The definition of kvm_irqchip_in_kernel() should be just "false" on macOS, in fact even the area you're changing should be simplified like diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h index 9de9c0d3038..7edb154a16e 100644 --- a/target/i386/kvm/kvm_i386.h +++ b/target/i386/kvm/kvm_i386.h @@ -13,8 +13,7 @@ #include "sysemu/kvm.h" -#ifdef CONFIG_KVM - +/* always false if !CONFIG_KVM */ #define kvm_pit_in_kernel() \ (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) #define kvm_pic_in_kernel() \ @@ -22,14 +21,6 @@ #define kvm_ioapic_in_kernel() \ (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) -#else - -#define kvm_pit_in_kernel() 0 -#define kvm_pic_in_kernel() 0 -#define kvm_ioapic_in_kernel() 0 - -#endif /* CONFIG_KVM */ - bool kvm_has_smm(void); bool kvm_enable_x2apic(void); Paolo > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > --- > target/i386/kvm/kvm_i386.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h > index 9de9c0d3038..7ce47388d90 100644 > --- a/target/i386/kvm/kvm_i386.h > +++ b/target/i386/kvm/kvm_i386.h > @@ -21,17 +21,18 @@ > (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) > #define kvm_ioapic_in_kernel() \ > (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) > +bool kvm_enable_x2apic(void); > > #else > > #define kvm_pit_in_kernel() 0 > #define kvm_pic_in_kernel() 0 > #define kvm_ioapic_in_kernel() 0 > +#define kvm_enable_x2apic() 0 > > #endif /* CONFIG_KVM */ > > bool kvm_has_smm(void); > -bool kvm_enable_x2apic(void); > bool kvm_hv_vpindex_settable(void); > bool kvm_enable_hypercall(uint64_t enable_mask); >
On 11/13/24 15:49, Phil Dennis-Jordan wrote: > It appears that existing call sites for the kvm_enable_x2apic() > function rely on the compiler eliding the calls during optimisation > when building with KVM disabled, or on platforms other than Linux, > where that function is declared but not defined. > > This fragile reliance recently broke down when commit b12cb38 added > a new call site which apparently failed to be optimised away when > building QEMU on macOS with clang, resulting in a link error. > > This change moves the function declaration into the existing > #if CONFIG_KVM > block in the same header file, while the corresponding > #else > block now #defines the symbol as 0, same as for various other > KVM-specific query functions. > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> Nevermind, this actually rung a bell and seems to be the same as this commit from last year: commit c04cfb4596ad5032a9869a8f77fe9114ca8af9e0 Author: Daniel Hoffman <dhoff749@gmail.com> Date: Sun Nov 19 12:31:16 2023 -0800 hw/i386: fix short-circuit logic with non-optimizing builds `kvm_enabled()` is compiled down to `0` and short-circuit logic is used to remove references to undefined symbols at the compile stage. Some build configurations with some compilers don't attempt to simplify this logic down in some cases (the pattern appears to be that the literal false must be the first term) and this was causing some builds to emit references to undefined symbols. An example of such a configuration is clang 16.0.6 with the following configure: ./configure --enable-debug --without-default-features --target-list=x86_64-softmmu --enable-tcg-interpreter Signed-off-by: Daniel Hoffman <dhoff749@gmail.com> Message-Id: <20231119203116.3027230-1-dhoff749@gmail.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> So, this should work: diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index 13af7211e11..af0f4da1f69 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -1657,9 +1657,11 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp) error_report("AMD IOMMU with x2APIC confguration requires xtsup=on"); exit(EXIT_FAILURE); } - if (s->xtsup && kvm_irqchip_is_split() && !kvm_enable_x2apic()) { - error_report("AMD IOMMU xtsup=on requires support on the KVM side"); - exit(EXIT_FAILURE); + if (s->xtsup) { + if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) { + error_report("AMD IOMMU xtsup=on requires support on the KVM side"); + exit(EXIT_FAILURE); + } } pci_setup_iommu(bus, &amdvi_iommu_ops, s); It's admittedly a bit brittle, but it's already done in the neighboring hw/i386/intel_iommu.c so I guess it's okay. Paolo
On Wed 13. Nov 2024 at 19:06, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 11/13/24 15:49, Phil Dennis-Jordan wrote: > > It appears that existing call sites for the kvm_enable_x2apic() > > function rely on the compiler eliding the calls during optimisation > > when building with KVM disabled, or on platforms other than Linux, > > where that function is declared but not defined. > > > > This fragile reliance recently broke down when commit b12cb38 added > > a new call site which apparently failed to be optimised away when > > building QEMU on macOS with clang, resulting in a link error. > > That's weird, can you check the preprocessor output? The definition > of kvm_irqchip_in_kernel() should be just "false" on macOS, in fact > even the area you're changing should be simplified like > Yeah, to be honest this header file seems a bit of a mess. It’s being #included by a bunch of .c files in hw/ despite being located in target/ not include/ which suggests to me that some of the declarations ought to be moved. As I’m not really familiar with any of the KVM code base I would not normally be attempting to clean this up but the staging & master branches currently don’t build on macOS, so I was aiming for the simplest fix possible for the immediate problem. (I will test your patch shortly however.) > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h > index 9de9c0d3038..7edb154a16e 100644 > --- a/target/i386/kvm/kvm_i386.h > +++ b/target/i386/kvm/kvm_i386.h > @@ -13,8 +13,7 @@ > > #include "sysemu/kvm.h" > > -#ifdef CONFIG_KVM > - > +/* always false if !CONFIG_KVM */ > #define kvm_pit_in_kernel() \ > (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) > #define kvm_pic_in_kernel() \ > @@ -22,14 +21,6 @@ > #define kvm_ioapic_in_kernel() \ > (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) > > -#else > - > -#define kvm_pit_in_kernel() 0 > -#define kvm_pic_in_kernel() 0 > -#define kvm_ioapic_in_kernel() 0 > - > -#endif /* CONFIG_KVM */ > - > bool kvm_has_smm(void); > bool kvm_enable_x2apic(void); > > Paolo > > > > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > > --- > > target/i386/kvm/kvm_i386.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h > > index 9de9c0d3038..7ce47388d90 100644 > > --- a/target/i386/kvm/kvm_i386.h > > +++ b/target/i386/kvm/kvm_i386.h > > @@ -21,17 +21,18 @@ > > (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) > > #define kvm_ioapic_in_kernel() \ > > (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) > > +bool kvm_enable_x2apic(void); > > > > #else > > > > #define kvm_pit_in_kernel() 0 > > #define kvm_pic_in_kernel() 0 > > #define kvm_ioapic_in_kernel() 0 > > +#define kvm_enable_x2apic() 0 > > > > #endif /* CONFIG_KVM */ > > > > bool kvm_has_smm(void); > > -bool kvm_enable_x2apic(void); > > bool kvm_hv_vpindex_settable(void); > > bool kvm_enable_hypercall(uint64_t enable_mask); > > > >
On 11/13/2024 11:41 PM, Paolo Bonzini wrote: > On 11/13/24 15:49, Phil Dennis-Jordan wrote: >> It appears that existing call sites for the kvm_enable_x2apic() >> function rely on the compiler eliding the calls during optimisation >> when building with KVM disabled, or on platforms other than Linux, >> where that function is declared but not defined. >> >> This fragile reliance recently broke down when commit b12cb38 added >> a new call site which apparently failed to be optimised away when >> building QEMU on macOS with clang, resulting in a link error. >> >> This change moves the function declaration into the existing >> #if CONFIG_KVM >> block in the same header file, while the corresponding >> #else >> block now #defines the symbol as 0, same as for various other >> KVM-specific query functions. >> >> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > > Nevermind, this actually rung a bell and seems to be the same as > this commit from last year: > > commit c04cfb4596ad5032a9869a8f77fe9114ca8af9e0 > Author: Daniel Hoffman <dhoff749@gmail.com> > Date: Sun Nov 19 12:31:16 2023 -0800 > > hw/i386: fix short-circuit logic with non-optimizing builds > `kvm_enabled()` is compiled down to `0` and short-circuit logic is > used to remove references to undefined symbols at the compile stage. > Some build configurations with some compilers don't attempt to > simplify this logic down in some cases (the pattern appears to be > that the literal false must be the first term) and this was causing > some builds to emit references to undefined symbols. > An example of such a configuration is clang 16.0.6 with the following > configure: ./configure --enable-debug --without-default-features > --target-list=x86_64-softmmu --enable-tcg-interpreter > Signed-off-by: Daniel Hoffman <dhoff749@gmail.com> > Message-Id: <20231119203116.3027230-1-dhoff749@gmail.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > So, this should work: > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > index 13af7211e11..af0f4da1f69 100644 > --- a/hw/i386/amd_iommu.c > +++ b/hw/i386/amd_iommu.c > @@ -1657,9 +1657,11 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp) > error_report("AMD IOMMU with x2APIC confguration requires xtsup=on"); > exit(EXIT_FAILURE); > } > - if (s->xtsup && kvm_irqchip_is_split() && !kvm_enable_x2apic()) { > - error_report("AMD IOMMU xtsup=on requires support on the KVM side"); > - exit(EXIT_FAILURE); > + if (s->xtsup) { > + if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) { > + error_report("AMD IOMMU xtsup=on requires support on the KVM side"); > + exit(EXIT_FAILURE); > + } > } > > pci_setup_iommu(bus, &amdvi_iommu_ops, s); > > > It's admittedly a bit brittle, but it's already done in the neighboring > hw/i386/intel_iommu.c so I guess it's okay. > Same proposed at https://lore.kernel.org/qemu-devel/cebca38a-5896-e2a5-8a68-5edad5dc9d8c@amd.com/ and I think Phil confirmed that it works. Thanks, Santosh > Paolo >
On Wed, Nov 13, 2024 at 7:25 PM Shukla, Santosh <santosh.shukla@amd.com> wrote: > Same proposed at https://lore.kernel.org/qemu-devel/cebca38a-5896-e2a5-8a68-5edad5dc9d8c@amd.com/ > and I think Phil confirmed that it works. Thanks Santosh, can you post it with commit message and everything? Paolo
On 11/14/2024 12:09 AM, Paolo Bonzini wrote: > On Wed, Nov 13, 2024 at 7:25 PM Shukla, Santosh <santosh.shukla@amd.com> wrote: >> Same proposed at https://lore.kernel.org/qemu-devel/cebca38a-5896-e2a5-8a68-5edad5dc9d8c@amd.com/ >> and I think Phil confirmed that it works. > > Thanks Santosh, can you post it with commit message and everything? > Sure thing, will send by tomorrow. Regards, Santosh > Paolo >
diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h index 9de9c0d3038..7ce47388d90 100644 --- a/target/i386/kvm/kvm_i386.h +++ b/target/i386/kvm/kvm_i386.h @@ -21,17 +21,18 @@ (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) #define kvm_ioapic_in_kernel() \ (kvm_irqchip_in_kernel() && !kvm_irqchip_is_split()) +bool kvm_enable_x2apic(void); #else #define kvm_pit_in_kernel() 0 #define kvm_pic_in_kernel() 0 #define kvm_ioapic_in_kernel() 0 +#define kvm_enable_x2apic() 0 #endif /* CONFIG_KVM */ bool kvm_has_smm(void); -bool kvm_enable_x2apic(void); bool kvm_hv_vpindex_settable(void); bool kvm_enable_hypercall(uint64_t enable_mask);
It appears that existing call sites for the kvm_enable_x2apic() function rely on the compiler eliding the calls during optimisation when building with KVM disabled, or on platforms other than Linux, where that function is declared but not defined. This fragile reliance recently broke down when commit b12cb38 added a new call site which apparently failed to be optimised away when building QEMU on macOS with clang, resulting in a link error. This change moves the function declaration into the existing #if CONFIG_KVM block in the same header file, while the corresponding #else block now #defines the symbol as 0, same as for various other KVM-specific query functions. Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> --- target/i386/kvm/kvm_i386.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)