Re: [Qemu-devel] [RFC PATCH 1/5] qdev: Create qdev_get_dev_path()

[Jan Kiszka <jan.kiszka@siemens.com>]

Alex Williamson wrote:
> On Mon, 2010-06-14 at 18:00 +0200, Jan Kiszka wrote:
>> Alex Williamson wrote:
>>> On Mon, 2010-06-14 at 14:09 +0100, Paul Brook wrote:
>>>>>>> "/main-system-bus/pci.0,addr=09.0/virtio-blk-pci"
>>>> There's a device missing between the main system bus and the pci bus.  Should 
>>>> be something like:
>>>>
>>>> /main-system-bus/piix4-pcihost/pci.0/_09.0
>>> Ok, I can easily come up with:
>>>
>>> /System/main-system-bus/i440FX-pcihost/PCI/pci.0/_09.0/virtio-blk-pci/virtio-blk
>> First two elements are redundant, '/' already stands for the main system
>> bus.
> 
> Ok, we can start printing after the system bus.
> 
>>  Then I don't get what last element expresses (the target device is
>> virtio-blk-pci). Is this due to some vmstate layout? But that should not
>> be part into qdev paths (maybe I'm missing your use case).
> 
> Sorry, I wasn't clear about that.  My printf is in the savevm code,
> after it's already appended the idstr passed in from the device.  The
> device path in the example above ends at virtio-blk-pci.  That's the
> dev->info->name of the device passed into this function.
> 
>> And instead of introducing another hierarchy level with the bus address,
>> I would also prefer to add this as prefix or suffix to the device name,
>> e.g. <driver>.<busaddr>.
> 
> That's what I had started with.  The first post in this thread has
> "pci.0,addr=09.0" as a single hierarchy level.  The "addr=" may be
> unnecessary there, but I also prefer something along those lines.  For
> PCI it'd make sense to have <name>:<addr>, which comes out to
> "pci.0:09.0".  (Maybe rather than flagging properties as being relevant
> to the path and printing them generically, we should extract specific
> properties based on the bus type.)

Not bus.addr, driver.addr. We only have one PCI bus here, not as many as
there are slots on that bus.

> 
>>>> For busses that don't have a consistent addressing scheme then some sort of 
>>>> instance ID is unavoidable. I guess it may be possible to invent something 
>>>> based on other device properties (e.g. address of the first IO port/memory 
>>>> region).
>>> Instance IDs aren't always bad, we just run into trouble with hotplug
>>> and maybe creating unique ramblock names.  But, such busses typically
>>> don't support hotplug and don't have multiple instances of the same
>>> device on the bus, so I don't expect us to hit many issues there as long
>>> as we can get a stable address path.  Thanks,
>>>
>> If stable instance numbers are required, we could simply keep them in
>> DeviceState and search for the smallest free one on additions. Actually,
>> I'm more in favor of this direction than including the bus address. That
>> way we could keep a canonical format across all buses and do not have to
>> provide possibly complex ID generation rules for each of them.
> 
> I started down that path, but it still breaks for hotplug.  If we start
> a VM with two e1000 NICs, then remove the first, we can no longer
> migrate because the target can't represent having a single e1000 with a
> non-zero instance ID.

That's indeed a good point.

Still, I'm worried about having to define numbering schemes for all the
buses qemu supports. Maybe we can run a mixture: address-based for
hotplug-capably buses (they tend to be cooperative in this regard) and
simple instance numbers for the rest, likely the majority.

> 
>> BTW, the problem isn't truly solved by printing paths. We also need to
>> parse them. It would be counterproductive if such paths ever see the
>> light of day (ie. "leak" outside vmstate) and a user tries to hack it
>> into the monitor or pass it on the command line. With my series, I'm
>> currently able to process paths like this one:
>>
>> /i440FX-pcihost.0/pci.0/PIIX4_PM.0/i2c/smbus-eeprom.6
> 
> Yes, these are only intended for internal use, but we should get them to
> sync up as much as possible.  Thanks,

Unless there is a good reason, the match should be 100%.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux