Patchwork [RFC,1/5] qdev: Create qdev_get_dev_path()

login
register
mail settings
Submitter Alex Williamson
Date June 14, 2010, 5:51 a.m.
Message ID <20100614055119.879.92321.stgit@localhost.localdomain>
Download mbox | patch
Permalink /patch/55470/
State New
Headers show

Comments

Alex Williamson - June 14, 2010, 5:51 a.m.
qdev_get_dev_path() is intended to be the canonical utility for creating
a string representing the qdev hierarchy of a device.  The path consists
of bus and device names as well as identified properties of the immediate
parent bus and device.  This results in strings like:

"/main-system-bus/pci.0,addr=00.0/i440FX"
"/main-system-bus/pci.0,addr=01.0/PIIX3"
"/main-system-bus/pci.0,addr=02.0/cirrus-vga"
"/main-system-bus/pci.0/isa.0/mc146818rtc"
"/main-system-bus/pci.0/isa.0/isa-serial"
"/main-system-bus/pci.0/isa.0/i8042"
"/main-system-bus/pci.0/isa.0/isa-fdc"
"/main-system-bus/pci.0,addr=03.0/i82551,mac=52:54:00:12:34:56"
"/main-system-bus/pci.0,addr=04.0/virtio-net-pci,mac=52:54:00:12:34:57"
"/main-system-bus/pci.0,addr=05.0/e1000,mac=52:54:00:12:34:58"
"/main-system-bus/pci.0,addr=06.0/rtl8139,mac=52:54:00:12:34:59"
"/main-system-bus/pci.0,addr=07.0/pcnet,mac=52:54:00:12:34:5a"
"/main-system-bus/pci.0,addr=01.1/piix3-ide"
"/main-system-bus/pci.0,addr=01.3/PIIX4_PM"
"/main-system-bus/pci.0,addr=08.0/lsi53c895a"
"/main-system-bus/pci.0,addr=09.0/virtio-blk-pci"

There are two primary targets for these strings.  The first is vmsave, where
we currently use a device provided string plus instance number to track
SaveStateEntries.  This fails when we introduce device hotplug, particularly
in a case were we have gaps in the instance numbers that cannot easily be
reproduced on a migration target.  The second is for naming RAMBlocks.  For
these, we would like a string that matches the vmstate string.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/qdev-properties.c |    2 ++
 hw/qdev.c            |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    5 ++++
 3 files changed, 64 insertions(+), 0 deletions(-)
Markus Armbruster - June 14, 2010, 6:39 a.m.
Alex Williamson <alex.williamson@redhat.com> writes:

> qdev_get_dev_path() is intended to be the canonical utility for creating
> a string representing the qdev hierarchy of a device.  The path consists
> of bus and device names as well as identified properties of the immediate
> parent bus and device.  This results in strings like:
>
> "/main-system-bus/pci.0,addr=00.0/i440FX"
> "/main-system-bus/pci.0,addr=01.0/PIIX3"
> "/main-system-bus/pci.0,addr=02.0/cirrus-vga"
> "/main-system-bus/pci.0/isa.0/mc146818rtc"
> "/main-system-bus/pci.0/isa.0/isa-serial"
> "/main-system-bus/pci.0/isa.0/i8042"
> "/main-system-bus/pci.0/isa.0/isa-fdc"
> "/main-system-bus/pci.0,addr=03.0/i82551,mac=52:54:00:12:34:56"
> "/main-system-bus/pci.0,addr=04.0/virtio-net-pci,mac=52:54:00:12:34:57"
> "/main-system-bus/pci.0,addr=05.0/e1000,mac=52:54:00:12:34:58"
> "/main-system-bus/pci.0,addr=06.0/rtl8139,mac=52:54:00:12:34:59"
> "/main-system-bus/pci.0,addr=07.0/pcnet,mac=52:54:00:12:34:5a"
> "/main-system-bus/pci.0,addr=01.1/piix3-ide"
> "/main-system-bus/pci.0,addr=01.3/PIIX4_PM"
> "/main-system-bus/pci.0,addr=08.0/lsi53c895a"
> "/main-system-bus/pci.0,addr=09.0/virtio-blk-pci"
>
> There are two primary targets for these strings.  The first is vmsave, where
> we currently use a device provided string plus instance number to track
> SaveStateEntries.  This fails when we introduce device hotplug, particularly
> in a case were we have gaps in the instance numbers that cannot easily be
> reproduced on a migration target.  The second is for naming RAMBlocks.  For
> these, we would like a string that matches the vmstate string.

Could you explain why you add "identified properties of the immediate
parent bus and device"?  They make the result ver much *not* a "dev
path" in the qdev sense...
Alex Williamson - June 14, 2010, 12:52 p.m.
On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > qdev_get_dev_path() is intended to be the canonical utility for creating
> > a string representing the qdev hierarchy of a device.  The path consists
> > of bus and device names as well as identified properties of the immediate
> > parent bus and device.  This results in strings like:
> >
> > "/main-system-bus/pci.0,addr=00.0/i440FX"
> > "/main-system-bus/pci.0,addr=01.0/PIIX3"
> > "/main-system-bus/pci.0,addr=02.0/cirrus-vga"
> > "/main-system-bus/pci.0/isa.0/mc146818rtc"
> > "/main-system-bus/pci.0/isa.0/isa-serial"
> > "/main-system-bus/pci.0/isa.0/i8042"
> > "/main-system-bus/pci.0/isa.0/isa-fdc"
> > "/main-system-bus/pci.0,addr=03.0/i82551,mac=52:54:00:12:34:56"
> > "/main-system-bus/pci.0,addr=04.0/virtio-net-pci,mac=52:54:00:12:34:57"
> > "/main-system-bus/pci.0,addr=05.0/e1000,mac=52:54:00:12:34:58"
> > "/main-system-bus/pci.0,addr=06.0/rtl8139,mac=52:54:00:12:34:59"
> > "/main-system-bus/pci.0,addr=07.0/pcnet,mac=52:54:00:12:34:5a"
> > "/main-system-bus/pci.0,addr=01.1/piix3-ide"
> > "/main-system-bus/pci.0,addr=01.3/PIIX4_PM"
> > "/main-system-bus/pci.0,addr=08.0/lsi53c895a"
> > "/main-system-bus/pci.0,addr=09.0/virtio-blk-pci"
> >
> > There are two primary targets for these strings.  The first is vmsave, where
> > we currently use a device provided string plus instance number to track
> > SaveStateEntries.  This fails when we introduce device hotplug, particularly
> > in a case were we have gaps in the instance numbers that cannot easily be
> > reproduced on a migration target.  The second is for naming RAMBlocks.  For
> > these, we would like a string that matches the vmstate string.
> 
> Could you explain why you add "identified properties of the immediate
> parent bus and device"?  They make the result ver much *not* a "dev
> path" in the qdev sense...

In order to try to get a unique string.  Without looking into device
properties, two e1000s would both be:

/main-system-bus/pci.0/e1000
/main-system-bus/pci.0/e1000

Which is no better than simply "e1000" and would require us to fall back
to instance ids again.  The goal here is that anything that makes use of
passing a dev when registering a vmstate gets an instance id of zero.

Alex
Jan Kiszka - June 14, 2010, 1 p.m.
Alex Williamson wrote:
> On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
>> Alex Williamson <alex.williamson@redhat.com> writes:
>>
>>> qdev_get_dev_path() is intended to be the canonical utility for creating
>>> a string representing the qdev hierarchy of a device.  The path consists
>>> of bus and device names as well as identified properties of the immediate
>>> parent bus and device.  This results in strings like:
>>>
>>> "/main-system-bus/pci.0,addr=00.0/i440FX"
>>> "/main-system-bus/pci.0,addr=01.0/PIIX3"
>>> "/main-system-bus/pci.0,addr=02.0/cirrus-vga"
>>> "/main-system-bus/pci.0/isa.0/mc146818rtc"
>>> "/main-system-bus/pci.0/isa.0/isa-serial"
>>> "/main-system-bus/pci.0/isa.0/i8042"
>>> "/main-system-bus/pci.0/isa.0/isa-fdc"
>>> "/main-system-bus/pci.0,addr=03.0/i82551,mac=52:54:00:12:34:56"
>>> "/main-system-bus/pci.0,addr=04.0/virtio-net-pci,mac=52:54:00:12:34:57"
>>> "/main-system-bus/pci.0,addr=05.0/e1000,mac=52:54:00:12:34:58"
>>> "/main-system-bus/pci.0,addr=06.0/rtl8139,mac=52:54:00:12:34:59"
>>> "/main-system-bus/pci.0,addr=07.0/pcnet,mac=52:54:00:12:34:5a"
>>> "/main-system-bus/pci.0,addr=01.1/piix3-ide"
>>> "/main-system-bus/pci.0,addr=01.3/PIIX4_PM"
>>> "/main-system-bus/pci.0,addr=08.0/lsi53c895a"
>>> "/main-system-bus/pci.0,addr=09.0/virtio-blk-pci"
>>>
>>> There are two primary targets for these strings.  The first is vmsave, where
>>> we currently use a device provided string plus instance number to track
>>> SaveStateEntries.  This fails when we introduce device hotplug, particularly
>>> in a case were we have gaps in the instance numbers that cannot easily be
>>> reproduced on a migration target.  The second is for naming RAMBlocks.  For
>>> these, we would like a string that matches the vmstate string.
>> Could you explain why you add "identified properties of the immediate
>> parent bus and device"?  They make the result ver much *not* a "dev
>> path" in the qdev sense...
> 
> In order to try to get a unique string.  Without looking into device
> properties, two e1000s would both be:
> 
> /main-system-bus/pci.0/e1000
> /main-system-bus/pci.0/e1000
> 
> Which is no better than simply "e1000" and would require us to fall back
> to instance ids again.  The goal here is that anything that makes use of
> passing a dev when registering a vmstate gets an instance id of zero.

Will soon (re-)post a patch that adds per-bus instance numbers to deal
with identically named devices. That's required as not all buses have
canonical node IDs (e.g. ISA or the main system bus).

Jan
Paul Brook - June 14, 2010, 1:09 p.m.
> > > "/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

> > Could you explain why you add "identified properties of the immediate
> > parent bus and device"?  They make the result ver much *not* a "dev
> > path" in the qdev sense...
> 
> In order to try to get a unique string.  Without looking into device
> properties, two e1000s would both be:
> 
> /main-system-bus/pci.0/e1000
> /main-system-bus/pci.0/e1000
> 
> Which is no better than simply "e1000" and would require us to fall back
> to instance ids again.  The goal here is that anything that makes use of
> passing a dev when registering a vmstate gets an instance id of zero.

You already got the information you need, you just put it in the wrong place. 
The canonical ID for the device could be its bus address. In practice we'd 
probably want to allow the user to specify it by name, provided these are 
unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
as an aias for [...]:_09.0. Device names have a restricted namespace, so we 
can use an initial prefix to disambiguate a name/label from a bus address.

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).

Paul
Alex Williamson - June 14, 2010, 3:29 p.m.
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

to expand the previous output.  Code below.

> > > Could you explain why you add "identified properties of the immediate
> > > parent bus and device"?  They make the result ver much *not* a "dev
> > > path" in the qdev sense...
> > 
> > In order to try to get a unique string.  Without looking into device
> > properties, two e1000s would both be:
> > 
> > /main-system-bus/pci.0/e1000
> > /main-system-bus/pci.0/e1000
> > 
> > Which is no better than simply "e1000" and would require us to fall back
> > to instance ids again.  The goal here is that anything that makes use of
> > passing a dev when registering a vmstate gets an instance id of zero.
> 
> You already got the information you need, you just put it in the wrong place. 
> The canonical ID for the device could be its bus address. In practice we'd 
> probably want to allow the user to specify it by name, provided these are 
> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
> as an aias for [...]:_09.0.

Sure, if there was a guaranteed unique char* on a DeviceState that was
consistent between guest invocations, we could just use that instead.  I
suppose all devices could have a global system id property and if that
was present we'd use that instead of creating the device path.

> Device names have a restricted namespace, so we 
> can use an initial prefix to disambiguate a name/label from a bus address.

I'm not sure it's necessary.  It seems like it would only be necessary
to differentiate the two if we wanted to translate between namespaces.
But I think it's reasonable to require that if a global name is
provided, it must always be provided for the life of the VM.

> 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,

Alex

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

static int qdev_strprint_path_props(DeviceState *dev, Property *props,
                                    char *buf, size_t len)
{
    int offset = 0;
    char pbuf[64];

    if (!props)
        return 0;

    while (props->name) {
        if (props->info->flags & PROP_FLAG_PATH) {
            if (props->info->print) {
                props->info->print(dev, props, pbuf, sizeof(pbuf));
                offset += snprintf(buf + offset, len - offset, "/_%s", pbuf);
            }
        }
        props++;
    }
    return offset;
}

static int qdev_strprint_dev(DeviceState *dev, char *buf, size_t len);

static int qdev_strprint_bus(DeviceState *dev, char *buf, size_t len)
{
    int offset = 0;

    if (dev->parent_bus->parent)
        offset += qdev_strprint_dev(dev->parent_bus->parent, buf, len);

    offset += snprintf(buf + offset, len - offset, "/%s/%s",
                       dev->parent_bus->info->name, dev->parent_bus->name);

    offset += qdev_strprint_path_props(dev, dev->parent_bus->info->props,
                                       buf + offset, len - offset);

    return offset;
}

static int qdev_strprint_dev(DeviceState *dev, char *buf, size_t len)
{
    int offset = 0;

    if (dev->parent_bus)
        offset += qdev_strprint_bus(dev, buf, len);

    offset += snprintf(buf + offset, len - offset, "/%s", dev->info->name);

    offset += qdev_strprint_path_props(dev, dev->info->props,
                                       buf + offset, len - offset);
    if (dev->id)
        offset += snprintf(buf + offset, len - offset, "/%s", dev->id);

    return offset;

}

char *qdev_get_dev_path(DeviceState *dev)
{
    char buf[256] = "";

    if (!dev)
        return NULL;

    qdev_strprint_dev(dev, buf, sizeof(buf));
    return qemu_strdup(buf);
}
Paul Brook - June 14, 2010, 3:42 p.m.
> 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/virti
> o-blk

No. Now you've got way to many elements in the path.  My point is that you 
replace the name of the device with the bus address of the device.

However you determine the element names the path should be /busname/devicename 
pairs. I'm undecided whether the root element should be the main system bus, 
or a system device node that contains the main system bus.

> > You already got the information you need, you just put it in the wrong
> > place. The canonical ID for the device could be its bus address. In
> > practice we'd probably want to allow the user to specify it by name,
> > provided these are unique. e.g. in the above machine we could accept
> > [...]/virtiio-blk-pci would as an aias for [...]:_09.0.
> 
> Sure, if there was a guaranteed unique char* on a DeviceState that was
> consistent between guest invocations, we could just use that instead.  I
> suppose all devices could have a global system id property and if that
> was present we'd use that instead of creating the device path.

All we require is some way of uniquely addressing each device on the bus. For 
PCI that's trivial (devfn). For other busses we get to pick something 
appropriate.
 
> > Device names have a restricted namespace, so we
> > can use an initial prefix to disambiguate a name/label from a bus
> > address.
> 
> I'm not sure it's necessary.  It seems like it would only be necessary
> to differentiate the two if we wanted to translate between namespaces.
> But I think it's reasonable to require that if a global name is
> provided, it must always be provided for the life of the VM.

I don't think requiring that all devices are given a globally unique name is 
going to fly. locally (per-bus) unique user-specified are still a PITA, but 
may be acceptable. Having a canonical addressing system that's independent of 
user assigned IDs seems like a good thing.

> > 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,

Simple consecutive instance IDs are inherently unstable. They depend on teh 
order of device creation. The ID really wants to be something that can be 
reliably determined from the device bus itself (and/or its interaction with 
its parent bus).

Paul
Jan Kiszka - June 14, 2010, 4 p.m.
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. 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).

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>.

> 
> to expand the previous output.  Code below.
> 
>>>> Could you explain why you add "identified properties of the immediate
>>>> parent bus and device"?  They make the result ver much *not* a "dev
>>>> path" in the qdev sense...
>>> In order to try to get a unique string.  Without looking into device
>>> properties, two e1000s would both be:
>>>
>>> /main-system-bus/pci.0/e1000
>>> /main-system-bus/pci.0/e1000
>>>
>>> Which is no better than simply "e1000" and would require us to fall back
>>> to instance ids again.  The goal here is that anything that makes use of
>>> passing a dev when registering a vmstate gets an instance id of zero.
>> You already got the information you need, you just put it in the wrong place. 
>> The canonical ID for the device could be its bus address. In practice we'd 
>> probably want to allow the user to specify it by name, provided these are 
>> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
>> as an aias for [...]:_09.0.
> 
> Sure, if there was a guaranteed unique char* on a DeviceState that was
> consistent between guest invocations, we could just use that instead.  I
> suppose all devices could have a global system id property and if that
> was present we'd use that instead of creating the device path.
> 
>> Device names have a restricted namespace, so we 
>> can use an initial prefix to disambiguate a name/label from a bus address.
> 
> I'm not sure it's necessary.  It seems like it would only be necessary
> to differentiate the two if we wanted to translate between namespaces.
> But I think it's reasonable to require that if a global name is
> provided, it must always be provided for the life of the VM.
> 
>> 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.

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

