[PATCH 00/NN] linux: public interface changes for arm

[PATCH 00/NN] linux: public interface changes for arm

[Ian Campbell]

The main change here is to switch evtchns to xen_ulong_t on arm, this
enables us to have the same interface on arm32 and arm64. This is an ABI
change for ARM but not x86. This is a counterpart to the Xen patch just
posted.

In that series I also made start_info x86 specific. ARM does not use
this struct in its ABI but for convenience in non-arch specific code I
have included an internal (non ABI) version on ARM too. Long term
perhaps we should consider improving the abstraction used in the common
code to avoid this.

Ian.

[PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long

[Ian Campbell]

On ARM we want these to be the same size on 32- and 64-bit.

This is an ABI change on ARM. X86 does not change.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir (Xen.org) <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/include/asm/xen/events.h |   22 +++++++++++
 arch/x86/include/asm/xen/events.h |    2 +
 drivers/xen/events.c              |   75 ++++++++++++++++++++----------------
 include/xen/interface/xen.h       |    8 ++--
 4 files changed, 70 insertions(+), 37 deletions(-)

diff --git a/arch/arm/include/asm/xen/events.h b/arch/arm/include/asm/xen/events.h
index 94b4e90..92dbb89 100644
--- a/arch/arm/include/asm/xen/events.h
+++ b/arch/arm/include/asm/xen/events.h
@@ -15,4 +15,26 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
 	return raw_irqs_disabled_flags(regs->ARM_cpsr);
 }
 
+/*
+ * We cannot use xchg because it does not support 8-byte
+ * values. However it is safe to use {ldr,dtd}exd directly because all
+ * platforms which Xen can run on support those instructions.
+ */
+static inline xen_ulong_t xen_read_evtchn_pending_sel(struct vcpu_info *vcpu_info)
+{
+	xen_ulong_t val;
+	unsigned int tmp;
+
+	wmb();
+	asm volatile("@ read_evtchn_pending_sel\n"
+		"1:     ldrexd  %0, %H0, [%3]\n"
+		"       strexd  %1, %2, %H2, [%3]\n"
+		"       teq     %1, #0\n"
+		"       bne     1b"
+			: "=&r" (val), "=&r" (tmp)
+			: "r" (0), "r" (&vcpu_info->evtchn_pending_sel)
+			: "memory", "cc");
+	return val;
+}
+
 #endif /* _ASM_ARM_XEN_EVENTS_H */
diff --git a/arch/x86/include/asm/xen/events.h b/arch/x86/include/asm/xen/events.h
index cc146d5..55edd58 100644
--- a/arch/x86/include/asm/xen/events.h
+++ b/arch/x86/include/asm/xen/events.h
@@ -16,4 +16,6 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
 	return raw_irqs_disabled_flags(regs->flags);
 }
 
+#define xen_read_evtchn_pending_sel(v) xchg(&(v)->evtchn_pending_sel, 0)
+
 #endif /* _ASM_X86_XEN_EVENTS_H */
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 0be4df3..e90a440 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -80,6 +80,12 @@ enum xen_irq_type {
 };
 
 /*
+ * Make a bitmask (i.e. unsigned long *) of a xen_ulong_t
+ * array. Primarily to avoid long lines (hence the terse name).
+ */
+#define BM(x) (unsigned long *)(x)
+
+/*
  * Packed IRQ information:
  * type - enum xen_irq_type
  * event channel - irq->event channel mapping
@@ -120,7 +126,9 @@ static unsigned long *pirq_eoi_map;
 #endif
 static bool (*pirq_needs_eoi)(unsigned irq);
 
-static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
+#define BITS_PER_EVTCHN_WORD (sizeof(xen_ulong_t)*8)
+
+static DEFINE_PER_CPU(xen_ulong_t [NR_EVENT_CHANNELS/BITS_PER_EVTCHN_WORD],
 		      cpu_evtchn_mask);
 
 /* Xen will never allocate port zero for any purpose. */
@@ -312,8 +320,8 @@ static void bind_evtchn_to_cpu(unsigned int chn, unsigned int cpu)
 	cpumask_copy(irq_to_desc(irq)->irq_data.affinity, cpumask_of(cpu));
 #endif
 
-	clear_bit(chn, per_cpu(cpu_evtchn_mask, cpu_from_irq(irq)));
-	set_bit(chn, per_cpu(cpu_evtchn_mask, cpu));
+	clear_bit(chn, BM(per_cpu(cpu_evtchn_mask, cpu_from_irq(irq))));
+	set_bit(chn, BM(per_cpu(cpu_evtchn_mask, cpu)));
 
 	info_for_irq(irq)->cpu = cpu;
 }
@@ -339,19 +347,19 @@ static void init_evtchn_cpu_bindings(void)
 static inline void clear_evtchn(int port)
 {
 	struct shared_info *s = HYPERVISOR_shared_info;
-	sync_clear_bit(port, &s->evtchn_pending[0]);
+	sync_clear_bit(port, BM(&s->evtchn_pending[0]));
 }
 
 static inline void set_evtchn(int port)
 {
 	struct shared_info *s = HYPERVISOR_shared_info;
-	sync_set_bit(port, &s->evtchn_pending[0]);
+	sync_set_bit(port, BM(&s->evtchn_pending[0]));
 }
 
 static inline int test_evtchn(int port)
 {
 	struct shared_info *s = HYPERVISOR_shared_info;
-	return sync_test_bit(port, &s->evtchn_pending[0]);
+	return sync_test_bit(port, BM(&s->evtchn_pending[0]));
 }
 
 
@@ -375,7 +383,7 @@ EXPORT_SYMBOL_GPL(notify_remote_via_irq);
 static void mask_evtchn(int port)
 {
 	struct shared_info *s = HYPERVISOR_shared_info;
-	sync_set_bit(port, &s->evtchn_mask[0]);
+	sync_set_bit(port, BM(&s->evtchn_mask[0]));
 }
 
 static void unmask_evtchn(int port)
@@ -389,7 +397,7 @@ static void unmask_evtchn(int port)
 	if (unlikely((cpu != cpu_from_evtchn(port))))
 		do_hypercall = 1;
 	else
-		evtchn_pending = sync_test_bit(port, &s->evtchn_pending[0]);
+		evtchn_pending = sync_test_bit(port, BM(&s->evtchn_pending[0]));
 
 	if (unlikely(evtchn_pending && xen_hvm_domain()))
 		do_hypercall = 1;
@@ -403,7 +411,7 @@ static void unmask_evtchn(int port)
 	} else {
 		struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
 
-		sync_clear_bit(port, &s->evtchn_mask[0]);
+		sync_clear_bit(port, BM(&s->evtchn_mask[0]));
 
 		/*
 		 * The following is basically the equivalent of
@@ -411,8 +419,8 @@ static void unmask_evtchn(int port)
 		 * the interrupt edge' if the channel is masked.
 		 */
 		if (evtchn_pending &&
-		    !sync_test_and_set_bit(port / BITS_PER_LONG,
-					   &vcpu_info->evtchn_pending_sel))
+		    !sync_test_and_set_bit(port / BITS_PER_EVTCHN_WORD,
+					   BM(&vcpu_info->evtchn_pending_sel)))
 			vcpu_info->evtchn_upcall_pending = 1;
 	}
 
