Re: [PATCH v2 10/12] iommu/amd: Add support for nested domain allocation

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Oct 01, 2025 at 06:09:52AM +0000, Suravee Suthikulpanit wrote:
> The nested domain is allocated with IOMMU_DOMAIN_NESTED type to store
> stage-1 translation (i.e. GVA->GPA). This includes the GCR3 root pointer
> table along with guest page tables. The struct iommu_hwpt_amd_guest
> contains this information, and is passed from user-space as a parameter
> of the struct iommu_ops.domain_alloc_nested().
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index d533bb8851ea..cc1f14899dfe 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -8,6 +8,7 @@
>  #define AMD_IOMMU_H
>  
>  #include <linux/iommu.h>
> +#include <uapi/linux/iommufd.h>
>  
>  #include "amd_iommu_types.h"

amd_iommu_types.h is adding the uAPI header in this patch too. So,
this new "include" here is redundant.

> +++ b/drivers/iommu/amd/nested.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#define pr_fmt(fmt)     "AMD-Vi: " fmt
> +#define dev_fmt(fmt)    pr_fmt(fmt)

#define pr_fmt(fmt)     "AMD-Vi: " fmt
#define dev_fmt		pr_fmt

> +
> +#include <linux/iommu.h>
> +
> +#include "amd_iommu.h"
> +#include "amd_iommu_types.h"

amd_iommu.h covers both linux/iommu.h and amd_iommu_types.h.

> +struct iommu_domain *
> +amd_iommu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
> +			      const struct iommu_user_data *user_data)
> +{
> +	int ret;
> +	struct iommu_hwpt_amd_guest gdte;
> +	struct nested_domain *ndom;
> +
> +	if (user_data->type != IOMMU_HWPT_DATA_AMD_GUEST)
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	/*
> +	 * Need to make sure size of struct iommu_hwpt_amd_guest and
> +	 * struct dev_table_entry are the same since it will be copied
> +	 * from one to the other later on.
> +	 */
> +	if (WARN_ON(sizeof(struct dev_table_entry) != sizeof(gdte)))
> +		return ERR_PTR(-EINVAL);

	static_assert(sizeof(struct dev_table_entry) == sizeof(gdte));

> +
> +	ret = iommu_copy_struct_from_user(&gdte, user_data,
> +					  IOMMU_HWPT_DATA_AMD_GUEST,
> +					  dte);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ndom = kzalloc(sizeof(*ndom), GFP_KERNEL);
> +	if (IS_ERR(ndom))
> +		return ERR_PTR(-ENOMEM);

	if (!ndom)
		return ERR_PTR(-ENOMEM);

> +
> +	ndom->domain.ops = &nested_domain_ops;
> +	ndom->domain.type = IOMMU_DOMAIN_NESTED;
> +	memcpy(&ndom->guest_dte, &gdte, sizeof(struct dev_table_entry));
> +
> +	/* Due to possible aliasing issue use per-device nested domain ID */
> +	ndom->id = amd_iommu_pdom_id_alloc();
> +	if (ndom->id <= 0) {
> +		ret = ndom->id;
> +		goto out_err;

if ndom->id = 0, ret = 0, meaning this function does not fail..

Given that ida_alloc_range has the range [1, MAX_DOMAIN_ID - 1],
0 should never be returned. So just check if (ndom->id < 0) will
be enough.

Or you could override the "ret" entirely like other places do:

	if (ndom->id <= 0) {
		ret = -ENOSPC;
		goto out_err;
	}

With all these fixed,

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>