Jan
Alex Williamson - June 14, 2010, 4:38 p.m.
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.)

> >> 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.

> 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,

Alex
Jan Kiszka - June 14, 2010, 4:49 p.m.
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
Alex Williamson - June 14, 2010, 6:35 p.m.
On Mon, 2010-06-14 at 18:49 +0200, Jan Kiszka wrote:
> Alex Williamson wrote:
> > On Mon, 2010-06-14 at 18:00 +0200, Jan Kiszka wrote: 
> >> 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.

Ok, I can get it down to something like:

/i440FX-pcihost/pci.0/virtio-blk-pci,09.0

The addr on the device is initially a little non-intuitive to me since
it's a property of the bus, but I guess it make sense if we think of
that level as slot, which includes an address and driver.

> >>>> 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.

Yep, I share that concern, which is I say instance numbers aren't always
bad.  If we have devices that we don't care about doing hotplug on, we
can live with instance numbers.  Thanks,

Alex
Paul Brook - June 14, 2010, 9:43 p.m.
> On Mon, 2010-06-14 at 18:49 +0200, Jan Kiszka wrote:
> > Alex Williamson wrote:
> > > On Mon, 2010-06-14 at 18:00 +0200, Jan Kiszka wrote:
> > >> 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.
> 
> Ok, I can get it down to something like:
> 
> /i440FX-pcihost/pci.0/virtio-blk-pci,09.0
> 
> The addr on the device is initially a little non-intuitive to me since
> it's a property of the bus, but I guess it make sense if we think of
> that level as slot, which includes an address and driver.

That indicates you're thinking about things the wrong way.
The point of this path is to uniquely identify an entity.

/i440FX-pcihost/pci0 identifies a PCI bus attached to the i440FX-pcihost 
device. To identify a device attached to that bus all you need to know is the 
devfn of the device.

For an end-user it may be helpful to allow devices to be identified by the 
device type (assuming only one device of a particular type on that bus), or 
specify the device type as a crude error checking mechanism. However we're 
talking about canonical addresses. These need not include the device type. 
Verifying that the device is actually what you expect is a separate problem, 
and the device type is not sufficient for that.

i.e. /i440FX-pcihost/pci.0/,09.0 Is an appropriate canonical address.
 
> > > 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.

That's a feature. If you start with two NICs and remove the first, the chances 
are that the second will be in a different place to the nice created in a 
single-nic config. Allowing migration between these two will cause a PCI 
device to suddenly change location. This is not physically or logically 
possible, and everyone looses.

Hot-removing a nic and then hotplugging a new nic in the same location may 
result in something that is reasonable to migrate.

Paul
Alex Williamson - June 14, 2010, 10:11 p.m.
On Mon, 2010-06-14 at 22:43 +0100, Paul Brook wrote:
> > On Mon, 2010-06-14 at 18:49 +0200, Jan Kiszka wrote:
> > > Alex Williamson wrote:
> > Ok, I can get it down to something like:
> > 
> > /i440FX-pcihost/pci.0/virtio-blk-pci,09.0
> > 
> > The addr on the device is initially a little non-intuitive to me since
> > it's a property of the bus, but I guess it make sense if we think of
> > that level as slot, which includes an address and driver.
> 
> That indicates you're thinking about things the wrong way.
> The point of this path is to uniquely identify an entity.
> 
> /i440FX-pcihost/pci0 identifies a PCI bus attached to the i440FX-pcihost 
> device. To identify a device attached to that bus all you need to know is the 
> devfn of the device.

Hmm, I think that identifies where the device is, not what the device
is.  It's more helpful to say "the e1000 in slot 7" than it is to say
"the device in slot 7".

> For an end-user it may be helpful to allow devices to be identified by the 
> device type (assuming only one device of a particular type on that bus), or 
> specify the device type as a crude error checking mechanism. However we're 
> talking about canonical addresses. These need not include the device type. 
> Verifying that the device is actually what you expect is a separate problem, 
> and the device type is not sufficient for that.
> 
> i.e. /i440FX-pcihost/pci.0/,09.0 Is an appropriate canonical address.

We seem to keep introducing new problems, and I'm not sure this one
exists.  If I drop the device name/type and use only the devfn, then
what's to prevent the /e1000,09.0/rom (/,09.0/rom) from being stuffed
into the /rtl8139,09.0/rom (/,09.0/rom) on a migration?  (or we match it
to /09.0/savevm when trying to migrate state)  We can argue that "e1000"
isn't a sufficient identifier, but I can't think of a case where it'd
fail.

> > > > 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.
> 
> That's a feature. If you start with two NICs and remove the first, the chances 
> are that the second will be in a different place to the nice created in a 
> single-nic config. Allowing migration between these two will cause a PCI 
> device to suddenly change location. This is not physically or logically 
> possible, and everyone looses.

If the BAR addresses don't follow the VM when it's migrated, that's
another bug that needs to be fixed, but we can't get there until we at
least have some infrastructure in place to make that bug possible.

> Hot-removing a nic and then hotplugging a new nic in the same location may 
> result in something that is reasonable to migrate.

It might... it might not.  I'd rather make it work than try to document
the restrictions.

Alex
Paul Brook - June 14, 2010, 10:46 p.m.
> > > Ok, I can get it down to something like:
> > > 
> > > /i440FX-pcihost/pci.0/virtio-blk-pci,09.0
> > > 
> > > The addr on the device is initially a little non-intuitive to me since
> > > it's a property of the bus, but I guess it make sense if we think of
> > > that level as slot, which includes an address and driver.
> > 
> > That indicates you're thinking about things the wrong way.
> > The point of this path is to uniquely identify an entity.
> > 
> > /i440FX-pcihost/pci0 identifies a PCI bus attached to the i440FX-pcihost
> > device. To identify a device attached to that bus all you need to know is
> > the devfn of the device.
> 
> Hmm, I think that identifies where the device is, not what the device
> is.  It's more helpful to say "the e1000 in slot 7" than it is to say
> "the device in slot 7".

Why is this more useful? Canonical addresses should not be helpful. They 
should identify entities within a machine that is already known to be 
consistent. Making them "helpful" just makes them more volatile.
 
> > For an end-user it may be helpful to allow devices to be identified by
> > the device type (assuming only one device of a particular type on that
> > bus), or specify the device type as a crude error checking mechanism.
> > However we're talking about canonical addresses. These need not include
> > the device type. Verifying that the device is actually what you expect
> > is a separate problem, and the device type is not sufficient for that.
> > 
> > i.e. /i440FX-pcihost/pci.0/,09.0 Is an appropriate canonical address.
> 
> We seem to keep introducing new problems, and I'm not sure this one
> exists.  If I drop the device name/type and use only the devfn, then
> what's to prevent the /e1000,09.0/rom (/,09.0/rom) from being stuffed
> into the /rtl8139,09.0/rom (/,09.0/rom) on a migration?  (or we match it
> to /09.0/savevm when trying to migrate state)  We can argue that "e1000"
> isn't a sufficient identifier, but I can't think of a case where it'd
> fail.

The migration code needs to check that the devices are actually compatible. 
I'd expect this to require much more than just the device name. What you 
actually need is more like "An e1000 with 64k eeprom, fast ethernet PHY, and 
frobnitz B". In fact what you really want to do is transfer the device tree 
(including properties), and create the machine from scratch, not load state 
into a pre-supplied device tree.

> > > > > 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.
> > 
> > That's a feature. If you start with two NICs and remove the first, the
> > chances are that the second will be in a different place to the nice
> > created in a single-nic config. Allowing migration between these two
> > will cause a PCI device to suddenly change location. This is not
> > physically or logically possible, and everyone looses.
> 
> If the BAR addresses don't follow the VM when it's migrated, that's
> another bug that needs to be fixed, but we can't get there until we at
> least have some infrastructure in place to make that bug possible.

Not BAR addresses, the actual PCI device addresses. Devices on the PCI bus are 
addressed by device and function.  This is guest visible.  The device part of 
this address corresponds to the physical slot, which typically effects IRQ 
routing (amongst other things).  If you arbitrarily move a device from slot A 
to slot B then this will have catastrophic effects on a running machine.

Paul
Alex Williamson - June 15, 2010, 1:14 a.m.
On Mon, 2010-06-14 at 23:46 +0100, Paul Brook wrote:
> > > > Ok, I can get it down to something like:
> > > > 
> > > > /i440FX-pcihost/pci.0/virtio-blk-pci,09.0
> > > > 
> > > > The addr on the device is initially a little non-intuitive to me since
> > > > it's a property of the bus, but I guess it make sense if we think of
> > > > that level as slot, which includes an address and driver.
> > > 
> > > That indicates you're thinking about things the wrong way.
> > > The point of this path is to uniquely identify an entity.
> > > 
> > > /i440FX-pcihost/pci0 identifies a PCI bus attached to the i440FX-pcihost
> > > device. To identify a device attached to that bus all you need to know is
> > > the devfn of the device.
> > 
> > Hmm, I think that identifies where the device is, not what the device
> > is.  It's more helpful to say "the e1000 in slot 7" than it is to say
> > "the device in slot 7".
> 
> Why is this more useful? Canonical addresses should not be helpful. They 
> should identify entities within a machine that is already known to be 
> consistent. Making them "helpful" just makes them more volatile.

Being able to check that device 09.0 is attached to the e1000 driver on
source and the rtl8139 driver on the target seems pretty useful to me.
 
> > > For an end-user it may be helpful to allow devices to be identified by
> > > the device type (assuming only one device of a particular type on that
> > > bus), or specify the device type as a crude error checking mechanism.
> > > However we're talking about canonical addresses. These need not include
> > > the device type. Verifying that the device is actually what you expect
> > > is a separate problem, and the device type is not sufficient for that.
> > > 
> > > i.e. /i440FX-pcihost/pci.0/,09.0 Is an appropriate canonical address.
> > 
> > We seem to keep introducing new problems, and I'm not sure this one
> > exists.  If I drop the device name/type and use only the devfn, then
> > what's to prevent the /e1000,09.0/rom (/,09.0/rom) from being stuffed
> > into the /rtl8139,09.0/rom (/,09.0/rom) on a migration?  (or we match it
> > to /09.0/savevm when trying to migrate state)  We can argue that "e1000"
> > isn't a sufficient identifier, but I can't think of a case where it'd
> > fail.
> 
> The migration code needs to check that the devices are actually compatible. 
> I'd expect this to require much more than just the device name. What you 
> actually need is more like "An e1000 with 64k eeprom, fast ethernet PHY, and 
> frobnitz B". 

No, that's savevm's problem, and it's perfectly capable of doing it via
the version ids.  We're not using the driver name to describe just any
random e1000, it's the one that the e1000 driver created, version foo.
If it added a frobnitz in version foo+1, either that savevm knows how to
import a version foo or rejects it.

> In fact what you really want to do is transfer the device tree 
> (including properties), and create the machine from scratch, not load state 
> into a pre-supplied device tree.

Well, I agree, but that's a lot more of an overhaul, and once again
we're changing the problem.

> > > > > > 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.
> > > 
> > > That's a feature. If you start with two NICs and remove the first, the
> > > chances are that the second will be in a different place to the nice
> > > created in a single-nic config. Allowing migration between these two
> > > will cause a PCI device to suddenly change location. This is not
> > > physically or logically possible, and everyone looses.
> > 
> > If the BAR addresses don't follow the VM when it's migrated, that's
> > another bug that needs to be fixed, but we can't get there until we at
> > least have some infrastructure in place to make that bug possible.
> 
> Not BAR addresses, the actual PCI device addresses. Devices on the PCI bus are 
> addressed by device and function.  This is guest visible.  The device part of 
> this address corresponds to the physical slot, which typically effects IRQ 
> routing (amongst other things).  If you arbitrarily move a device from slot A 
> to slot B then this will have catastrophic effects on a running machine.

Sorry, I jumped to BARs because the PCI device address mismatch is kinda
the point of where I'm going.  With these changes, we're at least
allowing that a smart enough management tool is actually able to create
a VM state to match a source instance that has done hotplug operations.
As it is today, I don't think it's possible to migrate a VM that has
gaps in it's savevm instance ids.

Alex
Markus Armbruster - June 15, 2010, 8:47 a.m.
Paul Brook <paul@codesourcery.com> writes:

> Alex Williamson <alex.williamson@redhat.com> writes:
>
>> On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
>> > Could you explain why you add "identified properties of the immediate
>> > parent bus and device"?  They make the result ver much *not* a "dev
>> > path" in the qdev sense...
>> 
>> In order to try to get a unique string.  Without looking into device
>> properties, two e1000s would both be:
>> 
>> /main-system-bus/pci.0/e1000
>> /main-system-bus/pci.0/e1000
>> 
>> Which is no better than simply "e1000" and would require us to fall back
>> to instance ids again.  The goal here is that anything that makes use of
>> passing a dev when registering a vmstate gets an instance id of zero.
>
> You already got the information you need, you just put it in the wrong place. 
> The canonical ID for the device could be its bus address. In practice we'd 
> probably want to allow the user to specify it by name, provided these are 
> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
> as an aias for [...]:_09.0. Device names have a restricted namespace, so we 
> can use an initial prefix to disambiguate a name/label from a bus address.
>
> 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).

When that's inconvenient or impossible, we can still punt to user: make
device ID mandatory.


We obviously need a way to unambigously name a device.  It's okay to
have multiple names for the same device.

If the device has a device ID, that's an unambigous name.

qdev paths may be ambigous when path components are resolved to driver
names instead of IDs.

Alex proposed to disambiguate by adding "identified properties of the
immediate parent bus and device" to the path component.  For PCI, these
are dev.fn.  Likewise for any other bus where devices have unambigous
bus address.  The driver name carries no information!

For other buses, we need to make something up.

Note that addressing by bus address rather than name is generally
useful, not just in the context of savevm.  For instance, I'd appreciate
being able to say something like "device_del pci.0/04.0".

An easy way to get that is to reserve part of the name space for bus
addresses.  If the path component starts with a letter, it's an ID or
driver name.  If it starts with say '@', it's a bus address in
bus-specific syntax.  The bus provides a method to look it up.

That way, we gain a useful feature, and avoid having an savevm-specific
"device path" that isn't recognized anywhere else.
Jan Kiszka - June 15, 2010, 9:34 a.m.
Markus Armbruster wrote:
> Paul Brook <paul@codesourcery.com> writes:
> 
>> Alex Williamson <alex.williamson@redhat.com> writes:
>>
>>> On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
>>>> Could you explain why you add "identified properties of the immediate
>>>> parent bus and device"?  They make the result ver much *not* a "dev
>>>> path" in the qdev sense...
>>> In order to try to get a unique string.  Without looking into device
>>> properties, two e1000s would both be:
>>>
>>> /main-system-bus/pci.0/e1000
>>> /main-system-bus/pci.0/e1000
>>>
>>> Which is no better than simply "e1000" and would require us to fall back
>>> to instance ids again.  The goal here is that anything that makes use of
>>> passing a dev when registering a vmstate gets an instance id of zero.
>> You already got the information you need, you just put it in the wrong place. 
>> The canonical ID for the device could be its bus address. In practice we'd 
>> probably want to allow the user to specify it by name, provided these are 
>> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
>> as an aias for [...]:_09.0. Device names have a restricted namespace, so we 
>> can use an initial prefix to disambiguate a name/label from a bus address.
>>
>> 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).
> 
> When that's inconvenient or impossible, we can still punt to user: make
> device ID mandatory.

No option due to auto-created devices. And auto-generating IDs would
just create usability issues.

> 
> 
> We obviously need a way to unambigously name a device.  It's okay to
> have multiple names for the same device.
> 
> If the device has a device ID, that's an unambigous name.
> 
> qdev paths may be ambigous when path components are resolved to driver
> names instead of IDs.
> 
> Alex proposed to disambiguate by adding "identified properties of the
> immediate parent bus and device" to the path component.  For PCI, these
> are dev.fn.  Likewise for any other bus where devices have unambigous
> bus address.  The driver name carries no information!

From user POV, driver names are very handly to address a device
intuitively - except for the case you have tones of devices on the same
bus that are handled by the same driver. For that case we need to
augment the device name with a useful per-bus ID, derived from the bus
address where available, otherwise based on instance numbers.

> 
> For other buses, we need to make something up.
> 
> Note that addressing by bus address rather than name is generally
> useful, not just in the context of savevm.  For instance, I'd appreciate
> being able to say something like "device_del pci.0/04.0".

And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
the bus first before you can identify which device you want to remove.

> 
> An easy way to get that is to reserve part of the name space for bus
> addresses.  If the path component starts with a letter, it's an ID or
> driver name.  If it starts with say '@', it's a bus address in
> bus-specific syntax.  The bus provides a method to look it up.

I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
set for buses that implement some to-be-defined device addressing
service, the latter is the default on buses where that service is not
available.

> 
> That way, we gain a useful feature, and avoid having an savevm-specific
> "device path" that isn't recognized anywhere else.

Agreed, we should find one solution for all use cases.

Jan
Paul Brook - June 15, 2010, 11:24 a.m.
> > In fact what you really want to do is transfer the device tree
> > (including properties), and create the machine from scratch, not load
> > state into a pre-supplied device tree.
> 
> Well, I agree, but that's a lot more of an overhaul, and once again
> we're changing the problem.

