[SLOF] [PATCH v1 06/27] virtio: add virtio 1.0 related struct and defines

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik at ozlabs.ru> writes:

> On 01/13/2016 10:16 PM, Nikunj A Dadhania wrote:
>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>> ---
>>   board-qemu/slof/virtio.fs | 14 ++++++++++++++
>>   lib/libvirtio/virtio.h    | 33 ++++++++++++++++++++++++++++++---
>>   2 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/board-qemu/slof/virtio.fs b/board-qemu/slof/virtio.fs
>> index 818c132..29d0a19 100644
>> --- a/board-qemu/slof/virtio.fs
>> +++ b/board-qemu/slof/virtio.fs
>> @@ -14,8 +14,22 @@
>>   STRUCT
>>      /n FIELD vd>base
>>      /l FIELD vd>type
>> +   /l FIELD vd>is-modern
>> +   16 FIELD vd>legacy
>
> Why not /w?

16 bytes field

>
>
>> +   16 FIELD vd>common
>> +   16 FIELD vd>notify
>> +   16 FIELD vd>isr
>> +   16 FIELD vd>device
>> +   16 FIELD vd>pci
>>   CONSTANT /vd-len
>>
>> +STRUCT
>> +   /n FIELD vd>cap>address
>> +   /c FIELD vd>cap>bar
>> +   /c FIELD vd>cap>is_io
>> +   /c FIELD vd>cap>cap_id
>> +   5  FIELD vd>cap>pad
>> +CONSTANT /vd-cap-len
>>
>>   \ Initialize virtiodev structure for the current node
>>   : virtio-setup-vd  ( vdstruct -- )
>> diff --git a/lib/libvirtio/virtio.h b/lib/libvirtio/virtio.h
>> index 8d3f376..56e044b 100644
>> --- a/lib/libvirtio/virtio.h
>> +++ b/lib/libvirtio/virtio.h
>> @@ -19,8 +19,17 @@
>>   #define VIRTIO_STAT_ACKNOWLEDGE		1
>>   #define VIRTIO_STAT_DRIVER		2
>>   #define VIRTIO_STAT_DRIVER_OK		4
>> +#define VIRTIO_STAT_FEATURES_OK		8
>> +#define VIRTIO_STAT_NEEDS_RESET		64
>>   #define VIRTIO_STAT_FAILED		128
>>
>> +#define BIT(x) (1UL << x)
>
> Put x in braces.

Sure

>
>
>> +
>> +/* VIRTIO 1.0 Device independent feature bits */
>> +#define VIRTIO_F_RING_INDIRECT_DESC	BIT(28)
>> +#define VIRTIO_F_RING_EVENT_IDX		BIT(29)
>> +#define VIRTIO_F_VERSION_1		BIT(32)
>> +
>>   #define VIRTIO_TIMEOUT		        5000 /* 5 sec timeout */
>>
>>   /* Definitions for vring_desc.flags */
>> @@ -34,7 +43,7 @@ struct vring_desc {
>>   	uint32_t len;		/* Length */
>>   	uint16_t flags;		/* The flags as indicated above */
>>   	uint16_t next;		/* Next field if flags & NEXT */
>> -};
>> +};
>
> Unrelated whitespace change?

As I was at this file I changed, I can submit as a separate patch if
needed.

>
>>
>>   /* Definitions for vring_avail.flags */
>>   #define VRING_AVAIL_F_NO_INTERRUPT	1
>> @@ -44,7 +53,7 @@ struct vring_avail {
>>   	uint16_t flags;
>>   	uint16_t idx;
>>   	uint16_t ring[];
>> -};
>> +};
>
>
> Unrelated whitespace change?

ditto

>
>
>>
>>
>>   /* Definitions for vring_used.flags */
>> @@ -62,10 +71,28 @@ struct vring_used {
>>   };
>>
>>   #define VIRTIO_TYPE_PCI 0	/* For virtio-pci interface */
>> +
>> +/* Structure shared with SLOF and is 16bytes */
>> +struct virtio_cap {
>> +	uint64_t addr;
>> +	uint8_t bar;
>> +	uint8_t is_io;
>> +	uint8_t cap_id;
>> +	uint8_t pad[5];
>> +} __attribute__ ((packed));
>> +
>>   struct virtio_device {
>>   	void *base;		/* base address */
>>   	int type;		/* VIRTIO_TYPE_PCI or VIRTIO_TYPE_VIO */
>
> Why "int", not uintxx_t?

legacy, i can change, but will be unrelated.

>
> In general, if it is a binary interface, it is better to use uintxx_t. 
> Unless it is widely assumed in SLOF what long==/l, short==/w, void*==/n, etc...
>
>
>
>> -};
>> +	int is_modern;     /* Indicates whether to use virtio 1.0 */
>> +	struct virtio_cap legacy;
>> +	struct virtio_cap common;
>> +	struct virtio_cap notify;
>> +	struct virtio_cap isr;
>> +	struct virtio_cap device;
>> +	struct virtio_cap pci;
>> +	uint32_t notify_off_mul;
>> +}__attribute__ ((packed));
>
>
> Why do you need "packed" now? I assume you are going to use it from SLOF 
> but why? :)

Initially, when I was writing this, I thought of keeping this option
open. As I was not sure whether i can completely do this in C. Currently,
with the below structure, I can access everything in slof as well if needed.

  STRUCT
     /n FIELD vd>base
     /l FIELD vd>type
     /l FIELD vd>is-modern
     16 FIELD vd>legacy
     16 FIELD vd>common
     16 FIELD vd>notify
     16 FIELD vd>isr
     16 FIELD vd>device
     16 FIELD vd>pci
  CONSTANT /vd-len

Regards
Nikunj