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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.13 at 12:55, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> +static int map_guest_page(struct domain *d, uint64_t gfn,
> +                          struct page_info **page, void **virt)
> +{
> +    struct page_info *p;
> +
> +    p = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> +    if ( !p )
> +        return -EINVAL;
> +
> +    if ( !get_page_type(p, PGT_writable_page) )
> +    {
> +        put_page(p);
> +        return -EINVAL;
> +    }
> +
> +    *virt = map_domain_page_global(gfn);

Since this one returns page aligned addresses, ...

> +    if ( !*virt )
> +    {
> +        put_page_and_type(p);
> +        return -ENOMEM;
> +    }
> +    *page = p;
> +    return 0;
> +}
> +
> +static void unmap_guest_page(struct page_info *page, void *virt)
> +{
> +    if ( page == NULL )
> +        return;
> +
> +    unmap_domain_page_global(virt);

... this one expects page aligned addresses, but the way you use
it below doesn't guarantee that (see b0581b92 ("x86: make
map_domain_page_global() a simple wrapper around vmap()")).

> +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;
> +    unsigned i;
> +    int rc;
> +
> +    if ( v->evtchn_fifo )
> +        return -EINVAL;
> +
> +    efv = xzalloc(struct evtchn_fifo_vcpu);
> +    if ( efv == NULL )
> +        return -ENOMEM;
> +
> +    rc = map_guest_page(d, gfn, &page, &virt);
> +    if ( rc < 0 )
> +    {
> +        xfree(efv);
> +        return rc;
> +    }
> +
> +    v->evtchn_fifo = efv;
> +
> +    v->evtchn_fifo->cb_page       = page;

There's no real need to store this if you only need it for freeing
later. x86 at least has domain_page_map_to_mfn() to recover
the MFN (and thus page) from the mapping, and if ARM doesn't
they should implement it.

> +struct evtchn_fifo_queue {
> +    uint32_t *head; /* points into control block */
> +    uint32_t tail;
> +    spinlock_t lock;
> +    uint8_t priority;
> +};

spinlock_t being at least 32 bits in size you win nothing with this
ordering, whereas if "tail" and "priority" would be adjacent the
structure size might decrease for certain cases (namely when
alignof(spinlock_t) > 4).

Jan


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