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? -- MST