lkml.org 
[lkml]   [2021]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
    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

    \
     
     \ /
      Last update: 2021-04-27 19:13    [W:6.916 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site