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

From: "Elnikety, Eslam" <elnikety@amazon.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Ian Jackson <ian.jackson@eu.citrix.com>,
	Anthony PERARD <anthony.perard@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Woodhouse, David" <dwmw@amazon.co.uk>
Subject: Re: [Xen-devel] [PATCH] evtchn: make support for different ABIs tunable
Date: Wed, 7 Aug 2019 15:43:34 +0000	[thread overview]
Message-ID: <9C00D857-BEEC-4805-A1AB-44986C1D795F@amazon.com> (raw)
In-Reply-To: <33d6bbb5-cecd-a499-89f4-1592a3fb0eac@citrix.com>

> On 7. Aug 2019, at 15:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 07/08/2019 12:20, Eslam Elnikety wrote:
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 19486d5e32..654b4fdd22 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -64,6 +64,9 @@ struct xen_domctl_createdomain {
>>  /* Is this a xenstore domain? */
>> #define _XEN_DOMCTL_CDF_xs_domain     4
>> #define XEN_DOMCTL_CDF_xs_domain      (1U<<_XEN_DOMCTL_CDF_xs_domain)
>> + /* Disable FIFO event channels? */
>> +#define _XEN_DOMCTL_CDF_disable_fifo  5
>> +#define XEN_DOMCTL_CDF_disable_fifo   (1U<<_XEN_DOMCTL_CDF_disable_fifo)
>>     uint32_t flags;
> 
> On the subject of the the patch itself, I think this is broadly the
> right principle, but wants to be expressed differently.
> 
> First, you'll want to rebase onto a very recent master, and specifically
> over c/s d8f2490561eb which has changed how this field is handled in Xen.

The patch was already on master c/s 0a6ad045c5fe. That said, I was not using the new d->options. Thanks for the hint. v2 will.

> 
> Furthermore, if there is this problem for event channels, then there is
> almost certainly a related problem for grant tables.

I would concur. For grant tables, there is at least the opt_gnttab_max_version controlled by commandline. It would be nice to have it per-domain.

> 
> The control in Xen should be expressed in a positive form, or the logic
> will become a tangle.  It should be a bit permitting the use of the FIFO
> ABI, rather than a bit saying "oh actually, you can't use that”.

I chose the negative form since otherwise the patch will have to enable that bit in many places to retain the current default behaviour of allowing FIFO unless stated otherwise. (The form, as is, is similar to other knobs: feature-disable-keyboard and feature-disable-pointer.)

> 
> That said, it might be easier to declare FIFO to be "event channel v2",
> and specify max_{grant,evntchn}_ver instead.
> 
> I'm open to other suggestions as well.
> 
> ~Andrew

Andrew, Jan, and Julien, thanks for reviewing and the feedback. I will revise and send v2 over the next couple of days. 

Thanks,
Eslam

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