@@ -1189,7 +1197,7 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 {
 	struct shared_info *sh = HYPERVISOR_shared_info;
 	int cpu = smp_processor_id();
-	unsigned long *cpu_evtchn = per_cpu(cpu_evtchn_mask, cpu);
+	xen_ulong_t *cpu_evtchn = per_cpu(cpu_evtchn_mask, cpu);
 	int i;
 	unsigned long flags;
 	static DEFINE_SPINLOCK(debug_lock);
@@ -1205,7 +1213,7 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 		pending = (get_irq_regs() && i == cpu)
 			? xen_irqs_disabled(get_irq_regs())
 			: v->evtchn_upcall_mask;
-		printk("%d: masked=%d pending=%d event_sel %0*lx\n  ", i,
+		printk("%d: masked=%d pending=%d event_sel %0*"PRI_xen_ulong"\n  ", i,
 		       pending, v->evtchn_upcall_pending,
 		       (int)(sizeof(v->evtchn_pending_sel)*2),
 		       v->evtchn_pending_sel);
@@ -1214,25 +1222,27 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 
 	printk("\npending:\n   ");
 	for (i = ARRAY_SIZE(sh->evtchn_pending)-1; i >= 0; i--)
-		printk("%0*lx%s", (int)sizeof(sh->evtchn_pending[0])*2,
+		printk("%0*"PRI_xen_ulong"%s",
+		       (int)sizeof(sh->evtchn_pending[0])*2,
 		       sh->evtchn_pending[i],
 		       i % 8 == 0 ? "\n   " : " ");
 	printk("\nglobal mask:\n   ");
 	for (i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
-		printk("%0*lx%s",
+		printk("%0*"PRI_xen_ulong"%s",
 		       (int)(sizeof(sh->evtchn_mask[0])*2),
 		       sh->evtchn_mask[i],
 		       i % 8 == 0 ? "\n   " : " ");
 
 	printk("\nglobally unmasked:\n   ");
 	for (i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
-		printk("%0*lx%s", (int)(sizeof(sh->evtchn_mask[0])*2),
+		printk("%0*"PRI_xen_ulong"%s",
+		       (int)(sizeof(sh->evtchn_mask[0])*2),
 		       sh->evtchn_pending[i] & ~sh->evtchn_mask[i],
 		       i % 8 == 0 ? "\n   " : " ");
 
 	printk("\nlocal cpu%d mask:\n   ", cpu);
-	for (i = (NR_EVENT_CHANNELS/BITS_PER_LONG)-1; i >= 0; i--)
-		printk("%0*lx%s", (int)(sizeof(cpu_evtchn[0])*2),
+	for (i = (NR_EVENT_CHANNELS/BITS_PER_EVTCHN_WORD)-1; i >= 0; i--)
+		printk("%0*"PRI_xen_ulong"%s", (int)(sizeof(cpu_evtchn[0])*2),
 		       cpu_evtchn[i],
 		       i % 8 == 0 ? "\n   " : " ");
 
@@ -1247,16 +1257,16 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 
 	printk("\npending list:\n");
 	for (i = 0; i < NR_EVENT_CHANNELS; i++) {
-		if (sync_test_bit(i, sh->evtchn_pending)) {
-			int word_idx = i / BITS_PER_LONG;
+		if (sync_test_bit(i, BM(sh->evtchn_pending))) {
+			int word_idx = i / BITS_PER_EVTCHN_WORD;
 			printk("  %d: event %d -> irq %d%s%s%s\n",
 			       cpu_from_evtchn(i), i,
 			       evtchn_to_irq[i],
-			       sync_test_bit(word_idx, &v->evtchn_pending_sel)
+			       sync_test_bit(word_idx, BM(&v->evtchn_pending_sel))
 					     ? "" : " l2-clear",
-			       !sync_test_bit(i, sh->evtchn_mask)
+			       !sync_test_bit(i, BM(sh->evtchn_mask))
 					     ? "" : " globally-masked",
-			       sync_test_bit(i, cpu_evtchn)
+			       sync_test_bit(i, BM(cpu_evtchn))
 					     ? "" : " locally-masked");
 		}
 	}
@@ -1304,9 +1314,8 @@ static void __xen_evtchn_do_upcall(void)
 
 #ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86. */
 		/* Clear master flag /before/ clearing selector flag. */
-		wmb();
 #endif
-		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
+		pending_words = xen_read_evtchn_pending_sel(vcpu_info);
 
 		start_word_idx = __this_cpu_read(current_word_idx);
 		start_bit_idx = __this_cpu_read(current_bit_idx);
@@ -1355,7 +1364,7 @@ static void __xen_evtchn_do_upcall(void)
 				bit_idx = __ffs(bits);
 
 				/* Process port. */
-				port = (word_idx * BITS_PER_LONG) + bit_idx;
+				port = (word_idx * BITS_PER_EVTCHN_WORD) + bit_idx;
 				irq = evtchn_to_irq[port];
 
 				if (irq != -1) {
@@ -1364,12 +1373,12 @@ static void __xen_evtchn_do_upcall(void)
 						generic_handle_irq_desc(irq, desc);
 				}
 
-				bit_idx = (bit_idx + 1) % BITS_PER_LONG;
+				bit_idx = (bit_idx + 1) % BITS_PER_EVTCHN_WORD;
 
 				/* Next caller starts at last processed + 1 */
 				__this_cpu_write(current_word_idx,
 						 bit_idx ? word_idx :
-						 (word_idx+1) % BITS_PER_LONG);
+						 (word_idx+1) % BITS_PER_EVTCHN_WORD);
 				__this_cpu_write(current_bit_idx, bit_idx);
 			} while (bit_idx != 0);
 
@@ -1377,7 +1386,7 @@ static void __xen_evtchn_do_upcall(void)
 			if ((word_idx != start_word_idx) || (i != 0))
 				pending_words &= ~(1UL << word_idx);
 
-			word_idx = (word_idx + 1) % BITS_PER_LONG;
+			word_idx = (word_idx + 1) % BITS_PER_EVTCHN_WORD;
 		}
 
 		BUG_ON(!irqs_disabled());
@@ -1487,8 +1496,8 @@ int resend_irq_on_evtchn(unsigned int irq)
 	if (!VALID_EVTCHN(evtchn))
 		return 1;
 
-	masked = sync_test_and_set_bit(evtchn, s->evtchn_mask);
-	sync_set_bit(evtchn, s->evtchn_pending);
+	masked = sync_test_and_set_bit(evtchn, BM(s->evtchn_mask));
+	sync_set_bit(evtchn, BM(s->evtchn_pending));
 	if (!masked)
 		unmask_evtchn(evtchn);
 
@@ -1536,8 +1545,8 @@ static int retrigger_dynirq(struct irq_data *data)
 	if (VALID_EVTCHN(evtchn)) {
 		int masked;
 
-		masked = sync_test_and_set_bit(evtchn, sh->evtchn_mask);
-		sync_set_bit(evtchn, sh->evtchn_pending);
+		masked = sync_test_and_set_bit(evtchn, BM(sh->evtchn_mask));
+		sync_set_bit(evtchn, BM(sh->evtchn_pending));
 		if (!masked)
 			unmask_evtchn(evtchn);
 		ret = 1;
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 886a5d8..53ec416 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -285,7 +285,7 @@ DEFINE_GUEST_HANDLE_STRUCT(multicall_entry);
  * Event channel endpoints per domain:
  *  1024 if a long is 32 bits; 4096 if a long is 64 bits.
  */
-#define NR_EVENT_CHANNELS (sizeof(unsigned long) * sizeof(unsigned long) * 64)
+#define NR_EVENT_CHANNELS (sizeof(xen_ulong_t) * sizeof(xen_ulong_t) * 64)
 
 struct vcpu_time_info {
 	/*
@@ -341,7 +341,7 @@ struct vcpu_info {
 	 */
 	uint8_t evtchn_upcall_pending;
 	uint8_t evtchn_upcall_mask;
-	unsigned long evtchn_pending_sel;
+	xen_ulong_t evtchn_pending_sel;
 	struct arch_vcpu_info arch;
 	struct pvclock_vcpu_time_info time;
 }; /* 64 bytes (x86) */
@@ -384,8 +384,8 @@ struct shared_info {
 	 * per-vcpu selector word to be set. Each bit in the selector covers a
 	 * 'C long' in the PENDING bitfield array.
 	 */
-	unsigned long evtchn_pending[sizeof(unsigned long) * 8];
-	unsigned long evtchn_mask[sizeof(unsigned long) * 8];
+	xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8];
+	xen_ulong_t evtchn_mask[sizeof(xen_ulong_t) * 8];
 
 	/*
 	 * Wallclock time: updated only by control software. Guests should base
-- 
1.7.2.5

Re: [PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long

[Stefano Stabellini]

On Thu, 14 Feb 2013, Ian Campbell wrote:
> On ARM we want these to be the same size on 32- and 64-bit.
> 
> This is an ABI change on ARM. X86 does not change.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir (Xen.org) <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  arch/arm/include/asm/xen/events.h |   22 +++++++++++
>  arch/x86/include/asm/xen/events.h |    2 +
>  drivers/xen/events.c              |   75 ++++++++++++++++++++----------------
>  include/xen/interface/xen.h       |    8 ++--
>  4 files changed, 70 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/events.h b/arch/arm/include/asm/xen/events.h
> index 94b4e90..92dbb89 100644
> --- a/arch/arm/include/asm/xen/events.h
> +++ b/arch/arm/include/asm/xen/events.h
> @@ -15,4 +15,26 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
>  	return raw_irqs_disabled_flags(regs->ARM_cpsr);
>  }
>  
> +/*
> + * We cannot use xchg because it does not support 8-byte
> + * values. However it is safe to use {ldr,dtd}exd directly because all
> + * platforms which Xen can run on support those instructions.
> + */
> +static inline xen_ulong_t xen_read_evtchn_pending_sel(struct vcpu_info *vcpu_info)
> +{
> +	xen_ulong_t val;
> +	unsigned int tmp;
> +
> +	wmb();
> +	asm volatile("@ read_evtchn_pending_sel\n"
> +		"1:     ldrexd  %0, %H0, [%3]\n"
> +		"       strexd  %1, %2, %H2, [%3]\n"
> +		"       teq     %1, #0\n"
> +		"       bne     1b"
> +			: "=&r" (val), "=&r" (tmp)
> +			: "r" (0), "r" (&vcpu_info->evtchn_pending_sel)
> +			: "memory", "cc");
> +	return val;
> +}

At this point we might as well introduce an xchg64, it is going to be
more useful to us in the future and to other non-Xen people too.



>  #endif /* _ASM_ARM_XEN_EVENTS_H */
> diff --git a/arch/x86/include/asm/xen/events.h b/arch/x86/include/asm/xen/events.h
> index cc146d5..55edd58 100644
> --- a/arch/x86/include/asm/xen/events.h
> +++ b/arch/x86/include/asm/xen/events.h
> @@ -16,4 +16,6 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
>  	return raw_irqs_disabled_flags(regs->flags);
>  }
>  
> +#define xen_read_evtchn_pending_sel(v) xchg(&(v)->evtchn_pending_sel, 0)
> +
>  #endif /* _ASM_X86_XEN_EVENTS_H */
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 0be4df3..e90a440 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -80,6 +80,12 @@ enum xen_irq_type {
>  };
>  
>  /*
> + * Make a bitmask (i.e. unsigned long *) of a xen_ulong_t
> + * array. Primarily to avoid long lines (hence the terse name).
> + */
> +#define BM(x) (unsigned long *)(x)
> +
> +/*
>   * Packed IRQ information:
>   * type - enum xen_irq_type
>   * event channel - irq->event channel mapping
> @@ -120,7 +126,9 @@ static unsigned long *pirq_eoi_map;
>  #endif
>  static bool (*pirq_needs_eoi)(unsigned irq);
>  
> -static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> +#define BITS_PER_EVTCHN_WORD (sizeof(xen_ulong_t)*8)
> +
> +static DEFINE_PER_CPU(xen_ulong_t [NR_EVENT_CHANNELS/BITS_PER_EVTCHN_WORD],
>  		      cpu_evtchn_mask);
>  
>  /* Xen will never allocate port zero for any purpose. */
> @@ -312,8 +320,8 @@ static void bind_evtchn_to_cpu(unsigned int chn, unsigned int cpu)
>  	cpumask_copy(irq_to_desc(irq)->irq_data.affinity, cpumask_of(cpu));
>  #endif
>  
> -	clear_bit(chn, per_cpu(cpu_evtchn_mask, cpu_from_irq(irq)));
> -	set_bit(chn, per_cpu(cpu_evtchn_mask, cpu));
> +	clear_bit(chn, BM(per_cpu(cpu_evtchn_mask, cpu_from_irq(irq))));
> +	set_bit(chn, BM(per_cpu(cpu_evtchn_mask, cpu)));
>  
>  	info_for_irq(irq)->cpu = cpu;
>  }
> @@ -339,19 +347,19 @@ static void init_evtchn_cpu_bindings(void)
>  static inline void clear_evtchn(int port)
>  {
>  	struct shared_info *s = HYPERVISOR_shared_info;
> -	sync_clear_bit(port, &s->evtchn_pending[0]);
> +	sync_clear_bit(port, BM(&s->evtchn_pending[0]));
>  }
>  
>  static inline void set_evtchn(int port)
>  {
>  	struct shared_info *s = HYPERVISOR_shared_info;
> -	sync_set_bit(port, &s->evtchn_pending[0]);
> +	sync_set_bit(port, BM(&s->evtchn_pending[0]));
>  }
>  
>  static inline int test_evtchn(int port)
>  {
>  	struct shared_info *s = HYPERVISOR_shared_info;
> -	return sync_test_bit(port, &s->evtchn_pending[0]);
> +	return sync_test_bit(port, BM(&s->evtchn_pending[0]));
>  }
>  

Are you sure that the implementation of sync_test_bit, sync_set_bit,
etc, can cope with a bit > 32?
For example ____atomic_set_bit blatantly (bit & 31) at the beginning of
the function.


> @@ -375,7 +383,7 @@ EXPORT_SYMBOL_GPL(notify_remote_via_irq);
>  static void mask_evtchn(int port)
>  {
>  	struct shared_info *s = HYPERVISOR_shared_info;
> -	sync_set_bit(port, &s->evtchn_mask[0]);
> +	sync_set_bit(port, BM(&s->evtchn_mask[0]));
>  }
>  
>  static void unmask_evtchn(int port)
> @@ -389,7 +397,7 @@ static void unmask_evtchn(int port)
>  	if (unlikely((cpu != cpu_from_evtchn(port))))
>  		do_hypercall = 1;
>  	else
> -		evtchn_pending = sync_test_bit(port, &s->evtchn_pending[0]);
> +		evtchn_pending = sync_test_bit(port, BM(&s->evtchn_pending[0]));
>  
>  	if (unlikely(evtchn_pending && xen_hvm_domain()))
>  		do_hypercall = 1;
> @@ -403,7 +411,7 @@ static void unmask_evtchn(int port)
>  	} else {
>  		struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
>  
> -		sync_clear_bit(port, &s->evtchn_mask[0]);
> +		sync_clear_bit(port, BM(&s->evtchn_mask[0]));
>  
>  		/*
>  		 * The following is basically the equivalent of
> @@ -411,8 +419,8 @@ static void unmask_evtchn(int port)
>  		 * the interrupt edge' if the channel is masked.
>  		 */
>  		if (evtchn_pending &&
> -		    !sync_test_and_set_bit(port / BITS_PER_LONG,
> -					   &vcpu_info->evtchn_pending_sel))
> +		    !sync_test_and_set_bit(port / BITS_PER_EVTCHN_WORD,
> +					   BM(&vcpu_info->evtchn_pending_sel)))
>  			vcpu_info->evtchn_upcall_pending = 1;
>  	}
>  
> @@ -1189,7 +1197,7 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>  {
>  	struct shared_info *sh = HYPERVISOR_shared_info;
>  	int cpu = smp_processor_id();
> -	unsigned long *cpu_evtchn = per_cpu(cpu_evtchn_mask, cpu);
> +	xen_ulong_t *cpu_evtchn = per_cpu(cpu_evtchn_mask, cpu);
>  	int i;
>  	unsigned long flags;
>  	static DEFINE_SPINLOCK(debug_lock);
> @@ -1205,7 +1213,7 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>  		pending = (get_irq_regs() && i == cpu)
>  			? xen_irqs_disabled(get_irq_regs())
>  			: v->evtchn_upcall_mask;
> -		printk("%d: masked=%d pending=%d event_sel %0*lx\n  ", i,
> +		printk("%d: masked=%d pending=%d event_sel %0*"PRI_xen_ulong"\n  ", i,
>  		       pending, v->evtchn_upcall_pending,
>  		       (int)(sizeof(v->evtchn_pending_sel)*2),
>  		       v->evtchn_pending_sel);
> @@ -1214,25 +1222,27 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>  
>  	printk("\npending:\n   ");
>  	for (i = ARRAY_SIZE(sh->evtchn_pending)-1; i >= 0; i--)
> -		printk("%0*lx%s", (int)sizeof(sh->evtchn_pending[0])*2,
> +		printk("%0*"PRI_xen_ulong"%s",
> +		       (int)sizeof(sh->evtchn_pending[0])*2,
>  		       sh->evtchn_pending[i],
>  		       i % 8 == 0 ? "\n   " : " ");
>  	printk("\nglobal mask:\n   ");
>  	for (i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
> -		printk("%0*lx%s",
> +		printk("%0*"PRI_xen_ulong"%s",
>  		       (int)(sizeof(sh->evtchn_mask[0])*2),
>  		       sh->evtchn_mask[i],
>  		       i % 8 == 0 ? "\n   " : " ");
>  
>  	printk("\nglobally unmasked:\n   ");
>  	for (i = ARRAY_SIZE(sh->evtchn_mask)-1; i >= 0; i--)
> -		printk("%0*lx%s", (int)(sizeof(sh->evtchn_mask[0])*2),
> +		printk("%0*"PRI_xen_ulong"%s",
> +		       (int)(sizeof(sh->evtchn_mask[0])*2),
>  		       sh->evtchn_pending[i] & ~sh->evtchn_mask[i],
>  		       i % 8 == 0 ? "\n   " : " ");
>  
>  	printk("\nlocal cpu%d mask:\n   ", cpu);
> -	for (i = (NR_EVENT_CHANNELS/BITS_PER_LONG)-1; i >= 0; i--)
> -		printk("%0*lx%s", (int)(sizeof(cpu_evtchn[0])*2),
> +	for (i = (NR_EVENT_CHANNELS/BITS_PER_EVTCHN_WORD)-1; i >= 0; i--)
> +		printk("%0*"PRI_xen_ulong"%s", (int)(sizeof(cpu_evtchn[0])*2),
>  		       cpu_evtchn[i],
>  		       i % 8 == 0 ? "\n   " : " ");
>  
> @@ -1247,16 +1257,16 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>  
>  	printk("\npending list:\n");
>  	for (i = 0; i < NR_EVENT_CHANNELS; i++) {
> -		if (sync_test_bit(i, sh->evtchn_pending)) {
> -			int word_idx = i / BITS_PER_LONG;
> +		if (sync_test_bit(i, BM(sh->evtchn_pending))) {
> +			int word_idx = i / BITS_PER_EVTCHN_WORD;
>  			printk("  %d: event %d -> irq %d%s%s%s\n",
>  			       cpu_from_evtchn(i), i,
>  			       evtchn_to_irq[i],
> -			       sync_test_bit(word_idx, &v->evtchn_pending_sel)
> +			       sync_test_bit(word_idx, BM(&v->evtchn_pending_sel))
>  					     ? "" : " l2-clear",
> -			       !sync_test_bit(i, sh->evtchn_mask)
> +			       !sync_test_bit(i, BM(sh->evtchn_mask))
>  					     ? "" : " globally-masked",
> -			       sync_test_bit(i, cpu_evtchn)
> +			       sync_test_bit(i, BM(cpu_evtchn))
>  					     ? "" : " locally-masked");
>  		}
>  	}
> @@ -1304,9 +1314,8 @@ static void __xen_evtchn_do_upcall(void)
>  
>  #ifndef CONFIG_X86 /* No need for a barrier -- XCHG is a barrier on x86. */
>  		/* Clear master flag /before/ clearing selector flag. */
> -		wmb();
>  #endif
> -		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
> +		pending_words = xen_read_evtchn_pending_sel(vcpu_info);
>  
>  		start_word_idx = __this_cpu_read(current_word_idx);
>  		start_bit_idx = __this_cpu_read(current_bit_idx);
> @@ -1355,7 +1364,7 @@ static void __xen_evtchn_do_upcall(void)
>  				bit_idx = __ffs(bits);
>  
>  				/* Process port. */
> -				port = (word_idx * BITS_PER_LONG) + bit_idx;
> +				port = (word_idx * BITS_PER_EVTCHN_WORD) + bit_idx;
>  				irq = evtchn_to_irq[port];
>  
>  				if (irq != -1) {
> @@ -1364,12 +1373,12 @@ static void __xen_evtchn_do_upcall(void)
>  						generic_handle_irq_desc(irq, desc);
>  				}
>  
> -				bit_idx = (bit_idx + 1) % BITS_PER_LONG;
> +				bit_idx = (bit_idx + 1) % BITS_PER_EVTCHN_WORD;
>  
>  				/* Next caller starts at last processed + 1 */
>  				__this_cpu_write(current_word_idx,
>  						 bit_idx ? word_idx :
> -						 (word_idx+1) % BITS_PER_LONG);
> +						 (word_idx+1) % BITS_PER_EVTCHN_WORD);
>  				__this_cpu_write(current_bit_idx, bit_idx);
>  			} while (bit_idx != 0);
>  
> @@ -1377,7 +1386,7 @@ static void __xen_evtchn_do_upcall(void)
>  			if ((word_idx != start_word_idx) || (i != 0))
>  				pending_words &= ~(1UL << word_idx);
>  
> -			word_idx = (word_idx + 1) % BITS_PER_LONG;
> +			word_idx = (word_idx + 1) % BITS_PER_EVTCHN_WORD;
>  		}
>  
>  		BUG_ON(!irqs_disabled());
> @@ -1487,8 +1496,8 @@ int resend_irq_on_evtchn(unsigned int irq)
>  	if (!VALID_EVTCHN(evtchn))
>  		return 1;
>  
> -	masked = sync_test_and_set_bit(evtchn, s->evtchn_mask);
> -	sync_set_bit(evtchn, s->evtchn_pending);
> +	masked = sync_test_and_set_bit(evtchn, BM(s->evtchn_mask));
> +	sync_set_bit(evtchn, BM(s->evtchn_pending));
>  	if (!masked)
>  		unmask_evtchn(evtchn);
>  
> @@ -1536,8 +1545,8 @@ static int retrigger_dynirq(struct irq_data *data)
>  	if (VALID_EVTCHN(evtchn)) {
>  		int masked;
>  
> -		masked = sync_test_and_set_bit(evtchn, sh->evtchn_mask);
> -		sync_set_bit(evtchn, sh->evtchn_pending);
> +		masked = sync_test_and_set_bit(evtchn, BM(sh->evtchn_mask));
> +		sync_set_bit(evtchn, BM(sh->evtchn_pending));
>  		if (!masked)
>  			unmask_evtchn(evtchn);
>  		ret = 1;
> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
> index 886a5d8..53ec416 100644
> --- a/include/xen/interface/xen.h
> +++ b/include/xen/interface/xen.h
> @@ -285,7 +285,7 @@ DEFINE_GUEST_HANDLE_STRUCT(multicall_entry);
>   * Event channel endpoints per domain:
>   *  1024 if a long is 32 bits; 4096 if a long is 64 bits.
>   */
> -#define NR_EVENT_CHANNELS (sizeof(unsigned long) * sizeof(unsigned long) * 64)
> +#define NR_EVENT_CHANNELS (sizeof(xen_ulong_t) * sizeof(xen_ulong_t) * 64)
>  
>  struct vcpu_time_info {
>  	/*
> @@ -341,7 +341,7 @@ struct vcpu_info {
>  	 */
>  	uint8_t evtchn_upcall_pending;
>  	uint8_t evtchn_upcall_mask;
> -	unsigned long evtchn_pending_sel;
> +	xen_ulong_t evtchn_pending_sel;
>  	struct arch_vcpu_info arch;
>  	struct pvclock_vcpu_time_info time;
>  }; /* 64 bytes (x86) */
> @@ -384,8 +384,8 @@ struct shared_info {
>  	 * per-vcpu selector word to be set. Each bit in the selector covers a
>  	 * 'C long' in the PENDING bitfield array.
>  	 */
> -	unsigned long evtchn_pending[sizeof(unsigned long) * 8];
> -	unsigned long evtchn_mask[sizeof(unsigned long) * 8];
> +	xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8];
> +	xen_ulong_t evtchn_mask[sizeof(xen_ulong_t) * 8];
>  
>  	/*
>  	 * Wallclock time: updated only by control software. Guests should base
> -- 
> 1.7.2.5
> 

Re: [PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long

[Ian Campbell]

On Fri, 2013-02-15 at 12:22 +0000, Stefano Stabellini wrote:
> On Thu, 14 Feb 2013, Ian Campbell wrote:
> > On ARM we want these to be the same size on 32- and 64-bit.
> > 
> > This is an ABI change on ARM. X86 does not change.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Jan Beulich <JBeulich@suse.com>
> > Cc: Keir (Xen.org) <keir@xen.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  arch/arm/include/asm/xen/events.h |   22 +++++++++++
> >  arch/x86/include/asm/xen/events.h |    2 +
> >  drivers/xen/events.c              |   75 ++++++++++++++++++++----------------
> >  include/xen/interface/xen.h       |    8 ++--
> >  4 files changed, 70 insertions(+), 37 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/xen/events.h b/arch/arm/include/asm/xen/events.h
> > index 94b4e90..92dbb89 100644
> > --- a/arch/arm/include/asm/xen/events.h
> > +++ b/arch/arm/include/asm/xen/events.h
> > @@ -15,4 +15,26 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
> >  	return raw_irqs_disabled_flags(regs->ARM_cpsr);
> >  }
> >  
> > +/*
> > + * We cannot use xchg because it does not support 8-byte
> > + * values. However it is safe to use {ldr,dtd}exd directly because all
> > + * platforms which Xen can run on support those instructions.
> > + */
> > +static inline xen_ulong_t xen_read_evtchn_pending_sel(struct vcpu_info *vcpu_info)
> > +{
> > +	xen_ulong_t val;
> > +	unsigned int tmp;
> > +
> > +	wmb();
> > +	asm volatile("@ read_evtchn_pending_sel\n"
> > +		"1:     ldrexd  %0, %H0, [%3]\n"
> > +		"       strexd  %1, %2, %H2, [%3]\n"
> > +		"       teq     %1, #0\n"
> > +		"       bne     1b"
> > +			: "=&r" (val), "=&r" (tmp)
> > +			: "r" (0), "r" (&vcpu_info->evtchn_pending_sel)
> > +			: "memory", "cc");
> > +	return val;
> > +}
> 
> At this point we might as well introduce an xchg64, it is going to be
> more useful to us in the future and to other non-Xen people too.

The problem with that is the conditional nature of these instructions.
We are guaranteed to have them because LPAE and/or the virtualisation
extensions require them, but other ARMv7 implementations may not have
this.

> >  static inline int test_evtchn(int port)
> >  {
> >  	struct shared_info *s = HYPERVISOR_shared_info;
> > -	return sync_test_bit(port, &s->evtchn_pending[0]);
> > +	return sync_test_bit(port, BM(&s->evtchn_pending[0]));
> >  }
> >  
> 
> Are you sure that the implementation of sync_test_bit, sync_set_bit,
> etc, can cope with a bit > 32?
> For example ____atomic_set_bit blatantly (bit & 31) at the beginning of
> the function.

I must confess I thought that these bit mask functions were designed to
work on arbitrary sized bitmaps. I was so sure I didn't even check...

        static inline void ____atomic_set_bit(unsigned int bit, volatile unsigned long *p)
        {
                unsigned long flags;
                unsigned long mask = 1UL << (bit & 31);
        
                p += bit >> 5;
        
                raw_local_irq_save(flags);
                *p |= mask;
                raw_local_irq_restore(flags);
        }
        
This is OK, it figures out the mask for the word containing @bit and
then increments @p to point to that word. The other bit ops are similar.

I think David V is right to be concerned about __ffs though, I'll need
to investigate that one further.

Ian.

Re: [PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long

[David Vrabel]

On 14/02/13 14:19, Ian Campbell wrote:
> On ARM we want these to be the same size on 32- and 64-bit.
> 
> This is an ABI change on ARM. X86 does not change.

Does this actually work for all possible events?  In a 32-bit guest on a
64-bit hypervisor, does the __ffs() find set bits in the upper 32-bit
word of the pending selector?  I don't think so.

David

[PATCH 2/2] xen: make start_info x86 specific.

[Ian Campbell]

ARM has no start info. Although it does not existing in the hypervisor ABI we
synthesize one for the benefit of the common code.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir (Xen.org) <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/include/asm/xen/interface.h |   24 +++++++++++++++++++++++
 arch/x86/include/asm/xen/interface.h |   35 ++++++++++++++++++++++++++++++++++
 include/xen/interface/xen.h          |   33 --------------------------------
 3 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 1151188..1db40d5 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -51,6 +51,30 @@ DEFINE_GUEST_HANDLE(uint32_t);
 DEFINE_GUEST_HANDLE(xen_pfn_t);
 DEFINE_GUEST_HANDLE(xen_ulong_t);
 
+/*
+ * On ARM this is not part of the hypervisor ABI but we provide it
+ * internally for the benefit of common code.
+ */
+struct start_info {
+        uint32_t flags;             /* SIF_xxx flags.                         */
+        uint32_t store_evtchn;      /* Event channel for store communication. */
+        xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
+        union {
+                struct {
+                        xen_pfn_t mfn;      /* MACHINE page number of console page.   */
+                        uint32_t  evtchn;   /* Event channel for console page.        */
+                } domU;
+                struct {
+                        uint32_t info_off;  /* Offset of console_info struct.         */
+                        uint32_t info_size; /* Size of console_info struct from start.*/
+                } dom0;
+        } console;
+	/* UNUSED ON ARM */
+        unsigned long nr_pages;     /* Total pages allocated to this domain.  */
+};
+#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
+#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
+
 /* Maximum number of virtual CPUs in multi-processor guests. */
 #define MAX_VIRT_CPUS 1
 
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index fd9cb76..ca9a203 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -131,6 +131,41 @@ struct arch_shared_info {
 #include <asm/pvclock-abi.h>
 
 #ifndef __ASSEMBLY__
+
+
+#define MAX_GUEST_CMDLINE 1024
+struct start_info {
+    /* THE FOLLOWING ARE FILLED IN BOTH ON INITIAL BOOT AND ON RESUME.    */
+    char magic[32];             /* "xen-<version>-<platform>".            */
+    unsigned long nr_pages;     /* Total pages allocated to this domain.  */
+    unsigned long shared_info;  /* MACHINE address of shared info struct. */
+    uint32_t flags;             /* SIF_xxx flags.                         */
+    xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
+    uint32_t store_evtchn;      /* Event channel for store communication. */
+    union {
+        struct {
+            xen_pfn_t mfn;      /* MACHINE page number of console page.   */
+            uint32_t  evtchn;   /* Event channel for console page.        */
+        } domU;
+        struct {
+            uint32_t info_off;  /* Offset of console_info struct.         */
+            uint32_t info_size; /* Size of console_info struct from start.*/
+        } dom0;
+    } console;
+    /* THE FOLLOWING ARE ONLY FILLED IN ON INITIAL BOOT (NOT RESUME).     */
+    unsigned long pt_base;      /* VIRTUAL address of page directory.     */
+    unsigned long nr_pt_frames; /* Number of bootstrap p.t. frames.       */
+    unsigned long mfn_list;     /* VIRTUAL address of page-frame list.    */
+    unsigned long mod_start;    /* VIRTUAL address of pre-loaded module.  */
+    unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
+    int8_t cmd_line[MAX_GUEST_CMDLINE];
+};
+
+/* These flags are passed in the 'flags' field of start_info_t. */
+#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
+#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
+#define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
+
 /*
  * The following is all CPU context. Note that the fpu_ctxt block is filled
  * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 53ec416..a9075df 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -422,34 +422,6 @@ struct shared_info {
  *     extended by an extra 4MB to ensure this.
  */
 
-#define MAX_GUEST_CMDLINE 1024
-struct start_info {
-	/* THE FOLLOWING ARE FILLED IN BOTH ON INITIAL BOOT AND ON RESUME.    */
-	char magic[32];             /* "xen-<version>-<platform>".            */
-	unsigned long nr_pages;     /* Total pages allocated to this domain.  */
-	unsigned long shared_info;  /* MACHINE address of shared info struct. */
-	uint32_t flags;             /* SIF_xxx flags.                         */
-	xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
-	uint32_t store_evtchn;      /* Event channel for store communication. */
-	union {
-		struct {
-			xen_pfn_t mfn;      /* MACHINE page number of console page.   */
-			uint32_t  evtchn;   /* Event channel for console page.        */
-		} domU;
-		struct {
-			uint32_t info_off;  /* Offset of console_info struct.         */
-			uint32_t info_size; /* Size of console_info struct from start.*/
-		} dom0;
-	} console;
-	/* THE FOLLOWING ARE ONLY FILLED IN ON INITIAL BOOT (NOT RESUME).     */
-	unsigned long pt_base;      /* VIRTUAL address of page directory.     */
-	unsigned long nr_pt_frames; /* Number of bootstrap p.t. frames.       */
-	unsigned long mfn_list;     /* VIRTUAL address of page-frame list.    */
-	unsigned long mod_start;    /* VIRTUAL address of pre-loaded module.  */
-	unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
-	int8_t cmd_line[MAX_GUEST_CMDLINE];
-};
-
 struct dom0_vga_console_info {
 	uint8_t video_type;
 #define XEN_VGATYPE_TEXT_MODE_3 0x03
@@ -490,11 +462,6 @@ struct dom0_vga_console_info {
 	} u;
 };
 
-/* These flags are passed in the 'flags' field of start_info_t. */
-#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
-#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
-#define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
-
 typedef uint64_t cpumap_t;
 
 typedef uint8_t xen_domain_handle_t[16];
-- 
1.7.2.5

Re: [PATCH 2/2] xen: make start_info x86 specific.

[Stefano Stabellini]

On Thu, 14 Feb 2013, Ian Campbell wrote:
> ARM has no start info. Although it does not existing in the hypervisor ABI we
> synthesize one for the benefit of the common code.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir (Xen.org) <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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


>  arch/arm/include/asm/xen/interface.h |   24 +++++++++++++++++++++++
>  arch/x86/include/asm/xen/interface.h |   35 ++++++++++++++++++++++++++++++++++
>  include/xen/interface/xen.h          |   33 --------------------------------
>  3 files changed, 59 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
> index 1151188..1db40d5 100644
> --- a/arch/arm/include/asm/xen/interface.h
> +++ b/arch/arm/include/asm/xen/interface.h
> @@ -51,6 +51,30 @@ DEFINE_GUEST_HANDLE(uint32_t);
>  DEFINE_GUEST_HANDLE(xen_pfn_t);
>  DEFINE_GUEST_HANDLE(xen_ulong_t);
>  
> +/*
> + * On ARM this is not part of the hypervisor ABI but we provide it
> + * internally for the benefit of common code.
> + */
> +struct start_info {
> +        uint32_t flags;             /* SIF_xxx flags.                         */
> +        uint32_t store_evtchn;      /* Event channel for store communication. */
> +        xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
> +        union {
> +                struct {
> +                        xen_pfn_t mfn;      /* MACHINE page number of console page.   */
> +                        uint32_t  evtchn;   /* Event channel for console page.        */
> +                } domU;
> +                struct {
> +                        uint32_t info_off;  /* Offset of console_info struct.         */
> +                        uint32_t info_size; /* Size of console_info struct from start.*/
> +                } dom0;
> +        } console;
> +	/* UNUSED ON ARM */
> +        unsigned long nr_pages;     /* Total pages allocated to this domain.  */
> +};
> +#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
> +#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
> +
>  /* Maximum number of virtual CPUs in multi-processor guests. */
>  #define MAX_VIRT_CPUS 1
>  
> diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
> index fd9cb76..ca9a203 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -131,6 +131,41 @@ struct arch_shared_info {
>  #include <asm/pvclock-abi.h>
>  
>  #ifndef __ASSEMBLY__
> +
> +
> +#define MAX_GUEST_CMDLINE 1024
> +struct start_info {
> +    /* THE FOLLOWING ARE FILLED IN BOTH ON INITIAL BOOT AND ON RESUME.    */
> +    char magic[32];             /* "xen-<version>-<platform>".            */
> +    unsigned long nr_pages;     /* Total pages allocated to this domain.  */
> +    unsigned long shared_info;  /* MACHINE address of shared info struct. */
> +    uint32_t flags;             /* SIF_xxx flags.                         */
> +    xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
> +    uint32_t store_evtchn;      /* Event channel for store communication. */
> +    union {
> +        struct {
> +            xen_pfn_t mfn;      /* MACHINE page number of console page.   */
> +            uint32_t  evtchn;   /* Event channel for console page.        */
> +        } domU;
> +        struct {
> +            uint32_t info_off;  /* Offset of console_info struct.         */
> +            uint32_t info_size; /* Size of console_info struct from start.*/
> +        } dom0;
> +    } console;
> +    /* THE FOLLOWING ARE ONLY FILLED IN ON INITIAL BOOT (NOT RESUME).     */
> +    unsigned long pt_base;      /* VIRTUAL address of page directory.     */
> +    unsigned long nr_pt_frames; /* Number of bootstrap p.t. frames.       */
> +    unsigned long mfn_list;     /* VIRTUAL address of page-frame list.    */
> +    unsigned long mod_start;    /* VIRTUAL address of pre-loaded module.  */
> +    unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
> +    int8_t cmd_line[MAX_GUEST_CMDLINE];
> +};
> +
> +/* These flags are passed in the 'flags' field of start_info_t. */
> +#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
> +#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
> +#define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
> +
>  /*
>   * The following is all CPU context. Note that the fpu_ctxt block is filled
>   * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
> index 53ec416..a9075df 100644
> --- a/include/xen/interface/xen.h
> +++ b/include/xen/interface/xen.h
> @@ -422,34 +422,6 @@ struct shared_info {
>   *     extended by an extra 4MB to ensure this.
>   */
>  
> -#define MAX_GUEST_CMDLINE 1024
> -struct start_info {
> -	/* THE FOLLOWING ARE FILLED IN BOTH ON INITIAL BOOT AND ON RESUME.    */
> -	char magic[32];             /* "xen-<version>-<platform>".            */
> -	unsigned long nr_pages;     /* Total pages allocated to this domain.  */
> -	unsigned long shared_info;  /* MACHINE address of shared info struct. */
> -	uint32_t flags;             /* SIF_xxx flags.                         */
> -	xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
> -	uint32_t store_evtchn;      /* Event channel for store communication. */
> -	union {
> -		struct {
> -			xen_pfn_t mfn;      /* MACHINE page number of console page.   */
> -			uint32_t  evtchn;   /* Event channel for console page.        */
> -		} domU;
> -		struct {
> -			uint32_t info_off;  /* Offset of console_info struct.         */
> -			uint32_t info_size; /* Size of console_info struct from start.*/
> -		} dom0;
> -	} console;
> -	/* THE FOLLOWING ARE ONLY FILLED IN ON INITIAL BOOT (NOT RESUME).     */
> -	unsigned long pt_base;      /* VIRTUAL address of page directory.     */
> -	unsigned long nr_pt_frames; /* Number of bootstrap p.t. frames.       */
> -	unsigned long mfn_list;     /* VIRTUAL address of page-frame list.    */
> -	unsigned long mod_start;    /* VIRTUAL address of pre-loaded module.  */
> -	unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
> -	int8_t cmd_line[MAX_GUEST_CMDLINE];
> -};
> -
>  struct dom0_vga_console_info {
>  	uint8_t video_type;
>  #define XEN_VGATYPE_TEXT_MODE_3 0x03
> @@ -490,11 +462,6 @@ struct dom0_vga_console_info {
>  	} u;
>  };
>  
> -/* These flags are passed in the 'flags' field of start_info_t. */
> -#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
> -#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
> -#define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm options */
> -
>  typedef uint64_t cpumap_t;
>  
>  typedef uint8_t xen_domain_handle_t[16];
> -- 
> 1.7.2.5
> 

Re: [PATCH 2/2] xen: make start_info x86 specific.

[Jan Beulich]

>>> On 15.02.13 at 13:08, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 14 Feb 2013, Ian Campbell wrote:
>> --- a/arch/arm/include/asm/xen/interface.h
>> +++ b/arch/arm/include/asm/xen/interface.h
>> @@ -51,6 +51,30 @@ DEFINE_GUEST_HANDLE(uint32_t);
>>  DEFINE_GUEST_HANDLE(xen_pfn_t);
>>  DEFINE_GUEST_HANDLE(xen_ulong_t);
>>  
>> +/*
>> + * On ARM this is not part of the hypervisor ABI but we provide it
>> + * internally for the benefit of common code.
>> + */
>> +struct start_info {
>> +        uint32_t flags;             /* SIF_xxx flags.                         */
>> +        uint32_t store_evtchn;      /* Event channel for store communication. */
>> +        xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
>> +        union {
>> +                struct {
>> +                        xen_pfn_t mfn;      /* MACHINE page number of console page.   */
>> +                        uint32_t  evtchn;   /* Event channel for console page.        */
>> +                } domU;
>> +                struct {
>> +                        uint32_t info_off;  /* Offset of console_info struct.         */
>> +                        uint32_t info_size; /* Size of console_info struct from start.*/
>> +                } dom0;
>> +        } console;
>> +	/* UNUSED ON ARM */
>> +        unsigned long nr_pages;     /* Total pages allocated to this domain.  */
>> +};
>> +#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
>> +#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */

With this it's even less clear to me why you want all of it removed
from a architecture independent header.

Jan

Re: [PATCH 2/2] xen: make start_info x86 specific.

[Ian Campbell]

On Fri, 2013-02-15 at 13:01 +0000, Jan Beulich wrote:
> >>> On 15.02.13 at 13:08, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 14 Feb 2013, Ian Campbell wrote:
> >> --- a/arch/arm/include/asm/xen/interface.h
> >> +++ b/arch/arm/include/asm/xen/interface.h
> >> @@ -51,6 +51,30 @@ DEFINE_GUEST_HANDLE(uint32_t);
> >>  DEFINE_GUEST_HANDLE(xen_pfn_t);
> >>  DEFINE_GUEST_HANDLE(xen_ulong_t);
> >>  
> >> +/*
> >> + * On ARM this is not part of the hypervisor ABI but we provide it
> >> + * internally for the benefit of common code.
> >> + */
> >> +struct start_info {
> >> +        uint32_t flags;             /* SIF_xxx flags.                         */
> >> +        uint32_t store_evtchn;      /* Event channel for store communication. */
> >> +        xen_pfn_t store_mfn;        /* MACHINE page number of shared page.    */
> >> +        union {
> >> +                struct {
> >> +                        xen_pfn_t mfn;      /* MACHINE page number of console page.   */
> >> +                        uint32_t  evtchn;   /* Event channel for console page.        */
> >> +                } domU;
> >> +                struct {
> >> +                        uint32_t info_off;  /* Offset of console_info struct.         */
> >> +                        uint32_t info_size; /* Size of console_info struct from start.*/
> >> +                } dom0;
> >> +        } console;
> >> +	/* UNUSED ON ARM */
> >> +        unsigned long nr_pages;     /* Total pages allocated to this domain.  */
> >> +};
> >> +#define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
> >> +#define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control domain? */
> 
> With this it's even less clear to me why you want all of it removed
> from a architecture independent header.

struct start_info is a hypervisor on x86, on ARM it is not, it's use is
a purely internal convenience for the guest, to allow it to share common
code between x86 and ARM.

Maybe moving things on the guest side is overkill, I did it for symmetry
with the Xen side patches.

All I actually care about is the removal of struct start_info from the
canonical copy of the hypervisor ABI (that is xen/include/public) for
ARM.

Ian.

Re: [PATCH 2/2] xen: make start_info x86 specific.

[Jan Beulich]

>>> On 15.02.13 at 14:24, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> All I actually care about is the removal of struct start_info from the
> canonical copy of the hypervisor ABI (that is xen/include/public) for
> ARM.

But as already said - to me this looks wrong. The structure has
generally useful fields (and unless ARM uses _none_ of them,
then it must have some other way to communicate the respective
stuff, in which case I'd ask why a second mechanism got invented
when there already was one available), and even more so from
the abstract PV perspective.

Jan

Re: [PATCH 2/2] xen: make start_info x86 specific.

[Ian Campbell]

On Fri, 2013-02-15 at 13:41 +0000, Jan Beulich wrote:
> >>> On 15.02.13 at 14:24, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > All I actually care about is the removal of struct start_info from the
> > canonical copy of the hypervisor ABI (that is xen/include/public) for
> > ARM.
> 
> But as already said - to me this looks wrong. The structure has
> generally useful fields (and unless ARM uses _none_ of them,

This is right, ARM uses none of them, at the hypervisor ABI level at
least. There's actually no way to pass a start info to an ARM guest
because they are launched via the native code paths.

> then it must have some other way to communicate the respective
> stuff, in which case I'd ask why a second mechanism got invented
> when there already was one available), and even more so from
> the abstract PV perspective.

ARM uses HVM params (e.g. the xenstore and console ring + evtchn) and
XENFEAT_dom0 etc (for privilegedness).

I guess it is obvious why HVM params were needed on HVM and you invented
XENFEAT_dom0 IIRC :-)

Ian.

Re: [PATCH 2/2] xen: make start_info x86 specific.

[Jan Beulich]

>>> On 15.02.13 at 15:06, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> I guess it is obvious why HVM params were needed on HVM and you invented
> XENFEAT_dom0 IIRC :-)

Yeah, as capability indication from the kernel to the hypervisor. The
inverse (indicating the granted privilege to the kernel) was only a
side effect.

Jan

Re: [PATCH 00/NN] linux: public interface changes for arm

[Ian Campbell]

Ahem, NN == 02. Sorry.

On Thu, 2013-02-14 at 14:16 +0000, Ian Campbell wrote:
> The main change here is to switch evtchns to xen_ulong_t on arm, this
> enables us to have the same interface on arm32 and arm64. This is an ABI
> change for ARM but not x86. This is a counterpart to the Xen patch just
> posted.
> 
> In that series I also made start_info x86 specific. ARM does not use
> this struct in its ABI but for convenience in non-arch specific code I
> have included an internal (non ABI) version on ARM too. Long term
> perhaps we should consider improving the abstraction used in the common
> code to avoid this.
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel