[PATCH 0/4] A few event channel-related fixes

[PATCH 0/4] A few event channel-related fixes

[Boris Ostrovsky]

Fixes for issues that we discovered during live migration when source and
target systems have different event channel assignments for guests.

Boris Ostrovsky (4):
  xen/events: Clear cpu_evtchn_mask before resuming
  xen/xenbus: Update xenbus event channel on resume
  xen/console: Update console event channel on resume
  xen/events: Set irq_info->evtchn before binding the channel to CPU in
    __startup_pirq()

 drivers/tty/hvc/hvc_xen.c         |   19 ++++++++++++++++++-
 drivers/xen/events/events_2l.c    |   10 ++++++++++
 drivers/xen/events/events_base.c  |    2 +-
 drivers/xen/xenbus/xenbus_comms.c |    1 +
 drivers/xen/xenbus/xenbus_probe.c |   28 ++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 2 deletions(-)

[PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

When a guest is resumed, the hypervisor may change event channel
assignments. If this happens and the guest uses 2-level events it
is possible for the interrupt to be claimed by wrong VCPU since
cpu_evtchn_mask bits may be stale. This can happen even though
evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
is passed in is not necessarily the original one (from pre-migration
times) but instead is freshly allocated during resume and so any
information about which CPU the channel was bound to is lost.

Thus we should clear the mask during resume.

We also need to make sure that bits for xenstore and console channels
are set when these two subsystems are resumed. While rebind_evtchn_irq()
(which is invoked for both of them on a resume) calls irq_set_affinity(),
the latter will in fact postpone setting affinity until handling the
interrupt. But because cpu_evtchn_mask will have bits for these two cleared
we won't be able to take the interrupt.

Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
allowing to set affinity immediately, which is safe for event-channel-based
interrupts.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reported-by: Annie Li <annie.li@oracle.com>
Tested-by: Annie Li <annie.li@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c         |    1 +
 drivers/xen/events/events_2l.c    |   10 ++++++++++
 drivers/xen/xenbus/xenbus_comms.c |    1 +
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index f1e5742..5eb80bd 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
 
 		info = vtermno_to_xencons(HVC_COOKIE);
 		info->irq = bind_evtchn_to_irq(info->evtchn);
+		irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
 	}
 	if (info->irq < 0)
 		info->irq = 0; /* NO_IRQ */
diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
index 5db43fc..7dd4631 100644
--- a/drivers/xen/events/events_2l.c
+++ b/drivers/xen/events/events_2l.c
@@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void evtchn_2l_resume(void)
+{
+	int i;
+
+	for_each_online_cpu(i)
+		memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
+				EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
+}
+
 static const struct evtchn_ops evtchn_ops_2l = {
 	.max_channels      = evtchn_2l_max_channels,
 	.nr_channels       = evtchn_2l_max_channels,
@@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
 	.mask              = evtchn_2l_mask,
 	.unmask            = evtchn_2l_unmask,
 	.handle_events     = evtchn_2l_handle_events,
+	.resume	           = evtchn_2l_resume,
 };
 
 void __init xen_evtchn_2l_init(void)
diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
index fdb0f33..30203d1 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -231,6 +231,7 @@ int xb_init_comms(void)
 		}
 
 		xenbus_irq = err;
+		irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
 	}
 
 	return 0;
-- 
1.7.1

[PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

When a guest is resumed, the hypervisor may change event channel
assignments. If this happens and the guest uses 2-level events it
is possible for the interrupt to be claimed by wrong VCPU since
cpu_evtchn_mask bits may be stale. This can happen even though
evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
is passed in is not necessarily the original one (from pre-migration
times) but instead is freshly allocated during resume and so any
information about which CPU the channel was bound to is lost.

Thus we should clear the mask during resume.

We also need to make sure that bits for xenstore and console channels
are set when these two subsystems are resumed. While rebind_evtchn_irq()
(which is invoked for both of them on a resume) calls irq_set_affinity(),
the latter will in fact postpone setting affinity until handling the
interrupt. But because cpu_evtchn_mask will have bits for these two cleared
we won't be able to take the interrupt.

Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
allowing to set affinity immediately, which is safe for event-channel-based
interrupts.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reported-by: Annie Li <annie.li@oracle.com>
Tested-by: Annie Li <annie.li@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c         |    1 +
 drivers/xen/events/events_2l.c    |   10 ++++++++++
 drivers/xen/xenbus/xenbus_comms.c |    1 +
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index f1e5742..5eb80bd 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
 
 		info = vtermno_to_xencons(HVC_COOKIE);
 		info->irq = bind_evtchn_to_irq(info->evtchn);
+		irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
 	}
 	if (info->irq < 0)
 		info->irq = 0; /* NO_IRQ */
diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
index 5db43fc..7dd4631 100644
--- a/drivers/xen/events/events_2l.c
+++ b/drivers/xen/events/events_2l.c
@@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void evtchn_2l_resume(void)
+{
+	int i;
+
+	for_each_online_cpu(i)
+		memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
+				EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
+}
+
 static const struct evtchn_ops evtchn_ops_2l = {
 	.max_channels      = evtchn_2l_max_channels,
 	.nr_channels       = evtchn_2l_max_channels,
@@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
 	.mask              = evtchn_2l_mask,
 	.unmask            = evtchn_2l_unmask,
 	.handle_events     = evtchn_2l_handle_events,
+	.resume	           = evtchn_2l_resume,
 };
 
 void __init xen_evtchn_2l_init(void)
diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
index fdb0f33..30203d1 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -231,6 +231,7 @@ int xb_init_comms(void)
 		}
 
 		xenbus_irq = err;
+		irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
 	}
 
 	return 0;
-- 
1.7.1

[PATCH 2/4] xen/xenbus: Update xenbus event channel on resume

[Boris Ostrovsky]

After a resume the hypervisor/tools may change xenbus event
channel number. We should re-query it.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 drivers/xen/xenbus/xenbus_probe.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 564b315..31737a4 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -57,6 +57,7 @@
 #include <xen/xen.h>
 #include <xen/xenbus.h>
 #include <xen/events.h>
+#include <xen/xen-ops.h>
 #include <xen/page.h>
 
 #include <xen/hvm.h>
@@ -735,6 +736,30 @@ static int __init xenstored_local_init(void)
 	return err;
 }
 
