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

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Aug 22, 2025 at 09:14:09AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, July 17, 2025 3:58 AM
> > 
> > The VT-D second stage format is almost the same as the x86 PAE format,
> > except the bit encodings in the PTE are different and a few new PTE
> > features, like force coherency are present.
> > 
> > Among all the formats it is unique in not having a designated present bit.
> > 
> > Comparing the performance of several operations to the existing version:
> > 
> > iommu_map()
> >    pgsz  ,avg new,old ns, min new,old ns  , min % (+ve is better)
> >      2^12,     53,66    ,      50,64      ,  21.21
> >      2^21,     59,70    ,      56,67      ,  16.16
> >      2^30,     54,66    ,      52,63      ,  17.17
> >  256*2^12,    384,524   ,     337,516     ,  34.34
> >  256*2^21,    387,632   ,     336,626     ,  46.46
> >  256*2^30,    376,629   ,     323,623     ,  48.48
> 
> It's a big win, thanks! Out of curiosity, is there a single aspect in the 
> new design contributing most of the improvement or is it just
> accumulated from many pieces?

I think this is principally from not rewalking so much. The new code
fills entire levels with leaf PTEs without rewalking back to the
level. This is somewhat tricky code.

> a side note - the variation between avg/min is getting bigger in the
> new code, but it's not that important for now compared to the
> overall gain. 😊

It could be a side effect of running in kvm..  I think the new code
has alot of instructions and branching so it is more sensitive to
cache performance.

> > +static inline u64 vtdss_pt_sw_bit(unsigned int bitnr)
> > +{
> > +	/* Bits marked Ignored in the specification */
> > +	switch (bitnr) {
> > +	case 0:
> > +		return BIT(10);
> > +	case 1 ... 10:
> > +		return BIT_ULL((bitnr - 1) + 52);
> 
> bit61 has a meaning now in the latest v5.0 spec, though it's not a
> good practice to repurpose an ignored bit.

"not a good practice" is an understatement.
 
> > +	case 11:
> > +		return BIT_ULL(63);
> > +	/* Remaing bits 9-3  are only available in some entries */
> 
> s/Remaing/remaining/
> 
> strictly speaking bit8 is A-bit in all entries. Probably no need to
> list each bit here or just remove the comment. People interested
> in the detail anyway will go to the spec.

I wanted to leave a note that this was not exhaustive

> btw bit2 is actually available for software usage.

That seems to be a spec mistake, it should be RSV since it used to be
X in earlier HW?
 
> > +
> > +	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;
> > +
> > +	if (pt_feature(common, PT_FEAT_VTDSS_FORCE_WRITEABLE) &&
> > +	    !(iommu_prot & IOMMU_READ)) {
> 
> s/IOMMU_READ/IOMMU_WRITE/

oops got it

Thanks,
Jason