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

[Michael S. Tsirkin]

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 <address@hidden>
> >>>>>>>
> >>>>>>> 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.

-- 
MST