[PATCH V1] accel/amdxdna: Block running when IOMMU is off

[PATCH V1] accel/amdxdna: Block running when IOMMU is off

[Lizhi Hou]

The AIE2 device firmware requires IOMMU on.

Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5319
Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/aie2_pci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
index c4d345d4c76b..22f66c7f534d 100644
--- a/drivers/accel/amdxdna/aie2_pci.c
+++ b/drivers/accel/amdxdna/aie2_pci.c
@@ -490,6 +490,11 @@ static int aie2_init(struct amdxdna_dev *xdna)
 		return -EINVAL;
 	}
 
+	if (!xdna->group) {
+		XDNA_ERR(xdna, "Running without IOMMU not supported");
+		return -EINVAL;
+	}
+
 	ndev = drmm_kzalloc(&xdna->ddev, sizeof(*ndev), GFP_KERNEL);
 	if (!ndev)
 		return -ENOMEM;
-- 
2.34.1

Re: [PATCH V1] accel/amdxdna: Block running when IOMMU is off

[Mario Limonciello]

On 5/20/26 17:35, Lizhi Hou wrote:
> The AIE2 device firmware requires IOMMU on.
> 
> Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5319
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> ---
>   drivers/accel/amdxdna/aie2_pci.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
> index c4d345d4c76b..22f66c7f534d 100644
> --- a/drivers/accel/amdxdna/aie2_pci.c
> +++ b/drivers/accel/amdxdna/aie2_pci.c
> @@ -490,6 +490,11 @@ static int aie2_init(struct amdxdna_dev *xdna)
>   		return -EINVAL;
>   	}
>   
> +	if (!xdna->group) {
> +		XDNA_ERR(xdna, "Running without IOMMU not supported");
> +		return -EINVAL;
> +	}
> +
>   	ndev = drmm_kzalloc(&xdna->ddev, sizeof(*ndev), GFP_KERNEL);
>   	if (!ndev)
>   		return -ENOMEM;

I think it would be better to change the return for amdxdna_iommu_init().

This has a check that lets !xdna->group pass; which can be turned into 
an error code instead.

Re: [PATCH V1] accel/amdxdna: Block running when IOMMU is off

[Lizhi Hou]

On 5/20/26 17:57, Mario Limonciello wrote:
>
>
> On 5/20/26 17:35, Lizhi Hou wrote:
>> The AIE2 device firmware requires IOMMU on.
>>
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5319
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>> ---
>>   drivers/accel/amdxdna/aie2_pci.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/accel/amdxdna/aie2_pci.c 
>> b/drivers/accel/amdxdna/aie2_pci.c
>> index c4d345d4c76b..22f66c7f534d 100644
>> --- a/drivers/accel/amdxdna/aie2_pci.c
>> +++ b/drivers/accel/amdxdna/aie2_pci.c
>> @@ -490,6 +490,11 @@ static int aie2_init(struct amdxdna_dev *xdna)
>>           return -EINVAL;
>>       }
>>   +    if (!xdna->group) {
>> +        XDNA_ERR(xdna, "Running without IOMMU not supported");
>> +        return -EINVAL;
>> +    }
>> +
>>       ndev = drmm_kzalloc(&xdna->ddev, sizeof(*ndev), GFP_KERNEL);
>>       if (!ndev)
>>           return -ENOMEM;
>
> I think it would be better to change the return for amdxdna_iommu_init().

amdxdna_iommu_init() is common code for all kinds of hardware. I would 
keep the check only for AIE2 now, which is known to require IOMMU on.


Lizhi

>
> This has a check that lets !xdna->group pass; which can be turned into 
> an error code instead.

Re: [PATCH V1] accel/amdxdna: Block running when IOMMU is off

[Mario Limonciello]

