Re: [PATCH v2 11/12] iommu/amd: Add support for nested domain attach/detach

[Suthikulpanit, Suravee]

On 10/6/2025 9:59 AM, Jason Gunthorpe wrote:
> On Wed, Oct 01, 2025 at 06:09:53AM +0000, Suravee Suthikulpanit wrote:
> > ....
> > +	if (WARN_ON(!ndom || !pdom || (pdom->iop.mode == PAGE_MODE_NONE)))
> > +		return;
> > +
> > +	amd_iommu_make_clear_dte(dev_data, &new);
> > +
> > +	new.data[0] |= iommu_virt_to_phys(pdom->iop.root);
> > +	new.data[0] |= FIELD_PREP(DTE_MODE_MASK, pdom->iop.mode);
> > +	new.data[0] |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_TV;
> > +	new.data[0] |= (DTE_FLAG_PPR & gdte->data[0]);
>
> > +	if (pdom->dirty_tracking)
> > +		new.data[0] |= DTE_FLAG_HAD;
> > +
> > +	if (dev_data->ats_enabled)
> > +		new.data[1] |= DTE_FLAG_IOTLB;
>
> This sequence should be in some set_dte_gcr3() ??

Not sure what you mean. This logic was in set_dte_entry(), and 
duplicated here in the set_dte_nested() since we no longer calling 
set_dte_entry() from the nested path. Also, it's not really related to 
GCR3 table.

> > +	/*
> > +	 * Restore cached persistent DTE bits, which can be set by information
> > +	 * in IVRS table. See set_dev_entry_from_acpi().
> > +	 */
> > +	initial_dte = amd_iommu_get_ivhd_dte_flags(iommu->pci_seg->id, dev_data->devid);
> > +	if (initial_dte) {
> > +		new.data128[0] |= initial_dte->data128[0];
> > +		new.data128[1] |= initial_dte->data128[1];
> > +	}
>
> This should go into amd_iommu_make_clear_dte() I think, and refactor
> it out of iommu_update_dte256() ?
> Every created DTE needs these bits set, right?

Currently, the amd_iommu_make_clear_dte() clears all the DTE bits and 
set the DTE[V] (valid) bit. This is used when preparing the DTE for 
programming, detaching domain, and when setting the blocking domain. 
Putting this logic in the function would change the behavior.

These bits affect Interrupt remapping (Lint0/Lint1/NMI/INIT/ExtInt 
interrupt pass-through) and System management message behavior. It 
should be okay to set these up for the specified devices in the current 
implementation.

> > +
> > +	/* Guest translation stuff */
> > +	new.data[0] |= (gdte->data[0] &
> > +		       (DTE_GLX | DTE_FLAG_GV | DTE_FLAG_GIOV));
> > +
> > +	/* GCR3 table */
> > +	new.data[0] |= (gdte->data[0] & DTE_GCR3_14_12);
> > +	new.data[1] |= (gdte->data[1] & (DTE_GCR3_30_15 | DTE_GCR3_51_31));
> > +
> > +	/* Guest paging mode */
> > +	new.data[2] |= (gdte->data[2] & DTE_GPT_LEVEL_MASK);
>
> I didn't see anything validating gdte has only permitted set bits in
> the prior patch?

Not sure which on are you referring to.

> If this is goint to decode array item by item then why not use struct
> iommu_hwpt_amd_guest in the nested_domain ?

The struct dev_table_entry *gdte is basically the same information as in 
the struct iommu_hwpt_amd_guest.dte that we copied from the userspace 
into the more appropriate in-kernel data structure type, which is used 
within the driver.

Here, we just select only what we needed for configuring guest page 
table specifically to be programmed onto the host DTE.

Thanks,
Suravee