Re: [PATCH 1/2] iommu: Prevent RESV_DIRECT devices from blocking domains

[Jason Gunthorpe]

On Mon, Jun 19, 2023 at 03:20:30PM +0100, Robin Murphy wrote:
> On 19/06/2023 2:41 pm, Jason Gunthorpe wrote:
> > On Mon, Jun 19, 2023 at 02:33:18PM +0100, Robin Murphy wrote:
> > > > @@ -2121,6 +2125,21 @@ static int __iommu_device_set_domain(struct iommu_group *group,
> > > >    {
> > > >    	int ret;
> > > > +	/*
> > > > +	 * If the driver has requested IOMMU_RESV_DIRECT then we cannot allow
> > > > +	 * the blocking domain to be attached as it does not contain the
> > > > +	 * required 1:1 mapping. This test effectively exclusive the device from
> > > > +	 * being used with iommu_group_claim_dma_owner() which will block vfio
> > > > +	 * and iommufd as well.
> > > > +	 */
> > > > +	if (dev->iommu->requires_direct &&
> > > > +	    (new_domain->type == IOMMU_DOMAIN_BLOCKED ||
> > > 
> > > Given the notion elsewhere that we want to use the blocking domain as a last
> > > resort to handle an attach failure,
> > 
> > We shouldn't do that for cases where requires_direct is true, the last
> > resort will have to be the static identity domain.
> > 
> > > at face value it looks suspect that failing to attach to a blocking
> > > domain could also be a thing. I guess technically this is failing at
> > > a slightly different level so maybe it does work out OK, but it's
> > > still smelly.
> > 
> > It basically says that this driver doesn't support blocking domains on
> > this device. What we don't want is for the driver to fail blocking or
> > identity attaches.
> 
> Is that really the relevant semantic though?

It is the semantic I have had in mind. The end goal:

1) Driver never fails identity or blocking attachments
2) Identity and blocking domains are global static so always available
3) Core code puts restrictions on when identity and blocking domains
   can be used (eg trusted, reserved regions)
4) Catastrophic error recovery ends up moving to either blocking
   (preferred) or identity.

> I thought the point was to prevent userspace (or anyone else for
> that matter) taking ownership of a device with reserved regions
> which we can't trust them to honour.

Yes, this is the practical side effect of enforcing the rules.

The other side effect is that it removes another special case where a
driver has special behavior tied to IOMMU_DOMAIN_DMA.

> assignment", not anything about attaching to blocking domains. Plus the
> existing intel-iommu behaviour being generalised is specific to
> IOMMU_DOMAIN_UNMANAGED.

Being specific to UNMANAGED is a driver mistake, IMHO. It is a hack to
make it only work with VFIO and avoid dma-iommu.

> On reflection, I don't think that aspect actually matters anyway - nobody
> can explicitly request attachment to a blocking domain

They are used as part of the ownership mechanism, blocking domains are
effectively explicitly requested by detaching domains from owned
groups.

Yes, take_ownership is very carefully a sufficient place to put the
check with today's design, but it feels wrong because the blocking
domain compatability has conceptually nothing to do with ownership. If
we use blocking domains in the future then the check will be in the
wrong place.

> so if the only time they're used is when the IOMMU driver has
> already had a catastrophic internal failure such that we decide to
> declare the device toasted and deliberately put it into an unusable
> state, blocking its reserved regions doesn't seem like a big deal.

I think we should discuss then when we get to actually implementing
the error recovery flow we want. I do like blocking in general for the
reasons you give, and that was my first plan.. But if the BIOS will
crash or something if we don't do the reserved region maps that isn't
so good either. IDK would like to hear from the people using this BIOS
feature.

Thanks,
Jason