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] [day] [month] [year] [list]

Message-ID: <5a0fcde1-d746-4663-bb9b-4eeb3c9036c4@amd.com>
Date: Wed, 27 Aug 2025 19:36:54 -0500
From: "Suthikulpanit, Suravee" <suravee.suthikulpanit@....com>
To: Nicolin Chen <nicolinc@...dia.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 8/22/2025 3:20 PM, Nicolin Chen wrote:
> 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.

right

>> @@ -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".

right.

>> +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)?

correct, I should have just checked for 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"?

correct.

Thanks for the review. I'll clean up the logic in 
amd_iommu_nested_attach_device() to return error early for non-pci device.

Thanks,
Suravee