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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, 2013-03-19 at 21:00 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> Add the implementation for the FIFO-based event channel ABI.  The new
> hypercall sub-ops (EVTCHNOP_init_control, EVTCHNOP_expand_array,
> EVTCHNOP_set_priority) and the required evtchn_ops (set_pending,
> unmask, etc.).
> 
> This current implementation has three main limitations:
> 
> - EVTCHNOP_set_limit is not yet implemented so any guest will be able
>   to use up to 2^17 (requiring 128 global mapping pages for a fully
>   populated event array).
> 

How can a guest know its limit? What if the limit change when a guest is
running?

> - The control block frames are not required to be shared with the
>   vcpu_info structure.  This requires an additional global mapping
>   page per-VCPU.  This does makes the guest implementation cleaner
>   though so perhaps we do not need to fix this?
> 
> - The allocation of the struct evtchns requires > PAGE_SIZE
>   allocations.  I plan to take Wei's "evtchn: alter internal object
>   handling scheme" patch for this.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
...
> +
> +#include <public/event_channel.h>
> +
> +static int map_guest_page(struct domain *d, uint64_t gfn,
                                               ^^^^^^^^
                                               xen_pfn_t?

> +                          struct page_info **page, void **virt)
> +{

You can use vmap() and vunmap() for all mapping / unmapping routines to
simplify your code. In that case you can also eliminate the "page" field
in your control structures.

> +    struct page_info *p;
> +
> +    p = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> +    if ( !p )
> +        return -EINVAL;
> +
...
> +static int setup_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
> +{
> +    struct domain *d = v->domain;
> +    struct evtchn_fifo_vcpu *efv;
> +    struct page_info *page;
> +    void *virt;
> +    int i;
> +    int rc;
> +
> +    if ( v->evtchn_fifo )
> +        return -EINVAL;

How about using EBUSY?

> +
> +    efv = xzalloc(struct evtchn_fifo_vcpu);
> +    if ( efv == NULL )
...
> +
> +/*
> + * Some ports may already be bound, bind them to the correct VCPU so
> + * they have a valid queue.
> + *
> + * Note: any events that are currently pending will not be resent and
> + * will be lost.
> + */

Does this need to be fixed? Can you scan the bitmap and pick up pending
events if there are any?

This function is called when setting up first control block. So,
consider:

(a) guest can only setup control block at start of day, this function
will not be necessary.

(b) guest is allowed to switch ABI when there are some pending events,
IMO re-injection is necessary.

> +static void rebind_all_ports(struct domain *d)
> +{
> +    int port;
> +
> +    for ( port = 1; port < d->max_evtchns; port++ )
> +    {
> +        struct evtchn *evtchn;
> +
> +        if ( !port_is_valid(d, port) )
> +            break;
> +

"continue", not "break".

> +        evtchn = evtchn_from_port(d, port);
> +        switch ( evtchn->state )
> +        {
> +        case ECS_INTERDOMAIN:
> +        case ECS_PIRQ:
> +        case ECS_VIRQ:
> +            evtchn_port_bind_to_vcpu(d, evtchn, 
> d->vcpu[evtchn->notify_vcpu_id]);
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +}
> +
...
> diff --git a/xen/common/event_port.c b/xen/common/event_port.c
> index b0ef25b..578acd6 100644
> --- a/xen/common/event_port.c
> +++ b/xen/common/event_port.c
> @@ -13,6 +13,9 @@
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/event.h>
> +#include <xen/event_fifo.h>
> +
> +#include <public/event_channel.h>
> 
>  static void evtchn_check_pollers(struct domain *d, int port)
>  {
> @@ -106,3 +109,187 @@ struct evtchn_port_ops evtchn_port_ops_2l =
>      .is_pending    = evtchn_2l_is_pending,
>      .is_masked     = evtchn_2l_is_masked,
>  };
> +
> +static inline event_word_t *evtchn_fifo_word_from_port(struct domain *d, int 
> port)
> +{
> +    int p, w;
> +
> +    if ( unlikely(port >= d->evtchn_fifo->num_evtchns) )
> +        return NULL;
> +

Should also check against d->max_evtchns? Or you need to keep
d->max_evtchns and num_evtchns in sync.


Wei.



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