+static int xenbus_resume_cb(struct notifier_block *nb,
+			    unsigned long action, void *data)
+{
+	int err = 0;
+
+	if (xen_hvm_domain()) {
+		uint64_t v;
+
+		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
+		if (!err && v)
+			xen_store_evtchn = v;
+		else
+			pr_warn("Cannot update xenstore event channel: %d\n",
+				err);
+	} else
+		xen_store_evtchn = xen_start_info->store_evtchn;
+
+	return err;
+}
+
+static struct notifier_block xenbus_resume_nb = {
+	.notifier_call = xenbus_resume_cb,
+};
+
 static int __init xenbus_init(void)
 {
 	int err = 0;
@@ -793,6 +818,9 @@ static int __init xenbus_init(void)
 		goto out_error;
 	}
 
+	if (!xen_initial_domain())
+		xen_resume_notifier_register(&xenbus_resume_nb);
+
 #ifdef CONFIG_XEN_COMPAT_XENFS
 	/*
 	 * Create xenfs mountpoint in /proc for compatibility with
-- 
1.7.1

[PATCH 2/4] xen/xenbus: Update xenbus event channel on resume

[Boris Ostrovsky]

After a resume the hypervisor/tools may change xenbus event
channel number. We should re-query it.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 drivers/xen/xenbus/xenbus_probe.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 564b315..31737a4 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -57,6 +57,7 @@
 #include <xen/xen.h>
 #include <xen/xenbus.h>
 #include <xen/events.h>
+#include <xen/xen-ops.h>
 #include <xen/page.h>
 
 #include <xen/hvm.h>
@@ -735,6 +736,30 @@ static int __init xenstored_local_init(void)
 	return err;
 }
 
+static int xenbus_resume_cb(struct notifier_block *nb,
+			    unsigned long action, void *data)
+{
+	int err = 0;
+
+	if (xen_hvm_domain()) {
+		uint64_t v;
+
+		err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
+		if (!err && v)
+			xen_store_evtchn = v;
+		else
+			pr_warn("Cannot update xenstore event channel: %d\n",
+				err);
+	} else
+		xen_store_evtchn = xen_start_info->store_evtchn;
+
+	return err;
+}
+
+static struct notifier_block xenbus_resume_nb = {
+	.notifier_call = xenbus_resume_cb,
+};
+
 static int __init xenbus_init(void)
 {
 	int err = 0;
@@ -793,6 +818,9 @@ static int __init xenbus_init(void)
 		goto out_error;
 	}
 
+	if (!xen_initial_domain())
+		xen_resume_notifier_register(&xenbus_resume_nb);
+
 #ifdef CONFIG_XEN_COMPAT_XENFS
 	/*
 	 * Create xenfs mountpoint in /proc for compatibility with
-- 
1.7.1

[PATCH 3/4] xen/console: Update console event channel on resume

[Boris Ostrovsky]

After a resume the hypervisor/tools may change console event
channel number. We should re-query it.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 5eb80bd..d5fedc0 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -299,11 +299,27 @@ static int xen_initial_domain_console_init(void)
 	return 0;
 }
 
+static void xen_console_update_evtchn(struct xencons_info *info)
+{
+	if (xen_hvm_domain()) {
+		uint64_t v;
+		int err;
+
+		err = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
+		if (!err && v)
+			info->evtchn = v;
+	} else
+		info->evtchn = xen_start_info->console.domU.evtchn;
+}
+
 void xen_console_resume(void)
 {
 	struct xencons_info *info = vtermno_to_xencons(HVC_COOKIE);
-	if (info != NULL && info->irq)
+	if (info != NULL && info->irq) {
+		if (!xen_initial_domain())
+			xen_console_update_evtchn(info);
 		rebind_evtchn_irq(info->evtchn, info->irq);
+	}
 }
 
 static void xencons_disconnect_backend(struct xencons_info *info)
-- 
1.7.1

[PATCH 3/4] xen/console: Update console event channel on resume

[Boris Ostrovsky]

After a resume the hypervisor/tools may change console event
channel number. We should re-query it.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 5eb80bd..d5fedc0 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -299,11 +299,27 @@ static int xen_initial_domain_console_init(void)
 	return 0;
 }
 
+static void xen_console_update_evtchn(struct xencons_info *info)
+{
+	if (xen_hvm_domain()) {
+		uint64_t v;
+		int err;
+
+		err = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
+		if (!err && v)
+			info->evtchn = v;
+	} else
+		info->evtchn = xen_start_info->console.domU.evtchn;
+}
+
 void xen_console_resume(void)
 {
 	struct xencons_info *info = vtermno_to_xencons(HVC_COOKIE);
-	if (info != NULL && info->irq)
+	if (info != NULL && info->irq) {
+		if (!xen_initial_domain())
+			xen_console_update_evtchn(info);
 		rebind_evtchn_irq(info->evtchn, info->irq);
+	}
 }
 
 static void xencons_disconnect_backend(struct xencons_info *info)
-- 
1.7.1

[PATCH 4/4] xen/events: Set irq_info->evtchn before binding the channel to CPU in __startup_pirq()

[Boris Ostrovsky]

.. because bind_evtchn_to_cpu(evtchn, cpu) will map evtchn to
'info' and pass 'info' down to xen_evtchn_port_bind_to_cpu().

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Tested-by: Annie Li <annie.li@oracle.com>
---
 drivers/xen/events/events_base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 70fba97..f81d322 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -529,8 +529,8 @@ static unsigned int __startup_pirq(unsigned int irq)
 	if (rc)
 		goto err;
 
-	bind_evtchn_to_cpu(evtchn, 0);
 	info->evtchn = evtchn;
+	bind_evtchn_to_cpu(evtchn, 0);
 
 	rc = xen_evtchn_port_setup(info);
 	if (rc)
-- 
1.7.1

[PATCH 4/4] xen/events: Set irq_info->evtchn before binding the channel to CPU in __startup_pirq()

[Boris Ostrovsky]

.. because bind_evtchn_to_cpu(evtchn, cpu) will map evtchn to
'info' and pass 'info' down to xen_evtchn_port_bind_to_cpu().

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Tested-by: Annie Li <annie.li@oracle.com>
---
 drivers/xen/events/events_base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 70fba97..f81d322 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -529,8 +529,8 @@ static unsigned int __startup_pirq(unsigned int irq)
 	if (rc)
 		goto err;
 
-	bind_evtchn_to_cpu(evtchn, 0);
 	info->evtchn = evtchn;
+	bind_evtchn_to_cpu(evtchn, 0);
 
 	rc = xen_evtchn_port_setup(info);
 	if (rc)
-- 
1.7.1

Re: [Xen-devel] [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> When a guest is resumed, the hypervisor may change event channel
> assignments. If this happens and the guest uses 2-level events it
> is possible for the interrupt to be claimed by wrong VCPU since
> cpu_evtchn_mask bits may be stale. This can happen even though
> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
> is passed in is not necessarily the original one (from pre-migration
> times) but instead is freshly allocated during resume and so any
> information about which CPU the channel was bound to is lost.
> 
> Thus we should clear the mask during resume.
> 
> We also need to make sure that bits for xenstore and console channels
> are set when these two subsystems are resumed. While rebind_evtchn_irq()
> (which is invoked for both of them on a resume) calls irq_set_affinity(),
> the latter will in fact postpone setting affinity until handling the
> interrupt. But because cpu_evtchn_mask will have bits for these two cleared
> we won't be able to take the interrupt.
> 
> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
> allowing to set affinity immediately, which is safe for event-channel-based
> interrupts.
[...]
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>  
>  		info = vtermno_to_xencons(HVC_COOKIE);
>  		info->irq = bind_evtchn_to_irq(info->evtchn);
> +		irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>  	}
>  	if (info->irq < 0)
>  		info->irq = 0; /* NO_IRQ */
> diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
> index 5db43fc..7dd4631 100644
> --- a/drivers/xen/events/events_2l.c
> +++ b/drivers/xen/events/events_2l.c
> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static void evtchn_2l_resume(void)
> +{
> +	int i;
> +
> +	for_each_online_cpu(i)
> +		memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
> +				EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
> +}
> +
>  static const struct evtchn_ops evtchn_ops_2l = {
>  	.max_channels      = evtchn_2l_max_channels,
>  	.nr_channels       = evtchn_2l_max_channels,
> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>  	.mask              = evtchn_2l_mask,
>  	.unmask            = evtchn_2l_unmask,
>  	.handle_events     = evtchn_2l_handle_events,
> +	.resume	           = evtchn_2l_resume,
>  };
>  
>  void __init xen_evtchn_2l_init(void)
> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
> index fdb0f33..30203d1 100644
> --- a/drivers/xen/xenbus/xenbus_comms.c
> +++ b/drivers/xen/xenbus/xenbus_comms.c
> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>  		}
>  
>  		xenbus_irq = err;
> +		irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);

IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
which doesn't really sound relevant to me here?

Thomas Glexnier is really not happy with mis-use of IRQ APIs.

>From the commit log the evtchn_2l_resume() fucntion that's added sounds
like it fixes the problem on its own?

David

Re: [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> When a guest is resumed, the hypervisor may change event channel
> assignments. If this happens and the guest uses 2-level events it
> is possible for the interrupt to be claimed by wrong VCPU since
> cpu_evtchn_mask bits may be stale. This can happen even though
> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
> is passed in is not necessarily the original one (from pre-migration
> times) but instead is freshly allocated during resume and so any
> information about which CPU the channel was bound to is lost.
> 
> Thus we should clear the mask during resume.
> 
> We also need to make sure that bits for xenstore and console channels
> are set when these two subsystems are resumed. While rebind_evtchn_irq()
> (which is invoked for both of them on a resume) calls irq_set_affinity(),
> the latter will in fact postpone setting affinity until handling the
> interrupt. But because cpu_evtchn_mask will have bits for these two cleared
> we won't be able to take the interrupt.
> 
> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
> allowing to set affinity immediately, which is safe for event-channel-based
> interrupts.
[...]
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>  
>  		info = vtermno_to_xencons(HVC_COOKIE);
>  		info->irq = bind_evtchn_to_irq(info->evtchn);
> +		irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>  	}
>  	if (info->irq < 0)
>  		info->irq = 0; /* NO_IRQ */
> diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
> index 5db43fc..7dd4631 100644
> --- a/drivers/xen/events/events_2l.c
> +++ b/drivers/xen/events/events_2l.c
> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static void evtchn_2l_resume(void)
> +{
> +	int i;
> +
> +	for_each_online_cpu(i)
> +		memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
> +				EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
> +}
> +
>  static const struct evtchn_ops evtchn_ops_2l = {
>  	.max_channels      = evtchn_2l_max_channels,
>  	.nr_channels       = evtchn_2l_max_channels,
> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>  	.mask              = evtchn_2l_mask,
>  	.unmask            = evtchn_2l_unmask,
>  	.handle_events     = evtchn_2l_handle_events,
> +	.resume	           = evtchn_2l_resume,
>  };
>  
>  void __init xen_evtchn_2l_init(void)
> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
> index fdb0f33..30203d1 100644
> --- a/drivers/xen/xenbus/xenbus_comms.c
> +++ b/drivers/xen/xenbus/xenbus_comms.c
> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>  		}
>  
>  		xenbus_irq = err;
> +		irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);

IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
which doesn't really sound relevant to me here?

Thomas Glexnier is really not happy with mis-use of IRQ APIs.

>From the commit log the evtchn_2l_resume() fucntion that's added sounds
like it fixes the problem on its own?

David

Re: [Xen-devel] [PATCH 2/4] xen/xenbus: Update xenbus event channel on resume

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> After a resume the hypervisor/tools may change xenbus event
> channel number. We should re-query it.
[...]
> @@ -793,6 +818,9 @@ static int __init xenbus_init(void)
>  		goto out_error;
>  	}
>  
> +	if (!xen_initial_domain())
> +		xen_resume_notifier_register(&xenbus_resume_nb);

I think this needs to be xenstore_domain_type != XS_LOCAL, to match
xenbus_init().

David

Re: [PATCH 2/4] xen/xenbus: Update xenbus event channel on resume

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> After a resume the hypervisor/tools may change xenbus event
> channel number. We should re-query it.
[...]
> @@ -793,6 +818,9 @@ static int __init xenbus_init(void)
>  		goto out_error;
>  	}
>  
> +	if (!xen_initial_domain())
> +		xen_resume_notifier_register(&xenbus_resume_nb);

I think this needs to be xenstore_domain_type != XS_LOCAL, to match
xenbus_init().

David

Re: [Xen-devel] [PATCH 3/4] xen/console: Update console event channel on resume

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> After a resume the hypervisor/tools may change console event
> channel number. We should re-query it.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 3/4] xen/console: Update console event channel on resume

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> After a resume the hypervisor/tools may change console event
> channel number. We should re-query it.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [Xen-devel] [PATCH 4/4] xen/events: Set irq_info->evtchn before binding the channel to CPU in __startup_pirq()

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> .. because bind_evtchn_to_cpu(evtchn, cpu) will map evtchn to
> 'info' and pass 'info' down to xen_evtchn_port_bind_to_cpu().

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 4/4] xen/events: Set irq_info->evtchn before binding the channel to CPU in __startup_pirq()

[David Vrabel]

