>>> On 04.01.18 at 14:06, <wei.l...@citrix.com> wrote: > From: Roger Pau Monne <roger....@citrix.com> > > 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 <aligu...@amazon.com> > Signed-off-by: Roger Pau Monné <roger....@citrix.com> > Signed-off-by: Sergey Dyasli <sergey.dya...@citrix.com>
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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel