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

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

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