Re: [PATCH 12/13] xen/events: Add the hypervisor interface for the FIFO-based event channels

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Sep 13, 2013 at 06:00:00PM +0100, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> Add the hypercall sub-ops and the structures for the shared data used
> in the FIFO-based event channel ABI.

And you should include an URL to the design document. Or include the
text version of it (if possible) in this description.

Perhaps also say in which version of the hypervisor this functionality
became available.

> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  include/xen/interface/event_channel.h |   67 +++++++++++++++++++++++++++++++++
>  1 files changed, 67 insertions(+), 0 deletions(-)
> 
> diff --git a/include/xen/interface/event_channel.h b/include/xen/interface/event_channel.h
> index f494292..f54ad0a 100644
> --- a/include/xen/interface/event_channel.h
> +++ b/include/xen/interface/event_channel.h
> @@ -190,6 +190,48 @@ struct evtchn_reset {
>  };
>  typedef struct evtchn_reset evtchn_reset_t;
>  
> +/*
> + * EVTCHNOP_init_control: initialize the control block for the FIFO ABI.
> + */
> +#define EVTCHNOP_init_control    11
> +struct evtchn_init_control {
> +	/* IN parameters. */
> +	uint64_t control_mfn;

Odd. The design document mentions 'control_gfn' ?

Should it be 'gmfn' ?

> +	uint32_t offset;
> +	uint32_t vcpu;
> +	/* OUT parameters. */
> +	uint8_t link_bits;

No __pad[7] ?

> +};
> +
> +/*
> + * EVTCHNOP_expand_array: add an additional page to the event array.
> + */
> +#define EVTCHNOP_expand_array    12
> +struct evtchn_expand_array {
> +	/* IN parameters. */
> +	uint64_t array_mfn;

Design says 'array_gfn' ?

> +};
> +
> +/*
> + * EVTCHNOP_set_priority: set the priority for an event channel.
> + */
> +#define EVTCHNOP_set_priority    13
> +struct evtchn_set_priority {
> +	/* IN parameters. */
> +	uint32_t port;
> +	uint32_t priority;
> +};
> +
> +/*
> + * EVTCHNOP_set_limit: set the maximum event channel port that may be bound.
> + */
> +#define EVTCHNOP_set_limit       14
> +struct evtchn_set_limit {
> +	/* IN parameters. */
> +	uint32_t domid;
> +	uint32_t max_port;
> +};
> +
>  struct evtchn_op {
>  	uint32_t cmd; /* EVTCHNOP_* */
>  	union {
> @@ -207,4 +249,29 @@ struct evtchn_op {
>  };
>  DEFINE_GUEST_HANDLE_STRUCT(evtchn_op);
>  
> +/*
> + * FIFO ABI
> + */
> +
> +/* Events may have priorities from 0 (highest) to 15 (lowest). */
> +#define EVTCHN_FIFO_PRIORITY_MIN     15
> +#define EVTCHN_FIFO_PRIORITY_DEFAULT 7
> +
> +#define EVTCHN_FIFO_MAX_QUEUES (EVTCHN_FIFO_PRIORITY_MIN + 1)
> +
> +typedef uint32_t event_word_t;
> +
> +#define EVTCHN_FIFO_PENDING 31
> +#define EVTCHN_FIFO_MASKED  30
> +#define EVTCHN_FIFO_LINKED  29
> +
> +#define EVTCHN_FIFO_LINK_BITS 17
> +#define EVTCHN_FIFO_LINK_MASK ((1 << EVTCHN_FIFO_LINK_BITS) - 1)
> +
> +struct evtchn_fifo_control_block {
> +	uint32_t     ready;
> +	uint32_t     _rsvd1;


The design document looks to have 'READY' be 2 bytes? Octet 0 and 1?
(figure 2).

Which one is correct?


> +	event_word_t head[EVTCHN_FIFO_MAX_QUEUES];
> +};
> +
>  #endif /* __XEN_PUBLIC_EVENT_CHANNEL_H__ */
> -- 
> 1.7.2.5
>