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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Aug 20, 2025 at 11:30:09AM +0000, Suravee Suthikulpanit wrote:
> @@ -2023,17 +2035,31 @@ static void set_dte_gcr3_table(struct amd_iommu *iommu,
>  			       struct dev_table_entry *target)
>  {
>  	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> +	struct protection_domain *pdom = dev_data->domain;
>  	u64 gcr3;
>  
> -	if (!gcr3_info->gcr3_tbl)
> +	if (!has_gcr3_table(gcr3_info))
>  		return;
>  
> -	pr_debug("%s: devid=%#x, glx=%#x, giov=%#x, gcr3_tbl=%#llx\n",
> +	/* We need to check host capability before setting the mode.  */
> +	if ((pdom->guest_paging_mode == PAGE_MODE_5_LEVEL) &&
> +	    (amd_iommu_gpt_level < PAGE_MODE_5_LEVEL)) {
> +		pr_err("Cannot support Guest paging mode=%#x (dom_id=%#x).\n",
> +		       pdom->guest_paging_mode, pdom->id);

Should be checked during allocation time

And again please don't mess up this function with nested DTEs.

The vDTE should be validated during creation or fail creation. I see
this is missing validation, every single bit in the vDTE needs to be
checked to be 0 or supported by the kernel.

The logic should simply take the vDTE and merge it with the host DTE
as a simple bitwise operation.

This is why I keep saying to fix the flow here so this can be written
properly, and don't mess with the gcr3_info.

> @@ -2293,7 +2326,8 @@ int __amd_iommu_attach_device(struct device *dev, struct protection_domain *doma
>  		goto out;
>  
>  	/* Setup GCR3 table */
> -	if (pdom_is_sva_capable(domain)) {
> +	if (!amd_iommu_domain_is_nested(domain) && pdom_is_sva_capable(domain)) {
> +		pr_warn("%s: Allocating guest page table\n", __func__);

??

> -const struct iommu_domain_ops nested_domain_ops = {
> -	.free = amd_iommu_domain_free,
> -};
> +const struct iommu_domain_ops nested_domain_ops;

Put stuff where it belongs when first adding it..

> +static inline u64 hwpt_to_gcr3_trp(u64 *dte)
> +{
> +	u64 gcr3;
> +
> +	gcr3  = (FIELD_GET(DTE_GCR3_14_12, dte[0]) << 12);
> +	gcr3 |= (FIELD_GET(DTE_GCR3_30_15, dte[1]) << 15);
> +	gcr3 |= (FIELD_GET(DTE_GCR3_51_31, dte[1]) << 31);
> +	return gcr3;
> +}
> +
> +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;
> +
> +	/* Note: Currently only support GCR3TRPMode with nested translation */
> +	if (!check_feature2(FEATURE_GCR3TRPMODE))
> +		return -EOPNOTSUPP;
> +
> +	if (FIELD_GET(DTE_GPT_LEVEL_MASK, hwpt->dte[2]) == GUEST_PGTABLE_5_LEVEL)
> +		pdom->guest_paging_mode = PAGE_MODE_5_LEVEL;
> +	else
> +		pdom->guest_paging_mode = PAGE_MODE_4_LEVEL;
> +
> +	dev_data->ppr = FIELD_GET(DTE_FLAG_PPR, hwpt->dte[0]);
> +	dev_data->gcr3_info.glx = FIELD_GET(DTE_FLAG_GLX, hwpt->dte[0]);
> +	dev_data->gcr3_info.giov = FIELD_GET(DTE_FLAG_GIOV, hwpt->dte[0]);
> +	dev_data->gcr3_info.trp_gpa = hwpt_to_gcr3_trp(hwpt->dte);
> +	/* Due to possible aliasing issue use nested domain ID */
> +	dev_data->gcr3_info.domid = pdom->id;
> +	pr_debug("%s: devid=%#x, domid=%#x, trp_gpa=%#llx, glx=%#x\n", __func__,
> +		 pci_dev_id(pdev),
> +		 dev_data->gcr3_info.domid,
> +		 dev_data->gcr3_info.trp_gpa,
> +		 dev_data->gcr3_info.glx);
> +
> +	return 0;
> +}

None of this logic is needed if the vDTE is treated bitwise.

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

I'm strongly against not supporting hitless vDTE update - this is part
of the HW spec, the VMM should implement it, not create problematic
bugs to deal with down the road. Everytime we let the VMM deviate from
the HW spec in undiscoverable ways it causes problems :(

Meaning you can't call detach_device, you have to support hitless
update of the DTE between different attachment types. You already did
the hard work of making update_dte256(), but the surrounding flows
still need fixing.

Jason