Re: [PATCH v3 2/3] iommu/amd: Do not convert IVMD exclusion range to unity range

[Sairaj Kodilkar <sarunkod@amd.com>]

On 11/6/2026 3:32 PM, Jindal, Akshay wrote:
> 
> 
> On 06-11-2026 09:46, Sairaj Kodilkar wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Original problem was there could be multiple IVMD exclusion entries
>> conflicting over a single exclusion range supported in older Linux
>> kernel. The fix was first introduced in commit 387caf0b759a ("iommu/amd:
>> Treat per-device exclusion ranges as r/w unity-mapped regions") and in
>> commit 0bbe4ced53e3 ("iommu/amd: Fix the overwritten field in IVMD header")
>> , where IVMD exclusions are converted to separate unity_map_entry objects
>> in pci_seg->unity_map so amd_iommu_get_resv_regions() treated every such
>> entry as IOMMU_RESV_DIRECT with R/W.
>>
>> The driver no longer programs the exclusion base/limit registers and
>> relies on the kernel to keep the IOVA ranges named by exclusion IVMDs
>> unmapped. Stop forcing exclusion entries to RW-unity. Store the raw
>> IVMD flags in struct unity_map_entry instead of a pre-shifted
>> protection value, and derive the reserved region from those flags.
>> Exclusion entries are now reported as IOMMU_RESV_RESERVED with no
>> read/write permission.
>>
>> Reject unity entries that grant neither IR nor IW with a FW_BUG
>> message, and warn about invalid flag combinations and unknown flag bits
>>
>> Fixes: 0bbe4ced53e3 ("iommu/amd: Fix the overwritten field in IVMD header")
>> Cc: Baoquan He <bhe@redhat.com>
>> Cc: Adrian Huang <ahuang12@lenovo.com>
>> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> Reviewed-by: Adrian Huang <ahuang12@lenovo.com>
>> Tested-by: Adam Pelka <adampelka26@gmail.com>
>> ---
>>  drivers/iommu/amd/amd_iommu_types.h | 12 +++++++-----
>>  drivers/iommu/amd/init.c            | 26 +++++++++-----------------
>>  drivers/iommu/amd/iommu.c           | 21 +++++++++++++++++++--
>>  3 files changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index 678ceddd711c..bcb88b476167 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -368,9 +368,11 @@
>>  /* DTE[128:179] | DTE[184:191] */
>>  #define DTE_DATA2_INTR_MASK    ~GENMASK_ULL(55, 52)
>>
>> -#define IOMMU_PROT_MASK 0x03
>> -#define IOMMU_PROT_IR 0x01
>> -#define IOMMU_PROT_IW 0x02
>> +#define IVMD_FLAG_UNITY                BIT(0> +#define IVMD_FLAG_IR           BIT(1)
>> +#define IVMD_FLAG_IW           BIT(2)
>> +#define IVMD_FLAG_EXCL         BIT(3)
>> +#define IVMD_FLAG_MASK         GENMASK_U32(3, 0)
>>
>>  /* IOMMU capabilities */
>>  #define IOMMU_CAP_IOTLB   24
>> @@ -880,8 +882,8 @@ struct unity_map_entry {
>>         /* end address to unity map (including) */
>>         u64 address_end;
>>
>> -       /* required protection */
>> -       int prot;
>> +       /* IVMD flags for the entry */
>> +       u8 flags;
>>  };
>>
>>  /*
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index 13c96feed64f..1f0ca675df1c 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -70,11 +70,6 @@
>>  #define IVHD_FLAG_RESPASSPW_EN_MASK     0x04
>>  #define IVHD_FLAG_ISOC_EN_MASK          0x08
>>
>> -#define IVMD_FLAG_EXCL_RANGE            0x08
>> -#define IVMD_FLAG_IW                    0x04
>> -#define IVMD_FLAG_IR                    0x02
>> -#define IVMD_FLAG_UNITY_MAP             0x01
>> -
>>  #define ACPI_DEVFLAG_INITPASS           0x01
>>  #define ACPI_DEVFLAG_EXTINT             0x02
>>  #define ACPI_DEVFLAG_NMI                0x04
>> @@ -2636,6 +2631,13 @@ static int __init init_unity_map_range(struct ivmd_header *m,
>>         struct amd_iommu_pci_seg *pci_seg;
>>         char *s;
>>
>> +       if ((m->flags & IVMD_FLAG_UNITY) &&
>> +           !(m->flags & (IVMD_FLAG_IR | IVMD_FLAG_IW | IVMD_FLAG_EXCL))) {
>> +               pr_err(FW_BUG "IVMD type %#x devid %#x range %#llx+%#llx: Unity set without IR/IW, ignoring\n",
>> +                      m->type, m->devid, m->range_start, m->range_length);
>> +               return -EINVAL;
>> +       }
>> +
>>         pci_seg = get_pci_segment(m->pci_seg, ivrs_base);
>>         if (pci_seg == NULL)
>>                 return -ENOMEM;
>> @@ -2665,17 +2667,7 @@ static int __init init_unity_map_range(struct ivmd_header *m,
>>         }
>>         e->address_start = PAGE_ALIGN(m->range_start);
>>         e->address_end = e->address_start + PAGE_ALIGN(m->range_length);
>> -       e->prot = m->flags >> 1;
>> -
>> -       /*
>> -        * Treat per-device exclusion ranges as r/w unity-mapped regions
>> -        * since some buggy BIOSes might lead to the overwritten exclusion
>> -        * range (exclusion_start and exclusion_length members). This
>> -        * happens when there are multiple exclusion ranges (IVMD entries)
>> -        * defined in ACPI table.
>> -        */
>> -       if (m->flags & IVMD_FLAG_EXCL_RANGE)
>> -               e->prot = (IVMD_FLAG_IW | IVMD_FLAG_IR) >> 1;
>> +       e->flags = m->flags;
>>
>>         DUMP_printk("%s devid_start: %04x:%02x:%02x.%x devid_end: "
>>                     "%04x:%02x:%02x.%x range_start: %016llx range_end: %016llx"
>> @@ -2702,7 +2694,7 @@ static int __init init_memory_definitions(struct acpi_table_header *table)
>>
>>         while (p < end) {
>>                 m = (struct ivmd_header *)p;
>> -               if (m->flags & (IVMD_FLAG_UNITY_MAP | IVMD_FLAG_EXCL_RANGE))
>> +               if (m->flags & (IVMD_FLAG_UNITY | IVMD_FLAG_EXCL))
> the original names were more descriptive, suggest keeping
> IVMD_FLAG_UNITY_MAP/IVMD_FLAG_EXCL_RANGE rather than shortening them.
> if you insist on rename, may be send in seperate patch?
> 
>>                         init_unity_map_range(m, table);
>>
>>                 p += m->length;
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 56262f6b1f70..699b5f530f11 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -3120,12 +3120,29 @@ static void amd_iommu_get_resv_regions(struct device *dev,
>>                 if (devid < entry->devid_start || devid > entry->devid_end)
>>                         continue;
>>
>> +               /*
>> +                * IVMD_FLAG_UNITY, IVMD_FLAG_IR, IVMD_FLAG_IW are ignored if
>> +                * IVMD_FLAG_EXCL is set.
>> +                */
>> +               if ((entry->flags & IVMD_FLAG_EXCL) &&
>> +                   (entry->flags & (IVMD_FLAG_UNITY | IVMD_FLAG_IR | IVMD_FLAG_IW)))
> Here EXCL & UNITY are flagged as invalid combination
> Above UNITY & !EXCL are also flagged as invalid.
> Suggesting revisiting these conditions.
>> +                       pr_err(FW_BUG "%s: Invalid IVMD flags combination: %#02x for sbdf=%#x. Ignore IW/IR/Unity flags\n",
>> +                              __func__, entry->flags, sbdf);
>> +
>> +               if (entry->flags & ~IVMD_FLAG_MASK)
>> +                       pr_err("%s: Unknown flags: %#02x for sbdf=%#x\n",
>> +                              __func__, entry->flags & ~IVMD_FLAG_MASK, sbdf);
>> +
>>                 type   = IOMMU_RESV_DIRECT;
>>                 length = entry->address_end - entry->address_start;
>> -               if (entry->prot & IOMMU_PROT_IR)
>> +               if (entry->flags & IVMD_FLAG_IR)
>>                         prot |= IOMMU_READ;
>> -               if (entry->prot & IOMMU_PROT_IW)
>> +               if (entry->flags & IVMD_FLAG_IW)
>>                         prot |= IOMMU_WRITE;
>> +               if (entry->flags & IVMD_FLAG_EXCL) {
>> +                       type = IOMMU_RESV_RESERVED;
>> +                       prot = 0;
>> +               }
> Since in case of invalid flag combinations, we are going ahead with region allocation,
> will it be beneficial to add a pr_debug emitting relevant details passed on to the
> iommu_alloc_resv_region()?

Sorry I made a mistake here,

Here's summary of what I wanted to do,
1. Reject UNITY mapping if IR and IW flags are not present
2. Reject exclusion mapping if either UNITY / IR / IW flags are present

Thanks for pointing it out. I'll update it in the next version and move
all the firmware related checks inside init_unity_map_range(). I'll also
reject this condition as it does not make sense to add the region with
invalid flag combination (a region cannot be both UNITY and EXCL).

Thanks
Sairaj

>>
>>                 region = iommu_alloc_resv_region(entry->address_start,
>>                                                  length, prot, type,
>> --
>> 2.34.1
>>
>>