Xen devel - Jun 2012 - [PATCH WIP 0/6] xen/arm: PV console support

Hi all,
this patch series fixes few Xen on ARM bugs as well as one generic Xen PV
on HVM bug (I''ll send out that patch separately).
It also implements a better way to initialize Xen on ARM and find out
whether we are a privileged or an unprivileged guest.

With this patch series applied to Linux and the corresponding Xen patch series
applied to Xen, I can boot a Linux guest up to userspace and connect to
it using a PV console in dom0.

The patch series is based on Ian''s Linux for Xen on ARM tree
(git://xenbits.xen.org/people/ianc/linux-2.6.git devel/arm-hacks).


Stefano Stabellini (6):
      xen/arm: fix the shared_info and vcpu_info structs
      xen/arm: Introduce xen_guest_init
      xen/arm: get privilege status
      xen/arm: implement hvm_op
      xen: fix unmask_evtchn for HVM guests
      xen/arm: enable evtchn irqs

 arch/arm/include/asm/xen/hypercall.h |    2 +-
 arch/arm/include/asm/xen/interface.h |   15 +++------------
 arch/arm/xen/enlighten.c             |   21 ++++++++++++++++-----
 arch/x86/xen/enlighten.c             |    8 ++++++++
 drivers/tty/hvc/hvc_xen.c            |    3 +++
 drivers/xen/events.c                 |   10 ++++++++--
 include/xen/interface/features.h     |    3 +++
 include/xen/xen.h                    |    2 ++
 8 files changed, 44 insertions(+), 20 deletions(-)

git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git xenarmv7-2

Cheers,

Stefano

Fix the shared_info and vcpu_info struct definitions to match the ones
in Xen.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/include/asm/xen/interface.h |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
index 3ad2d4b..8ab7cebb 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -40,19 +40,10 @@ DEFINE_GUEST_HANDLE(xen_pfn_t);
 #endif
 
 /* Maximum number of virtual CPUs in multi-processor guests. */
-#define MAX_VIRT_CPUS 32
+#define MAX_VIRT_CPUS 1
 
-struct arch_vcpu_info {
-    unsigned long cr2;
-    unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
-};
-
-struct arch_shared_info {
-    unsigned long max_pfn;                  /* max pfn that appears in table */
-    /* Frame containing list of mfns containing list of mfns containing p2m. */
-    unsigned long pfn_to_mfn_frame_list_list;
-    unsigned long nmi_reason;
-};
+struct arch_vcpu_info { };
+struct arch_shared_info { };
 
 /* XXX: Move pvclock definitions some place arch independent */
 struct pvclock_vcpu_time_info {
-- 
1.7.2.5

We used to rely on a core_initcall to initialize Xen on ARM, however
core_initcalls are actually called after early consoles are initialized.
That means that hvc_xen.c is going to be initialized before Xen.

Given the lack of a better alternative, just call a new Xen
initialization function (xen_guest_init) from xen_cons_init.

xen_guest_init has to be arch independant, so write both an ARM and an
x86 implementation. The x86 implementation is currently empty because we
can be sure that xen_hvm_guest_init is called early enough.

Probably we can get rid of this as soon as we have better DT support.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/xen/enlighten.c  |   12 ++++++++----
 arch/x86/xen/enlighten.c  |    8 ++++++++
 drivers/tty/hvc/hvc_xen.c |    3 +++
 include/xen/xen.h         |    2 ++
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 7d76962..e1c2e4d 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -127,12 +127,16 @@ out:
 }
 EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
 
-void __ref xen_hvm_init_shared_info(void)
+static void __init xen_hvm_init_shared_info(void)
 {
 	int cpu;
 	struct xen_add_to_physmap xatp;
 	static struct shared_info *shared_info_page = 0;
 
+	/* already setup */
+	if (shared_info_page != 0 && HYPERVISOR_shared_info ==
shared_info_page)
+		return;
+
 	if (!shared_info_page)
 		shared_info_page = (struct shared_info *)
 			get_zeroed_page(GFP_KERNEL);
@@ -161,11 +165,11 @@ void __ref xen_hvm_init_shared_info(void)
 		per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
 	}
 }
+core_initcall(xen_hvm_init_shared_info);
 
-static int __init xen_hvm_guest_init(void)
+int __init xen_guest_init(void)
 {
 	xen_hvm_init_shared_info();
 	return 0;
 }
-
-core_initcall(xen_hvm_guest_init);
+EXPORT_SYMBOL(xen_guest_init);
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 1f92865..0bad7b5 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1420,4 +1420,12 @@ const struct hypervisor_x86 x86_hyper_xen_hvm __refconst
= {
 	.init_platform		= xen_hvm_guest_init,
 };
 EXPORT_SYMBOL(x86_hyper_xen_hvm);
+
+int __init xen_guest_init(void)
+{
+	/* do nothing: rely on x86_hyper_xen_hvm for the initialization */
+	return 0;
+	
+}
+EXPORT_SYMBOL(xen_guest_init);
 #endif
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 3ae96a9..52a98ff 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -578,6 +578,9 @@ static int xen_cons_init(void)
 	if (!xen_domain())
 		return 0;
 
+	/* retrieve xen infos  */
+	xen_guest_init();
+
 	if (xen_initial_domain())
 		ops = &dom0_hvc_ops;
 	else {
diff --git a/include/xen/xen.h b/include/xen/xen.h
index 2c0d3a5..792a4d2 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -9,8 +9,10 @@ enum xen_domain_type {
 
 #ifdef CONFIG_XEN
 extern enum xen_domain_type xen_domain_type;
+int xen_guest_init(void);
 #else
 #define xen_domain_type		XEN_NATIVE
+static inline int xen_guest_init(void) { return 0; }
 #endif
 
 #define xen_domain()		(xen_domain_type != XEN_NATIVE)
-- 
1.7.2.5

Use Xen features to figure out if we are privileged.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/xen/enlighten.c         |    9 ++++++++-
 include/xen/interface/features.h |    3 +++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index e1c2e4d..ddacecf 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -3,6 +3,7 @@
 #include <xen/interface/memory.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
+#include <xen/features.h>
 
 #include <linux/module.h>
 
@@ -11,7 +12,7 @@
 
 #include <asm/pgtable.h>
 
-struct start_info _xen_start_info = { .flags = (SIF_INITDOMAIN|SIF_PRIVILEGED)
};
+struct start_info _xen_start_info;
 struct start_info *xen_start_info = &_xen_start_info;
 EXPORT_SYMBOL_GPL(xen_start_info);
 
@@ -133,6 +134,12 @@ static void __init xen_hvm_init_shared_info(void)
 	struct xen_add_to_physmap xatp;
 	static struct shared_info *shared_info_page = 0;
 
+	xen_setup_features();
+	if (xen_feature(XENFEAT_dom0))
+		xen_start_info->flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
+	else
+		xen_start_info->flags &= ~(SIF_INITDOMAIN|SIF_PRIVILEGED);
+
 	/* already setup */
 	if (shared_info_page != 0 && HYPERVISOR_shared_info ==
shared_info_page)
 		return;
diff --git a/include/xen/interface/features.h b/include/xen/interface/features.h
index b6ca39a..131a6cc 100644
--- a/include/xen/interface/features.h
+++ b/include/xen/interface/features.h
@@ -50,6 +50,9 @@
 /* x86: pirq can be used by HVM guests */
 #define XENFEAT_hvm_pirqs           10
 
+/* operation as Dom0 is supported */
+#define XENFEAT_dom0                      11
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
1.7.2.5

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/include/asm/xen/hypercall.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/xen/hypercall.h
b/arch/arm/include/asm/xen/hypercall.h
index 2fe7434..7503256 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -189,7 +189,7 @@ HYPERVISOR_event_channel_op(int cmd, void *arg)
 
 static inline unsigned long HYPERVISOR_hvm_op(int op, void *arg)
 {
-	return -ENOSYS;
+	return _hypercall2(int, HYPERCALL(hvm_op), op, arg);
 }
 
 	static inline int
-- 
1.7.2.5

When unmask_evtchn is called, if we already have an event pending, we
just set evtchn_pending_sel waiting for irq to be re-enabled. That is
because x86 pv guests overwrite the irq_enable pvops with
xen_irq_enable_direct that also handles pending events.

However x86 HVM guests and ARM guests do not change or do not have the
irq_enable pvop, so this scheme doesn''t work properly for them.
Considering that having a pending irq when unmask_evtchn is called is
not very common, and it is better to keep the native_irq_enable
implementation for HVM guests and ARM guests, the best thing to do is
just using the EVTCHNOP_unmask callback (Xen re-injects pending
events in response).

Considering that this patch fixes a bug on x86 for current PV on HVM
guests, I''ll resend it separately.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/events.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index eae0d0b..0132505 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -372,8 +372,11 @@ static void unmask_evtchn(int port)
 
 	BUG_ON(!irqs_disabled());
 
-	/* Slow path (hypercall) if this is a non-local port. */
-	if (unlikely(cpu != cpu_from_evtchn(port))) {
+	/* Slow path (hypercall) if this is a non-local port or if this is
+	 * an hvm domain and an event is pending (hvm domains don''t have
+	 * their own implementation of irq_enable). */
+	if (unlikely((cpu != cpu_from_evtchn(port)) ||
+			(xen_hvm_domain() && sync_test_bit(port,
&s->evtchn_pending[0])))) {
 		struct evtchn_unmask unmask = { .port = port };
 		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
 	} else {
-- 
1.7.2.5

On ARM irqs are not enabled by default:

- call enable_percpu_irq for IRQ_EVTCHN_CALLBACK;

- set the IRQF_VALID flag for the other irqs bound to evtchns. It causes
IRQ_NOAUTOEN to be set and as a consequence irq_unmask is going to be
called when a xenbus driver calls request_irq.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 0132505..ca92755 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -53,6 +53,7 @@
 #include <xen/interface/hvm/params.h>
 #include <xen/interface/physdev.h>
 #include <xen/interface/sched.h>
+#include <asm/hw_irq.h>
 
 /*
  * This lock protects updates to the following mapping and reference-count
@@ -827,6 +828,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 
 		xen_irq_info_evtchn_init(irq, evtchn);
 	}
+	set_irq_flags(irq, IRQF_VALID);
 
 out:
 	mutex_unlock(&irq_mapping_update_lock);
@@ -1751,6 +1753,7 @@ int __init xen_init_IRQ_arm(void)
 	if (rc) {
 		printk(KERN_ERR "Error requesting IRQ %d\n", IRQ_EVTCHN_CALLBACK);
 	}
+	enable_percpu_irq(IRQ_EVTCHN_CALLBACK, 0);
 	return rc;
 }
 core_initcall(xen_init_IRQ_arm);
-- 
1.7.2.5

When unmask_evtchn is called, if we already have an event pending, we
just set evtchn_pending_sel waiting for local_irq_enable to be called.
That is because PV guests set the irq_enable pvops to
xen_irq_enable_direct that also handles pending events.

However HVM guests (and ARM guests) do not change or do not have the
irq_enable pvop, so evtchn_unmask cannot work properly for them.

Considering that having the pending_irq bit set when unmask_evtchn is
called is not very common, and it is simpler to keep the
native_irq_enable implementation for HVM guests (and ARM guests), the
best thing to do is just use the EVTCHNOP_unmask hypercall (Xen
re-injects pending events in response).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index eae0d0b..0132505 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -372,8 +372,11 @@ static void unmask_evtchn(int port)
 
 	BUG_ON(!irqs_disabled());
 
-	/* Slow path (hypercall) if this is a non-local port. */
-	if (unlikely(cpu != cpu_from_evtchn(port))) {
+	/* Slow path (hypercall) if this is a non-local port or if this is
+	 * an hvm domain and an event is pending (hvm domains don''t have
+	 * their own implementation of irq_enable). */
+	if (unlikely((cpu != cpu_from_evtchn(port)) ||
+			(xen_hvm_domain() && sync_test_bit(port,
&s->evtchn_pending[0])))) {
 		struct evtchn_unmask unmask = { .port = port };
 		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
 	} else {

On Fri, Jun 22, 2012 at 05:26:07PM +0100, Stefano Stabellini
wrote:
> When unmask_evtchn is called, if we already have an event pending, we
> just set evtchn_pending_sel waiting for local_irq_enable to be called.
> That is because PV guests set the irq_enable pvops to
Can you point out where the PV guests do that please? Even just
including a snippet of code would be nice so that somebody
in the future has an idea of where it was/is.
> xen_irq_enable_direct that also handles pending events.
> 
> However HVM guests (and ARM guests) do not change or do not have the
> irq_enable pvop, so evtchn_unmask cannot work properly for them.
Duh!
> 
> Considering that having the pending_irq bit set when unmask_evtchn is
> called is not very common, and it is simpler to keep the
Unless you pin the guests on the vCPUS on which domain0 is not present..
> native_irq_enable implementation for HVM guests (and ARM guests), the
> best thing to do is just use the EVTCHNOP_unmask hypercall (Xen
> re-injects pending events in response).
And by re-injects you mean than the IOAPIC or (whatever it is on ARM)
is armed to show that there is a pending interrupt, right?
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/xen/events.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index eae0d0b..0132505 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -372,8 +372,11 @@ static void unmask_evtchn(int port)
>  
>  	BUG_ON(!irqs_disabled());
>  
> -	/* Slow path (hypercall) if this is a non-local port. */
> -	if (unlikely(cpu != cpu_from_evtchn(port))) {
> +	/* Slow path (hypercall) if this is a non-local port or if this is
> +	 * an hvm domain and an event is pending (hvm domains don''t have
> +	 * their own implementation of irq_enable). */
> +	if (unlikely((cpu != cpu_from_evtchn(port)) ||
> +			(xen_hvm_domain() && sync_test_bit(port,
&s->evtchn_pending[0])))) {
>  		struct evtchn_unmask unmask = { .port = port };
We already have two seperate acks - for when there is an GMFN APIC bitmap and
when there is not. Can we also have to seperate unmask_evtchn then? And
just have the HVM and ARM just do a straightforward unmaks_evtchn while
the PV remains the same?
>  		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
>  	} else {
>

On Fri, Jun 22, 2012 at 05:14:45PM +0100, Stefano Stabellini
wrote:
> On ARM irqs are not enabled by default:
Which IRQs? Xen IRQs? Linux IRQs?
> 
> - call enable_percpu_irq for IRQ_EVTCHN_CALLBACK;
Why do we want to do that?
> 
> - set the IRQF_VALID flag for the other irqs bound to evtchns. It causes
> IRQ_NOAUTOEN to be set and as a consequence irq_unmask is going to be
> called when a xenbus driver calls request_irq.
Can you explain in more details why we need that?
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/xen/events.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 0132505..ca92755 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -53,6 +53,7 @@
>  #include <xen/interface/hvm/params.h>
>  #include <xen/interface/physdev.h>
>  #include <xen/interface/sched.h>
> +#include <asm/hw_irq.h>
>  
>  /*
>   * This lock protects updates to the following mapping and reference-count
> @@ -827,6 +828,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
>  
>  		xen_irq_info_evtchn_init(irq, evtchn);
>  	}
> +	set_irq_flags(irq, IRQF_VALID);
>  
>  out:
>  	mutex_unlock(&irq_mapping_update_lock);
> @@ -1751,6 +1753,7 @@ int __init xen_init_IRQ_arm(void)
>  	if (rc) {
>  		printk(KERN_ERR "Error requesting IRQ %d\n",
IRQ_EVTCHN_CALLBACK);
>  	}
> +	enable_percpu_irq(IRQ_EVTCHN_CALLBACK, 0);
>  	return rc;
>  }
>  core_initcall(xen_init_IRQ_arm);
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

On Fri, Jun 22, 2012 at 05:14:42PM +0100, Stefano Stabellini
wrote:
> Use Xen features to figure out if we are privileged.
Is there a corresponding Xen c/s for the XNEFEAT_dom0?

Why can''t the existing SIF_PRIVILIGED call work? Isn''t that
somethign that the hypervisor sets for the guest?
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/arm/xen/enlighten.c         |    9 ++++++++-
>  include/xen/interface/features.h |    3 +++
>  2 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index e1c2e4d..ddacecf 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -3,6 +3,7 @@
>  #include <xen/interface/memory.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
> +#include <xen/features.h>
>  
>  #include <linux/module.h>
>  
> @@ -11,7 +12,7 @@
>  
>  #include <asm/pgtable.h>
>  
> -struct start_info _xen_start_info = { .flags =
(SIF_INITDOMAIN|SIF_PRIVILEGED) };
> +struct start_info _xen_start_info;
>  struct start_info *xen_start_info = &_xen_start_info;
>  EXPORT_SYMBOL_GPL(xen_start_info);
>  
> @@ -133,6 +134,12 @@ static void __init xen_hvm_init_shared_info(void)
>  	struct xen_add_to_physmap xatp;
>  	static struct shared_info *shared_info_page = 0;
>  
> +	xen_setup_features();
> +	if (xen_feature(XENFEAT_dom0))
> +		xen_start_info->flags |= SIF_INITDOMAIN|SIF_PRIVILEGED;
> +	else
> +		xen_start_info->flags &= ~(SIF_INITDOMAIN|SIF_PRIVILEGED);
> +
>  	/* already setup */
>  	if (shared_info_page != 0 && HYPERVISOR_shared_info ==
shared_info_page)
>  		return;
> diff --git a/include/xen/interface/features.h
b/include/xen/interface/features.h
> index b6ca39a..131a6cc 100644
> --- a/include/xen/interface/features.h
> +++ b/include/xen/interface/features.h
> @@ -50,6 +50,9 @@
>  /* x86: pirq can be used by HVM guests */
>  #define XENFEAT_hvm_pirqs           10
>  
> +/* operation as Dom0 is supported */
> +#define XENFEAT_dom0                      11
> +
>  #define XENFEAT_NR_SUBMAPS 1
>  
>  #endif /* __XEN_PUBLIC_FEATURES_H__ */
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Fri, Jun 22, 2012 at 05:14:40PM +0100, Stefano Stabellini
wrote:
> Fix the shared_info and vcpu_info struct definitions to match the ones
> in Xen.
Is there a corresponding c/s in the Xen tree for this?
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/arm/include/asm/xen/interface.h |   15 +++------------
>  1 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
> index 3ad2d4b..8ab7cebb 100644
> --- a/arch/arm/include/asm/xen/interface.h
> +++ b/arch/arm/include/asm/xen/interface.h
> @@ -40,19 +40,10 @@ DEFINE_GUEST_HANDLE(xen_pfn_t);
>  #endif
>  
>  /* Maximum number of virtual CPUs in multi-processor guests. */
> -#define MAX_VIRT_CPUS 32
> +#define MAX_VIRT_CPUS 1
>  
> -struct arch_vcpu_info {
> -    unsigned long cr2;
> -    unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
> -};
> -
> -struct arch_shared_info {
> -    unsigned long max_pfn;                  /* max pfn that appears in
table */
> -    /* Frame containing list of mfns containing list of mfns containing
p2m. */
> -    unsigned long pfn_to_mfn_frame_list_list;
> -    unsigned long nmi_reason;
> -};
> +struct arch_vcpu_info { };
> +struct arch_shared_info { };
>  
>  /* XXX: Move pvclock definitions some place arch independent */
>  struct pvclock_vcpu_time_info {
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

On Fri, Jun 22, 2012 at 05:14:41PM +0100, Stefano Stabellini
wrote:
> We used to rely on a core_initcall to initialize Xen on ARM, however
> core_initcalls are actually called after early consoles are initialized.
> That means that hvc_xen.c is going to be initialized before Xen.
> 
> Given the lack of a better alternative, just call a new Xen
> initialization function (xen_guest_init) from xen_cons_init.
> 
> xen_guest_init has to be arch independant, so write both an ARM and an
> x86 implementation. The x86 implementation is currently empty because we
> can be sure that xen_hvm_guest_init is called early enough.
> 
> Probably we can get rid of this as soon as we have better DT support.
What is DT?
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/arm/xen/enlighten.c  |   12 ++++++++----
>  arch/x86/xen/enlighten.c  |    8 ++++++++
>  drivers/tty/hvc/hvc_xen.c |    3 +++
>  include/xen/xen.h         |    2 ++
>  4 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 7d76962..e1c2e4d 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -127,12 +127,16 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>  
> -void __ref xen_hvm_init_shared_info(void)
> +static void __init xen_hvm_init_shared_info(void)
>  {
>  	int cpu;
>  	struct xen_add_to_physmap xatp;
>  	static struct shared_info *shared_info_page = 0;
>  
> +	/* already setup */
> +	if (shared_info_page != 0 && HYPERVISOR_shared_info ==
shared_info_page)
> +		return;
> +
>  	if (!shared_info_page)
>  		shared_info_page = (struct shared_info *)
>  			get_zeroed_page(GFP_KERNEL);
> @@ -161,11 +165,11 @@ void __ref xen_hvm_init_shared_info(void)
>  		per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
>  	}
>  }
> +core_initcall(xen_hvm_init_shared_info);
>  
> -static int __init xen_hvm_guest_init(void)
> +int __init xen_guest_init(void)
>  {
>  	xen_hvm_init_shared_info();
>  	return 0;
Don''t you want to do:
	return xen_hvm_init_shared_info();

why do we need to return int?
>  }
> -
> -core_initcall(xen_hvm_guest_init);
> +EXPORT_SYMBOL(xen_guest_init);
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 1f92865..0bad7b5 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1420,4 +1420,12 @@ const struct hypervisor_x86 x86_hyper_xen_hvm
__refconst = {
>  	.init_platform		= xen_hvm_guest_init,
>  };
>  EXPORT_SYMBOL(x86_hyper_xen_hvm);
> +
> +int __init xen_guest_init(void)
> +{
> +	/* do nothing: rely on x86_hyper_xen_hvm for the initialization */
> +	return 0;
> +	
> +}
> +EXPORT_SYMBOL(xen_guest_init);
_GPL please.

>  #endif
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 3ae96a9..52a98ff 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -578,6 +578,9 @@ static int xen_cons_init(void)
>  	if (!xen_domain())
>  		return 0;
>  
> +	/* retrieve xen infos  */
> +	xen_guest_init();
So shouldn''t this be:
	if (xen_guest_init())
		return .. something?
> +
>  	if (xen_initial_domain())
>  		ops = &dom0_hvc_ops;
>  	else {
> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index 2c0d3a5..792a4d2 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -9,8 +9,10 @@ enum xen_domain_type {
>  
>  #ifdef CONFIG_XEN
>  extern enum xen_domain_type xen_domain_type;
> +int xen_guest_init(void);
>  #else
>  #define xen_domain_type		XEN_NATIVE
> +static inline int xen_guest_init(void) { return 0; }
>  #endif
>  
>  #define xen_domain()		(xen_domain_type != XEN_NATIVE)
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On 09/07/12 15:45, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 22, 2012 at 05:14:41PM +0100, Stefano Stabellini wrote:
>> We used to rely on a core_initcall to initialize Xen on ARM, however
>> core_initcalls are actually called after early consoles are
initialized.
>> That means that hvc_xen.c is going to be initialized before Xen.
>>
>> Given the lack of a better alternative, just call a new Xen
>> initialization function (xen_guest_init) from xen_cons_init.
>>
>> xen_guest_init has to be arch independant, so write both an ARM and an
>> x86 implementation. The x86 implementation is currently empty because
we
>> can be sure that xen_hvm_guest_init is called early enough.
>>
>> Probably we can get rid of this as soon as we have better DT support.
> 
> What is DT?
Device Tree.  It''s a binary describing the hardware and some system
configuration that is passed to the kernel by the boot loader or (in
this case) the hypervisor.  Vaguely analogous to ACPI except it''s not
crazy ;).

We really should get the device tree bindings sorted out before
accepting any kernel side patches.  I think we can do this even if
Xen''s
device tree support is incomplete.

David

David Vrabel wrote:
> On 09/07/12 15:45, Konrad Rzeszutek Wilk wrote:
>> On Fri, Jun 22, 2012 at 05:14:41PM +0100, Stefano Stabellini wrote:
>>> We used to rely on a core_initcall to initialize Xen on ARM,
however
>>> core_initcalls are actually called after early consoles are
initialized.
>>> That means that hvc_xen.c is going to be initialized before Xen.
>>>
>>> Given the lack of a better alternative, just call a new Xen
>>> initialization function (xen_guest_init) from xen_cons_init.
>>>
>>> xen_guest_init has to be arch independant, so write both an ARM and
an
>>> x86 implementation. The x86 implementation is currently empty
because we
>>> can be sure that xen_hvm_guest_init is called early enough.
>>>
>>> Probably we can get rid of this as soon as we have better DT
support.
>> What is DT?
> 
> Device Tree.  It''s a binary describing the hardware and some
system
> configuration that is passed to the kernel by the boot loader or (in
> this case) the hypervisor.  Vaguely analogous to ACPI except it''s
not
> crazy ;).
> 
> We really should get the device tree bindings sorted out before
> accepting any kernel side patches.  I think we can do this even if
Xen''s
> device tree support is incomplete.
Will this be passed from the hypervisor to the linux kernel using a
specific mechanism (different than the native one)?

On 12/07/12 12:49, Roger Pau Monne wrote:
> David Vrabel wrote:
>> On 09/07/12 15:45, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jun 22, 2012 at 05:14:41PM +0100, Stefano Stabellini wrote:
>>>> We used to rely on a core_initcall to initialize Xen on ARM,
however
>>>> core_initcalls are actually called after early consoles are
initialized.
>>>> That means that hvc_xen.c is going to be initialized before
Xen.
>>>>
>>>> Given the lack of a better alternative, just call a new Xen
>>>> initialization function (xen_guest_init) from xen_cons_init.
>>>>
>>>> xen_guest_init has to be arch independant, so write both an ARM
and an
>>>> x86 implementation. The x86 implementation is currently empty
because we
>>>> can be sure that xen_hvm_guest_init is called early enough.
>>>>
>>>> Probably we can get rid of this as soon as we have better DT
support.
>>> What is DT?
>>
>> Device Tree.  It''s a binary describing the hardware and some
system
>> configuration that is passed to the kernel by the boot loader or (in
>> this case) the hypervisor.  Vaguely analogous to ACPI except
it''s not
>> crazy ;).
>>
>> We really should get the device tree bindings sorted out before
>> accepting any kernel side patches.  I think we can do this even if
Xen''s
>> device tree support is incomplete.
> 
> Will this be passed from the hypervisor to the linux kernel using a
> specific mechanism (different than the native one)?
The same mechanism.  The kernel is booted with the physical address of
the device tree blob in a register (r2 I think) .  Xen sorts this out
for dom0 and the toolstack is responsible for this for domUs.

I would expect the device tree to include the physical address of the
shared page with something like this.

hypervisor {
   xen {
       shared-info = <0x00 0x12345678 0 4096>;
   };
};

Arch code in ARM would check for the hypervisor node (very) early on and
call a hypervisor specific init function based on the name of the child
node (xen in this case).

David

On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 22, 2012 at 05:14:42PM +0100, Stefano Stabellini wrote:
> > Use Xen features to figure out if we are privileged.
> 
> Is there a corresponding Xen c/s for the XNEFEAT_dom0?
Yes, that would be 23735
(http://xenbits.xensource.com/hg/xen-unstable.hg/rev/537918f518ee).
> Why can''t the existing SIF_PRIVILIGED call work? Isn''t
that
> somethign that the hypervisor sets for the guest?
SIF_PRIVILIGED is in the start_info page that ARM guests don''t have.

On Thu, 12 Jul 2012, David Vrabel wrote:
> On 12/07/12 12:49, Roger Pau Monne wrote:
> > David Vrabel wrote:
> >> On 09/07/12 15:45, Konrad Rzeszutek Wilk wrote:
> >>> On Fri, Jun 22, 2012 at 05:14:41PM +0100, Stefano Stabellini
wrote:
> >>>> We used to rely on a core_initcall to initialize Xen on
ARM, however
> >>>> core_initcalls are actually called after early consoles
are initialized.
> >>>> That means that hvc_xen.c is going to be initialized
before Xen.
> >>>>
> >>>> Given the lack of a better alternative, just call a new
Xen
> >>>> initialization function (xen_guest_init) from
xen_cons_init.
> >>>>
> >>>> xen_guest_init has to be arch independant, so write both
an ARM and an
> >>>> x86 implementation. The x86 implementation is currently
empty because we
> >>>> can be sure that xen_hvm_guest_init is called early
enough.
> >>>>
> >>>> Probably we can get rid of this as soon as we have better
DT support.
> >>> What is DT?
> >>
> >> Device Tree.  It''s a binary describing the hardware and
some system
> >> configuration that is passed to the kernel by the boot loader or
(in
> >> this case) the hypervisor.  Vaguely analogous to ACPI except
it''s not
> >> crazy ;).
> >>
> >> We really should get the device tree bindings sorted out before
> >> accepting any kernel side patches.  I think we can do this even if
Xen''s
> >> device tree support is incomplete.
> > 
> > Will this be passed from the hypervisor to the linux kernel using a
> > specific mechanism (different than the native one)?
> 
> The same mechanism.  The kernel is booted with the physical address of
> the device tree blob in a register (r2 I think) .  Xen sorts this out
> for dom0 and the toolstack is responsible for this for domUs.
> 
> I would expect the device tree to include the physical address of the
> shared page with something like this.
> 
> hypervisor {
>    xen {
>        shared-info = <0x00 0x12345678 0 4096>;
>    };
> };
> 
> Arch code in ARM would check for the hypervisor node (very) early on and
> call a hypervisor specific init function based on the name of the child
> node (xen in this case).
 
There is no need to specify the shared-info page address in the DT as we
already have a mechanism to map it dynamically (XENMAPSPACE_shared_info).

However we could use DT to pass the address of the grant table pages
instead of introducing HVM_PARAM_GRANT_START_PFN (see
http://marc.info/?l=linux-kernel&m=134140077708602&w=2).

On Thu, 2012-07-12 at 13:50 -0400, Stefano Stabellini
wrote:
> On Thu, 12 Jul 2012, David Vrabel wrote:
> > On 12/07/12 12:49, Roger Pau Monne wrote:
> > > David Vrabel wrote:
> > >> On 09/07/12 15:45, Konrad Rzeszutek Wilk wrote:
> > >>> On Fri, Jun 22, 2012 at 05:14:41PM +0100, Stefano
Stabellini wrote:
> > >>>> We used to rely on a core_initcall to initialize Xen
on ARM, however
> > >>>> core_initcalls are actually called after early
consoles are initialized.
> > >>>> That means that hvc_xen.c is going to be initialized
before Xen.
> > >>>>
> > >>>> Given the lack of a better alternative, just call a
new Xen
> > >>>> initialization function (xen_guest_init) from
xen_cons_init.
> > >>>>
> > >>>> xen_guest_init has to be arch independant, so write
both an ARM and an
> > >>>> x86 implementation. The x86 implementation is
currently empty because we
> > >>>> can be sure that xen_hvm_guest_init is called early
enough.
> > >>>>
> > >>>> Probably we can get rid of this as soon as we have
better DT support.
> > >>> What is DT?
> > >>
> > >> Device Tree.  It''s a binary describing the hardware
and some system
> > >> configuration that is passed to the kernel by the boot loader
or (in
> > >> this case) the hypervisor.  Vaguely analogous to ACPI except
it''s not
> > >> crazy ;).
> > >>
> > >> We really should get the device tree bindings sorted out
before
> > >> accepting any kernel side patches.  I think we can do this
even if Xen''s
> > >> device tree support is incomplete.
> > > 
> > > Will this be passed from the hypervisor to the linux kernel using
a
> > > specific mechanism (different than the native one)?
> > 
> > The same mechanism.  The kernel is booted with the physical address of
> > the device tree blob in a register (r2 I think) .  Xen sorts this out
> > for dom0 and the toolstack is responsible for this for domUs.
> > 
> > I would expect the device tree to include the physical address of the
> > shared page with something like this.
> > 
> > hypervisor {
> >    xen {
> >        shared-info = <0x00 0x12345678 0 4096>;
> >    };
> > };
> > 
> > Arch code in ARM would check for the hypervisor node (very) early on
and
> > call a hypervisor specific init function based on the name of the
child
> > node (xen in this case).
>  
> There is no need to specify the shared-info page address in the DT as we
> already have a mechanism to map it dynamically (XENMAPSPACE_shared_info).
> 
> However we could use DT to pass the address of the grant table pages
> instead of introducing HVM_PARAM_GRANT_START_PFN (see
> http://marc.info/?l=linux-kernel&m=134140077708602&w=2).
We need to be sure there is something there which signals "running under
Xen" in the general sense too so we can detect it early on. Perhaps
something explicitly for that purpose or perhaps one of these required
items could serve as a key. 

Ian

On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 22, 2012 at 05:14:41PM +0100, Stefano Stabellini wrote:
> > We used to rely on a core_initcall to initialize Xen on ARM, however
> > core_initcalls are actually called after early consoles are
initialized.
> > That means that hvc_xen.c is going to be initialized before Xen.
> > 
> > Given the lack of a better alternative, just call a new Xen
> > initialization function (xen_guest_init) from xen_cons_init.
> > 
> > xen_guest_init has to be arch independant, so write both an ARM and an
> > x86 implementation. The x86 implementation is currently empty because
we
> > can be sure that xen_hvm_guest_init is called early enough.
> > 
> > Probably we can get rid of this as soon as we have better DT support.
> 
> What is DT?
> 
> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  arch/arm/xen/enlighten.c  |   12 ++++++++----
> >  arch/x86/xen/enlighten.c  |    8 ++++++++
> >  drivers/tty/hvc/hvc_xen.c |    3 +++
> >  include/xen/xen.h         |    2 ++
> >  4 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 7d76962..e1c2e4d 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -127,12 +127,16 @@ out:
> >  }
> >  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> >  
> > -void __ref xen_hvm_init_shared_info(void)
> > +static void __init xen_hvm_init_shared_info(void)
> >  {
> >  	int cpu;
> >  	struct xen_add_to_physmap xatp;
> >  	static struct shared_info *shared_info_page = 0;
> >  
> > +	/* already setup */
> > +	if (shared_info_page != 0 && HYPERVISOR_shared_info ==
shared_info_page)
> > +		return;
> > +
> >  	if (!shared_info_page)
> >  		shared_info_page = (struct shared_info *)
> >  			get_zeroed_page(GFP_KERNEL);
> > @@ -161,11 +165,11 @@ void __ref xen_hvm_init_shared_info(void)
> >  		per_cpu(xen_vcpu, cpu) =
&HYPERVISOR_shared_info->vcpu_info[cpu];
> >  	}
> >  }
> > +core_initcall(xen_hvm_init_shared_info);
> >  
> > -static int __init xen_hvm_guest_init(void)
> > +int __init xen_guest_init(void)
> >  {
> >  	xen_hvm_init_shared_info();
> >  	return 0;
> 
> Don''t you want to do:
> 	return xen_hvm_init_shared_info();
> 
> why do we need to return int?
We don''t need to, but it might be a good idea to propagate setup
errors.

> >  }
> > -
> > -core_initcall(xen_hvm_guest_init);
> > +EXPORT_SYMBOL(xen_guest_init);
> > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> > index 1f92865..0bad7b5 100644
> > --- a/arch/x86/xen/enlighten.c
> > +++ b/arch/x86/xen/enlighten.c
> > @@ -1420,4 +1420,12 @@ const struct hypervisor_x86 x86_hyper_xen_hvm
__refconst = {
> >  	.init_platform		= xen_hvm_guest_init,
> >  };
> >  EXPORT_SYMBOL(x86_hyper_xen_hvm);
> > +
> > +int __init xen_guest_init(void)
> > +{
> > +	/* do nothing: rely on x86_hyper_xen_hvm for the initialization */
> > +	return 0;
> > +	
> > +}
> > +EXPORT_SYMBOL(xen_guest_init);
> 
> _GPL please.
OK
> 
> >  #endif
> > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > index 3ae96a9..52a98ff 100644
> > --- a/drivers/tty/hvc/hvc_xen.c
> > +++ b/drivers/tty/hvc/hvc_xen.c
> > @@ -578,6 +578,9 @@ static int xen_cons_init(void)
> >  	if (!xen_domain())
> >  		return 0;
> >  
> > +	/* retrieve xen infos  */
> > +	xen_guest_init();
> 
> So shouldn''t this be:
> 	if (xen_guest_init())
> 		return .. something?
yes, I''ll do that

> > +
> >  	if (xen_initial_domain())
> >  		ops = &dom0_hvc_ops;
> >  	else {
> > diff --git a/include/xen/xen.h b/include/xen/xen.h
> > index 2c0d3a5..792a4d2 100644
> > --- a/include/xen/xen.h
> > +++ b/include/xen/xen.h
> > @@ -9,8 +9,10 @@ enum xen_domain_type {
> >  
> >  #ifdef CONFIG_XEN
> >  extern enum xen_domain_type xen_domain_type;
> > +int xen_guest_init(void);
> >  #else
> >  #define xen_domain_type		XEN_NATIVE
> > +static inline int xen_guest_init(void) { return 0; }
> >  #endif
> >  
> >  #define xen_domain()		(xen_domain_type != XEN_NATIVE)
> > -- 
> > 1.7.2.5
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>

On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 22, 2012 at 05:14:40PM +0100, Stefano Stabellini wrote:
> > Fix the shared_info and vcpu_info struct definitions to match the ones
> > in Xen.
> 
> Is there a corresponding c/s in the Xen tree for this?
That''s the problem: arch_vcpu_info and arch_shared_info
haven''t been
defined in Xen yet on ARM.
I think the ones below have been copied from xen/x86 by mistake.

> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  arch/arm/include/asm/xen/interface.h |   15 +++------------
> >  1 files changed, 3 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
> > index 3ad2d4b..8ab7cebb 100644
> > --- a/arch/arm/include/asm/xen/interface.h
> > +++ b/arch/arm/include/asm/xen/interface.h
> > @@ -40,19 +40,10 @@ DEFINE_GUEST_HANDLE(xen_pfn_t);
> >  #endif
> >  
> >  /* Maximum number of virtual CPUs in multi-processor guests. */
> > -#define MAX_VIRT_CPUS 32
> > +#define MAX_VIRT_CPUS 1
> >  
> > -struct arch_vcpu_info {
> > -    unsigned long cr2;
> > -    unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
> > -};
> > -
> > -struct arch_shared_info {
> > -    unsigned long max_pfn;                  /* max pfn that appears
in table */
> > -    /* Frame containing list of mfns containing list of mfns
containing p2m. */
> > -    unsigned long pfn_to_mfn_frame_list_list;
> > -    unsigned long nmi_reason;
> > -};
> > +struct arch_vcpu_info { };
> > +struct arch_shared_info { };
> >  
> >  /* XXX: Move pvclock definitions some place arch independent */
> >  struct pvclock_vcpu_time_info {
> > -- 
> > 1.7.2.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
>

On Fri, 2012-07-13 at 12:48 -0400, Stefano Stabellini
wrote:
> On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jun 22, 2012 at 05:14:40PM +0100, Stefano Stabellini wrote:
> > > Fix the shared_info and vcpu_info struct definitions to match the
ones
> > > in Xen.
> > 
> > Is there a corresponding c/s in the Xen tree for this?
> 
> That''s the problem: arch_vcpu_info and arch_shared_info
haven''t been
> defined in Xen yet on ARM.
Strictly speaking they are defined (in xen/include/public/arch-arm.h),
they are just empty ;-)
> I think the ones below have been copied from xen/x86 by mistake.
Yes, it does look that way.
> 
> 
> > > 
> > > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > > ---
> > >  arch/arm/include/asm/xen/interface.h |   15 +++------------
> > >  1 files changed, 3 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
> > > index 3ad2d4b..8ab7cebb 100644
> > > --- a/arch/arm/include/asm/xen/interface.h
> > > +++ b/arch/arm/include/asm/xen/interface.h
> > > @@ -40,19 +40,10 @@ DEFINE_GUEST_HANDLE(xen_pfn_t);
> > >  #endif
> > >  
> > >  /* Maximum number of virtual CPUs in multi-processor guests. */
> > > -#define MAX_VIRT_CPUS 32
> > > +#define MAX_VIRT_CPUS 1
> > >  
> > > -struct arch_vcpu_info {
> > > -    unsigned long cr2;
> > > -    unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
> > > -};
> > > -
> > > -struct arch_shared_info {
> > > -    unsigned long max_pfn;                  /* max pfn that
appears in table */
> > > -    /* Frame containing list of mfns containing list of mfns
containing p2m. */
> > > -    unsigned long pfn_to_mfn_frame_list_list;
> > > -    unsigned long nmi_reason;
> > > -};
> > > +struct arch_vcpu_info { };
> > > +struct arch_shared_info { };
> > >  
> > >  /* XXX: Move pvclock definitions some place arch independent */
> > >  struct pvclock_vcpu_time_info {
> > > -- 
> > > 1.7.2.5
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at 
http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> >

On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 22, 2012 at 05:14:45PM +0100, Stefano Stabellini wrote:
> > On ARM irqs are not enabled by default:
> 
> Which IRQs? Xen IRQs? Linux IRQs?
Linux IRQs

> > - call enable_percpu_irq for IRQ_EVTCHN_CALLBACK;
> 
> Why do we want to do that?
 
drivers are supposed to call enable_irq after request_irq, however on
x86 we could get away without it because Linux irqs are enabled by
default (see below)

> > - set the IRQF_VALID flag for the other irqs bound to evtchns. It
causes
> > IRQ_NOAUTOEN to be set and as a consequence irq_unmask is going to be
> > called when a xenbus driver calls request_irq.
> 
> Can you explain in more details why we need that?
If _IRQ_NOAUTOEN is set, irq_settings_can_autoenable returns false and
__setup_irq doesn''t call irq_startup.

IRQ_NOAUTOEN is set by default by set_irq_flags on ARM, but not on x86.


> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  drivers/xen/events.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 0132505..ca92755 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -53,6 +53,7 @@
> >  #include <xen/interface/hvm/params.h>
> >  #include <xen/interface/physdev.h>
> >  #include <xen/interface/sched.h>
> > +#include <asm/hw_irq.h>
> >  
> >  /*
> >   * This lock protects updates to the following mapping and
reference-count
> > @@ -827,6 +828,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
> >  
> >  		xen_irq_info_evtchn_init(irq, evtchn);
> >  	}
> > +	set_irq_flags(irq, IRQF_VALID);
> >  
> >  out:
> >  	mutex_unlock(&irq_mapping_update_lock);
> > @@ -1751,6 +1753,7 @@ int __init xen_init_IRQ_arm(void)
> >  	if (rc) {
> >  		printk(KERN_ERR "Error requesting IRQ %d\n",
IRQ_EVTCHN_CALLBACK);
> >  	}
> > +	enable_percpu_irq(IRQ_EVTCHN_CALLBACK, 0);
> >  	return rc;
> >  }
> >  core_initcall(xen_init_IRQ_arm);
> > -- 
> > 1.7.2.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
>

On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 22, 2012 at 05:26:07PM +0100, Stefano Stabellini wrote:
> > When unmask_evtchn is called, if we already have an event pending, we
> > just set evtchn_pending_sel waiting for local_irq_enable to be called.
> > That is because PV guests set the irq_enable pvops to
> 
> Can you point out where the PV guests do that please? Even just
> including a snippet of code would be nice so that somebody
> in the future has an idea of where it was/is.
Do you mean where PV guests set the irq_enable pvop?

That would be in xen_setup_vcpu_info_placement.
irq_enable is set to xen_irq_enable_direct that is implemented in
assembly in arch/x86/xen/xen-asm.S: it tests for XEN_vcpu_info_pending
and call xen_force_evtchn_callback.

> > xen_irq_enable_direct that also handles pending events.
> > 
> > However HVM guests (and ARM guests) do not change or do not have the
> > irq_enable pvop, so evtchn_unmask cannot work properly for them.
> 
> Duh!
> > 
> > Considering that having the pending_irq bit set when unmask_evtchn is
> > called is not very common, and it is simpler to keep the
> 
> Unless you pin the guests on the vCPUS on which domain0 is not present..
Considering that __xen_evtchn_do_upcall keeps looping around until no
more events are set in the shared_info page and also that
xen_dynamic_chip and xen_pirq_chip only mask irqs on irq_mask, the only
way that pending_irq can be set before unmask_evtchn is called is when
the guest receives multiple notifications for the same event before
acking the first one.
Arguably it is not a extremely common case at least in domUs.

> > native_irq_enable implementation for HVM guests (and ARM guests), the
> > best thing to do is just use the EVTCHNOP_unmask hypercall (Xen
> > re-injects pending events in response).
> 
> And by re-injects you mean than the IOAPIC or (whatever it is on ARM)
> is armed to show that there is a pending interrupt, right?
Right. A new notification is going to be sent by Xen to the guest, via
the best mechanism available. On X86 it could be a vector callback.

> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  drivers/xen/events.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index eae0d0b..0132505 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -372,8 +372,11 @@ static void unmask_evtchn(int port)
> >  
> >  	BUG_ON(!irqs_disabled());
> >  
> > -	/* Slow path (hypercall) if this is a non-local port. */
> > -	if (unlikely(cpu != cpu_from_evtchn(port))) {
> > +	/* Slow path (hypercall) if this is a non-local port or if this is
> > +	 * an hvm domain and an event is pending (hvm domains don''t
have
> > +	 * their own implementation of irq_enable). */
> > +	if (unlikely((cpu != cpu_from_evtchn(port)) ||
> > +			(xen_hvm_domain() && sync_test_bit(port,
&s->evtchn_pending[0])))) {
> >  		struct evtchn_unmask unmask = { .port = port };
> 
> We already have two seperate acks - for when there is an GMFN APIC bitmap
and
> when there is not. Can we also have to seperate unmask_evtchn then? And
> just have the HVM and ARM just do a straightforward unmaks_evtchn while
> the PV remains the same?
Do you mean HVM and ARM do a straightforward EVTCHNOP_unmask hypercall?

In that case we would lose performances because most of the time an
hypercall won''t be necessary.
If we keep the code as it is, it makes sense to have the PV and PVHVM
cases in the same function.

> >  		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
> >  	} else {
> > 
>

On Fri, Jul 13, 2012 at 06:14:33PM +0100, Stefano Stabellini
wrote:
> On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jun 22, 2012 at 05:14:45PM +0100, Stefano Stabellini wrote:
> > > On ARM irqs are not enabled by default:
> > 
> > Which IRQs? Xen IRQs? Linux IRQs?
> 
> Linux IRQs
> 
> 
> > > - call enable_percpu_irq for IRQ_EVTCHN_CALLBACK;
> > 
> > Why do we want to do that?
>  
> drivers are supposed to call enable_irq after request_irq, however on
> x86 we could get away without it because Linux irqs are enabled by
> default (see below)
> 
> 
> > > - set the IRQF_VALID flag for the other irqs bound to evtchns. It
causes
> > > IRQ_NOAUTOEN to be set and as a consequence irq_unmask is going
to be
> > > called when a xenbus driver calls request_irq.
> > 
> > Can you explain in more details why we need that?
> 
> If _IRQ_NOAUTOEN is set, irq_settings_can_autoenable returns false and
> __setup_irq doesn''t call irq_startup.
> 
> IRQ_NOAUTOEN is set by default by set_irq_flags on ARM, but not on x86.
OK, please include those questions/answers in the git commit and
repost.
> 
> 
> 
> > > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > > ---
> > >  drivers/xen/events.c |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > > index 0132505..ca92755 100644
> > > --- a/drivers/xen/events.c
> > > +++ b/drivers/xen/events.c
> > > @@ -53,6 +53,7 @@
> > >  #include <xen/interface/hvm/params.h>
> > >  #include <xen/interface/physdev.h>
> > >  #include <xen/interface/sched.h>
> > > +#include <asm/hw_irq.h>
> > >  
> > >  /*
> > >   * This lock protects updates to the following mapping and
reference-count
> > > @@ -827,6 +828,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
> > >  
> > >  		xen_irq_info_evtchn_init(irq, evtchn);
> > >  	}
> > > +	set_irq_flags(irq, IRQF_VALID);
> > >  
> > >  out:
> > >  	mutex_unlock(&irq_mapping_update_lock);
> > > @@ -1751,6 +1753,7 @@ int __init xen_init_IRQ_arm(void)
> > >  	if (rc) {
> > >  		printk(KERN_ERR "Error requesting IRQ %d\n",
IRQ_EVTCHN_CALLBACK);
> > >  	}
> > > +	enable_percpu_irq(IRQ_EVTCHN_CALLBACK, 0);
> > >  	return rc;
> > >  }
> > >  core_initcall(xen_init_IRQ_arm);
> > > -- 
> > > 1.7.2.5
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at 
http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> >

On Fri, Jul 13, 2012 at 11:08:58AM -0600, Ian Campbell
wrote:
> On Fri, 2012-07-13 at 12:48 -0400, Stefano Stabellini wrote:
> > On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Jun 22, 2012 at 05:14:40PM +0100, Stefano Stabellini
wrote:
> > > > Fix the shared_info and vcpu_info struct definitions to
match the ones
> > > > in Xen.
> > > 
> > > Is there a corresponding c/s in the Xen tree for this?
> > 
> > That''s the problem: arch_vcpu_info and arch_shared_info
haven''t been
> > defined in Xen yet on ARM.
> 
> Strictly speaking they are defined (in xen/include/public/arch-arm.h),
> they are just empty ;-)
> 
> > I think the ones below have been copied from xen/x86 by mistake.
> 
> Yes, it does look that way.
Ah, pls mention that in the git commit description.
> 
> > 
> > 
> > > > 
> > > > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > > > ---
> > > >  arch/arm/include/asm/xen/interface.h |   15 +++------------
> > > >  1 files changed, 3 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
> > > > index 3ad2d4b..8ab7cebb 100644
> > > > --- a/arch/arm/include/asm/xen/interface.h
> > > > +++ b/arch/arm/include/asm/xen/interface.h
> > > > @@ -40,19 +40,10 @@ DEFINE_GUEST_HANDLE(xen_pfn_t);
> > > >  #endif
> > > >  
> > > >  /* Maximum number of virtual CPUs in multi-processor
guests. */
> > > > -#define MAX_VIRT_CPUS 32
> > > > +#define MAX_VIRT_CPUS 1
> > > >  
> > > > -struct arch_vcpu_info {
> > > > -    unsigned long cr2;
> > > > -    unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
> > > > -};
> > > > -
> > > > -struct arch_shared_info {
> > > > -    unsigned long max_pfn;                  /* max pfn that
appears in table */
> > > > -    /* Frame containing list of mfns containing list of
mfns containing p2m. */
> > > > -    unsigned long pfn_to_mfn_frame_list_list;
> > > > -    unsigned long nmi_reason;
> > > > -};
> > > > +struct arch_vcpu_info { };
> > > > +struct arch_shared_info { };
> > > >  
> > > >  /* XXX: Move pvclock definitions some place arch
independent */
> > > >  struct pvclock_vcpu_time_info {
> > > > -- 
> > > > 1.7.2.5
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line
"unsubscribe linux-kernel" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at 
http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at  http://www.tux.org/lkml/
> > > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

On Fri, Jul 13, 2012 at 06:48:35PM +0100, Stefano Stabellini
wrote:
> On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jun 22, 2012 at 05:26:07PM +0100, Stefano Stabellini wrote:
> > > When unmask_evtchn is called, if we already have an event
pending, we
> > > just set evtchn_pending_sel waiting for local_irq_enable to be
called.
> > > That is because PV guests set the irq_enable pvops to
> > 
> > Can you point out where the PV guests do that please? Even just
> > including a snippet of code would be nice so that somebody
> > in the future has an idea of where it was/is.
> 
> Do you mean where PV guests set the irq_enable pvop?
> 
> That would be in xen_setup_vcpu_info_placement.
> irq_enable is set to xen_irq_enable_direct that is implemented in
> assembly in arch/x86/xen/xen-asm.S: it tests for XEN_vcpu_info_pending
> and call xen_force_evtchn_callback.
Excellent. Pls include that comment in the git commit.
> 
> 
> > > xen_irq_enable_direct that also handles pending events.
> > > 
> > > However HVM guests (and ARM guests) do not change or do not have
the
> > > irq_enable pvop, so evtchn_unmask cannot work properly for them.
> > 
> > Duh!
> > > 
> > > Considering that having the pending_irq bit set when
unmask_evtchn is
> > > called is not very common, and it is simpler to keep the
> > 
> > Unless you pin the guests on the vCPUS on which domain0 is not
present..
> 
> Considering that __xen_evtchn_do_upcall keeps looping around until no
> more events are set in the shared_info page and also that
> xen_dynamic_chip and xen_pirq_chip only mask irqs on irq_mask, the only
> way that pending_irq can be set before unmask_evtchn is called is when
> the guest receives multiple notifications for the same event before
> acking the first one.
> Arguably it is not a extremely common case at least in domUs.
> 
> 
> > > native_irq_enable implementation for HVM guests (and ARM guests),
the
> > > best thing to do is just use the EVTCHNOP_unmask hypercall (Xen
> > > re-injects pending events in response).
> > 
> > And by re-injects you mean than the IOAPIC or (whatever it is on ARM)
> > is armed to show that there is a pending interrupt, right?
> 
> Right. A new notification is going to be sent by Xen to the guest, via
> the best mechanism available. On X86 it could be a vector callback.
> 
> 
> > > 
> > > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > > ---
> > >  drivers/xen/events.c |    7 +++++--
> > >  1 files changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > > index eae0d0b..0132505 100644
> > > --- a/drivers/xen/events.c
> > > +++ b/drivers/xen/events.c
> > > @@ -372,8 +372,11 @@ static void unmask_evtchn(int port)
> > >  
> > >  	BUG_ON(!irqs_disabled());
> > >  
> > > -	/* Slow path (hypercall) if this is a non-local port. */
> > > -	if (unlikely(cpu != cpu_from_evtchn(port))) {
> > > +	/* Slow path (hypercall) if this is a non-local port or if this
is
> > > +	 * an hvm domain and an event is pending (hvm domains
don''t have
> > > +	 * their own implementation of irq_enable). */
> > > +	if (unlikely((cpu != cpu_from_evtchn(port)) ||
> > > +			(xen_hvm_domain() && sync_test_bit(port,
&s->evtchn_pending[0])))) {
> > >  		struct evtchn_unmask unmask = { .port = port };
> > 
> > We already have two seperate acks - for when there is an GMFN APIC
bitmap and
> > when there is not. Can we also have to seperate unmask_evtchn then?
And
> > just have the HVM and ARM just do a straightforward unmaks_evtchn
while
> > the PV remains the same?
> 
> Do you mean HVM and ARM do a straightforward EVTCHNOP_unmask hypercall?
I was thinking of some way to lessen the impact of the ''if
(..)''  statement.
There is already a check from the cpu, and now there is a bit check
and another check for domain. Was wondering if it would make more sense
to abstract the code the unmask_evtchn calls, and provide two variants
of the unmask_evtchn: a one that is mostly called on PV/PVHVM on x86 and
then the ARM version?

Or won''t that really give us any performance benefits and that
extra check for hvm_domain and test_bit is negligible?

Perhaps a better question is - do you have further plans for this
function? As in expanding it with more ''if'' conditionals?
> 
> In that case we would lose performances because most of the time an
> hypercall won''t be necessary.
> If we keep the code as it is, it makes sense to have the PV and PVHVM
> cases in the same function.
The two things that roam my mind is:
 - performance impact
 - code readability.

Granted this code is the slow patch so maybe the performance part is
not an issue. But that ''sync_test_bit'' isn''t that an
atomic locked
call so it flushes the bus? There is a ''xen_hvm_domain()''
condition
before it so that does lessen the impact to be only done on HVM.

If we do run this under HVM, we would do:
 1) cpu == cpu_from_evtchn, so 
 2).sync_test_bit .. say it returns false
 3). sync_clear_bit
 4). sync_test_bit on the same word that 2) was done.

If this was re-organized a bit differently could we remove 2)
out of the picture so that under HVM we just do 1) 3) and 4) ?

And for that we might have to have two implementations of unmaks_evtchn - were
both of them might call the same underlaying functions that do the
bit-operations, but the ''if'' conditionals are differently
organized.
Or is this scenario really unlikely and I am just thinking to hard about this?

On Mon, 16 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 13, 2012 at 11:08:58AM -0600, Ian Campbell wrote:
> > On Fri, 2012-07-13 at 12:48 -0400, Stefano Stabellini wrote:
> > > On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > > On Fri, Jun 22, 2012 at 05:14:40PM +0100, Stefano Stabellini
wrote:
> > > > > Fix the shared_info and vcpu_info struct definitions to
match the ones
> > > > > in Xen.
> > > > 
> > > > Is there a corresponding c/s in the Xen tree for this?
> > > 
> > > That''s the problem: arch_vcpu_info and arch_shared_info
haven''t been
> > > defined in Xen yet on ARM.
> > 
> > Strictly speaking they are defined (in xen/include/public/arch-arm.h),
> > they are just empty ;-)
> > 
> > > I think the ones below have been copied from xen/x86 by mistake.
> > 
> > Yes, it does look that way.
> 
> Ah, pls mention that in the git commit description.
OK, see below.

I am not sending out a second version of this patch series because I
still consider it a work in progress.
I''ll send out a new up-to-date patch series with all the fixes as soon
as I have it.


---

xen/arm: fix the shared_info and vcpu_info structs

Fix the shared_info and vcpu_info struct definitions to match the ones
in Xen: the current arch_vcpu_info and arch_shared_info have been copied
from x86 by mistake (the Xen version of these structs are just empty).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/arch/arm/include/asm/xen/interface.h
b/arch/arm/include/asm/xen/interface.h
index 3ad2d4b..8ab7cebb 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -40,19 +40,10 @@ DEFINE_GUEST_HANDLE(xen_pfn_t);
 #endif
 
 /* Maximum number of virtual CPUs in multi-processor guests. */
-#define MAX_VIRT_CPUS 32
+#define MAX_VIRT_CPUS 1
 
-struct arch_vcpu_info {
-    unsigned long cr2;
-    unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
-};
-
-struct arch_shared_info {
-    unsigned long max_pfn;                  /* max pfn that appears in table */
-    /* Frame containing list of mfns containing list of mfns containing p2m. */
-    unsigned long pfn_to_mfn_frame_list_list;
-    unsigned long nmi_reason;
-};
+struct arch_vcpu_info { };
+struct arch_shared_info { };
 
 /* XXX: Move pvclock definitions some place arch independent */
 struct pvclock_vcpu_time_info {

On Mon, 16 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 13, 2012 at 06:14:33PM +0100, Stefano Stabellini wrote:
> > On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Jun 22, 2012 at 05:14:45PM +0100, Stefano Stabellini
wrote:
> > > > On ARM irqs are not enabled by default:
> > > 
> > > Which IRQs? Xen IRQs? Linux IRQs?
> > 
> > Linux IRQs
> > 
> > 
> > > > - call enable_percpu_irq for IRQ_EVTCHN_CALLBACK;
> > > 
> > > Why do we want to do that?
> >  
> > drivers are supposed to call enable_irq after request_irq, however on
> > x86 we could get away without it because Linux irqs are enabled by
> > default (see below)
> > 
> > 
> > > > - set the IRQF_VALID flag for the other irqs bound to
evtchns. It causes
> > > > IRQ_NOAUTOEN to be set and as a consequence irq_unmask is
going to be
> > > > called when a xenbus driver calls request_irq.
> > > 
> > > Can you explain in more details why we need that?
> > 
> > If _IRQ_NOAUTOEN is set, irq_settings_can_autoenable returns false and
> > __setup_irq doesn''t call irq_startup.
> > 
> > IRQ_NOAUTOEN is set by default by set_irq_flags on ARM, but not on
x86.
> 
> OK, please include those questions/answers in the git commit and
> repost.
---

xen/arm: enable evtchn irqs

On ARM Linux irqs are not enabled by default:

- call enable_percpu_irq for IRQ_EVTCHN_CALLBACK (drivers are supposed
to call enable_irq after request_irq);

- set the IRQF_VALID flag for the other irqs bound to evtchns. It causes
IRQ_NOAUTOEN to be set and as a consequence irq_unmask is going to be
called when a xenbus driver calls request_irq.
This is needed because IRQ_NOAUTOEN is set by set_irq_flags on ARM.
If IRQ_NOAUTOEN is set __setup_irq doesn''t call irq_startup that is
responsible for calling irq_unmask at startup time.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index eae0d0b..ca92755 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -53,6 +53,7 @@
 #include <xen/interface/hvm/params.h>
 #include <xen/interface/physdev.h>
 #include <xen/interface/sched.h>
+#include <asm/hw_irq.h>
 
 /*
  * This lock protects updates to the following mapping and reference-count
@@ -824,6 +828,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 
 		xen_irq_info_evtchn_init(irq, evtchn);
 	}
+	set_irq_flags(irq, IRQF_VALID);
 
 out:
 	mutex_unlock(&irq_mapping_update_lock);
@@ -1748,6 +1753,7 @@ int __init xen_init_IRQ_arm(void)
 	if (rc) {
 		printk(KERN_ERR "Error requesting IRQ %d\n", IRQ_EVTCHN_CALLBACK);
 	}
+	enable_percpu_irq(IRQ_EVTCHN_CALLBACK, 0);
 	return rc;
 }
 core_initcall(xen_init_IRQ_arm);

On Mon, 16 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 13, 2012 at 06:48:35PM +0100, Stefano Stabellini wrote:
> > On Mon, 9 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Jun 22, 2012 at 05:26:07PM +0100, Stefano Stabellini
wrote:
> > > > When unmask_evtchn is called, if we already have an event
pending, we
> > > > just set evtchn_pending_sel waiting for local_irq_enable to
be called.
> > > > That is because PV guests set the irq_enable pvops to
> > > 
> > > Can you point out where the PV guests do that please? Even just
> > > including a snippet of code would be nice so that somebody
> > > in the future has an idea of where it was/is.
> > 
> > Do you mean where PV guests set the irq_enable pvop?
> > 
> > That would be in xen_setup_vcpu_info_placement.
> > irq_enable is set to xen_irq_enable_direct that is implemented in
> > assembly in arch/x86/xen/xen-asm.S: it tests for XEN_vcpu_info_pending
> > and call xen_force_evtchn_callback.
> 
> Excellent. Pls include that comment in the git commit.
OK

> > > > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > > > index eae0d0b..0132505 100644
> > > > --- a/drivers/xen/events.c
> > > > +++ b/drivers/xen/events.c
> > > > @@ -372,8 +372,11 @@ static void unmask_evtchn(int port)
> > > >  
> > > >  	BUG_ON(!irqs_disabled());
> > > >  
> > > > -	/* Slow path (hypercall) if this is a non-local port. */
> > > > -	if (unlikely(cpu != cpu_from_evtchn(port))) {
> > > > +	/* Slow path (hypercall) if this is a non-local port or if
this is
> > > > +	 * an hvm domain and an event is pending (hvm domains
don''t have
> > > > +	 * their own implementation of irq_enable). */
> > > > +	if (unlikely((cpu != cpu_from_evtchn(port)) ||
> > > > +			(xen_hvm_domain() && sync_test_bit(port,
&s->evtchn_pending[0])))) {
> > > >  		struct evtchn_unmask unmask = { .port = port };
> > > 
> > > We already have two seperate acks - for when there is an GMFN
APIC bitmap and
> > > when there is not. Can we also have to seperate unmask_evtchn
then? And
> > > just have the HVM and ARM just do a straightforward unmaks_evtchn
while
> > > the PV remains the same?
> > 
> > Do you mean HVM and ARM do a straightforward EVTCHNOP_unmask
hypercall?
> 
> I was thinking of some way to lessen the impact of the ''if
(..)''  statement.
> There is already a check from the cpu, and now there is a bit check
> and another check for domain. Was wondering if it would make more sense
> to abstract the code the unmask_evtchn calls, and provide two variants
> of the unmask_evtchn: a one that is mostly called on PV/PVHVM on x86 and
> then the ARM version?
> 
> Or won''t that really give us any performance benefits and that
> extra check for hvm_domain and test_bit is negligible?
> 
> Perhaps a better question is - do you have further plans for this
> function? As in expanding it with more ''if'' conditionals?
Nope, I certainly don''t. In fact I agree with you on the fact that is
not very readable as it is.

> > In that case we would lose performances because most of the time an
> > hypercall won''t be necessary.
> > If we keep the code as it is, it makes sense to have the PV and PVHVM
> > cases in the same function.
> 
> The two things that roam my mind is:
>  - performance impact
>  - code readability.
> 
> Granted this code is the slow patch so maybe the performance part is
> not an issue. But that ''sync_test_bit'' isn''t
that an atomic locked
> call so it flushes the bus? There is a ''xen_hvm_domain()''
condition
> before it so that does lessen the impact to be only done on HVM.
> 
> If we do run this under HVM, we would do:
>  1) cpu == cpu_from_evtchn, so 
>  2).sync_test_bit .. say it returns false
>  3). sync_clear_bit
>  4). sync_test_bit on the same word that 2) was done.
> 
> If this was re-organized a bit differently could we remove 2)
> out of the picture so that under HVM we just do 1) 3) and 4) ?
I see what you mean now. It might make sense.