On 28/04/15 16:52, Boris Ostrovsky wrote:
> .. because bind_evtchn_to_cpu(evtchn, cpu) will map evtchn to
> 'info' and pass 'info' down to xen_evtchn_port_bind_to_cpu().

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [Xen-devel] [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

On 04/28/2015 12:28 PM, David Vrabel wrote:
> On 28/04/15 16:52, Boris Ostrovsky wrote:
>> When a guest is resumed, the hypervisor may change event channel
>> assignments. If this happens and the guest uses 2-level events it
>> is possible for the interrupt to be claimed by wrong VCPU since
>> cpu_evtchn_mask bits may be stale. This can happen even though
>> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
>> is passed in is not necessarily the original one (from pre-migration
>> times) but instead is freshly allocated during resume and so any
>> information about which CPU the channel was bound to is lost.
>>
>> Thus we should clear the mask during resume.
>>
>> We also need to make sure that bits for xenstore and console channels
>> are set when these two subsystems are resumed. While rebind_evtchn_irq()
>> (which is invoked for both of them on a resume) calls irq_set_affinity(),
>> the latter will in fact postpone setting affinity until handling the
>> interrupt. But because cpu_evtchn_mask will have bits for these two cleared
>> we won't be able to take the interrupt.
>>
>> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
>> allowing to set affinity immediately, which is safe for event-channel-based
>> interrupts.
> [...]
>> --- a/drivers/tty/hvc/hvc_xen.c
>> +++ b/drivers/tty/hvc/hvc_xen.c
>> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>>   
>>   		info = vtermno_to_xencons(HVC_COOKIE);
>>   		info->irq = bind_evtchn_to_irq(info->evtchn);
>> +		irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>>   	}
>>   	if (info->irq < 0)
>>   		info->irq = 0; /* NO_IRQ */
>> diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
>> index 5db43fc..7dd4631 100644
>> --- a/drivers/xen/events/events_2l.c
>> +++ b/drivers/xen/events/events_2l.c
>> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +static void evtchn_2l_resume(void)
>> +{
>> +	int i;
>> +
>> +	for_each_online_cpu(i)
>> +		memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
>> +				EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
>> +}
>> +
>>   static const struct evtchn_ops evtchn_ops_2l = {
>>   	.max_channels      = evtchn_2l_max_channels,
>>   	.nr_channels       = evtchn_2l_max_channels,
>> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>>   	.mask              = evtchn_2l_mask,
>>   	.unmask            = evtchn_2l_unmask,
>>   	.handle_events     = evtchn_2l_handle_events,
>> +	.resume	           = evtchn_2l_resume,
>>   };
>>   
>>   void __init xen_evtchn_2l_init(void)
>> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
>> index fdb0f33..30203d1 100644
>> --- a/drivers/xen/xenbus/xenbus_comms.c
>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>>   		}
>>   
>>   		xenbus_irq = err;
>> +		irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
> IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
> which doesn't really sound relevant to me here?
>
> Thomas Glexnier is really not happy with mis-use of IRQ APIs.

Yes, this is somewhat expanding definition of IRQ_MOVE_PCNTXT but then 
there are other devices that do the same (HPET, for one).

>
>  From the commit log the evtchn_2l_resume() fucntion that's added sounds
> like it fixes the problem on its own?

It in fact makes this problem worse since now that cpu_evtchn_mask is 
cleared during resume we cannot process the interrupt anymore in 
evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for 
an interrupt to be processed.

We already have this issue even without clearing the mask if, say, 
xenstore channel changes or if it gets bound to cpu other than 0 before 
suspend. It's just that we rarely (if ever) see this happen.

We can explicitly call events_base.c:set_affinity_irq() from 
rebind_evtchn_irq() after irq_set_affinity() if we want to avoid using 
IRQ_MOVE_PCNTXT flag but that will also looks kind of strange.


-boris

Re: [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

On 04/28/2015 12:28 PM, David Vrabel wrote:
> On 28/04/15 16:52, Boris Ostrovsky wrote:
>> When a guest is resumed, the hypervisor may change event channel
>> assignments. If this happens and the guest uses 2-level events it
>> is possible for the interrupt to be claimed by wrong VCPU since
>> cpu_evtchn_mask bits may be stale. This can happen even though
>> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
>> is passed in is not necessarily the original one (from pre-migration
>> times) but instead is freshly allocated during resume and so any
>> information about which CPU the channel was bound to is lost.
>>
>> Thus we should clear the mask during resume.
>>
>> We also need to make sure that bits for xenstore and console channels
>> are set when these two subsystems are resumed. While rebind_evtchn_irq()
>> (which is invoked for both of them on a resume) calls irq_set_affinity(),
>> the latter will in fact postpone setting affinity until handling the
>> interrupt. But because cpu_evtchn_mask will have bits for these two cleared
>> we won't be able to take the interrupt.
>>
>> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
>> allowing to set affinity immediately, which is safe for event-channel-based
>> interrupts.
> [...]
>> --- a/drivers/tty/hvc/hvc_xen.c
>> +++ b/drivers/tty/hvc/hvc_xen.c
>> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>>   
>>   		info = vtermno_to_xencons(HVC_COOKIE);
>>   		info->irq = bind_evtchn_to_irq(info->evtchn);
>> +		irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>>   	}
>>   	if (info->irq < 0)
>>   		info->irq = 0; /* NO_IRQ */
>> diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
>> index 5db43fc..7dd4631 100644
>> --- a/drivers/xen/events/events_2l.c
>> +++ b/drivers/xen/events/events_2l.c
>> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +static void evtchn_2l_resume(void)
>> +{
>> +	int i;
>> +
>> +	for_each_online_cpu(i)
>> +		memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
>> +				EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
>> +}
>> +
>>   static const struct evtchn_ops evtchn_ops_2l = {
>>   	.max_channels      = evtchn_2l_max_channels,
>>   	.nr_channels       = evtchn_2l_max_channels,
>> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>>   	.mask              = evtchn_2l_mask,
>>   	.unmask            = evtchn_2l_unmask,
>>   	.handle_events     = evtchn_2l_handle_events,
>> +	.resume	           = evtchn_2l_resume,
>>   };
>>   
>>   void __init xen_evtchn_2l_init(void)
>> diff --git a/drivers/xen/xenbus/xenbus_comms.c b/drivers/xen/xenbus/xenbus_comms.c
>> index fdb0f33..30203d1 100644
>> --- a/drivers/xen/xenbus/xenbus_comms.c
>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>>   		}
>>   
>>   		xenbus_irq = err;
>> +		irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
> IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
> which doesn't really sound relevant to me here?
>
> Thomas Glexnier is really not happy with mis-use of IRQ APIs.

