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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 08, 2018 at 09:05:40AM -0700, Jan Beulich wrote:
> >>> On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> > +        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).

32bit client? You mean building a shim that runs in 32bit mode? If so
I haven't really through of it, but in any case BITS_PER_LOG would be
OK also in that case?

> > +                                                                        \
> > +    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?

I'm not sure I see your point here, certainly L0 already must have
it's own locking. AFAICT the shim just needs to lock event_lock when
fiddling with event channel data of the guest.

> > +    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.

For the unmask operation? I think so, if there was a pending event the
shim will already take care of injecting it to the guest.

> > @@ -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?

That would only work if the shim is a compile time option, but not a
run time one, the hypercall table is ro.

Thanks, Roger.

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