* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:16 [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout Ian Campbell
@ 2015-02-25 12:23 ` Andrew Cooper
2015-02-25 12:37 ` Ian Campbell
2015-02-25 12:34 ` Ian Campbell
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-02-25 12:23 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: keir, ian.jackson, tim, jbeulich, Wei Liu, Roger Pau Monné
On 25/02/15 12:16, Ian Campbell wrote:
> In RFC style, rather than relying on the implicit assumptions of a
> particular C ABI.
>
> I have also confirmed, using the Python gdb extension technique in
> [0], that the struct offsets (in a Linux binary at least) are the same
> as described here.
>
> I took the opportunity to also confirm that x86_32, x86_64, arm32 and
> arm64 are all the same.
>
> This highlighted that struct netif_rx_request was missing some
> explicit padding, which is added here.
>
> Lastly, fixup some struct names to allow the generated docs to
> properly hyperlink, mainly by adding the _t to type names where
> appropriate, but also s/netif_tx_extra/netif_extra_info_t/.
>
> [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
`pahole` from the dwarves package is your friend.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:23 ` Andrew Cooper
@ 2015-02-25 12:37 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2015-02-25 12:37 UTC (permalink / raw)
To: Andrew Cooper
Cc: Wei Liu, ian.jackson, tim, xen-devel, jbeulich, keir,
Roger Pau Monné
On Wed, 2015-02-25 at 12:23 +0000, Andrew Cooper wrote:
> On 25/02/15 12:16, Ian Campbell wrote:
> > I have also confirmed, using the Python gdb extension technique in
> > [0], that the struct offsets (in a Linux binary at least) are the same
> > as described here.
[...]
> > [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
> `pahole` from the dwarves package is your friend.
Indeed, I always forget it.
For reference here is the output, which matches the new docs too (phew!
).
$ pahole \
-C xen_netif_rx_request,xen_netif_rx_response,xen_netif_tx_request,xen_netif_tx_response,xen_netif_extra_info \
../linux-build-master-arm-native/drivers/net/xen-netback/netback.o
struct xen_netif_extra_info {
uint8_t type; /* 0 1 */
uint8_t flags; /* 1 1 */
union {
struct {
uint16_t size; /* 2 2 */
uint8_t type; /* 4 1 */
uint8_t pad; /* 5 1 */
uint16_t features; /* 6 2 */
} gso; /* 6 */
uint16_t pad[3]; /* 6 */
} u; /* 2 6 */
/* size: 8, cachelines: 1, members: 3 */
/* last cacheline: 8 bytes */
};
struct xen_netif_rx_request {
uint16_t id; /* 0 2 */
uint16_t pad; /* 2 2 */
grant_ref_t gref; /* 4 4 */
/* size: 8, cachelines: 1, members: 3 */
/* last cacheline: 8 bytes */
};
struct xen_netif_rx_response {
uint16_t id; /* 0 2 */
uint16_t offset; /* 2 2 */
uint16_t flags; /* 4 2 */
int16_t status; /* 6 2 */
/* size: 8, cachelines: 1, members: 4 */
/* last cacheline: 8 bytes */
};
struct xen_netif_tx_request {
grant_ref_t gref; /* 0 4 */
uint16_t offset; /* 4 2 */
uint16_t flags; /* 6 2 */
uint16_t id; /* 8 2 */
uint16_t size; /* 10 2 */
/* size: 12, cachelines: 1, members: 5 */
/* last cacheline: 12 bytes */
};
struct xen_netif_tx_response {
uint16_t id; /* 0 2 */
int16_t status; /* 2 2 */
/* size: 4, cachelines: 1, members: 2 */
/* last cacheline: 4 bytes */
};
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:16 [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout Ian Campbell
2015-02-25 12:23 ` Andrew Cooper
@ 2015-02-25 12:34 ` Ian Campbell
2015-02-25 12:48 ` Jürgen Groß
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2015-02-25 12:34 UTC (permalink / raw)
To: xen-devel; +Cc: keir, tim, ian.jackson, jbeulich, Wei Liu, Roger Pau Monné
On Wed, 2015-02-25 at 12:16 +0000, Ian Campbell wrote:
> In RFC style, rather than relying on the implicit assumptions of a
> particular C ABI.
>
> I have also confirmed, using the Python gdb extension technique in
> [0], that the struct offsets (in a Linux binary at least) are the same
> as described here.
[...]
> [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
I had intended after the cut to include for reference:
struct xen_netif_tx_request {
gref => 0
offset => 4
flags => 6
id => 8
size => 10
}
struct xen_netif_extra_info {
type => 0
flags => 1
u => 2
}
struct xen_netif_tx_response {
id => 0
status => 2
}
struct xen_netif_rx_request {
id => 0
gref => 4
}
struct xen_netif_rx_response {
id => 0
offset => 2
flags => 4
status => 6
}
(same for all arches, of course)
Ian.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:16 [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout Ian Campbell
2015-02-25 12:23 ` Andrew Cooper
2015-02-25 12:34 ` Ian Campbell
@ 2015-02-25 12:48 ` Jürgen Groß
2015-02-25 12:56 ` Ian Campbell
2015-02-25 12:59 ` Andrew Cooper
2015-02-25 13:17 ` Wei Liu
4 siblings, 1 reply; 9+ messages in thread
From: Jürgen Groß @ 2015-02-25 12:48 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: keir, ian.jackson, tim, jbeulich, Wei Liu, Roger Pau Monné
On 02/25/2015 01:16 PM, Ian Campbell wrote:
> In RFC style, rather than relying on the implicit assumptions of a
> particular C ABI.
>
> I have also confirmed, using the Python gdb extension technique in
> [0], that the struct offsets (in a Linux binary at least) are the same
> as described here.
>
> I took the opportunity to also confirm that x86_32, x86_64, arm32 and
> arm64 are all the same.
>
> This highlighted that struct netif_rx_request was missing some
> explicit padding, which is added here.
>
> Lastly, fixup some struct names to allow the generated docs to
> properly hyperlink, mainly by adding the _t to type names where
> appropriate, but also s/netif_tx_extra/netif_extra_info_t/.
>
> [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
One general question: should the RFC-like description really contain
macro names instead of explicit values?
> ---
> xen/include/public/io/netif.h | 137 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 127 insertions(+), 10 deletions(-)
>
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index 61e9aea..03b14fe 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -137,13 +137,129 @@
>
> /*
> * This is the 'wire' format for packets:
> - * Request 1: netif_tx_request -- NETTXF_* (any flags)
> - * [Request 2: netif_tx_extra] (only if request 1 has NETTXF_extra_info)
> - * [Request 3: netif_tx_extra] (only if request 2 has XEN_NETIF_EXTRA_MORE)
> - * Request 4: netif_tx_request -- NETTXF_more_data
> - * Request 5: netif_tx_request -- NETTXF_more_data
> + * Request 1: netif_tx_request_t -- NETTXF_* (any flags)
> + * [Request 2: netif_extra_info_t] (only if request 1 has NETTXF_extra_info)
> + * [Request 3: netif_extra_info_t] (only if request 2 has XEN_NETIF_EXTRA_MORE)
> + * Request 4: netif_tx_request_t -- NETTXF_more_data
> + * Request 5: netif_tx_request_t -- NETTXF_more_data
> * ...
> - * Request N: netif_tx_request -- 0
> + * Request N: netif_tx_request_t -- 0
> + */
> +
> +/*
> + * Guest transmit
> + * ==============
> + *
> + * Ring slot size is 12 octets, however not all request/response
> + * structs use the full size.
> + *
> + * tx request data (netif_tx_request_t)
> + * ------------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | grant ref | offset | flags |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | size |
> + * +-----+-----+-----+-----+
> + *
> + * grant ref: Reference to buffer page.
> + * offset: Offset within buffer page.
> + * flags: NETTXF_*.
> + * id: request identifier, echoed in response.
> + * size: packet size in bytes.
> + *
> + * tx response (netif_tx_response_t)
> + * ---------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | status | unused |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | unused |
> + * +-----+-----+-----+-----+
> + *
> + * id: reflects id in transmit request
> + * status: NETIF_RSP_*
> + *
> + * Guest receive
> + * =============
> +
Empty line intended?
> + * Ring slot size is 8 octets.
> + *
> + * rx request (netif_rx_request_t)
> + * -------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | pad | gref |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * id: request identifier, echoed in response.
> + * gref: reference to incoming granted frame.
> + *
> + * rx response (netif_rx_response_t)
> + * ---------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | offset | flags | status |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * id: reflects id in receive request
> + * offset: offset in page of start of received packet
> + * flags: NETRXF_*
> + * status: -ve: NETIF_RSP_*; +ve: Rx'ed pkt size.
What is -/+ve?
> + *
> + * Extra Info
> + * ==========
> + *
> + * Can be present if initial request has NET{T,R}XF_extra_info, or
> + * previous extra request has XEN_NETIF_EXTRA_MORE.
> + *
> + * The struct therefore needs to fit into either a tx or rx slot and
> + * is therefore limited to 8 octets.
> + *
> + * extra info (netif_extra_info_t)
> + * -------------------------------
> + *
> + * General format:
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |type |flags| type specfic data |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | padding for tx |
> + * +-----+-----+-----+-----+
> + *
> + * type: XEN_NETIF_EXTRA_TYPE_*
> + * flags: XEN_NETIF_EXTRA_FLAG_*
> + * padding for tx: present only in the tx case due to 8 octet limit
> + * from rx case. Not shown in type specific entries below.
> + *
> + * XEN_NETIF_EXTRA_TYPE_GSO:
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |type |flags| size |type | pad | features |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * type: Must be XEN_NETIF_EXTRA_TYPE_GSO
> + * flags: XEN_NETIF_EXTRA_FLAG_*
> + * size: Maximum payload size of each segment.
> + * type: XEN_NETIF_GSO_TYPE_*
> + * features: EN_NETIF_GSO_FEAT_*
> + *
> + * XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}:
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |type |flags| addr |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * type: Must be XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}
> + * flags: XEN_NETIF_EXTRA_FLAG_*
> + * addr: addrss to add/remove
s/addrss/address/
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:48 ` Jürgen Groß
@ 2015-02-25 12:56 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2015-02-25 12:56 UTC (permalink / raw)
To: Jürgen Groß
Cc: keir, tim, ian.jackson, xen-devel, jbeulich, Wei Liu,
Roger Pau Monné
On Wed, 2015-02-25 at 13:48 +0100, Jürgen Groß wrote:
> On 02/25/2015 01:16 PM, Ian Campbell wrote:
> > In RFC style, rather than relying on the implicit assumptions of a
> > particular C ABI.
> >
> > I have also confirmed, using the Python gdb extension technique in
> > [0], that the struct offsets (in a Linux binary at least) are the same
> > as described here.
> >
> > I took the opportunity to also confirm that x86_32, x86_64, arm32 and
> > arm64 are all the same.
> >
> > This highlighted that struct netif_rx_request was missing some
> > explicit padding, which is added here.
> >
> > Lastly, fixup some struct names to allow the generated docs to
> > properly hyperlink, mainly by adding the _t to type names where
> > appropriate, but also s/netif_tx_extra/netif_extra_info_t/.
> >
> > [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
>
> One general question: should the RFC-like description really contain
> macro names instead of explicit values?
The real purpose here is to document the binary layout of the fields,
the actual meanings of them are already documented (at least somewhat)
elsewhere in the header.
Improvements to that aspect in patch form would obviously still be
appreciated.
> > + * status: -ve: NETIF_RSP_*; +ve: Rx'ed pkt size.
>
> What is -/+ve?
This was from the existing comment in the struct, I thought it was a
pretty common abbreviation for positive and negative.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:16 [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout Ian Campbell
` (2 preceding siblings ...)
2015-02-25 12:48 ` Jürgen Groß
@ 2015-02-25 12:59 ` Andrew Cooper
2015-02-25 13:19 ` Ian Campbell
2015-02-25 13:17 ` Wei Liu
4 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-02-25 12:59 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: keir, ian.jackson, tim, jbeulich, Wei Liu, Roger Pau Monné
On 25/02/15 12:16, Ian Campbell wrote:
> In RFC style, rather than relying on the implicit assumptions of a
> particular C ABI.
>
> I have also confirmed, using the Python gdb extension technique in
> [0], that the struct offsets (in a Linux binary at least) are the same
> as described here.
>
> I took the opportunity to also confirm that x86_32, x86_64, arm32 and
> arm64 are all the same.
>
> This highlighted that struct netif_rx_request was missing some
> explicit padding, which is added here.
>
> Lastly, fixup some struct names to allow the generated docs to
> properly hyperlink, mainly by adding the _t to type names where
> appropriate, but also s/netif_tx_extra/netif_extra_info_t/.
>
> [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
I don't see any mention of endianness, either preexisting in the file,
or part of this patch.
Other than that, the changes look good.
~Andrew
> ---
> xen/include/public/io/netif.h | 137 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 127 insertions(+), 10 deletions(-)
>
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index 61e9aea..03b14fe 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -137,13 +137,129 @@
>
> /*
> * This is the 'wire' format for packets:
> - * Request 1: netif_tx_request -- NETTXF_* (any flags)
> - * [Request 2: netif_tx_extra] (only if request 1 has NETTXF_extra_info)
> - * [Request 3: netif_tx_extra] (only if request 2 has XEN_NETIF_EXTRA_MORE)
> - * Request 4: netif_tx_request -- NETTXF_more_data
> - * Request 5: netif_tx_request -- NETTXF_more_data
> + * Request 1: netif_tx_request_t -- NETTXF_* (any flags)
> + * [Request 2: netif_extra_info_t] (only if request 1 has NETTXF_extra_info)
> + * [Request 3: netif_extra_info_t] (only if request 2 has XEN_NETIF_EXTRA_MORE)
> + * Request 4: netif_tx_request_t -- NETTXF_more_data
> + * Request 5: netif_tx_request_t -- NETTXF_more_data
> * ...
> - * Request N: netif_tx_request -- 0
> + * Request N: netif_tx_request_t -- 0
> + */
> +
> +/*
> + * Guest transmit
> + * ==============
> + *
> + * Ring slot size is 12 octets, however not all request/response
> + * structs use the full size.
> + *
> + * tx request data (netif_tx_request_t)
> + * ------------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | grant ref | offset | flags |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | size |
> + * +-----+-----+-----+-----+
> + *
> + * grant ref: Reference to buffer page.
> + * offset: Offset within buffer page.
> + * flags: NETTXF_*.
> + * id: request identifier, echoed in response.
> + * size: packet size in bytes.
> + *
> + * tx response (netif_tx_response_t)
> + * ---------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | status | unused |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | unused |
> + * +-----+-----+-----+-----+
> + *
> + * id: reflects id in transmit request
> + * status: NETIF_RSP_*
> + *
> + * Guest receive
> + * =============
> +
> + * Ring slot size is 8 octets.
> + *
> + * rx request (netif_rx_request_t)
> + * -------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | pad | gref |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * id: request identifier, echoed in response.
> + * gref: reference to incoming granted frame.
> + *
> + * rx response (netif_rx_response_t)
> + * ---------------------------------
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | id | offset | flags | status |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * id: reflects id in receive request
> + * offset: offset in page of start of received packet
> + * flags: NETRXF_*
> + * status: -ve: NETIF_RSP_*; +ve: Rx'ed pkt size.
> + *
> + * Extra Info
> + * ==========
> + *
> + * Can be present if initial request has NET{T,R}XF_extra_info, or
> + * previous extra request has XEN_NETIF_EXTRA_MORE.
> + *
> + * The struct therefore needs to fit into either a tx or rx slot and
> + * is therefore limited to 8 octets.
> + *
> + * extra info (netif_extra_info_t)
> + * -------------------------------
> + *
> + * General format:
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |type |flags| type specfic data |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * | padding for tx |
> + * +-----+-----+-----+-----+
> + *
> + * type: XEN_NETIF_EXTRA_TYPE_*
> + * flags: XEN_NETIF_EXTRA_FLAG_*
> + * padding for tx: present only in the tx case due to 8 octet limit
> + * from rx case. Not shown in type specific entries below.
> + *
> + * XEN_NETIF_EXTRA_TYPE_GSO:
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |type |flags| size |type | pad | features |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * type: Must be XEN_NETIF_EXTRA_TYPE_GSO
> + * flags: XEN_NETIF_EXTRA_FLAG_*
> + * size: Maximum payload size of each segment.
> + * type: XEN_NETIF_GSO_TYPE_*
> + * features: EN_NETIF_GSO_FEAT_*
> + *
> + * XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}:
> + *
> + * 0 1 2 3 4 5 6 7 octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |type |flags| addr |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * type: Must be XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}
> + * flags: XEN_NETIF_EXTRA_FLAG_*
> + * addr: addrss to add/remove
> */
>
> /* Protocol checksum field is blank in the packet (hardware offload)? */
> @@ -179,7 +295,7 @@ typedef struct netif_tx_request netif_tx_request_t;
> #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3) /* u.mcast */
> #define XEN_NETIF_EXTRA_TYPE_MAX (4)
>
> -/* netif_extra_info flags. */
> +/* netif_extra_info_t flags. */
> #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
> #define XEN_NETIF_EXTRA_FLAG_MORE (1U<<_XEN_NETIF_EXTRA_FLAG_MORE)
>
> @@ -189,8 +305,8 @@ typedef struct netif_tx_request netif_tx_request_t;
> #define XEN_NETIF_GSO_TYPE_TCPV6 (2)
>
> /*
> - * This structure needs to fit within both netif_tx_request and
> - * netif_rx_response for compatibility.
> + * This structure needs to fit within both netif_tx_request_t and
> + * netif_rx_response_t for compatibility.
> */
> struct netif_extra_info {
> uint8_t type; /* XEN_NETIF_EXTRA_TYPE_* */
> @@ -251,6 +367,7 @@ typedef struct netif_tx_response netif_tx_response_t;
>
> struct netif_rx_request {
> uint16_t id; /* Echoed in response message. */
> + uint16_t pad;
> grant_ref_t gref; /* Reference to incoming granted frame */
> };
> typedef struct netif_rx_request netif_rx_request_t;
> @@ -289,7 +406,7 @@ DEFINE_RING_TYPES(netif_rx, struct netif_rx_request, struct netif_rx_response);
> #define NETIF_RSP_DROPPED -2
> #define NETIF_RSP_ERROR -1
> #define NETIF_RSP_OKAY 0
> -/* No response: used for auxiliary requests (e.g., netif_tx_extra). */
> +/* No response: used for auxiliary requests (e.g., netif_extra_info_t). */
> #define NETIF_RSP_NULL 1
>
> #endif
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:59 ` Andrew Cooper
@ 2015-02-25 13:19 ` Ian Campbell
0 siblings, 0 replies; 9+ messages in thread
From: Ian Campbell @ 2015-02-25 13:19 UTC (permalink / raw)
To: Andrew Cooper
Cc: Wei Liu, ian.jackson, tim, xen-devel, jbeulich, keir,
Roger Pau Monné
On Wed, 2015-02-25 at 12:59 +0000, Andrew Cooper wrote:
> On 25/02/15 12:16, Ian Campbell wrote:
> > In RFC style, rather than relying on the implicit assumptions of a
> > particular C ABI.
> >
> > I have also confirmed, using the Python gdb extension technique in
> > [0], that the struct offsets (in a Linux binary at least) are the same
> > as described here.
> >
> > I took the opportunity to also confirm that x86_32, x86_64, arm32 and
> > arm64 are all the same.
> >
> > This highlighted that struct netif_rx_request was missing some
> > explicit padding, which is added here.
> >
> > Lastly, fixup some struct names to allow the generated docs to
> > properly hyperlink, mainly by adding the _t to type names where
> > appropriate, but also s/netif_tx_extra/netif_extra_info_t/.
> >
> > [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
>
> I don't see any mention of endianness, either preexisting in the file,
> or part of this patch.
I guess that is more a property of the XEN_IO_PROTO_ABI_* negotiated by
front and back than the individual pv protocols (at least by default,
i.e. unless the specific fooif.h says otherwise).
There is probably space to make that clearer overall, but I think that's
orthogonal to this patch.
>
> Other than that, the changes look good.
Thanks.
>
> ~Andrew
>
> > ---
> > xen/include/public/io/netif.h | 137 ++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 127 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> > index 61e9aea..03b14fe 100644
> > --- a/xen/include/public/io/netif.h
> > +++ b/xen/include/public/io/netif.h
> > @@ -137,13 +137,129 @@
> >
> > /*
> > * This is the 'wire' format for packets:
> > - * Request 1: netif_tx_request -- NETTXF_* (any flags)
> > - * [Request 2: netif_tx_extra] (only if request 1 has NETTXF_extra_info)
> > - * [Request 3: netif_tx_extra] (only if request 2 has XEN_NETIF_EXTRA_MORE)
> > - * Request 4: netif_tx_request -- NETTXF_more_data
> > - * Request 5: netif_tx_request -- NETTXF_more_data
> > + * Request 1: netif_tx_request_t -- NETTXF_* (any flags)
> > + * [Request 2: netif_extra_info_t] (only if request 1 has NETTXF_extra_info)
> > + * [Request 3: netif_extra_info_t] (only if request 2 has XEN_NETIF_EXTRA_MORE)
> > + * Request 4: netif_tx_request_t -- NETTXF_more_data
> > + * Request 5: netif_tx_request_t -- NETTXF_more_data
> > * ...
> > - * Request N: netif_tx_request -- 0
> > + * Request N: netif_tx_request_t -- 0
> > + */
> > +
> > +/*
> > + * Guest transmit
> > + * ==============
> > + *
> > + * Ring slot size is 12 octets, however not all request/response
> > + * structs use the full size.
> > + *
> > + * tx request data (netif_tx_request_t)
> > + * ------------------------------------
> > + *
> > + * 0 1 2 3 4 5 6 7 octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | grant ref | offset | flags |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | id | size |
> > + * +-----+-----+-----+-----+
> > + *
> > + * grant ref: Reference to buffer page.
> > + * offset: Offset within buffer page.
> > + * flags: NETTXF_*.
> > + * id: request identifier, echoed in response.
> > + * size: packet size in bytes.
> > + *
> > + * tx response (netif_tx_response_t)
> > + * ---------------------------------
> > + *
> > + * 0 1 2 3 4 5 6 7 octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | id | status | unused |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | unused |
> > + * +-----+-----+-----+-----+
> > + *
> > + * id: reflects id in transmit request
> > + * status: NETIF_RSP_*
> > + *
> > + * Guest receive
> > + * =============
> > +
> > + * Ring slot size is 8 octets.
> > + *
> > + * rx request (netif_rx_request_t)
> > + * -------------------------------
> > + *
> > + * 0 1 2 3 4 5 6 7 octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | id | pad | gref |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + *
> > + * id: request identifier, echoed in response.
> > + * gref: reference to incoming granted frame.
> > + *
> > + * rx response (netif_rx_response_t)
> > + * ---------------------------------
> > + *
> > + * 0 1 2 3 4 5 6 7 octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | id | offset | flags | status |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + *
> > + * id: reflects id in receive request
> > + * offset: offset in page of start of received packet
> > + * flags: NETRXF_*
> > + * status: -ve: NETIF_RSP_*; +ve: Rx'ed pkt size.
> > + *
> > + * Extra Info
> > + * ==========
> > + *
> > + * Can be present if initial request has NET{T,R}XF_extra_info, or
> > + * previous extra request has XEN_NETIF_EXTRA_MORE.
> > + *
> > + * The struct therefore needs to fit into either a tx or rx slot and
> > + * is therefore limited to 8 octets.
> > + *
> > + * extra info (netif_extra_info_t)
> > + * -------------------------------
> > + *
> > + * General format:
> > + *
> > + * 0 1 2 3 4 5 6 7 octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * |type |flags| type specfic data |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * | padding for tx |
> > + * +-----+-----+-----+-----+
> > + *
> > + * type: XEN_NETIF_EXTRA_TYPE_*
> > + * flags: XEN_NETIF_EXTRA_FLAG_*
> > + * padding for tx: present only in the tx case due to 8 octet limit
> > + * from rx case. Not shown in type specific entries below.
> > + *
> > + * XEN_NETIF_EXTRA_TYPE_GSO:
> > + *
> > + * 0 1 2 3 4 5 6 7 octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * |type |flags| size |type | pad | features |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + *
> > + * type: Must be XEN_NETIF_EXTRA_TYPE_GSO
> > + * flags: XEN_NETIF_EXTRA_FLAG_*
> > + * size: Maximum payload size of each segment.
> > + * type: XEN_NETIF_GSO_TYPE_*
> > + * features: EN_NETIF_GSO_FEAT_*
> > + *
> > + * XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}:
> > + *
> > + * 0 1 2 3 4 5 6 7 octet
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + * |type |flags| addr |
> > + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > + *
> > + * type: Must be XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}
> > + * flags: XEN_NETIF_EXTRA_FLAG_*
> > + * addr: addrss to add/remove
> > */
> >
> > /* Protocol checksum field is blank in the packet (hardware offload)? */
> > @@ -179,7 +295,7 @@ typedef struct netif_tx_request netif_tx_request_t;
> > #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3) /* u.mcast */
> > #define XEN_NETIF_EXTRA_TYPE_MAX (4)
> >
> > -/* netif_extra_info flags. */
> > +/* netif_extra_info_t flags. */
> > #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
> > #define XEN_NETIF_EXTRA_FLAG_MORE (1U<<_XEN_NETIF_EXTRA_FLAG_MORE)
> >
> > @@ -189,8 +305,8 @@ typedef struct netif_tx_request netif_tx_request_t;
> > #define XEN_NETIF_GSO_TYPE_TCPV6 (2)
> >
> > /*
> > - * This structure needs to fit within both netif_tx_request and
> > - * netif_rx_response for compatibility.
> > + * This structure needs to fit within both netif_tx_request_t and
> > + * netif_rx_response_t for compatibility.
> > */
> > struct netif_extra_info {
> > uint8_t type; /* XEN_NETIF_EXTRA_TYPE_* */
> > @@ -251,6 +367,7 @@ typedef struct netif_tx_response netif_tx_response_t;
> >
> > struct netif_rx_request {
> > uint16_t id; /* Echoed in response message. */
> > + uint16_t pad;
> > grant_ref_t gref; /* Reference to incoming granted frame */
> > };
> > typedef struct netif_rx_request netif_rx_request_t;
> > @@ -289,7 +406,7 @@ DEFINE_RING_TYPES(netif_rx, struct netif_rx_request, struct netif_rx_response);
> > #define NETIF_RSP_DROPPED -2
> > #define NETIF_RSP_ERROR -1
> > #define NETIF_RSP_OKAY 0
> > -/* No response: used for auxiliary requests (e.g., netif_tx_extra). */
> > +/* No response: used for auxiliary requests (e.g., netif_extra_info_t). */
> > #define NETIF_RSP_NULL 1
> >
> > #endif
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout
2015-02-25 12:16 [PATCH DOCDAY] netif.h: describe request/response structures in terms of binary layout Ian Campbell
` (3 preceding siblings ...)
2015-02-25 12:59 ` Andrew Cooper
@ 2015-02-25 13:17 ` Wei Liu
4 siblings, 0 replies; 9+ messages in thread
From: Wei Liu @ 2015-02-25 13:17 UTC (permalink / raw)
To: Ian Campbell
Cc: keir, ian.jackson, tim, xen-devel, jbeulich, Wei Liu,
Roger Pau Monné
On Wed, Feb 25, 2015 at 12:16:50PM +0000, Ian Campbell wrote:
> In RFC style, rather than relying on the implicit assumptions of a
> particular C ABI.
>
> I have also confirmed, using the Python gdb extension technique in
> [0], that the struct offsets (in a Linux binary at least) are the same
> as described here.
>
> I took the opportunity to also confirm that x86_32, x86_64, arm32 and
> arm64 are all the same.
>
> This highlighted that struct netif_rx_request was missing some
> explicit padding, which is added here.
>
> Lastly, fixup some struct names to allow the generated docs to
> properly hyperlink, mainly by adding the _t to type names where
> appropriate, but also s/netif_tx_extra/netif_extra_info_t/.
>
> [] http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c,
Missing "0" and extraneous comma at the end.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
The rest looks good to me. Thanks for doing this.
Acked-by: Wei Liu <wei.liu2@citrix.com>
Wei.
^ permalink raw reply [flat|nested] 9+ messages in thread