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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3146 bytes --]

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.kiszka@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?

> Can this be part of device assignment code instead of qdev?

How does VFIO address their host devices? And Xen?

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]