Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs

From: Jason Gunthorpe <jgg@nvidia.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	"Liu, Yi L" <yi.l.liu@intel.com>,
	Jacob Pan <jacob.jun.pan@linux.intel.com>,
	Auger Eric <eric.auger@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Joerg Roedel <joro@8bytes.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	David Woodhouse <dwmw2@infradead.org>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	Tejun Heo <tj@kernel.org>, Li Zefan <lizefan@huawei.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	Jonathan Corbet <corbet@lwn.net>,
	"Raj, Ashok" <ashok.raj@intel.com>, "Wu, Hao" <hao.wu@intel.com>,
	"Jiang, Dave" <dave.jiang@intel.com>,
	Alexey Kardashevskiy <aik@ozlabs.ru>
Subject: Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Date: Tue, 27 Apr 2021 14:12:12 -0300	[thread overview]
Message-ID: <20210427171212.GD1370958@nvidia.com> (raw)
In-Reply-To: <YIecXkaEGNgICePO@yekko.fritz.box>

On Tue, Apr 27, 2021 at 03:08:46PM +1000, David Gibson wrote:
> > Starting from a BDF the general pseudo code is:
> >  device_name = first_directory_of("/sys/bus/pci/devices/BDF/vfio/")
> >  device_fd = open("/dev/vfio/"+device_name)
> >  ioasidfd = open("/dev/ioasid")
> >  ioctl(device_fd, JOIN_IOASID_FD, ioasidfd)
> 
> This line is the problem.
> 
> [Historical aside: Alex's early drafts for the VFIO interface looked
> quite similar to this.  Ben Herrenschmidt and myself persuaded him it
> was a bad idea, and groups were developed instead.  I still think it's
> a bad idea, and not just for POWER]

Spawning the VFIO device FD from the group FD is incredibly gross from
a kernel design perspective. Since that was done the struct
vfio_device missed out on a sysfs presence and doesn't have the
typical 'struct device' member or dedicated char device you'd expect a
FD based subsystem to have.

This basically traded normal usage of the driver core for something
that doesn't serve a technical usage. Given we are now nearly 10 years
later and see that real widely deployed applications are not doing
anything special with the group FD it makes me question the wisdom of
this choice.

> As Alex says, if this line fails because of the group restrictions,
> that's not great because it's not very obvious what's gone wrong.  

Okay, that is fair, but let's solve that problem directly. For
instance netlink has been going in the direction of adding a "extack"
from the kernel which is a descriptive error string. If the failing
ioctl returned the string:

  "cannot join this device to the IOASID because device XXX in the
   same group #10 is in use"

Would you agree it is now obvious what has gone wrong? In fact would
you agree this is a lot better user experience than what applications
do today even though they have the group FD?

> But IMO, the success path on a multi-device group is kind of worse:
> you've now made made a meaningful and visible change to the setup of
> devices which are not mentioned in this line *at all*.  

I don't think spawning a single device_fd from the guoup clearly says
there are repercussions outside that immediate, single, device.

That comes from understanding what the ioctls are doing, and reading
the documentation. The same applies to some non-group FD world.

> Yes, it makes set up more of a pain, but it's necessary complexity to
> actually understand what's going on here.

There is a real technical problem here - the VFIO group is the thing
that spawns the device_fd and that is incompatible with the idea to
centralize the group security logic in drivers/iommu/ and share it
with multiple subsystems.

We also don't have an obvious clean way to incorporate a group FD into
other subsystems (nor would I want to).

One option is VFIO can keep its group FD but nothing else will have
anthing like it. However I don't much like the idea that VFIO will
have a special and unique programming model to do that same things
other subsystem will do. That will make it harder for userspace to
implement.

But again, lets see what the draft ioasid proposal looks like and
maybe someone will see a different solution.

Jason