Re: [PATCH 5/9] iommupt: Add the Intel VT-D second stage page table format

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jul 22, 2025 at 11:11:02AM +0800, Baolu Lu wrote:
> > diff --git a/drivers/iommu/generic_pt/Kconfig b/drivers/iommu/generic_pt/Kconfig
> > index 953856e4b48369..b631cf00eba559 100644
> > --- a/drivers/iommu/generic_pt/Kconfig
> > +++ b/drivers/iommu/generic_pt/Kconfig
> > @@ -56,6 +56,17 @@ config IOMMU_PT_RISCV64
> >   	  Selected automatically by an IOMMU driver that uses this format.
> > +config IOMMU_PT_VTDSS
> > +       tristate "IOMMU page table for Intel VT-D IOMMU Second Stage"
> > +	depends on !GENERIC_ATOMIC64 # for cmpxchg64
> > +	default n
> 
> The default value is a "n". So what's the value of putting a "default n"
> here?

I do not know, I cargo culted this from somewhere else, lots of
examples. Do you think we should drop it?

> > @@ -72,6 +83,7 @@ config IOMMU_PT_KUNIT_TEST
> >   	depends on KUNIT
> >   	depends on IOMMU_PT_AMDV1 || !IOMMU_PT_AMDV1
> >   	depends on IOMMU_PT_RISCV64 || !IOMMU_PT_RISCV64
> > +	depends on IOMMU_PT_VTDSS || !IOMMU_PT_VTDSS
> 
> This line implies that the IOMMU_PT kunit test functions regardless of
> whether IOMMU_PT_VTDSS is enabled. But if IOMMU_PT_VTDSS is enabled,
> this kunit test will also cover it. Do I understand this correctly?

Yes.

The kunit test will build a unique:

kunit_test_suites(&NS(generic_pt_suite));

For VTDSS if it is compiled in and that macro eventually drops an ELF section:

	__used __section(".kunit_test_suites") = { __VA_ARGS__ }

And then the linker and some kunit magic will automatically run it
just be virtue of having compiled it.

The odd || expression is a kconfig trick that ensures that the kunit
and vtdss have compatible modularity. ie the kunit cannot be built in
while the vtdss is modular.

> > +static inline enum pt_entry_type vtdss_pt_load_entry_raw(struct pt_state *pts)
> > +{
> > +	const u64 *tablep = pt_cur_table(pts, u64);
> > +	u64 entry;
> > +
> > +	pts->entry = entry = READ_ONCE(tablep[pts->index]);
> > +	if (!entry)
> > +		return PT_ENTRY_EMPTY;
> 
> Would it be more reasonable to check the present bit of the entry
> here?

VTDSS has no present bit? Did I misunderstand that in the spec?

IIRC this design uses all bits as 0 to mean non-present.

> Otherwise, it implies that when a PTE is non-present, all fields must be
> cleared. I'm concerned about any potential corner cases.

Since this code makes all the PTEs it does do that correctly, and we
have a great test suite that looks for corner cases :)

> > +static inline int vtdss_pt_iommu_set_prot(struct pt_common *common,
> > +					  struct pt_write_attrs *attrs,
> > +					  unsigned int iommu_prot)
> > +{
> > +	u64 pte = 0;
> > +
> > +	/*
> > +	 * VTDSS does not have a present bit, so we tell if any entry is present
> > +	 * by checking for R or W.
> > +	 */
> > +	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> > +		return -EINVAL;
> > +
> > +	if (iommu_prot & IOMMU_READ)
> > +		pte |= VTDSS_FMT_R;
> > +	if (iommu_prot & IOMMU_WRITE)
> > +		pte |= VTDSS_FMT_W;
> > +	if (pt_feature(common, PT_FEAT_VTDSS_FORCE_COHERENCE))
> > +		pte |= VTDSS_FMT_SNP;
> 
> The comment says:
> 
>  /*
>   * The PTEs are set to prevent cache incoherent traffic, such as PCI no
>   * snoop. This is set either at creation time or before the first map
>   * operation.
>   */
>   PT_FEAT_VTDSS_FORCE_COHERENCE = PT_FEAT_FMT_START,
> 
> It seems that you are okay with setting this feature after iommu_pt
> creation and before the first map operation? 

Yes it works as it is now.

> Do we still need to reform the enforce_cache_coherency callback
> mechanism?

I think that was motivated by the code in the driver, not so much this
code? I can't recall the detail right now but I didn't think it was a
very high priority.

Jason