V4V is a copy based inter vm communication system. Please have a look at this thread for more detail about V4V. http://lists.xen.org/archives/html/xen-devel/2012-05/msg01866.html This patch series is work in progress but I wanted to post it early enough so I can get feedback from people. v2 changes: - Cleanup plugin header - Include basic access control - Use guest_handle_for_field changes requested not a v2: - Switch to event channel instead of virq Jean Guyader (5): xen: add ssize_t v4v: Introduce VIRQ_V4V xen: Enforce introduce guest_handle_for_field xen: Add V4V implementation v4v: Introduce basic access control to V4V xen/arch/x86/hvm/hvm.c | 9 +- xen/arch/x86/x86_32/entry.S | 2 + xen/arch/x86/x86_64/compat/entry.S | 2 + xen/arch/x86/x86_64/entry.S | 2 + xen/common/Makefile | 1 + xen/common/domain.c | 11 +- xen/common/event_channel.c | 1 + xen/common/v4v.c | 2020 ++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/types.h | 1 + xen/include/asm-x86/guest_access.h | 3 + xen/include/asm-x86/types.h | 6 + xen/include/public/v4v.h | 243 +++++ xen/include/public/xen.h | 4 +- xen/include/xen/sched.h | 5 + xen/include/xen/v4v.h | 213 ++++ 15 files changed, 2517 insertions(+), 6 deletions(-) create mode 100644 xen/common/v4v.c create mode 100644 xen/include/public/v4v.h create mode 100644 xen/include/xen/v4v.h -- 1.7.9.5 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/include/asm-arm/types.h | 1 + xen/include/asm-x86/types.h | 6 ++++++ 2 files changed, 7 insertions(+) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Introduce the global virq VIRQ_V4V Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/common/event_channel.c | 1 + xen/include/public/xen.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jean Guyader
2012-Jun-28 16:26 UTC
[PATCH 3/5] xen: Enforce introduce guest_handle_for_field
This helper turns a field of a GUEST_HANDLE in a GUEST_HANDLE. Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/include/asm-x86/guest_access.h | 3 +++ 1 file changed, 3 insertions(+) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Setup of v4v domains a domain gets created and cleanup when a domain die. Wire up the v4v hypercall. Include v4v internal and public headers. Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/arch/x86/hvm/hvm.c | 9 +- xen/arch/x86/x86_32/entry.S | 2 + xen/arch/x86/x86_64/compat/entry.S | 2 + xen/arch/x86/x86_64/entry.S | 2 + xen/common/Makefile | 1 + xen/common/domain.c | 11 +- xen/common/v4v.c | 1755 ++++++++++++++++++++++++++++++++++++ xen/include/public/v4v.h | 240 +++++ xen/include/public/xen.h | 2 +- xen/include/xen/sched.h | 5 + xen/include/xen/v4v.h | 187 ++++ 11 files changed, 2211 insertions(+), 5 deletions(-) create mode 100644 xen/common/v4v.c create mode 100644 xen/include/public/v4v.h create mode 100644 xen/include/xen/v4v.h _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Signed-off-by: Jean Guyader <jean.guyader@citrix.com> --- xen/common/v4v.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++ xen/include/public/v4v.h | 3 + xen/include/xen/v4v.h | 26 +++++ 3 files changed, 294 insertions(+) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote:If this is really needed (which I doubt, looking at the two users and their - respectively - sole callers), then for x86 please put the definitions alongside the size_t ones. Until the need for the type is shown, this is a NAK from me. Jan
>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote:This appears to be unchanged from v1, so the inconsistency between implementation (per-vCPU vIRQ) and documentation (global vIRQ) remains. Jan
Jan Beulich
2012-Jun-29 08:10 UTC
Re: [PATCH 3/5] xen: Enforce introduce guest_handle_for_field
>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: > This helper turns a field of a GUEST_HANDLE in > a GUEST_HANDLE. > > Signed-off-by: Jean Guyader <jean.guyader@citrix.com>Minor or not - this patch is not from you originally. Jan
>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: > --- /dev/null > +++ b/xen/include/public/v4v.h > @@ -0,0 +1,240 @@ > +/****************************************************************************** > + * V4V > + * > + * Version 2 of v2v (Virtual-to-Virtual) > + * > + * Copyright (c) 2010, Citrix Systems > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#ifndef __XEN_PUBLIC_V4V_H__ > +#define __XEN_PUBLIC_V4V_H__ > + > +#include "xen.h" > + > +/* > + * Structure definitions > + */ > + > +#define V4V_PROTO_DGRAM 0x3c2c1db8 > +#define V4V_PROTO_STREAM 0x70f6a8e5 > + > +#ifdef __i386__How about ARM?> +# define V4V_RING_MAGIC 0xdf6977f231abd910ULL > +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302dULLIs there any reason these can''t be also used for 64-bit?> +#else > +# define V4V_RING_MAGIC 0xdf6977f231abd910 > +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302d > +#endif > +#define V4V_DOMID_INVALID (0x7FFFU)Any reason not to use DOMID_INVALID instead?> +#define V4V_DOMID_NONE V4V_DOMID_INVALID > +#define V4V_DOMID_ANY V4V_DOMID_INVALID > +#define V4V_PORT_NONE 0 > + > +/* > + * struct v4v_iov > + * { > + * 64 bits: iov_base > + * 64 bits: iov_len > + * } > + */What''s the point of not defining the types here?> + > +/* > + * struct v4v_addr > + * { > + * 32 bits: port > + * 16 bits: domid > + * } > + */ > + > +/* > + * v4v_ring_id > + * { > + * struct v4v_addr: addr > + * 16 bits: partner > + * } > + */ > + > +/* > + * v4v_ring > + * { > + * 64 bits: magic > + * v4v_rind_id: id > + * 32 bits: len > + * 32 bits: rx_ptr > + * 32 bits: tx_ptr > + * 64 bits: padding > + * ... : ring > + * } > + * > + * id: > + * xen only looks at this during register/unregister > + * and will fill in id.addr.domain > + * > + * rx_ptr: rx pointer, modified by domain > + * tx_ptr: tx pointer, modified by xen > + */ > + > +#ifdef __i386__ > +#define V4V_RING_DATA_MAGIC 0x4ce4d30fbc82e92aULLSame here as above.> +#else > +#define V4V_RING_DATA_MAGIC 0x4ce4d30fbc82e92a > +#endif > + > +#define V4V_RING_DATA_F_EMPTY 1U << 0 /* Ring is empty */ > +#define V4V_RING_DATA_F_EXISTS 1U << 1 /* Ring exists */ > +#define V4V_RING_DATA_F_PENDING 1U << 2 /* Pending interrupt exists - do not > + rely on this field - for > + profiling only */ > +#define V4V_RING_DATA_F_SUFFICIENT 1U << 3 /* Sufficient space to queue > + space_required bytes exists */ > + > +/* > + * v4v_ring_data_ent > + * { > + * v4v_addr: ring > + * 16 bits: flags > + * 16 bits: padding > + * 32 bits: space_required > + * 32 bits: max_message_size > + * } > + */ > + > +/* > + * v4v_ring_data > + * { > + * 64 bits: magic (V4V_RING_DATA_MAGIC) > + * 32 bits: nent > + * 32 bits: padding > + * 256 bits: reserved > + * ... : v4v_ring_data_ent > + * } > + */ > + > + > +#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf) > +/* > + * Messages on the ring are padded to 128 bits > + * Len here refers to the exact length of the data not including the > + * 128 bit header. The message uses > + * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes > + */ > + > +/* > + * v4v_stream_header > + * { > + * 32 bits: flags > + * 32 bits: conid > + * } > + */ > + > +/* > + * v4v_ring_message_header > + * { > + * 32 bits: len > + * v4v_addr: source > + * 32 bits: protocol > + * ... : data > + * } > + */ > + > +/* > + * HYPERCALLS > + */ > + > +#define V4VOP_register_ring 1 > +/* > + * Registers a ring with Xen, if a ring with the same v4v_ring_id exists, > + * this ring takes its place, registration will not change tx_ptr > + * unless it is invalid > + * > + * do_v4v_op(V4VOP_unregister_ring, > + * v4v_ring, XEN_GUEST_HANDLE(v4v_pfn), > + * NULL, npage, 0) > + */ > + > + > +#define V4VOP_unregister_ring 2 > +/* > + * Unregister a ring. > + * > + * v4v_hypercall(V4VOP_send, v4v_ring, NULL, NULL, 0, 0)Assuming this and the earlier do_v4v_op() refer to the same thing, please use a single name consistently.> + */ > + > +#define V4VOP_send 3 > +/* > + * Sends len bytes of buf to dst, giving src as the source address (xen will > + * ignore src->domain and put your domain in the actually message), xen > + * first looks for a ring with id.addr==dst and id.partner==sending_domain > + * if that fails it looks for id.addr==dst and id.partner==DOMID_ANY. > + * protocol is the 32 bit protocol number used from the message > + * most likely V4V_PROTO_DGRAM or STREAM. If insufficient space exists > + * it will return -EAGAIN and xen will twing the V4V_INTERRUPT when > + * sufficient space becomes available > + * > + * v4v_hypercall(V4VOP_send, > + * v4v_addr src, > + * v4v_addr dst, > + * void* buf, > + * uint32_t len, > + * uint32_t protocol) > + */ > + > + > +#define V4VOP_notify 4 > +/* Asks xen for information about other rings in the system > + * > + * ent->ring is the v4v_addr_t of the ring you want information on > + * the same matching rules are used as for V4VOP_send. > + * > + * ent->space_required if this field is not null xen will check > + * that there is space in the destination ring for this many bytes > + * of payload. If there is it will set the V4V_RING_DATA_F_SUFFICIENT > + * and CANCEL any pending interrupt for that ent->ring, if insufficient > + * space is available it will schedule an interrupt and the flag will > + * not be set. > + * > + * The flags are set by xen when notify replies > + * V4V_RING_DATA_F_EMPTY ring is empty > + * V4V_RING_DATA_F_PENDING interrupt is pending - don''t rely on this > + * V4V_RING_DATA_F_SUFFICIENT sufficient space for space_required is there > + * V4V_RING_DATA_F_EXISTS ring exists > + * > + * v4v_hypercall(V4VOP_notify, > + * XEN_GUEST_HANDLE(v4v_ring_data_ent) ent, > + * NULL, NULL, nent, 0) > + */ > + > + > +#define V4VOP_sendv 5 > +/* > + * Identical to V4VOP_send except rather than buf and len it takes > + * an array of v4v_iov and a length of the array. > + * > + * v4v_hypercall(V4VOP_sendv, > + * v4v_addr src, > + * v4v_addr dst, > + * v4v_iov iov, > + * uint32_t niov, > + * uint32_t protocol) > + */ > + > +#endif /* __XEN_PUBLIC_V4V_H__ */ > --- /dev/null > +++ b/xen/include/xen/v4v.h > @@ -0,0 +1,187 @@ > +/****************************************************************************** > + * V4V > + * > + * Version 2 of v2v (Virtual-to-Virtual) > + * > + * Copyright (c) 2010, Citrix Systems > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#ifndef __V4V_PRIVATE_H__ > +#define __V4V_PRIVATE_H__ > + > +#include <xen/config.h> > +#include <xen/types.h> > +#include <xen/spinlock.h> > +#include <xen/smp.h> > +#include <xen/shared.h> > +#include <xen/list.h> > +#include <public/v4v.h> > + > +#define V4V_HTABLE_SIZE 32 > + > +#define V4V_PACKED __attribute__ ((packed)) > + > +/* > + * Structures > + */ > + > +typedef struct v4v_iov > +{ > + uint64_t iov_base; > + uint64_t iov_len; > +} V4V_PACKED v4v_iov_t;While you moved this to a private header now, I continue to think that none of the uses of V4V_PACKED are actually warranted (and hence these can''t serve as reason for moving these public definitions into a private header). Use explicit padding fields, and you ought to be fine.> +DEFINE_XEN_GUEST_HANDLE (v4v_iov_t); > + > +typedef struct v4v_addr > +{ > + uint32_t port; > + domid_t domain; > +} V4V_PACKED v4v_addr_t; > +DEFINE_XEN_GUEST_HANDLE (v4v_addr_t); > + > +typedef struct v4v_ring_id > +{ > + struct v4v_addr addr; > + domid_t partner; > +} V4V_PACKED v4v_ring_id_t; > + > +typedef uint64_t v4v_pfn_t; > +DEFINE_XEN_GUEST_HANDLE (v4v_pfn_t); > + > +typedef struct v4v_ring > +{ > + uint64_t magic; > + struct v4v_ring_id id; > + uint32_t len; > + uint32_t rx_ptr; > + uint32_t tx_ptr; > + uint64_t reserved[4]; > + uint8_t ring[0]; > +} V4V_PACKED v4v_ring_t; > +DEFINE_XEN_GUEST_HANDLE (v4v_ring_t);If moved into the public header (where they belong imo), apart from this one none of the guest handle defines should actually be there, as there''s no guest interface that would make use of them (they''re used internally to v4v.c only). Also, conventionally there''s no space before the opening parenthesis here. Jan> + > +typedef struct v4v_ring_data_ent > +{ > + struct v4v_addr ring; > + uint16_t flags; > + uint16_t pad0; > + uint32_t space_required; > + uint32_t max_message_size; > +} V4V_PACKED v4v_ring_data_ent_t; > +DEFINE_XEN_GUEST_HANDLE (v4v_ring_data_ent_t); > + > +typedef struct v4v_ring_data > +{ > + uint64_t magic; > + uint32_t nent; > + uint32_t padding; > + uint64_t reserved[4]; > + v4v_ring_data_ent_t ring[0]; > +} V4V_PACKED v4v_ring_data_t; > +DEFINE_XEN_GUEST_HANDLE (v4v_ring_data_t); > + > +struct v4v_stream_header > +{ > + uint32_t flags; > + uint32_t conid; > +} V4V_PACKED; > + > +struct v4v_ring_message_header > +{ > + uint32_t len; > + struct v4v_addr source; > + uint32_t protocol; > + uint8_t data[0]; > +} V4V_PACKED; > + > +/* > + * Helper functions > + */ > + > + > +static inline uint16_t > +v4v_hash_fn (struct v4v_ring_id *id) > +{ > + uint16_t ret; > + ret = (uint16_t) (id->addr.port >> 16); > + ret ^= (uint16_t) id->addr.port; > + ret ^= id->addr.domain; > + ret ^= id->partner; > + > + ret &= (V4V_HTABLE_SIZE-1); > + > + return ret; > +} > + > +struct v4v_pending_ent > +{ > + struct hlist_node node; > + domid_t id; > + uint32_t len; > +} V4V_PACKED; > + > + > +struct v4v_ring_info > +{ > + /* next node in the hash, protected by L2 */ > + struct hlist_node node; > + /* this ring''s id, protected by L2 */ > + struct v4v_ring_id id; > + /* L3 */ > + spinlock_t lock; > + /* cached length of the ring (from ring->len), protected by L3 */ > + uint32_t len; > + uint32_t npage; > + /* cached tx pointer location, protected by L3 */ > + uint32_t tx_ptr; > + /* guest ring, protected by L3 */ > + XEN_GUEST_HANDLE(v4v_ring_t) ring; > + /* mapped ring pages protected by L3*/ > + uint8_t **mfn_mapping; > + /* list of mfns of guest ring */ > + mfn_t *mfns; > + /* list of struct v4v_pending_ent for this ring, L3 */ > + struct hlist_head pending; > +} V4V_PACKED; > + > +/* > + * The value of the v4v element in a struct domain is > + * protected by the global lock L1 > + */ > +struct v4v_domain > +{ > + /* L2 */ > + rwlock_t lock; > + /* protected by L2 */ > + struct hlist_head ring_hash[V4V_HTABLE_SIZE]; > +} V4V_PACKED; > + > +void v4v_destroy(struct domain *d); > +int v4v_init(struct domain *d); > +long do_v4v_op (int cmd, > + XEN_GUEST_HANDLE (void) arg1, > + XEN_GUEST_HANDLE (void) arg2, > + XEN_GUEST_HANDLE (void) arg3, > + uint32_t arg4, > + uint32_t arg5); > + > +#endif /* __V4V_PRIVATE_H__ */
On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >> --- /dev/null >> +++ b/xen/include/public/v4v.h >> @@ -0,0 +1,240 @@ >> +/****************************************************************************** >> + * V4V >> + * >> + * Version 2 of v2v (Virtual-to-Virtual) >> + * >> + * Copyright (c) 2010, Citrix Systems >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >> + */ >> + >> +#ifndef __XEN_PUBLIC_V4V_H__ >> +#define __XEN_PUBLIC_V4V_H__ >> + >> +#include "xen.h" >> + >> +/* >> + * Structure definitions >> + */ >> + >> +#define V4V_PROTO_DGRAM 0x3c2c1db8 >> +#define V4V_PROTO_STREAM 0x70f6a8e5 >> + >> +#ifdef __i386__ > > How about ARM? > >> +# define V4V_RING_MAGIC 0xdf6977f231abd910ULL >> +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302dULL > > Is there any reason these can''t be also used for 64-bit? > >> +#else >> +# define V4V_RING_MAGIC 0xdf6977f231abd910 >> +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302d >> +#endif >> +#define V4V_DOMID_INVALID (0x7FFFU) > > Any reason not to use DOMID_INVALID instead? >DOMID_INVALID has changed in the last releases we wanted to have our definition so it will be stable accross Xen 3.* Xen 4.*.>> +#define V4V_DOMID_NONE V4V_DOMID_INVALID >> +#define V4V_DOMID_ANY V4V_DOMID_INVALID >> +#define V4V_PORT_NONE 0 >> + >> +/* >> + * struct v4v_iov >> + * { >> + * 64 bits: iov_base >> + * 64 bits: iov_len >> + * } >> + */ > > What''s the point of not defining the types here?Answer bellow.> >> + >> +/* >> + * struct v4v_addr >> + * { >> + * 32 bits: port >> + * 16 bits: domid >> + * } >> + */ >> + >> +/* >> + * v4v_ring_id >> + * { >> + * struct v4v_addr: addr >> + * 16 bits: partner >> + * } >> + */ >> + >> +/* >> + * v4v_ring >> + * { >> + * 64 bits: magic >> + * v4v_rind_id: id >> + * 32 bits: len >> + * 32 bits: rx_ptr >> + * 32 bits: tx_ptr >> + * 64 bits: padding >> + * ... : ring >> + * } >> + * >> + * id: >> + * xen only looks at this during register/unregister >> + * and will fill in id.addr.domain >> + * >> + * rx_ptr: rx pointer, modified by domain >> + * tx_ptr: tx pointer, modified by xen >> + */ >> + >> +#ifdef __i386__ >> +#define V4V_RING_DATA_MAGIC 0x4ce4d30fbc82e92aULL > > Same here as above. > >> +#else >> +#define V4V_RING_DATA_MAGIC 0x4ce4d30fbc82e92a >> +#endif >> + >> +#define V4V_RING_DATA_F_EMPTY 1U << 0 /* Ring is empty */ >> +#define V4V_RING_DATA_F_EXISTS 1U << 1 /* Ring exists */ >> +#define V4V_RING_DATA_F_PENDING 1U << 2 /* Pending interrupt exists - do not >> + rely on this field - for >> + profiling only */ >> +#define V4V_RING_DATA_F_SUFFICIENT 1U << 3 /* Sufficient space to queue >> + space_required bytes exists */ >> + >> +/* >> + * v4v_ring_data_ent >> + * { >> + * v4v_addr: ring >> + * 16 bits: flags >> + * 16 bits: padding >> + * 32 bits: space_required >> + * 32 bits: max_message_size >> + * } >> + */ >> + >> +/* >> + * v4v_ring_data >> + * { >> + * 64 bits: magic (V4V_RING_DATA_MAGIC) >> + * 32 bits: nent >> + * 32 bits: padding >> + * 256 bits: reserved >> + * ... : v4v_ring_data_ent >> + * } >> + */ >> + >> + >> +#define V4V_ROUNDUP(a) (((a) +0xf ) & ~0xf) >> +/* >> + * Messages on the ring are padded to 128 bits >> + * Len here refers to the exact length of the data not including the >> + * 128 bit header. The message uses >> + * ((len +0xf) & ~0xf) + sizeof(v4v_ring_message_header) bytes >> + */ >> + >> +/* >> + * v4v_stream_header >> + * { >> + * 32 bits: flags >> + * 32 bits: conid >> + * } >> + */ >> + >> +/* >> + * v4v_ring_message_header >> + * { >> + * 32 bits: len >> + * v4v_addr: source >> + * 32 bits: protocol >> + * ... : data >> + * } >> + */ >> + >> +/* >> + * HYPERCALLS >> + */ >> + >> +#define V4VOP_register_ring 1 >> +/* >> + * Registers a ring with Xen, if a ring with the same v4v_ring_id exists, >> + * this ring takes its place, registration will not change tx_ptr >> + * unless it is invalid >> + * >> + * do_v4v_op(V4VOP_unregister_ring, >> + * v4v_ring, XEN_GUEST_HANDLE(v4v_pfn), >> + * NULL, npage, 0) >> + */ >> + >> + >> +#define V4VOP_unregister_ring 2 >> +/* >> + * Unregister a ring. >> + * >> + * v4v_hypercall(V4VOP_send, v4v_ring, NULL, NULL, 0, 0) > > Assuming this and the earlier do_v4v_op() refer to the same > thing, please use a single name consistently. >Ack.>> + */ >> + >> +#define V4VOP_send 3 >> +/* >> + * Sends len bytes of buf to dst, giving src as the source address (xen will >> + * ignore src->domain and put your domain in the actually message), xen >> + * first looks for a ring with id.addr==dst and id.partner==sending_domain >> + * if that fails it looks for id.addr==dst and id.partner==DOMID_ANY. >> + * protocol is the 32 bit protocol number used from the message >> + * most likely V4V_PROTO_DGRAM or STREAM. If insufficient space exists >> + * it will return -EAGAIN and xen will twing the V4V_INTERRUPT when >> + * sufficient space becomes available >> + * >> + * v4v_hypercall(V4VOP_send, >> + * v4v_addr src, >> + * v4v_addr dst, >> + * void* buf, >> + * uint32_t len, >> + * uint32_t protocol) >> + */ >> + >> + >> +#define V4VOP_notify 4 >> +/* Asks xen for information about other rings in the system >> + * >> + * ent->ring is the v4v_addr_t of the ring you want information on >> + * the same matching rules are used as for V4VOP_send. >> + * >> + * ent->space_required if this field is not null xen will check >> + * that there is space in the destination ring for this many bytes >> + * of payload. If there is it will set the V4V_RING_DATA_F_SUFFICIENT >> + * and CANCEL any pending interrupt for that ent->ring, if insufficient >> + * space is available it will schedule an interrupt and the flag will >> + * not be set. >> + * >> + * The flags are set by xen when notify replies >> + * V4V_RING_DATA_F_EMPTY ring is empty >> + * V4V_RING_DATA_F_PENDING interrupt is pending - don''t rely on this >> + * V4V_RING_DATA_F_SUFFICIENT sufficient space for space_required is there >> + * V4V_RING_DATA_F_EXISTS ring exists >> + * >> + * v4v_hypercall(V4VOP_notify, >> + * XEN_GUEST_HANDLE(v4v_ring_data_ent) ent, >> + * NULL, NULL, nent, 0) >> + */ >> + >> + >> +#define V4VOP_sendv 5 >> +/* >> + * Identical to V4VOP_send except rather than buf and len it takes >> + * an array of v4v_iov and a length of the array. >> + * >> + * v4v_hypercall(V4VOP_sendv, >> + * v4v_addr src, >> + * v4v_addr dst, >> + * v4v_iov iov, >> + * uint32_t niov, >> + * uint32_t protocol) >> + */ >> + >> +#endif /* __XEN_PUBLIC_V4V_H__ */ >> --- /dev/null >> +++ b/xen/include/xen/v4v.h >> @@ -0,0 +1,187 @@ >> +/****************************************************************************** >> + * V4V >> + * >> + * Version 2 of v2v (Virtual-to-Virtual) >> + * >> + * Copyright (c) 2010, Citrix Systems >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >> + */ >> + >> +#ifndef __V4V_PRIVATE_H__ >> +#define __V4V_PRIVATE_H__ >> + >> +#include <xen/config.h> >> +#include <xen/types.h> >> +#include <xen/spinlock.h> >> +#include <xen/smp.h> >> +#include <xen/shared.h> >> +#include <xen/list.h> >> +#include <public/v4v.h> >> + >> +#define V4V_HTABLE_SIZE 32 >> + >> +#define V4V_PACKED __attribute__ ((packed)) >> + >> +/* >> + * Structures >> + */ >> + >> +typedef struct v4v_iov >> +{ >> + uint64_t iov_base; >> + uint64_t iov_len; >> +} V4V_PACKED v4v_iov_t; > > While you moved this to a private header now, I continue to > think that none of the uses of V4V_PACKED are actually > warranted (and hence these can''t serve as reason for moving > these public definitions into a private header). Use explicit > padding fields, and you ought to be fine. >Answer bellow.>> +DEFINE_XEN_GUEST_HANDLE (v4v_iov_t); >> + >> +typedef struct v4v_addr >> +{ >> + uint32_t port; >> + domid_t domain; >> +} V4V_PACKED v4v_addr_t; >> +DEFINE_XEN_GUEST_HANDLE (v4v_addr_t); >> + >> +typedef struct v4v_ring_id >> +{ >> + struct v4v_addr addr; >> + domid_t partner; >> +} V4V_PACKED v4v_ring_id_t; >> +This structure is really the one that cause trouble. domid_t is 16b and v4v_addr_t is used inside v4v_ring_t. I would like the structure to remind as close as we can from the original version as we already versions in the field. Having explicit padding will make all the structures different which will make much harder to write a driver that will support the two versions of the API. Also most all the consumer of those headers will have to rewrite the structure anyway, for instance the Linux kernel have it''s own naming convention, macros definitions which are different, etc..>> +typedef uint64_t v4v_pfn_t; >> +DEFINE_XEN_GUEST_HANDLE (v4v_pfn_t); >> + >> +typedef struct v4v_ring >> +{ >> + uint64_t magic; >> + struct v4v_ring_id id; >> + uint32_t len; >> + uint32_t rx_ptr; >> + uint32_t tx_ptr; >> + uint64_t reserved[4]; >> + uint8_t ring[0]; >> +} V4V_PACKED v4v_ring_t; >> +DEFINE_XEN_GUEST_HANDLE (v4v_ring_t); > > If moved into the public header (where they belong imo), apart > from this one none of the guest handle defines should actually be > there, as there''s no guest interface that would make use of them > (they''re used internally to v4v.c only). > > Also, conventionally there''s no space before the opening > parenthesis here. > > Jan > >> + >> +typedef struct v4v_ring_data_ent >> +{ >> + struct v4v_addr ring; >> + uint16_t flags; >> + uint16_t pad0; >> + uint32_t space_required; >> + uint32_t max_message_size; >> +} V4V_PACKED v4v_ring_data_ent_t; >> +DEFINE_XEN_GUEST_HANDLE (v4v_ring_data_ent_t); >> + >> +typedef struct v4v_ring_data >> +{ >> + uint64_t magic; >> + uint32_t nent; >> + uint32_t padding; >> + uint64_t reserved[4]; >> + v4v_ring_data_ent_t ring[0]; >> +} V4V_PACKED v4v_ring_data_t; >> +DEFINE_XEN_GUEST_HANDLE (v4v_ring_data_t); >> + >> +struct v4v_stream_header >> +{ >> + uint32_t flags; >> + uint32_t conid; >> +} V4V_PACKED; >> + >> +struct v4v_ring_message_header >> +{ >> + uint32_t len; >> + struct v4v_addr source; >> + uint32_t protocol; >> + uint8_t data[0]; >> +} V4V_PACKED; >> + >> +/* >> + * Helper functions >> + */ >> + >> + >> +static inline uint16_t >> +v4v_hash_fn (struct v4v_ring_id *id) >> +{ >> + uint16_t ret; >> + ret = (uint16_t) (id->addr.port >> 16); >> + ret ^= (uint16_t) id->addr.port; >> + ret ^= id->addr.domain; >> + ret ^= id->partner; >> + >> + ret &= (V4V_HTABLE_SIZE-1); >> + >> + return ret; >> +} >> + >> +struct v4v_pending_ent >> +{ >> + struct hlist_node node; >> + domid_t id; >> + uint32_t len; >> +} V4V_PACKED; >> + >> + >> +struct v4v_ring_info >> +{ >> + /* next node in the hash, protected by L2 */ >> + struct hlist_node node; >> + /* this ring''s id, protected by L2 */ >> + struct v4v_ring_id id; >> + /* L3 */ >> + spinlock_t lock; >> + /* cached length of the ring (from ring->len), protected by L3 */ >> + uint32_t len; >> + uint32_t npage; >> + /* cached tx pointer location, protected by L3 */ >> + uint32_t tx_ptr; >> + /* guest ring, protected by L3 */ >> + XEN_GUEST_HANDLE(v4v_ring_t) ring; >> + /* mapped ring pages protected by L3*/ >> + uint8_t **mfn_mapping; >> + /* list of mfns of guest ring */ >> + mfn_t *mfns; >> + /* list of struct v4v_pending_ent for this ring, L3 */ >> + struct hlist_head pending; >> +} V4V_PACKED; >> + >> +/* >> + * The value of the v4v element in a struct domain is >> + * protected by the global lock L1 >> + */ >> +struct v4v_domain >> +{ >> + /* L2 */ >> + rwlock_t lock; >> + /* protected by L2 */ >> + struct hlist_head ring_hash[V4V_HTABLE_SIZE]; >> +} V4V_PACKED; >> + >> +void v4v_destroy(struct domain *d); >> +int v4v_init(struct domain *d); >> +long do_v4v_op (int cmd, >> + XEN_GUEST_HANDLE (void) arg1, >> + XEN_GUEST_HANDLE (void) arg2, >> + XEN_GUEST_HANDLE (void) arg3, >> + uint32_t arg4, >> + uint32_t arg5); >> + >> +#endif /* __V4V_PRIVATE_H__ */ >Jean
On 29 June 2012 09:05, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: > > If this is really needed (which I doubt, looking at the two users > and their - respectively - sole callers), then for x86 please put > the definitions alongside the size_t ones. > > Until the need for the type is shown, this is a NAK from me. >The ssize_t can''t be replaced with a size_t because the functions that use it can return negative number and size_t is unsigned. ssize_t could be replaced by a long long which will work for all the case 32 and 64b. Jean
On 29 June 2012 09:07, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: > > This appears to be unchanged from v1, so the inconsistency > between implementation (per-vCPU vIRQ) and documentation > (global vIRQ) remains. >Yes, I will change this once I''ve switched to event channel in a new version. Jean
>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: > On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >>> +typedef struct v4v_ring_id >>> +{ >>> + struct v4v_addr addr; >>> + domid_t partner; >>> +} V4V_PACKED v4v_ring_id_t; >>> + > > This structure is really the one that cause trouble. domid_t is 16b > and v4v_addr_t is used > inside v4v_ring_t. I would like the structure to remind as close as we > can from the original version > as we already versions in the field. Having explicit padding will make > all the structures different > which will make much harder to write a driver that will support the > two versions of the API.Oh, I see, "partner" would end up on a different offset if the packed attribute was removed from v4v_addr_t. But that could still be solved by making this type a union: typedef union v4v_ring_id { struct v4v_addr addr; struct { uint32_t port; domid_t domain; domid_t partner; } full; } v4v_ring_id_t; That would guarantee binary compatibility. And you could even achieve source compatibility for gcc users by making the naming of the second structure conditional upon __GNUC__ being undefined (or adding a second instance of the same, just unnamed structure within a respective #ifdef - that would make it possible to write code that can be compiled by both gcc and non-gcc, yet existing gcc-only code would need changing).> Also most all the consumer of those headers will have to rewrite the > structure anyway, for instance > the Linux kernel have it''s own naming convention, macros definitions > which are different, etc..Such can usually be done via scripts, so having a fully defined public header is still worthwhile. Jan
>>> On 29.06.12 at 12:09, Jean Guyader <jean.guyader@gmail.com> wrote: > On 29 June 2012 09:05, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >> >> If this is really needed (which I doubt, looking at the two users >> and their - respectively - sole callers), then for x86 please put >> the definitions alongside the size_t ones. >> >> Until the need for the type is shown, this is a NAK from me. >> > > The ssize_t can''t be replaced with a size_t because the functions that use > it can return negative number and size_t is unsigned.Did you really look at the users? One stores the ssize_t value in a uint32_t (losing the signedness), and the other stores it into an int (losing the wider size).> ssize_t could be replaced by a long long which will work for all the > case 32 and 64b.A long would do as well. Jan
OK, some detailed comments below. A lot of it is just nits, but one or two serious concerns. We still have some ongoing discussion of the overall design in other threads, too... At 17:26 +0100 on 28 Jun (1340904385), Jean Guyader wrote:> +#ifdef V4V_DEBUG > +#define MY_FILE "v4v.c"Something wrong with __FILE__ ?> +#define v4v_dprintk(format, args...) \ > + do { \ > + printk("%s:%d " format, \ > + MY_FILE, __LINE__, ## args ); \ > + } while ( 1 == 0 ) > +#else > +#define v4v_dprintk(format, ... ) (void)0 > +#endif > + > > +#ifdef V4V_DEBUG > +static void > +v4v_hexdump (void *_p, int len) > +{ > + uint8_t *buf = (uint8_t *) _p; > + int i, j; > + > + for (i = 0; i < len; i += 16)Coding style is ''for ( i = 0; i < len; i += 16 )'' (and similarly throughout).> + { > + printk (KERN_ERR "%p:", &buf[i]); > + for (j = 0; j < 16; ++j) > + { > + int k = i + j; > + if (k < len)Likewise ''if ( k < len )''> + printk (" %02x", buf[k]);but ''printk(...)'' with no space before the args.> +/* > + * ring buffer > + */ > + > +/* called must have L3 */Maybe make these comments into ASSERT()s?> +static void > +v4v_ring_unmap (struct v4v_ring_info *ring_info) > +{ > + int i; > + for (i = 0; i < ring_info->npage; ++i) > + { > + if (!ring_info->mfn_mapping[i]) > + continue; > + v4v_dprintk("");I''m OK with having a lot of compiled-out debug printks, but that''s taking it a bit far. :)> +/* called must have L3 */ > +static int > +v4v_memcpy_from_guest_ring (void *_dst, struct v4v_ring_info *ring_info, > + uint32_t offset, uint32_t len)This function is only ever called to copy the ring_info out of a ring so it probably doesn''t need to be so general (handling multiple pages &c). I guess the compiler can figure out that offset is always == 0 and trim the dead code but we might as well cut it from the source too. :)> +{ > + int page = offset >> PAGE_SHIFT; > + uint8_t *src; > + uint8_t *dst = _dst; > + > + offset &= PAGE_SIZE - 1; > + > + while ((offset + len) > PAGE_SIZE) > + { > + src = v4v_ring_map_page (ring_info, page); > + > + if (!src) > + { > + return -EFAULT; > + }While I''m kvetching about style, maybe lose the braces around single-line clauses like this.> + > + v4v_dprintk("memcpy(%p,%p+%d,%d)\n", > + dst, src, offset, > + (int) (PAGE_SIZE - offset)); > + memcpy (dst, src + offset, PAGE_SIZE - offset); > + > + page++; > + len -= PAGE_SIZE - offset; > + dst += PAGE_SIZE - offset; > + offset = 0; > + } > + > + src = v4v_ring_map_page (ring_info, page); > + if (!src) > + { > + return -EFAULT; > + } > + > + v4v_dprintk("memcpy(%p,%p+%d,%d)\n", dst, src, offset, len); > + memcpy (dst, src + offset, len); > + > + return 0; > +} > + > + > +/* called must have L3 */ > +static int > +v4v_update_tx_ptr (struct v4v_ring_info *ring_info, uint32_t tx_ptr) > +{ > + uint8_t *dst = v4v_ring_map_page (ring_info, 0); > + volatile uint32_t *p = (uint32_t *)(dst + offsetof (v4v_ring_t, tx_ptr));What''s the intention of using ''volatile'' here? If it''s to make sure you get a single atomic write you should probably use the write_atomic() macro, which compiles to an explicit asm op of the right size -- GCC explicitly does _not_ guarantee that it will use atomic updates (though in practice it surely will). If you want to make sure the receiver doesn''t see the tx update before the data, I think you need to use explicit memory barriers. GCC doesn''t guarantee not to reorder other non-volatile accesses past this volatile one, and even if it did this is common code and you can''t rely on x86''s program-order semantics.> + > + if (!dst) > + return -EFAULT; > + *p = tx_ptr; > + return 0; > +} > + > +/* called must have L3 */ > +static int > +v4v_memcpy_to_guest_ring (struct v4v_ring_info *ring_info, uint32_t offset, > + void *_src, uint32_t len)This function and the _from_guest one are nearly identical, except for the actual copy and updating the source pointer. Is there any sensible way to combine them? Or would the result be too ugly?> +static int > +v4v_ringbuf_get_rx_ptr (struct domain *d, struct v4v_ring_info *ring_info, > + uint32_t * rx_ptr) > +{ > + v4v_ring_t *ringp; > + > + if ( ring_info->npage == 0 ) > + return -1; > + > + ringp = map_domain_page (mfn_x (ring_info->mfns[0])); > + > + v4v_dprintk("v4v_ringbuf_payload_space: mapped %p to %p\n", > + (void *) mfn_x (ring_info->mfns[0]), ringp); > + if ( !ringp ) > + return -1; > + > + *rx_ptr = *(volatile uint32_t *) &ringp->rx_ptr;I have the same comments about ''volatile'' as I did above.> +/*caller must have L3*/ > +static size_t > +v4v_ringbuf_insert (struct domain *d, > + struct v4v_ring_info *ring_info, > + struct v4v_ring_id *src_id, uint32_t proto, > + XEN_GUEST_HANDLE (void) buf_hnd_void, uint32_t len) > > +static ssize_t > +v4v_ringbuf_insertv (struct domain *d, > + struct v4v_ring_info *ring_info, > + struct v4v_ring_id *src_id, uint32_t proto, > + XEN_GUEST_HANDLE (v4v_iov_t) iovs, uint32_t niov, > + uint32_t len)These two functions have a lot of repeated code as well. Could insert() be coded as a wrapper around insertv()? If the guest-handle-munging is a problem, maybe we could push the same decision up the stack and only provide the vector version at the hypercall interface?> +/*caller must hold W(L2) */ > +static void v4v_ring_remove_mfns (struct v4v_ring_info *ring_info) > +{ > + int i; > + > + if ( ring_info->mfns ) > + { > + for ( i=0; i < ring_info->npage; ++i ) > + if (mfn_x(ring_info->mfns[i]) != 0) > + put_page_and_type(mfn_to_page(mfn_x(ring_info->mfns[i]))); > + xfree (ring_info->mfns); > + } > + ring_info->mfns = NULL;I think this should be freeing mfn_mapping too.> +#ifdef __i386__ > +# define V4V_RING_MAGIC 0xdf6977f231abd910ULL > +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302dULL > +#else > +# define V4V_RING_MAGIC 0xdf6977f231abd910 > +# define V4V_PFN_LIST_MAGIC 0x91dd6159045b302d > +#endifWhy the ifdef (and likewise for other magic numbers in this header)?> +#define V4V_DOMID_INVALID (0x7FFFU) > +#define V4V_DOMID_NONE V4V_DOMID_INVALID > +#define V4V_DOMID_ANY V4V_DOMID_INVALIDThe only one of these actually used in the rest of the patch is V4V_DOMID_NONE (in a context where surely _ANY would be better). Can you get rid of the others?> +#define V4V_PORT_NONE 0 > + > +/* > + * struct v4v_iov > + * { > + * 64 bits: iov_base > + * 64 bits: iov_len > + * } > + */I agree with Jan - it would be better to provide the actual definitions of these structures, even if non-GCC users might need to post-process or rewrite the header.> +#define V4V_RING_DATA_F_EMPTY 1U << 0 /* Ring is empty */ > +#define V4V_RING_DATA_F_EXISTS 1U << 1 /* Ring exists */ > +#define V4V_RING_DATA_F_PENDING 1U << 2 /* Pending interrupt exists - do not > + rely on this field - for > + profiling only */ > +#define V4V_RING_DATA_F_SUFFICIENT 1U << 3 /* Sufficient space to queue > + space_required bytes exists */ > +Please put parentheses around these flags.> +static inline uint16_t > +v4v_hash_fn (struct v4v_ring_id *id) > +{ > + uint16_t ret;Your indentation has got confused here (and for the rest of this file). Cheers, Tim.
Tim Deegan
2012-Jul-05 14:23 UTC
Re: [PATCH 5/5] v4v: Introduce basic access control to V4V
Hi, At 17:26 +0100 on 28 Jun (1340904386), Jean Guyader wrote:> +#ifdef V4V_DEBUG > +void > +v4v_viptables_print_rule (struct v4v_viptables_rule_node *rule) > +{ > + if (rule == NULL) > + { > + printk("(null)\n"); > + return; > + }The indentation doesn''t follow the coding style at all in this patch.> +int > +v4v_viptables_add (struct domain *src_d, XEN_GUEST_HANDLE(v4v_viptables_rule_t) rule, > + int32_t position) > +{ > + struct v4v_viptables_rule_node* new; > + struct list_head* tmp; > + > + /* First rule is n.1 */ > + position--; > + > + new = xmalloc (struct v4v_viptables_rule_node);What if xmalloc() fails?> + if (copy_field_from_guest (new, rule, src)) > + return -EFAULT; > + if (copy_field_from_guest (new, rule, dst)) > + return -EFAULT; > + if (copy_field_from_guest (new, rule, accept)) > + return -EFAULT;Leaking ''new'' here.> +#ifdef V4V_DEBUG > + printk(KERN_ERR "VIPTables: "); > + v4v_viptables_print_rule(new); > +#endif /* V4V_DEBUG */ > + > + tmp = &viprules; > + while (position != 0 && tmp->next != &viprules) > + { > + tmp = tmp->next; > + position--; > + } > + list_add(&new->list, tmp);Doesn''t this need to be protected by a lock? AFAICS this function is called under domain_lock(d) but modifies a global shared list, and the readers don''t take any locks. If you expect table updates to be rare then maybe write-locking the L1 lock would suffice.> + > + return 0; > +} > + > +int > +v4v_viptables_del (struct domain *src_d, XEN_GUEST_HANDLE(v4v_viptables_rule_t) rule, > + int32_t position) > +{ > + struct list_head *tmp = NULL; > + struct list_head *next = NULL; > + struct v4v_viptables_rule_node *node; > + struct v4v_viptables_rule *r; > + > + if (position != -1) > + { > + /* We want to delete the rule number <position> */ > + tmp = &viprules; > + while (position != 0 && tmp->next != &viprules) > + { > + tmp = tmp->next; > + position--; > + } > + } > + else if (!guest_handle_is_null(rule)) > + { > + /* We want to delete the rule <rule> */ > + r = xmalloc (struct v4v_viptables_rule);It''s probably OK for this to go on the stack - it''s not that big, and...> + if (copy_field_from_guest (r, rule, src)) > + return -EFAULT; > + if (copy_field_from_guest (r, rule, dst)) > + return -EFAULT; > + if (copy_field_from_guest (r, rule, accept)) > + return -EFAULT;... it would stop you leaking ''r'' here.> + list_for_each(tmp, &viprules) > + { > + node = list_entry(tmp, struct v4v_viptables_rule_node, list); > + > + if ((node->src.domain == r->src.domain) && > + (node->src.port == r->src.port) && > + (node->dst.domain == r->dst.domain) && > + (node->dst.port == r->dst.port)) > + { > + position = 0; > + break; > + } > + } > + xfree(r); > + } > + else > + { > + /* We want to flush the rules! */ > + printk(KERN_ERR "VIPTables: flushing rules\n"); > + list_for_each_safe(tmp, next, &viprules) > + { > + node = list_entry(tmp, struct v4v_viptables_rule_node, list); > + list_del(tmp); > + xfree(node); > + } > + } > + > +#ifdef V4V_DEBUG > + if (position == 0 && tmp != &viprules) > + { > + printk(KERN_ERR "VIPTables: deleting rule: "); > + node = list_entry(tmp, struct v4v_viptables_rule_node, list); > + v4v_viptables_print_rule(node); > + list_del(tmp); > + xfree(node);This list_del/xfree should definitely not be #ifdef V4V_DEBUG :)> + } > +#endif /* V4V_DEBUG */ > + > + return 0; > +} > + > +static size_t > +v4v_viptables_list (struct domain *src_d, XEN_GUEST_HANDLE(v4v_viptables_list_t) list_hnd) > +{ > + struct list_head *ptr; > + struct v4v_viptables_rule_node *node; > + struct v4v_viptables_list rules_list; > + uint32_t nbrules; > + > + memset(&rules_list, 0, sizeof (rules_list)); > + if (copy_field_from_guest (&rules_list, list_hnd, nb_rules)) > + return -EFAULT; > + > + ptr = viprules.next; > + while (rules_list.nb_rules != 0 && ptr->next != &viprules) > + { > + ptr = ptr->next; > + rules_list.start_rule--; > + } > + > + if (rules_list.nb_rules != 0) > + return -EFAULT;Surely s/nb_rules/start_rule/ in both the while() and the if() above? How much testing has this had? It seems like this function could never get as far as copying the rules out.> + > + nbrules = 0; > + while (nbrules < rules_list.nb_rules && ptr != &viprules) > + { > + node = list_entry(ptr, struct v4v_viptables_rule_node, list); > + > + rules_list.rules[rules_list.nb_rules].src = node->src; > + rules_list.rules[rules_list.nb_rules].dst = node->dst; > + rules_list.rules[rules_list.nb_rules].accept = node->accept;Aiee! Good thing it never gets that far, because (a) you''re indirecting a user-supplied distance into a stack array, and (b) the stack array has zero length.> + > + nbrules++; > + ptr = ptr->next; > + } > + > + if (copy_to_guest(list_hnd, &rules_list, 1)) > + return -EFAULT;And at the end you only copy the header back to the caller. :|> + > + return 0; > +} > + > +static size_t > +v4v_viptables_check (v4v_addr_t * src, v4v_addr_t * dst) > +{ > + struct list_head *ptr; > + struct v4v_viptables_rule_node *node; > + > + list_for_each(ptr, &viprules) > + { > + node = list_entry(ptr, struct v4v_viptables_rule_node, list); > + > + if ((node->src.domain == DOMID_INVALID || node->src.domain == src->domain) &&Having defined a new magic V4V_DOMID_* to avoid using DOMID_INVALID, you should probably use it here. (oh, and while I''m here please keep lines < 80 characters).> + (node->src.port == -1 || node->src.port == src->port) &&Likewise, why is this not V4V_PORT_NONE?> + (node->dst.domain == DOMID_INVALID || node->dst.domain == dst->domain) && > + (node->dst.port == -1 || node->dst.port == dst->port))And what about protocol? Protocol seems to have ended up as a bit of a second-class citizen in v4v; it''s defined, and indeed required, but not used for routing or for acccess control, so all traffic to a given port _on every protocol_ ends up on the same ring. This is the inverse of the TCP/IP namespace that you''re copying, where protocol demux happens before port demux. And I think it will bite someone if you ever, for example, want to send ICMP or GRE over a v4v channel.> + return !node->accept; > + } > + > + /* Defaulting to ACCEPT */ > + return 0; > +} > > /*Hypercall to do the send*/ > static size_t > @@ -1351,6 +1566,15 @@ v4v_send (struct domain *src_d, v4v_addr_t * src_addr, > return -ECONNREFUSED; > } > > + if (v4v_viptables_check(src_addr, dst_addr) != 0) > + { > + read_unlock (&v4v_lock); > + printk(KERN_ERR "V4V: VIPTables REJECTED %i:%i -> %i:%i\n", > + src_addr->domain, src_addr->port, > + dst_addr->domain, dst_addr->port);This should be at most a gdprintk to stop a badly behaved VM from spamming the console.> + return -ECONNREFUSED; > + } > + > do > { > if ( !dst_d->v4v ) > @@ -1437,6 +1661,15 @@ v4v_sendv (struct domain *src_d, v4v_addr_t * src_addr, > return -ECONNREFUSED; > } > > + if (v4v_viptables_check(src_addr, dst_addr) != 0) > + { > + read_unlock (&v4v_lock); > + printk(KERN_ERR "V4V: VIPTables REJECTED %i:%i -> %i:%i\n", > + src_addr->domain, src_addr->port, > + dst_addr->domain, dst_addr->port);Likewise.> + return -ECONNREFUSED; > + } > + > do > { > if ( !dst_d->v4v ) > @@ -1596,6 +1829,38 @@ do_v4v_op (int cmd, XEN_GUEST_HANDLE (void) arg1, > rc = v4v_notify (d, ring_data_hnd); > break; > } > + case V4VOP_viptables_add: > + { > + uint32_t position = arg4; > + XEN_GUEST_HANDLE (v4v_viptables_rule_t) rule_hnd > + guest_handle_cast (arg1, v4v_viptables_rule_t); > + rc = -EPERM; > + if (!IS_PRIV(d)) > + goto out; > + rc = v4v_viptables_add (d, rule_hnd, position); > + break; > + } > + case V4VOP_viptables_del: > + { > + uint32_t position = arg4; > + XEN_GUEST_HANDLE (v4v_viptables_rule_t) rule_hnd > + guest_handle_cast (arg1, v4v_viptables_rule_t); > + rc = -EPERM; > + if (!IS_PRIV(d)) > + goto out; > + rc = v4v_viptables_del (d, rule_hnd, position); > + break; > + } > + case V4VOP_viptables_list: > + { > + XEN_GUEST_HANDLE (v4v_viptables_list_t) rules_list_hnd > + guest_handle_cast(arg1, v4v_viptables_list_t); > + rc = -EPERM; > + if (!IS_PRIV(d)) > + goto out; > + rc = v4v_viptables_list (d, rules_list_hnd); > + break; > + } > default: > rc = -ENOSYS; > break; > diff --git a/xen/include/public/v4v.h b/xen/include/public/v4v.h > index 197770e..d8ca507 100644 > --- a/xen/include/public/v4v.h > +++ b/xen/include/public/v4v.h > @@ -213,6 +213,9 @@ > * NULL, NULL, nent, 0) > */ > > +#define V4VOP_viptables_add 6 > +#define V4VOP_viptables_del 7 > +#define V4VOP_viptables_list 8This will need documentation, and descriptions of the arguments (assuming the actual definitions don''t end up moving back into this file). And they should probably go below the V4VOP_sendv definition. Cheers, Tim.> #define V4VOP_sendv 5 > /* > diff --git a/xen/include/xen/v4v.h b/xen/include/xen/v4v.h > index 641a6a8..e5b4cb7 100644 > --- a/xen/include/xen/v4v.h > +++ b/xen/include/xen/v4v.h > @@ -103,6 +103,32 @@ struct v4v_ring_message_header > uint8_t data[0]; > } V4V_PACKED; > > +typedef struct v4v_viptables_rule > +{ > + struct v4v_addr src; > + struct v4v_addr dst; > + uint32_t accept; > +} V4V_PACKED v4v_viptables_rule_t; > + > +DEFINE_XEN_GUEST_HANDLE (v4v_viptables_rule_t); > + > +struct v4v_viptables_rule_node > +{ > + struct list_head list; > + struct v4v_addr src; > + struct v4v_addr dst; > + uint32_t accept; > +} V4V_PACKED; > + > +typedef struct v4v_viptables_list > +{ > + uint32_t start_rule; > + uint32_t nb_rules; > + struct v4v_viptables_rule rules[0]; > +} V4V_PACKED v4v_viptables_list_t; > + > +DEFINE_XEN_GUEST_HANDLE (v4v_viptables_list_t); > + > /* > * Helper functions > */
On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: >> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >>>> +typedef struct v4v_ring_id >>>> +{ >>>> + struct v4v_addr addr; >>>> + domid_t partner; >>>> +} V4V_PACKED v4v_ring_id_t; >>>> + >> >> This structure is really the one that cause trouble. domid_t is 16b >> and v4v_addr_t is used >> inside v4v_ring_t. I would like the structure to remind as close as we >> can from the original version >> as we already versions in the field. Having explicit padding will make >> all the structures different >> which will make much harder to write a driver that will support the >> two versions of the API. > > Oh, I see, "partner" would end up on a different offset if the > packed attribute was removed from v4v_addr_t. But that > could still be solved by making this type a union: > > typedef union v4v_ring_id > { > struct v4v_addr addr; > struct { > uint32_t port; > domid_t domain; > domid_t partner; > } full; > } v4v_ring_id_t; > > That would guarantee binary compatibility. And you could even > achieve source compatibility for gcc users by making the naming > of the second structure conditional upon __GNUC__ being > undefined (or adding a second instance of the same, just > unnamed structure within a respective #ifdef - that would make > it possible to write code that can be compiled by both gcc and > non-gcc, yet existing gcc-only code would need changing). > >> Also most all the consumer of those headers will have to rewrite the >> structure anyway, for instance >> the Linux kernel have it''s own naming convention, macros definitions >> which are different, etc.. > > Such can usually be done via scripts, so having a fully defined > public header is still worthwhile. >Hi, I''ve been working on this and it work for most of it apart from one case. Let''s take this structure: struct a { uint64_t a; uint32_t b; uint16_t c; uint16_t d; uint32_t e; uint32_t f; uint32_t g; uint8_t h[32]; uint8_t q[0]; }; On 32b with and without packing sizeof the struct a is 60 but on 64b the size of the struct a is 64 without packing and 60 with packing. However offset of q is 60 is all the case below. One option would be to replace in the code sizeof "struct q" with offset of "q" be I think there is probably something better that could be done. Thanks, Jean
On 18/07/12 21:09, Jean Guyader wrote:> On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: >>> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>> +typedef struct v4v_ring_id >>>>> +{ >>>>> + struct v4v_addr addr; >>>>> + domid_t partner; >>>>> +} V4V_PACKED v4v_ring_id_t; >>>>> + >>> This structure is really the one that cause trouble. domid_t is 16b >>> and v4v_addr_t is used >>> inside v4v_ring_t. I would like the structure to remind as close as we >>> can from the original version >>> as we already versions in the field. Having explicit padding will make >>> all the structures different >>> which will make much harder to write a driver that will support the >>> two versions of the API. >> Oh, I see, "partner" would end up on a different offset if the >> packed attribute was removed from v4v_addr_t. But that >> could still be solved by making this type a union: >> >> typedef union v4v_ring_id >> { >> struct v4v_addr addr; >> struct { >> uint32_t port; >> domid_t domain; >> domid_t partner; >> } full; >> } v4v_ring_id_t; >> >> That would guarantee binary compatibility. And you could even >> achieve source compatibility for gcc users by making the naming >> of the second structure conditional upon __GNUC__ being >> undefined (or adding a second instance of the same, just >> unnamed structure within a respective #ifdef - that would make >> it possible to write code that can be compiled by both gcc and >> non-gcc, yet existing gcc-only code would need changing). >> >>> Also most all the consumer of those headers will have to rewrite the >>> structure anyway, for instance >>> the Linux kernel have it''s own naming convention, macros definitions >>> which are different, etc.. >> Such can usually be done via scripts, so having a fully defined >> public header is still worthwhile. >> > Hi, > > I''ve been working on this and it work for most of it apart from one case. > Let''s take this structure: > > struct a > { > uint64_t a; > uint32_t b;uint32_t _pad0;> uint16_t c; > uint16_t d; > uint32_t e; > uint32_t f; > uint32_t g; > uint8_t h[32]; > uint8_t q[0]; > };Manually padding so the alignment is the same on 32 and 64 bit is the only way to do this in the public headers, which cant have gcc''isms for compatibility reasons with other compilers. ~Andrew> > On 32b with and without packing sizeof the struct a is 60 but on 64b the size of > the struct a is 64 without packing and 60 with packing. > However offset of q is 60 is all the case below. > > One option would be to replace in the code sizeof "struct q" with > offset of "q" be > I think there is probably something better that could be done. > > Thanks, > Jean > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel-- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On 19/07/12 10:58, Jean Guyader wrote:> On 19 July 2012 10:34, Andrew Cooper<andrew.cooper3@citrix.com> wrote: > >> On 18/07/12 21:09, Jean Guyader wrote: >> >>> On 29 June 2012 11:36, Jan Beulich<JBeulich@suse.com> wrote: >>> >>>>>>> On 29.06.12 at 12:03, Jean Guyader<jean.guyader@gmail.com> wrote: >>>>>>> >>>>> On 29 June 2012 09:33, Jan Beulich<JBeulich@suse.com> wrote: >>>>> >>>>>>>>> On 28.06.12 at 18:26, Jean Guyader<jean.guyader@citrix.com> wrote: >>>>>>>>> >>>>>>> +typedef struct v4v_ring_id >>>>>>> +{ >>>>>>> + struct v4v_addr addr; >>>>>>> + domid_t partner; >>>>>>> +} V4V_PACKED v4v_ring_id_t; >>>>>>> + >>>>>>> >>>>> This structure is really the one that cause trouble. domid_t is 16b >>>>> and v4v_addr_t is used >>>>> inside v4v_ring_t. I would like the structure to remind as close as we >>>>> can from the original version >>>>> as we already versions in the field. Having explicit padding will make >>>>> all the structures different >>>>> which will make much harder to write a driver that will support the >>>>> two versions of the API. >>>>> >>>> Oh, I see, "partner" would end up on a different offset if the >>>> packed attribute was removed from v4v_addr_t. But that >>>> could still be solved by making this type a union: >>>> >>>> typedef union v4v_ring_id >>>> { >>>> struct v4v_addr addr; >>>> struct { >>>> uint32_t port; >>>> domid_t domain; >>>> domid_t partner; >>>> } full; >>>> } v4v_ring_id_t; >>>> >>>> That would guarantee binary compatibility. And you could even >>>> achieve source compatibility for gcc users by making the naming >>>> of the second structure conditional upon __GNUC__ being >>>> undefined (or adding a second instance of the same, just >>>> unnamed structure within a respective #ifdef - that would make >>>> it possible to write code that can be compiled by both gcc and >>>> non-gcc, yet existing gcc-only code would need changing). >>>> >>>> >>>>> Also most all the consumer of those headers will have to rewrite the >>>>> structure anyway, for instance >>>>> the Linux kernel have it''s own naming convention, macros definitions >>>>> which are different, etc.. >>>>> >>>> Such can usually be done via scripts, so having a fully defined >>>> public header is still worthwhile. >>>> >>>> >>> Hi, >>> >>> I''ve been working on this and it work for most of it apart from one case. >>> Let''s take this structure: >>> >>> struct a >>> { >>> uint64_t a; >>> uint32_t b; >>> >> uint32_t _pad0; >> >>> uint16_t c; >>> uint16_t d; >>> uint32_t e; >>> uint32_t f; >>> uint32_t g; >>> uint8_t h[32]; >>> uint8_t q[0]; >>> }; >>> >> Manually padding so the alignment is the same on 32 and 64 bit is the >> only way to do this in the public headers, which cant have gcc''isms for >> compatibility reasons with other compilers. >> >> > The problem isn''t with the individual fields (they are all correctly > aligned) it is > the the overall structure size which is 64 even so offset of q is 60 > (and sizeof q > should be 0). > > I think there is no way around it. The structure I have should be > aligned on 64b anyway. > >Can you use gcc __attribute__((aligned(64))) for this? Or we try to avoid gcc-ism at all? Attilio
On 19 July 2012 10:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> On 18/07/12 21:09, Jean Guyader wrote: >> On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: >>>> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>>> +typedef struct v4v_ring_id >>>>>> +{ >>>>>> + struct v4v_addr addr; >>>>>> + domid_t partner; >>>>>> +} V4V_PACKED v4v_ring_id_t; >>>>>> + >>>> This structure is really the one that cause trouble. domid_t is 16b >>>> and v4v_addr_t is used >>>> inside v4v_ring_t. I would like the structure to remind as close as we >>>> can from the original version >>>> as we already versions in the field. Having explicit padding will make >>>> all the structures different >>>> which will make much harder to write a driver that will support the >>>> two versions of the API. >>> Oh, I see, "partner" would end up on a different offset if the >>> packed attribute was removed from v4v_addr_t. But that >>> could still be solved by making this type a union: >>> >>> typedef union v4v_ring_id >>> { >>> struct v4v_addr addr; >>> struct { >>> uint32_t port; >>> domid_t domain; >>> domid_t partner; >>> } full; >>> } v4v_ring_id_t; >>> >>> That would guarantee binary compatibility. And you could even >>> achieve source compatibility for gcc users by making the naming >>> of the second structure conditional upon __GNUC__ being >>> undefined (or adding a second instance of the same, just >>> unnamed structure within a respective #ifdef - that would make >>> it possible to write code that can be compiled by both gcc and >>> non-gcc, yet existing gcc-only code would need changing). >>> >>>> Also most all the consumer of those headers will have to rewrite the >>>> structure anyway, for instance >>>> the Linux kernel have it''s own naming convention, macros definitions >>>> which are different, etc.. >>> Such can usually be done via scripts, so having a fully defined >>> public header is still worthwhile. >>> >> Hi, >> >> I''ve been working on this and it work for most of it apart from one case. >> Let''s take this structure: >> >> struct a >> { >> uint64_t a; >> uint32_t b; > uint32_t _pad0; >> uint16_t c; >> uint16_t d; >> uint32_t e; >> uint32_t f; >> uint32_t g; >> uint8_t h[32]; >> uint8_t q[0]; >> }; > > Manually padding so the alignment is the same on 32 and 64 bit is the > only way to do this in the public headers, which cant have gcc''isms for > compatibility reasons with other compilers. >The problem isn''t with the individual fields (they are all correctly aligned) it is the the overall structure size which is 64 even so offset of q is 60 (and sizeof q should be 0). I think there is no way around it. The structure I have should be aligned on 64b anyway. Thanks, Jean
On 19/07/12 11:06, Jean Guyader wrote:> On 19 July 2012 10:54, Attilio Rao<attilio.rao@citrix.com> wrote: > >> On 19/07/12 10:58, Jean Guyader wrote: >> >>> On 19 July 2012 10:34, Andrew Cooper<andrew.cooper3@citrix.com> wrote: >>> >>> >>>> On 18/07/12 21:09, Jean Guyader wrote: >>>> >>>> >>>>> On 29 June 2012 11:36, Jan Beulich<JBeulich@suse.com> wrote: >>>>> >>>>> >>>>>>>>> On 29.06.12 at 12:03, Jean Guyader<jean.guyader@gmail.com> wrote: >>>>>>>>> >>>>>>>>> >>>>>>> On 29 June 2012 09:33, Jan Beulich<JBeulich@suse.com> wrote: >>>>>>> >>>>>>> >>>>>>>>>>> On 28.06.12 at 18:26, Jean Guyader<jean.guyader@citrix.com> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> +typedef struct v4v_ring_id >>>>>>>>> +{ >>>>>>>>> + struct v4v_addr addr; >>>>>>>>> + domid_t partner; >>>>>>>>> +} V4V_PACKED v4v_ring_id_t; >>>>>>>>> + >>>>>>>>> >>>>>>>>> >>>>>>> This structure is really the one that cause trouble. domid_t is 16b >>>>>>> and v4v_addr_t is used >>>>>>> inside v4v_ring_t. I would like the structure to remind as close as we >>>>>>> can from the original version >>>>>>> as we already versions in the field. Having explicit padding will make >>>>>>> all the structures different >>>>>>> which will make much harder to write a driver that will support the >>>>>>> two versions of the API. >>>>>>> >>>>>>> >>>>>> Oh, I see, "partner" would end up on a different offset if the >>>>>> packed attribute was removed from v4v_addr_t. But that >>>>>> could still be solved by making this type a union: >>>>>> >>>>>> typedef union v4v_ring_id >>>>>> { >>>>>> struct v4v_addr addr; >>>>>> struct { >>>>>> uint32_t port; >>>>>> domid_t domain; >>>>>> domid_t partner; >>>>>> } full; >>>>>> } v4v_ring_id_t; >>>>>> >>>>>> That would guarantee binary compatibility. And you could even >>>>>> achieve source compatibility for gcc users by making the naming >>>>>> of the second structure conditional upon __GNUC__ being >>>>>> undefined (or adding a second instance of the same, just >>>>>> unnamed structure within a respective #ifdef - that would make >>>>>> it possible to write code that can be compiled by both gcc and >>>>>> non-gcc, yet existing gcc-only code would need changing). >>>>>> >>>>>> >>>>>> >>>>>>> Also most all the consumer of those headers will have to rewrite the >>>>>>> structure anyway, for instance >>>>>>> the Linux kernel have it''s own naming convention, macros definitions >>>>>>> which are different, etc.. >>>>>>> >>>>>>> >>>>>> Such can usually be done via scripts, so having a fully defined >>>>>> public header is still worthwhile. >>>>>> >>>>>> >>>>>> >>>>> Hi, >>>>> >>>>> I''ve been working on this and it work for most of it apart from one >>>>> case. >>>>> Let''s take this structure: >>>>> >>>>> struct a >>>>> { >>>>> uint64_t a; >>>>> uint32_t b; >>>>> >>>>> >>>> uint32_t _pad0; >>>> >>>> >>>>> uint16_t c; >>>>> uint16_t d; >>>>> uint32_t e; >>>>> uint32_t f; >>>>> uint32_t g; >>>>> uint8_t h[32]; >>>>> uint8_t q[0]; >>>>> }; >>>>> >>>>> >>>> Manually padding so the alignment is the same on 32 and 64 bit is the >>>> only way to do this in the public headers, which cant have gcc''isms for >>>> compatibility reasons with other compilers. >>>> >>>> >>>> >>> The problem isn''t with the individual fields (they are all correctly >>> aligned) it is >>> the the overall structure size which is 64 even so offset of q is 60 >>> (and sizeof q >>> should be 0). >>> >>> I think there is no way around it. The structure I have should be >>> aligned on 64b anyway. >>> >>> >>> >> >> Can you use gcc __attribute__((aligned(64))) for this? Or we try to avoid >> gcc-ism at all? >> >> > I''m trying to avoid any compiler specific statement. I''ll added some > manual padding > to have it explicitly aligned to 64b. > >Ideally though you can have an intermediate header where you workout compiler specific code (in particular if you are almost sure your code will always be likely compiled with gcc). So, you can have an header which does the following: #ifdef __GNUC__ #define _compiler_align(x) __attribute__((__aligned__(x))) #elif __INTEL_COMPILER #define _compiler_align(x) foo #else #define _compiler_align(x) #endif Probabily for something like that you want to use some header that is known to be included in a lot of files. Attilio
On 19 July 2012 10:54, Attilio Rao <attilio.rao@citrix.com> wrote:> On 19/07/12 10:58, Jean Guyader wrote: >> >> On 19 July 2012 10:34, Andrew Cooper<andrew.cooper3@citrix.com> wrote: >> >>> >>> On 18/07/12 21:09, Jean Guyader wrote: >>> >>>> >>>> On 29 June 2012 11:36, Jan Beulich<JBeulich@suse.com> wrote: >>>> >>>>>>>> >>>>>>>> On 29.06.12 at 12:03, Jean Guyader<jean.guyader@gmail.com> wrote: >>>>>>>> >>>>>> >>>>>> On 29 June 2012 09:33, Jan Beulich<JBeulich@suse.com> wrote: >>>>>> >>>>>>>>>> >>>>>>>>>> On 28.06.12 at 18:26, Jean Guyader<jean.guyader@citrix.com> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>> >>>>>>>> +typedef struct v4v_ring_id >>>>>>>> +{ >>>>>>>> + struct v4v_addr addr; >>>>>>>> + domid_t partner; >>>>>>>> +} V4V_PACKED v4v_ring_id_t; >>>>>>>> + >>>>>>>> >>>>>> >>>>>> This structure is really the one that cause trouble. domid_t is 16b >>>>>> and v4v_addr_t is used >>>>>> inside v4v_ring_t. I would like the structure to remind as close as we >>>>>> can from the original version >>>>>> as we already versions in the field. Having explicit padding will make >>>>>> all the structures different >>>>>> which will make much harder to write a driver that will support the >>>>>> two versions of the API. >>>>>> >>>>> >>>>> Oh, I see, "partner" would end up on a different offset if the >>>>> packed attribute was removed from v4v_addr_t. But that >>>>> could still be solved by making this type a union: >>>>> >>>>> typedef union v4v_ring_id >>>>> { >>>>> struct v4v_addr addr; >>>>> struct { >>>>> uint32_t port; >>>>> domid_t domain; >>>>> domid_t partner; >>>>> } full; >>>>> } v4v_ring_id_t; >>>>> >>>>> That would guarantee binary compatibility. And you could even >>>>> achieve source compatibility for gcc users by making the naming >>>>> of the second structure conditional upon __GNUC__ being >>>>> undefined (or adding a second instance of the same, just >>>>> unnamed structure within a respective #ifdef - that would make >>>>> it possible to write code that can be compiled by both gcc and >>>>> non-gcc, yet existing gcc-only code would need changing). >>>>> >>>>> >>>>>> >>>>>> Also most all the consumer of those headers will have to rewrite the >>>>>> structure anyway, for instance >>>>>> the Linux kernel have it''s own naming convention, macros definitions >>>>>> which are different, etc.. >>>>>> >>>>> >>>>> Such can usually be done via scripts, so having a fully defined >>>>> public header is still worthwhile. >>>>> >>>>> >>>> >>>> Hi, >>>> >>>> I''ve been working on this and it work for most of it apart from one >>>> case. >>>> Let''s take this structure: >>>> >>>> struct a >>>> { >>>> uint64_t a; >>>> uint32_t b; >>>> >>> >>> uint32_t _pad0; >>> >>>> >>>> uint16_t c; >>>> uint16_t d; >>>> uint32_t e; >>>> uint32_t f; >>>> uint32_t g; >>>> uint8_t h[32]; >>>> uint8_t q[0]; >>>> }; >>>> >>> >>> Manually padding so the alignment is the same on 32 and 64 bit is the >>> only way to do this in the public headers, which cant have gcc''isms for >>> compatibility reasons with other compilers. >>> >>> >> >> The problem isn''t with the individual fields (they are all correctly >> aligned) it is >> the the overall structure size which is 64 even so offset of q is 60 >> (and sizeof q >> should be 0). >> >> I think there is no way around it. The structure I have should be >> aligned on 64b anyway. >> >> > > > Can you use gcc __attribute__((aligned(64))) for this? Or we try to avoid > gcc-ism at all? >I''m trying to avoid any compiler specific statement. I''ll added some manual padding to have it explicitly aligned to 64b. Jean
> Ideally though you can have an intermediate header where you workout > compiler specific code (in particular if you are almost sure your code > will always be likely compiled with gcc). > So, you can have an header which does the following:[...]> Probabily for something like that you want to use some header that is > known to be included in a lot of files.For better or worse the policy for xen/include/public is that remains pure ANSI C. So no magic per-compiler headers etc. Ian.
On 19/07/12 10:58, Jean Guyader wrote:> On 19 July 2012 10:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 18/07/12 21:09, Jean Guyader wrote: >>> On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: >>>>> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>>>> +typedef struct v4v_ring_id >>>>>>> +{ >>>>>>> + struct v4v_addr addr; >>>>>>> + domid_t partner; >>>>>>> +} V4V_PACKED v4v_ring_id_t; >>>>>>> + >>>>> This structure is really the one that cause trouble. domid_t is 16b >>>>> and v4v_addr_t is used >>>>> inside v4v_ring_t. I would like the structure to remind as close as we >>>>> can from the original version >>>>> as we already versions in the field. Having explicit padding will make >>>>> all the structures different >>>>> which will make much harder to write a driver that will support the >>>>> two versions of the API. >>>> Oh, I see, "partner" would end up on a different offset if the >>>> packed attribute was removed from v4v_addr_t. But that >>>> could still be solved by making this type a union: >>>> >>>> typedef union v4v_ring_id >>>> { >>>> struct v4v_addr addr; >>>> struct { >>>> uint32_t port; >>>> domid_t domain; >>>> domid_t partner; >>>> } full; >>>> } v4v_ring_id_t; >>>> >>>> That would guarantee binary compatibility. And you could even >>>> achieve source compatibility for gcc users by making the naming >>>> of the second structure conditional upon __GNUC__ being >>>> undefined (or adding a second instance of the same, just >>>> unnamed structure within a respective #ifdef - that would make >>>> it possible to write code that can be compiled by both gcc and >>>> non-gcc, yet existing gcc-only code would need changing). >>>> >>>>> Also most all the consumer of those headers will have to rewrite the >>>>> structure anyway, for instance >>>>> the Linux kernel have it''s own naming convention, macros definitions >>>>> which are different, etc.. >>>> Such can usually be done via scripts, so having a fully defined >>>> public header is still worthwhile. >>>> >>> Hi, >>> >>> I''ve been working on this and it work for most of it apart from one case. >>> Let''s take this structure: >>> >>> struct a >>> { >>> uint64_t a; >>> uint32_t b; >> uint32_t _pad0; >>> uint16_t c; >>> uint16_t d; >>> uint32_t e; >>> uint32_t f; >>> uint32_t g; >>> uint8_t h[32]; >>> uint8_t q[0]; >>> }; >> Manually padding so the alignment is the same on 32 and 64 bit is the >> only way to do this in the public headers, which cant have gcc''isms for >> compatibility reasons with other compilers. >> > The problem isn''t with the individual fields (they are all correctly > aligned) it is > the the overall structure size which is 64 even so offset of q is 60 > (and sizeof q > should be 0). > > I think there is no way around it. The structure I have should be > aligned on 64b anyway. > > Thanks, > JeanAh yes - silly me. I understand your problem now struct b { uint64_t a; uint32_t b; uint16_t c; uint16_t d; uint32_t e; uint32_t f; uint32_t g; uint8_t h[32]; union { uint8_t q[0]; uint32_t _pad; } u; }; This works for me on gcc and gives identical sizeof and offsetof results on both 32 and 64bit. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On Thu, 19 Jul 2012, Andrew Cooper wrote:> On 19/07/12 10:58, Jean Guyader wrote: > > On 19 July 2012 10:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> On 18/07/12 21:09, Jean Guyader wrote: > >>> On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote: > >>>>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: > >>>>> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: > >>>>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: > >>>>>>> +typedef struct v4v_ring_id > >>>>>>> +{ > >>>>>>> + struct v4v_addr addr; > >>>>>>> + domid_t partner; > >>>>>>> +} V4V_PACKED v4v_ring_id_t; > >>>>>>> + > >>>>> This structure is really the one that cause trouble. domid_t is 16b > >>>>> and v4v_addr_t is used > >>>>> inside v4v_ring_t. I would like the structure to remind as close as we > >>>>> can from the original version > >>>>> as we already versions in the field. Having explicit padding will make > >>>>> all the structures different > >>>>> which will make much harder to write a driver that will support the > >>>>> two versions of the API. > >>>> Oh, I see, "partner" would end up on a different offset if the > >>>> packed attribute was removed from v4v_addr_t. But that > >>>> could still be solved by making this type a union: > >>>> > >>>> typedef union v4v_ring_id > >>>> { > >>>> struct v4v_addr addr; > >>>> struct { > >>>> uint32_t port; > >>>> domid_t domain; > >>>> domid_t partner; > >>>> } full; > >>>> } v4v_ring_id_t; > >>>> > >>>> That would guarantee binary compatibility. And you could even > >>>> achieve source compatibility for gcc users by making the naming > >>>> of the second structure conditional upon __GNUC__ being > >>>> undefined (or adding a second instance of the same, just > >>>> unnamed structure within a respective #ifdef - that would make > >>>> it possible to write code that can be compiled by both gcc and > >>>> non-gcc, yet existing gcc-only code would need changing). > >>>> > >>>>> Also most all the consumer of those headers will have to rewrite the > >>>>> structure anyway, for instance > >>>>> the Linux kernel have it''s own naming convention, macros definitions > >>>>> which are different, etc.. > >>>> Such can usually be done via scripts, so having a fully defined > >>>> public header is still worthwhile. > >>>> > >>> Hi, > >>> > >>> I''ve been working on this and it work for most of it apart from one case. > >>> Let''s take this structure: > >>> > >>> struct a > >>> { > >>> uint64_t a; > >>> uint32_t b; > >> uint32_t _pad0; > >>> uint16_t c; > >>> uint16_t d; > >>> uint32_t e; > >>> uint32_t f; > >>> uint32_t g; > >>> uint8_t h[32]; > >>> uint8_t q[0]; > >>> }; > >> Manually padding so the alignment is the same on 32 and 64 bit is the > >> only way to do this in the public headers, which cant have gcc''isms for > >> compatibility reasons with other compilers. > >> > > The problem isn''t with the individual fields (they are all correctly > > aligned) it is > > the the overall structure size which is 64 even so offset of q is 60 > > (and sizeof q > > should be 0). > > > > I think there is no way around it. The structure I have should be > > aligned on 64b anyway. > > > > Thanks, > > Jean > > Ah yes - silly me. I understand your problem now > > struct b > { > uint64_t a; > uint32_t b; > uint16_t c; > uint16_t d; > uint32_t e; > uint32_t f; > uint32_t g; > uint8_t h[32]; > union { uint8_t q[0]; uint32_t _pad; } u; > }; > > This works for me on gcc and gives identical sizeof and offsetof results > on both 32 and 64bit.Yes, but then the access to q becomes b.u.q unless we use anonymous unions that are only available in C11.
On 19/07/12 12:33, Stefano Stabellini wrote:>>> The problem isn''t with the individual fields (they are all correctly >>> aligned) it is >>> the the overall structure size which is 64 even so offset of q is 60 >>> (and sizeof q >>> should be 0). >>> >>> I think there is no way around it. The structure I have should be >>> aligned on 64b anyway. >>> >>> Thanks, >>> Jean >> Ah yes - silly me. I understand your problem now >> >> struct b >> { >> uint64_t a; >> uint32_t b; >> uint16_t c; >> uint16_t d; >> uint32_t e; >> uint32_t f; >> uint32_t g; >> uint8_t h[32]; >> union { uint8_t q[0]; uint32_t _pad; } u; >> }; >> >> This works for me on gcc and gives identical sizeof and offsetof results >> on both 32 and 64bit. > Yes, but then the access to q becomes b.u.q unless we use anonymous > unions that are only available in C11.Combine it with the Linux style of #define q u.q although with rather more sensible names than q and u, or just use a macro for all accesses to q from b. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
On 19 July 2012 11:42, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> > On 19/07/12 10:58, Jean Guyader wrote: >> On 19 July 2012 10:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> On 18/07/12 21:09, Jean Guyader wrote: >>>> On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: >>>>>> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>>>>> +typedef struct v4v_ring_id >>>>>>>> +{ >>>>>>>> + struct v4v_addr addr; >>>>>>>> + domid_t partner; >>>>>>>> +} V4V_PACKED v4v_ring_id_t; >>>>>>>> + >>>>>> This structure is really the one that cause trouble. domid_t is 16b >>>>>> and v4v_addr_t is used >>>>>> inside v4v_ring_t. I would like the structure to remind as close as we >>>>>> can from the original version >>>>>> as we already versions in the field. Having explicit padding will make >>>>>> all the structures different >>>>>> which will make much harder to write a driver that will support the >>>>>> two versions of the API. >>>>> Oh, I see, "partner" would end up on a different offset if the >>>>> packed attribute was removed from v4v_addr_t. But that >>>>> could still be solved by making this type a union: >>>>> >>>>> typedef union v4v_ring_id >>>>> { >>>>> struct v4v_addr addr; >>>>> struct { >>>>> uint32_t port; >>>>> domid_t domain; >>>>> domid_t partner; >>>>> } full; >>>>> } v4v_ring_id_t; >>>>> >>>>> That would guarantee binary compatibility. And you could even >>>>> achieve source compatibility for gcc users by making the naming >>>>> of the second structure conditional upon __GNUC__ being >>>>> undefined (or adding a second instance of the same, just >>>>> unnamed structure within a respective #ifdef - that would make >>>>> it possible to write code that can be compiled by both gcc and >>>>> non-gcc, yet existing gcc-only code would need changing). >>>>> >>>>>> Also most all the consumer of those headers will have to rewrite the >>>>>> structure anyway, for instance >>>>>> the Linux kernel have it''s own naming convention, macros definitions >>>>>> which are different, etc.. >>>>> Such can usually be done via scripts, so having a fully defined >>>>> public header is still worthwhile. >>>>> >>>> Hi, >>>> >>>> I''ve been working on this and it work for most of it apart from one case. >>>> Let''s take this structure: >>>> >>>> struct a >>>> { >>>> uint64_t a; >>>> uint32_t b; >>> uint32_t _pad0; >>>> uint16_t c; >>>> uint16_t d; >>>> uint32_t e; >>>> uint32_t f; >>>> uint32_t g; >>>> uint8_t h[32]; >>>> uint8_t q[0]; >>>> }; >>> Manually padding so the alignment is the same on 32 and 64 bit is the >>> only way to do this in the public headers, which cant have gcc''isms for >>> compatibility reasons with other compilers. >>> >> The problem isn''t with the individual fields (they are all correctly >> aligned) it is >> the the overall structure size which is 64 even so offset of q is 60 >> (and sizeof q >> should be 0). >> >> I think there is no way around it. The structure I have should be >> aligned on 64b anyway. >> >> Thanks, >> Jean > > Ah yes - silly me. I understand your problem now > > struct b > { > uint64_t a; > uint32_t b; > uint16_t c; > uint16_t d; > uint32_t e; > uint32_t f; > uint32_t g; > uint8_t h[32]; > union { uint8_t q[0]; uint32_t _pad; } u; > }; >The code assumes that sizeof q is offset of q so that won''t really work. I''ll add some padding in h and I''ll loose the binary compatibility I think I have no choice. Thanks for you help though. Jean
>>> On 18.07.12 at 22:09, Jean Guyader <jean.guyader@gmail.com> wrote: > On 29 June 2012 11:36, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 29.06.12 at 12:03, Jean Guyader <jean.guyader@gmail.com> wrote: >>> On 29 June 2012 09:33, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@citrix.com> wrote: >>>>> +typedef struct v4v_ring_id >>>>> +{ >>>>> + struct v4v_addr addr; >>>>> + domid_t partner; >>>>> +} V4V_PACKED v4v_ring_id_t; >>>>> + >>> >>> This structure is really the one that cause trouble. domid_t is 16b >>> and v4v_addr_t is used >>> inside v4v_ring_t. I would like the structure to remind as close as we >>> can from the original version >>> as we already versions in the field. Having explicit padding will make >>> all the structures different >>> which will make much harder to write a driver that will support the >>> two versions of the API. >> >> Oh, I see, "partner" would end up on a different offset if the >> packed attribute was removed from v4v_addr_t. But that >> could still be solved by making this type a union: >> >> typedef union v4v_ring_id >> { >> struct v4v_addr addr; >> struct { >> uint32_t port; >> domid_t domain; >> domid_t partner; >> } full; >> } v4v_ring_id_t; >> >> That would guarantee binary compatibility. And you could even >> achieve source compatibility for gcc users by making the naming >> of the second structure conditional upon __GNUC__ being >> undefined (or adding a second instance of the same, just >> unnamed structure within a respective #ifdef - that would make >> it possible to write code that can be compiled by both gcc and >> non-gcc, yet existing gcc-only code would need changing). >> >>> Also most all the consumer of those headers will have to rewrite the >>> structure anyway, for instance >>> the Linux kernel have it''s own naming convention, macros definitions >>> which are different, etc.. >> >> Such can usually be done via scripts, so having a fully defined >> public header is still worthwhile. >> > > Hi, > > I''ve been working on this and it work for most of it apart from one case. > Let''s take this structure: > > struct a > { > uint64_t a; > uint32_t b; > uint16_t c; > uint16_t d; > uint32_t e; > uint32_t f; > uint32_t g; > uint8_t h[32]; > uint8_t q[0]; > }; > > On 32b with and without packing sizeof the struct a is 60 but on 64b the > size of > the struct a is 64 without packing and 60 with packing. > However offset of q is 60 is all the case below. > > One option would be to replace in the code sizeof "struct q" with > offset of "q" be > I think there is probably something better that could be done.Not sure what you''re trying to tell me here, but I''d like to note that sizeof() is mostly meaningless on a structure with a trailing [0] member anyway, so all you''re after is that all offsetof()-s are identical, which you say they are. Jan