On Tue, 14 Feb 2023 19:25:19 -0400
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Tue, Feb 14, 2023 at 03:26:27PM -0700, Alex Williamson wrote:
> > > index 857d6ba349e1..d869913baafd 100644
> > > --- a/virt/kvm/vfio.c
> > > +++ b/virt/kvm/vfio.c
> > > @@ -286,18 +286,18 @@ static int kvm_vfio_set_file(struct kvm_device *dev, long attr,
> > > int32_t fd;
> > >
> > > switch (attr) {
> > > - case KVM_DEV_VFIO_GROUP_ADD:
> > > + case KVM_DEV_VFIO_FILE_ADD:
> > > if (get_user(fd, argp))
> > > return -EFAULT;
> > > return kvm_vfio_file_add(dev, fd);
> > >
> > > - case KVM_DEV_VFIO_GROUP_DEL:
> > > + case KVM_DEV_VFIO_FILE_DEL:
> > > if (get_user(fd, argp))
> > > return -EFAULT;
> > > return kvm_vfio_file_del(dev, fd);
> > >
> > > #ifdef CONFIG_SPAPR_TCE_IOMMU
> > > - case KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE:
> > > + case KVM_DEV_VFIO_FILE_SET_SPAPR_TCE:
> > > return kvm_vfio_file_set_spapr_tce(dev, arg);
> >
> > I don't see that the SPAPR code is so easily fungible to a device
> > file descriptor. The kvm_vfio_spapr_tce data structure includes a
> > groupfd, which is required to match a groupfd on the file_list. So
> > a SPAPR user cannot pass a cdev via FILE_ADD if they intend to use
> > this TCE code.
>
> SPAPR cannot use cdev at all, cdev mode only works with iommufd.
>
> So with my other remark about blocking unbound cdevs, in SPAPR mode
> you can never open a cdev and make it bound thus
> kvm_vfio_file_iommu_group() and others will return NULL always for
> cdev.
>
> Thus AFAICT this is all fine.
A device file opened through a group could be passed through this
interface though, right? Do we just chalk that up to user error?
Maybe the SPAPR extension at least needs to be documented as relying on
registering groups rather than devices.
> Yi, you should also add some kconfig stuff to ensure that SPAPR always
> has the group interface compiled in.
>
> > That also makes me wonder what we're really gaining in forcing this
> > generalization from group to file.
>
> I think it is just less code overall. Otherwise we need to needlessly
> double quite a lot of stuff, rather pointlessly, IMHO.
>
> I'm still thinking about proposing to just delete all this SPAPR
> stuff. Power still hasn't had the patches applied to make it work
> again so it seems to all be dead.
There's been some off-list discussion about at least fixing SPAPR
support, but yes, it either needs to get some love or we ought to think
about its future. Thanks,
Alex
