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.
Ian Campbell
2013-Feb-14 14:19 UTC
[PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long
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
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
Ian Campbell
2013-Feb-14 14:20 UTC
Re: [PATCH 00/NN] linux: public interface changes for arm
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
Stefano Stabellini
2013-Feb-15 12:08 UTC
Re: [PATCH 2/2] xen: make start_info x86 specific.
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 >
Stefano Stabellini
2013-Feb-15 12:22 UTC
Re: [PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long
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 >
>>> 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
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.
>>> 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
David Vrabel
2013-Feb-15 13:54 UTC
Re: [PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long
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
Ian Campbell
2013-Feb-15 14:03 UTC
Re: [PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long
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.
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.
>>> 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