On Sun, 2012-06-10 at 15:01 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 10, 2012 at 01:25:41PM +0200, Jan Kiszka wrote: > > On 2012-06-10 13:17, Michael S. Tsirkin wrote: > > > On Sun, Jun 10, 2012 at 01:00:35PM +0200, Jan Kiszka wrote: > > >> On 2012-06-10 12:58, Michael S. Tsirkin wrote: > > >>> On Sun, Jun 10, 2012 at 12:52:45PM +0200, Jan Kiszka wrote: > > >>>> On 2012-06-10 12:49, Michael S. Tsirkin wrote: > > >>>>> On Sun, Jun 10, 2012 at 12:14:36PM +0200, Jan Kiszka wrote: > > >>>>>> On 2012-06-10 11:35, Michael S. Tsirkin wrote: > > >>>>>>> On Mon, Jun 04, 2012 at 10:52:21AM +0200, Jan Kiszka wrote: > > >>>>>>>> Add a property to receive a fully qualified PCI device address. > > >>>>>>>> > > >>>>>>>> Will be used by KVM device assignment. > > >>>>>>>> > > >>>>>>>> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> > > >>>>>>> > > >>>>>>> I'd like to ponder this a bit more. What bothers me is that this > > >>>>>>> mixes > > >>>>>>> two things: > > >>>>>>> - addressing of qemu devices > > >>>>>>> Using full device addresses there is a legacy feature, > > >>>>>>> users really should supply the parent bus and > > >>>>>>> the bus local address. > > >>>>>>> - addressing devices on the linux host for assignment > > >>>>>>> It so happens that the syntax matches > > >>>>>>> the legacy naming very closely, > > >>>>>>> but conceptually is completely unrelated > > >>>>>> > > >>>>>> We can keep code duplications, of course. > > >>>>>> > > >>>>>>> > > >>>>>>>> --- > > >>>>>>>> hw/qdev-properties.c | 48 > > >>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++ > > >>>>>>>> hw/qdev.h | 3 +++ > > >>>>>>>> 2 files changed, 51 insertions(+), 0 deletions(-) > > >>>>>>>> > > >>>>>>>> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > > >>>>>>>> index 32e41f1..6634f22 100644 > > >>>>>>>> --- a/hw/qdev-properties.c > > >>>>>>>> +++ b/hw/qdev-properties.c > > >>>>>>>> @@ -946,6 +946,54 @@ PropertyInfo qdev_prop_pci_devfn = { > > >>>>>>>> .max = 0xFFFFFFFFULL, > > >>>>>>>> }; > > >>>>>>>> > > >>>>>>>> +static void get_pci_devaddr(Object *obj, Visitor *v, void *opaque, > > >>>>>>>> + const char *name, Error **errp) > > >>>>>>>> +{ > > >>>>>>>> + DeviceState *dev = DEVICE(obj); > > >>>>>>>> + Property *prop = opaque; > > >>>>>>>> + PCIDeviceAddress *addr = qdev_get_prop_ptr(dev, prop); > > >>>>>>>> + char buffer[10 + 3 + 1]; > > >>>>>>>> + char *p = buffer; > > >>>>>>>> + > > >>>>>>>> + snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%02x", > > >>>>>>>> + addr->domain, addr->bus, addr->slot, addr->function); > > >>>>>>>> + > > >>>>>>>> + visit_type_str(v, &p, name, errp); > > >>>>>>>> +} > > >>>>>>>> + > > >>>>>>>> +static void set_pci_devaddr(Object *obj, Visitor *v, void *opaque, > > >>>>>>>> + const char *name, Error **errp) > > >>>>>>>> +{ > > >>>>>>>> + DeviceState *dev = DEVICE(obj); > > >>>>>>>> + Property *prop = opaque; > > >>>>>>>> + PCIDeviceAddress *addr = qdev_get_prop_ptr(dev, prop); > > >>>>>>>> + Error *local_err = NULL; > > >>>>>>>> + char *str; > > >>>>>>>> + > > >>>>>>>> + if (dev->state != DEV_STATE_CREATED) { > > >>>>>>>> + error_set(errp, QERR_PERMISSION_DENIED); > > >>>>>>>> + return; > > >>>>>>>> + } > > >>>>>>>> + > > >>>>>>>> + visit_type_str(v, &str, name, &local_err); > > >>>>>>>> + if (local_err) { > > >>>>>>>> + error_propagate(errp, local_err); > > >>>>>>>> + return; > > >>>>>>>> + } > > >>>>>>>> + > > >>>>>>>> + if (qemu_parse_pci_devaddr(str, addr, > > >>>>>>>> + PCI_DEVADDR_WITH_DOM_BUS_OPT | > > >>>>>>>> + PCI_DEVADDR_WITH_FUNC) < 0) { > > >>>>>>>> + error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, > > >>>>>>>> str); > > >>>>>>>> + } > > >>>>>>>> +} > > >>>>>>>> + > > >>>>>>>> +PropertyInfo qdev_prop_pci_devaddr = { > > >>>>>>>> + .name = "pci-devaddr", > > >>>>>>> > > >>>>>>> This is a very confusing name. Something like host-pci-address? > > >>>>>> > > >>>>>> That might be an option. > > >>>>>> > > >>>>>>> This also should be built on linux only. > > >>>>>> > > >>>>>> Why, what do we gain with #ifdefs? And isn't the addressing concept > > >>>>>> generic? > > >>>>> > > >>>>> Not the XXX:XX.X format. And not the concept of a domain. > > >>>>> > > >>>>>>> Can this be part of device assignment code instead of qdev? > > >>>>>> > > >>>>>> How does VFIO address their host devices? > > >>>>> > > >>>>> You get an fd I think. I think you don't need to know the host > > >>>>> address. > > >>>> > > >>>> 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. > > > 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