Re: [PATCH] iommu/virtio: Detach domain on endpoint release - Jean-Philippe Brucker

From: Jean-Philippe Brucker <jean-philippe@linaro.org>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: akihiko.odaki@daynix.com, will@kernel.org, joro@8bytes.org,
	virtualization@lists.linux-foundation.org, eric.auger@redhat.com,
	iommu@lists.linux.dev, robin.murphy@arm.com
Subject: Re: [PATCH] iommu/virtio: Detach domain on endpoint release
Date: Thu, 18 May 2023 17:49:37 +0100	[thread overview]
Message-ID: <20230518164937.GA2934820@myrica> (raw)
In-Reply-To: <ZGYvMDbkBN+2UgzG@nvidia.com>

On Thu, May 18, 2023 at 10:59:12AM -0300, Jason Gunthorpe wrote:
> On Thu, May 18, 2023 at 02:56:38PM +0100, Jean-Philippe Brucker wrote:
> > > Can you wrapper this into a BLOCKED domain like we are moving drivers
> > > toward, and then attach the blocked domain instead of introducing this
> > > special case?
> > 
> > Yes, I think the way the virtio-iommu driver should implement BLOCKED
> > domains is initially clearing the global-bypass bit, and then issuing
> > DETACH requests when the core asks to attach a BLOCKED domain. This has
> > the same effect as issuing an ATTACH request with an empty domain, but
> > requires fewer resources in the VMM.
> 
> Does that exclude identity though?

No, identity attaches a domain with the ATTACH_F_BYPASS flag (or an
identity-mapped domain if the feature is missing), it doesn't rely on
global-bypass.

> 
> It seems like the protocol should not have an implicit operation like
> this, the desired translation mode should always be made
> explicit.

I probably misunderstood your plan for BLOCKED. This particular patch is
about removing devices from the machine, for example PCIe hot-unplug. So I
thought you were suggesting the core will at some point attach a BLOCKED
domain to a device being removed, in order to block translation while the
device is being removed and while a new one is being plugged in with the
same RID. For that case I think DETACH, rather than ATTACH an empty
domain, makes more sense. Otherwise with the same reasoning we'd need to
attach all 4 billion endpoint IDs to an empty domain at boot which isn't
feasible. In addition I don't think we'll want to force the VMMs to keep
endpoint ID state internally after destroying devices, though that does
need to be specified one way or another.

If BLOCKED is only for transient states, for example while a struct device
is not bound to a driver, then attaching an empty domain works and is
simpler to implement. Probably the best is to implement BLOCKED this way
and still call DETACH in the release_device() op.

In any case, it shouldn't make a difference to the core. I'll see which
one is better for the VMMs.

Thanks,
Jean

> Having a boot time default makes sense, but there is little
> reason for an OS to go back to the boot time default...
> 
> Jason
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization