[PATCH v2 0/1] evtchn: make EVTCHNOP_reset suitable for kexec

[PATCH v2 0/1] evtchn: make EVTCHNOP_reset suitable for kexec

[Vitaly Kuznetsov]

With the help from David Vrabel (thanks!) I was able to make event channel
rebinding work in both kexec/kdump cases. A guest should do the following
on kexec/kdump:
1) Figure out store/console interdomain mapping
2) Call PHYSDEVOP_unmap_pirq if needed
3) Call EVTCHNOP_reset
4) Rebind store/console channels with EVTCHNOP_bind_interdomain
5) Update HVM_PARAM_CONSOLE_EVTCHN/HVM_PARAM_STORE_EVTCHN

Here is an example of guest code (to be called from kexec/kdump handlers):

void xen_hvm_reset_eventchannels(void)
{
	struct evtchn_status status;
	struct physdev_unmap_pirq unmap_irq;
	struct evtchn_close close;
	struct evtchn_reset reset;
	struct evtchn_bind_interdomain int_bind;
	struct xen_hvm_param xhv;
	uint64_t store_evtchn = 0, console_evtchn = 0;
	domid_t store_domain = 0, console_domain = 0;
	evtchn_port_t store_rem_port = 0, console_rem_port = 0;
	int port, rc = -ENOENT;

	memset(&status, 0, sizeof(status));
	status.dom = DOMID_SELF;
	if (hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &store_evtchn) == 0) {
		pr_debug("xen: need to rebind store evtchn %d\n",
			 (uint32_t)store_evtchn);
		status.port = store_evtchn;
		rc = HYPERVISOR_event_channel_op(EVTCHNOP_status, &status);
		if (!rc && status.status == EVTCHNSTAT_interdomain) {
			pr_debug("xen: store port: %d, interdomain %d:%d",
				 status.port, status.u.interdomain.dom,
				 status.u.interdomain.port);
			store_domain = status.u.interdomain.dom;
			store_rem_port = status.u.interdomain.port;
		}
	}
	if (hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &console_evtchn) == 0) {
		pr_debug("xen: need to rebind console evtchn %d\n",
			 (uint32_t)console_evtchn);
		status.port = console_evtchn;
		rc = HYPERVISOR_event_channel_op(EVTCHNOP_status, &status);
		if (!rc && status.status == EVTCHNSTAT_interdomain) {
			pr_debug("xen: console port: %d, interdomain %d:%d",
				 status.port, status.u.interdomain.dom,
				 status.u.interdomain.port);
			console_domain = status.u.interdomain.dom;
			console_rem_port = status.u.interdomain.port;
		}
	}

	for (port = 0; port < xen_evtchn_max_channels(); port++) {
		status.dom = DOMID_SELF;
		status.port = port;
		rc = HYPERVISOR_event_channel_op(EVTCHNOP_status, &status);
		if (rc < 0)
			continue;
		if (status.status != EVTCHNSTAT_closed) {
			close.port = port;
			if (HYPERVISOR_event_channel_op(EVTCHNOP_close,
							&close) != 0)
				pr_warn("xen: failed to close evtchn %d\n",
					port);
		}

		if (status.status == EVTCHNSTAT_pirq) {
			unmap_irq.pirq = status.u.pirq;
			unmap_irq.domid = DOMID_SELF;
			pr_warn("xen: unmapping previously mapped pirq %d\n",
				unmap_irq.pirq);
			if (HYPERVISOR_physdev_op(PHYSDEVOP_unmap_pirq,
						  &unmap_irq) != 0)
				pr_warn("xen: failed to unmap pirq %d\n",
					unmap_irq.pirq);
		}
	}

	reset.dom = DOMID_SELF;
	if (HYPERVISOR_event_channel_op(EVTCHNOP_reset, &reset))
		pr_warn("failed to reset all event channels!\n");

	if (store_rem_port > 0) {
		int_bind.remote_dom = store_domain;
		int_bind.remote_port = store_rem_port;
		pr_debug("rebinding store: %d -> %d:%d\n", int_bind.local_port,
			 int_bind.remote_port, int_bind.remote_dom);
		if (HYPERVISOR_event_channel_op(EVTCHNOP_bind_interdomain, &int_bind))
			pr_warn("failed to rebind store!\n");
		if (int_bind.local_port != store_evtchn) {
			xhv.domid = DOMID_SELF;
			xhv.index = HVM_PARAM_STORE_EVTCHN;
			xhv.value = int_bind.local_port;
			if (HYPERVISOR_hvm_op(HVMOP_set_param, &xhv)) {
				pr_warn("Cannot set HVM_PARAM_STORE_EVTCHN!\n");
			}
		}
	}

	if (console_rem_port > 0) {
		int_bind.remote_dom = console_domain;
		int_bind.remote_port = console_rem_port;
		pr_debug("rebinding console: %d -> %d:%d\n", int_bind.local_port,
			 int_bind.remote_port, int_bind.remote_dom);
		if (HYPERVISOR_event_channel_op(EVTCHNOP_bind_interdomain, &int_bind))
			pr_warn("failed to rebind console!\n");
		if (int_bind.local_port != console_evtchn) {
			xhv.domid = DOMID_SELF;
			xhv.index = HVM_PARAM_CONSOLE_EVTCHN;
			xhv.value = int_bind.local_port;
			if (HYPERVISOR_hvm_op(HVMOP_set_param, &xhv)) {
				pr_warn("Cannot set HVM_PARAM_CONSOLE_EVTCHN!\n");
			}
		}
	}
}

Vitaly Kuznetsov (1):
  evtchn: make EVTCHNOP_reset suitable for kexec

 xen/common/event_channel.c | 11 +++++++++++
 xen/common/event_fifo.c    |  1 +
 2 files changed, 12 insertions(+)

-- 
1.9.3

[PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[Vitaly Kuznetsov]

It would be nice to allow guests to close all event channels in
ABI-agnostic way in case of kexec/kdump. EVTCHNOP_reset looks suitable
for this purpose. However control blocks for vcpus and event array need
cleanup when FIFO ABI is being used.

With this change a guest can simply do EVTCHNOP_reset before kexec in
both 2-level and FIFO cases. It is also important to perform store/console
channel remapping after such call.

The issue can also be solved by introducing a new EVTCHNOP operation but
it seems that EVTCHNOP_reset can be reused.

[The idea was suggested by Ian Campbell, Andrew Cooper, and David Vrabel]

Changes from v1:
- Change EVTCHNOP_reset behavior when it is being called by the guest
  with DOMID_SELF only [Andrew Cooper, Jan Beulich].
- Do not skip interdomain store/console channels, let the guest rebind
  them [David Vrabel].
- Do evtchn_2l_init() after evtchn_fifo_destroy() so the guest can
  perform e.g. EVTCHNOP_bind_interdomain operations after EVTCHNOP_reset.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 xen/common/event_channel.c | 11 +++++++++++
 xen/common/event_fifo.c    |  1 +
 2 files changed, 12 insertions(+)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index db952af..0dad08d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
     for ( i = 0; port_is_valid(d, i); i++ )
         (void)__evtchn_close(d, i);
 
+    if ( (d == current->domain) && d->evtchn_fifo )
+    {
+        /*
+         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
+         * FIFO event array and control blocks, resetting evtchn_port_ops to
+         * evtchn_port_ops_2l.
+         */
+        evtchn_fifo_destroy(d);
+        evtchn_2l_init(d);
+    }
+
     rc = 0;
 
 out:
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 1fce3f1..51b4ff6 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -451,6 +451,7 @@ static void cleanup_event_array(struct domain *d)
     for ( i = 0; i < EVTCHN_FIFO_MAX_EVENT_ARRAY_PAGES; i++ )
         unmap_guest_page(d->evtchn_fifo->event_array[i]);
     xfree(d->evtchn_fifo);
+    d->evtchn_fifo = NULL;
 }
 
 static void setup_ports(struct domain *d)
-- 
1.9.3