Yes, this is somewhat expanding definition of IRQ_MOVE_PCNTXT but then 
there are other devices that do the same (HPET, for one).

>
>  From the commit log the evtchn_2l_resume() fucntion that's added sounds
> like it fixes the problem on its own?

It in fact makes this problem worse since now that cpu_evtchn_mask is 
cleared during resume we cannot process the interrupt anymore in 
evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for 
an interrupt to be processed.

We already have this issue even without clearing the mask if, say, 
xenstore channel changes or if it gets bound to cpu other than 0 before 
suspend. It's just that we rarely (if ever) see this happen.

We can explicitly call events_base.c:set_affinity_irq() from 
rebind_evtchn_irq() after irq_set_affinity() if we want to avoid using 
IRQ_MOVE_PCNTXT flag but that will also looks kind of strange.


-boris

Re: [Xen-devel] [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[David Vrabel]

On 28/04/15 19:29, Boris Ostrovsky wrote:
> On 04/28/2015 12:28 PM, David Vrabel wrote:
>> On 28/04/15 16:52, Boris Ostrovsky wrote:
>>> When a guest is resumed, the hypervisor may change event channel
>>> assignments. If this happens and the guest uses 2-level events it
>>> is possible for the interrupt to be claimed by wrong VCPU since
>>> cpu_evtchn_mask bits may be stale. This can happen even though
>>> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
>>> is passed in is not necessarily the original one (from pre-migration
>>> times) but instead is freshly allocated during resume and so any
>>> information about which CPU the channel was bound to is lost.
>>>
>>> Thus we should clear the mask during resume.
>>>
>>> We also need to make sure that bits for xenstore and console channels
>>> are set when these two subsystems are resumed. While rebind_evtchn_irq()
>>> (which is invoked for both of them on a resume) calls
>>> irq_set_affinity(),
>>> the latter will in fact postpone setting affinity until handling the
>>> interrupt. But because cpu_evtchn_mask will have bits for these two
>>> cleared
>>> we won't be able to take the interrupt.
>>>
>>> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
>>> allowing to set affinity immediately, which is safe for
>>> event-channel-based
>>> interrupts.
>> [...]
>>> --- a/drivers/tty/hvc/hvc_xen.c
>>> +++ b/drivers/tty/hvc/hvc_xen.c
>>> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>>>             info = vtermno_to_xencons(HVC_COOKIE);
>>>           info->irq = bind_evtchn_to_irq(info->evtchn);
>>> +        irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>>>       }
>>>       if (info->irq < 0)
>>>           info->irq = 0; /* NO_IRQ */
>>> diff --git a/drivers/xen/events/events_2l.c
>>> b/drivers/xen/events/events_2l.c
>>> index 5db43fc..7dd4631 100644
>>> --- a/drivers/xen/events/events_2l.c
>>> +++ b/drivers/xen/events/events_2l.c
>>> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void
>>> *dev_id)
>>>       return IRQ_HANDLED;
>>>   }
>>>   +static void evtchn_2l_resume(void)
>>> +{
>>> +    int i;
>>> +
>>> +    for_each_online_cpu(i)
>>> +        memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
>>> +                EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
>>> +}
>>> +
>>>   static const struct evtchn_ops evtchn_ops_2l = {
>>>       .max_channels      = evtchn_2l_max_channels,
>>>       .nr_channels       = evtchn_2l_max_channels,
>>> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>>>       .mask              = evtchn_2l_mask,
>>>       .unmask            = evtchn_2l_unmask,
>>>       .handle_events     = evtchn_2l_handle_events,
>>> +    .resume               = evtchn_2l_resume,
>>>   };
>>>     void __init xen_evtchn_2l_init(void)
>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c
>>> b/drivers/xen/xenbus/xenbus_comms.c
>>> index fdb0f33..30203d1 100644
>>> --- a/drivers/xen/xenbus/xenbus_comms.c
>>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>>> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>>>           }
>>>             xenbus_irq = err;
>>> +        irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
>> IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
>> which doesn't really sound relevant to me here?
>>
>> Thomas Glexnier is really not happy with mis-use of IRQ APIs.
> 
> Yes, this is somewhat expanding definition of IRQ_MOVE_PCNTXT but then
> there are other devices that do the same (HPET, for one).

Ok.  But why set it on only these two event channels and not every one?

>>  From the commit log the evtchn_2l_resume() fucntion that's added sounds
>> like it fixes the problem on its own?
> 
> It in fact makes this problem worse since now that cpu_evtchn_mask is
> cleared during resume we cannot process the interrupt anymore in
> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
> an interrupt to be processed.

Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
event channels? And then you wouldn't need IRQ_MOVE_PCNTX.

> We already have this issue even without clearing the mask if, say,
> xenstore channel changes or if it gets bound to cpu other than 0 before
> suspend. It's just that we rarely (if ever) see this happen.
> 
> We can explicitly call events_base.c:set_affinity_irq() from
> rebind_evtchn_irq() after irq_set_affinity() if we want to avoid using
> IRQ_MOVE_PCNTXT flag but that will also looks kind of strange.

David

