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 <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?As far as I understand the API, there is no device file per assigned device. Also, this [domain:]bus:dev.fn format is more handy for the command line. Jan
signature.asc
Description: OpenPGP digital signature
