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

[Baolu Lu <baolu.lu@linux.intel.com>]

On 7/30/25 07:05, Jason Gunthorpe wrote:
> 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?

If there's no special purpose, I'd suggest dropping 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.

Thanks for the explanation.

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

You are right. The VT-d spec doesn't define the present bit for the SS
paging entries. Historically, the VT-d driver treats the R (bit 0) field
as the present bit.

> 
> 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 :)

Okay, fair enough.

> 
>>> +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 ever thought both.

> I can't recall the detail right now but I didn't think it was a
> very high priority.

Okay, so I'll treat that as a driver improvement, not a feature required
by this series.

Thanks,
baolu