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

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

[Sairaj Kodilkar]

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

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

[Philippe Mathieu-Daudé]

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

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

And queued, thanks.

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

[Philippe Mathieu-Daudé]

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?

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

[Philippe Mathieu-Daudé]

On 29/11/24 11:29, Philippe Mathieu-Daudé wrote:
> 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(-)
> 
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> And queued, thanks.

Already merged by Paolo as commit 0266aef8cd6. Noticing that
earlier would have saved me some time.

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

[Philippe Mathieu-Daudé]

On 29/11/24 11:29, Philippe Mathieu-Daudé wrote:
> 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(-)
> 
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> And queued, thanks.

Already merged by Paolo as commit 0266aef8cd6. Noticing that
earlier would have saved me some time.