Re: [Xen-devel] [PATCH 8/8] evtchn: add FIFO-based event channel hypercalls and port ops

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.08.13 at 20:08, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> +static void evtchn_fifo_bind_to_vcpu(struct domain *d, struct evtchn *evtchn,
> +                                     struct vcpu *v)
> +{
> +    unsigned priority;
> +
> +    /* Keep the same priority if possible, otherwise use the
> +       default. */
> +    if ( evtchn->queue )
> +        priority = evtchn->queue->priority;
> +    else
> +        priority = EVTCHN_FIFO_PRIORITY_DEFAULT;
> +
> +    evtchn->queue = &v->evtchn_fifo->queue[priority];

Why not simply

    if ( !evtchn->queue )
        evtchn->queue = &v->evtchn_fifo->queue[EVTCHN_FIFO_PRIORITY_DEFAULT];

?

> +static int setup_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)

No fixed-size integer types please when they're not really needed
(here they could be unsigned long and unsigned int respectively).

> +static int setup_event_array(struct domain *d)
> +{
> +    if ( d->evtchn_fifo )
> +        return 0;
> +
> +    d->evtchn_fifo = xzalloc(struct evtchn_fifo_domain);
> +    if ( d->evtchn_fifo == NULL )
> +        return -ENOMEM;
> +
> +    d->evtchn_fifo->num_evtchns = 0;

So you just used xzalloc() and then set this field to zero again?

> +int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
> +{
> +    struct domain *d = current->domain;
> +    uint32_t vcpu_id;
> +    uint64_t gfn;
> +    uint32_t offset;
> +    struct vcpu *v;
> +    int rc;
> +
> +    init_control->link_bits = EVTCHN_FIFO_LINK_BITS;
> +
> +    vcpu_id = init_control->vcpu;
> +    gfn     = init_control->control_mfn;
> +    offset  = init_control->offset;
> +
> +    if ( (vcpu_id >= d->max_vcpus) || (d->vcpu[vcpu_id] == NULL) )
> +        return -ENOENT;
> +    v = d->vcpu[vcpu_id];
> +
> +    /* Must not cross page boundary. */
> +    if ( offset > (PAGE_SIZE - sizeof(evtchn_fifo_control_block_t)) )
> +        return -EINVAL;
> +
> +    /* Must be 8-bytes aligned. */
> +    if ( offset & (8 - 1) )

This would better use __alignof() to document where the requirement
stems from.

> +int evtchn_fifo_expand_array(struct evtchn_expand_array *expand_array)
> +{
> +    struct domain *d = current->domain;
> +    int rc;
> +
> +    spin_lock(&d->event_lock);
> +    rc = add_page_to_event_array(d, expand_array->array_mfn);

Neither here, not in its caller, nor in the called function seems to
be any protection against this being called without first having
used EVTCHNOP_init_control.

Same for evtchn_fifo_set_priority().

Further all but evtchn_fifo_init_control() appear to have no outputs,
and hence - for documentation purposes - constifying the parameters
of the respective handler functions would be appreciated.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -96,6 +96,7 @@ struct evtchn
>          } pirq;        /* state == ECS_PIRQ */
>          u16 virq;      /* state == ECS_VIRQ */
>      } u;
> +    struct evtchn_fifo_queue *queue;

I wonder,whether you need to store the (8-byte) queue pointer
here at all: Why can't you just store the (1-byte) priority instead?
Can't the queue always be determined via notify_vcpu_id?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel