[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property |
Date: |
Sun, 10 Jun 2012 07:41:51 -0600 |
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 <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.
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
- [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, (continued)
- [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Jan Kiszka, 2012/06/04
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Jan Kiszka, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Jan Kiszka, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Jan Kiszka, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Jan Kiszka, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property,
Alex Williamson <=
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Alex Williamson, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Alex Williamson, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Alex Williamson, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Alex Williamson, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10
- Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property, Michael S. Tsirkin, 2012/06/10