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

[Jan Beulich <JBeulich@xxxxxxxx>]

On 08.08.2019 16:31,  Elnikety, Eslam  wrote:

First of all - can you please try to get your reply quoting
improved, such that readers can actually tell your replies
from context? (I didn't check whether perhaps your mail was
a HTML one, and my plain text reading of it discarded the
markings. If so - please don't send HTML mail.)

> On 8. Aug 2019, at 15:27, Jan Beulich 
> <JBeulich@xxxxxxxx<mailto:JBeulich@xxxxxxxx>> wrote:
> On 07.08.2019 19:42, Eslam Elnikety wrote:
> --- 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->options & XEN_DOMCTL_CDF_disable_fifo )
> +            return -EPERM;
> +
>           if ( copy_from_guest(&init_control, arg, 1) != 0 )
>               return -EFAULT;
>           rc = evtchn_fifo_init_control(&init_control);
> 
> I think the check would better go into evtchn_fifo_init_control(),
> at least as long as the setting really is FIFO-centric. Also the
> 
> Not sure. It is the FIFO ABI that defines EVTCHNOP_init_control
> (not defined in 2L). Short-circuiting the hypercall at this place
> seems more appropriate.

I'd expect any 3rd variant to also have a need for an init-control
operation, and hence at that point this would become a hook like
many of the other ops. And at that point the check would need to
move anyway. Hence it's better to put it in its long term
designated place right away.

> Moreover, doing copy_from_guest and calling into
> evtchn_fifo_init_control only to return error is not optimal.

True, yet from my pov the more logical alternative code structure
is still preferable.

> Irrespective of these remarks, and along the lines of comments on
> the v1 thread, introducing wider control that would also allow
> disabling 2-level (for HVM guests) would seem better to me. This
> would then hopefully be coded up in a way that would make extending
> it straightforward, once a 3rd mechanism appears.
> 
> Hmmm... we cannot force guests to call init_control (in order to flip from 2L 
> to FIFO). Quoting from [1] 4.4 “If this call (EVTCHNOP_init_control) fails on 
> the boot VCPU, the guest should continue to use the 2-level event channel ABI 
> for all VCPUs.” Support for 2L ABI does not sound like something that can be 
> optional. Once a 3rd mechanism appears, I imagine adding a corresponding 
> domctl flag to control such mechanism.

For HVM, event channels as a whole should be optional; we shouldn't
default a possible control for this to off though. For PV, the
2-level interface indeed has to be considered mandatory. Hence
today there are these (theoretically) possible combinations

                PV              HVM
nothing         invalid         valid
2-level only    valid           valid
FIFO only       ???             valid
both            valid           valid

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