[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: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 13/13] qdev-properties: Add pci-devaddr property |
Date: |
Sun, 10 Jun 2012 14:17:42 +0300 |
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?
--
MST
- Re: [Qemu-devel] [PATCH 11/13] Move pci_parse_devaddr to qdev-properties, (continued)
- [Qemu-devel] [PATCH 01/13] pci: Refactor pci_change_irq_level, Jan Kiszka, 2012/06/04
- [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 <=
- 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, 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