| Message ID | 1288525209-3303-2-git-send-email-gleb@redhat.com |
|---|---|
| State | New |
| Headers | show |
Gleb Natapov <gleb@redhat.com> writes: > Add "deriver_name" to DeviceInfo to use in device path building. In Typo "deriver". Same in subject. > contrast to "name" "driver_name" should refer to functionality device > provides instead of particular device model like "name" does. Why is that useful in a device path? I'm afraid "driver_name" is too confusing, because we already use "driver" and "name" for the name of the device model: DeviceInfo member name, and qemu_device_opts option name "driver". Perhaps something like "class"? > Signed-off-by: Gleb Natapov <gleb@redhat.com> > --- > hw/fdc.c | 1 + > hw/ide/isa.c | 1 + > hw/ide/piix.c | 1 + > hw/ide/qdev.c | 1 + > hw/isa-bus.c | 1 + > hw/piix_pci.c | 2 ++ > hw/qdev.h | 6 ++++++ > hw/virtio-pci.c | 2 ++ > 8 files changed, 15 insertions(+), 0 deletions(-) > > diff --git a/hw/fdc.c b/hw/fdc.c > index c159dcb..b4217a3 100644 > --- a/hw/fdc.c > +++ b/hw/fdc.c > @@ -2040,6 +2040,7 @@ static const VMStateDescription vmstate_isa_fdc ={ > static ISADeviceInfo isa_fdc_info = { > .init = isabus_fdc_init1, > .qdev.name = "isa-fdc", > + .qdev.driver_name = "fdc", > .qdev.size = sizeof(FDCtrlISABus), > .qdev.no_user = 1, > .qdev.vmsd = &vmstate_isa_fdc, > diff --git a/hw/ide/isa.c b/hw/ide/isa.c > index 6b57e0d..489cc99 100644 > --- a/hw/ide/isa.c > +++ b/hw/ide/isa.c > @@ -98,6 +98,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, > > static ISADeviceInfo isa_ide_info = { > .qdev.name = "isa-ide", > + .qdev.driver_name = "ata", > .qdev.size = sizeof(ISAIDEState), > .init = isa_ide_initfn, > .qdev.reset = isa_ide_reset, > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 07483e8..6206201 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -182,6 +182,7 @@ PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) > static PCIDeviceInfo piix_ide_info[] = { > { > .qdev.name = "piix3-ide", > + .qdev.driver_name = "ata", > .qdev.size = sizeof(PCIIDEState), > .qdev.no_user = 1, > .init = pci_piix3_ide_initfn, > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > index 0808760..341548e 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -134,6 +134,7 @@ static int ide_drive_initfn(IDEDevice *dev) > > static IDEDeviceInfo ide_drive_info = { > .qdev.name = "ide-drive", > + .qdev.driver_name = "ata-disk", > .qdev.size = sizeof(IDEDrive), > .init = ide_drive_initfn, > .qdev.props = (Property[]) { "ata-disk" is misleading, as it doesn't have to be a disk. "ata-drive"? Hmm, can't we stick to "ide-drive" just as well? > diff --git a/hw/isa-bus.c b/hw/isa-bus.c > index 4e306de..2c42b80 100644 > --- a/hw/isa-bus.c > +++ b/hw/isa-bus.c > @@ -153,6 +153,7 @@ static int isabus_bridge_init(SysBusDevice *dev) > static SysBusDeviceInfo isabus_bridge_info = { > .init = isabus_bridge_init, > .qdev.name = "isabus-bridge", > + .qdev.driver_name = "isa", > .qdev.size = sizeof(SysBusDevice), > .qdev.no_user = 1, > }; > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index 00060f3..4e5e5df 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -364,6 +364,7 @@ static PCIDeviceInfo i440fx_info[] = { > .config_write = i440fx_write_config, > },{ > .qdev.name = "PIIX3", > + .qdev.driver_name = "pci-isa-bridge", > .qdev.desc = "ISA bridge", > .qdev.size = sizeof(PIIX3State), > .qdev.vmsd = &vmstate_piix3, > @@ -377,6 +378,7 @@ static PCIDeviceInfo i440fx_info[] = { > static SysBusDeviceInfo i440fx_pcihost_info = { > .init = i440fx_pcihost_initfn, > .qdev.name = "i440FX-pcihost", > + .qdev.driver_name = "pci", > .qdev.size = sizeof(I440FXState), > .qdev.no_user = 1, > }; > diff --git a/hw/qdev.h b/hw/qdev.h > index 579328a..a9a98f8 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -139,6 +139,7 @@ typedef void (*qdev_resetfn)(DeviceState *dev); > > struct DeviceInfo { > const char *name; > + const char *driver_name; > const char *alias; > const char *desc; > size_t size; > @@ -288,6 +289,11 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props); > void qdev_prop_register_global_list(GlobalProperty *props); > void qdev_prop_set_globals(DeviceState *dev); > > +static inline const char *qdev_driver_name(DeviceState *dev) > +{ > + return dev->info->driver_name ? : dev->info->name; > +} > + Aha, it defaults to the device model name dev->info->name. That's why you get away with not defining it in most DeviceInfo. > /* This is a nasty hack to allow passing a NULL bus to qdev_create. */ > extern struct BusInfo system_bus_info; > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 2d78ca6..b67ded6 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -769,6 +769,7 @@ static int virtio_9p_init_pci(PCIDevice *pci_dev) > static PCIDeviceInfo virtio_info[] = { > { > .qdev.name = "virtio-blk-pci", > + .qdev.driver_name = "virtio-blk", > .qdev.size = sizeof(VirtIOPCIProxy), > .init = virtio_blk_init_pci, > .exit = virtio_blk_exit_pci, > @@ -782,6 +783,7 @@ static PCIDeviceInfo virtio_info[] = { > .qdev.reset = virtio_pci_reset, > },{ > .qdev.name = "virtio-net-pci", > + .qdev.driver_name = "ethernet", > .qdev.size = sizeof(VirtIOPCIProxy), > .init = virtio_net_init_pci, > .exit = virtio_net_exit_pci, Do we want a free-for-all ad hoc set of values for driver_name? The values become ABI instantly... Can we adopt some existing set of names for device classes? Else, can we define our own with a bit more control? Note that a single device could implement more than one device class. I guess a sane PCI device would do that with a separate function each, so we'd be fine there. Do we want to hardcode "single class per qdev"?
On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > Add "deriver_name" to DeviceInfo to use in device path building. In > > Typo "deriver". Same in subject. > Heh. > > contrast to "name" "driver_name" should refer to functionality device > > provides instead of particular device model like "name" does. > > Why is that useful in a device path? > Sometimes it is sometimes it is not. Lets look at path like that: /pci@i0cf8/ethernet@4/ethernet-phy@0 It is important to have pci in the fist path element since it defines what kind of bus addressing is used by next element ethernet@4. 4 is slot number in case of pci bus. On the other hand ethernet part is not important since OS can figure it out by looking in pci config space. > I'm afraid "driver_name" is too confusing, because we already use > "driver" and "name" for the name of the device model: DeviceInfo member > name, and qemu_device_opts option name "driver". I use "driver_name" here since I am coding to Open Firmware spec now and this element in device path is called driver_name by the spec. > > Perhaps something like "class"? > > > Signed-off-by: Gleb Natapov <gleb@redhat.com> > > --- > > hw/fdc.c | 1 + > > hw/ide/isa.c | 1 + > > hw/ide/piix.c | 1 + > > hw/ide/qdev.c | 1 + > > hw/isa-bus.c | 1 + > > hw/piix_pci.c | 2 ++ > > hw/qdev.h | 6 ++++++ > > hw/virtio-pci.c | 2 ++ > > 8 files changed, 15 insertions(+), 0 deletions(-) > > > > diff --git a/hw/fdc.c b/hw/fdc.c > > index c159dcb..b4217a3 100644 > > --- a/hw/fdc.c > > +++ b/hw/fdc.c > > @@ -2040,6 +2040,7 @@ static const VMStateDescription vmstate_isa_fdc ={ > > static ISADeviceInfo isa_fdc_info = { > > .init = isabus_fdc_init1, > > .qdev.name = "isa-fdc", > > + .qdev.driver_name = "fdc", > > .qdev.size = sizeof(FDCtrlISABus), > > .qdev.no_user = 1, > > .qdev.vmsd = &vmstate_isa_fdc, > > diff --git a/hw/ide/isa.c b/hw/ide/isa.c > > index 6b57e0d..489cc99 100644 > > --- a/hw/ide/isa.c > > +++ b/hw/ide/isa.c > > @@ -98,6 +98,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, > > > > static ISADeviceInfo isa_ide_info = { > > .qdev.name = "isa-ide", > > + .qdev.driver_name = "ata", > > .qdev.size = sizeof(ISAIDEState), > > .init = isa_ide_initfn, > > .qdev.reset = isa_ide_reset, > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > > index 07483e8..6206201 100644 > > --- a/hw/ide/piix.c > > +++ b/hw/ide/piix.c > > @@ -182,6 +182,7 @@ PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) > > static PCIDeviceInfo piix_ide_info[] = { > > { > > .qdev.name = "piix3-ide", > > + .qdev.driver_name = "ata", > > .qdev.size = sizeof(PCIIDEState), > > .qdev.no_user = 1, > > .init = pci_piix3_ide_initfn, > > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > > index 0808760..341548e 100644 > > --- a/hw/ide/qdev.c > > +++ b/hw/ide/qdev.c > > @@ -134,6 +134,7 @@ static int ide_drive_initfn(IDEDevice *dev) > > > > static IDEDeviceInfo ide_drive_info = { > > .qdev.name = "ide-drive", > > + .qdev.driver_name = "ata-disk", > > .qdev.size = sizeof(IDEDrive), > > .init = ide_drive_initfn, > > .qdev.props = (Property[]) { > > "ata-disk" is misleading, as it doesn't have to be a disk. "ata-drive"? > Hmm, can't we stick to "ide-drive" just as well? > IIRC I changed this to just "disk" already. > > diff --git a/hw/isa-bus.c b/hw/isa-bus.c > > index 4e306de..2c42b80 100644 > > --- a/hw/isa-bus.c > > +++ b/hw/isa-bus.c > > @@ -153,6 +153,7 @@ static int isabus_bridge_init(SysBusDevice *dev) > > static SysBusDeviceInfo isabus_bridge_info = { > > .init = isabus_bridge_init, > > .qdev.name = "isabus-bridge", > > + .qdev.driver_name = "isa", > > .qdev.size = sizeof(SysBusDevice), > > .qdev.no_user = 1, > > }; > > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > > index 00060f3..4e5e5df 100644 > > --- a/hw/piix_pci.c > > +++ b/hw/piix_pci.c > > @@ -364,6 +364,7 @@ static PCIDeviceInfo i440fx_info[] = { > > .config_write = i440fx_write_config, > > },{ > > .qdev.name = "PIIX3", > > + .qdev.driver_name = "pci-isa-bridge", > > .qdev.desc = "ISA bridge", > > .qdev.size = sizeof(PIIX3State), > > .qdev.vmsd = &vmstate_piix3, > > @@ -377,6 +378,7 @@ static PCIDeviceInfo i440fx_info[] = { > > static SysBusDeviceInfo i440fx_pcihost_info = { > > .init = i440fx_pcihost_initfn, > > .qdev.name = "i440FX-pcihost", > > + .qdev.driver_name = "pci", > > .qdev.size = sizeof(I440FXState), > > .qdev.no_user = 1, > > }; > > diff --git a/hw/qdev.h b/hw/qdev.h > > index 579328a..a9a98f8 100644 > > --- a/hw/qdev.h > > +++ b/hw/qdev.h > > @@ -139,6 +139,7 @@ typedef void (*qdev_resetfn)(DeviceState *dev); > > > > struct DeviceInfo { > > const char *name; > > + const char *driver_name; > > const char *alias; > > const char *desc; > > size_t size; > > @@ -288,6 +289,11 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props); > > void qdev_prop_register_global_list(GlobalProperty *props); > > void qdev_prop_set_globals(DeviceState *dev); > > > > +static inline const char *qdev_driver_name(DeviceState *dev) > > +{ > > + return dev->info->driver_name ? : dev->info->name; > > +} > > + > > Aha, it defaults to the device model name dev->info->name. That's why > you get away with not defining it in most DeviceInfo. > > > /* This is a nasty hack to allow passing a NULL bus to qdev_create. */ > > extern struct BusInfo system_bus_info; > > > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > > index 2d78ca6..b67ded6 100644 > > --- a/hw/virtio-pci.c > > +++ b/hw/virtio-pci.c > > @@ -769,6 +769,7 @@ static int virtio_9p_init_pci(PCIDevice *pci_dev) > > static PCIDeviceInfo virtio_info[] = { > > { > > .qdev.name = "virtio-blk-pci", > > + .qdev.driver_name = "virtio-blk", > > .qdev.size = sizeof(VirtIOPCIProxy), > > .init = virtio_blk_init_pci, > > .exit = virtio_blk_exit_pci, > > @@ -782,6 +783,7 @@ static PCIDeviceInfo virtio_info[] = { > > .qdev.reset = virtio_pci_reset, > > },{ > > .qdev.name = "virtio-net-pci", > > + .qdev.driver_name = "ethernet", > > .qdev.size = sizeof(VirtIOPCIProxy), > > .init = virtio_net_init_pci, > > .exit = virtio_net_exit_pci, > > Do we want a free-for-all ad hoc set of values for driver_name? The > values become ABI instantly... Can we adopt some existing set of names > for device classes? Else, can we define our own with a bit more > control? > I am open to suggestion. Open firmware describes this field as: The driver name field is a sequence of between one and 31 letters, digits, and punctuation characters from the set “, . _ + - ”. Uppercase and lowercase characters are distinct. By convention, this name includes the name of the device’s manufacturer and the device’s model name separated by a “,”. (See the definition of “name” in annex A.) Inclusion of the manufacturer name within driver name is especially important for devices intended to plug into standard buses, as this minimizes the risk of accidental name collisions. It is somewhat less important for devices that are permanently attached to a particular system. If the manufacturer name component is omitted (i.e., there is no “,” within the driver name), the convention is to assume that the manufacturer name is the same as that of the nearest ancestor node (parent node, or grandparent node, etc.) that has an explicit manufacturer name component. I am looking on existing implementations an copy what they do. > Note that a single device could implement more than one device class. I > guess a sane PCI device would do that with a separate function each, so > we'd be fine there. Do we want to hardcode "single class per qdev"? Isn't it already the case? -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > Add "deriver_name" to DeviceInfo to use in device path building. In >> >> Typo "deriver". Same in subject. >> > Heh. > >> > contrast to "name" "driver_name" should refer to functionality device >> > provides instead of particular device model like "name" does. >> >> Why is that useful in a device path? >> > Sometimes it is sometimes it is not. Lets look at path like that: > /pci@i0cf8/ethernet@4/ethernet-phy@0 > > It is important to have pci in the fist path element since it defines > what kind of bus addressing is used by next element ethernet@4. It is for consumers that don't know what's sitting at i0cf8 on the system bus. > 4 is > slot number in case of pci bus. On the other hand ethernet part is not > important since OS can figure it out by looking in pci config space. The OS does know what's sitting in slot 4 on the PCI bus. If the OS number doesn't know what's sitting at i0cf8, why is "pci" sufficient information? Why don't we have to distinguish between the different PCI host bridges? >> I'm afraid "driver_name" is too confusing, because we already use >> "driver" and "name" for the name of the device model: DeviceInfo member >> name, and qemu_device_opts option name "driver". > I use "driver_name" here since I am coding to Open Firmware spec now > and this element in device path is called driver_name by the spec. Why is it different from our DeviceInfo member name? We already have name (e.g. "lsi53c895a") and alias ("lsi"), why do we need a third? Do you envisage different device models sharing the same driver_name? [...] >> Do we want a free-for-all ad hoc set of values for driver_name? The >> values become ABI instantly... Can we adopt some existing set of names >> for device classes? Else, can we define our own with a bit more >> control? >> > I am open to suggestion. Open firmware describes this field as: > > The driver name field is a sequence of between one and 31 letters, digits, > and punctuation characters from the set “, . _ + - ”. Uppercase and > lowercase characters are distinct. By convention, this name includes > the name of the device’s manufacturer and the device’s model name > separated by a “,”. (See the definition of “name” in annex A.) > Inclusion of the manufacturer name within driver name is especially > important for devices intended to plug into standard buses, as this > minimizes the risk of accidental name collisions. It is somewhat less > important for devices that are permanently attached to a particular > system. If the manufacturer name component is omitted (i.e., there is > no “,” within the driver name), the convention is to assume that > the manufacturer name is the same as that of the nearest ancestor node > (parent node, or grandparent node, etc.) that has an explicit manufacturer > name component. I suspect that's exactly what DeviceInfo member name is. > I am looking on existing implementations an copy what they do. [...]
On Thu, Nov 04, 2010 at 03:58:03PM +0100, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > Add "deriver_name" to DeviceInfo to use in device path building. In > >> > >> Typo "deriver". Same in subject. > >> > > Heh. > > > >> > contrast to "name" "driver_name" should refer to functionality device > >> > provides instead of particular device model like "name" does. > >> > >> Why is that useful in a device path? > >> > > Sometimes it is sometimes it is not. Lets look at path like that: > > /pci@i0cf8/ethernet@4/ethernet-phy@0 > > > > It is important to have pci in the fist path element since it defines > > what kind of bus addressing is used by next element ethernet@4. > > It is for consumers that don't know what's sitting at i0cf8 on the > system bus. Yes. Same firmware may support different boards that have same pci controller but on different addresses. Device name may even contain manufacturer ID, so firmware will be able to find matching driver and support more board without recompiling. > > > 4 is > > slot number in case of pci bus. On the other hand ethernet part is not > > important since OS can figure it out by looking in pci config space. > > The OS does know what's sitting in slot 4 on the PCI bus. > Yes, and? That what I wrote above. "ethernet" part is redundant in case of PCI bus. A little bit of redundancy will not hurt and required by the spec. > If the OS number doesn't know what's sitting at i0cf8, why is "pci" > sufficient information? Why don't we have to distinguish between the > different PCI host bridges? Manufacturer ID may be put along with pci. Full FDT contains much more information about each node though. It may even list compatible HW. Here we are concerned with device paths only. > > >> I'm afraid "driver_name" is too confusing, because we already use > >> "driver" and "name" for the name of the device model: DeviceInfo member > >> name, and qemu_device_opts option name "driver". > > I use "driver_name" here since I am coding to Open Firmware spec now > > and this element in device path is called driver_name by the spec. > > Why is it different from our DeviceInfo member name? > > We already have name (e.g. "lsi53c895a") and alias ("lsi"), why do we > need a third? I haven't noticed we have alias! What is it used for? I think I can use it instead. > > Do you envisage different device models sharing the same driver_name? > Yes. isa-ide, piix3-ide, piix4-ide all provides exactly same ata bus. > [...] > >> Do we want a free-for-all ad hoc set of values for driver_name? The > >> values become ABI instantly... Can we adopt some existing set of names > >> for device classes? Else, can we define our own with a bit more > >> control? > >> > > I am open to suggestion. Open firmware describes this field as: > > > > The driver name field is a sequence of between one and 31 letters, digits, > > and punctuation characters from the set “, . _ + - ”. Uppercase and > > lowercase characters are distinct. By convention, this name includes > > the name of the device’s manufacturer and the device’s model name > > separated by a “,”. (See the definition of “name” in annex A.) > > Inclusion of the manufacturer name within driver name is especially > > important for devices intended to plug into standard buses, as this > > minimizes the risk of accidental name collisions. It is somewhat less > > important for devices that are permanently attached to a particular > > system. If the manufacturer name component is omitted (i.e., there is > > no “,” within the driver name), the convention is to assume that > > the manufacturer name is the same as that of the nearest ancestor node > > (parent node, or grandparent node, etc.) that has an explicit manufacturer > > name component. > > I suspect that's exactly what DeviceInfo member name is. > In cases where this is the case I just use "name". > > I am looking on existing implementations an copy what they do. > [...] -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Thu, Nov 04, 2010 at 03:58:03PM +0100, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > Add "deriver_name" to DeviceInfo to use in device path building. In >> >> >> >> Typo "deriver". Same in subject. >> >> >> > Heh. >> > >> >> > contrast to "name" "driver_name" should refer to functionality device >> >> > provides instead of particular device model like "name" does. >> >> >> >> Why is that useful in a device path? >> >> >> > Sometimes it is sometimes it is not. Lets look at path like that: >> > /pci@i0cf8/ethernet@4/ethernet-phy@0 >> > >> > It is important to have pci in the fist path element since it defines >> > what kind of bus addressing is used by next element ethernet@4. >> >> It is for consumers that don't know what's sitting at i0cf8 on the >> system bus. > Yes. Same firmware may support different boards that have same pci > controller but on different addresses. Device name may even contain > manufacturer ID, so firmware will be able to find matching driver and > support more board without recompiling. "pci" tells us it's some kind of PCI host bridge. Why is that enough? Why don't we have to identify the particular host bridge, such as "i440FX-pcihost"? >> > 4 is >> > slot number in case of pci bus. On the other hand ethernet part is not >> > important since OS can figure it out by looking in pci config space. >> >> The OS does know what's sitting in slot 4 on the PCI bus. >> > Yes, and? That what I wrote above. "ethernet" part is redundant in case > of PCI bus. A little bit of redundancy will not hurt and required by the > spec. > >> If the OS number doesn't know what's sitting at i0cf8, why is "pci" >> sufficient information? Why don't we have to distinguish between the >> different PCI host bridges? > Manufacturer ID may be put along with pci. Full FDT contains much more > information about each node though. It may even list compatible HW. Here > we are concerned with device paths only. > >> >> >> I'm afraid "driver_name" is too confusing, because we already use >> >> "driver" and "name" for the name of the device model: DeviceInfo member >> >> name, and qemu_device_opts option name "driver". >> > I use "driver_name" here since I am coding to Open Firmware spec now >> > and this element in device path is called driver_name by the spec. >> >> Why is it different from our DeviceInfo member name? >> >> We already have name (e.g. "lsi53c895a") and alias ("lsi"), why do we >> need a third? > I haven't noticed we have alias! What is it used for? I think I can use > it instead. > >> >> Do you envisage different device models sharing the same driver_name? >> > Yes. isa-ide, piix3-ide, piix4-ide all provides exactly same ata bus. But they're different devices! Isn't Open Firmware's "driver name" meant to identify a device type unambigously? Consider the case of an ISA soundcard providing an IDE channel. Want to call it "ata", too? >> [...] >> >> Do we want a free-for-all ad hoc set of values for driver_name? The >> >> values become ABI instantly... Can we adopt some existing set of names >> >> for device classes? Else, can we define our own with a bit more >> >> control? >> >> >> > I am open to suggestion. Open firmware describes this field as: >> > >> > The driver name field is a sequence of between one and 31 letters, digits, >> > and punctuation characters from the set “, . _ + - ”. Uppercase and >> > lowercase characters are distinct. By convention, this name includes >> > the name of the device’s manufacturer and the device’s model name >> > separated by a “,”. (See the definition of “name” in annex A.) >> > Inclusion of the manufacturer name within driver name is especially >> > important for devices intended to plug into standard buses, as this >> > minimizes the risk of accidental name collisions. It is somewhat less >> > important for devices that are permanently attached to a particular >> > system. If the manufacturer name component is omitted (i.e., there is >> > no “,” within the driver name), the convention is to assume that >> > the manufacturer name is the same as that of the nearest ancestor node >> > (parent node, or grandparent node, etc.) that has an explicit manufacturer >> > name component. >> >> I suspect that's exactly what DeviceInfo member name is. >> > In cases where this is the case I just use "name". > >> > I am looking on existing implementations an copy what they do. >> [...]
On Fri, Nov 05, 2010 at 03:14:20PM +0100, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Thu, Nov 04, 2010 at 03:58:03PM +0100, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > Add "deriver_name" to DeviceInfo to use in device path building. In > >> >> > >> >> Typo "deriver". Same in subject. > >> >> > >> > Heh. > >> > > >> >> > contrast to "name" "driver_name" should refer to functionality device > >> >> > provides instead of particular device model like "name" does. > >> >> > >> >> Why is that useful in a device path? > >> >> > >> > Sometimes it is sometimes it is not. Lets look at path like that: > >> > /pci@i0cf8/ethernet@4/ethernet-phy@0 > >> > > >> > It is important to have pci in the fist path element since it defines > >> > what kind of bus addressing is used by next element ethernet@4. > >> > >> It is for consumers that don't know what's sitting at i0cf8 on the > >> system bus. > > Yes. Same firmware may support different boards that have same pci > > controller but on different addresses. Device name may even contain > > manufacturer ID, so firmware will be able to find matching driver and > > support more board without recompiling. > > "pci" tells us it's some kind of PCI host bridge. Why is that enough? > Why don't we have to identify the particular host bridge, such as > "i440FX-pcihost"? > As I said below manufacturer ID may be part of device name. It should be separated by comma though. Something like "i440FX,pci". But for x86 qemu emulation this is not needed since all chipsets implement essentially the same pci controller. Other platforms qemu emulates may use more elaborate names. Other platforms may want to get full FDT tree from qemu anyway. > >> > 4 is > >> > slot number in case of pci bus. On the other hand ethernet part is not > >> > important since OS can figure it out by looking in pci config space. > >> > >> The OS does know what's sitting in slot 4 on the PCI bus. > >> > > Yes, and? That what I wrote above. "ethernet" part is redundant in case > > of PCI bus. A little bit of redundancy will not hurt and required by the > > spec. > > > >> If the OS number doesn't know what's sitting at i0cf8, why is "pci" > >> sufficient information? Why don't we have to distinguish between the > >> different PCI host bridges? > > Manufacturer ID may be put along with pci. Full FDT contains much more > > information about each node though. It may even list compatible HW. Here > > we are concerned with device paths only. Here I said it already :) > > > >> > >> >> I'm afraid "driver_name" is too confusing, because we already use > >> >> "driver" and "name" for the name of the device model: DeviceInfo member > >> >> name, and qemu_device_opts option name "driver". > >> > I use "driver_name" here since I am coding to Open Firmware spec now > >> > and this element in device path is called driver_name by the spec. > >> > >> Why is it different from our DeviceInfo member name? > >> > >> We already have name (e.g. "lsi53c895a") and alias ("lsi"), why do we > >> need a third? > > I haven't noticed we have alias! What is it used for? I think I can use > > it instead. > > > >> > >> Do you envisage different device models sharing the same driver_name? > >> > > Yes. isa-ide, piix3-ide, piix4-ide all provides exactly same ata bus. > > But they're different devices! Isn't Open Firmware's "driver name" > meant to identify a device type unambigously? > Not necessary as far as I see from examples. Full FDT contains more info. In this case all of those different devices present exactly same HW interface, so from FW point of view they are the same. To make FW life more easy it is better to have one name for all of them. > Consider the case of an ISA soundcard providing an IDE channel. Want to > call it "ata", too? If it is exactly like interface provided by devices above why FW cares that this is soundcard? -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Fri, Nov 05, 2010 at 03:14:20PM +0100, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Thu, Nov 04, 2010 at 03:58:03PM +0100, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: >> >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> >> >> > Add "deriver_name" to DeviceInfo to use in device path building. In >> >> >> >> >> >> Typo "deriver". Same in subject. >> >> >> >> >> > Heh. >> >> > >> >> >> > contrast to "name" "driver_name" should refer to functionality device >> >> >> > provides instead of particular device model like "name" does. >> >> >> >> >> >> Why is that useful in a device path? >> >> >> >> >> > Sometimes it is sometimes it is not. Lets look at path like that: >> >> > /pci@i0cf8/ethernet@4/ethernet-phy@0 >> >> > >> >> > It is important to have pci in the fist path element since it defines >> >> > what kind of bus addressing is used by next element ethernet@4. >> >> >> >> It is for consumers that don't know what's sitting at i0cf8 on the >> >> system bus. >> > Yes. Same firmware may support different boards that have same pci >> > controller but on different addresses. Device name may even contain >> > manufacturer ID, so firmware will be able to find matching driver and >> > support more board without recompiling. >> >> "pci" tells us it's some kind of PCI host bridge. Why is that enough? >> Why don't we have to identify the particular host bridge, such as >> "i440FX-pcihost"? >> > As I said below manufacturer ID may be part of device name. It should be > separated by comma though. Something like "i440FX,pci". I'd expect "intel,i440FX". Note that comma makes for extremely user-hostile -device usage. Right now, it doesn't work at all. > But for x86 qemu > emulation this is not needed since all chipsets implement essentially > the same pci controller. Will it stay that way? What about Isaku's q35 work? > Other platforms qemu emulates may use more > elaborate names. Other platforms may want to get full FDT tree from > qemu anyway. > >> >> > 4 is >> >> > slot number in case of pci bus. On the other hand ethernet part is not >> >> > important since OS can figure it out by looking in pci config space. >> >> >> >> The OS does know what's sitting in slot 4 on the PCI bus. >> >> >> > Yes, and? That what I wrote above. "ethernet" part is redundant in case >> > of PCI bus. A little bit of redundancy will not hurt and required by the >> > spec. >> > >> >> If the OS number doesn't know what's sitting at i0cf8, why is "pci" >> >> sufficient information? Why don't we have to distinguish between the >> >> different PCI host bridges? >> > Manufacturer ID may be put along with pci. Full FDT contains much more >> > information about each node though. It may even list compatible HW. Here >> > we are concerned with device paths only. > Here I said it already :) > >> > >> >> >> >> >> I'm afraid "driver_name" is too confusing, because we already use >> >> >> "driver" and "name" for the name of the device model: DeviceInfo member >> >> >> name, and qemu_device_opts option name "driver". >> >> > I use "driver_name" here since I am coding to Open Firmware spec now >> >> > and this element in device path is called driver_name by the spec. >> >> >> >> Why is it different from our DeviceInfo member name? >> >> >> >> We already have name (e.g. "lsi53c895a") and alias ("lsi"), why do we >> >> need a third? >> > I haven't noticed we have alias! What is it used for? I think I can use >> > it instead. >> > >> >> >> >> Do you envisage different device models sharing the same driver_name? >> >> >> > Yes. isa-ide, piix3-ide, piix4-ide all provides exactly same ata bus. >> >> But they're different devices! Isn't Open Firmware's "driver name" >> meant to identify a device type unambigously? >> > Not necessary as far as I see from examples. Full FDT contains more > info. In this case all of those different devices present exactly same > HW interface, so from FW point of view they are the same. To make FW > life more easy it is better to have one name for all of them. Okay. It's a name for a sufficiently compatible set of devices, where "sufficient compatibility" is defined from the consumer's point of view. The consumer here is SeaBios, right? To be precise: the specific version of SeaBios we ship together with QEMU, right? Then why are our existing driver names (DevinceInfo member name) not good enough? >> Consider the case of an ISA soundcard providing an IDE channel. Want to >> call it "ata", too? > If it is exactly like interface provided by devices above why FW cares > that this is soundcard? What if firmware cares about soundcards as well?
On Fri, Nov 05, 2010 at 05:24:01PM +0100, Markus Armbruster wrote: > Gleb Natapov <gleb@redhat.com> writes: > > > On Fri, Nov 05, 2010 at 03:14:20PM +0100, Markus Armbruster wrote: > >> Gleb Natapov <gleb@redhat.com> writes: > >> > >> > On Thu, Nov 04, 2010 at 03:58:03PM +0100, Markus Armbruster wrote: > >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> > >> >> > On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: > >> >> >> Gleb Natapov <gleb@redhat.com> writes: > >> >> >> > >> >> >> > Add "deriver_name" to DeviceInfo to use in device path building. In > >> >> >> > >> >> >> Typo "deriver". Same in subject. > >> >> >> > >> >> > Heh. > >> >> > > >> >> >> > contrast to "name" "driver_name" should refer to functionality device > >> >> >> > provides instead of particular device model like "name" does. > >> >> >> > >> >> >> Why is that useful in a device path? > >> >> >> > >> >> > Sometimes it is sometimes it is not. Lets look at path like that: > >> >> > /pci@i0cf8/ethernet@4/ethernet-phy@0 > >> >> > > >> >> > It is important to have pci in the fist path element since it defines > >> >> > what kind of bus addressing is used by next element ethernet@4. > >> >> > >> >> It is for consumers that don't know what's sitting at i0cf8 on the > >> >> system bus. > >> > Yes. Same firmware may support different boards that have same pci > >> > controller but on different addresses. Device name may even contain > >> > manufacturer ID, so firmware will be able to find matching driver and > >> > support more board without recompiling. > >> > >> "pci" tells us it's some kind of PCI host bridge. Why is that enough? > >> Why don't we have to identify the particular host bridge, such as > >> "i440FX-pcihost"? > >> > > As I said below manufacturer ID may be part of device name. It should be > > separated by comma though. Something like "i440FX,pci". > > I'd expect "intel,i440FX". > It is impossible to figure what i440FX is. Anyway as I said many times already device path shouldn't contain full information about all devices in the path but only enough information to find device the path points to. FDT contains full information about device including all resources it uses, full device name, compatible device list an so on. This patch is not about passing FDT to FW, just about creating Open Firmware complaint device path. > Note that comma makes for extremely user-hostile -device usage. Right > now, it doesn't work at all. > > > But for x86 qemu > > emulation this is not needed since all chipsets implement essentially > > the same pci controller. > > Will it stay that way? What about Isaku's q35 work? > AFAIK all PC chipsets implement same PCI config interface accessible through io ports 0cf8-0cff. Otherwise each OS will have to have support for each chipset. > > Other platforms qemu emulates may use more > > elaborate names. Other platforms may want to get full FDT tree from > > qemu anyway. > > > >> >> > 4 is > >> >> > slot number in case of pci bus. On the other hand ethernet part is not > >> >> > important since OS can figure it out by looking in pci config space. > >> >> > >> >> The OS does know what's sitting in slot 4 on the PCI bus. > >> >> > >> > Yes, and? That what I wrote above. "ethernet" part is redundant in case > >> > of PCI bus. A little bit of redundancy will not hurt and required by the > >> > spec. > >> > > >> >> If the OS number doesn't know what's sitting at i0cf8, why is "pci" > >> >> sufficient information? Why don't we have to distinguish between the > >> >> different PCI host bridges? > >> > Manufacturer ID may be put along with pci. Full FDT contains much more > >> > information about each node though. It may even list compatible HW. Here > >> > we are concerned with device paths only. > > Here I said it already :) > > > >> > > >> >> > >> >> >> I'm afraid "driver_name" is too confusing, because we already use > >> >> >> "driver" and "name" for the name of the device model: DeviceInfo member > >> >> >> name, and qemu_device_opts option name "driver". > >> >> > I use "driver_name" here since I am coding to Open Firmware spec now > >> >> > and this element in device path is called driver_name by the spec. > >> >> > >> >> Why is it different from our DeviceInfo member name? > >> >> > >> >> We already have name (e.g. "lsi53c895a") and alias ("lsi"), why do we > >> >> need a third? > >> > I haven't noticed we have alias! What is it used for? I think I can use > >> > it instead. > >> > > >> >> > >> >> Do you envisage different device models sharing the same driver_name? > >> >> > >> > Yes. isa-ide, piix3-ide, piix4-ide all provides exactly same ata bus. > >> > >> But they're different devices! Isn't Open Firmware's "driver name" > >> meant to identify a device type unambigously? > >> > > Not necessary as far as I see from examples. Full FDT contains more > > info. In this case all of those different devices present exactly same > > HW interface, so from FW point of view they are the same. To make FW > > life more easy it is better to have one name for all of them. > > Okay. It's a name for a sufficiently compatible set of devices, where > "sufficient compatibility" is defined from the consumer's point of view. > > The consumer here is SeaBios, right? To be precise: the specific > version of SeaBios we ship together with QEMU, right? Then why are our > existing driver names (DevinceInfo member name) not good enough? > Why should Seabios match against three (or even more) different type of devices to detect ata interface? Why require Seabios changes when this can be avoided (if new device that provide ata is added)? OpenBIOS also supports qemu BTW (this is Open Firmware implementation for pc, you can run and see what kind of device paths it generates). > >> Consider the case of an ISA soundcard providing an IDE channel. Want to > >> call it "ata", too? > > If it is exactly like interface provided by devices above why FW cares > > that this is soundcard? > > What if firmware cares about soundcards as well? Then I expect device hierarchy reflect the fact that one device provides two different devices one of which is ata and another one is soundcard. Or are you suggesting that the same ISA card provides ata and soundcard functionality through the same IO ports? Then don't call it ata. Call is something else like ACME,atablaster. It will have to have special support in firmware anyway. -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Fri, Nov 05, 2010 at 05:24:01PM +0100, Markus Armbruster wrote: >> Gleb Natapov <gleb@redhat.com> writes: >> >> > On Fri, Nov 05, 2010 at 03:14:20PM +0100, Markus Armbruster wrote: >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> > On Thu, Nov 04, 2010 at 03:58:03PM +0100, Markus Armbruster wrote: >> >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> >> >> > On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote: >> >> >> >> Gleb Natapov <gleb@redhat.com> writes: >> >> >> >> >> >> >> >> > Add "deriver_name" to DeviceInfo to use in device path building. In >> >> >> >> >> >> >> >> Typo "deriver". Same in subject. >> >> >> >> >> >> >> > Heh. >> >> >> > >> >> >> >> > contrast to "name" "driver_name" should refer to functionality device >> >> >> >> > provides instead of particular device model like "name" does. >> >> >> >> >> >> >> >> Why is that useful in a device path? >> >> >> >> >> >> >> > Sometimes it is sometimes it is not. Lets look at path like that: >> >> >> > /pci@i0cf8/ethernet@4/ethernet-phy@0 >> >> >> > >> >> >> > It is important to have pci in the fist path element since it defines >> >> >> > what kind of bus addressing is used by next element ethernet@4. >> >> >> >> >> >> It is for consumers that don't know what's sitting at i0cf8 on the >> >> >> system bus. >> >> > Yes. Same firmware may support different boards that have same pci >> >> > controller but on different addresses. Device name may even contain >> >> > manufacturer ID, so firmware will be able to find matching driver and >> >> > support more board without recompiling. >> >> >> >> "pci" tells us it's some kind of PCI host bridge. Why is that enough? >> >> Why don't we have to identify the particular host bridge, such as >> >> "i440FX-pcihost"? >> >> >> > As I said below manufacturer ID may be part of device name. It should be >> > separated by comma though. Something like "i440FX,pci". >> >> I'd expect "intel,i440FX". >> > It is impossible to figure what i440FX is. Anyway as I said many times > already device path shouldn't contain full information about all devices > in the path but only enough information to find device the path points > to. FDT contains full information about device including all resources > it uses, full device name, compatible device list an so on. This patch > is not about passing FDT to FW, just about creating Open Firmware > complaint device path. > >> Note that comma makes for extremely user-hostile -device usage. Right >> now, it doesn't work at all. >> >> > But for x86 qemu >> > emulation this is not needed since all chipsets implement essentially >> > the same pci controller. >> >> Will it stay that way? What about Isaku's q35 work? >> > AFAIK all PC chipsets implement same PCI config interface accessible > through io ports 0cf8-0cff. Otherwise each OS will have to have support > for each chipset. Nice to have some compatibility, for once. >> > Other platforms qemu emulates may use more >> > elaborate names. Other platforms may want to get full FDT tree from >> > qemu anyway. >> > >> >> >> > 4 is >> >> >> > slot number in case of pci bus. On the other hand ethernet part is not >> >> >> > important since OS can figure it out by looking in pci config space. >> >> >> >> >> >> The OS does know what's sitting in slot 4 on the PCI bus. >> >> >> >> >> > Yes, and? That what I wrote above. "ethernet" part is redundant in case >> >> > of PCI bus. A little bit of redundancy will not hurt and required by the >> >> > spec. >> >> > >> >> >> If the OS number doesn't know what's sitting at i0cf8, why is "pci" >> >> >> sufficient information? Why don't we have to distinguish between the >> >> >> different PCI host bridges? >> >> > Manufacturer ID may be put along with pci. Full FDT contains much more >> >> > information about each node though. It may even list compatible HW. Here >> >> > we are concerned with device paths only. >> > Here I said it already :) >> > >> >> > >> >> >> >> >> >> >> I'm afraid "driver_name" is too confusing, because we already use >> >> >> >> "driver" and "name" for the name of the device model: DeviceInfo member >> >> >> >> name, and qemu_device_opts option name "driver". >> >> >> > I use "driver_name" here since I am coding to Open Firmware spec now >> >> >> > and this element in device path is called driver_name by the spec. >> >> >> >> >> >> Why is it different from our DeviceInfo member name? >> >> >> >> >> >> We already have name (e.g. "lsi53c895a") and alias ("lsi"), why do we >> >> >> need a third? >> >> > I haven't noticed we have alias! What is it used for? I think I can use >> >> > it instead. >> >> > >> >> >> >> >> >> Do you envisage different device models sharing the same driver_name? >> >> >> >> >> > Yes. isa-ide, piix3-ide, piix4-ide all provides exactly same ata bus. >> >> >> >> But they're different devices! Isn't Open Firmware's "driver name" >> >> meant to identify a device type unambigously? >> >> >> > Not necessary as far as I see from examples. Full FDT contains more >> > info. In this case all of those different devices present exactly same >> > HW interface, so from FW point of view they are the same. To make FW >> > life more easy it is better to have one name for all of them. >> >> Okay. It's a name for a sufficiently compatible set of devices, where >> "sufficient compatibility" is defined from the consumer's point of view. >> >> The consumer here is SeaBios, right? To be precise: the specific >> version of SeaBios we ship together with QEMU, right? Then why are our >> existing driver names (DevinceInfo member name) not good enough? >> > Why should Seabios match against three (or even more) different type of > devices to detect ata interface? Why require Seabios changes when this > can be avoided (if new device that provide ata is added)? OpenBIOS also > supports qemu BTW (this is Open Firmware implementation for pc, you can > run and see what kind of device paths it generates). I think we've finally cut through the confusion caused by the unfortunate choice of driver_name for this new device attribute. The fact that you choose values of your driver_name in a way that is inspired by the syntactic conventions of IEEE 1275 is not its distinguishing characteristic. The values of existing member name are inspired by that as well. driver_name's distinguishing characteristic is its purpose: communication with SeaBIOS. I'm fine with you choosing its values however it's convenient for that purpose, as long as you give it a name reflecting that purpose. What about fw_name and qdev_fw_name()? Next, I'm worried about overloading of method get_dev_path(). It's being used for a very specific purpose: savevm/loadvm. * It's currently defined only for PCI devices. Your PATCH 7/8 changes its value there, from DOMAIN:BUS:SLOT.FUNCTION to FW_NAME@SLOT. - The old value identifies the qdev. The new value does not (remember, we have a separate qdev per PCI function). Why is this okay? - Is the value saved with the VM? If yes, this is an incompatible change. * You extend it for ISA and System bus (PATCH 5,6/8). How does this affect savevm? [...]
On Sat, Nov 06, 2010 at 10:01:25AM +0100, Markus Armbruster wrote: [skip] > > Why should Seabios match against three (or even more) different type of > > devices to detect ata interface? Why require Seabios changes when this > > can be avoided (if new device that provide ata is added)? OpenBIOS also > > supports qemu BTW (this is Open Firmware implementation for pc, you can > > run and see what kind of device paths it generates). > > I think we've finally cut through the confusion caused by the > unfortunate choice of driver_name for this new device attribute. > > The fact that you choose values of your driver_name in a way that is > inspired by the syntactic conventions of IEEE 1275 is not its > distinguishing characteristic. The values of existing member name are > inspired by that as well. driver_name's distinguishing characteristic > is its purpose: communication with SeaBIOS. > My understanding of this name in IEEE 1275 is that it specifies what driver in FW handles a device. > I'm fine with you choosing its values however it's convenient for that > purpose, as long as you give it a name reflecting that purpose. What > about fw_name and qdev_fw_name()? > I am not attached to the name. Can "alias" be used for that purpose? > > Next, I'm worried about overloading of method get_dev_path(). It's > being used for a very specific purpose: savevm/loadvm. > This part of the patch is not completed yet. I intend to change the code in savevm/loadvm to call qdev_get_dev_path() to get full device path there. > * It's currently defined only for PCI devices. Your PATCH 7/8 changes > its value there, from DOMAIN:BUS:SLOT.FUNCTION to FW_NAME@SLOT. > Old definition is buggy BTW. BUS part is controlled by a guest and may be different from default value at destination. > - The old value identifies the qdev. The new value does not > (remember, we have a separate qdev per PCI function). Why is this > okay? > No no. New value is FW_NAME@SLOT,FUNC. Spec says that if FUNC is zero it can be omitted. > - Is the value saved with the VM? If yes, this is an incompatible > change. Don't understand that remark. > > * You extend it for ISA and System bus (PATCH 5,6/8). How does this > affect savevm? > We should ask savevm experts. As far as I can see it affects id creation. As long as id is unique we should be OK. We may send more info on migration after the patches. -- Gleb.
Gleb Natapov <gleb@redhat.com> writes: > On Sat, Nov 06, 2010 at 10:01:25AM +0100, Markus Armbruster wrote: > [skip] >> > Why should Seabios match against three (or even more) different type of >> > devices to detect ata interface? Why require Seabios changes when this >> > can be avoided (if new device that provide ata is added)? OpenBIOS also >> > supports qemu BTW (this is Open Firmware implementation for pc, you can >> > run and see what kind of device paths it generates). >> >> I think we've finally cut through the confusion caused by the >> unfortunate choice of driver_name for this new device attribute. >> >> The fact that you choose values of your driver_name in a way that is >> inspired by the syntactic conventions of IEEE 1275 is not its >> distinguishing characteristic. The values of existing member name are >> inspired by that as well. driver_name's distinguishing characteristic >> is its purpose: communication with SeaBIOS. >> > My understanding of this name in IEEE 1275 is that it specifies what > driver in FW handles a device. > >> I'm fine with you choosing its values however it's convenient for that >> purpose, as long as you give it a name reflecting that purpose. What >> about fw_name and qdev_fw_name()? >> > I am not attached to the name. Can "alias" be used for that purpose? alias needs to be an unambigous name of the device, just like name. If I understand you correctly, you want to use the same string for different, yet sufficiently compatible devices, so alias won't do. >> Next, I'm worried about overloading of method get_dev_path(). It's >> being used for a very specific purpose: savevm/loadvm. >> > This part of the patch is not completed yet. I intend to change the code > in savevm/loadvm to call qdev_get_dev_path() to get full device path > there. > >> * It's currently defined only for PCI devices. Your PATCH 7/8 changes >> its value there, from DOMAIN:BUS:SLOT.FUNCTION to FW_NAME@SLOT. >> > Old definition is buggy BTW. BUS part is controlled by a guest and may > be different from default value at destination. Yes, it's problematic for anything domain:bus 0:0. Which happens to be the only one that can occur here currently, as far as I know. >> - The old value identifies the qdev. The new value does not >> (remember, we have a separate qdev per PCI function). Why is this >> okay? >> > No no. New value is FW_NAME@SLOT,FUNC. Spec says that if FUNC is zero it > can be omitted. Okay. >> - Is the value saved with the VM? If yes, this is an incompatible >> change. > Don't understand that remark. If the value somehow makes it into the savevm data, then changing values could render the data incompatible: old qemu can't load new data, and vice versa. >> * You extend it for ISA and System bus (PATCH 5,6/8). How does this >> affect savevm? >> > We should ask savevm experts. As far as I can see it affects id > creation. As long as id is unique we should be OK. We may send more info > on migration after the patches. We definitely need review and testing there.
diff --git a/hw/fdc.c b/hw/fdc.c index c159dcb..b4217a3 100644 --- a/hw/fdc.c +++ b/hw/fdc.c @@ -2040,6 +2040,7 @@ static const VMStateDescription vmstate_isa_fdc ={ static ISADeviceInfo isa_fdc_info = { .init = isabus_fdc_init1, .qdev.name = "isa-fdc", + .qdev.driver_name = "fdc", .qdev.size = sizeof(FDCtrlISABus), .qdev.no_user = 1, .qdev.vmsd = &vmstate_isa_fdc, diff --git a/hw/ide/isa.c b/hw/ide/isa.c index 6b57e0d..489cc99 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -98,6 +98,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, static ISADeviceInfo isa_ide_info = { .qdev.name = "isa-ide", + .qdev.driver_name = "ata", .qdev.size = sizeof(ISAIDEState), .init = isa_ide_initfn, .qdev.reset = isa_ide_reset, diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 07483e8..6206201 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -182,6 +182,7 @@ PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) static PCIDeviceInfo piix_ide_info[] = { { .qdev.name = "piix3-ide", + .qdev.driver_name = "ata", .qdev.size = sizeof(PCIIDEState), .qdev.no_user = 1, .init = pci_piix3_ide_initfn, diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 0808760..341548e 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -134,6 +134,7 @@ static int ide_drive_initfn(IDEDevice *dev) static IDEDeviceInfo ide_drive_info = { .qdev.name = "ide-drive", + .qdev.driver_name = "ata-disk", .qdev.size = sizeof(IDEDrive), .init = ide_drive_initfn, .qdev.props = (Property[]) { diff --git a/hw/isa-bus.c b/hw/isa-bus.c index 4e306de..2c42b80 100644 --- a/hw/isa-bus.c +++ b/hw/isa-bus.c @@ -153,6 +153,7 @@ static int isabus_bridge_init(SysBusDevice *dev) static SysBusDeviceInfo isabus_bridge_info = { .init = isabus_bridge_init, .qdev.name = "isabus-bridge", + .qdev.driver_name = "isa", .qdev.size = sizeof(SysBusDevice), .qdev.no_user = 1, }; diff --git a/hw/piix_pci.c b/hw/piix_pci.c index 00060f3..4e5e5df 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -364,6 +364,7 @@ static PCIDeviceInfo i440fx_info[] = { .config_write = i440fx_write_config, },{ .qdev.name = "PIIX3", + .qdev.driver_name = "pci-isa-bridge", .qdev.desc = "ISA bridge", .qdev.size = sizeof(PIIX3State), .qdev.vmsd = &vmstate_piix3, @@ -377,6 +378,7 @@ static PCIDeviceInfo i440fx_info[] = { static SysBusDeviceInfo i440fx_pcihost_info = { .init = i440fx_pcihost_initfn, .qdev.name = "i440FX-pcihost", + .qdev.driver_name = "pci", .qdev.size = sizeof(I440FXState), .qdev.no_user = 1, }; diff --git a/hw/qdev.h b/hw/qdev.h index 579328a..a9a98f8 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -139,6 +139,7 @@ typedef void (*qdev_resetfn)(DeviceState *dev); struct DeviceInfo { const char *name; + const char *driver_name; const char *alias; const char *desc; size_t size; @@ -288,6 +289,11 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props); void qdev_prop_register_global_list(GlobalProperty *props); void qdev_prop_set_globals(DeviceState *dev); +static inline const char *qdev_driver_name(DeviceState *dev) +{ + return dev->info->driver_name ? : dev->info->name; +} + /* This is a nasty hack to allow passing a NULL bus to qdev_create. */ extern struct BusInfo system_bus_info; diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 2d78ca6..b67ded6 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -769,6 +769,7 @@ static int virtio_9p_init_pci(PCIDevice *pci_dev) static PCIDeviceInfo virtio_info[] = { { .qdev.name = "virtio-blk-pci", + .qdev.driver_name = "virtio-blk", .qdev.size = sizeof(VirtIOPCIProxy), .init = virtio_blk_init_pci, .exit = virtio_blk_exit_pci, @@ -782,6 +783,7 @@ static PCIDeviceInfo virtio_info[] = { .qdev.reset = virtio_pci_reset, },{ .qdev.name = "virtio-net-pci", + .qdev.driver_name = "ethernet", .qdev.size = sizeof(VirtIOPCIProxy), .init = virtio_net_init_pci, .exit = virtio_net_exit_pci,
Add "deriver_name" to DeviceInfo to use in device path building. In contrast to "name" "driver_name" should refer to functionality device provides instead of particular device model like "name" does. Signed-off-by: Gleb Natapov <gleb@redhat.com> --- hw/fdc.c | 1 + hw/ide/isa.c | 1 + hw/ide/piix.c | 1 + hw/ide/qdev.c | 1 + hw/isa-bus.c | 1 + hw/piix_pci.c | 2 ++ hw/qdev.h | 6 ++++++ hw/virtio-pci.c | 2 ++ 8 files changed, 15 insertions(+), 0 deletions(-)