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

[Markus Armbruster]

Gleb Natapov <address@hidden> 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 <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?

> 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"?