Re: [Xen-devel] [PATCH RFC v1 55/74] xen/pvshim: forward evtchn ops between L0 Xen and L2 DomU

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> +    case EVTCHNOP_close: {
> +        struct evtchn_close close;
> +
> +        if ( copy_from_guest(&close, arg, 1) != 0 )
> +            return -EFAULT;
> +
> +        if ( !port_is_valid(d, close.port) )
> +            return -EINVAL;
> +
> +        if ( evtchn_handled(d, close.port) )
> +        {
> +            rc = evtchn_close(d, close.port, true);
> +            if ( rc )
> +                break;
> +        }
> +        else
> +            evtchn_free(d, evtchn_from_port(d, close.port));

Judging by other callers of this function you ought to hold the
domain event lock and the event channel lock around it. That's
one of the reasons the original function was static and didn't
follow the evtchn_*() naming scheme of externally usable
functions. Perhaps you want to introduce evtchn_free() as a
wrapper around free_evtchn(), acquiring and releasing the
locks? In this particular case use of evtchn_from_port() may
also be deemed a layering violation.

> +        rc = xen_hypercall_event_channel_op(EVTCHNOP_close, &close);
> +        if ( rc )
> +            /*
> +             * If the port cannot be closed on the L0 mark it as reserved
> +             * in the shim to avoid re-using it.
> +             */
> +            evtchn_reserve(d, close.port);
> +
> +        set_bit(close.port, XEN_shared_info->evtchn_mask);

Wouldn't this better be set earlier? The port is certainly available for
re-use prior to making it here. And I wonder whether the bit wouldn't
also want setting on some of the error paths closing ports, just to be
on the safe side.

> +    case EVTCHNOP_bind_ipi: {
> +        struct evtchn_bind_ipi ipi;
> +
> +        if ( copy_from_guest(&ipi, arg, 1) != 0 )
> +            return -EFAULT;
> +
> +        rc = xen_hypercall_event_channel_op(EVTCHNOP_bind_ipi, &ipi);
> +        if ( rc )
> +            break;
> +
> +        spin_lock(&d->event_lock);
> +        rc = evtchn_allocate_port(d, ipi.port);
> +        if ( rc )
> +        {
> +            struct evtchn_close close = {
> +                .port = ipi.port,
> +            };
> +
> +            /*
> +             * If closing the event channel port also fails there's not
> +             * much the shim can do, since it has been unable to reserve
> +             * the port in it's event channel space.
> +             */
> +            BUG_ON(xen_hypercall_event_channel_op(EVTCHNOP_close, &close));
> +            break;

There's a spin_unlock() missing here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel