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

[Alexey Kardashevskiy]

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 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.


> +
> +/* 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?

>
>   /* 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?


>
>
>   /* 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?

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? :)


>
>   struct vqs {
>   	uint64_t id;	/* Queue ID */
>


-- 
Alexey