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@xxxxxxxxxxxxx>
>
> Nevermind, this actually rung a bell and seems to be the same as
> this commit from last year:
>
> commit c04cfb4596ad5032a9869a8f77fe9114ca8af9e0
> Author: Daniel Hoffman <dhoff749@xxxxxxxxx>
> 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@xxxxxxxxx>
> Message-Id: <20231119203116.3027230-1-dhoff749@xxxxxxxxx>
> Reviewed-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>
> 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@xxxxxxx/
and I think Phil confirmed that it works.
Thanks,
Santosh
> Paolo
>