> And for that we might have to have two implementations of unmaks_evtchn -
were
> both of them might call the same underlaying functions that do the
> bit-operations, but the ''if'' conditionals are differently
organized.
> Or is this scenario really unlikely and I am just thinking to hard about
this?
Let just say that I only managed to reproduce it with a buggy hypervisor
port to a new architecture :-)

But I don''t like the idea of writing obfuscated and inefficient code,
so
let me give it a second try.

---

xen/events: fix unmask_evtchn for PV on HVM guests

When unmask_evtchn is called, if we already have an event pending, we
just set evtchn_pending_sel waiting for local_irq_enable to be called.
That is because PV guests set the irq_enable pvops to
xen_irq_enable_direct in xen_setup_vcpu_info_placement:
xen_irq_enable_direct is implemented in assembly in
arch/x86/xen/xen-asm.S and call xen_force_evtchn_callback if
XEN_vcpu_info_pending is set.

However HVM guests (and ARM guests) do not change or do not have the
irq_enable pvop, so evtchn_unmask cannot work properly for them.

Considering that having the pending_irq bit set when unmask_evtchn is
called is not very common, and it is simpler to keep the
native_irq_enable implementation for HVM guests (and ARM guests), the
best thing to do is just use the EVTCHNOP_unmask hypercall (Xen
re-injects pending events in response).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 0a8a17c..d75cc39 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -373,11 +373,22 @@ static void unmask_evtchn(int port)
 {
 	struct shared_info *s = HYPERVISOR_shared_info;
 	unsigned int cpu = get_cpu();
+	int do_hypercall = 0, evtchn_pending = 0;
 
 	BUG_ON(!irqs_disabled());
 
-	/* Slow path (hypercall) if this is a non-local port. */
-	if (unlikely(cpu != cpu_from_evtchn(port))) {
+	if (unlikely((cpu != cpu_from_evtchn(port))))
+		do_hypercall = 1;
+	else
+		evtchn_pending = sync_test_bit(port, &s->evtchn_pending[0]);
+
+	if (unlikely(evtchn_pending && xen_hvm_domain()))
+		do_hypercall = 1;
+
+	/* Slow path (hypercall) if this is a non-local port or if this is
+	 * an hvm domain and an event is pending (hvm domains don''t have
+	 * their own implementation of irq_enable). */
+	if (do_hypercall) {
 		struct evtchn_unmask unmask = { .port = port };
 		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
 	} else {
@@ -390,7 +401,7 @@ static void unmask_evtchn(int port)
 		 * ''hw_resend_irq''. Just like a real IO-APIC we
''lose
 		 * the interrupt edge'' if the channel is masked.
 		 */
-		if (sync_test_bit(port, &s->evtchn_pending[0]) &&
+		if (evtchn_pending &&
 		    !sync_test_and_set_bit(port / BITS_PER_LONG,
 					   &vcpu_info->evtchn_pending_sel))
 			vcpu_info->evtchn_upcall_pending = 1;
-- 
1.7.2.5

> > OK, please include those questions/answers in the git commit and
> > repost.
I seem to be missing the rest of the patches. I see the drivers/xen/events also
has the  xen_init_IRQ_arm... is there a git tree with the base
patches?
> 
> ---
> 
> xen/arm: enable evtchn irqs
> 
> On ARM Linux irqs are not enabled by default:
> 
> - call enable_percpu_irq for IRQ_EVTCHN_CALLBACK (drivers are supposed
> to call enable_irq after request_irq);
> 
> - set the IRQF_VALID flag for the other irqs bound to evtchns. It causes
> IRQ_NOAUTOEN to be set and as a consequence irq_unmask is going to be
> called when a xenbus driver calls request_irq.
> This is needed because IRQ_NOAUTOEN is set by set_irq_flags on ARM.
> If IRQ_NOAUTOEN is set __setup_irq doesn''t call irq_startup that
is
> responsible for calling irq_unmask at startup time.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index eae0d0b..ca92755 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -53,6 +53,7 @@
>  #include <xen/interface/hvm/params.h>
>  #include <xen/interface/physdev.h>
>  #include <xen/interface/sched.h>
> +#include <asm/hw_irq.h>
>  
>  /*
>   * This lock protects updates to the following mapping and reference-count
> @@ -824,6 +828,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
>  
>  		xen_irq_info_evtchn_init(irq, evtchn);
>  	}
> +	set_irq_flags(irq, IRQF_VALID);
>  
>  out:
>  	mutex_unlock(&irq_mapping_update_lock);
> @@ -1748,6 +1753,7 @@ int __init xen_init_IRQ_arm(void)
>  	if (rc) {
>  		printk(KERN_ERR "Error requesting IRQ %d\n",
IRQ_EVTCHN_CALLBACK);
>  	}
> +	enable_percpu_irq(IRQ_EVTCHN_CALLBACK, 0);
>  	return rc;
>  }
>  core_initcall(xen_init_IRQ_arm);

On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > OK, please include those questions/answers in the git commit and
> > > repost.
> 
> I seem to be missing the rest of the patches. I see the drivers/xen/events
also
> has the  xen_init_IRQ_arm... is there a git tree with the base patches?
Yes, the latest git tree, based on
55b02d2f4445ad625213817a1736bf2884d32547, is available here:

git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git xenarmv7-3

On Fri, Jul 20, 2012 at 12:09:56PM +0100, Stefano Stabellini
wrote:
> On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > > OK, please include those questions/answers in the git commit
and
> > > > repost.
> > 
> > I seem to be missing the rest of the patches. I see the
drivers/xen/events also
> > has the  xen_init_IRQ_arm... is there a git tree with the base
patches?
> 
> Yes, the latest git tree, based on
> 55b02d2f4445ad625213817a1736bf2884d32547, is available here:
> 
> git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git xenarmv7-3
Uh there is a bit of stuff there. Can you just repost those you want me to
review and Ack that touch common code?

Or are you posting them to collect feedback/Acks and then want to post them
as a bigger set?
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 20, 2012 at 12:09:56PM +0100, Stefano Stabellini wrote:
> > On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > > > OK, please include those questions/answers in the git
commit and
> > > > > repost.
> > > 
> > > I seem to be missing the rest of the patches. I see the
drivers/xen/events also
> > > has the  xen_init_IRQ_arm... is there a git tree with the base
patches?
> > 
> > Yes, the latest git tree, based on
> > 55b02d2f4445ad625213817a1736bf2884d32547, is available here:
> > 
> > git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git xenarmv7-3
> 
> Uh there is a bit of stuff there.
Actually, aside from many backports and hacks, there are just three
interesting patch series from my point of view:

http://marc.info/?l=xen-devel&m=133001901013674&w=2 xen/arm: receive Xen
events and initialize xenbus
http://marc.info/?l=xen-devel&m=134038186610813&w=2 xen/arm: PV console
support
http://marc.info/?l=xen-devel&m=134140081408627&w=2 xen/arm:
grant_table, blkfront and blkback

The last two are recent and I have received feedback from you on the second
one.

> Can you just repost those you want me to
> review and Ack that touch common code?
Good idea, but fortunately there aren''t that many of them.
In fact if we exclude the ones that just add more #include, these are the ones
that remains:

d122a6b075e582af17dd2b3ddd8ee4ded3f85300 xen/arm: receive xen events on arm
1902bd5574447c03c1e6081cce98d8bf6e1f35e7 xen/arm: compile and run xenbus
5bbda4a165284aceee19b54954f10344b724b506 xen/arm: enable evtchn irqs
d4fda31cf5411e8ada3f1163c68595b7474d7c1d xen/arm: initialize grant_table on ARM

it would be nice if you could take a look at them and tell me what you think.

> Or are you posting them to collect feedback/Acks and then want to post them
> as a bigger set?
Yes, the idea is to post them in a single set.
Now that I have support for the basic functionalities (guests booting, PV
console, disk and network), I can start working on a single patch
series for a recent kernel.

On Fri, Jul 20, 2012 at 04:23:07PM +0100, Stefano Stabellini
wrote:
> On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jul 20, 2012 at 12:09:56PM +0100, Stefano Stabellini wrote:
> > > On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > > > > OK, please include those questions/answers in the
git commit and
> > > > > > repost.
> > > > 
> > > > I seem to be missing the rest of the patches. I see the
drivers/xen/events also
> > > > has the  xen_init_IRQ_arm... is there a git tree with the
base patches?
> > > 
> > > Yes, the latest git tree, based on
> > > 55b02d2f4445ad625213817a1736bf2884d32547, is available here:
> > > 
> > > git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git
xenarmv7-3
> > 
> > Uh there is a bit of stuff there.
> 
> Actually, aside from many backports and hacks, there are just three
> interesting patch series from my point of view:
> 
> http://marc.info/?l=xen-devel&m=133001901013674&w=2 xen/arm:
receive Xen events and initialize xenbus
> http://marc.info/?l=xen-devel&m=134038186610813&w=2 xen/arm: PV
console support
> http://marc.info/?l=xen-devel&m=134140081408627&w=2 xen/arm:
grant_table, blkfront and blkback
> 
> The last two are recent and I have received feedback from you on the second
> one.
> 
> 
> > Can you just repost those you want me to
> > review and Ack that touch common code?
> 
> Good idea, but fortunately there aren''t that many of them.
> In fact if we exclude the ones that just add more #include, these are the
ones
> that remains:
> 
> d122a6b075e582af17dd2b3ddd8ee4ded3f85300 xen/arm: receive xen events on arm
> 1902bd5574447c03c1e6081cce98d8bf6e1f35e7 xen/arm: compile and run xenbus
> 5bbda4a165284aceee19b54954f10344b724b506 xen/arm: enable evtchn irqs
> d4fda31cf5411e8ada3f1163c68595b7474d7c1d xen/arm: initialize grant_table on
ARM
> 
> it would be nice if you could take a look at them and tell me what you
think.
> 
> 
> > Or are you posting them to collect feedback/Acks and then want to post
them
> > as a bigger set?
I am going to be lazy and wait for you to git-send-email the right patches. If
you like, you can just email them privately to me and I can look at them.
> 
> Yes, the idea is to post them in a single set.
> Now that I have support for the basic functionalities (guests booting, PV
> console, disk and network), I can start working on a single patch
> series for a recent kernel.

On Wed, 25 Jul 2012, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 20, 2012 at 04:23:07PM +0100, Stefano Stabellini wrote:
> > On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Jul 20, 2012 at 12:09:56PM +0100, Stefano Stabellini
wrote:
> > > > On Fri, 20 Jul 2012, Konrad Rzeszutek Wilk wrote:
> > > > > > > OK, please include those questions/answers in
the git commit and
> > > > > > > repost.
> > > > > 
> > > > > I seem to be missing the rest of the patches. I see the
drivers/xen/events also
> > > > > has the  xen_init_IRQ_arm... is there a git tree with
the base patches?
> > > > 
> > > > Yes, the latest git tree, based on
> > > > 55b02d2f4445ad625213817a1736bf2884d32547, is available here:
> > > > 
> > > > git://xenbits.xen.org/people/sstabellini/linux-pvhvm.git
xenarmv7-3
> > > 
> > > Uh there is a bit of stuff there.
> > 
> > Actually, aside from many backports and hacks, there are just three
> > interesting patch series from my point of view:
> > 
> > http://marc.info/?l=xen-devel&m=133001901013674&w=2 xen/arm:
receive Xen events and initialize xenbus
> > http://marc.info/?l=xen-devel&m=134038186610813&w=2 xen/arm:
PV console support
> > http://marc.info/?l=xen-devel&m=134140081408627&w=2 xen/arm:
grant_table, blkfront and blkback
> > 
> > The last two are recent and I have received feedback from you on the
second
> > one.
> > 
> > 
> > > Can you just repost those you want me to
> > > review and Ack that touch common code?
> > 
> > Good idea, but fortunately there aren''t that many of them.
> > In fact if we exclude the ones that just add more #include, these are
the ones
> > that remains:
> > 
> > d122a6b075e582af17dd2b3ddd8ee4ded3f85300 xen/arm: receive xen events
on arm
> > 1902bd5574447c03c1e6081cce98d8bf6e1f35e7 xen/arm: compile and run
xenbus
> > 5bbda4a165284aceee19b54954f10344b724b506 xen/arm: enable evtchn irqs
> > d4fda31cf5411e8ada3f1163c68595b7474d7c1d xen/arm: initialize
grant_table on ARM
> > 
> > it would be nice if you could take a look at them and tell me what you
think.
> > 
> > 
> > > Or are you posting them to collect feedback/Acks and then want to
post them
> > > as a bigger set?
> 
> I am going to be lazy and wait for you to git-send-email the right patches.
If
> you like, you can just email them privately to me and I can look at them.
No, that''s fine. I should be able to send out the proper patch series
within the next few days.

Konrad,
I cannot see this patch anywhere in your trees. Did I miss it?
Or maybe it just fell through the cracks?
Let me know if you want me to do anything more.
Cheers,
Stefano

On Wed, 18 Jul 2012, Stefano Stabellini wrote:
> xen/events: fix unmask_evtchn for PV on HVM guests
> 
> When unmask_evtchn is called, if we already have an event pending, we
> just set evtchn_pending_sel waiting for local_irq_enable to be called.
> That is because PV guests set the irq_enable pvops to
> xen_irq_enable_direct in xen_setup_vcpu_info_placement:
> xen_irq_enable_direct is implemented in assembly in
> arch/x86/xen/xen-asm.S and call xen_force_evtchn_callback if
> XEN_vcpu_info_pending is set.
> 
> However HVM guests (and ARM guests) do not change or do not have the
> irq_enable pvop, so evtchn_unmask cannot work properly for them.
> 
> Considering that having the pending_irq bit set when unmask_evtchn is
> called is not very common, and it is simpler to keep the
> native_irq_enable implementation for HVM guests (and ARM guests), the
> best thing to do is just use the EVTCHNOP_unmask hypercall (Xen
> re-injects pending events in response).
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  drivers/xen/events.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 0a8a17c..d75cc39 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -373,11 +373,22 @@ static void unmask_evtchn(int port)
>  {
>  	struct shared_info *s = HYPERVISOR_shared_info;
>  	unsigned int cpu = get_cpu();
> +	int do_hypercall = 0, evtchn_pending = 0;
>  
>  	BUG_ON(!irqs_disabled());
>  
> -	/* Slow path (hypercall) if this is a non-local port. */
> -	if (unlikely(cpu != cpu_from_evtchn(port))) {
> +	if (unlikely((cpu != cpu_from_evtchn(port))))
> +		do_hypercall = 1;
> +	else
> +		evtchn_pending = sync_test_bit(port, &s->evtchn_pending[0]);
> +
> +	if (unlikely(evtchn_pending && xen_hvm_domain()))
> +		do_hypercall = 1;
> +
> +	/* Slow path (hypercall) if this is a non-local port or if this is
> +	 * an hvm domain and an event is pending (hvm domains don''t have
> +	 * their own implementation of irq_enable). */
> +	if (do_hypercall) {
>  		struct evtchn_unmask unmask = { .port = port };
>  		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
>  	} else {
> @@ -390,7 +401,7 @@ static void unmask_evtchn(int port)
>  		 * ''hw_resend_irq''. Just like a real IO-APIC we
''lose
>  		 * the interrupt edge'' if the channel is masked.
>  		 */
> -		if (sync_test_bit(port, &s->evtchn_pending[0]) &&
> +		if (evtchn_pending &&
>  		    !sync_test_and_set_bit(port / BITS_PER_LONG,
>  					   &vcpu_info->evtchn_pending_sel))
>  			vcpu_info->evtchn_upcall_pending = 1;
> -- 
> 1.7.2.5
>

On Wed, Aug 22, 2012 at 12:20:09PM +0100, Stefano Stabellini
wrote:
> Konrad,
> I cannot see this patch anywhere in your trees. Did I miss it?
> Or maybe it just fell through the cracks?
Fell through the cracks.. Was there a new version of this? Can you
send me a subset of the patches you want me to pick up?
> Let me know if you want me to do anything more.
> Cheers,
> Stefano
> 
> On Wed, 18 Jul 2012, Stefano Stabellini wrote:
> > xen/events: fix unmask_evtchn for PV on HVM guests
> > 
> > When unmask_evtchn is called, if we already have an event pending, we
> > just set evtchn_pending_sel waiting for local_irq_enable to be called.
> > That is because PV guests set the irq_enable pvops to
> > xen_irq_enable_direct in xen_setup_vcpu_info_placement:
> > xen_irq_enable_direct is implemented in assembly in
> > arch/x86/xen/xen-asm.S and call xen_force_evtchn_callback if
> > XEN_vcpu_info_pending is set.
> > 
> > However HVM guests (and ARM guests) do not change or do not have the
> > irq_enable pvop, so evtchn_unmask cannot work properly for them.
> > 
> > Considering that having the pending_irq bit set when unmask_evtchn is
> > called is not very common, and it is simpler to keep the
> > native_irq_enable implementation for HVM guests (and ARM guests), the
> > best thing to do is just use the EVTCHNOP_unmask hypercall (Xen
> > re-injects pending events in response).
> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > ---
> >  drivers/xen/events.c |   17 ++++++++++++++---
> >  1 files changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 0a8a17c..d75cc39 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -373,11 +373,22 @@ static void unmask_evtchn(int port)
> >  {
> >  	struct shared_info *s = HYPERVISOR_shared_info;
> >  	unsigned int cpu = get_cpu();
> > +	int do_hypercall = 0, evtchn_pending = 0;
> >  
> >  	BUG_ON(!irqs_disabled());
> >  
> > -	/* Slow path (hypercall) if this is a non-local port. */
> > -	if (unlikely(cpu != cpu_from_evtchn(port))) {
> > +	if (unlikely((cpu != cpu_from_evtchn(port))))
> > +		do_hypercall = 1;
> > +	else
> > +		evtchn_pending = sync_test_bit(port, &s->evtchn_pending[0]);
> > +
> > +	if (unlikely(evtchn_pending && xen_hvm_domain()))
> > +		do_hypercall = 1;
> > +
> > +	/* Slow path (hypercall) if this is a non-local port or if this is
> > +	 * an hvm domain and an event is pending (hvm domains don''t
have
> > +	 * their own implementation of irq_enable). */
> > +	if (do_hypercall) {
> >  		struct evtchn_unmask unmask = { .port = port };
> >  		(void)HYPERVISOR_event_channel_op(EVTCHNOP_unmask, &unmask);
> >  	} else {
> > @@ -390,7 +401,7 @@ static void unmask_evtchn(int port)
> >  		 * ''hw_resend_irq''. Just like a real IO-APIC we
''lose
> >  		 * the interrupt edge'' if the channel is masked.
> >  		 */
> > -		if (sync_test_bit(port, &s->evtchn_pending[0]) &&
> > +		if (evtchn_pending &&
> >  		    !sync_test_and_set_bit(port / BITS_PER_LONG,
> >  					   &vcpu_info->evtchn_pending_sel))
> >  			vcpu_info->evtchn_upcall_pending = 1;
> > -- 
> > 1.7.2.5
> >

On Wed, 22 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 22, 2012 at 12:20:09PM +0100, Stefano Stabellini wrote:
> > Konrad,
> > I cannot see this patch anywhere in your trees. Did I miss it?
> > Or maybe it just fell through the cracks?
> 
> Fell through the cracks.. Was there a new version of this? Can you
> send me a subset of the patches you want me to pick up?
OK, I''ll send out a small patch series based on v3.6-rc2