|SUCCESS| bus/uacce: support no-iommu mode

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163226

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-22

Reviewing patch for UACCE bus no-iommu mode support.

commit 20260422075326.34206-1-fengchengwen at huawei.com
Author: Chengwen Feng <fengchengwen at huawei.com>
Subject: [PATCH] bus/uacce: support no-iommu mode

This patch extends the uacce bus to support no-iommu mode for devices
without or not enabling IOMMU/SVA. It also fixes a typo in the flag name
UACCE_DEV_FLGA_SVA -> UACCE_DEV_FLAG_SVA.

> diff --git a/drivers/bus/uacce/bus_uacce_driver.h b/drivers/bus/uacce/bus_uacce_driver.h
> index c7445778a6..7ff9c7b483 100644
> --- a/drivers/bus/uacce/bus_uacce_driver.h
> +++ b/drivers/bus/uacce/bus_uacce_driver.h
> @@ -15,6 +15,7 @@
>  #include <stdlib.h>
>  #include <linux/types.h>
>  
> +#include <rte_bitops.h>
>  #include <rte_compat.h>
>  #include <rte_devargs.h>
>  #include <dev_driver.h>
> @@ -28,6 +29,11 @@ extern "C" {
>  #define RTE_UACCE_ALGS_NAME_SIZE	384
>  #define RTE_UACCE_ATTR_MAX_SIZE		384
>  
> +/* UACCE device flag of SVA. */
> +#define UACCE_DEV_FLAG_SVA	RTE_BIT32(0)
> +/* UACCE device flag of NOIOMMU */
> +#define UACCE_DEV_FLAG_NOIOMMU	RTE_BIT32(1)

The comment style is inconsistent. The SVA flag comment ends with a
period while the NOIOMMU comment does not. Following DPDK coding style,
both single-line comments should be consistent. Should both comments end
with periods?

[ ... ]

> diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
> index ade2452ad5..2dab3e1c4d 100644
> --- a/drivers/bus/uacce/uacce.c
> +++ b/drivers/bus/uacce/uacce.c
> @@ -15,7 +15,6 @@
>  #include <sys/types.h>
>  
>  #include <eal_export.h>
> -#include <rte_bitops.h>
>  #include <rte_common.h>

[ ... ]

> @@ -151,9 +147,19 @@ uacce_read_attr_u32(const char *dev_root, const char *attr, uint32_t *val)
>  static int
>  uacce_read_api(struct rte_uacce_device *dev)
>  {
> -	int ret = uacce_read_attr(dev->dev_root, "api", dev->api, sizeof(dev->api) - 1);
> +#define NOIOMMU_SUBFIX		"_noiommu"

There is a typo in the macro name. Should this be NOIOMMU_SUFFIX instead
of NOIOMMU_SUBFIX?

> +	size_t api_len, sub_len;
> +	int ret;
> +
> +	ret = uacce_read_attr(dev->dev_root, "api", dev->api, sizeof(dev->api) - 1);
>  	if (ret < 0)
>  		return ret;
> +
> +	api_len = strlen(dev->api);
> +	sub_len = strlen(NOIOMMU_SUBFIX);
> +	if (api_len > sub_len && strcmp(dev->api + api_len - sub_len, NOIOMMU_SUBFIX) == 0)
> +		dev->api[api_len - sub_len] = 0;

Following DPDK coding style, explicit comparisons should be used for
boolean conditions. Should the strcmp() call be compared explicitly
against 0 like: strcmp(...) == 0?

Actually, looking more carefully, the strcmp() is already compared
against 0. However, the code modifies dev->api by setting a null
terminator to strip the suffix. Does this operation correctly preserve
the api string for later use? Also, is there any documentation or comment
explaining why this suffix stripping is needed?

Also, does this code set the UACCE_DEV_FLAG_NOIOMMU flag in dev->flags
anywhere? The commit message mentions "The device flags bit1 was set
(UACCE_DEV_FLAG_NOIOMMU)" but I don't see where in uacce_read_api() or
its callers this flag gets set when the suffix is detected.

> +
>  	return 0;
>  }
>  
> @@ -196,8 +202,8 @@ uacce_read_qfrt_sz(struct rte_uacce_device *dev)
>  static int
>  uacce_verify(struct rte_uacce_device *dev)
>  {
> -	if (!(dev->flags & UACCE_DEV_FLGA_SVA)) {
> -		UACCE_BUS_WARN("device %s don't support SVA, skip it!", dev->name);
> +	if (!(dev->flags & (UACCE_DEV_FLAG_SVA | UACCE_DEV_FLAG_NOIOMMU))) {
> +		UACCE_BUS_WARN("device %s don't support SVA or NOIOMMU, skip it!", dev->name);
>  		return 1; /* >0 will skip this device. */
>  	}

[ ... ]

> @@ -333,12 +339,16 @@ static bool
>  uacce_match(const struct rte_uacce_driver *dr, struct rte_uacce_device *dev)
>  {
>  	bool forward_compat = !!(dr->drv_flags & RTE_UACCE_DRV_FORWARD_COMPATIBILITY_DEV);
> +	bool support_noiommu = !!(dr->drv_flags & RTE_UACCE_DRV_SUPPORT_NOIOMMU_MODE);
>  	uint32_t api_ver = uacce_calc_api_ver(dev->api, NULL);
>  	const struct rte_uacce_id *id_table;
>  	const char *map;
>  	uint32_t len;
>  
>  	for (id_table = dr->id_table; id_table->dev_api != NULL; id_table++) {
> +		if ((dev->flags & UACCE_DEV_FLAG_NOIOMMU) && !support_noiommu)
> +			continue;

Following DPDK coding style guidelines, should the bitwise AND operation
be compared explicitly? Like: if ((dev->flags & UACCE_DEV_FLAG_NOIOMMU)
!= 0 && support_noiommu == false)?

Also, there seems to be a logic issue here. When a device has the
NOIOMMU flag set but the driver doesn't support it, the code skips to
the next id_table entry. But does this correctly handle the case where
multiple id_table entries exist? Should the entire device be rejected
rather than just continuing to the next table entry?

> +
>  		if (!uacce_match_api(dev, forward_compat, id_table))
>  			continue;

Summary: The main concerns are:
1. The typo NOIOMMU_SUBFIX should likely be NOIOMMU_SUFFIX
2. Missing code to set UACCE_DEV_FLAG_NOIOMMU when the suffix is detected
3. Inconsistent comment punctuation style
4. Potential logic issue with driver matching when NOIOMMU is unsupported