Hi Santosh,
On Mon, 11 Nov 2024 at 06:39, Shukla, Santosh <santosh.shu...@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 <m...@redhat.com> wrote:
> >>
> >> From: Suravee Suthikulpanit <suravee.suthikulpa...@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.jime...@oracle.com>
> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
> >> Signed-off-by: Santosh Shukla <santosh.shu...@amd.com>
> >> Message-Id: <20240927172913.121477-6-santosh.shu...@amd.com>
> >> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <m...@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
>