Re: [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[David Vrabel]

On 28/04/15 19:29, Boris Ostrovsky wrote:
> On 04/28/2015 12:28 PM, David Vrabel wrote:
>> On 28/04/15 16:52, Boris Ostrovsky wrote:
>>> When a guest is resumed, the hypervisor may change event channel
>>> assignments. If this happens and the guest uses 2-level events it
>>> is possible for the interrupt to be claimed by wrong VCPU since
>>> cpu_evtchn_mask bits may be stale. This can happen even though
>>> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
>>> is passed in is not necessarily the original one (from pre-migration
>>> times) but instead is freshly allocated during resume and so any
>>> information about which CPU the channel was bound to is lost.
>>>
>>> Thus we should clear the mask during resume.
>>>
>>> We also need to make sure that bits for xenstore and console channels
>>> are set when these two subsystems are resumed. While rebind_evtchn_irq()
>>> (which is invoked for both of them on a resume) calls
>>> irq_set_affinity(),
>>> the latter will in fact postpone setting affinity until handling the
>>> interrupt. But because cpu_evtchn_mask will have bits for these two
>>> cleared
>>> we won't be able to take the interrupt.
>>>
>>> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
>>> allowing to set affinity immediately, which is safe for
>>> event-channel-based
>>> interrupts.
>> [...]
>>> --- a/drivers/tty/hvc/hvc_xen.c
>>> +++ b/drivers/tty/hvc/hvc_xen.c
>>> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>>>             info = vtermno_to_xencons(HVC_COOKIE);
>>>           info->irq = bind_evtchn_to_irq(info->evtchn);
>>> +        irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>>>       }
>>>       if (info->irq < 0)
>>>           info->irq = 0; /* NO_IRQ */
>>> diff --git a/drivers/xen/events/events_2l.c
>>> b/drivers/xen/events/events_2l.c
>>> index 5db43fc..7dd4631 100644
>>> --- a/drivers/xen/events/events_2l.c
>>> +++ b/drivers/xen/events/events_2l.c
>>> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void
>>> *dev_id)
>>>       return IRQ_HANDLED;
>>>   }
>>>   +static void evtchn_2l_resume(void)
>>> +{
>>> +    int i;
>>> +
>>> +    for_each_online_cpu(i)
>>> +        memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
>>> +                EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
>>> +}
>>> +
>>>   static const struct evtchn_ops evtchn_ops_2l = {
>>>       .max_channels      = evtchn_2l_max_channels,
>>>       .nr_channels       = evtchn_2l_max_channels,
>>> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>>>       .mask              = evtchn_2l_mask,
>>>       .unmask            = evtchn_2l_unmask,
>>>       .handle_events     = evtchn_2l_handle_events,
>>> +    .resume               = evtchn_2l_resume,
>>>   };
>>>     void __init xen_evtchn_2l_init(void)
>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c
>>> b/drivers/xen/xenbus/xenbus_comms.c
>>> index fdb0f33..30203d1 100644
>>> --- a/drivers/xen/xenbus/xenbus_comms.c
>>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>>> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>>>           }
>>>             xenbus_irq = err;
>>> +        irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
>> IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
>> which doesn't really sound relevant to me here?
>>
>> Thomas Glexnier is really not happy with mis-use of IRQ APIs.
> 
> Yes, this is somewhat expanding definition of IRQ_MOVE_PCNTXT but then
> there are other devices that do the same (HPET, for one).

Ok.  But why set it on only these two event channels and not every one?

>>  From the commit log the evtchn_2l_resume() fucntion that's added sounds
>> like it fixes the problem on its own?
> 
> It in fact makes this problem worse since now that cpu_evtchn_mask is
> cleared during resume we cannot process the interrupt anymore in
> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
> an interrupt to be processed.

Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
event channels? And then you wouldn't need IRQ_MOVE_PCNTX.

> We already have this issue even without clearing the mask if, say,
> xenstore channel changes or if it gets bound to cpu other than 0 before
> suspend. It's just that we rarely (if ever) see this happen.
> 
> We can explicitly call events_base.c:set_affinity_irq() from
> rebind_evtchn_irq() after irq_set_affinity() if we want to avoid using
> IRQ_MOVE_PCNTXT flag but that will also looks kind of strange.

David

Re: [Xen-devel] [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

On 04/29/2015 12:32 PM, David Vrabel wrote:
> On 28/04/15 19:29, Boris Ostrovsky wrote:
>> On 04/28/2015 12:28 PM, David Vrabel wrote:
>>> On 28/04/15 16:52, Boris Ostrovsky wrote:
>>>> When a guest is resumed, the hypervisor may change event channel
>>>> assignments. If this happens and the guest uses 2-level events it
>>>> is possible for the interrupt to be claimed by wrong VCPU since
>>>> cpu_evtchn_mask bits may be stale. This can happen even though
>>>> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
>>>> is passed in is not necessarily the original one (from pre-migration
>>>> times) but instead is freshly allocated during resume and so any
>>>> information about which CPU the channel was bound to is lost.
>>>>
>>>> Thus we should clear the mask during resume.
>>>>
>>>> We also need to make sure that bits for xenstore and console channels
>>>> are set when these two subsystems are resumed. While rebind_evtchn_irq()
>>>> (which is invoked for both of them on a resume) calls
>>>> irq_set_affinity(),
>>>> the latter will in fact postpone setting affinity until handling the
>>>> interrupt. But because cpu_evtchn_mask will have bits for these two
>>>> cleared
>>>> we won't be able to take the interrupt.
>>>>
>>>> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
>>>> allowing to set affinity immediately, which is safe for
>>>> event-channel-based
>>>> interrupts.
>>> [...]
>>>> --- a/drivers/tty/hvc/hvc_xen.c
>>>> +++ b/drivers/tty/hvc/hvc_xen.c
>>>> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>>>>              info = vtermno_to_xencons(HVC_COOKIE);
>>>>            info->irq = bind_evtchn_to_irq(info->evtchn);
>>>> +        irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>>>>        }
>>>>        if (info->irq < 0)
>>>>            info->irq = 0; /* NO_IRQ */
>>>> diff --git a/drivers/xen/events/events_2l.c
>>>> b/drivers/xen/events/events_2l.c
>>>> index 5db43fc..7dd4631 100644
>>>> --- a/drivers/xen/events/events_2l.c
>>>> +++ b/drivers/xen/events/events_2l.c
>>>> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void
>>>> *dev_id)
>>>>        return IRQ_HANDLED;
>>>>    }
>>>>    +static void evtchn_2l_resume(void)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for_each_online_cpu(i)
>>>> +        memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
>>>> +                EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
>>>> +}
>>>> +
>>>>    static const struct evtchn_ops evtchn_ops_2l = {
>>>>        .max_channels      = evtchn_2l_max_channels,
>>>>        .nr_channels       = evtchn_2l_max_channels,
>>>> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>>>>        .mask              = evtchn_2l_mask,
>>>>        .unmask            = evtchn_2l_unmask,
>>>>        .handle_events     = evtchn_2l_handle_events,
>>>> +    .resume               = evtchn_2l_resume,
>>>>    };
>>>>      void __init xen_evtchn_2l_init(void)
>>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c
>>>> b/drivers/xen/xenbus/xenbus_comms.c
>>>> index fdb0f33..30203d1 100644
>>>> --- a/drivers/xen/xenbus/xenbus_comms.c
>>>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>>>> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>>>>            }
>>>>              xenbus_irq = err;
>>>> +        irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
>>> IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
>>> which doesn't really sound relevant to me here?
>>>
>>> Thomas Glexnier is really not happy with mis-use of IRQ APIs.
>> Yes, this is somewhat expanding definition of IRQ_MOVE_PCNTXT but then
>> there are other devices that do the same (HPET, for one).
> Ok.  But why set it on only these two event channels and not every one?

On resume in xen_irq_resume() we do

         for_each_possible_cpu(cpu) {
                 restore_cpu_virqs(cpu);
                 restore_cpu_ipis(cpu);
         }
         restore_pirqs();

And console and xenstore are neither of the those three types. 
rebind_irq_to_cpu() essentially plays the role of these restore_*() 
routines, except it does not make the hypercall to let the hypervisor 
assign notify_vcpu_id and it doesn't do explicit bind_evtchn_to_cpu().

