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

[Alexey Kardashevskiy]

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



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


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