I think it's you that's changing the problem.
The requirement is to uniquely identify a device within a machine.
Verifying that this device is that compatible with the device at the same 
address in a different machine is a separate problem. We should not be trying 
to encode this information in the canonical device path.

Paul
Paul Brook - June 15, 2010, 11:28 a.m.
> > Alex proposed to disambiguate by adding "identified properties of the
> > immediate parent bus and device" to the path component.  For PCI, these
> > are dev.fn.  Likewise for any other bus where devices have unambigous
> > bus address.  The driver name carries no information!
> 
> From user POV, driver names are very handly to address a device
> intuitively - except for the case you have tones of devices on the same
> bus that are handled by the same driver. For that case we need to
> augment the device name with a useful per-bus ID, derived from the bus
> address where available, otherwise based on instance numbers.

This is where I think you're missing a trick. We don't need to augment the 
name, we just need to allow the bus id to be used instead.
 
> > For other buses, we need to make something up.
> > 
> > Note that addressing by bus address rather than name is generally
> > useful, not just in the context of savevm.  For instance, I'd appreciate
> > being able to say something like "device_del pci.0/04.0".
> 
> And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
> the bus first before you can identify which device you want to remove.

We can allow both.

A bus address is sufficient to uniquely identify a device.  I see no reason to 
require the driver name,  or to include it in the canonical device address.

> > An easy way to get that is to reserve part of the name space for bus
> > addresses.  If the path component starts with a letter, it's an ID or
> > driver name.  If it starts with say '@', it's a bus address in
> > bus-specific syntax.  The bus provides a method to look it up.
> 
> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
> set for buses that implement some to-be-defined device addressing
> service, the latter is the default on buses where that service is not
> available.

If we have bus-address then I see no good reason to also add instance-no.
For busses that no natural address, we can define the address to be an 
instance number.

> > That way, we gain a useful feature, and avoid having an savevm-specific
> > "device path" that isn't recognized anywhere else.
> 
> Agreed, we should find one solution for all use cases.

I wasn't aware that there was any suggestion of a separate savevm-specific 
path.  The whole point of a device path is to uniquely identify a device 
within a machine. There may be many different paths that identify the same 
device.  When given a device and asked to generate  path, the result should be 
the canonical address.  IMO this should be the least volatile, and avoid 
redundant information.

Paul
Markus Armbruster - June 15, 2010, 11:42 a.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Markus Armbruster wrote:
>> Paul Brook <paul@codesourcery.com> writes:
>> 
>>> Alex Williamson <alex.williamson@redhat.com> writes:
>>>
>>>> On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
>>>>> Could you explain why you add "identified properties of the immediate
>>>>> parent bus and device"?  They make the result ver much *not* a "dev
>>>>> path" in the qdev sense...
>>>> In order to try to get a unique string.  Without looking into device
>>>> properties, two e1000s would both be:
>>>>
>>>> /main-system-bus/pci.0/e1000
>>>> /main-system-bus/pci.0/e1000
>>>>
>>>> Which is no better than simply "e1000" and would require us to fall back
>>>> to instance ids again.  The goal here is that anything that makes use of
>>>> passing a dev when registering a vmstate gets an instance id of zero.
>>> You already got the information you need, you just put it in the wrong place. 
>>> The canonical ID for the device could be its bus address. In practice we'd 
>>> probably want to allow the user to specify it by name, provided these are 
>>> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
>>> as an aias for [...]:_09.0. Device names have a restricted namespace, so we 
>>> can use an initial prefix to disambiguate a name/label from a bus address.
>>>
>>> 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).
>> 
>> When that's inconvenient or impossible, we can still punt to user: make
>> device ID mandatory.
>
> No option due to auto-created devices. And auto-generating IDs would
> just create usability issues.

Auto-generated IDs would become part of the ABI.  Really so bad that
it's "no option"?  Mind, device ID becomes mandatory *only* for devices
that don't have a useful bus address.  We could even waive the ID
requirement for the first device of a kind, i.e. require ID if and only
if it's needed to disambiguate.

>> We obviously need a way to unambigously name a device.  It's okay to
>> have multiple names for the same device.
>> 
>> If the device has a device ID, that's an unambigous name.
>> 
>> qdev paths may be ambigous when path components are resolved to driver
>> names instead of IDs.
>> 
>> Alex proposed to disambiguate by adding "identified properties of the
>> immediate parent bus and device" to the path component.  For PCI, these
>> are dev.fn.  Likewise for any other bus where devices have unambigous
>> bus address.  The driver name carries no information!
>
>>From user POV, driver names are very handly to address a device
> intuitively - except for the case you have tones of devices on the same
> bus that are handled by the same driver. For that case we need to
> augment the device name with a useful per-bus ID, derived from the bus
> address where available, otherwise based on instance numbers.

I'm not arguing against the use of driver names at all.

>> For other buses, we need to make something up.
>> 
>> Note that addressing by bus address rather than name is generally
>> useful, not just in the context of savevm.  For instance, I'd appreciate
>> being able to say something like "device_del pci.0/04.0".
>
> And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
> the bus first before you can identify which device you want to remove.

It's not either/or.  Addressing by ID continues to work.  Addressing by
bus/driver-name continues to work.  We merely add addressing by
bus/@bus-address.

>> An easy way to get that is to reserve part of the name space for bus
>> addresses.  If the path component starts with a letter, it's an ID or
>> driver name.  If it starts with say '@', it's a bus address in
>> bus-specific syntax.  The bus provides a method to look it up.
>
> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
> set for buses that implement some to-be-defined device addressing
> service, the latter is the default on buses where that service is not
> available.

I object to <driver>@<bus-address>, because the <driver> part carries no
information.

Not the case for <driver>.<instance-no>.  We still need a suitable
definition of <instance-no>.  Possible definitions:

* n-th creation of a <driver> device.  Drawbacks: depends on creation
  order.  Relatively hard to maintain across migration.

* n-th instance of a <driver> device.  Drawback: changes on unplug.
  Good enough for interactive use, but it doesn't provide a stable
  device name.

When counting <driver> devices either way, we can count per bus or
globally.  I prefer per bus.

None of the above instance numbers are nearly as neat as bus addresses.

>> That way, we gain a useful feature, and avoid having an savevm-specific
>> "device path" that isn't recognized anywhere else.
>
> Agreed, we should find one solution for all use cases.
>
> Jan
Jan Kiszka - June 15, 2010, 11:45 a.m.
Paul Brook wrote:
>>> Alex proposed to disambiguate by adding "identified properties of the
>>> immediate parent bus and device" to the path component.  For PCI, these
>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>> bus address.  The driver name carries no information!
>> From user POV, driver names are very handly to address a device
>> intuitively - except for the case you have tones of devices on the same
>> bus that are handled by the same driver. For that case we need to
>> augment the device name with a useful per-bus ID, derived from the bus
>> address where available, otherwise based on instance numbers.
> 
> This is where I think you're missing a trick. We don't need to augment the 
> name, we just need to allow the bus id to be used instead.

I prefer having one name per device, both unique AND human-friendly.
Adding yet another alias will solve only the first requirement. E.g.,
which one should I present to the monitor user when listing a bus for
auto-completion or path error reporting?

>  
>>> For other buses, we need to make something up.
>>>
>>> Note that addressing by bus address rather than name is generally
>>> useful, not just in the context of savevm.  For instance, I'd appreciate
>>> being able to say something like "device_del pci.0/04.0".
>> And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
>> the bus first before you can identify which device you want to remove.
> 
> We can allow both.
> 
> A bus address is sufficient to uniquely identify a device.  I see no reason to 
> require the driver name,  or to include it in the canonical device address.

Readability and simplicity (less aliases - for the same reason, I'm
removing ID-based addresses from qtree paths, restricting them to the
global, flat namespace).

> 
>>> An easy way to get that is to reserve part of the name space for bus
>>> addresses.  If the path component starts with a letter, it's an ID or
>>> driver name.  If it starts with say '@', it's a bus address in
>>> bus-specific syntax.  The bus provides a method to look it up.
>> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
>> set for buses that implement some to-be-defined device addressing
>> service, the latter is the default on buses where that service is not
>> available.
> 
> If we have bus-address then I see no good reason to also add instance-no.
> For busses that no natural address, we can define the address to be an 
> instance number.

Again readability: isa-serial.0 & isa-serial.1 is more intuitive than
isa-serial.6 & isa-serial.7 just because there happen to be 6 other ISA
devices registered before them.

> 
>>> That way, we gain a useful feature, and avoid having an savevm-specific
>>> "device path" that isn't recognized anywhere else.
>> Agreed, we should find one solution for all use cases.
> 
> I wasn't aware that there was any suggestion of a separate savevm-specific 
> path.  The whole point of a device path is to uniquely identify a device 
> within a machine. There may be many different paths that identify the same 
> device.  When given a device and asked to generate  path, the result should be 
> the canonical address.  IMO this should be the least volatile, and avoid 
> redundant information.

Given that it is also user-visible, it should also have an intuitive and
informative format to avoid confusions. That may imply slightly more
information than strictly required for machine-based processing.

Jan
Jan Kiszka - June 15, 2010, 11:59 a.m.
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> Markus Armbruster wrote:
>>> Paul Brook <paul@codesourcery.com> writes:
>>>
>>>> Alex Williamson <alex.williamson@redhat.com> writes:
>>>>
>>>>> On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
>>>>>> Could you explain why you add "identified properties of the immediate
>>>>>> parent bus and device"?  They make the result ver much *not* a "dev
>>>>>> path" in the qdev sense...
>>>>> In order to try to get a unique string.  Without looking into device
>>>>> properties, two e1000s would both be:
>>>>>
>>>>> /main-system-bus/pci.0/e1000
>>>>> /main-system-bus/pci.0/e1000
>>>>>
>>>>> Which is no better than simply "e1000" and would require us to fall back
>>>>> to instance ids again.  The goal here is that anything that makes use of
>>>>> passing a dev when registering a vmstate gets an instance id of zero.
>>>> You already got the information you need, you just put it in the wrong place. 
>>>> The canonical ID for the device could be its bus address. In practice we'd 
>>>> probably want to allow the user to specify it by name, provided these are 
>>>> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
>>>> as an aias for [...]:_09.0. Device names have a restricted namespace, so we 
>>>> can use an initial prefix to disambiguate a name/label from a bus address.
>>>>
>>>> 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).
>>> When that's inconvenient or impossible, we can still punt to user: make
>>> device ID mandatory.
>> No option due to auto-created devices. And auto-generating IDs would
>> just create usability issues.
> 
> Auto-generated IDs would become part of the ABI.  Really so bad that
> it's "no option"?  Mind, device ID becomes mandatory *only* for devices
> that don't have a useful bus address.  We could even waive the ID
> requirement for the first device of a kind, i.e. require ID if and only
> if it's needed to disambiguate.

IDs are there to find devices the user (or a higher level tool) passed
to QEMU, qtree paths allow to locate _every_ device in a VM, and that in
a well-organized hierarchy. That allows to explore and address a qtree
element at the same time.

> 
>>> We obviously need a way to unambigously name a device.  It's okay to
>>> have multiple names for the same device.
>>>
>>> If the device has a device ID, that's an unambigous name.
>>>
>>> qdev paths may be ambigous when path components are resolved to driver
>>> names instead of IDs.
>>>
>>> Alex proposed to disambiguate by adding "identified properties of the
>>> immediate parent bus and device" to the path component.  For PCI, these
>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>> bus address.  The driver name carries no information!
>> >From user POV, driver names are very handly to address a device
>> intuitively - except for the case you have tones of devices on the same
>> bus that are handled by the same driver. For that case we need to
>> augment the device name with a useful per-bus ID, derived from the bus
>> address where available, otherwise based on instance numbers.
> 
> I'm not arguing against the use of driver names at all.
> 
>>> For other buses, we need to make something up.
>>>
>>> Note that addressing by bus address rather than name is generally
>>> useful, not just in the context of savevm.  For instance, I'd appreciate
>>> being able to say something like "device_del pci.0/04.0".
>> And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
>> the bus first before you can identify which device you want to remove.
> 
> It's not either/or.  Addressing by ID continues to work.  Addressing by
> bus/driver-name continues to work.  We merely add addressing by
> bus/@bus-address.

The format I will propose is "global-ID|/absolute/path", no more
/path/global-ID as this comes with the risk of ambiguity (ID may shadow
bus-local name of a device).

> 
>>> An easy way to get that is to reserve part of the name space for bus
>>> addresses.  If the path component starts with a letter, it's an ID or
>>> driver name.  If it starts with say '@', it's a bus address in
>>> bus-specific syntax.  The bus provides a method to look it up.
>> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
>> set for buses that implement some to-be-defined device addressing
>> service, the latter is the default on buses where that service is not
>> available.
> 
> I object to <driver>@<bus-address>, because the <driver> part carries no
> information.

I does for a human being as bus addresses tend to be unreadable and can
easily be confused, hence the additional, sometimes redundant driver name.

> 
> Not the case for <driver>.<instance-no>.  We still need a suitable
> definition of <instance-no>.  Possible definitions:
> 
> * n-th creation of a <driver> device.  Drawbacks: depends on creation
>   order.  Relatively hard to maintain across migration.
> 
> * n-th instance of a <driver> device.  Drawback: changes on unplug.
>   Good enough for interactive use, but it doesn't provide a stable
>   device name.

Every hotplug-capable bus must have a proper addressing scheme, I think
this is a reasonable and achievable requirement. Then we don't need
instance numbers for those buses.

> 
> When counting <driver> devices either way, we can count per bus or
> globally.  I prefer per bus.

