[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 
> Note that the unmask and the virq operations are handled by the shim
> itself, and that FIFO event channels are not exposed to the guest.
> 
> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>

In RFC state this certainly doesn't matter yet, but generally I'd
expect From: to match the first S-o-b.

> @@ -155,11 +156,31 @@ static void set_vcpu_id(void)
>  static void xen_evtchn_upcall(struct cpu_user_regs *regs)
>  {
>      struct vcpu_info *vcpu_info = this_cpu(vcpu_info);
> +    unsigned long pending;
>  
>      vcpu_info->evtchn_upcall_pending = 0;
> -    xchg(&vcpu_info->evtchn_pending_sel, 0);
> +    pending = xchg(&vcpu_info->evtchn_pending_sel, 0);
>  
> -    pv_console_rx(regs);
> +    while ( pending )
> +    {
> +        unsigned int l1 = ffsl(pending) - 1;

find_first_set_bit() would look to be the better match here (and
below), not the least because it translates (on capable hardware)
to TZCNT instead of BSF.

> +        unsigned long evtchn = xchg(&XEN_shared_info->evtchn_pending[l1], 0);
> +
> +        __clear_bit(l1, &pending);
> +        evtchn &= ~XEN_shared_info->evtchn_mask[l1];
> +        while ( evtchn )
> +        {
> +            unsigned int port = ffsl(evtchn) - 1;
> +
> +            __clear_bit(port, &evtchn);
> +            port += l1 * BITS_PER_LONG;

What about a 32-bit client? If that's not intended to be supported,
building of such a guest should be prevented (in dom0_build.c).

> @@ -63,6 +65,31 @@ static void __init replace_va(struct domain *d, 
> l4_pgentry_t *l4start,
>                                                        : COMPAT_L1_PROT));
>  }
>  
> +static void evtchn_reserve(struct domain *d, unsigned int port)

const (perhaps also for other helpers below)?

> @@ -101,6 +133,233 @@ void pv_shim_shutdown(uint8_t reason)
>      xen_hypercall_shutdown(reason);
>  }
>  
> +long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct domain *d = current->domain;
> +    long rc;
> +
> +    switch ( cmd )
> +    {
> +#define EVTCHN_FORWARD(cmd, port_field)                                 \
> +case EVTCHNOP_##cmd: {                                                  \
> +    struct evtchn_##cmd op;                                             \

I think this whole macro body would better be indented one more
level, matching up with actual indentation at this point.

> +                                                                        \
> +    if ( copy_from_guest(&op, arg, 1) != 0 )                            \
> +        return -EFAULT;                                                 \
> +                                                                        \
> +    rc = xen_hypercall_event_channel_op(EVTCHNOP_##cmd, &op);           \
> +    if ( rc )                                                           \
> +        break;                                                          \
> +                                                                        \
> +    spin_lock(&d->event_lock);                                          \

Would the lock better be acquired already before the hypercall
above?

> +    rc = evtchn_allocate_port(d, op.port_field);                        \
> +    if ( rc )                                                           \
> +    {                                                                   \
> +        struct evtchn_close close = {                                   \
> +            .port = op.port_field,                                      \
> +        };                                                              \
> +                                                                        \
> +        BUG_ON(xen_hypercall_event_channel_op(EVTCHNOP_close, &close)); \
> +    }                                                                   \
> +    else                                                                \
> +        evtchn_reserve(d, op.port_field);                               \
> +    spin_unlock(&d->event_lock);                                        \
> +                                                                        \
> +    if ( !rc && __copy_to_guest(arg, &op, 1) )                          \
> +        rc = -EFAULT;                                                   \
> +                                                                        \
> +    break;                                                              \
> +    }
> +    EVTCHN_FORWARD(alloc_unbound, port)
> +    EVTCHN_FORWARD(bind_interdomain, local_port)
> +#undef EVTCHN_FORWARD
> +
> +    case EVTCHNOP_bind_virq: {
> +        struct evtchn_bind_virq virq;
> +        struct evtchn_alloc_unbound alloc = {
> +            .dom = DOMID_SELF,
> +            .remote_dom = DOMID_SELF,
> +        };
> +
> +        if ( copy_from_guest(&virq, arg, 1) != 0 )
> +            return -EFAULT;
> +        /*
> +         * The event channel space is actually controlled by L0 Xen, so
> +         * allocate a port from L0 and then force the VIRQ to be bound to 
> that
> +         * specific port.
> +         *
> +         * This is only required for VIRQ because the rest of the event 
> channel
> +         * operations are handled directly by L0.
> +         */
> +        rc = xen_hypercall_event_channel_op(EVTCHNOP_alloc_unbound, &alloc);
> +        if ( rc )
> +           break;
> +
> +        /* Force L1 to use the event channel port allocated on L0. */
> +        rc = evtchn_bind_virq(&virq, alloc.port);
> +        if ( rc )
> +        {
> +             struct evtchn_close free = {

Why is this not named "close", like the other one? Perhaps a single
function wide instance of this structure would suffice?

> +                .port = alloc.port,
> +             };
> +
> +              xen_hypercall_event_channel_op(EVTCHNOP_close, &free);
> +        }
> +
> +        if ( !rc && __copy_to_guest(arg, &virq, 1) )
> +            rc = -EFAULT;
> +
> +        break;
> +    }
> +    case EVTCHNOP_status: {

Blank lines between non-fall-through case blocks please.

> +        struct evtchn_status status;
> +
> +        if ( copy_from_guest(&status, arg, 1) != 0 )
> +            return -EFAULT;
> +
> +        if ( port_is_valid(d, status.port) && evtchn_handled(d, status.port) 
> )

Please be consistent with the validity checks: Compare this one
with ...

> +            rc = evtchn_status(&status);
> +        else
> +            rc = xen_hypercall_event_channel_op(EVTCHNOP_status, &status);
> +
> +        break;
> +    }
> +    case EVTCHNOP_bind_vcpu: {
> +        struct evtchn_bind_vcpu vcpu;
> +
> +        if ( copy_from_guest(&vcpu, arg, 1) != 0 )
> +            return -EFAULT;
> +
> +        if ( !port_is_valid(d, vcpu.port) )
> +            return -EINVAL;
> +
> +        if ( evtchn_handled(d, vcpu.port) )

... the one here. Or otherwise add a comment clarifying why they
are being done differently.

> +    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));

A similar BUG_ON() further up went without comment, which I think
would be fine here too.

> +    case EVTCHNOP_unmask: {
> +        struct evtchn_unmask unmask;
> +
> +        if ( copy_from_guest(&unmask, arg, 1) != 0 )
> +            return -EFAULT;
> +
> +        /* Unmask is handled in L1 */
> +        rc = evtchn_unmask(unmask.port);
> +
> +        break;
> +    }

Is this really sufficient, without handing anything through to L0?
Perhaps it's fine as long as there's no pass-through support here.

> +    default:
> +        /* No FIFO or PIRQ support for now */
> +        rc = -ENOSYS;

-EOPNOTSUPP please.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -63,6 +63,8 @@ struct domain *domain_list;
>  
>  struct domain *hardware_domain __read_mostly;
>  
> +struct domain *pv_domain __read_mostly;

This shouldn't really live in common code, and even less so outside
of any #ifdef (same for its declaration being placed in a common
header).

