Re: [Qemu-devel] [v2][RFC][PATCH] virtio: uniform virtio device IDs

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/2/6 20:14, Cornelia Huck wrote:
> On Fri,  6 Feb 2015 13:41:26 +0800
> Tiejun Chen <tiejun.chen@intel.com> wrote:
>
>> Actually we define these device IDs in virtio standard, so
>> we'd better put them into one common place to manage conveniently.
>> Here I also add VIRTIO_ID_RESERVE according to virtio spec.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   hw/9pfs/virtio-9p.h                |  2 --
>>   include/hw/virtio/virtio-balloon.h |  3 ---
>>   include/hw/virtio/virtio-blk.h     |  3 ---
>>   include/hw/virtio/virtio-rng.h     |  3 ---
>>   include/hw/virtio/virtio-scsi.h    |  3 ---
>>   include/hw/virtio/virtio-serial.h  |  3 ---
>>   include/hw/virtio/virtio.h         | 16 ++++++++++++++++
>>   pc-bios/s390-ccw/virtio.h          |  8 +-------
>>   8 files changed, 17 insertions(+), 24 deletions(-)
>>
>
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index f24997d..9ad6bb2 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -23,6 +23,22 @@
>>   #include "hw/virtio/virtio-9p.h"
>>   #endif
>>
>> +/* Refer to Linux's linux/virtio_ids.h */
>
> Why not refer to the virtio spec instead? :) And maybe add in the ids

Actually they're same now but this really is a potential risk.

> that already have been reserved.

So what about this?

@@ -23,6 +23,22 @@
  #include "hw/virtio/virtio-9p.h"
  #endif

+/* Refer to VirtIO Spec 1.0. */
+
+#define VIRTIO_ID_RESERVED  0           /* reserved (invalid)*/
+#define VIRTIO_ID_NET       1           /* network card */
+#define VIRTIO_ID_BLOCK     2           /* block device */
+#define VIRTIO_ID_CONSOLE   3           /* console */
+#define VIRTIO_ID_RNG       4           /* entropy source */
+#define VIRTIO_ID_BALLOON   5           /* memory ballooning */
+#define VIRTIO_ID_IOMEMORY  6           /* ioMemory */
+#define VIRTIO_ID_RPMSG     7           /* rpmsg */
+#define VIRTIO_ID_SCSI      8           /* SCSI host */
+#define VIRTIO_ID_9P        9           /* 9P transport */
+#define VIRTIO_ID_MAC80211_WALN 10      /* mac80211 wlan */
+#define VIRTIO_ID_RPROC_SERIAL  11      /* rproc seria */
+#define VIRTIO_ID_CAIF      12          /* virtio CAIF */
+
  /* from Linux's linux/virtio_config.h */

  /* Status byte for guest to report progress, and synchronize features. */


>
>> +
>> +enum virtio_dev_type {
>> +    VIRTIO_ID_RESERVED  = 0, /* invalid virtio device */
>> +    VIRTIO_ID_NET = 1, /* virtio net */
>> +    VIRTIO_ID_BLOCK	= 2, /* virtio block */
>> +    VIRTIO_ID_CONSOLE = 3, /* virtio console */
>> +    VIRTIO_ID_RNG = 4, /* virtio rng */
>> +    VIRTIO_ID_BALLOON = 5, /* virtio balloon */
>
> /* virtio balloon (legacy) */
>
>> +    VIRTIO_ID_RPMSG	= 7, /* virtio remote processor messaging */
>> +    VIRTIO_ID_SCSI = 8, /* virtio scsi */
>> +    VIRTIO_ID_9P = 9, /* 9p virtio console */
>> +    VIRTIO_ID_RPROC_SERIAL = 11, /* virtio remoteproc serial link */
>> +    VIRTIO_ID_CAIF = 12, /* Virtio caif */
>> +};
>> +
>>   /* from Linux's linux/virtio_config.h */
>>
>>   /* Status byte for guest to report progress, and synchronize features. */
>> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
>> index c23466b..2eabcb4 100644
>> --- a/pc-bios/s390-ccw/virtio.h
>> +++ b/pc-bios/s390-ccw/virtio.h
>> @@ -11,6 +11,7 @@
>>   #ifndef VIRTIO_H
>>   #define VIRTIO_H
>>
>> +#include "hw/virtio/virtio.h"
>
> This won't work, the bios can't use the common headers.

Thanks for your caught.

>
>>   #include "s390-ccw.h"
>>
>>   /* Status byte for guest to report progress, and synchronize features. */
>> @@ -23,13 +24,6 @@
>>   /* We've given up on this device. */
>>   #define VIRTIO_CONFIG_S_FAILED          0x80
>>
>> -enum virtio_dev_type {
>> -    VIRTIO_ID_NET = 1,
>> -    VIRTIO_ID_BLOCK = 2,
>> -    VIRTIO_ID_CONSOLE = 3,
>> -    VIRTIO_ID_BALLOON = 5,
>> -};
>
> Even though this one is incomplete; but we don't need anything but the
> block id anyway.
>
>> -
>>   struct virtio_dev_header {
>>       enum virtio_dev_type type : 8;
>>       u8  num_vq;
>
>

I will remove all s390 stuff in this patch.

Thanks
Tiejun