Re: [Xen-devel] [PATCH] evtchn: make support for different ABIs tunable

[Jan Beulich <jbeulich@suse.com>]

On 07.08.2019 13:20, Eslam Elnikety wrote:
> Adding support for FIFO event channel ABI was first introduced in Xen 4.4
> (see 88910061ec6). Make this support tunable, since the choice of which
> event channel ABI has implications for hibernation. Consider resuming a
> pre Xen 4.4 hibernated Linux guest. The guest boot kernel defaults to FIFO
> ABI, whereas the resume kernel assumes 2L. This, in turn, results in Xen
> and the resumed kernel talking past each other (due to different protocols
> FIFO vs 2L).
> 
> In order to announce to guests that the event channel ABI does not support
> FIFO, the hypervisor returns ENOSYS on init_control operation. When this
> operation fails, the guest should continue to use the 2L event channel ABI.
> For example, in Linux drivers/xen/events/events_base.c:
> 
>      if (fifo_events)
>          ret = xen_evtchn_fifo_init();
>      if (ret < 0)
>          xen_evtchn_2l_init();
> 
> and xen_evtchn_fifo_init fails when EVTCHNOP_init_control fails. This commit
> does not change the current default behaviour: announce FIFO event channels
> ABI support for guests unless explicitly stated otherwise at domaincreate.
> 
> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>

Seeing that there looks to form agreement that restricting evtchn
variants just like gnttab ones is wanted (I'm still not really
convinced it is the right approach for the problem at hand, but I
do agree having such control may be helpful in general), a few
remarks on the patch itself:

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -444,6 +444,9 @@ struct domain *domain_create(domid_t domid,
>           d->controller_pause_count = 1;
>           atomic_inc(&d->pause_count);
>   
> +        if ( d->options & XEN_DOMCTL_CDF_disable_fifo )
> +            d->disable_evtchn_fifo = 1;

This wants to be "true".

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1170,6 +1170,11 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>   
>       case EVTCHNOP_init_control: {
>           struct evtchn_init_control init_control;
> +
> +        /* Fail init_control for domains that must use 2l ABI */
> +        if ( current->domain->disable_evtchn_fifo )
> +            return -ENOSYS;

ENOSYS is not an appropriate error code here. EOPNOTSUPP might
be, and I guess there are more options (like EPERM or EACCES).

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -338,6 +338,7 @@ struct domain
>      struct evtchn  **evtchn_group[NR_EVTCHN_GROUPS]; /* all other buckets */
>      unsigned int     max_evtchns;     /* number supported by ABI */
>      unsigned int     max_evtchn_port; /* max permitted port number */
> +    bool             disable_evtchn_fifo;            /* force 2l ABI */
>      unsigned int     valid_evtchns;   /* number of allocated event channels */
>      spinlock_t       event_lock;
>      const struct evtchn_port_ops *evtchn_port_ops;

I suppose you can find a better place to put this 1-byte field
than between two 32-bit ones, leaving a 3-byte hole.

Jan

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