Yes, counting should be both per-driver and per-bus ("the <n>th device
managed by <driver> on this bus").

> 
> None of the above instance numbers are nearly as neat as bus addresses.

Right, wherever they are available.

Jan
Paul Brook - June 15, 2010, 12:04 p.m.
> >> From user POV, driver names are very handly to address a device
> >> intuitively - except for the case you have tones of devices on the same
> >> bus that are handled by the same driver. For that case we need to
> >> augment the device name with a useful per-bus ID, derived from the bus
> >> address where available, otherwise based on instance numbers.
> > 
> > This is where I think you're missing a trick. We don't need to augment
> > the name, we just need to allow the bus id to be used instead.
> 
> I prefer having one name per device, both unique AND human-friendly.
> Adding yet another alias will solve only the first requirement. E.g.,
> which one should I present to the monitor user when listing a bus for
> auto-completion or path error reporting?

Autocompletion can report all of them. Errors should report either what the 
user specified (if the problem is with the address), or the canonical address 
(if the problem is unrelated to the address).

Remember that we also have global aliases (paths that do not begin with "/").

> >>> An easy way to get that is to reserve part of the name space for bus
> >>> addresses.  If the path component starts with a letter, it's an ID or
> >>> driver name.  If it starts with say '@', it's a bus address in
> >>> bus-specific syntax.  The bus provides a method to look it up.
> >> 
> >> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
> >> set for buses that implement some to-be-defined device addressing
> >> service, the latter is the default on buses where that service is not
> >> available.
> > 
> > If we have bus-address then I see no good reason to also add instance-no.
> > For busses that no natural address, we can define the address to be an
> > instance number.
> 
> Again readability: isa-serial.0 & isa-serial.1 is more intuitive than
> isa-serial.6 & isa-serial.7 just because there happen to be 6 other ISA
> devices registered before them.

I don't think either of these are intuitive. There's a good chance that
isa-serial.0 will not correspond to the first serial port in the guest.
Much better would be allowing use of IO port or MMIO addresses to identify ISA 
devices.  Some modification to the ISABus code may be required to implement 
this.

Paul
Jan Kiszka - June 15, 2010, 12:16 p.m.
Paul Brook wrote:
>>>> From user POV, driver names are very handly to address a device
>>>> intuitively - except for the case you have tones of devices on the same
>>>> bus that are handled by the same driver. For that case we need to
>>>> augment the device name with a useful per-bus ID, derived from the bus
>>>> address where available, otherwise based on instance numbers.
>>> This is where I think you're missing a trick. We don't need to augment
>>> the name, we just need to allow the bus id to be used instead.
>> I prefer having one name per device, both unique AND human-friendly.
>> Adding yet another alias will solve only the first requirement. E.g.,
>> which one should I present to the monitor user when listing a bus for
>> auto-completion or path error reporting?
> 
> Autocompletion can report all of them.

Autocompletion can only provide what is later on parseable. It doesn't
help to see "e1000" and "03.0" as device addresses because you do not
know their relation that way. Only combining the information into a
single name does.

> Errors should report either what the 
> user specified (if the problem is with the address), or the canonical address 
> (if the problem is unrelated to the address).
> 
> Remember that we also have global aliases (paths that do not begin with "/").

They only help if the user set them and therefore should know their
semantics.

> 
>>>>> An easy way to get that is to reserve part of the name space for bus
>>>>> addresses.  If the path component starts with a letter, it's an ID or
>>>>> driver name.  If it starts with say '@', it's a bus address in
>>>>> bus-specific syntax.  The bus provides a method to look it up.
>>>> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
>>>> set for buses that implement some to-be-defined device addressing
>>>> service, the latter is the default on buses where that service is not
>>>> available.
>>> If we have bus-address then I see no good reason to also add instance-no.
>>> For busses that no natural address, we can define the address to be an
>>> instance number.
>> Again readability: isa-serial.0 & isa-serial.1 is more intuitive than
>> isa-serial.6 & isa-serial.7 just because there happen to be 6 other ISA
>> devices registered before them.
> 
> I don't think either of these are intuitive. There's a good chance that
> isa-serial.0 will not correspond to the first serial port in the guest.

Only if you start tweaking the base addresses. Then it will still
correspond to the addition order AND the user should be aware of this
special setup.

> Much better would be allowing use of IO port or MMIO addresses to identify ISA 
> devices.  Some modification to the ISABus code may be required to implement 
> this.

Works for serial, but fails for ISA devices not occupying an address.

Jan
Paul Brook - June 15, 2010, 12:39 p.m.
> Paul Brook wrote:
> >>>> From user POV, driver names are very handly to address a device
> >>>> intuitively - except for the case you have tones of devices on the
> >>>> same bus that are handled by the same driver. For that case we need
> >>>> to augment the device name with a useful per-bus ID, derived from the
> >>>> bus address where available, otherwise based on instance numbers.
> >>> 
> >>> This is where I think you're missing a trick. We don't need to augment
> >>> the name, we just need to allow the bus id to be used instead.
> >> 
> >> I prefer having one name per device, both unique AND human-friendly.
> >> Adding yet another alias will solve only the first requirement. E.g.,
> >> which one should I present to the monitor user when listing a bus for
> >> auto-completion or path error reporting?
> > 
> > Autocompletion can report all of them.
> 
> Autocompletion can only provide what is later on parseable.

Of course.

> It doesn't
> help to see "e1000" and "03.0" as device addresses because you do not
> know their relation that way. Only combining the information into a
> single name does.

I don't get your argument here. Why shouldn't e1000 and @03.0 refer to the 
same device? Querying the device itself will tell you both, so it's not hard 
to figure out that they refer to the same thing. Either piece of information 
is sufficient, so why do we require both?

Note that autocompletion and enumeration for mechanical traversal are 
different problems. The former should include useful aliases for humans (i.e. 
both e1000 and @03.0). The latter should be limited to the unique values 
corresponding to canonical addresses (i.e. @03.0).

> >> Again readability: isa-serial.0 & isa-serial.1 is more intuitive than
> >> isa-serial.6 & isa-serial.7 just because there happen to be 6 other ISA
> >> devices registered before them.
> > 
> > I don't think either of these are intuitive. There's a good chance that
> > isa-serial.0 will not correspond to the first serial port in the guest.
> 
> Only if you start tweaking the base addresses. Then it will still
> correspond to the addition order AND the user should be aware of this
> special setup.

I'm pretty sure there are some machines that have both internal UARTs 
(considered to be the primary ports), and secondary ports on an ISA bus.

If you really want instance numbers, then they may make most sense appended to 
the driver name. However I think this should be independent of bus addressing, 
and bus addresses make most sense as the canonical address.

> > Much better would be allowing use of IO port or MMIO addresses to
> > identify ISA devices.  Some modification to the ISABus code may be
> > required to implement this.
> 
> Works for serial, but fails for ISA devices not occupying an address.

An ISA device without an IO/MMIO capabilities seems extremely unlikely. What 
exactly would such a device do?

Paul
Jan Kiszka - June 15, 2010, 1 p.m.
Paul Brook wrote:
>> Paul Brook wrote:
>>>>>> From user POV, driver names are very handly to address a device
>>>>>> intuitively - except for the case you have tones of devices on the
>>>>>> same bus that are handled by the same driver. For that case we need
>>>>>> to augment the device name with a useful per-bus ID, derived from the
>>>>>> bus address where available, otherwise based on instance numbers.
>>>>> This is where I think you're missing a trick. We don't need to augment
>>>>> the name, we just need to allow the bus id to be used instead.
>>>> I prefer having one name per device, both unique AND human-friendly.
>>>> Adding yet another alias will solve only the first requirement. E.g.,
>>>> which one should I present to the monitor user when listing a bus for
>>>> auto-completion or path error reporting?
>>> Autocompletion can report all of them.
>> Autocompletion can only provide what is later on parseable.
> 
> Of course.
> 
>> It doesn't
>> help to see "e1000" and "03.0" as device addresses because you do not
>> know their relation that way. Only combining the information into a
>> single name does.
> 
> I don't get your argument here. Why shouldn't e1000 and @03.0 refer to the 
> same device? Querying the device itself will tell you both, so it's not hard 
> to figure out that they refer to the same thing. Either piece of information 
> is sufficient, so why do we require both?

To avoid having to issue an "info qtree" in the middle of an
auto-completion for some other command.

> 
> Note that autocompletion and enumeration for mechanical traversal are 
> different problems. The former should include useful aliases for humans (i.e. 
> both e1000 and @03.0). The latter should be limited to the unique values 
> corresponding to canonical addresses (i.e. @03.0).
> 
>>>> Again readability: isa-serial.0 & isa-serial.1 is more intuitive than
>>>> isa-serial.6 & isa-serial.7 just because there happen to be 6 other ISA
>>>> devices registered before them.
>>> I don't think either of these are intuitive. There's a good chance that
>>> isa-serial.0 will not correspond to the first serial port in the guest.
>> Only if you start tweaking the base addresses. Then it will still
>> correspond to the addition order AND the user should be aware of this
>> special setup.
> 
> I'm pretty sure there are some machines that have both internal UARTs 
> (considered to be the primary ports), and secondary ports on an ISA bus.
> 
> If you really want instance numbers, then they may make most sense appended to 
> the driver name. However I think this should be independent of bus addressing, 
> and bus addresses make most sense as the canonical address.

That's how my current implementation looks like.

> 
>>> Much better would be allowing use of IO port or MMIO addresses to
>>> identify ISA devices.  Some modification to the ISABus code may be
>>> required to implement this.
>> Works for serial, but fails for ISA devices not occupying an address.
> 
> An ISA device without an IO/MMIO capabilities seems extremely unlikely. What 
> exactly would such a device do?

Inject interrupts via that bus (while exposing registers in some other
way). The m48t59 seems to fall in this category (unless I'm missing
something ATM).

Jan
Markus Armbruster - June 15, 2010, 1:07 p.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> Markus Armbruster wrote:
>>>> Paul Brook <paul@codesourcery.com> writes:
>>>>
>>>>> Alex Williamson <alex.williamson@redhat.com> writes:
>>>>>
>>>>>> On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
>>>>>>> Could you explain why you add "identified properties of the immediate
>>>>>>> parent bus and device"?  They make the result ver much *not* a "dev
>>>>>>> path" in the qdev sense...
>>>>>> In order to try to get a unique string.  Without looking into device
>>>>>> properties, two e1000s would both be:
>>>>>>
>>>>>> /main-system-bus/pci.0/e1000
>>>>>> /main-system-bus/pci.0/e1000
>>>>>>
>>>>>> Which is no better than simply "e1000" and would require us to fall back
>>>>>> to instance ids again.  The goal here is that anything that makes use of
>>>>>> passing a dev when registering a vmstate gets an instance id of zero.
>>>>> You already got the information you need, you just put it in the wrong place. 
>>>>> The canonical ID for the device could be its bus address. In practice we'd 
>>>>> probably want to allow the user to specify it by name, provided these are 
>>>>> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
>>>>> as an aias for [...]:_09.0. Device names have a restricted namespace, so we 
>>>>> can use an initial prefix to disambiguate a name/label from a bus address.
>>>>>
>>>>> 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).
>>>> When that's inconvenient or impossible, we can still punt to user: make
>>>> device ID mandatory.
>>> No option due to auto-created devices. And auto-generating IDs would
>>> just create usability issues.
>> 
>> Auto-generated IDs would become part of the ABI.  Really so bad that
>> it's "no option"?  Mind, device ID becomes mandatory *only* for devices
>> that don't have a useful bus address.  We could even waive the ID
>> requirement for the first device of a kind, i.e. require ID if and only
>> if it's needed to disambiguate.
>
> IDs are there to find devices the user (or a higher level tool) passed
> to QEMU, qtree paths allow to locate _every_ device in a VM, and that in
> a well-organized hierarchy. That allows to explore and address a qtree
> element at the same time.
>
>> 
>>>> We obviously need a way to unambigously name a device.  It's okay to
>>>> have multiple names for the same device.
>>>>
>>>> If the device has a device ID, that's an unambigous name.
>>>>
>>>> qdev paths may be ambigous when path components are resolved to driver
>>>> names instead of IDs.
>>>>
>>>> Alex proposed to disambiguate by adding "identified properties of the
>>>> immediate parent bus and device" to the path component.  For PCI, these
>>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>>> bus address.  The driver name carries no information!
>>> >From user POV, driver names are very handly to address a device
>>> intuitively - except for the case you have tones of devices on the same
>>> bus that are handled by the same driver. For that case we need to
>>> augment the device name with a useful per-bus ID, derived from the bus
>>> address where available, otherwise based on instance numbers.
>> 
>> I'm not arguing against the use of driver names at all.
>> 
>>>> For other buses, we need to make something up.
>>>>
>>>> Note that addressing by bus address rather than name is generally
>>>> useful, not just in the context of savevm.  For instance, I'd appreciate
>>>> being able to say something like "device_del pci.0/04.0".
>>> And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
>>> the bus first before you can identify which device you want to remove.
>> 
>> It's not either/or.  Addressing by ID continues to work.  Addressing by
>> bus/driver-name continues to work.  We merely add addressing by
>> bus/@bus-address.
>
> The format I will propose is "global-ID|/absolute/path", no more
> /path/global-ID as this comes with the risk of ambiguity (ID may shadow
> bus-local name of a device).

Doesn't this break existing usage?

We have a rule to resolve any ambiguity added by ID: it always takes
precedence over driver name.  What path/ID does add is shadowing: it can
make a device inaccessible by driver name.  Not much of a difference to
adding a second device with the same driver name.

>>>> An easy way to get that is to reserve part of the name space for bus
>>>> addresses.  If the path component starts with a letter, it's an ID or
>>>> driver name.  If it starts with say '@', it's a bus address in
>>>> bus-specific syntax.  The bus provides a method to look it up.
>>> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
>>> set for buses that implement some to-be-defined device addressing
>>> service, the latter is the default on buses where that service is not
>>> available.
>> 
>> I object to <driver>@<bus-address>, because the <driver> part carries no
>> information.
>
> I does for a human being as bus addresses tend to be unreadable and can
> easily be confused, hence the additional, sometimes redundant driver name.

I *strenuously* object to making the driver name mandatory with
bus-address addressing.

If you think the human user needs to be protected from mistakes by
making him supply redundant information, then "device_del pci.0/04.0
e1000" is much better way than complicating device paths for human and
machine users alike, not to mention uses internal to QEMU.

>> Not the case for <driver>.<instance-no>.  We still need a suitable
>> definition of <instance-no>.  Possible definitions:
>> 
>> * n-th creation of a <driver> device.  Drawbacks: depends on creation
>>   order.  Relatively hard to maintain across migration.
>> 
>> * n-th instance of a <driver> device.  Drawback: changes on unplug.
>>   Good enough for interactive use, but it doesn't provide a stable
>>   device name.
>
> Every hotplug-capable bus must have a proper addressing scheme, I think
> this is a reasonable and achievable requirement. Then we don't need
> instance numbers for those buses.

What about USB?

>> When counting <driver> devices either way, we can count per bus or
>> globally.  I prefer per bus.
>
> Yes, counting should be both per-driver and per-bus ("the <n>th device
> managed by <driver> on this bus").

Works for me.

>> None of the above instance numbers are nearly as neat as bus addresses.
>
> Right, wherever they are available.
>
> Jan
Paul Brook - June 15, 2010, 1:14 p.m.
> >> Works for serial, but fails for ISA devices not occupying an address.
> > 
> > An ISA device without an IO/MMIO capabilities seems extremely unlikely.
> > What exactly would such a device do?
> 
> Inject interrupts via that bus (while exposing registers in some other
> way). The m48t59 seems to fall in this category (unless I'm missing
> something ATM).

m48t59_isa responds to IO ports.  You're probably confused because it has not 
been properly converted to the qdev.

Paul
Markus Armbruster - June 15, 2010, 1:16 p.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Paul Brook wrote:
>>>> Alex proposed to disambiguate by adding "identified properties of the
>>>> immediate parent bus and device" to the path component.  For PCI, these
>>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>>> bus address.  The driver name carries no information!
>>> From user POV, driver names are very handly to address a device
>>> intuitively - except for the case you have tones of devices on the same
>>> bus that are handled by the same driver. For that case we need to
>>> augment the device name with a useful per-bus ID, derived from the bus
>>> address where available, otherwise based on instance numbers.
>> 
>> This is where I think you're missing a trick. We don't need to augment the 
>> name, we just need to allow the bus id to be used instead.
>
> I prefer having one name per device, both unique AND human-friendly.
> Adding yet another alias will solve only the first requirement. E.g.,
> which one should I present to the monitor user when listing a bus for
> auto-completion or path error reporting?
>
>>  
>>>> For other buses, we need to make something up.
>>>>
>>>> Note that addressing by bus address rather than name is generally
>>>> useful, not just in the context of savevm.  For instance, I'd appreciate
>>>> being able to say something like "device_del pci.0/04.0".
>>> And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
>>> the bus first before you can identify which device you want to remove.
>> 
>> We can allow both.
>> 
>> A bus address is sufficient to uniquely identify a device.  I see no reason to 
>> require the driver name,  or to include it in the canonical device address.
>
> Readability and simplicity (less aliases - for the same reason, I'm
> removing ID-based addresses from qtree paths, restricting them to the
> global, flat namespace).
>
>> 
>>>> An easy way to get that is to reserve part of the name space for bus
>>>> addresses.  If the path component starts with a letter, it's an ID or
>>>> driver name.  If it starts with say '@', it's a bus address in
>>>> bus-specific syntax.  The bus provides a method to look it up.
>>> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
>>> set for buses that implement some to-be-defined device addressing
>>> service, the latter is the default on buses where that service is not
>>> available.
>> 
>> If we have bus-address then I see no good reason to also add instance-no.
>> For busses that no natural address, we can define the address to be an 
>> instance number.
>
> Again readability: isa-serial.0 & isa-serial.1 is more intuitive than
> isa-serial.6 & isa-serial.7 just because there happen to be 6 other ISA
> devices registered before them.

Readability is in the eye of the beholder.  If the beholder wants to
number his serial devices a certain way, he better makes his wishes
known with id=, because the system has a hard time guessing them.

>>>> That way, we gain a useful feature, and avoid having an savevm-specific
>>>> "device path" that isn't recognized anywhere else.
>>> Agreed, we should find one solution for all use cases.
>> 
>> I wasn't aware that there was any suggestion of a separate savevm-specific 
>> path.  The whole point of a device path is to uniquely identify a device 
>> within a machine. There may be many different paths that identify the same 
>> device.  When given a device and asked to generate  path, the result should be 
>> the canonical address.  IMO this should be the least volatile, and avoid 
>> redundant information.
>
> Given that it is also user-visible, it should also have an intuitive and
> informative format to avoid confusions. That may imply slightly more
> information than strictly required for machine-based processing.

I'm with Paul here.
Paul Brook - June 15, 2010, 1:19 p.m.
> > Every hotplug-capable bus must have a proper addressing scheme, I think
> > this is a reasonable and achievable requirement. Then we don't need
> > instance numbers for those buses.
> 
> What about USB?

USB has useful device addresses (physical ports).
These aren't used by most of the higher-level protocols (logically USB is a 
flat bus structure), but the physically tree topology is still there if you 
look hard enough.

Paul
Paul Brook - June 15, 2010, 1:32 p.m.
> > > Every hotplug-capable bus must have a proper addressing scheme, I think
> > > this is a reasonable and achievable requirement. Then we don't need
> > > instance numbers for those buses.
> > 
> > What about USB?
> 
> USB has useful device addresses (physical ports).
> These aren't used by most of the higher-level protocols (logically USB is a
> flat bus structure), but the physically tree topology is still there if you
> look hard enough.

... and thinking about it a bit more, this adds an extra addressing 
possibility. 

USB devices have both physical and logical.  The physical addresses is which 
port the device is plugged into (including port addresses of any intermediate 
hubs). These never change, so should be the canonical address.

The logical address is set by the guest OS so may change (or be absent). Most 
software uses the logical address, so this is by far the most useful option 
for mapping from a guest device to a qemu device.

Paul
Jan Kiszka - June 15, 2010, 1:32 p.m.
Markus Armbruster wrote:
>>>>> That way, we gain a useful feature, and avoid having an savevm-specific
>>>>> "device path" that isn't recognized anywhere else.
>>>> Agreed, we should find one solution for all use cases.
>>> I wasn't aware that there was any suggestion of a separate savevm-specific 
>>> path.  The whole point of a device path is to uniquely identify a device 
>>> within a machine. There may be many different paths that identify the same 
>>> device.  When given a device and asked to generate  path, the result should be 
>>> the canonical address.  IMO this should be the least volatile, and avoid 
>>> redundant information.
>> Given that it is also user-visible, it should also have an intuitive and
>> informative format to avoid confusions. That may imply slightly more
>> information than strictly required for machine-based processing.
> 
> I'm with Paul here.