Re: [PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[Jan Beulich]

>>> On 30.07.14 at 15:26, <vkuznets@redhat.com> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
>      for ( i = 0; port_is_valid(d, i); i++ )
>          (void)__evtchn_close(d, i);
>  
> +    if ( (d == current->domain) && d->evtchn_fifo )
> +    {
> +        /*
> +         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
> +         * FIFO event array and control blocks, resetting evtchn_port_ops to
> +         * evtchn_port_ops_2l.
> +         */

At the very least comment and code should match. In the case here I
think you want to change the code: Replace "d == current->domain"
with "dom == DOMID_SELF", leaving the unaltered behavior still
available when a domain passes its own ID.

Jan

Re: [PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[David Vrabel]

On 30/07/14 14:26, Vitaly Kuznetsov wrote:
> It would be nice to allow guests to close all event channels in
> ABI-agnostic way in case of kexec/kdump. EVTCHNOP_reset looks suitable
> for this purpose. However control blocks for vcpus and event array need
> cleanup when FIFO ABI is being used.
> 
> With this change a guest can simply do EVTCHNOP_reset before kexec in
> both 2-level and FIFO cases. It is also important to perform store/console
> channel remapping after such call.

Can you add documentation to xen/include/public/event_channels.h?

> The issue can also be solved by introducing a new EVTCHNOP operation but
> it seems that EVTCHNOP_reset can be reused.
> 
> [The idea was suggested by Ian Campbell, Andrew Cooper, and David Vrabel]
> 
> Changes from v1:

List changes after a --- marker so they don't end up in the final commit
message.

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
>      for ( i = 0; port_is_valid(d, i); i++ )
>          (void)__evtchn_close(d, i);
>  
> +    if ( (d == current->domain) && d->evtchn_fifo )
> +    {
> +        /*
> +         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
> +         * FIFO event array and control blocks, resetting evtchn_port_ops to
> +         * evtchn_port_ops_2l.
> +         */
> +        evtchn_fifo_destroy(d);
> +        evtchn_2l_init(d);

You need to take d->event_lock around this if, or the guest could try to
bind another event whilst the ABI is in an inconsistent state.

David

Re: [PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[Jan Beulich]

>>> On 30.07.14 at 15:42, <david.vrabel@citrix.com> wrote:
> On 30/07/14 14:26, Vitaly Kuznetsov wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
>>      for ( i = 0; port_is_valid(d, i); i++ )
>>          (void)__evtchn_close(d, i);
>>  
>> +    if ( (d == current->domain) && d->evtchn_fifo )
>> +    {
>> +        /*
>> +         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
>> +         * FIFO event array and control blocks, resetting evtchn_port_ops to
>> +         * evtchn_port_ops_2l.
>> +         */
>> +        evtchn_fifo_destroy(d);
>> +        evtchn_2l_init(d);
> 
> You need to take d->event_lock around this if, or the guest could try to
> bind another event whilst the ABI is in an inconsistent state.

True, but then not just around this. Subsequent to any of the
__evtchn_close() invocations above, a port could also get
re-used (resulting in at least leaks). So I guess after taking the
lock there would also need to be a loop checking that all ports
are still closed (and return an error if they aren't).

Jan

Re: [PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[Vitaly Kuznetsov]

"Jan Beulich" <JBeulich@suse.com> writes:

>>>> On 30.07.14 at 15:26, <vkuznets@redhat.com> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
>>      for ( i = 0; port_is_valid(d, i); i++ )
>>          (void)__evtchn_close(d, i);
>>  
>> +    if ( (d == current->domain) && d->evtchn_fifo )
>> +    {
>> +        /*
>> +         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
>> +         * FIFO event array and control blocks, resetting evtchn_port_ops to
>> +         * evtchn_port_ops_2l.
>> +         */
>
> At the very least comment and code should match. In the case here I
> think you want to change the code: Replace "d == current->domain"
> with "dom == DOMID_SELF", leaving the unaltered behavior still
> available when a domain passes its own ID.

Thanks, missed that case. Is there a reason why unaltered behavior is
better in that case? I understand it gives a choice but it would look
very unobvious...

>
> Jan

-- 
  Vitaly

Re: [PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[David Vrabel]

On 30/07/14 14:53, Jan Beulich wrote:
>>>> On 30.07.14 at 15:42, <david.vrabel@citrix.com> wrote:
>> On 30/07/14 14:26, Vitaly Kuznetsov wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
>>>      for ( i = 0; port_is_valid(d, i); i++ )
>>>          (void)__evtchn_close(d, i);
>>>  
>>> +    if ( (d == current->domain) && d->evtchn_fifo )
>>> +    {
>>> +        /*
>>> +         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
>>> +         * FIFO event array and control blocks, resetting evtchn_port_ops to
>>> +         * evtchn_port_ops_2l.
>>> +         */
>>> +        evtchn_fifo_destroy(d);
>>> +        evtchn_2l_init(d);
>>
>> You need to take d->event_lock around this if, or the guest could try to
>> bind another event whilst the ABI is in an inconsistent state.
> 
> True, but then not just around this. Subsequent to any of the
> __evtchn_close() invocations above, a port could also get
> re-used (resulting in at least leaks). So I guess after taking the
> lock there would also need to be a loop checking that all ports
> are still closed (and return an error if they aren't).

I considered this but I think the only side effect would be losing a
pending event, and it would be sufficient to document that guests should
not bind events while calling EVTCHNOP_reset.

David

Re: [PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[Jan Beulich]

>>> On 30.07.14 at 15:59, <vkuznets@redhat.com> wrote:
> "Jan Beulich" <JBeulich@suse.com> writes:
> 
>>>>> On 30.07.14 at 15:26, <vkuznets@redhat.com> wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
>>>      for ( i = 0; port_is_valid(d, i); i++ )
>>>          (void)__evtchn_close(d, i);
>>>  
>>> +    if ( (d == current->domain) && d->evtchn_fifo )
>>> +    {
>>> +        /*
>>> +         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
>>> +         * FIFO event array and control blocks, resetting evtchn_port_ops 
> to
>>> +         * evtchn_port_ops_2l.
>>> +         */
>>
>> At the very least comment and code should match. In the case here I
>> think you want to change the code: Replace "d == current->domain"
>> with "dom == DOMID_SELF", leaving the unaltered behavior still
>> available when a domain passes its own ID.
> 
> Thanks, missed that case. Is there a reason why unaltered behavior is
> better in that case? I understand it gives a choice but it would look
> very unobvious...

No, it's indeed solely for the caller to have a way to pick between both
behaviors.

Jan

Re: [PATCH v2 1/1] evtchn: make EVTCHNOP_reset suitable for kexec

[Jan Beulich]

>>> On 30.07.14 at 16:03, <david.vrabel@citrix.com> wrote:
> On 30/07/14 14:53, Jan Beulich wrote:
>>>>> On 30.07.14 at 15:42, <david.vrabel@citrix.com> wrote:
>>> On 30/07/14 14:26, Vitaly Kuznetsov wrote:
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -957,6 +957,17 @@ static long evtchn_reset(evtchn_reset_t *r)
>>>>      for ( i = 0; port_is_valid(d, i); i++ )
>>>>          (void)__evtchn_close(d, i);
>>>>  
>>>> +    if ( (d == current->domain) && d->evtchn_fifo )
>>>> +    {
>>>> +        /*
>>>> +         * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
>>>> +         * FIFO event array and control blocks, resetting evtchn_port_ops 
> to
>>>> +         * evtchn_port_ops_2l.
>>>> +         */
>>>> +        evtchn_fifo_destroy(d);
>>>> +        evtchn_2l_init(d);
>>>
>>> You need to take d->event_lock around this if, or the guest could try to
>>> bind another event whilst the ABI is in an inconsistent state.
>> 
>> True, but then not just around this. Subsequent to any of the
>> __evtchn_close() invocations above, a port could also get
>> re-used (resulting in at least leaks). So I guess after taking the
>> lock there would also need to be a loop checking that all ports
>> are still closed (and return an error if they aren't).
> 
> I considered this but I think the only side effect would be losing a
> pending event, and it would be sufficient to document that guests should
> not bind events while calling EVTCHNOP_reset.

Hmm, indeed, with what 2-level and FIFO currently do, there
wouldn't appear to be any leak or other badness.

Jan