|SUCCESS| [v2] bus/uacce: support no-iommu mode

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-18

Review of patch: bus/uacce: support no-iommu mode

Commit: fbb54ba3c3c2 (reconstructed from patch context)
Author: Chengwen Feng <fengchengwen at huawei.com>
Subject: [PATCH v2] bus/uacce: support no-iommu mode

This commit extends the uacce bus driver to support no-iommu mode for
devices without or not enabling IOMMU/SVA capabilities, and 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..d2ff8a2d53 100644
> --- a/drivers/bus/uacce/bus_uacce_driver.h
> +++ b/drivers/bus/uacce/bus_uacce_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)

These flags are newly introduced in the public header but seem to be
used by device driver implementations rather than applications. Can you
clarify if these belong in the public API or should be in an internal
header? The flag values themselves appear correct.

[ ... ]

> diff --git a/drivers/bus/uacce/uacce.c b/drivers/bus/uacce/uacce.c
> index ade2452ad5..70fb5c730a 100644
> --- a/drivers/bus/uacce/uacce.c
> +++ b/drivers/bus/uacce/uacce.c

[ ... ]

> @@ -151,9 +147,25 @@ 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_SUFFIX		"_noiommu"
> +	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;
> +
> +	/*
> +	 * If the device is in no-iommu mode, the UACCE_DEV_FLAG_NOIOMMU flag
> +	 * in its "flags" will be set, and its "api" will contain the suffix
> +	 * _noiommu. To facilitate matching with the driver, the suffix is
> +	 * removed here.
> +	 */
> +	api_len = strlen(dev->api);
> +	sub_len = strlen(NOIOMMU_SUFFIX);
> +	if (api_len > sub_len && strcmp(dev->api + api_len - sub_len, NOIOMMU_SUFFIX) == 0)
> +		dev->api[api_len - sub_len] = 0;
> +
>  	return 0;
>  }

Does this code handle the case where the api buffer contains no NUL
terminator after uacce_read_attr()? The strlen() call could read past
the buffer if uacce_read_attr() fills the entire buffer without a
terminating NUL byte. Can you verify that dev->api is always properly
NUL-terminated by uacce_read_attr()?

The suffix removal logic in uacce_read_api() appears correct, but the
pointer arithmetic dev->api + api_len - sub_len could be clearer if
written as &dev->api[api_len - sub_len].

[ ... ]

> @@ -208,8 +208,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. */
>  	}

The wording "don't support" should be "doesn't support" for correct
grammar in the log message.

[ ... ]

> @@ -333,11 +345,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 drv_support_noiommu = !!(dr->drv_flags & RTE_UACCE_DRV_SUPPORT_NOIOMMU_MODE);
> +	bool dev_in_noiommu = !!(dev->flags & UACCE_DEV_FLAG_NOIOMMU);
>  	uint32_t api_ver = uacce_calc_api_ver(dev->api, NULL);
>  	const struct rte_uacce_id *id_table;
>  	const char *map;
>  	uint32_t len;
>  
> +	if (dev_in_noiommu && !drv_support_noiommu)
> +		return false;

Should this condition also emit a log message explaining why the device
was not matched? Currently it silently returns false which could make
debugging difficult. The existing logic in uacce_verify() logs a warning
when devices are skipped.