> @@ -395,6 +397,11 @@ struct domain *domain_create(domid_t domid, unsigned int 
> domcr_flags,
>          rcu_assign_pointer(*pd, d);
>          rcu_assign_pointer(domain_hash[DOMAIN_HASH(domid)], d);
>          spin_unlock(&domlist_update_lock);
> +
> +#ifdef CONFIG_X86
> +        if ( pv_shim )
> +            pv_domain = d;
> +#endif

I assume this #ifdef could be more restrictive.

> @@ -345,13 +365,13 @@ static long 
> evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
>  }
>  
>  
> -static long evtchn_bind_virq(evtchn_bind_virq_t *bind)
> +int evtchn_bind_virq(evtchn_bind_virq_t *bind, int port)

evtchn_port_t please (also in evtchn_allocate_port()), and ...

>  {
>      struct evtchn *chn;
>      struct vcpu   *v;
>      struct domain *d = current->domain;
> -    int            port, virq = bind->virq, vcpu = bind->vcpu;
> -    long           rc = 0;
> +    int            virq = bind->virq, vcpu = bind->vcpu;
> +    int            rc = 0;
>  
>      if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>          return -EINVAL;
> @@ -368,7 +388,12 @@ static long evtchn_bind_virq(evtchn_bind_virq_t *bind)
>      if ( v->virq_to_evtchn[virq] != 0 )
>          ERROR_EXIT(-EEXIST);
>  
> -    if ( (port = get_free_port(d)) < 0 )
> +    if ( port >= 0 )

... use zero as the "please allocate" indicator here (and in the
respective caller).

> @@ -511,7 +536,7 @@ static long evtchn_bind_pirq(evtchn_bind_pirq_t *bind)
>  }
>  
>  
> -static long evtchn_close(struct domain *d1, int port1, bool_t guest)
> +long evtchn_close(struct domain *d1, int port1, bool guest)

Convert return type to "int" at the same time?

> @@ -839,7 +864,7 @@ static void clear_global_virq_handlers(struct domain *d)
>      }
>  }
>  
> -static long evtchn_status(evtchn_status_t *status)
> +long evtchn_status(evtchn_status_t *status)

Same here.

> @@ -1030,6 +1055,11 @@ long do_event_channel_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      long rc;
>  
> +#ifdef CONFIG_X86
> +    if ( pv_shim )
> +        return pv_shim_event_channel_op(cmd, arg);
> +#endif

Patch it right into the hypercall table instead?

Jan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.