On 5/20/26 22:43, Lizhi Hou wrote:
> 
> On 5/20/26 17:57, Mario Limonciello wrote:
>>
>>
>> On 5/20/26 17:35, Lizhi Hou wrote:
>>> The AIE2 device firmware requires IOMMU on.
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5319
>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>>> ---
>>>   drivers/accel/amdxdna/aie2_pci.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/ 
>>> amdxdna/aie2_pci.c
>>> index c4d345d4c76b..22f66c7f534d 100644
>>> --- a/drivers/accel/amdxdna/aie2_pci.c
>>> +++ b/drivers/accel/amdxdna/aie2_pci.c
>>> @@ -490,6 +490,11 @@ static int aie2_init(struct amdxdna_dev *xdna)
>>>           return -EINVAL;
>>>       }
>>>   +    if (!xdna->group) {
>>> +        XDNA_ERR(xdna, "Running without IOMMU not supported");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>       ndev = drmm_kzalloc(&xdna->ddev, sizeof(*ndev), GFP_KERNEL);
>>>       if (!ndev)
>>>           return -ENOMEM;
>>
>> I think it would be better to change the return for amdxdna_iommu_init().
> 
> amdxdna_iommu_init() is common code for all kinds of hardware. I would 
> keep the check only for AIE2 now, which is known to require IOMMU on.
Got it.  Then this makes sense.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> 
> Lizhi
> 
>>
>> This has a check that lets !xdna->group pass; which can be turned into 
>> an error code instead.

Re: [PATCH V1] accel/amdxdna: Block running when IOMMU is off

[Lizhi Hou]

Applied to drm-misc-fixes

On 5/20/26 21:32, Mario Limonciello wrote:
>
>
> On 5/20/26 22:43, Lizhi Hou wrote:
>>
>> On 5/20/26 17:57, Mario Limonciello wrote:
>>>
>>>
>>> On 5/20/26 17:35, Lizhi Hou wrote:
>>>> The AIE2 device firmware requires IOMMU on.
>>>>
>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/work_items/5319
>>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>>>> ---
>>>>   drivers/accel/amdxdna/aie2_pci.c | 5 +++++
>>>>   1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/ 
>>>> amdxdna/aie2_pci.c
>>>> index c4d345d4c76b..22f66c7f534d 100644
>>>> --- a/drivers/accel/amdxdna/aie2_pci.c
>>>> +++ b/drivers/accel/amdxdna/aie2_pci.c
>>>> @@ -490,6 +490,11 @@ static int aie2_init(struct amdxdna_dev *xdna)
>>>>           return -EINVAL;
>>>>       }
>>>>   +    if (!xdna->group) {
>>>> +        XDNA_ERR(xdna, "Running without IOMMU not supported");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>>       ndev = drmm_kzalloc(&xdna->ddev, sizeof(*ndev), GFP_KERNEL);
>>>>       if (!ndev)
>>>>           return -ENOMEM;
>>>
>>> I think it would be better to change the return for 
>>> amdxdna_iommu_init().
>>
>> amdxdna_iommu_init() is common code for all kinds of hardware. I 
>> would keep the check only for AIE2 now, which is known to require 
>> IOMMU on.
> Got it.  Then this makes sense.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>>
>> Lizhi
>>
>>>
>>> This has a check that lets !xdna->group pass; which can be turned 
>>> into an error code instead.
>

Claude review: accel/amdxdna: Block running when IOMMU is off

[Claude Code Review Bot]

Overall Series Review

Subject: accel/amdxdna: Block running when IOMMU is off
Author: Lizhi Hou <lizhi.hou@amd.com>
Patches: 5
Reviewed: 2026-05-25T21:10:20.417579

---

This is a single patch that adds an IOMMU presence check during AIE2 device initialization. The intent is clear and the fix addresses a real requirement — the AIE2 firmware needs IOMMU to be enabled. The patch is small and focused. However, there is a question about whether this is the right place for the check, and the error return code could be more specific.

**Verdict:** Reasonable patch with minor suggestions.

---
Generated by Claude Code Patch Reviewer

Claude review: accel/amdxdna: Block running when IOMMU is off

[Claude Code Review Bot]

Patch Review

**Correctness of the check:** The patch checks `!xdna->group` in `aie2_init()`. Looking at `amdxdna_iommu_init()` in `amdxdna_iommu.c:161`, `xdna->group` is set via `iommu_group_get()` — it will be NULL when no IOMMU is present. The `amdxdna_iommu_init()` call happens at `amdxdna_pci_drv.c:374` **before** `xdna->dev_info->ops->init(xdna)` at line 385, which dispatches to `aie2_init()`. So by the time `aie2_init()` runs, `xdna->group` is already populated (or NULL). The check is valid.

**Placement concern:** The check is placed inside the AIE2-specific `aie2_init()`, which means it only protects AIE2 devices. That seems intentional per the commit message ("The AIE2 device firmware requires IOMMU on"), and I confirmed that `aie4_pci.c` has no similar check and no hypervisor check either, so this is AIE2-specific policy. That's fine.

**Return code:** The patch returns `-EINVAL`, but `-ENODEV` or `-EOPNOTSUPP` might be more semantically appropriate — the hardware is present but lacks a required system feature (IOMMU). `-EINVAL` typically means "bad argument" which doesn't describe the situation well. That said, the existing hypervisor check on the line just above also returns `-EINVAL`, so at least it's consistent within the function.

**Context mismatch:** The patch's diff context shows the new check being added *after* the hypervisor check and *before* the `ndev = drmm_kzalloc(...)` allocation. But in the drm-next tree, the `ndev` allocation is already at line 493, immediately after the hypervisor check with no blank line gap for the patch's `@@ -490,6 +490,11 @@` hunk to land cleanly. This explains why the patches didn't apply — the base the patch was developed against differs from drm-next.

**Error message:** The message `"Running without IOMMU not supported"` is adequate but could be slightly more informative, e.g., `"AIE2 firmware requires IOMMU to be enabled"` to help users understand what to fix.

**Minor style observation:** Checking `xdna->group` as a proxy for "IOMMU is available" works because `iommu_group_get()` returns NULL when there's no IOMMU. However, this is an indirect test — it relies on the fact that `amdxdna_iommu_init()` always runs first and sets `xdna->group`. If that initialization order ever changes, this check would silently break. A brief comment could help, but the ordering is well-established in `amdxdna_pci_drv.c` so this is a very low risk.

**Overall:** This is a straightforward, low-risk patch. The main suggestion would be to consider `-ENODEV` or `-EOPNOTSUPP` instead of `-EINVAL`, though consistency with the adjacent hypervisor check is a reasonable argument for keeping it. No bugs found.

---
Generated by Claude Code Patch Reviewer