Well, what I'm proposing is derived from my experiences collected while
playing with device_show and device_add/del over the past weeks. For
these monitor scenarios, it was very handy to have expressive path
elements which avoid having to issue 'info qtree' all the time (which is
annoying if you are in the middle of a command input).

Jan
Jan Kiszka - June 15, 2010, 3:08 p.m.
Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> Markus Armbruster wrote:
>>>>> Paul Brook <paul@codesourcery.com> writes:
>>>>>
>>>>>> Alex Williamson <alex.williamson@redhat.com> writes:
>>>>>>
>>>>>>> On Mon, 2010-06-14 at 08:39 +0200, Markus Armbruster wrote:
>>>>>>>> Could you explain why you add "identified properties of the immediate
>>>>>>>> parent bus and device"?  They make the result ver much *not* a "dev
>>>>>>>> path" in the qdev sense...
>>>>>>> In order to try to get a unique string.  Without looking into device
>>>>>>> properties, two e1000s would both be:
>>>>>>>
>>>>>>> /main-system-bus/pci.0/e1000
>>>>>>> /main-system-bus/pci.0/e1000
>>>>>>>
>>>>>>> Which is no better than simply "e1000" and would require us to fall back
>>>>>>> to instance ids again.  The goal here is that anything that makes use of
>>>>>>> passing a dev when registering a vmstate gets an instance id of zero.
>>>>>> You already got the information you need, you just put it in the wrong place. 
>>>>>> The canonical ID for the device could be its bus address. In practice we'd 
>>>>>> probably want to allow the user to specify it by name, provided these are 
>>>>>> unique. e.g. in the above machine we could accept [...]/virtiio-blk-pci would 
>>>>>> as an aias for [...]:_09.0. Device names have a restricted namespace, so we 
>>>>>> can use an initial prefix to disambiguate a name/label from a bus address.
>>>>>>
>>>>>> 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).
>>>>> When that's inconvenient or impossible, we can still punt to user: make
>>>>> device ID mandatory.
>>>> No option due to auto-created devices. And auto-generating IDs would
>>>> just create usability issues.
>>> Auto-generated IDs would become part of the ABI.  Really so bad that
>>> it's "no option"?  Mind, device ID becomes mandatory *only* for devices
>>> that don't have a useful bus address.  We could even waive the ID
>>> requirement for the first device of a kind, i.e. require ID if and only
>>> if it's needed to disambiguate.
>> IDs are there to find devices the user (or a higher level tool) passed
>> to QEMU, qtree paths allow to locate _every_ device in a VM, and that in
>> a well-organized hierarchy. That allows to explore and address a qtree
>> element at the same time.
>>
>>>>> We obviously need a way to unambigously name a device.  It's okay to
>>>>> have multiple names for the same device.
>>>>>
>>>>> If the device has a device ID, that's an unambigous name.
>>>>>
>>>>> qdev paths may be ambigous when path components are resolved to driver
>>>>> names instead of IDs.
>>>>>
>>>>> Alex proposed to disambiguate by adding "identified properties of the
>>>>> immediate parent bus and device" to the path component.  For PCI, these
>>>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>>>> bus address.  The driver name carries no information!
>>>> >From user POV, driver names are very handly to address a device
>>>> intuitively - except for the case you have tones of devices on the same
>>>> bus that are handled by the same driver. For that case we need to
>>>> augment the device name with a useful per-bus ID, derived from the bus
>>>> address where available, otherwise based on instance numbers.
>>> I'm not arguing against the use of driver names at all.
>>>
>>>>> For other buses, we need to make something up.
>>>>>
>>>>> Note that addressing by bus address rather than name is generally
>>>>> useful, not just in the context of savevm.  For instance, I'd appreciate
>>>>> being able to say something like "device_del pci.0/04.0".
>>>> And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
>>>> the bus first before you can identify which device you want to remove.
>>> It's not either/or.  Addressing by ID continues to work.  Addressing by
>>> bus/driver-name continues to work.  We merely add addressing by
>>> bus/@bus-address.
>> The format I will propose is "global-ID|/absolute/path", no more
>> /path/global-ID as this comes with the risk of ambiguity (ID may shadow
>> bus-local name of a device).
> 
> Doesn't this break existing usage?

Please name one. It could only be weird corner cases like "device_add
driver,bus=bus1/ID/bus2" or "bus=ID/bus". But given that we always
allowed to address "bus2" directly (and this is something I cannot and
will not change), does this really matter? Maybe allowing paths to start
with an ID is something worth considering, will think about this again.

> 
> We have a rule to resolve any ambiguity added by ID: it always takes
> precedence over driver name.  What path/ID does add is shadowing: it can
> make a device inaccessible by driver name.  Not much of a difference to
> adding a second device with the same driver name.

ID was introduces as a _global_ address, there is simply no point using
it _inside_ qtree paths, it will just cause troubles and confusions.
Rather, we need to resolve the ambiguity of bus-local names - what this
thread is about.

> 
>>>>> An easy way to get that is to reserve part of the name space for bus
>>>>> addresses.  If the path component starts with a letter, it's an ID or
>>>>> driver name.  If it starts with say '@', it's a bus address in
>>>>> bus-specific syntax.  The bus provides a method to look it up.
>>>> I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
>>>> set for buses that implement some to-be-defined device addressing
>>>> service, the latter is the default on buses where that service is not
>>>> available.
>>> I object to <driver>@<bus-address>, because the <driver> part carries no
>>> information.
>> I does for a human being as bus addresses tend to be unreadable and can
>> easily be confused, hence the additional, sometimes redundant driver name.
> 
> I *strenuously* object to making the driver name mandatory with
> bus-address addressing.
> 
> If you think the human user needs to be protected from mistakes by
> making him supply redundant information, then "device_del pci.0/04.0
> e1000" is much better way than complicating device paths for human and
> machine users alike, not to mention uses internal to QEMU.

The only think that may speak against expressive device names might be
the overall path length. Not sure if we can already reach critical
limits for any involved subsystem, though. Anything else can and should
easily be encapsulated for qdev users in a single service providing the
unique device name for a given device - independent of the final format.

Jan
Alex Williamson - June 15, 2010, 8:53 p.m.
On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
> > > Alex proposed to disambiguate by adding "identified properties of the
> > > immediate parent bus and device" to the path component.  For PCI, these
> > > are dev.fn.  Likewise for any other bus where devices have unambigous
> > > bus address.  The driver name carries no information!
> > 
> > From user POV, driver names are very handly to address a device
> > intuitively - except for the case you have tones of devices on the same
> > bus that are handled by the same driver. For that case we need to
> > augment the device name with a useful per-bus ID, derived from the bus
> > address where available, otherwise based on instance numbers.
> 
> This is where I think you're missing a trick. We don't need to augment the 
> name, we just need to allow the bus id to be used instead.

For the case of a hot remove, I agree.  If the user specifies "pci_del
pci.0/03.0", that's completely sufficient because we don't care what's
in that slot, just remove it.  However, I still see some use cases for
device names in the path.  Take for example:

(A): /i440FX-pcihost/pci.0/e1000.05.0

vs

(B): /pci.0/05.0

(removing both the root bridge driver name and the device driver name)

If we attach a pci option rom to the device, create some ram to store
the option rom and name the ram block $(PATH)/rom, with (A) we know more
about the hierarchy to get to the actual devfn device, and we know the
type of device that's in the slot.  With (B), there's no robustness.  If
we migrated using (B), we could be stuffing a pc e1000 option rom into a
ppc lsi895, just because it happened to live that the same place and
have a ram block named rom.  Including driver names increases the
uniqueness of the path.

Another example; if we have two drivers that create a vmstate with name
"foo", plug one driver into slot 03.0 on the migration source, the other
into slot 03.0 on the migration destination, what happens?  It seems
likely that the destination will try to load the vmstate from a
different driver and fail in wonderful and bizarre ways.  If we use (A),
each path automatically has it's own namespace.

ISA is also a good example even though it doesn't do hotplug.  Given
this set:

/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/mc146818rtc
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-serial.0x3f8
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-parallel.0x378
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/i8042
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-fdc
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/ne2k_isa.0x300

versus this set:

/pci.0.01.0/isa.0
/pci.0.01.0/isa.0/0x3f8
/pci.0.01.0/isa.0/0x378
/pci.0.01.0/isa.0
/pci.0.01.0/isa.0
/pci.0.01.0/isa.0/0x300

Which one has devices that are easier to uniquely identify?
 
> > > For other buses, we need to make something up.
> > > 
> > > Note that addressing by bus address rather than name is generally
> > > useful, not just in the context of savevm.  For instance, I'd appreciate
> > > being able to say something like "device_del pci.0/04.0".
> > 
> > And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
> > the bus first before you can identify which device you want to remove.
> 
> We can allow both.
> 
> A bus address is sufficient to uniquely identify a device.

A bus address (assuming it exists) is sufficient to uniquely identify a
device, on a given VM.  A bus address only identifies a location when
comparing two separate VMs.

>   I see no reason to 
> require the driver name,  or to include it in the canonical device address.

Migration.  Including the driver name extends our ability to uniquely
identify a device across separate VMs.  It's then up to the vmstate code
to figure out whether the device are compatible for migrate state.

> > > An easy way to get that is to reserve part of the name space for bus
> > > addresses.  If the path component starts with a letter, it's an ID or
> > > driver name.  If it starts with say '@', it's a bus address in
> > > bus-specific syntax.  The bus provides a method to look it up.
> > 
> > I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
> > set for buses that implement some to-be-defined device addressing
> > service, the latter is the default on buses where that service is not
> > available.
> 
> If we have bus-address then I see no good reason to also add instance-no.
> For busses that no natural address, we can define the address to be an 
> instance number.

I agree, it should be a bug for any device with a bus address to have an
instance number other than zero.  Anything without a bus number can make
use of instance numbers, and hopefully will never be hotplugged.

> > > That way, we gain a useful feature, and avoid having an savevm-specific
> > > "device path" that isn't recognized anywhere else.
> > 
> > Agreed, we should find one solution for all use cases.
> 
> I wasn't aware that there was any suggestion of a separate savevm-specific 
> path.  The whole point of a device path is to uniquely identify a device 
> within a machine. There may be many different paths that identify the same 
> device.  When given a device and asked to generate  path, the result should be 
> the canonical address.  IMO this should be the least volatile, and avoid 
> redundant information.

Yep, I'm certainly aiming to not make this specific to savevm, but maybe
the difference is that savevm does care about migration.  If we take
migration out of the picture, then I would tend to agree that we can
remove driver names.  A bus address uniquely identifies a given device
on a given VM.  But if we add migration back in, I think we can increase
the robustness of the device path without reducing the stability by
including the driver names.  Most of my examples above are pathological
cases that aren't going to happen.  I think the driver name does provide
useful information across VMs, but I'll go with the consensus whether to
include it.  BTW, find below output for a VM using the full driver name
path versus no driver names.

Alex

(A)

/i440FX-pcihost
/i440FX-pcihost/pci.0/i440FX.00.0
/i440FX-pcihost/pci.0/PIIX3.01.0
/i440FX-pcihost/pci.0/cirrus-vga.02.0
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/mc146818rtc
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-serial.0x3f8
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-parallel.0x378
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/i8042
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-fdc
/i440FX-pcihost/pci.0/i82551.03.0
/i440FX-pcihost/pci.0/virtio-net-pci.04.0
/i440FX-pcihost/pci.0/e1000.05.0
/i440FX-pcihost/pci.0/rtl8139.06.0
/i440FX-pcihost/pci.0/pcnet.07.0
/i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/ne2k_isa.0x300
/i440FX-pcihost/pci.0/piix3-ide.01.1
/i440FX-pcihost/pci.0/piix3-ide.01.1/ide.0/ide-drive.0
/i440FX-pcihost/pci.0/piix3-ide.01.1/ide.1/ide-drive.0
/i440FX-pcihost/pci.0/piix3-usb-uhci.01.2
/i440FX-pcihost/pci.0/PIIX4_PM.01.3
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.80
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.81
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.82
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.83
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.84
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.85
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.86
/i440FX-pcihost/pci.0/PIIX4_PM.01.3/i2c/smbus-eeprom.87
/i440FX-pcihost/pci.0/lsi53c895a.08.0/scsi.0/scsi-disk.0
/i440FX-pcihost/pci.0/lsi53c895a.08.0/scsi.0/scsi-disk.3
/i440FX-pcihost/pci.0/lsi53c895a.08.0
/i440FX-pcihost/pci.0/piix3-usb-uhci.01.2/usb.0/usb-storage.usb0/scsi.0/scsi-disk.0
/i440FX-pcihost/pci.0/piix3-usb-uhci.01.2/usb.0/usb-storage.usb0
/i440FX-pcihost/pci.0/virtio-blk-pci.09.0

(B)

/pci.0.00.0
/pci.0.01.0
/pci.0.02.0
/pci.0.01.0/isa.0
/pci.0.01.0/isa.0/0x3f8
/pci.0.01.0/isa.0/0x378
/pci.0.01.0/isa.0
/pci.0.01.0/isa.0
/pci.0.03.0
/pci.0.04.0
/pci.0.05.0
/pci.0.06.0
/pci.0.07.0
/pci.0.01.0/isa.0/0x300
/pci.0.01.1
/pci.0.01.1/ide.0.0
/pci.0.01.1/ide.1.0
/pci.0.01.2
/pci.0.01.3
/pci.0.01.3/i2c/80
/pci.0.01.3/i2c/81
/pci.0.01.3/i2c/82
/pci.0.01.3/i2c/83
/pci.0.01.3/i2c/84
/pci.0.01.3/i2c/85
/pci.0.01.3/i2c/86
/pci.0.01.3/i2c/87
/pci.0.08.0/scsi.0/0
/pci.0.08.0/scsi.0/3
/pci.0.08.0
/pci.0.01.2/usb.0/usb0/scsi.0/0
/pci.0.01.2/usb.0/usb0
/pci.0.09.0
Paul Brook - June 15, 2010, 9:55 p.m.
> On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
> > > > Alex proposed to disambiguate by adding "identified properties of the
> > > > immediate parent bus and device" to the path component.  For PCI,
> > > > these are dev.fn.  Likewise for any other bus where devices have
> > > > unambigous bus address.  The driver name carries no information!
> > > 
> > > From user POV, driver names are very handly to address a device
> > > intuitively - except for the case you have tones of devices on the same
> > > bus that are handled by the same driver. For that case we need to
> > > augment the device name with a useful per-bus ID, derived from the bus
> > > address where available, otherwise based on instance numbers.
> > 
> > This is where I think you're missing a trick. We don't need to augment
> > the name, we just need to allow the bus id to be used instead.
> 
> For the case of a hot remove, I agree.  If the user specifies "pci_del
> pci.0/03.0", that's completely sufficient because we don't care what's
> in that slot, just remove it.  However, I still see some use cases for
> device names in the path.  Take for example:
> 
> (A): /i440FX-pcihost/pci.0/e1000.05.0
> 
> vs
> 
> (B): /pci.0/05.0

> (removing both the root bridge driver name and the device driver name)

This is wrong. You still need an entry for the host pci bridge.

 > If we attach a pci option rom to the device, create some ram to store
> the option rom and name the ram block $(PATH)/rom, with (A) we know more
> about the hierarchy to get to the actual devfn device, and we know the
> type of device that's in the slot.  With (B), there's no robustness.  If
> we migrated using (B), we could be stuffing a pc e1000 option rom into a
> ppc lsi895, just because it happened to live that the same place and
> have a ram block named rom.  Including driver names increases the
> uniqueness of the path.

No it doesn't. It adds redundant information and a false sense of security. 
Driver name is not sufficient to determine whether you've actually got a 
compatible device. The type of the device doesn't reliably tell you anything 
about how many ram blocks that device has, or how big they are.

> > > > For other buses, we need to make something up.
> > > > 
> > > > Note that addressing by bus address rather than name is generally
> > > > useful, not just in the context of savevm.  For instance, I'd
> > > > appreciate being able to say something like "device_del pci.0/04.0".
> > > 
> > > And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
> > > the bus first before you can identify which device you want to remove.
> > 
> > We can allow both.
> > A bus address is sufficient to uniquely identify a device.
> 
> A bus address (assuming it exists) is sufficient to uniquely identify a
> device, on a given VM.  A bus address only identifies a location when
> comparing two separate VMs.

I can't make any sense of this statement.
 
> >   I see no reason to
> > require the driver name,  or to include it in the canonical device
> > address.
> 
> Migration.  Including the driver name extends our ability to uniquely
> identify a device across separate VMs.  It's then up to the vmstate code
> to figure out whether the device are compatible for migrate state.

I find this argument contradictory. The migration code already needs to check 
whether a device is compatible before it allows migration.  The driver name is 
not sufficient to ensure compatibility, so I see no benefit in including it in 
the device address. If we include the device name, why aren't we also 
including all other properties that would make the devices incompatible?

Paul
Alex Williamson - June 15, 2010, 10:33 p.m.
On Tue, 2010-06-15 at 22:55 +0100, Paul Brook wrote:
> > On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
> > > > > Alex proposed to disambiguate by adding "identified properties of the
> > > > > immediate parent bus and device" to the path component.  For PCI,
> > > > > these are dev.fn.  Likewise for any other bus where devices have
> > > > > unambigous bus address.  The driver name carries no information!
> > > > 
> > > > From user POV, driver names are very handly to address a device
> > > > intuitively - except for the case you have tones of devices on the same
> > > > bus that are handled by the same driver. For that case we need to
> > > > augment the device name with a useful per-bus ID, derived from the bus
> > > > address where available, otherwise based on instance numbers.
> > > 
> > > This is where I think you're missing a trick. We don't need to augment
> > > the name, we just need to allow the bus id to be used instead.
> > 
> > For the case of a hot remove, I agree.  If the user specifies "pci_del
> > pci.0/03.0", that's completely sufficient because we don't care what's
> > in that slot, just remove it.  However, I still see some use cases for
> > device names in the path.  Take for example:
> > 
> > (A): /i440FX-pcihost/pci.0/e1000.05.0
> > 
> > vs
> > 
> > (B): /pci.0/05.0
> 
> > (removing both the root bridge driver name and the device driver name)
> 
> This is wrong. You still need an entry for the host pci bridge.

The host pci bridge has no identifying properties, so all we'd be
printing is the pci bridge driver name, which we claim adds no value.
Or are we saying that some driver names are useful while others aren't?

>  > If we attach a pci option rom to the device, create some ram to store
> > the option rom and name the ram block $(PATH)/rom, with (A) we know more
> > about the hierarchy to get to the actual devfn device, and we know the
> > type of device that's in the slot.  With (B), there's no robustness.  If
> > we migrated using (B), we could be stuffing a pc e1000 option rom into a
> > ppc lsi895, just because it happened to live that the same place and
> > have a ram block named rom.  Including driver names increases the
> > uniqueness of the path.
> 
> No it doesn't. It adds redundant information and a false sense of security. 
> Driver name is not sufficient to determine whether you've actually got a 
> compatible device. The type of the device doesn't reliably tell you anything 
> about how many ram blocks that device has, or how big they are.

Driver name is sufficient to tell me that driver "foo" created this
device on both the source and destination and hand off responsibility to
driver "foo" to make the determination about whether it's compatible.  I
think it's reasonably safe to say we're not going to have two completely
different drivers with name "foo"s within an instance of qemu and we're
not going to turn over the name to a different driver and expect
anything sane to happen wrt to migration.  I disagree that this is a
false sense of security, it gives me warm fuzzies.  Please convince me
otherwise.

> > > > > For other buses, we need to make something up.
> > > > > 
> > > > > Note that addressing by bus address rather than name is generally
> > > > > useful, not just in the context of savevm.  For instance, I'd
> > > > > appreciate being able to say something like "device_del pci.0/04.0".
> > > > 
> > > > And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
> > > > the bus first before you can identify which device you want to remove.
> > > 
> > > We can allow both.
> > > A bus address is sufficient to uniquely identify a device.
> > 
> > A bus address (assuming it exists) is sufficient to uniquely identify a
> > device, on a given VM.  A bus address only identifies a location when
> > comparing two separate VMs.
> 
> I can't make any sense of this statement.

Makes sense to me ;)  A bus address is unique only within the context of
the VM.  Within that VM I can figure out that the device at that bus
address uses driver foo, so foo seems redundant in the device path.  Now
I try to apply that bus address to another VM, the migration target, and
all I have is a location.  I have to make a leap of faith that the
device at that location is the same as the device at the same location
on the migration source.  Where are those warm fuzzies?

> > >   I see no reason to
> > > require the driver name,  or to include it in the canonical device
> > > address.
> > 
> > Migration.  Including the driver name extends our ability to uniquely
> > identify a device across separate VMs.  It's then up to the vmstate code
> > to figure out whether the device are compatible for migrate state.
> 
> I find this argument contradictory. The migration code already needs to check 
> whether a device is compatible before it allows migration.  The driver name is 
> not sufficient to ensure compatibility, so I see no benefit in including it in 
> the device address.

See my comment above, I'm not seeing a sufficient argument about why
driver name matching is a false sense of security.  If on an incoming
migration I'm able to match the source provided e1000.03.0/vmstate
against the target registered e1000.03.0/vmstate and hand off to the
e1000 driver to check version ids, you bet I'm feeling a lot more secure
than if I'm handing off to whatever happened to register 03.0/vmstate on
the target.

> If we include the device name, why aren't we also 
> including all other properties that would make the devices incompatible?

Well, in some ways that'd be really nice, then we could rebuild the
device on the fly in the target.  But, I'm attempting to work within the
existing migration code, because I'd really like something working
better than it does today very soon.  Therefore, we do want to reduce
the path to only those elements that are stable and add value.  I argue
the driver name is both stable and adds value.  Thanks,

Alex
Paul Brook - June 15, 2010, 11:01 p.m.
> > I find this argument contradictory. The migration code already needs to
> > check whether a device is compatible before it allows migration.  The
> > driver name is not sufficient to ensure compatibility, so I see no
> > benefit in including it in the device address.
> 
> See my comment above, I'm not seeing a sufficient argument about why
> driver name matching is a false sense of security.  If on an incoming
> migration I'm able to match the source provided e1000.03.0/vmstate
> against the target registered e1000.03.0/vmstate and hand off to the
> e1000 driver to check version ids, you bet I'm feeling a lot more secure
> than if I'm handing off to whatever happened to register 03.0/vmstate on
> the target.

I still say it should be the migration code that checks that both vmstate 
structures are for the same type of device. i.e. if necessary the device name 
should be embedded in the device state, not the device path.

Paul
Alex Williamson - June 15, 2010, 11:10 p.m.
On Wed, 2010-06-16 at 00:01 +0100, Paul Brook wrote:
> > > I find this argument contradictory. The migration code already needs to
> > > check whether a device is compatible before it allows migration.  The
> > > driver name is not sufficient to ensure compatibility, so I see no
> > > benefit in including it in the device address.
> > 
> > See my comment above, I'm not seeing a sufficient argument about why
> > driver name matching is a false sense of security.  If on an incoming
> > migration I'm able to match the source provided e1000.03.0/vmstate
> > against the target registered e1000.03.0/vmstate and hand off to the
> > e1000 driver to check version ids, you bet I'm feeling a lot more secure
> > than if I'm handing off to whatever happened to register 03.0/vmstate on
> > the target.
> 
> I still say it should be the migration code that checks that both vmstate 
> structures are for the same type of device. i.e. if necessary the device name 
> should be embedded in the device state, not the device path.

The migration code would check that ("%s/%s", path, name) match.  So
embedding the driver name into path gives us a per path namespace.  Sure
the migration code could check ("%s/%s/%s, path, dev->info->name, name),
but should it be the migration code's responsibility to dig that out?
And if you think that i440FX-pcihost is a useful driver name, then we'd
have to figure out which driver names are useful.  I think it's more
consistent to simply put them all there.  Thanks,

Alex
Chris Wright - June 16, 2010, 12:25 a.m.
* Paul Brook (paul@codesourcery.com) wrote:
> > > I find this argument contradictory. The migration code already needs to
> > > check whether a device is compatible before it allows migration.  The
> > > driver name is not sufficient to ensure compatibility, so I see no
> > > benefit in including it in the device address.
> > 
> > See my comment above, I'm not seeing a sufficient argument about why
> > driver name matching is a false sense of security.  If on an incoming
> > migration I'm able to match the source provided e1000.03.0/vmstate
> > against the target registered e1000.03.0/vmstate and hand off to the
> > e1000 driver to check version ids, you bet I'm feeling a lot more secure
> > than if I'm handing off to whatever happened to register 03.0/vmstate on
> > the target.
> 
> I still say it should be the migration code that checks that both vmstate 
> structures are for the same type of device. i.e. if necessary the device name 
> should be embedded in the device state, not the device path.

I not sure how that fixes the ramblock issue that started this whole
discussion.  It's not part of device's vmstate, it goes w/ ram.  I think
I'm missing a piece of the puzzle here?

thanks,
-chris
Paul Brook - June 16, 2010, 12:30 a.m.
> * Paul Brook (paul@codesourcery.com) wrote:
> > > > I find this argument contradictory. The migration code already needs
> > > > to check whether a device is compatible before it allows migration. 
> > > > The driver name is not sufficient to ensure compatibility, so I see
> > > > no benefit in including it in the device address.
> > > 
> > > See my comment above, I'm not seeing a sufficient argument about why
> > > driver name matching is a false sense of security.  If on an incoming
> > > migration I'm able to match the source provided e1000.03.0/vmstate
> > > against the target registered e1000.03.0/vmstate and hand off to the
> > > e1000 driver to check version ids, you bet I'm feeling a lot more
> > > secure than if I'm handing off to whatever happened to register
> > > 03.0/vmstate on the target.
> > 
> > I still say it should be the migration code that checks that both vmstate
> > structures are for the same type of device. i.e. if necessary the device
> > name should be embedded in the device state, not the device path.
> 
> I not sure how that fixes the ramblock issue that started this whole
> discussion.  It's not part of device's vmstate, it goes w/ ram.  I think
> I'm missing a piece of the puzzle here?

My main point there was that ram blocks should be associated with a device 
state.  Once you do this it should just work as we already know how to match 
device states.

It you're trying to transfer ram blocks before matching up the rest of the 
state then you're likely to loose in all kinds of different ways.

Paul
Chris Wright - June 16, 2010, 12:35 a.m.
* Paul Brook (paul@codesourcery.com) wrote:
> > * Paul Brook (paul@codesourcery.com) wrote:
> > > > > I find this argument contradictory. The migration code already needs
> > > > > to check whether a device is compatible before it allows migration. 
> > > > > The driver name is not sufficient to ensure compatibility, so I see
> > > > > no benefit in including it in the device address.
> > > > 
> > > > See my comment above, I'm not seeing a sufficient argument about why
> > > > driver name matching is a false sense of security.  If on an incoming
> > > > migration I'm able to match the source provided e1000.03.0/vmstate
> > > > against the target registered e1000.03.0/vmstate and hand off to the
> > > > e1000 driver to check version ids, you bet I'm feeling a lot more
> > > > secure than if I'm handing off to whatever happened to register
> > > > 03.0/vmstate on the target.
> > > 
> > > I still say it should be the migration code that checks that both vmstate
> > > structures are for the same type of device. i.e. if necessary the device
> > > name should be embedded in the device state, not the device path.
> > 
> > I not sure how that fixes the ramblock issue that started this whole
> > discussion.  It's not part of device's vmstate, it goes w/ ram.  I think
> > I'm missing a piece of the puzzle here?
> 
> My main point there was that ram blocks should be associated with a device 
> state.  Once you do this it should just work as we already know how to match 
> device states.
> 
> It you're trying to transfer ram blocks before matching up the rest of the 
> state then you're likely to loose in all kinds of different ways.

Yes, I see, thanks for clarifying.  I agree, ideally we'd create the
entire target image dynamically.  It'd still need to know how to connect
all the guest devices to the host, but it makes more sense to me than
half building the system from cmdline on target, then rest filled in.

I think that level of work is a ways away though.

thanks,
-chris
Paul Brook - June 16, 2010, 1:30 a.m.
> > > > > See my comment above, I'm not seeing a sufficient argument about
> > > > > why driver name matching is a false sense of security.
> > > > 
> > > > I still say it should be the migration code that checks that both
> > > > vmstate structures are for the same type of device. i.e. if
> > > > necessary the device name should be embedded in the device state,
> > > > not the device path.
> > > 
> > > I not sure how that fixes the ramblock issue that started this whole
> > > discussion.  It's not part of device's vmstate, it goes w/ ram.  I
> > > think I'm missing a piece of the puzzle here?
> > 
> > My main point there was that ram blocks should be associated with a
> > device state.  Once you do this it should just work as we already know
> > how to match device states.
> > 
> > It you're trying to transfer ram blocks before matching up the rest of
> > the state then you're likely to loose in all kinds of different ways.

... or not. See below.

> Yes, I see, thanks for clarifying.  I agree, ideally we'd create the
> entire target image dynamically.  It'd still need to know how to connect
> all the guest devices to the host, but it makes more sense to me than
> half building the system from cmdline on target, then rest filled in.

Transferring the machine description on migration is a separate problem.

Lets say we associate each RAM block with a device. Each ram block also has a 
name.  These names distinguish between blocks attached to a given device, but 
need not be globally unique.  i.e. devices A and B can both have block named 
"foo".  RAM block migration happens before device state migration (including 
device properties).

There are three relevant migration failure modes:

(1) The same device is present, but has a different size property.
  If the incoming block is larger than the allocated block then you loose.
(2) A different device is present, but does not have a ram block of the same 
name.
  This safely fails migration because of the block name mismatch.
(3) A different device is present, that happens to have a ram block of the 
same name.
  If the blocks are the same size then transferring the contents is harmless.
  If they are different sizes then this will be caught by (1). Either way, the
  migration will be failed once we get to the vmstate check.

Note how adding the device type to the canonical address does not effect the 
outcome.

Going back to the original problem, (1) is the most interesting.

I suggest that the initial migration phase transfers a list of ram blocks. 
Each entry in that list should be {canonical device path, name, size}. You 
should lookup all these ram blocks, and fail migration if they are not present 
with the correct size[1].  This list also gives you a convenient numeric index 
to identify the block during RAM migration.

[1] In the future we may be able to resize blocks. However this is not safe 
with the current API.

Paul
Alex Williamson - June 16, 2010, 2:55 a.m.
On Wed, 2010-06-16 at 02:30 +0100, Paul Brook wrote:
> Transferring the machine description on migration is a separate problem.
> 
> Lets say we associate each RAM block with a device. Each ram block also has a 
> name.  These names distinguish between blocks attached to a given device, but 
> need not be globally unique.  i.e. devices A and B can both have block named 
> "foo".  RAM block migration happens before device state migration (including 
> device properties).
> 
> There are three relevant migration failure modes:
> 
> (1) The same device is present, but has a different size property.
>   If the incoming block is larger than the allocated block then you loose.
> (2) A different device is present, but does not have a ram block of the same 
> name.
>   This safely fails migration because of the block name mismatch.
> (3) A different device is present, that happens to have a ram block of the 
> same name.
>   If the blocks are the same size then transferring the contents is harmless.
>   If they are different sizes then this will be caught by (1). Either way, the
>   migration will be failed once we get to the vmstate check.
> 
> Note how adding the device type to the canonical address does not effect the 
> outcome.
> 
> Going back to the original problem, (1) is the most interesting.
> 
> I suggest that the initial migration phase transfers a list of ram blocks. 
> Each entry in that list should be {canonical device path, name, size}. You 
> should lookup all these ram blocks, and fail migration if they are not present 
> with the correct size[1].  This list also gives you a convenient numeric index 
> to identify the block during RAM migration.
> 
> [1] In the future we may be able to resize blocks. However this is not safe 
> with the current API.

I think for the most part, you've just described the RAMBlock series of
patches I sent out last week.  I'll note that that series creates ram
blocks on the target if they aren't present because of the technicality
that we currently do not have a qemu_ram_free() to cleanup the list when
things go away.  Once we have that and cleanup drivers to use it, I
agree we should fail the migration if it occurs, or at least print out a
big warning so we can go fix the driver.  If I'm missing where else it's
significantly different please let me know.

Yes, case (3) would fail in the vmstate code without driver name in the
canonical path... or at least we hope it would.  But with the driver
name in the canonical path, we can avoid doing a useless operation, fail
earlier, and provide the vmstate with a key piece of information it can
use to help ensure that the incoming state information belongs to the
driver it thinks it does.  Seems like a win to me.  Thanks,

Alex
Markus Armbruster - June 16, 2010, 8:23 a.m.
Alex Williamson <alex.williamson@redhat.com> writes:

> On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
>> > > Alex proposed to disambiguate by adding "identified properties of the
>> > > immediate parent bus and device" to the path component.  For PCI, these
>> > > are dev.fn.  Likewise for any other bus where devices have unambigous
>> > > bus address.  The driver name carries no information!
>> > 
>> > From user POV, driver names are very handly to address a device
>> > intuitively - except for the case you have tones of devices on the same
>> > bus that are handled by the same driver. For that case we need to
>> > augment the device name with a useful per-bus ID, derived from the bus
>> > address where available, otherwise based on instance numbers.
>> 
>> This is where I think you're missing a trick. We don't need to augment the 
>> name, we just need to allow the bus id to be used instead.
>
> For the case of a hot remove, I agree.  If the user specifies "pci_del
> pci.0/03.0", that's completely sufficient because we don't care what's
> in that slot, just remove it.  However, I still see some use cases for
> device names in the path.  Take for example:
>
> (A): /i440FX-pcihost/pci.0/e1000.05.0
>
> vs
>
> (B): /pci.0/05.0
>
> (removing both the root bridge driver name and the device driver name)

/ is the main system bus.  System bus defines no bus address at the
moment.  Therefore, you have to use the driver name i440FX-pcihost.

/i440FX-pcihost/pci.0 is the PCI bus.  PCI bus defines a bus address:
dev.fn.  Therefore, you can either use the bus address @05.0, or the
driver name e1000.

We have "/i440FX-pcihost/pci.0/e1000" vs. "/i440FX-pcihost/pci.0/@05.0".

> If we attach a pci option rom to the device, create some ram to store
> the option rom and name the ram block $(PATH)/rom, with (A) we know more
> about the hierarchy to get to the actual devfn device, and we know the
> type of device that's in the slot.  With (B), there's no robustness.  If
> we migrated using (B), we could be stuffing a pc e1000 option rom into a
> ppc lsi895, just because it happened to live that the same place and
> have a ram block named rom.  Including driver names increases the
> uniqueness of the path.
>
> Another example; if we have two drivers that create a vmstate with name
> "foo", plug one driver into slot 03.0 on the migration source, the other
> into slot 03.0 on the migration destination, what happens?  It seems
> likely that the destination will try to load the vmstate from a
> different driver and fail in wonderful and bizarre ways.  If we use (A),
> each path automatically has it's own namespace.
>
> ISA is also a good example even though it doesn't do hotplug.  Given
> this set:
>
> /i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/mc146818rtc
> /i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-serial.0x3f8
> /i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-parallel.0x378
> /i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/i8042
> /i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/isa-fdc
> /i440FX-pcihost/pci.0/PIIX3.01.0/isa.0/ne2k_isa.0x300
>
> versus this set:
>
> /pci.0.01.0/isa.0
> /pci.0.01.0/isa.0/0x3f8
> /pci.0.01.0/isa.0/0x378
> /pci.0.01.0/isa.0
> /pci.0.01.0/isa.0
> /pci.0.01.0/isa.0/0x300
>
> Which one has devices that are easier to uniquely identify?
>  
>> > > For other buses, we need to make something up.
>> > > 
>> > > Note that addressing by bus address rather than name is generally
>> > > useful, not just in the context of savevm.  For instance, I'd appreciate
>> > > being able to say something like "device_del pci.0/04.0".
>> > 
>> > And I prefer "device_del [.../]pci.0/e1000". Otherwise you need to dump
>> > the bus first before you can identify which device you want to remove.
>> 
>> We can allow both.
>> 
>> A bus address is sufficient to uniquely identify a device.
>
> A bus address (assuming it exists) is sufficient to uniquely identify a
> device, on a given VM.  A bus address only identifies a location when
> comparing two separate VMs.

Identifying a device on a given VM is all a qdev path does.

If you want to check two VMs have the same device in the same slot, then
you need to compare *devices*, not their names.  You propose to encode
*one* property of the device in the name, namely its driver.  This is
far from sufficient.  If you tell me you need it anyway for migration,
I'll have to take that at face value (I'm not an expert there).  But
please do not call it "qdev path", because it ain't.

