Re: [Qemu-devel] [PATCHv2 1/8] Introduce deriver_name field to DeviceInfo structure.

[Gleb Natapov]

On Thu, Nov 04, 2010 at 10:20:18AM +0100, Markus Armbruster wrote:
> Gleb Natapov <address@hidden> 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:
/address@hidden/address@hidden/address@hidden

It is important to have pci in the fist path element since it defines
what kind of bus addressing is used by next element address@hidden 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 <address@hidden>
> > ---
> >  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.