[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/14/2016 09:33 PM, Nikunj A Dadhania wrote:
>> 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
>
>
> oh right. But Thomas is right, it is 10 :)
>
> Could not it be as well this?
>
> /vd-cap-len FIELD vd>legacy

Will try this out.

>
>
>
>>
>>>
>>>
>>>> +   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.
>
>
> Ignore my comment, this was not a change from this patch.
>
>
>>
>>>
>>> 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.
>
>
> But you do not? Put a note in the commit log that this is for the future 
> use. Or (better) drop it if you are not really planning on accessing it 
> from SLOF.

I will make a note in the commit log.


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