[PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Phil Dennis-Jordan]

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(-)

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);
 
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Paolo Bonzini]

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);
>   

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 3177 bytes --]

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);
> >
>
>

[-- Attachment #2: Type: text/html, Size: 4317 bytes --]

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Paolo Bonzini]

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

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Shukla, Santosh]

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
> 

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Paolo Bonzini]

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

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Shukla, Santosh]

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
> 

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

Paolo, could we please apply either Sairaj and Santosh's fix at
https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/
or mine to fix this link error? As neither patch has so far been merged,
9.2.0-rc2 still fails to build on macOS, at least on my local systems. I'm
not sure why CI builds aren't jumping up and down about this, but neither
the Xcode 15.2 nor 16.1 toolchains are happy on macOS 14.7/arm64.


On Wed, 13 Nov 2024 at 19:39, Paolo Bonzini <pbonzini@redhat.com> 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?
>
> Paolo
>
>
>

[-- Attachment #2: Type: text/html, Size: 1467 bytes --]

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Philippe Mathieu-Daudé]

On 28/11/24 17:38, Phil Dennis-Jordan wrote:
> Paolo, could we please apply either Sairaj and Santosh's fix at
> https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/ 
> <https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/>
> or mine to fix this link error? As neither patch has so far been merged, 
> 9.2.0-rc2 still fails to build on macOS, at least on my local systems. 
> I'm not sure why CI builds aren't jumping up and down about this, but 
> neither the Xcode 15.2 nor 16.1 toolchains are happy on macOS 14.7/arm64.

Just curious, is your build configured with --enable-hvf --enable-tcg?

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]

On Thu, 28 Nov 2024 at 17:46, Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 28/11/24 17:38, Phil Dennis-Jordan wrote:
> > Paolo, could we please apply either Sairaj and Santosh's fix at
> > https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/
> > <https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/>
> > or mine to fix this link error? As neither patch has so far been merged,
> > 9.2.0-rc2 still fails to build on macOS, at least on my local systems.
> > I'm not sure why CI builds aren't jumping up and down about this, but
> > neither the Xcode 15.2 nor 16.1 toolchains are happy on macOS 14.7/arm64.
>
> Just curious, is your build configured with --enable-hvf --enable-tcg?
>
>
It's my understanding that both HVF and TCG are enabled by default when
building on macOS - they both show up as "YES" in the ./configure output,
and the relevant -accel works; at any rate, specifying them explicitly made
no difference with regard to this link error. Your question did however
prompt me to dig a little deeper and check which of my test configurations
was affected.

It looks like the critical setting is --enable-debug. I think that changes
the exact optimisation level (not -O0 but less aggressive than the
default), so it's not unreasonable that this would change the compiler
pass(es) for eliminating constant conditional branches.

So yeah, when I build latest master/staging with --enable-debug on macOS
and my --target-list includes x86_64, QEMU fails to link with an undefined
symbol error for _kvm_enable_x2apic. This happens on both arm64 and x86-64
hosts, and with various Xcode 15.x and 16.y toolchains.

I have to admit I'm personally not a big fan of relying on the optimiser
for removing references to these symbols, but restructuring the conditional
expression like in Sairaj and Santosh's patch seems to allow even the
optimisation level used for debug builds to do it, so I guess I can't argue
with the result. :-)

Phil

[-- Attachment #2: Type: text/html, Size: 2697 bytes --]

Re: [PATCH] i386/kvm: Fix kvm_enable_x2apic link error in non-KVM builds

[Philippe Mathieu-Daudé]

On 28/11/24 20:06, Phil Dennis-Jordan wrote:
> On Thu, 28 Nov 2024 at 17:46, Philippe Mathieu-Daudé <philmd@linaro.org 
> <mailto:philmd@linaro.org>> wrote:
> 
>     On 28/11/24 17:38, Phil Dennis-Jordan wrote:
>      > Paolo, could we please apply either Sairaj and Santosh's fix at
>      > https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/
>     <https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/>
>      >
>     <https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/
>     <https://patchew.org/QEMU/20241114114509.15350-1-sarunkod@amd.com/>>
>      > or mine to fix this link error? As neither patch has so far been
>     merged,
>      > 9.2.0-rc2 still fails to build on macOS, at least on my local
>     systems.
>      > I'm not sure why CI builds aren't jumping up and down about this,
>     but
>      > neither the Xcode 15.2 nor 16.1 toolchains are happy on macOS
>     14.7/arm64.
> 
>     Just curious, is your build configured with --enable-hvf --enable-tcg?
> 
> 
> It's my understanding that both HVF and TCG are enabled by default when 
> building on macOS - they both show up as "YES" in the ./configure 
> output, and the relevant -accel works; at any rate, specifying them 
> explicitly made no difference with regard to this link error. Your 
> question did however prompt me to dig a little deeper and check which of 
> my test configurations was affected.
> 
> It looks like the critical setting is --enable-debug. I think that 
> changes the exact optimisation level (not -O0 but less aggressive than 
> the default), so it's not unreasonable that this would change the 
> compiler pass(es) for eliminating constant conditional branches.
> 
> So yeah, when I build latest master/staging with --enable-debug on macOS 
> and my --target-list includes x86_64, QEMU fails to link with an 
> undefined symbol error for _kvm_enable_x2apic. This happens on both 
> arm64 and x86-64 hosts, and with various Xcode 15.x and 16.y toolchains.

Indeed:

C compiler for the host machine: clang (clang 16.0.0 "Apple clang 
version 16.0.0 (clang-1600.0.26.4)")
C linker for the host machine: clang ld64 1115.7.3
Host machine cpu family: aarch64
Host machine cpu: aarch64
   Compilation
     host CPU                        : aarch64
     host endianness                 : little
     C compiler                      : clang
     Host C compiler                 : clang
     C++ compiler                    : NO
     Objective-C compiler            : clang
     Rust support                    : NO
     CFLAGS                          : -g -O0
   User defined options
     optimization                    : 0

Undefined symbols for architecture arm64:
   "_kvm_enable_x2apic", referenced from:
       _amdvi_sysbus_realize in hw_i386_amd_iommu.c.o
ld: symbol(s) not found for architecture arm64


> I have to admit I'm personally not a big fan of relying on the optimiser 
> for removing references to these symbols, but restructuring the 
> conditional expression like in Sairaj and Santosh's patch seems to allow 
> even the optimisation level used for debug builds to do it, so I guess I 
> can't argue with the result. :-)

See related commit 9926cf34de5 ("target/i386: Allow elision of 
kvm_enable_x2apic()").