linux-kernel - Re: [PATCH 8/8] iommu/amd: Add support for nested domain attach/detach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aKjRB5MqCrJ2Px7G@Asurada-Nvidia>
Date: Fri, 22 Aug 2025 13:20:23 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
CC: <jgg@...dia.com>, <linux-kernel@...r.kernel.org>, <robin.murphy@....com>,
	<will@...nel.org>, <joro@...tes.org>, <kevin.tian@...el.com>,
	<jsnitsel@...hat.com>, <vasant.hegde@....com>, <iommu@...ts.linux.dev>,
	<santosh.shukla@....com>, <sairaj.arunkodilkar@....com>, <jon.grimm@....com>,
	<prashanthpra@...gle.com>, <wvw@...gle.com>, <wnliu@...gle.com>,
	<gptran@...gle.com>, <kpsingh@...gle.com>
Subject: Re: [PATCH 8/8] iommu/amd: Add support for nested domain
 attach/detach

On Wed, Aug 20, 2025 at 11:30:09AM +0000, Suravee Suthikulpanit wrote:
> +static inline bool has_gcr3_table(struct gcr3_tbl_info *gcr3_info)
> +{
> +	if (!gcr3_info || (!gcr3_info->gcr3_tbl && !gcr3_info->trp_gpa))
> +		return false;

"gcr3_info" seems always pointing to "&dev_data->gcr3_info", which
can never be NULL.

> @@ -2061,7 +2087,14 @@ static void set_dte_entry(struct amd_iommu *iommu,
>  	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>  	struct dev_table_entry *dte = &get_dev_table(iommu)[dev_data->devid];
>  
> -	if (gcr3_info && gcr3_info->gcr3_tbl)
> +	/*
> +	 * For nested domain, use parent domain to setup v1 table
> +	 * information and domain id.
> +	 */
> +	if (amd_iommu_domain_is_nested(domain))
> +		domain = domain->parent;
> +
> +	if (has_gcr3_table(gcr3_info))
>  		domid = dev_data->gcr3_info.domid;

There is already a local variable "gcr3_info".

> +static int nested_gcr3_update(struct protection_domain *pdom, struct device *dev)
> +{
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	struct iommu_hwpt_amd_v2 *hwpt = &pdom->guest_hwpt;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (!pdev || !hwpt)
> +		return -EINVAL;

to_pci_dev is a container_of from the dev. !pdev indicates a !dev
that should never happen in the path of an attach_dev op. Or, did
you actually want to check if dev_is_pci(dev)?

Also, hwpt is "&pdom->guest_hwpt", which would never be NULL.

> +static int amd_iommu_nested_attach_device(struct iommu_domain *dom, struct device *dev)
> +{
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	struct protection_domain *pdom = to_pdomain(dom);
> +	struct pci_dev *pdev;
> +	int ret;
> +
> +	if (dev_data->domain == pdom)
> +		return 0;
> +
> +	ret = nested_gcr3_update(pdom, dev);
> +	if (ret)
> +		return ret;
> +
> +	if (dev_data->domain)
> +		amd_iommu_detach_device(dev);
> +
> +	ret = __amd_iommu_attach_device(dev, pdom);
> +	if (ret)
> +		return ret;
> +
> +	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
> +	if (pdev)
> +		amd_iommu_pdev_enable_cap_ats(pdev);

Is "dev_data->dev" expected to be "dev"?

Nicolin