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

Jan Kiszka
2010-06-14T16:50:03Z

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