>>   I see no reason to 
>> require the driver name,  or to include it in the canonical device address.
>
> Migration.  Including the driver name extends our ability to uniquely
> identify a device across separate VMs.  It's then up to the vmstate code
> to figure out whether the device are compatible for migrate state.

Migration needs to recreate the same qdev tree on the destination.
Driver name is just *one* property of a device.  Migration needs to
transfer *all* properties.  Why encode driver name in the path, but not
the rest?  Why can't you put the driver name wherever you put the rest?

>> > > An easy way to get that is to reserve part of the name space for bus
>> > > addresses.  If the path component starts with a letter, it's an ID or
>> > > driver name.  If it starts with say '@', it's a bus address in
>> > > bus-specific syntax.  The bus provides a method to look it up.
>> > 
>> > I would prefer <driver>[@<bus-address>|.<instance-no>]. The former is
>> > set for buses that implement some to-be-defined device addressing
>> > service, the latter is the default on buses where that service is not
>> > available.
>> 
>> If we have bus-address then I see no good reason to also add instance-no.
>> For busses that no natural address, we can define the address to be an 
>> instance number.
>
> I agree, it should be a bug for any device with a bus address to have an
> instance number other than zero.  Anything without a bus number can make
> use of instance numbers, and hopefully will never be hotplugged.
>
>> > > That way, we gain a useful feature, and avoid having an savevm-specific
>> > > "device path" that isn't recognized anywhere else.
>> > 
>> > Agreed, we should find one solution for all use cases.
>> 
>> I wasn't aware that there was any suggestion of a separate savevm-specific 
>> path.  The whole point of a device path is to uniquely identify a device 
>> within a machine. There may be many different paths that identify the same 
>> device.  When given a device and asked to generate  path, the result should be 
>> the canonical address.  IMO this should be the least volatile, and avoid 
>> redundant information.

Agree.

[...]
Markus Armbruster - June 16, 2010, 1:02 p.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
[...]
>>> The format I will propose is "global-ID|/absolute/path", no more
>>> /path/global-ID as this comes with the risk of ambiguity (ID may shadow
>>> bus-local name of a device).
>> 
>> Doesn't this break existing usage?
>
> Please name one. It could only be weird corner cases like "device_add
> driver,bus=bus1/ID/bus2" or "bus=ID/bus". But given that we always
> allowed to address "bus2" directly (and this is something I cannot and
> will not change), does this really matter? Maybe allowing paths to start
> with an ID is something worth considering, will think about this again.

I checked with Dan: libvirt doesn't care.  So I don't either.

[...]
Alex Williamson - June 17, 2010, 10:25 p.m.
On Wed, 2010-06-16 at 10:23 +0200, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
> >> > > Alex proposed to disambiguate by adding "identified properties of the
> >> > > immediate parent bus and device" to the path component.  For PCI, these
> >> > > are dev.fn.  Likewise for any other bus where devices have unambigous
> >> > > bus address.  The driver name carries no information!
> >> > 
> >> > From user POV, driver names are very handly to address a device
> >> > intuitively - except for the case you have tones of devices on the same
> >> > bus that are handled by the same driver. For that case we need to
> >> > augment the device name with a useful per-bus ID, derived from the bus
> >> > address where available, otherwise based on instance numbers.
> >> 
> >> This is where I think you're missing a trick. We don't need to augment the 
> >> name, we just need to allow the bus id to be used instead.
> >
> > For the case of a hot remove, I agree.  If the user specifies "pci_del
> > pci.0/03.0", that's completely sufficient because we don't care what's
> > in that slot, just remove it.  However, I still see some use cases for
> > device names in the path.  Take for example:
> >
> > (A): /i440FX-pcihost/pci.0/e1000.05.0
> >
> > vs
> >
> > (B): /pci.0/05.0
> >
> > (removing both the root bridge driver name and the device driver name)
> 
> / is the main system bus.  System bus defines no bus address at the
> moment.  Therefore, you have to use the driver name i440FX-pcihost.

So is the general rule "If a device's parent bus does not provide an
address, print device name"?

> /i440FX-pcihost/pci.0 is the PCI bus.  PCI bus defines a bus address:
> dev.fn.  Therefore, you can either use the bus address @05.0, or the
> driver name e1000.
> 
> We have "/i440FX-pcihost/pci.0/e1000" vs. "/i440FX-pcihost/pci.0/@05.0".

I object to being able to use anything but an address under a bus that
supports hotplug, but that aside, I think the paths for my example
system look like below.  Note that anything behind and including the $
is not the canonical path, that begins the free form, usage specific
string, here filled by missing device names (open to suggestions other
than $ here).  Note that were isa devices do not have a bus identifier,
I'm using the device name, which I think follows the same rule as
i440FX-pcihost.  Can we agree this is the goal?  Thanks,

Alex

/i440FX-pcihost/pci.0/@00.0$i440FX
/i440FX-pcihost/pci.0/@01.0$PIIX3
/i440FX-pcihost/pci.0/@02.0$cirrus-vga
/i440FX-pcihost/pci.0/@01.0/isa.0/mc146818rtc
/i440FX-pcihost/pci.0/@01.0/isa.0/@0x3f8$isa-serial
/i440FX-pcihost/pci.0/@01.0/isa.0/@0x378$isa-parallel
/i440FX-pcihost/pci.0/@01.0/isa.0/i8042
/i440FX-pcihost/pci.0/@01.0/isa.0/isa-fdc
/i440FX-pcihost/pci.0/@03.0$i82551
/i440FX-pcihost/pci.0/@04.0$virtio-net-pci
/i440FX-pcihost/pci.0/@05.0$e1000
/i440FX-pcihost/pci.0/@06.0$rtl8139
/i440FX-pcihost/pci.0/@07.0$pcnet
/i440FX-pcihost/pci.0/@01.0/isa.0/@0x300$ne2k_isa
/i440FX-pcihost/pci.0/@01.1$piix3-ide
/i440FX-pcihost/pci.0/@01.1/ide.0/@0$ide-drive
/i440FX-pcihost/pci.0/@01.1/ide.1/@0$ide-drive
/i440FX-pcihost/pci.0/@01.2$piix3-usb-uhci
/i440FX-pcihost/pci.0/@01.3$PIIX4_PM
/i440FX-pcihost/pci.0/@01.3/i2c/@80$smbus-eeprom
/i440FX-pcihost/pci.0/@01.3/i2c/@81$smbus-eeprom
/i440FX-pcihost/pci.0/@01.3/i2c/@82$smbus-eeprom
/i440FX-pcihost/pci.0/@01.3/i2c/@83$smbus-eeprom
/i440FX-pcihost/pci.0/@01.3/i2c/@84$smbus-eeprom
/i440FX-pcihost/pci.0/@01.3/i2c/@85$smbus-eeprom
/i440FX-pcihost/pci.0/@01.3/i2c/@86$smbus-eeprom
/i440FX-pcihost/pci.0/@01.3/i2c/@87$smbus-eeprom
/i440FX-pcihost/pci.0/@08.0/scsi.0/@0$scsi-disk
/i440FX-pcihost/pci.0/@08.0/scsi.0/@3$scsi-disk
/i440FX-pcihost/pci.0/@08.0$lsi53c895a
/i440FX-pcihost/pci.0/@01.2/usb.0/@usb0/scsi.0/@0$scsi-disk
/i440FX-pcihost/pci.0/@01.2/usb.0/@usb0$usb-storage
/i440FX-pcihost/pci.0/@09.0$virtio-blk-pci
Jan Kiszka - June 18, 2010, 9:16 a.m.
Alex Williamson wrote:
> On Wed, 2010-06-16 at 10:23 +0200, Markus Armbruster wrote:
>> Alex Williamson <alex.williamson@redhat.com> writes:
>>
>>> On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
>>>>>> Alex proposed to disambiguate by adding "identified properties of the
>>>>>> immediate parent bus and device" to the path component.  For PCI, these
>>>>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>>>>> bus address.  The driver name carries no information!
>>>>> From user POV, driver names are very handly to address a device
>>>>> intuitively - except for the case you have tones of devices on the same
>>>>> bus that are handled by the same driver. For that case we need to
>>>>> augment the device name with a useful per-bus ID, derived from the bus
>>>>> address where available, otherwise based on instance numbers.
>>>> This is where I think you're missing a trick. We don't need to augment the 
>>>> name, we just need to allow the bus id to be used instead.
>>> For the case of a hot remove, I agree.  If the user specifies "pci_del
>>> pci.0/03.0", that's completely sufficient because we don't care what's
>>> in that slot, just remove it.  However, I still see some use cases for
>>> device names in the path.  Take for example:
>>>
>>> (A): /i440FX-pcihost/pci.0/e1000.05.0
>>>
>>> vs
>>>
>>> (B): /pci.0/05.0
>>>
>>> (removing both the root bridge driver name and the device driver name)
>> / is the main system bus.  System bus defines no bus address at the
>> moment.  Therefore, you have to use the driver name i440FX-pcihost.
> 
> So is the general rule "If a device's parent bus does not provide an
> address, print device name"?

What is the device name?

> 
>> /i440FX-pcihost/pci.0 is the PCI bus.  PCI bus defines a bus address:
>> dev.fn.  Therefore, you can either use the bus address @05.0, or the
>> driver name e1000.
>>
>> We have "/i440FX-pcihost/pci.0/e1000" vs. "/i440FX-pcihost/pci.0/@05.0".
> 
> I object to being able to use anything but an address under a bus that
> supports hotplug, but that aside, I think the paths for my example
> system look like below.  Note that anything behind and including the $
> is not the canonical path, that begins the free form, usage specific
> string, here filled by missing device names (open to suggestions other
> than $ here).

What about ":" instead of "$"? Visually more appealing IMO.

>  Note that were isa devices do not have a bus identifier,
> I'm using the device name,

Don't understand. Why don't they have their I/O base as prefix? This is
inconsistent, and if we consider anything behind "$" (or ":")
informative, the bus address is mandatory for any device (on a bus with
an addressing scheme).

> which I think follows the same rule as
> i440FX-pcihost.  Can we agree this is the goal?  Thanks,

It looks almost acceptable from my POV. We just need to define how to
get hold of devices on buses without an addressing scheme (e.g. the
system bus). Using the driver name is insufficient once you have > 1
devices of the same type. Introducing device instance numbers to those
buses would be straightforward IMO.

Once this is done, the next step should be a specification for later use
in docs/qdev-device-use.txt.

Jan

> 
> Alex
> 
> /i440FX-pcihost/pci.0/@00.0$i440FX
> /i440FX-pcihost/pci.0/@01.0$PIIX3
> /i440FX-pcihost/pci.0/@02.0$cirrus-vga
> /i440FX-pcihost/pci.0/@01.0/isa.0/mc146818rtc
> /i440FX-pcihost/pci.0/@01.0/isa.0/@0x3f8$isa-serial
> /i440FX-pcihost/pci.0/@01.0/isa.0/@0x378$isa-parallel
> /i440FX-pcihost/pci.0/@01.0/isa.0/i8042
> /i440FX-pcihost/pci.0/@01.0/isa.0/isa-fdc
> /i440FX-pcihost/pci.0/@03.0$i82551
> /i440FX-pcihost/pci.0/@04.0$virtio-net-pci
> /i440FX-pcihost/pci.0/@05.0$e1000
> /i440FX-pcihost/pci.0/@06.0$rtl8139
> /i440FX-pcihost/pci.0/@07.0$pcnet
> /i440FX-pcihost/pci.0/@01.0/isa.0/@0x300$ne2k_isa
> /i440FX-pcihost/pci.0/@01.1$piix3-ide
> /i440FX-pcihost/pci.0/@01.1/ide.0/@0$ide-drive
> /i440FX-pcihost/pci.0/@01.1/ide.1/@0$ide-drive
> /i440FX-pcihost/pci.0/@01.2$piix3-usb-uhci
> /i440FX-pcihost/pci.0/@01.3$PIIX4_PM
> /i440FX-pcihost/pci.0/@01.3/i2c/@80$smbus-eeprom
> /i440FX-pcihost/pci.0/@01.3/i2c/@81$smbus-eeprom
> /i440FX-pcihost/pci.0/@01.3/i2c/@82$smbus-eeprom
> /i440FX-pcihost/pci.0/@01.3/i2c/@83$smbus-eeprom
> /i440FX-pcihost/pci.0/@01.3/i2c/@84$smbus-eeprom
> /i440FX-pcihost/pci.0/@01.3/i2c/@85$smbus-eeprom
> /i440FX-pcihost/pci.0/@01.3/i2c/@86$smbus-eeprom
> /i440FX-pcihost/pci.0/@01.3/i2c/@87$smbus-eeprom
> /i440FX-pcihost/pci.0/@08.0/scsi.0/@0$scsi-disk
> /i440FX-pcihost/pci.0/@08.0/scsi.0/@3$scsi-disk
> /i440FX-pcihost/pci.0/@08.0$lsi53c895a
> /i440FX-pcihost/pci.0/@01.2/usb.0/@usb0/scsi.0/@0$scsi-disk
> /i440FX-pcihost/pci.0/@01.2/usb.0/@usb0$usb-storage
> /i440FX-pcihost/pci.0/@09.0$virtio-blk-pci
>
Markus Armbruster - June 18, 2010, 2:03 p.m.
Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 2010-06-16 at 10:23 +0200, Markus Armbruster wrote:
>> Alex Williamson <alex.williamson@redhat.com> writes:
>> 
>> > On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
>> >> > > Alex proposed to disambiguate by adding "identified properties of the
>> >> > > immediate parent bus and device" to the path component.  For PCI, these
>> >> > > are dev.fn.  Likewise for any other bus where devices have unambigous
>> >> > > bus address.  The driver name carries no information!
>> >> > 
>> >> > From user POV, driver names are very handly to address a device
>> >> > intuitively - except for the case you have tones of devices on the same
>> >> > bus that are handled by the same driver. For that case we need to
>> >> > augment the device name with a useful per-bus ID, derived from the bus
>> >> > address where available, otherwise based on instance numbers.
>> >> 
>> >> This is where I think you're missing a trick. We don't need to augment the 
>> >> name, we just need to allow the bus id to be used instead.
>> >
>> > For the case of a hot remove, I agree.  If the user specifies "pci_del
>> > pci.0/03.0", that's completely sufficient because we don't care what's
>> > in that slot, just remove it.  However, I still see some use cases for
>> > device names in the path.  Take for example:
>> >
>> > (A): /i440FX-pcihost/pci.0/e1000.05.0
>> >
>> > vs
>> >
>> > (B): /pci.0/05.0
>> >
>> > (removing both the root bridge driver name and the device driver name)
>> 
>> / is the main system bus.  System bus defines no bus address at the
>> moment.  Therefore, you have to use the driver name i440FX-pcihost.
>
> So is the general rule "If a device's parent bus does not provide an
> address, print device name"?

I think the general rule for constructing a *canonical* qdev path should
be:

* If it's the main system bus, the path is /.

* If it's another bus, the path is P/B, where P is the canonical path of
  the device providing the bus, and B is the bus name.  Unambiguous,
  since no device ever defines two buses with the same name.

* If it's a device, the path is P/D, where P is the canonical path of
  the bus.  If the bus defines bus addresses, then D is @A, where A is
  the device's bus address.

  We haven't made up our minds whether the else case exists, or what to
  do if it does.  The simple "else D is the device model driver's name"
  works only if the bus can't take multiple device models with the same
  driver.

The canonical path is not the only path.  For instance, a qdev ID is a
valid path, but it's not canonical.  /i440FX-pcihost/pci.0/e1000 is
another valid, non-canonical path.

[...]
Jan Kiszka - June 18, 2010, 2:14 p.m.
Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
>> On Wed, 2010-06-16 at 10:23 +0200, Markus Armbruster wrote:
>>> Alex Williamson <alex.williamson@redhat.com> writes:
>>>
>>>> On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
>>>>>>> Alex proposed to disambiguate by adding "identified properties of the
>>>>>>> immediate parent bus and device" to the path component.  For PCI, these
>>>>>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>>>>>> bus address.  The driver name carries no information!
>>>>>> From user POV, driver names are very handly to address a device
>>>>>> intuitively - except for the case you have tones of devices on the same
>>>>>> bus that are handled by the same driver. For that case we need to
>>>>>> augment the device name with a useful per-bus ID, derived from the bus
>>>>>> address where available, otherwise based on instance numbers.
>>>>> This is where I think you're missing a trick. We don't need to augment the 
>>>>> name, we just need to allow the bus id to be used instead.
>>>> For the case of a hot remove, I agree.  If the user specifies "pci_del
>>>> pci.0/03.0", that's completely sufficient because we don't care what's
>>>> in that slot, just remove it.  However, I still see some use cases for
>>>> device names in the path.  Take for example:
>>>>
>>>> (A): /i440FX-pcihost/pci.0/e1000.05.0
>>>>
>>>> vs
>>>>
>>>> (B): /pci.0/05.0
>>>>
>>>> (removing both the root bridge driver name and the device driver name)
>>> / is the main system bus.  System bus defines no bus address at the
>>> moment.  Therefore, you have to use the driver name i440FX-pcihost.
>> So is the general rule "If a device's parent bus does not provide an
>> address, print device name"?
> 
> I think the general rule for constructing a *canonical* qdev path should
> be:
> 
> * If it's the main system bus, the path is /.
> 
> * If it's another bus, the path is P/B, where P is the canonical path of
>   the device providing the bus, and B is the bus name.  Unambiguous,
>   since no device ever defines two buses with the same name.
> 
> * If it's a device, the path is P/D, where P is the canonical path of
>   the bus.  If the bus defines bus addresses, then D is @A, where A is
>   the device's bus address.
> 
>   We haven't made up our minds whether the else case exists, or what to
>   do if it does.  The simple "else D is the device model driver's name"
>   works only if the bus can't take multiple device models with the same
>   driver.

...which is already on x86 the case with multiple APICs or HPETs on the
system bus.

> 
> The canonical path is not the only path.  For instance, a qdev ID is a
> valid path, but it's not canonical.  /i440FX-pcihost/pci.0/e1000 is
> another valid, non-canonical path.

Not only canonical paths need to be specified, also alias like the
above. We should limit those alias to a required minimum ("required"
means to me: improves human-friendliness compared to canonical form and
preserves backward compatibility where relevant).

Jan
Alex Williamson - June 18, 2010, 3:01 p.m.
On Fri, 2010-06-18 at 11:16 +0200, Jan Kiszka wrote:
> Alex Williamson wrote:
> > On Wed, 2010-06-16 at 10:23 +0200, Markus Armbruster wrote:
> >> Alex Williamson <alex.williamson@redhat.com> writes:
> >>
> >>> On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
> >>>>>> Alex proposed to disambiguate by adding "identified properties of the
> >>>>>> immediate parent bus and device" to the path component.  For PCI, these
> >>>>>> are dev.fn.  Likewise for any other bus where devices have unambigous
> >>>>>> bus address.  The driver name carries no information!
> >>>>> From user POV, driver names are very handly to address a device
> >>>>> intuitively - except for the case you have tones of devices on the same
> >>>>> bus that are handled by the same driver. For that case we need to
> >>>>> augment the device name with a useful per-bus ID, derived from the bus
> >>>>> address where available, otherwise based on instance numbers.
> >>>> This is where I think you're missing a trick. We don't need to augment the 
> >>>> name, we just need to allow the bus id to be used instead.
> >>> For the case of a hot remove, I agree.  If the user specifies "pci_del
> >>> pci.0/03.0", that's completely sufficient because we don't care what's
> >>> in that slot, just remove it.  However, I still see some use cases for
> >>> device names in the path.  Take for example:
> >>>
> >>> (A): /i440FX-pcihost/pci.0/e1000.05.0
> >>>
> >>> vs
> >>>
> >>> (B): /pci.0/05.0
> >>>
> >>> (removing both the root bridge driver name and the device driver name)
> >> / is the main system bus.  System bus defines no bus address at the
> >> moment.  Therefore, you have to use the driver name i440FX-pcihost.
> > 
> > So is the general rule "If a device's parent bus does not provide an
> > address, print device name"?
> 
> What is the device name?

dev->info->name

> > 
> >> /i440FX-pcihost/pci.0 is the PCI bus.  PCI bus defines a bus address:
> >> dev.fn.  Therefore, you can either use the bus address @05.0, or the
> >> driver name e1000.
> >>
> >> We have "/i440FX-pcihost/pci.0/e1000" vs. "/i440FX-pcihost/pci.0/@05.0".
> > 
> > I object to being able to use anything but an address under a bus that
> > supports hotplug, but that aside, I think the paths for my example
> > system look like below.  Note that anything behind and including the $
> > is not the canonical path, that begins the free form, usage specific
> > string, here filled by missing device names (open to suggestions other
> > than $ here).
> 
> What about ":" instead of "$"? Visually more appealing IMO.

Sure, that works.

> >  Note that were isa devices do not have a bus identifier,
> > I'm using the device name,
> 
> Don't understand. Why don't they have their I/O base as prefix? This is
> inconsistent, and if we consider anything behind "$" (or ":")
> informative, the bus address is mandatory for any device (on a bus with
> an addressing scheme).

Not all isa devices expose an iobase property.  isa doesn't have an
addressing scheme, it's basically a free for all of grabbing addresses
and interrupts and hoping they don't conflict with no means to uniquely
address a specific device.  That's why isa cards are loaded with jumpers
so you can try to move them around.

> > which I think follows the same rule as
> > i440FX-pcihost.  Can we agree this is the goal?  Thanks,
> 
> It looks almost acceptable from my POV. We just need to define how to
> get hold of devices on buses without an addressing scheme (e.g. the
> system bus). Using the driver name is insufficient once you have > 1
> devices of the same type. Introducing device instance numbers to those
> buses would be straightforward IMO.

Yep, as I mentioned, I can live with instance numbers for devices that
do not live on a hotpluggable bus.  Where will the instance number go?
A property on the parent bus, the device, field in the DeviceState?
Thanks,

Alex
Alex Williamson - June 18, 2010, 3:21 p.m.
On Fri, 2010-06-18 at 16:03 +0200, Markus Armbruster wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> > On Wed, 2010-06-16 at 10:23 +0200, Markus Armbruster wrote:
> >> Alex Williamson <alex.williamson@redhat.com> writes:
> >> >
> >> > (A): /i440FX-pcihost/pci.0/e1000.05.0
> >> >
> >> > vs
> >> >
> >> > (B): /pci.0/05.0
> >> >
> >> > (removing both the root bridge driver name and the device driver name)
> >> 
> >> / is the main system bus.  System bus defines no bus address at the
> >> moment.  Therefore, you have to use the driver name i440FX-pcihost.
> >
> > So is the general rule "If a device's parent bus does not provide an
> > address, print device name"?
> 
> I think the general rule for constructing a *canonical* qdev path should
> be:
> 
> * If it's the main system bus, the path is /.
> 
> * If it's another bus, the path is P/B, where P is the canonical path of
>   the device providing the bus, and B is the bus name.  Unambiguous,
>   since no device ever defines two buses with the same name.
> 
> * If it's a device, the path is P/D, where P is the canonical path of
>   the bus.  If the bus defines bus addresses, then D is @A, where A is
>   the device's bus address.
> 
>   We haven't made up our minds whether the else case exists, or what to
>   do if it does.  The simple "else D is the device model driver's name"
>   works only if the bus can't take multiple device models with the same
>   driver.

It certainly exists today, so do we make up bus addresses, do we use
driver name + instance number, or ...?  Both of these cases need to make
sure they're not artificially imposing a creation order than we can't
guarantee across migration.  Hopefully Paul has a better suggestion than
creation order.

> The canonical path is not the only path.  For instance, a qdev ID is a
> valid path, but it's not canonical.  /i440FX-pcihost/pci.0/e1000 is
> another valid, non-canonical path.

Is it not canonical because of "e1000" or because or "i440FX-pcihost".
It seems like both to me with the nuance that i440FX-pcihost doesn't
support multiple instances, so we know which one it is.  Is the correct
canonical path for this then:

/<TBD>/pci.0/@d.f

How do we make progress on defining that TBD and all the ones under isa
buses?  Thanks,

Alex
Jan Kiszka - June 18, 2010, 3:22 p.m.
Alex Williamson wrote:
> On Fri, 2010-06-18 at 11:16 +0200, Jan Kiszka wrote:
>> Alex Williamson wrote:
>>> On Wed, 2010-06-16 at 10:23 +0200, Markus Armbruster wrote:
>>>> Alex Williamson <alex.williamson@redhat.com> writes:
>>>>
>>>>> On Tue, 2010-06-15 at 12:28 +0100, Paul Brook wrote:
>>>>>>>> Alex proposed to disambiguate by adding "identified properties of the
>>>>>>>> immediate parent bus and device" to the path component.  For PCI, these
>>>>>>>> are dev.fn.  Likewise for any other bus where devices have unambigous
>>>>>>>> bus address.  The driver name carries no information!
>>>>>>> From user POV, driver names are very handly to address a device
>>>>>>> intuitively - except for the case you have tones of devices on the same
>>>>>>> bus that are handled by the same driver. For that case we need to
>>>>>>> augment the device name with a useful per-bus ID, derived from the bus
>>>>>>> address where available, otherwise based on instance numbers.
>>>>>> This is where I think you're missing a trick. We don't need to augment the 
>>>>>> name, we just need to allow the bus id to be used instead.
>>>>> For the case of a hot remove, I agree.  If the user specifies "pci_del
>>>>> pci.0/03.0", that's completely sufficient because we don't care what's
>>>>> in that slot, just remove it.  However, I still see some use cases for
>>>>> device names in the path.  Take for example:
>>>>>
>>>>> (A): /i440FX-pcihost/pci.0/e1000.05.0
>>>>>
>>>>> vs
>>>>>
>>>>> (B): /pci.0/05.0
>>>>>
>>>>> (removing both the root bridge driver name and the device driver name)
>>>> / is the main system bus.  System bus defines no bus address at the
>>>> moment.  Therefore, you have to use the driver name i440FX-pcihost.
>>> So is the general rule "If a device's parent bus does not provide an
>>> address, print device name"?
>> What is the device name?
> 
> dev->info->name

That's for me the driver name. However, this one needs extension by an
instance number.

> 
>>>> /i440FX-pcihost/pci.0 is the PCI bus.  PCI bus defines a bus address:
>>>> dev.fn.  Therefore, you can either use the bus address @05.0, or the
>>>> driver name e1000.
>>>>
>>>> We have "/i440FX-pcihost/pci.0/e1000" vs. "/i440FX-pcihost/pci.0/@05.0".
>>> I object to being able to use anything but an address under a bus that
>>> supports hotplug, but that aside, I think the paths for my example
>>> system look like below.  Note that anything behind and including the $
>>> is not the canonical path, that begins the free form, usage specific
>>> string, here filled by missing device names (open to suggestions other
>>> than $ here).
>> What about ":" instead of "$"? Visually more appealing IMO.
> 
> Sure, that works.
> 
>>>  Note that were isa devices do not have a bus identifier,
>>> I'm using the device name,
>> Don't understand. Why don't they have their I/O base as prefix? This is
>> inconsistent, and if we consider anything behind "$" (or ":")
>> informative, the bus address is mandatory for any device (on a bus with
>> an addressing scheme).
> 
> Not all isa devices expose an iobase property.  isa doesn't have an
> addressing scheme, it's basically a free for all of grabbing addresses
> and interrupts and hoping they don't conflict with no means to uniquely
> address a specific device.  That's why isa cards are loaded with jumpers
> so you can try to move them around.

The question is if there are ISA devices that do not use io, thus have
no such property. Paul pointed out that this might be unrealistic, and
in fact we have no such devices in QEMU so far.

> 
>>> which I think follows the same rule as
>>> i440FX-pcihost.  Can we agree this is the goal?  Thanks,
>> It looks almost acceptable from my POV. We just need to define how to
>> get hold of devices on buses without an addressing scheme (e.g. the
>> system bus). Using the driver name is insufficient once you have > 1
>> devices of the same type. Introducing device instance numbers to those
>> buses would be straightforward IMO.
> 
> Yep, as I mentioned, I can live with instance numbers for devices that
> do not live on a hotpluggable bus.  Where will the instance number go?
> A property on the parent bus, the device, field in the DeviceState?

So far I'm generating them on the fly. We could store them in the
DeviceState if aliases based on <driver-name>.<instance-no> shall be
forbidden on hotplug buses, i.e. the instance numbers will not change
during VM lifetime. but the question is if such a restriction of alias
(even if the are unstable) wouldn't make things too much asymmetric.

Jan

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 9ffdba7..e30df88 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -453,6 +453,7 @@  PropertyInfo qdev_prop_macaddr = {
     .name  = "macaddr",
     .type  = PROP_TYPE_MACADDR,
     .size  = sizeof(MACAddr),
+    .flags = PROP_FLAG_PATH,
     .parse = parse_mac,
     .print = print_mac,
 };
@@ -496,6 +497,7 @@  PropertyInfo qdev_prop_pci_devfn = {
     .name  = "pci-devfn",
     .type  = PROP_TYPE_UINT32,
     .size  = sizeof(uint32_t),
+    .flags = PROP_FLAG_PATH,
     .parse = parse_pci_devfn,
     .print = print_pci_devfn,
 };
diff --git a/hw/qdev.c b/hw/qdev.c
index 36f29ea..dea44fe 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -120,6 +120,63 @@  DeviceState *qdev_create(BusState *bus, const char *name)
     return qdev_create_from_info(bus, info);
 }
 
+static int qdev_strprint_parent_path(DeviceState *dev, char *buf, size_t len)
+{
+    int offset = 0;
+
+    if (dev->parent_bus && dev->parent_bus->parent)
+        offset = qdev_strprint_parent_path(dev->parent_bus->parent, buf, len);
+
+    offset += snprintf(buf + offset, len - offset,
+                        "/%s", dev->parent_bus->name);
+    return offset;
+}
+
+static int qdev_strprint_path_props(DeviceState *dev, Property *props,
+                                    char *buf, size_t len)
+{
+    int offset = 0;
+    char pbuf[64];
+
+    if (!props)
+        return 0;
+
+    while (props->name) {
+        if (props->info->flags & PROP_FLAG_PATH) {
+            if (props->info->print) {
+                props->info->print(dev, props, pbuf, sizeof(pbuf));
+                offset += snprintf(buf + offset, len - offset, ",%s=%s",
+                                   props->name, pbuf);
+            }
+        }
+        props++;
+    }
+    return offset;
+}
+
+char *qdev_get_dev_path(DeviceState *dev)
+{
+    char buf[256] = "";
+    int offset;
+
+    if (!dev)
+        return NULL;
+
+    offset = qdev_strprint_parent_path(dev, buf, sizeof(buf));
+
+    offset += qdev_strprint_path_props(dev, dev->parent_bus->info->props,
+                                       buf + offset, sizeof(buf) - offset);
+
+    offset += snprintf(buf + offset, sizeof(buf) - offset, "/%s",
+                       dev->info->name);
+    if (dev->id)
+        offset += snprintf(buf + offset, sizeof(buf) - offset, "-%s", dev->id);
+
+    offset += qdev_strprint_path_props(dev, dev->info->props,
+                                       buf + offset, sizeof(buf) - offset);
+    return qemu_strdup(buf);
+}
+
 static void qdev_print_devinfo(DeviceInfo *info)
 {
     error_printf("name \"%s\", bus %s",
diff --git a/hw/qdev.h b/hw/qdev.h
index a44060e..2702384 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -96,6 +96,7 @@  struct PropertyInfo {
     const char *name;
     size_t size;
     enum PropertyType type;
+    int flags;
     int (*parse)(DeviceState *dev, Property *prop, const char *str);
     int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
 };
@@ -201,6 +202,8 @@  extern PropertyInfo qdev_prop_netdev;
 extern PropertyInfo qdev_prop_vlan;
 extern PropertyInfo qdev_prop_pci_devfn;
 
+#define PROP_FLAG_PATH        (1<<0)
+
 #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
         .name      = (_name),                                    \
         .info      = &(_prop),                                   \
@@ -280,6 +283,8 @@  void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
 
+char *qdev_get_dev_path(DeviceState *dev);
+
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 extern struct BusInfo system_bus_info;