|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |