Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property

[Michael S. Tsirkin]

On Sun, Jun 10, 2012 at 11:29:10AM -0600, Alex Williamson wrote:
> On Sun, 2012-06-10 at 19:22 +0300, Michael S. Tsirkin wrote:
> > On Sun, Jun 10, 2012 at 09:58:17AM -0600, Alex Williamson wrote:
> > > On Sun, 2012-06-10 at 18:37 +0300, Michael S. Tsirkin wrote:
> > > > On Sun, Jun 10, 2012 at 09:15:10AM -0600, Alex Williamson wrote:
> > > > > On Sun, 2012-06-10 at 17:54 +0300, Michael S. Tsirkin wrote:
> > > > > > On Sun, Jun 10, 2012 at 08:41:03AM -0600, Alex Williamson wrote:
> > > > > > > On Sun, 2012-06-10 at 17:03 +0300, Michael S. Tsirkin wrote:
> > > > > > > > On Sun, Jun 10, 2012 at 07:41:51AM -0600, Alex Williamson wrote:
> > > > > > > > > > > >>>> vfio_pci.c contains a nice function called 
> > > > > > > > > > > >>>> "parse_hostaddr". You may
> > > > > > > > > > > >>>> guess what it does. ;)
> > > > > > > > > > > >>>
> > > > > > > > > > > >>> Interesting. Why? This looks strange to me:
> > > > > > > > > > > >>> I would expect the admin to bind a device to vfio
> > > > > > > > > > > >>> the way it's now bound to a stub.
> > > > > > > > > > > >>> The pass /dev/vfioXXX to qemu.
> > > > > > > > > > > >>
> > > > > > > > > > > >> That's the "libvirt way". We surely also want the 
> > > > > > > > > > > >> "qemu command line
> > > > > > > > > > > >> way" for which this kind of service is needed.
> > > > > > > > > > > >>
> > > > > > > > > > > >> Jan
> > > > > > > > > > > >>
> > > > > > > > > > > > 
> > > > > > > > > > > > Yes, I imagine the qemu command line passing in 
> > > > > > > > > > > > /dev/vfioXXX,
> > > > > > > > > > > > the libvirt way will pass in an fd for above. No?
> > > > > > > > > > > 
> > > > > > > > > > > As far as I understand the API, there is no device file 
> > > > > > > > > > > per assigned
> > > > > > > > > > > device.
> > > > > > > > > > 
> > > > > > > > > > Does it do pci_get_domain_bus_and_slot like kvm then?
> > > > > > > > > > With all the warts like you have to remember to bind pci 
> > > > > > > > > > stub
> > > > > > > > > > or you get two drivers for one device?
> > > > > > > > > > If true that's unfortunate IMHO.
> > > > > > > > 
> > > > > > > > I hope the answer to the above is no?
> > > > > > > 
> > > > > > > No, it does a probe for devices.  We need the devaddr to compare 
> > > > > > > against
> > > > > > > dev_name of the device to figure out which device the user is 
> > > > > > > attempting
> > > > > > > to identify.
> > > > > > > 
> > > > > > > > > > > Also, this [domain:]bus:dev.fn format is more handy for 
> > > > > > > > > > > the
> > > > > > > > > > > command line.
> > > > > > > > > > > 
> > > > > > > > > > > Jan
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Then users could add udev rules that will name vfio devices
> > > > > > > > > > like this.  Another interesting option: /dev/vfio/eth0/vf1.
> > > > > > > > > > That's better I think: no one really likes running lspci
> > > > > > > > > > and guessing the address from there.
> > > > > > > > > 
> > > > > > > > > That's not at all how VFIO works.  /dev/vfio/# represents a 
> > > > > > > > > group, which
> > > > > > > > > may contain one or more devices.  Even if libvirt passes a 
> > > > > > > > > file
> > > > > > > > > descriptor for the group, qemu needs to know which device in 
> > > > > > > > > the group
> > > > > > > > > to add to the guest, so parsing a device address is still 
> > > > > > > > > necessary.
> > > > > > > > > Thanks,
> > > > > > > > > 
> > > > > > > > > Alex
> > > > > > > > 
> > > > > > > > That's very unusual, and unfortunate.  For example this means 
> > > > > > > > that I
> > > > > > > > must update applications just because I move a card to another 
> > > > > > > > slot.
> > > > > > > > UIO does not have this problem.
> > > > > > > > The fact that it's broken in kvm ATM seems to have made people
> > > > > > > > think it's okay, but it really is a bug. We didn't fix it
> > > > > > > > because vfio was supposed to be the solution.
> > > > > > > 
> > > > > > > I don't know what you're talking about here.  Are you suggesting 
> > > > > > > that
> > > > > > > needing to specify -device pci-assign,host=3.0 changing to 
> > > > > > > host=4.0 when
> > > > > > > you move a card is broken?
> > > > > > 
> > > > > > Yes. Absolutely. Admin should be able to abstract it away without 
> > > > > > users
> > > > > > knowing anything about it.
> > > > > 
> > > > > We don't have UUIDs on PCI devices, so who's to say that the device 
> > > > > that
> > > > > was in slot 3 is the same device that's now in slot 4 and the user
> > > > > should still have access to it?  That sounds even more problematic.
> > > > 
> > > > PF has a driver loaded so you can identify that, and
> > > > identify the VF through it. Again this is really policy,
> > > > it should be up to the admin how to name the device.
> > > 
> > > Do PFs have a UUID?  Some devices support a serial number, but that's
> > > not related to being a PF vs VF.  We need to support both PFs and VFs
> > > regardless of whether they have any kind of UUID.
> > 
> > This is a solved problem. udev has class-specific ways to
> > get the device id and use it to find the name.
> 
> Usually via things discovered via device specific drivers, ex. MACs,
> disk UUIDs, etc.  We don't have those in VFIO.  Don't forget, VFIO is
> also device agnostic, VFIO-pci is just one possible driver backend.

Exactly. But udev can see that what it got is a VF anf query the PF.

> > > I think we're inventing a problem though.
> > 
> > You think persistent names in udev were a solution in search of a
> > problem?
> 
> Of course not, but I've never heard any indication that this is a real
> problem for device assignment.

How is it different from any other workload?

>  If it is, like udev, why not solve it in
> userspace?  The kernel doesn't provide the persistence that udev
> exposes.

To solve it you need to find the PF. Admin does it easily
but VFIO conceptually handles one VF at a time.
So let the admin set the name.
And it is better using standard udev tools than some
hacked interface specific to vfio.

> > > > > > >  How does UIO avoid such a problem.
> > > > > > 
> > > > > > Normally you use a misc device that you can name with udev.
> > > > > > 
> > > > > > >  UIO-pci
> > > > > > > requires the user to use pci-sysfs for resource access, so it 
> > > > > > > surely
> > > > > > > cares about the device address.
> > > > > > 
> > > > > > Only uio_pci_generic. Other uio devices let you drive the
> > > > > > device.
> > > > > 
> > > > > If this is actually a problem, this is the first ever complaint I've
> > > > > heard about it.  As above, I don't think we can assume the same access
> > > > > when a device is moved.
> > > > 
> > > > I thought need for sane naming and for sysfs interface was discussed
> > > > multiple times. But maybe I'm misremembering.
> > > 
> > > There is sane naming and a sysfs interface...
> > 
> > It's not sane if the admin can't rename the device without breaking
> > applications.
> 
> Again, udev provides that persistence, not the kernel.

So let it do its thing.

> > > > > > > > I do realize you want to represent a group of devices somehow 
> > > > > > > > but can't
> > > > > > > > this be solved without breaking naming devices with udev? For 
> > > > > > > > example, the
> > > > > > > > device could be a file as well. You would then use the fd to 
> > > > > > > > identify the
> > > > > > > > device within the group. And in a somewhat common case of a 
> > > > > > > > single device
> > > > > > > > within the group, you can even make opening the group optional.
> > > > > > > > Don't know if this fix I suggest makes sense at all but it's a 
> > > > > > > > real
> > > > > > > > problem all the same.
> > > > > > > 
> > > > > > > Unfortunately, exposing individual devices just confuses the 
> > > > > > > ownership
> > > > > > > model we require for groups.  It would provide the illusion of 
> > > > > > > being
> > > > > > > able to assign an individual device, without the reality of the
> > > > > > > grouping.  Groups are owned either by _a_ user or by the kernel, 
> > > > > > > they
> > > > > > > can't be split across multiple users (at least not with any 
> > > > > > > guarantees
> > > > > > > of isolation).  The current interface makes this clear.  Thanks,
> > > > > > > 
> > > > > > > Alex
> > > > > > 
> > > > > > So do users pass in group=/dev/vfio/1,host=0:3.0 then?
> > > > > 
> > > > > No, vfio syntax is -device vfio-pci,host=0:3.0, just like pci-assign.
> > > > > Qemu will figure out which group that device belongs to and "do the
> > > > > right thing".  If we add support for libvirt passing a groupfd, it 
> > > > > will
> > > > > be mostly the same, just using scm_rights to get the groupfd instead 
> > > > > of
> > > > > opening it directly.  Thanks,
> > > > > 
> > > > > Alex
> > > > 
> > > > Then how do you know which /dev/vfio/# to open?
> > > 
> > > This is all in the documentation patch... groups are exposed in sysfs
> > > in /sys/kernel/iommu_groups.  Each group has a unique number which is
> > > exposed as a directory.  Each group directory has a subdirectory called
> > > devices which links to all devices in the group.  Each device within a
> > > group as an iommu_group link back to the group directory.
> > > The /dev/vfio/# entry matches the group number in sysfs.  So it's all
> > > pretty easy.  Thanks,
> > > 
> > > Alex
> > 
> > So what's the problem to have devices in sysfs linked e.g. from
> > /sys/class/vfio/ ?
> 
> They have to be bound to a VFIO driver, ex. vfio-pci.  We have iommu
> groups in sysfs and can use that for this purpose.  I did have a class
> for this before iommu_group were more integrated at the device level,
> now it just seems redundant.
> 
> >   udev could create the nodes e.g. in
> > /dev/vfio/devices/.  User can then pass the device name and qemu can
> > figure out the group from sysfs.
> 
> As previously noted, we don't, and I don't think it makes sense to
> expose individual devices in VFIO as the interface is fundamentally
> group based by necessity.  Groups are just containers and they do
> support a name, which is exposed in sysfs when it's used, but groups are
> created by topology and don't follow the devices, so I don't think
> that's what you're looking for.  It seems that if someone wants to
> create persistence for VFIO, they just need to identify something unique
> about a device in sysfs, then follow the device to the group.

So you are forced to code this all upfront in qemu.
And since you don't in practice, no percictence and no way to provide it.

>  Userpsace
> is far more capable of providing this than the kernel.

It's the admin that knows. Not applications.

>  Thanks,
> 
> Alex