thr3ads.net - Linux Virtualization - [PATCH net-next v3 01/13] virtio: add packed ring types and macros [Nov 2018]

If this information is useful, please help other people find it:
Share via:

Tiwei Bie

2018-Nov-30 16:24 UTC

[PATCH net-next v3 01/13] virtio: add packed ring types and macros

On Fri, Nov 30, 2018 at 10:53:07AM -0500, Michael S. Tsirkin
wrote:> On Fri, Nov 30, 2018 at 11:37:37PM +0800, Tiwei Bie wrote:
> > On Fri, Nov 30, 2018 at 08:52:42AM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Nov 30, 2018 at 02:01:06PM +0100, Maxime Coquelin wrote:
> > > > On 11/30/18 1:47 PM, Michael S. Tsirkin wrote:
> > > > > On Fri, Nov 30, 2018 at 05:53:40PM +0800, Tiwei Bie
wrote:
> > > > > > On Fri, Nov 30, 2018 at 04:10:55PM +0800, Jason
Wang wrote:
> > > > > > > 
> > > > > > > On 2018/11/21 ??6:03, Tiwei Bie wrote:
> > > > > > > > Add types and macros for packed ring.
> > > > > > > > 
> > > > > > > > Signed-off-by: Tiwei Bie <tiwei.bie
at intel.com>
> > > > > > > > ---
> > > > > > > >    include/uapi/linux/virtio_config.h | 
3 +++
> > > > > > > >    include/uapi/linux/virtio_ring.h   |
52 ++++++++++++++++++++++++++++++++++++++
> > > > > > > >    2 files changed, 55 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git
a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > > index 449132c76b1c..1196e1c1d4f6 100644
> > > > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > > > @@ -75,6 +75,9 @@
> > > > > > > >     */
> > > > > > > >    #define VIRTIO_F_IOMMU_PLATFORM		33
> > > > > > > > +/* This feature indicates support for
the packed virtqueue layout. */
> > > > > > > > +#define VIRTIO_F_RING_PACKED		34
> > > > > > > > +
> > > > > > > >    /*
> > > > > > > >     * Does the device support Single
Root I/O Virtualization?
> > > > > > > >     */
> > > > > > > > diff --git
a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> > > > > > > > index 6d5d5faa989b..2414f8af26b3 100644
> > > > > > > > --- a/include/uapi/linux/virtio_ring.h
> > > > > > > > +++ b/include/uapi/linux/virtio_ring.h
> > > > > > > > @@ -44,6 +44,13 @@
> > > > > > > >    /* This means the buffer contains a
list of buffer descriptors. */
> > > > > > > >    #define VRING_DESC_F_INDIRECT	4
> > > > > > > > +/*
> > > > > > > > + * Mark a descriptor as available or
used in packed ring.
> > > > > > > > + * Notice: they are defined as shifts
instead of shifted values.
> > > > > > > 
> > > > > > > 
> > > > > > > This looks inconsistent to previous flags,
any reason for using shifts?
> > > > > > 
> > > > > > Yeah, it was suggested to use shifts, as _F_
should be a bit
> > > > > > number, not a shifted value:
> > > > > > 
> > > > > > https://patchwork.ozlabs.org/patch/942296/#1989390
> > > > > 
> > > > > But let's add a _SPLIT_ variant that uses shifts
consistently.
> > > > 
> > > > Maybe we could avoid adding SPLIT and PACKED, but define as
follow:
> > > > 
> > > > #define VRING_DESC_F_INDIRECT_SHIFT 2
> > > > #define VRING_DESC_F_INDIRECT (1ull << 
VRING_DESC_F_INDIRECT_SHIFT)
> > > > 
> > > > #define VRING_DESC_F_AVAIL_SHIFT 7
> > > > #define VRING_DESC_F_AVAIL (1ull <<
VRING_DESC_F_AVAIL_SHIFT)
> > > > 
> > > > I think it would be better for consistency.
> > > 
> > > I don't think that will help. the problem is that
> > > most of the existing virtio code consistently uses _F_ as shifts.
> > > So we just need to do something about these 5 being inconsistent:
> > > 
> > > include/uapi/linux/virtio_ring.h:#define VRING_DESC_F_NEXT      1
> > > include/uapi/linux/virtio_ring.h:#define VRING_DESC_F_WRITE     2
> > > include/uapi/linux/virtio_ring.h:#define VRING_DESC_F_INDIRECT  4
> > > include/uapi/linux/virtio_ring.h:#define VRING_USED_F_NO_NOTIFY 1
> > > include/uapi/linux/virtio_ring.h:#define
VRING_AVAIL_F_NO_INTERRUPT     1
> > > 
> > > I do not want all of virtio to become verbose with _SHIFT, ergo
> > > we need to change the above 5 to have names which are with _F_
and
> > > have the bit number.
> > 
> > How about something like this:
> > 
> > #define VRING_COMM_DESC_F_NEXT			0
> > #define VRING_COMM_DESC_F_WRITE			1
> > #define VRING_COMM_DESC_F_INDIRECT		2
> > 
> > #define VRING_SPLIT_USED_F_NO_NOTIFY		0
> > #define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
> > 
> > or
> > 
> > #define VRING_SPLIT_DESC_F_NEXT			0
> > #define VRING_SPLIT_DESC_F_WRITE		1
> > #define VRING_SPLIT_DESC_F_INDIRECT		2
> > 
> > #define VRING_SPLIT_USED_F_NO_NOTIFY		0
> > #define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
> > 
> > #define VRING_PACKED_DESC_F_NEXT		0
> > #define VRING_PACKED_DESC_F_WRITE		1
> > #define VRING_PACKED_DESC_F_INDIRECT		2
> 
> As we aren't sharing code I think I prefer the second form.
> Maybe add VRING_NO_LEGACY so people can audit their code
> for assumptions?
Maybe it's better to name it as VIRTIO_RING_NO_LEGACY
which is more consistent with the names in other files.
> 
> We also want to guard layout definitions at the end of that file
> I think.
Do you mean guard the definitions of `struct vring` and
`struct vring_packed` with _NO_LEGACY?
> 
> > > 
> > > 
> > > 
> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > + */
> > > > > > > > +#define VRING_PACKED_DESC_F_AVAIL	7
> > > > > > > > +#define VRING_PACKED_DESC_F_USED	15
> > > > > > > > +
> > > > > > > >    /* The Host uses this in
used->flags to advise the Guest: don't kick me when
> > > > > > > >     * you add a buffer.  It's
unreliable, so it's simply an optimization.  Guest
> > > > > > > >     * will still kick if it's out of
buffers. */
> > > > > > > > @@ -53,6 +60,23 @@
> > > > > > > >     * optimization.  */
> > > > > > > >    #define VRING_AVAIL_F_NO_INTERRUPT	1
> > > > > > > > +/* Enable events in packed ring. */
> > > > > > > > +#define VRING_PACKED_EVENT_FLAG_ENABLE
0x0
> > > > > > > > +/* Disable events in packed ring. */
> > > > > > > > +#define VRING_PACKED_EVENT_FLAG_DISABLE
0x1
> > > > > > > > +/*
> > > > > > > > + * Enable events for a specific
descriptor in packed ring.
> > > > > > > > + * (as specified by Descriptor Ring
Change Event Offset/Wrap Counter).
> > > > > > > > + * Only valid if
VIRTIO_RING_F_EVENT_IDX has been negotiated.
> > > > > > > > + */
> > > > > > > > +#define VRING_PACKED_EVENT_FLAG_DESC
0x2
> > > > > > > 
> > > > > > > 
> > > > > > > Any reason for using _FLAG_ instead of _F_?
> > > > > > 
> > > > > > Yeah, it was suggested to not use _F_, as these
are values,
> > > > > > should not have _F_:
> > > > > > 
> > > > > > https://patchwork.ozlabs.org/patch/942296/#1989390
> > > > > > 
> > > > > > Regards,
> > > > > > Tiwei
> > > > > > 
> > > > > > > 
> > > > > > > Thanks
> > > > > > > 
> > > > > > > 
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Wrap counter bit shift in event
suppression structure
> > > > > > > > + * of packed ring.
> > > > > > > > + */
> > > > > > > > +#define VRING_PACKED_EVENT_F_WRAP_CTR
15
> > > > > > > > +
> > > > > > > >    /* We support indirect buffer
descriptors */
> > > > > > > >    #define VIRTIO_RING_F_INDIRECT_DESC
28
> > > > > > > > @@ -171,4 +195,32 @@ static inline int
vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
> > > > > > > >    	return (__u16)(new_idx - event_idx -
1) < (__u16)(new_idx - old);
> > > > > > > >    }
> > > > > > > > +struct vring_packed_desc_event {
> > > > > > > > +	/* Descriptor Ring Change Event
Offset/Wrap Counter. */
> > > > > > > > +	__le16 off_wrap;
> > > > > > > > +	/* Descriptor Ring Change Event Flags.
*/
> > > > > > > > +	__le16 flags;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +struct vring_packed_desc {
> > > > > > > > +	/* Buffer Address. */
> > > > > > > > +	__le64 addr;
> > > > > > > > +	/* Buffer Length. */
> > > > > > > > +	__le32 len;
> > > > > > > > +	/* Buffer ID. */
> > > > > > > > +	__le16 id;
> > > > > > > > +	/* The flags depending on descriptor
type. */
> > > > > > > > +	__le16 flags;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +struct vring_packed {
> > > > > > > > +	unsigned int num;
> > > > > > > > +
> > > > > > > > +	struct vring_packed_desc *desc;
> > > > > > > > +
> > > > > > > > +	struct vring_packed_desc_event
*driver;
> > > > > > > > +
> > > > > > > > +	struct vring_packed_desc_event
*device;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >    #endif /* _UAPI_LINUX_VIRTIO_RING_H
*/

Michael S. Tsirkin

2018-Nov-30 16:46 UTC

head link

[PATCH net-next v3 01/13] virtio: add packed ring types and macros

On Sat, Dec 01, 2018 at 12:24:16AM +0800, Tiwei Bie
wrote:> On Fri, Nov 30, 2018 at 10:53:07AM -0500, Michael S. Tsirkin wrote:
> > On Fri, Nov 30, 2018 at 11:37:37PM +0800, Tiwei Bie wrote:
> > > On Fri, Nov 30, 2018 at 08:52:42AM -0500, Michael S. Tsirkin
wrote:
> > > > On Fri, Nov 30, 2018 at 02:01:06PM +0100, Maxime Coquelin
wrote:
> > > > > On 11/30/18 1:47 PM, Michael S. Tsirkin wrote:
> > > > > > On Fri, Nov 30, 2018 at 05:53:40PM +0800, Tiwei
Bie wrote:
> > > > > > > On Fri, Nov 30, 2018 at 04:10:55PM +0800,
Jason Wang wrote:
> > > > > > > > 
> > > > > > > > On 2018/11/21 ??6:03, Tiwei Bie wrote:
> > > > > > > > > Add types and macros for packed
ring.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Tiwei Bie
<tiwei.bie at intel.com>
> > > > > > > > > ---
> > > > > > > > >   
include/uapi/linux/virtio_config.h |  3 +++
> > > > > > > > >    include/uapi/linux/virtio_ring.h
| 52 ++++++++++++++++++++++++++++++++++++++
> > > > > > > > >    2 files changed, 55
insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git
a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > > > index 449132c76b1c..1196e1c1d4f6
100644
> > > > > > > > > ---
a/include/uapi/linux/virtio_config.h
> > > > > > > > > +++
b/include/uapi/linux/virtio_config.h
> > > > > > > > > @@ -75,6 +75,9 @@
> > > > > > > > >     */
> > > > > > > > >    #define VIRTIO_F_IOMMU_PLATFORM	
33
> > > > > > > > > +/* This feature indicates support
for the packed virtqueue layout. */
> > > > > > > > > +#define VIRTIO_F_RING_PACKED		34
> > > > > > > > > +
> > > > > > > > >    /*
> > > > > > > > >     * Does the device support
Single Root I/O Virtualization?
> > > > > > > > >     */
> > > > > > > > > diff --git
a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> > > > > > > > > index 6d5d5faa989b..2414f8af26b3
100644
> > > > > > > > > ---
a/include/uapi/linux/virtio_ring.h
> > > > > > > > > +++
b/include/uapi/linux/virtio_ring.h
> > > > > > > > > @@ -44,6 +44,13 @@
> > > > > > > > >    /* This means the buffer
contains a list of buffer descriptors. */
> > > > > > > > >    #define VRING_DESC_F_INDIRECT	4
> > > > > > > > > +/*
> > > > > > > > > + * Mark a descriptor as available
or used in packed ring.
> > > > > > > > > + * Notice: they are defined as
shifts instead of shifted values.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > This looks inconsistent to previous
flags, any reason for using shifts?
> > > > > > > 
> > > > > > > Yeah, it was suggested to use shifts, as _F_
should be a bit
> > > > > > > number, not a shifted value:
> > > > > > > 
> > > > > > >
https://patchwork.ozlabs.org/patch/942296/#1989390
> > > > > > 
> > > > > > But let's add a _SPLIT_ variant that uses
shifts consistently.
> > > > > 
> > > > > Maybe we could avoid adding SPLIT and PACKED, but
define as follow:
> > > > > 
> > > > > #define VRING_DESC_F_INDIRECT_SHIFT 2
> > > > > #define VRING_DESC_F_INDIRECT (1ull << 
VRING_DESC_F_INDIRECT_SHIFT)
> > > > > 
> > > > > #define VRING_DESC_F_AVAIL_SHIFT 7
> > > > > #define VRING_DESC_F_AVAIL (1ull <<
VRING_DESC_F_AVAIL_SHIFT)
> > > > > 
> > > > > I think it would be better for consistency.
> > > > 
> > > > I don't think that will help. the problem is that
> > > > most of the existing virtio code consistently uses _F_ as
shifts.
> > > > So we just need to do something about these 5 being
inconsistent:
> > > > 
> > > > include/uapi/linux/virtio_ring.h:#define VRING_DESC_F_NEXT  
1
> > > > include/uapi/linux/virtio_ring.h:#define VRING_DESC_F_WRITE 
2
> > > > include/uapi/linux/virtio_ring.h:#define
VRING_DESC_F_INDIRECT  4
> > > > include/uapi/linux/virtio_ring.h:#define
VRING_USED_F_NO_NOTIFY 1
> > > > include/uapi/linux/virtio_ring.h:#define
VRING_AVAIL_F_NO_INTERRUPT     1
> > > > 
> > > > I do not want all of virtio to become verbose with _SHIFT,
ergo
> > > > we need to change the above 5 to have names which are with
_F_ and
> > > > have the bit number.
> > > 
> > > How about something like this:
> > > 
> > > #define VRING_COMM_DESC_F_NEXT			0
> > > #define VRING_COMM_DESC_F_WRITE			1
> > > #define VRING_COMM_DESC_F_INDIRECT		2
> > > 
> > > #define VRING_SPLIT_USED_F_NO_NOTIFY		0
> > > #define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
> > > 
> > > or
> > > 
> > > #define VRING_SPLIT_DESC_F_NEXT			0
> > > #define VRING_SPLIT_DESC_F_WRITE		1
> > > #define VRING_SPLIT_DESC_F_INDIRECT		2
> > > 
> > > #define VRING_SPLIT_USED_F_NO_NOTIFY		0
> > > #define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
> > > 
> > > #define VRING_PACKED_DESC_F_NEXT		0
> > > #define VRING_PACKED_DESC_F_WRITE		1
> > > #define VRING_PACKED_DESC_F_INDIRECT		2
> > 
> > As we aren't sharing code I think I prefer the second form.
> > Maybe add VRING_NO_LEGACY so people can audit their code
> > for assumptions?
> 
> Maybe it's better to name it as VIRTIO_RING_NO_LEGACY
> which is more consistent with the names in other files.
OK
> > 
> > We also want to guard layout definitions at the end of that file
> > I think.
> 
> Do you mean guard the definitions of `struct vring` and
> `struct vring_packed` with _NO_LEGACY?
I really mean vring_size/vring_init/vring_used_event/vring_avail_event
these are pre- virtio 1 and should be disabled when
building with no legacy.

But yes I would say let's make sure people can set a flag and
find all dependencies of the split layout in their code easily.
> > 
> > > > 
> > > > 
> > > > 
> > > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > + */
> > > > > > > > > +#define VRING_PACKED_DESC_F_AVAIL
7
> > > > > > > > > +#define VRING_PACKED_DESC_F_USED
15
> > > > > > > > > +
> > > > > > > > >    /* The Host uses this in
used->flags to advise the Guest: don't kick me when
> > > > > > > > >     * you add a buffer.  It's
unreliable, so it's simply an optimization.  Guest
> > > > > > > > >     * will still kick if it's
out of buffers. */
> > > > > > > > > @@ -53,6 +60,23 @@
> > > > > > > > >     * optimization.  */
> > > > > > > > >    #define
VRING_AVAIL_F_NO_INTERRUPT	1
> > > > > > > > > +/* Enable events in packed ring.
*/
> > > > > > > > > +#define
VRING_PACKED_EVENT_FLAG_ENABLE	0x0
> > > > > > > > > +/* Disable events in packed ring.
*/
> > > > > > > > > +#define
VRING_PACKED_EVENT_FLAG_DISABLE	0x1
> > > > > > > > > +/*
> > > > > > > > > + * Enable events for a specific
descriptor in packed ring.
> > > > > > > > > + * (as specified by Descriptor
Ring Change Event Offset/Wrap Counter).
> > > > > > > > > + * Only valid if
VIRTIO_RING_F_EVENT_IDX has been negotiated.
> > > > > > > > > + */
> > > > > > > > > +#define
VRING_PACKED_EVENT_FLAG_DESC	0x2
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Any reason for using _FLAG_ instead of
_F_?
> > > > > > > 
> > > > > > > Yeah, it was suggested to not use _F_, as
these are values,
> > > > > > > should not have _F_:
> > > > > > > 
> > > > > > >
https://patchwork.ozlabs.org/patch/942296/#1989390
> > > > > > > 
> > > > > > > Regards,
> > > > > > > Tiwei
> > > > > > > 
> > > > > > > > 
> > > > > > > > Thanks
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * Wrap counter bit shift in event
suppression structure
> > > > > > > > > + * of packed ring.
> > > > > > > > > + */
> > > > > > > > > +#define
VRING_PACKED_EVENT_F_WRAP_CTR	15
> > > > > > > > > +
> > > > > > > > >    /* We support indirect buffer
descriptors */
> > > > > > > > >    #define
VIRTIO_RING_F_INDIRECT_DESC	28
> > > > > > > > > @@ -171,4 +195,32 @@ static inline
int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
> > > > > > > > >    	return (__u16)(new_idx -
event_idx - 1) < (__u16)(new_idx - old);
> > > > > > > > >    }
> > > > > > > > > +struct vring_packed_desc_event {
> > > > > > > > > +	/* Descriptor Ring Change Event
Offset/Wrap Counter. */
> > > > > > > > > +	__le16 off_wrap;
> > > > > > > > > +	/* Descriptor Ring Change Event
Flags. */
> > > > > > > > > +	__le16 flags;
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +struct vring_packed_desc {
> > > > > > > > > +	/* Buffer Address. */
> > > > > > > > > +	__le64 addr;
> > > > > > > > > +	/* Buffer Length. */
> > > > > > > > > +	__le32 len;
> > > > > > > > > +	/* Buffer ID. */
> > > > > > > > > +	__le16 id;
> > > > > > > > > +	/* The flags depending on
descriptor type. */
> > > > > > > > > +	__le16 flags;
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +struct vring_packed {
> > > > > > > > > +	unsigned int num;
> > > > > > > > > +
> > > > > > > > > +	struct vring_packed_desc *desc;
> > > > > > > > > +
> > > > > > > > > +	struct vring_packed_desc_event
*driver;
> > > > > > > > > +
> > > > > > > > > +	struct vring_packed_desc_event
*device;
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > >    #endif /*
_UAPI_LINUX_VIRTIO_RING_H */

Tiwei Bie

2018-Dec-01 02:03 UTC

head link

[PATCH net-next v3 01/13] virtio: add packed ring types and macros

On Fri, Nov 30, 2018 at 11:46:57AM -0500, Michael S. Tsirkin
wrote:> On Sat, Dec 01, 2018 at 12:24:16AM +0800, Tiwei Bie wrote:
> > On Fri, Nov 30, 2018 at 10:53:07AM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Nov 30, 2018 at 11:37:37PM +0800, Tiwei Bie wrote:
> > > > On Fri, Nov 30, 2018 at 08:52:42AM -0500, Michael S. Tsirkin
wrote:
> > > > > On Fri, Nov 30, 2018 at 02:01:06PM +0100, Maxime
Coquelin wrote:
> > > > > > On 11/30/18 1:47 PM, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Nov 30, 2018 at 05:53:40PM +0800,
Tiwei Bie wrote:
> > > > > > > > On Fri, Nov 30, 2018 at 04:10:55PM
+0800, Jason Wang wrote:
> > > > > > > > > 
> > > > > > > > > On 2018/11/21 ??6:03, Tiwei Bie
wrote:
> > > > > > > > > > Add types and macros for
packed ring.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Tiwei Bie
<tiwei.bie at intel.com>
> > > > > > > > > > ---
> > > > > > > > > >   
include/uapi/linux/virtio_config.h |  3 +++
> > > > > > > > > >   
include/uapi/linux/virtio_ring.h   | 52 ++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >    2 files changed, 55
insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git
a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > > > > index
449132c76b1c..1196e1c1d4f6 100644
> > > > > > > > > > ---
a/include/uapi/linux/virtio_config.h
> > > > > > > > > > +++
b/include/uapi/linux/virtio_config.h
> > > > > > > > > > @@ -75,6 +75,9 @@
> > > > > > > > > >     */
> > > > > > > > > >    #define
VIRTIO_F_IOMMU_PLATFORM		33
> > > > > > > > > > +/* This feature indicates
support for the packed virtqueue layout. */
> > > > > > > > > > +#define VIRTIO_F_RING_PACKED	
34
> > > > > > > > > > +
> > > > > > > > > >    /*
> > > > > > > > > >     * Does the device support
Single Root I/O Virtualization?
> > > > > > > > > >     */
> > > > > > > > > > diff --git
a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> > > > > > > > > > index
6d5d5faa989b..2414f8af26b3 100644
> > > > > > > > > > ---
a/include/uapi/linux/virtio_ring.h
> > > > > > > > > > +++
b/include/uapi/linux/virtio_ring.h
> > > > > > > > > > @@ -44,6 +44,13 @@
> > > > > > > > > >    /* This means the buffer
contains a list of buffer descriptors. */
> > > > > > > > > >    #define
VRING_DESC_F_INDIRECT	4
> > > > > > > > > > +/*
> > > > > > > > > > + * Mark a descriptor as
available or used in packed ring.
> > > > > > > > > > + * Notice: they are defined
as shifts instead of shifted values.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This looks inconsistent to previous
flags, any reason for using shifts?
> > > > > > > > 
> > > > > > > > Yeah, it was suggested to use shifts, as
_F_ should be a bit
> > > > > > > > number, not a shifted value:
> > > > > > > > 
> > > > > > > >
https://patchwork.ozlabs.org/patch/942296/#1989390
> > > > > > > 
> > > > > > > But let's add a _SPLIT_ variant that uses
shifts consistently.
> > > > > > 
> > > > > > Maybe we could avoid adding SPLIT and PACKED, but
define as follow:
> > > > > > 
> > > > > > #define VRING_DESC_F_INDIRECT_SHIFT 2
> > > > > > #define VRING_DESC_F_INDIRECT (1ull << 
VRING_DESC_F_INDIRECT_SHIFT)
> > > > > > 
> > > > > > #define VRING_DESC_F_AVAIL_SHIFT 7
> > > > > > #define VRING_DESC_F_AVAIL (1ull <<
VRING_DESC_F_AVAIL_SHIFT)
> > > > > > 
> > > > > > I think it would be better for consistency.
> > > > > 
> > > > > I don't think that will help. the problem is that
> > > > > most of the existing virtio code consistently uses _F_
as shifts.
> > > > > So we just need to do something about these 5 being
inconsistent:
> > > > > 
> > > > > include/uapi/linux/virtio_ring.h:#define
VRING_DESC_F_NEXT      1
> > > > > include/uapi/linux/virtio_ring.h:#define
VRING_DESC_F_WRITE     2
> > > > > include/uapi/linux/virtio_ring.h:#define
VRING_DESC_F_INDIRECT  4
> > > > > include/uapi/linux/virtio_ring.h:#define
VRING_USED_F_NO_NOTIFY 1
> > > > > include/uapi/linux/virtio_ring.h:#define
VRING_AVAIL_F_NO_INTERRUPT     1
> > > > > 
> > > > > I do not want all of virtio to become verbose with
_SHIFT, ergo
> > > > > we need to change the above 5 to have names which are
with _F_ and
> > > > > have the bit number.
> > > > 
> > > > How about something like this:
> > > > 
> > > > #define VRING_COMM_DESC_F_NEXT			0
> > > > #define VRING_COMM_DESC_F_WRITE			1
> > > > #define VRING_COMM_DESC_F_INDIRECT		2
> > > > 
> > > > #define VRING_SPLIT_USED_F_NO_NOTIFY		0
> > > > #define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
> > > > 
> > > > or
> > > > 
> > > > #define VRING_SPLIT_DESC_F_NEXT			0
> > > > #define VRING_SPLIT_DESC_F_WRITE		1
> > > > #define VRING_SPLIT_DESC_F_INDIRECT		2
> > > > 
> > > > #define VRING_SPLIT_USED_F_NO_NOTIFY		0
> > > > #define VRING_SPLIT_AVAIL_F_NO_INTERRUPT	0
> > > > 
> > > > #define VRING_PACKED_DESC_F_NEXT		0
> > > > #define VRING_PACKED_DESC_F_WRITE		1
> > > > #define VRING_PACKED_DESC_F_INDIRECT		2
> > > 
> > > As we aren't sharing code I think I prefer the second form.
> > > Maybe add VRING_NO_LEGACY so people can audit their code
> > > for assumptions?
> > 
> > Maybe it's better to name it as VIRTIO_RING_NO_LEGACY
> > which is more consistent with the names in other files.
> 
> OK
> 
> > > 
> > > We also want to guard layout definitions at the end of that file
> > > I think.
> > 
> > Do you mean guard the definitions of `struct vring` and
> > `struct vring_packed` with _NO_LEGACY?
> 
> I really mean vring_size/vring_init/vring_used_event/vring_avail_event
> these are pre- virtio 1 and should be disabled when
> building with no legacy.
> 
> But yes I would say let's make sure people can set a flag and
> find all dependencies of the split layout in their code easily.
I see. Thanks!
> 
> > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > + */
> > > > > > > > > > +#define
VRING_PACKED_DESC_F_AVAIL	7
> > > > > > > > > > +#define
VRING_PACKED_DESC_F_USED	15
> > > > > > > > > > +
> > > > > > > > > >    /* The Host uses this in
used->flags to advise the Guest: don't kick me when
> > > > > > > > > >     * you add a buffer. 
It's unreliable, so it's simply an optimization.  Guest
> > > > > > > > > >     * will still kick if
it's out of buffers. */
> > > > > > > > > > @@ -53,6 +60,23 @@
> > > > > > > > > >     * optimization.  */
> > > > > > > > > >    #define
VRING_AVAIL_F_NO_INTERRUPT	1
> > > > > > > > > > +/* Enable events in packed
ring. */
> > > > > > > > > > +#define
VRING_PACKED_EVENT_FLAG_ENABLE	0x0
> > > > > > > > > > +/* Disable events in packed
ring. */
> > > > > > > > > > +#define
VRING_PACKED_EVENT_FLAG_DISABLE	0x1
> > > > > > > > > > +/*
> > > > > > > > > > + * Enable events for a
specific descriptor in packed ring.
> > > > > > > > > > + * (as specified by
Descriptor Ring Change Event Offset/Wrap Counter).
> > > > > > > > > > + * Only valid if
VIRTIO_RING_F_EVENT_IDX has been negotiated.
> > > > > > > > > > + */
> > > > > > > > > > +#define
VRING_PACKED_EVENT_FLAG_DESC	0x2
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Any reason for using _FLAG_ instead
of _F_?
> > > > > > > > 
> > > > > > > > Yeah, it was suggested to not use _F_,
as these are values,
> > > > > > > > should not have _F_:
> > > > > > > > 
> > > > > > > >
https://patchwork.ozlabs.org/patch/942296/#1989390
> > > > > > > > 
> > > > > > > > Regards,
> > > > > > > > Tiwei
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Thanks
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > +
> > > > > > > > > > +/*
> > > > > > > > > > + * Wrap counter bit shift in
event suppression structure
> > > > > > > > > > + * of packed ring.
> > > > > > > > > > + */
> > > > > > > > > > +#define
VRING_PACKED_EVENT_F_WRAP_CTR	15
> > > > > > > > > > +
> > > > > > > > > >    /* We support indirect
buffer descriptors */
> > > > > > > > > >    #define
VIRTIO_RING_F_INDIRECT_DESC	28
> > > > > > > > > > @@ -171,4 +195,32 @@ static
inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
> > > > > > > > > >    	return (__u16)(new_idx -
event_idx - 1) < (__u16)(new_idx - old);
> > > > > > > > > >    }
> > > > > > > > > > +struct
vring_packed_desc_event {
> > > > > > > > > > +	/* Descriptor Ring Change
Event Offset/Wrap Counter. */
> > > > > > > > > > +	__le16 off_wrap;
> > > > > > > > > > +	/* Descriptor Ring Change
Event Flags. */
> > > > > > > > > > +	__le16 flags;
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +struct vring_packed_desc {
> > > > > > > > > > +	/* Buffer Address. */
> > > > > > > > > > +	__le64 addr;
> > > > > > > > > > +	/* Buffer Length. */
> > > > > > > > > > +	__le32 len;
> > > > > > > > > > +	/* Buffer ID. */
> > > > > > > > > > +	__le16 id;
> > > > > > > > > > +	/* The flags depending on
descriptor type. */
> > > > > > > > > > +	__le16 flags;
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +struct vring_packed {
> > > > > > > > > > +	unsigned int num;
> > > > > > > > > > +
> > > > > > > > > > +	struct vring_packed_desc
*desc;
> > > > > > > > > > +
> > > > > > > > > > +	struct
vring_packed_desc_event *driver;
> > > > > > > > > > +
> > > > > > > > > > +	struct
vring_packed_desc_event *device;
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > >    #endif /*
_UAPI_LINUX_VIRTIO_RING_H */

Maybe Matching Threads

Search for more apparently analagous threads

Linux Virtualization - Nov 2018 - [PATCH net-next v3 01/13] virtio: add packed ring types and macros

[PATCH net-next v3 01/13] virtio: add packed ring types and macros

[PATCH net-next v3 01/13] virtio: add packed ring types and macros

[PATCH net-next v3 01/13] virtio: add packed ring types and macros

Maybe Matching Threads