Which makes me think that perhaps we should modify rebind_irq_to_cpu() 
to in fact do these two things. We may still need to keep 
irq_set_affinity() so that generic affinity data is properly set up but 
this will be deferred until the first interrupt.

>
>>>   From the commit log the evtchn_2l_resume() fucntion that's added sounds
>>> like it fixes the problem on its own?
>> It in fact makes this problem worse since now that cpu_evtchn_mask is
>> cleared during resume we cannot process the interrupt anymore in
>> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
>> an interrupt to be processed.
> Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
> event channels? And then you wouldn't need IRQ_MOVE_PCNTX.

But then (at least in 2-level case) more than one VCPUs may pick the 
same interrupt, won't they? Because the local cpu mask is what tells a 
VCPU that it is allowed to claim an interrupt.

-boris

>
>> We already have this issue even without clearing the mask if, say,
>> xenstore channel changes or if it gets bound to cpu other than 0 before
>> suspend. It's just that we rarely (if ever) see this happen.
>>
>> We can explicitly call events_base.c:set_affinity_irq() from
>> rebind_evtchn_irq() after irq_set_affinity() if we want to avoid using
>> IRQ_MOVE_PCNTXT flag but that will also looks kind of strange.
> David

Re: [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

On 04/29/2015 12:32 PM, David Vrabel wrote:
> On 28/04/15 19:29, Boris Ostrovsky wrote:
>> On 04/28/2015 12:28 PM, David Vrabel wrote:
>>> On 28/04/15 16:52, Boris Ostrovsky wrote:
>>>> When a guest is resumed, the hypervisor may change event channel
>>>> assignments. If this happens and the guest uses 2-level events it
>>>> is possible for the interrupt to be claimed by wrong VCPU since
>>>> cpu_evtchn_mask bits may be stale. This can happen even though
>>>> evtchn_2l_bind_to_cpu() attempts to clear old bits: irq_info that
>>>> is passed in is not necessarily the original one (from pre-migration
>>>> times) but instead is freshly allocated during resume and so any
>>>> information about which CPU the channel was bound to is lost.
>>>>
>>>> Thus we should clear the mask during resume.
>>>>
>>>> We also need to make sure that bits for xenstore and console channels
>>>> are set when these two subsystems are resumed. While rebind_evtchn_irq()
>>>> (which is invoked for both of them on a resume) calls
>>>> irq_set_affinity(),
>>>> the latter will in fact postpone setting affinity until handling the
>>>> interrupt. But because cpu_evtchn_mask will have bits for these two
>>>> cleared
>>>> we won't be able to take the interrupt.
>>>>
>>>> Setting IRQ_MOVE_PCNTXT flag for the two irqs avoids this problem by
>>>> allowing to set affinity immediately, which is safe for
>>>> event-channel-based
>>>> interrupts.
>>> [...]
>>>> --- a/drivers/tty/hvc/hvc_xen.c
>>>> +++ b/drivers/tty/hvc/hvc_xen.c
>>>> @@ -533,6 +533,7 @@ static int __init xen_hvc_init(void)
>>>>              info = vtermno_to_xencons(HVC_COOKIE);
>>>>            info->irq = bind_evtchn_to_irq(info->evtchn);
>>>> +        irq_set_status_flags(info->irq, IRQ_MOVE_PCNTXT);
>>>>        }
>>>>        if (info->irq < 0)
>>>>            info->irq = 0; /* NO_IRQ */
>>>> diff --git a/drivers/xen/events/events_2l.c
>>>> b/drivers/xen/events/events_2l.c
>>>> index 5db43fc..7dd4631 100644
>>>> --- a/drivers/xen/events/events_2l.c
>>>> +++ b/drivers/xen/events/events_2l.c
>>>> @@ -345,6 +345,15 @@ irqreturn_t xen_debug_interrupt(int irq, void
>>>> *dev_id)
>>>>        return IRQ_HANDLED;
>>>>    }
>>>>    +static void evtchn_2l_resume(void)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for_each_online_cpu(i)
>>>> +        memset(per_cpu(cpu_evtchn_mask, i), 0, sizeof(xen_ulong_t) *
>>>> +                EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
>>>> +}
>>>> +
>>>>    static const struct evtchn_ops evtchn_ops_2l = {
>>>>        .max_channels      = evtchn_2l_max_channels,
>>>>        .nr_channels       = evtchn_2l_max_channels,
>>>> @@ -356,6 +365,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
>>>>        .mask              = evtchn_2l_mask,
>>>>        .unmask            = evtchn_2l_unmask,
>>>>        .handle_events     = evtchn_2l_handle_events,
>>>> +    .resume               = evtchn_2l_resume,
>>>>    };
>>>>      void __init xen_evtchn_2l_init(void)
>>>> diff --git a/drivers/xen/xenbus/xenbus_comms.c
>>>> b/drivers/xen/xenbus/xenbus_comms.c
>>>> index fdb0f33..30203d1 100644
>>>> --- a/drivers/xen/xenbus/xenbus_comms.c
>>>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>>>> @@ -231,6 +231,7 @@ int xb_init_comms(void)
>>>>            }
>>>>              xenbus_irq = err;
>>>> +        irq_set_status_flags(xenbus_irq, IRQ_MOVE_PCNTXT);
>>> IRQ_MOVE_PCNTXT means "Interrupt can be migrated from process context"
>>> which doesn't really sound relevant to me here?
>>>
>>> Thomas Glexnier is really not happy with mis-use of IRQ APIs.
>> Yes, this is somewhat expanding definition of IRQ_MOVE_PCNTXT but then
>> there are other devices that do the same (HPET, for one).
> Ok.  But why set it on only these two event channels and not every one?

On resume in xen_irq_resume() we do

         for_each_possible_cpu(cpu) {
                 restore_cpu_virqs(cpu);
                 restore_cpu_ipis(cpu);
         }
         restore_pirqs();

And console and xenstore are neither of the those three types. 
rebind_irq_to_cpu() essentially plays the role of these restore_*() 
routines, except it does not make the hypercall to let the hypervisor 
assign notify_vcpu_id and it doesn't do explicit bind_evtchn_to_cpu().

Which makes me think that perhaps we should modify rebind_irq_to_cpu() 
to in fact do these two things. We may still need to keep 
irq_set_affinity() so that generic affinity data is properly set up but 
this will be deferred until the first interrupt.

>
>>>   From the commit log the evtchn_2l_resume() fucntion that's added sounds
>>> like it fixes the problem on its own?
>> It in fact makes this problem worse since now that cpu_evtchn_mask is
>> cleared during resume we cannot process the interrupt anymore in
>> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
>> an interrupt to be processed.
> Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
> event channels? And then you wouldn't need IRQ_MOVE_PCNTX.

