Re: [PULL 32/65] amd_iommu: Check APIC ID > 255 for XTSup

[Shukla, Santosh]

Hi Phil,

On 11/10/2024 4:36 PM, Phil Dennis-Jordan wrote:
> Hi,
> 
> This commit seems to be causing link errors, likely on all platforms
> where KVM is not available, but at minimum that's what I'm seeing when
> trying to build the staging branch on macOS.
> 
> ld: Undefined symbols:
>   _kvm_enable_x2apic, referenced from:
>       _amdvi_sysbus_realize in hw_i386_amd_iommu.c.o
> clang: error: linker command failed with exit code 1 (use -v to see 
> invocation)
> 
> 

Hmm,.

Thank you for reporting.

> On Mon, 4 Nov 2024 at 22:10, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> The XTSup mode enables x2APIC support for AMD IOMMU, which is needed
>> to support vcpu w/ APIC ID > 255.
>>
>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Santosh Shukla <santosh.shukla@amd.com>
>> Message-Id: <20240927172913.121477-6-santosh.shukla@amd.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  hw/i386/amd_iommu.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 38297376e7..13af7211e1 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -32,6 +32,7 @@
>>  #include "trace.h"
>>  #include "hw/i386/apic-msidef.h"
>>  #include "hw/qdev-properties.h"
>> +#include "kvm/kvm_i386.h"
>>
>>  /* used AMD-Vi MMIO registers */
>>  const char *amdvi_mmio_low[] = {
>> @@ -1651,6 +1652,16 @@ 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);
>> +    }
> 
> I suspect this last if() { … } block should be wrapped in an #ifdef
> CONFIG_KVM block? I don't know anything about this code though, so if
> this whole code path is generally a KVM-only feature, perhaps the KVM
> check should be implemented at the build system dependency level?
>

Could you please try below in your target system w/ clang and confirm back?

-----
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 13af7211e1..7081d448e4 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1657,9 +1657,12 @@ 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);
-----

Thank you!
Santosh