Re: [PATCH] amd_iommu: Fix kvm_enable_x2apic link error with clang in non-KVM builds

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 14/11/24 12:45, Sairaj Kodilkar wrote:
> Commit b12cb3819 (amd_iommu: Check APIC ID > 255 for XTSup) throws
> linking error for the `kvm_enable_x2apic` when kvm is disabled
> and Clang is used for compilation.
> 
> This issue comes up because Clang does not remove the function callsite
> (kvm_enable_x2apic in this case) during optimization when if condition
> have variable. Intel IOMMU driver solves this issue by creating separate
> if condition for checking variables, which causes call site being
> optimized away by virtue of `kvm_irqchip_is_split()` being defined as 0.
> Implement same solution for the AMD driver.
> 
> Fixes: b12cb3819baf (amd_iommu: Check APIC ID > 255 for XTSup)
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
> Tested-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   hw/i386/amd_iommu.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 13af7211e11d..af0f4da1f69e 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);

Actually I think a clearer fix is:

-- >8 --
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 13af7211e11..9456f494385 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1652,14 +1652,15 @@ static void amdvi_sysbus_realize(DeviceState 
*dev, Error **errp)
      memory_region_add_subregion_overlap(&s->mr_sys, AMDVI_INT_ADDR_FIRST,
                                          &s->mr_ir, 1);

-    /* AMD IOMMU with x2APIC mode requires xtsup=on */
-    if (x86ms->apic_id_limit > 255 && !s->xtsup) {
-        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 (kvm_enabled()) {
+        if (x86ms->apic_id_limit > 255 && !s->xtsup) {
+            error_report("AMD IOMMU with x2APIC configuration 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);
+        }
      }

      pci_setup_iommu(bus, &amdvi_iommu_ops, s);

---

Although half of these checks are already done in x86_cpus_init():

     /*
      * Can we support APIC ID 255 or higher?  With KVM, that requires
      * both in-kernel lapic and X2APIC userspace API.
      *
      * kvm_enabled() must go first to ensure that kvm_* references are
      * not emitted for the linker to consume (kvm_enabled() is
      * a literal `0` in configurations where kvm_* aren't defined)
      */
     if (kvm_enabled() && x86ms->apic_id_limit > 255 &&
         kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
         error_report("current -smp configuration requires kernel "
                      "irqchip and X2APIC API support.");
         exit(EXIT_FAILURE);
     }

So the fix can be simplified as:

-- >8 --
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 13af7211e11..39b6d6ef295 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1652,13 +1652,8 @@ static void amdvi_sysbus_realize(DeviceState 
*dev, Error **errp)
      memory_region_add_subregion_overlap(&s->mr_sys, AMDVI_INT_ADDR_FIRST,
                                          &s->mr_ir, 1);

-    /* AMD IOMMU with x2APIC mode requires xtsup=on */
-    if (x86ms->apic_id_limit > 255 && !s->xtsup) {
-        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");
+    if (kvm_enabled() && x86ms->apic_id_limit > 255 && !s->xtsup) {
+        error_report("AMD IOMMU with x2APIC configuration requires 
xtsup=on");
          exit(EXIT_FAILURE);
      }

---

WDYT?