But then (at least in 2-level case) more than one VCPUs may pick the 
same interrupt, won't they? Because the local cpu mask is what tells a 
VCPU that it is allowed to claim an interrupt.

-boris

>
>> We already have this issue even without clearing the mask if, say,
>> xenstore channel changes or if it gets bound to cpu other than 0 before
>> suspend. It's just that we rarely (if ever) see this happen.
>>
>> We can explicitly call events_base.c:set_affinity_irq() from
>> rebind_evtchn_irq() after irq_set_affinity() if we want to avoid using
>> IRQ_MOVE_PCNTXT flag but that will also looks kind of strange.
> David

Re: [Xen-devel] [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[David Vrabel]

On 29/04/15 17:54, Boris Ostrovsky wrote:
> On 04/29/2015 12:32 PM, David Vrabel wrote:
>> On 28/04/15 19:29, Boris Ostrovsky wrote:
>>> On 04/28/2015 12:28 PM, David Vrabel wrote:
>>>> 
>>>>   From the commit log the evtchn_2l_resume() fucntion that's added
>>>> sounds
>>>> like it fixes the problem on its own?
>>> It in fact makes this problem worse since now that cpu_evtchn_mask is
>>> cleared during resume we cannot process the interrupt anymore in
>>> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
>>> an interrupt to be processed.
>> Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
>> event channels? And then you wouldn't need IRQ_MOVE_PCNTX.
> 
> But then (at least in 2-level case) more than one VCPUs may pick the
> same interrupt, won't they? Because the local cpu mask is what tells a
> VCPU that it is allowed to claim an interrupt.


We know that all event channels at this point are on VCPU0 (right?) so
we only set the bit in that VCPU's mask.

David

Re: [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[David Vrabel]

On 29/04/15 17:54, Boris Ostrovsky wrote:
> On 04/29/2015 12:32 PM, David Vrabel wrote:
>> On 28/04/15 19:29, Boris Ostrovsky wrote:
>>> On 04/28/2015 12:28 PM, David Vrabel wrote:
>>>> 
>>>>   From the commit log the evtchn_2l_resume() fucntion that's added
>>>> sounds
>>>> like it fixes the problem on its own?
>>> It in fact makes this problem worse since now that cpu_evtchn_mask is
>>> cleared during resume we cannot process the interrupt anymore in
>>> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
>>> an interrupt to be processed.
>> Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
>> event channels? And then you wouldn't need IRQ_MOVE_PCNTX.
> 
> But then (at least in 2-level case) more than one VCPUs may pick the
> same interrupt, won't they? Because the local cpu mask is what tells a
> VCPU that it is allowed to claim an interrupt.


We know that all event channels at this point are on VCPU0 (right?) so
we only set the bit in that VCPU's mask.

David

Re: [Xen-devel] [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

On 04/29/2015 01:59 PM, David Vrabel wrote:
> On 29/04/15 17:54, Boris Ostrovsky wrote:
>> On 04/29/2015 12:32 PM, David Vrabel wrote:
>>> On 28/04/15 19:29, Boris Ostrovsky wrote:
>>>> On 04/28/2015 12:28 PM, David Vrabel wrote:
>>>>>    From the commit log the evtchn_2l_resume() fucntion that's added
>>>>> sounds
>>>>> like it fixes the problem on its own?
>>>> It in fact makes this problem worse since now that cpu_evtchn_mask is
>>>> cleared during resume we cannot process the interrupt anymore in
>>>> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
>>>> an interrupt to be processed.
>>> Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
>>> event channels? And then you wouldn't need IRQ_MOVE_PCNTX.
>> But then (at least in 2-level case) more than one VCPUs may pick the
>> same interrupt, won't they? Because the local cpu mask is what tells a
>> VCPU that it is allowed to claim an interrupt.
>
> We know that all event channels at this point are on VCPU0 (right?) so
> we only set the bit in that VCPU's mask.

This is pretty much what I was suggesting since setting the mask is done 
via bind_evtchn_to_cpu(). Except that I also want to call 
EVTCHNOP_bind_vcpu before that, just in case.

-boris

Re: [PATCH 1/4] xen/events: Clear cpu_evtchn_mask before resuming

[Boris Ostrovsky]

On 04/29/2015 01:59 PM, David Vrabel wrote:
> On 29/04/15 17:54, Boris Ostrovsky wrote:
>> On 04/29/2015 12:32 PM, David Vrabel wrote:
>>> On 28/04/15 19:29, Boris Ostrovsky wrote:
>>>> On 04/28/2015 12:28 PM, David Vrabel wrote:
>>>>>    From the commit log the evtchn_2l_resume() fucntion that's added
>>>>> sounds
>>>>> like it fixes the problem on its own?
>>>> It in fact makes this problem worse since now that cpu_evtchn_mask is
>>>> cleared during resume we cannot process the interrupt anymore in
>>>> evtchn_2l_handle_events(): irqs have to be bound to a cpu in order for
>>>> an interrupt to be processed.
>>> Perhaps evtchn_2l_resume() should set the local cpu mask for any bound
>>> event channels? And then you wouldn't need IRQ_MOVE_PCNTX.
>> But then (at least in 2-level case) more than one VCPUs may pick the
>> same interrupt, won't they? Because the local cpu mask is what tells a
>> VCPU that it is allowed to claim an interrupt.
>
> We know that all event channels at this point are on VCPU0 (right?) so
> we only set the bit in that VCPU's mask.

This is pretty much what I was suggesting since setting the mask is done 
via bind_evtchn_to_cpu(). Except that I also want to call 
EVTCHNOP_bind_vcpu before that, just in case.

-boris

[PATCH 0/4] A few event channel-related fixes

[Boris Ostrovsky]

Fixes for issues that we discovered during live migration when source and
target systems have different event channel assignments for guests.

Boris Ostrovsky (4):
  xen/events: Clear cpu_evtchn_mask before resuming
  xen/xenbus: Update xenbus event channel on resume
  xen/console: Update console event channel on resume
  xen/events: Set irq_info->evtchn before binding the channel to CPU in
    __startup_pirq()

 drivers/tty/hvc/hvc_xen.c         |   19 ++++++++++++++++++-
 drivers/xen/events/events_2l.c    |   10 ++++++++++
 drivers/xen/events/events_base.c  |    2 +-
 drivers/xen/xenbus/xenbus_comms.c |    1 +
 drivers/xen/xenbus/xenbus_probe.c |   28 ++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 2 deletions(-)