|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/5] xen: Add V4V implementation
On 29 June 2012 09:33, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 28.06.12 at 18:26, Jean Guyader <jean.guyader@xxxxxxxxxx> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |