Re: [PATCH v4 09/17] iommufd: Data structure to provide IOVA to PFN mapping

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Nov 16, 2022 at 12:09:52AM +0000, Tian, Kevin wrote:

> > > > 0 is commonly used as an errant value for uninitialized things. We
> > > > don't automatically map it into a process mm because it can cause
> > > > security problems if we don't trap a bogus 0/NULL pointer reference.
> > > >
> > > > The same logic applies here too, the allocator should not return 0 to
> > > > reserve it as an unmapped IOVA page to catch bugs.
> > >
> > > CPU doesn't reference IOVA. Where do such bugs exist?
> > 
> > SW is always buggy and SW programs the DMA address, so it could leave
> > a 0 behind or something during the programming.
> 
> address 0 is never a bug in DMA to IOVA. if it is, it will be out of the
> aperture or in the reserved IOVA list. 

It is a SW bug in the sense that 0 is commonly an uninitialized value or
uninitialized memory.

> DMA API is also a auto-iova scheme from driver p.o.v while it doesn't
> impose any restriction on address 0.

It probably shouldn't do that. It also allocates -1ULL which causes
real bugs too. :(

 
> > > > > > +		if (!__alloc_iova_check_used(&allowed_span, length,
> > > > > > +					     iova_alignment,
> > page_offset))
> > > > > > +			continue;
> > > > > > +
> > > > > > +		interval_tree_for_each_span(&area_span, &iopt-
> > >area_itree,
> > > > > > +					    allowed_span.start_used,
> > > > > > +					    allowed_span.last_used) {
> > > > > > +			if (!__alloc_iova_check_hole(&area_span,
> > length,
> > > > > > +						     iova_alignment,
> > > > > > +						     page_offset))
> > > > > > +				continue;
> > > > > > +
> > > > > > +
> > 	interval_tree_for_each_span(&reserved_span,
> > > > > > +						    &iopt-
> > >reserved_itree,
> > > > > > +
> > area_span.start_used,
> > > > > > +
> > area_span.last_used) {
> > > > > > +				if (!__alloc_iova_check_hole(
> > > > > > +					    &reserved_span, length,
> > > > > > +					    iova_alignment,
> > page_offset))
> > > > > > +					continue;
> > > > >
> > > > > this could be simplified by double span.
> > > >
> > > > It is subtly not compatible, the double span looks for used areas.
> > > > This is looking for a used area in the allowed_itree, a hole in the
> > > > area_itree, and a hole in the reserved_itree.
> > >
> > > the inner two loops can be replaced by double span, since both
> > > are skipping used areas.
> > 
> > The 2nd loop is looking for a used on allowed and the 3rd loop is
> > looking for a hole in reserved. To fix it we'd have to invert allowed
> > to work like reserved - which complicates the uAPI code.
> 
> The 1st loop finds an allowed range which can hold requested length
> 
> The 2nd loop finds an *unused* hole in the allowed range
> 
> The 3rd loop further looks for a hole in reserved.
> 
> last two both try to find a hole.

Ooh, OK, I read that in the wrong order, you know I looked at this
many times to see if it could use the double span..

Ugh that is a pain, the double_span.h isn't setup for two .c files to
use it.

Anyhow, so like this:

	interval_tree_for_each_span(&allowed_span, &iopt->allowed_itree,
				    PAGE_SIZE, ULONG_MAX - PAGE_SIZE) {
		if (RB_EMPTY_ROOT(&iopt->allowed_itree.rb_root)) {
			allowed_span.start_used = PAGE_SIZE;
			allowed_span.last_used = ULONG_MAX - PAGE_SIZE;
			allowed_span.is_hole = false;
		}

		if (!__alloc_iova_check_used(&allowed_span, length,
					     iova_alignment, page_offset))
			continue;

		interval_tree_for_each_double_span(
			&used_span, &iopt->reserved_itree, &iopt->area_itree,
			allowed_span.start_used, allowed_span.last_used) {
			if (!__alloc_iova_check_hole(&used_span, length,
						     iova_alignment,
						     page_offset))
				continue;

			*iova = used_span.start_hole;
			return 0;
		}
	}


> > This is the comment:
> > 
> > /*
> >  * This is part of the VFIO compatibility support for VFIO_TYPE1_IOMMU.
> > That
> >  * mode permits splitting a mapped area up, and then one of the splits is
> >  * unmapped. Doing this normally would cause us to violate our invariant of
> >  * pairing map/unmap. Thus, to support old VFIO compatibility disable
> > support
> >  * for batching consecutive PFNs. All PFNs mapped into the iommu are done
> > in
> >  * PAGE_SIZE units, not larger or smaller.
> >  */
> > static int batch_iommu_map_small(struct iommu_domain *domain,
> > 				 unsigned long iova, phys_addr_t paddr,
> > 				 size_t size, int prot)
> > 
> 
> I meant a comment in iopt_calculate_iova_alignment().

How about "see batch_iommu_map_small()" ?

Jason