Re: [PATCH v1] iommu/amd: Recognize Hygon southbridge IOAPIC for interrupt remapping

[Wei Wang]

On 3/25/26 7:37 PM, Pu Wen wrote:
> On 2026/3/25 19:23, Wei Wang wrote:
> > On 3/25/26 6:54 PM, Pu Wen wrote:
> > > On 2026/3/25 17:39, Wei Wang wrote:
> > > > Hygon systems use the AMD IOMMU driver but newer Hygon platforms report
> > > > the southbridge IOAPIC at a different PCI device (00:0b.0), whereas AMD
> > > > systems use device 00:14.0.
> > > ......
> > > > +/* SB IOAPIC device IDs */
> > > > +#define IOAPIC_SB_DEVID_AMD    ((0x00 << 8) | PCI_DEVFN(0x14, 0))
> > > > +#define IOAPIC_SB_DEVID_HYGON    ((0x00 << 8) | PCI_DEVFN(0x0b, 0))
> > > These macro definitions is not accurate, old Hygon processor also use
> > > device 0x14, as you mentioned device 0xb is only for 'newer Hygon 
> > > platforms'.
> >
> > Yes, I noticed that. I mentioned this in the commit log:
> >
> > Older Hygon systems still use the original AMD device ID (00:14.0) while
> > newer systems use 00:0b.0, so a strict CPU vendor check is insufficient.
> >
> > That is also why the code uses the logical OR:
> > if (devid == IOAPIC_SB_DEVID_AMD || devid == IOAPIC_SB_DEVID_HYGON)
>
> IMO, if only newer Hygon processor will use device 0xb, the macro should
> use a exact name(eg: with a new Hygon family/model), let IOAPIC_SB_DEVID
> as the common macro for AMD and Hygon.

Yes, that's one naming convention. The one I used is based on the 
origination. IOAPIC_SB_DEVID_AMD originated from AMD, though it is 
reusable for Hygon.

The naming might not be that critical. I've been thinking maybe we need 
to make this more robust and scalable. Rather than hardcoding 
per‑vendor/per‑generation device IDs that may require patching every 
time a new architecture is released, we can actually get the Southbridge 
IOAPIC devid (the southbridge IOAPIC is part of the FCH) from PCI topo:

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 56ad020df494..c3940fafe3a0 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3096,18 +3096,44 @@ static void __init free_iommu_resources(void)
        free_pci_segments();
 }

-/* SB IOAPIC is always on this device in AMD systems */
-#define IOAPIC_SB_DEVID                ((0x00 << 8) | PCI_DEVFN(0x14, 0))
+/*
+ * Find the Southbridge IOAPIC DevID before PCI probing. Scans Bus 0 
for the
+ * FCH (Southbridge) by looking for the ISA Bridge or SMBus identifier.
+ */
+static int __init get_sb_ioapic_devid_early(void)
+{
+       int dev;
+       u32 val;
+
+       /* Scan Bus 0, only checking Function 0 of each device */
+       for (dev = 0; dev < 32; dev++) {
+               val = read_pci_config(0, dev, 0, PCI_VENDOR_ID);
+               if (val == 0xffffffff)
+                       continue;
+
+               val = read_pci_config_16(0, dev, 0, PCI_CLASS_DEVICE);
+               if (val == PCI_CLASS_BRIDGE_ISA ||
+                   val == PCI_CLASS_SERIAL_SMBUS)
+                       return PCI_DEVID(0, PCI_DEVFN(dev, 0));
+       }
+
+       return -ENODEV;
+}

 static bool __init check_ioapic_information(void)
 {
        const char *fw_bug = FW_BUG;
        bool ret, has_sb_ioapic;
-       int idx;
+       int idx, sb_ioapic_devid = get_sb_ioapic_devid_early();

        has_sb_ioapic = false;
        ret           = false;

+       if (sb_ioapic_devid == -ENODEV) {
+               fw_bug = HW_ERR;
+               goto out;
+       }
+
        /*
         * If we have map overrides on the kernel command line the
         * messages in this function might not describe firmware bugs
@@ -3124,12 +3150,13 @@ static bool __init check_ioapic_information(void)
                        pr_err("%s: IOAPIC[%d] not in IVRS table\n",
                                fw_bug, id);
                        ret = false;
-               } else if (devid == IOAPIC_SB_DEVID) {
+               } else if (devid == sb_ioapic_devid) {
                        has_sb_ioapic = true;
                        ret           = true;
                }
        }

+out:
        if (!has_sb_ioapic) {
                /*
                 * We expect the SB IOAPIC to be listed in the IVRS