| Message ID | b12cb3819baf6d9ee8140d4dd6d36fa829e2c6d9.1730754238.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/65] softmmu: Expand comments describing max_bounce_buffer_size | expand |
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)
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?
> +
> pci_setup_iommu(bus, &amdvi_iommu_ops, s);
> amdvi_init(s);
> }
> --
> MST
>
Thanks,
Phil
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
Hi Santosh, On Mon, 11 Nov 2024 at 06:39, Shukla, Santosh <santosh.shukla@amd.com> wrote: > > 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); > + } > } Hmm, that does indeed appear to work, but it relies entirely on the call site being optimised away by virtue of kvm_irqchip_is_split() being #defined as 0 and triggering the short-circuit of the && operator at compile time. This seems rather fragile. If kvm_enable_x2apic() needs to be used in code that's built on non-KVM systems, you should really provide a version of it that returns an appropriate value on such systems. kvm_enable_x2apic() is declared in target/i386/kvm/kvm_i386.h So, that's not a "public" cross-subsystem header in "include/…", but it's included from a file in hw/… - this already seems sub-optimal in itself. Be that as it may, target/i386/kvm/kvm_i386.h already starts off with a bunch of conditional declarations in #ifdef CONFIG_KVM … #else … #endif: https://gitlab.com/qemu-project/qemu/-/blob/master/target/i386/kvm/kvm_i386.h#L27 Perhaps you could move the declaration inside the #ifdef CONFIG_KVM and provide a #define kvm_enable_x2apic() 0 for the #else case? It looks like there are other callers of kvm_enable_x2apic() in the Intel IOMMU and some common x86 code. They seem to rely on the same short-circuit optimisation. If the code around those sites is correct *even when not using KVM*, then I think the #define kvm_enable_x2apic() 0 approach would be best. I don't know the correct answer here though, and I can't gauge if the calling code is correct. I'm no expert on IOMMUs nor on KVM, all I know is this commit broke the build for me. :-) > > pci_setup_iommu(bus, &amdvi_iommu_ops, s); > ----- > > Thank you! > Santosh >
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); + } + pci_setup_iommu(bus, &amdvi_iommu_ops, s); amdvi_init(s); }