Re: [PATCH v2 02/23] xen/arm: hypercalls
On Mon, Aug 06, 2012 at 03:27:05PM +0100, Stefano Stabellini wrote:
> Use r12 to pass the hypercall number to the hypervisor.
>
> We need a register to pass the hypercall number because we might not
> know it at compile time and HVC only takes an immediate argument.
>
> Among the available registers r12 seems to be the best choice because it
> is defined as "intra-procedure call scratch register".
>
> Use the ISS to pass an hypervisor specific tag.
>
> Changes in v2:
> - define an HYPERCALL macro for 5 arguments hypercall wrappers, even if
> at the moment is unused;
> - use ldm instead of pop;
> - fix up comments.
>
> Signed-off-by: Stefano Stabellini
> ---
> arch/arm/include/asm/xen/hypercall.h | 50
> arch/arm/xen/Makefile|2 +-
> arch/arm/xen/hypercall.S | 106
> ++
> 3 files changed, 157 insertions(+), 1 deletions(-)
> create mode 100644 arch/arm/include/asm/xen/hypercall.h
> create mode 100644 arch/arm/xen/hypercall.S
[...]
> diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
> new file mode 100644
> index 000..074f5ed
> --- /dev/null
> +++ b/arch/arm/xen/hypercall.S
> @@ -0,0 +1,106 @@
> +/**
> + * hypercall.S
> + *
> + * Xen hypercall wrappers
> + *
> + * Stefano Stabellini , Citrix, 2012
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation; or, when distributed
> + * separately from the Linux kernel or incorporated into other
> + * software packages, subject to the following license:
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this source file (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use, copy, modify,
> + * merge, publish, distribute, sublicense, and/or sell copies of the
> Software,
> + * and to permit persons to whom the Software is furnished to do so, subject
> to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +/*
> + * The Xen hypercall calling convention is very similar to the ARM
> + * procedure calling convention: the first paramter is passed in r0, the
> + * second in r1, the third in r2 and the fourth in r3. Considering that
> + * Xen hypercalls have 5 arguments at most, the fifth paramter is passed
> + * in r4, differently from the procedure calling convention of using the
> + * stack for that case.
> + *
> + * The hypercall number is passed in r12.
> + *
> + * The return value is in r0.
> + *
> + * The hvc ISS is required to be 0xEA1, that is the Xen specific ARM
> + * hypercall tag.
> + */
> +
> +#include
> +#include
> +#include
> +
> +
> +/* HVC 0xEA1 */
> +#ifdef CONFIG_THUMB2_KERNEL
> +#define xen_hvc .word 0xf7e08ea1
> +#else
> +#define xen_hvc .word 0xe140ea71
> +#endif
Consider using my opcode injection helpers patch for this (see
separate repost: [PATCH v2 REPOST 0/4] ARM: opcodes: Facilitate custom
opcode injection), assuming that nobody objects to it. This should mean
that the right opcodes get generated when building a kernel for a big-
endian target for example.
I believe the __HVC(imm) macro which I put in as an
example should do what you need in this case.
> +
> +#define HYPERCALL_SIMPLE(hypercall) \
> +ENTRY(HYPERVISOR_##hypercall)\
> + mov r12, #__HYPERVISOR_##hypercall; \
> + xen_hvc;\
> + mov pc, lr; \
> +ENDPROC(HYPERVISOR_##hypercall)
> +
> +#define HYPERCALL0 HYPERCALL_SIMPLE
> +#define HYPERCALL1 HYPERCALL_SIMPLE
> +#define HYPERCALL2 HYPERCALL_SIMPLE
> +#define HYPERCALL3 HYPERCALL_SIMPLE
> +#define HYPERCALL4 HYPERCALL_SIMPLE
> +
> +#define HYPERCALL5(hypercall)\
> +ENTRY(HYPERVISOR_##hypercall)\
> + stmdb sp!, {r4} \
> + ldr r4, [sp, #4]\
> + mov r12, #__HYPERVISOR_##hypercall; \
> + xen_hvc \
>
Re: [Xen-devel] [PATCH v2 07/23] xen/arm: Xen detection and shared_info page mapping
On Mon, 6 Aug 2012, David Vrabel wrote:
> On 06/08/12 15:27, Stefano Stabellini wrote:
> > Check for a "/xen" node in the device tree, if it is present set
> > xen_domain_type to XEN_HVM_DOMAIN and continue initialization.
> >
> > Map the real shared info page using XENMEM_add_to_physmap with
> > XENMAPSPACE_shared_info.
> >
> > Changes in v2:
> >
> > - replace pr_info with pr_debug.
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > arch/arm/xen/enlighten.c | 52
> > ++
> > 1 files changed, 52 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index d27c2a6..102d823 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -5,6 +5,9 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > +#include
> >
> > struct start_info _xen_start_info;
> > struct start_info *xen_start_info = &_xen_start_info;
> > @@ -33,3 +36,52 @@ int xen_remap_domain_mfn_range(struct vm_area_struct
> > *vma,
> > return -ENOSYS;
> > }
> > EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> > +
> > +/*
> > + * == Xen Device Tree format ==
> > + * - /xen node;
> > + * - compatible "arm,xen";
> > + * - one interrupt for Xen event notifications;
> > + * - one memory region to map the grant_table.
> > + */
>
> These needs to be documented in Documentation/devicetree/bindings/ and
> should be sent to the devicetree-discuss mailing list for review.
That's a good idea.
> The node should be called 'hypervisor' I think.
>
> The first word of the compatible string is the vendor/organization that
> defined the binding so should be "xen" here. This does give a odd
> looking "xen,xen" but we'll have to live with that.
>
> I'd suggest that the DT provided by the hypervisor or tools give the
> hypercall ABI version in the compatible string as well. e.g.,
>
> hypervisor {
> compatible = "xen,xen-4.3", "xen,xen"
> };
It makes sense, I'll do that.
> I missed the Xen patch that adds this node for dom0. Can you point me
> to it?
Nope, you didn't miss it: I don't have a patch for Xen yet.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 01/23] arm: initial Xen support
On Tue, 7 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 06, 2012 at 03:27:04PM +0100, Stefano Stabellini wrote:
> > - Basic hypervisor.h and interface.h definitions.
> > - Skeleton enlighten.c, set xen_start_info to an empty struct.
> > - Make xen_initial_domain dependent on the SIF_PRIVILIGED_BIT.
> >
> > The new code only compiles when CONFIG_XEN is set, that is going to be
> > added to arch/arm/Kconfig in patch #11 "xen/arm: introduce CONFIG_XEN on
> > ARM".
>
> You can add my Ack, but do one change pls:
Thanks! I'll make the changes.
> > +/* XXX: Move pvclock definitions some place arch independent */
>
> Just use 'TODO'
>
> > +struct pvclock_vcpu_time_info {
> > + u32 version;
> > + u32 pad0;
> > + u64 tsc_timestamp;
> > + u64 system_time;
> > + u32 tsc_to_system_mul;
> > + s8tsc_shift;
> > + u8flags;
> > + u8pad[2];
> > +} __attribute__((__packed__)); /* 32 bytes */
> > +
> > +struct pvclock_wall_clock {
> > + u32 version;
> > + u32 sec;
> > + u32 nsec;
> > +} __attribute__((__packed__));
>
> Mention the size and why it is OK to have it be a weird
> size while the one above is nicely padded.
>
> > +#endif
> > +
> > +#endif /* _ASM_ARM_XEN_INTERFACE_H */
> > diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
> > new file mode 100644
> > index 000..0bad594
> > --- /dev/null
> > +++ b/arch/arm/xen/Makefile
> > @@ -0,0 +1 @@
> > +obj-y := enlighten.o
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > new file mode 100644
> > index 000..d27c2a6
> > --- /dev/null
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -0,0 +1,35 @@
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +struct start_info _xen_start_info;
> > +struct start_info *xen_start_info = &_xen_start_info;
> > +EXPORT_SYMBOL_GPL(xen_start_info);
> > +
> > +enum xen_domain_type xen_domain_type = XEN_NATIVE;
> > +EXPORT_SYMBOL_GPL(xen_domain_type);
> > +
> > +struct shared_info xen_dummy_shared_info;
> > +struct shared_info *HYPERVISOR_shared_info = (void
> > *)&xen_dummy_shared_info;
> > +
> > +DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
> > +
> > +/* XXX: to be removed */
>
> s/XXX/TODO/ here, and mention pls why it needs to be removed.
>
> > +__read_mostly int xen_have_vector_callback;
> > +EXPORT_SYMBOL_GPL(xen_have_vector_callback);
> > +
> > +int xen_platform_pci_unplug = XEN_UNPLUG_ALL;
> > +EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 03/23] xen/arm: page.h definitions
On Tue, 7 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 06, 2012 at 03:27:06PM +0100, Stefano Stabellini wrote:
> > ARM Xen guests always use paging in hardware, like PV on HVM guests in
> > the X86 world.
> >
> > Signed-off-by: Stefano Stabellini
>
> Ack.. with one nitpick
>
> > +/* XXX: this shouldn't be here */
>
> .. but its here b/c the frontend drivers are using it (its rolled in
> headers)- even though we won't hit the code path. So for right now
> just punt with this.
Yep, I'll do that.
> > +static inline pte_t *lookup_address(unsigned long address, unsigned int
> > *level)
> > +{
> > + BUG();
> > + return NULL;
> > +}
> > +
> > +static inline int m2p_add_override(unsigned long mfn, struct page *page,
> > + struct gnttab_map_grant_ref *kmap_op)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline int m2p_remove_override(struct page *page, bool clear_pte)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline bool set_phys_to_machine(unsigned long pfn, unsigned long
> > mfn)
> > +{
> > + BUG();
> > + return false;
> > +}
> > +#endif /* _ASM_ARM_XEN_PAGE_H */
> > --
> > 1.7.2.5
>
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 06/23] xen: missing includes
On Tue, 7 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 06, 2012 at 03:27:09PM +0100, Stefano Stabellini wrote:
> > Changes in v2:
> > - remove pvclock hack;
> > - remove include linux/types.h from xen/interface/xen.h.
>
> I think I can take in my tree now right by itself right? Or do
> you want to keep this in your patchqueue? If so, Ack from me.
Yep, just go ahead and take the patch, thanks.
> > Signed-off-by: Stefano Stabellini
> > ---
> > arch/x86/include/asm/xen/interface.h |2 ++
> > drivers/tty/hvc/hvc_xen.c |2 ++
> > drivers/xen/grant-table.c |1 +
> > drivers/xen/xenbus/xenbus_probe_frontend.c |1 +
> > include/xen/interface/xen.h|1 -
> > include/xen/privcmd.h |1 +
> > 6 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/xen/interface.h
> > b/arch/x86/include/asm/xen/interface.h
> > index cbf0c9d..a93db16 100644
> > --- a/arch/x86/include/asm/xen/interface.h
> > +++ b/arch/x86/include/asm/xen/interface.h
> > @@ -121,6 +121,8 @@ struct arch_shared_info {
> > #include "interface_64.h"
> > #endif
> >
> > +#include
> > +
> > #ifndef __ASSEMBLY__
> > /*
> > * The following is all CPU context. Note that the fpu_ctxt block is filled
> > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > index 944eaeb..dc07f56 100644
> > --- a/drivers/tty/hvc/hvc_xen.c
> > +++ b/drivers/tty/hvc/hvc_xen.c
> > @@ -21,6 +21,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -35,6 +36,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> > index 0bfc1ef..1d0d95e 100644
> > --- a/drivers/xen/grant-table.c
> > +++ b/drivers/xen/grant-table.c
> > @@ -47,6 +47,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c
> > b/drivers/xen/xenbus/xenbus_probe_frontend.c
> > index a31b54d..3159a37 100644
> > --- a/drivers/xen/xenbus/xenbus_probe_frontend.c
> > +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
> > @@ -21,6 +21,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> >
> > diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
> > index a890804..3871e47 100644
> > --- a/include/xen/interface/xen.h
> > +++ b/include/xen/interface/xen.h
> > @@ -10,7 +10,6 @@
> > #define __XEN_PUBLIC_XEN_H__
> >
> > #include
> > -#include
> >
> > /*
> > * XEN "SYSTEM CALLS" (a.k.a. HYPERCALLS).
> > diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> > index 17857fb..4d58881 100644
> > --- a/include/xen/privcmd.h
> > +++ b/include/xen/privcmd.h
> > @@ -35,6 +35,7 @@
> >
> > #include
> > #include
> > +#include
> >
> > typedef unsigned long xen_pfn_t;
> >
> > --
> > 1.7.2.5
>
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 09/23] xen/arm: Introduce xen_ulong_t for unsigned long
On Tue, 7 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 06, 2012 at 03:27:12PM +0100, Stefano Stabellini wrote:
> > All the original Xen headers have xen_ulong_t as unsigned long type, however
> > when they have been imported in Linux, xen_ulong_t has been replaced with
> > unsigned long. That might work for x86 and ia64 but it does not for arm.
> > Bring back xen_ulong_t and let each architecture define xen_ulong_t as they
> > see fit.
> >
> > Also explicitly size pointers (__DEFINE_GUEST_HANDLE) to 64 bit.
>
> Looks ok to me.
Considering that I'll have to change it a bit in the next version
(remove the apic_physbase and multicall_entry changes), I won't add your
acked-by here just yet.
> > Signed-off-by: Stefano Stabellini
> > ---
> > arch/arm/include/asm/xen/interface.h |8 ++--
> > arch/ia64/include/asm/xen/interface.h |1 +
> > arch/x86/include/asm/xen/interface.h |1 +
> > include/xen/interface/memory.h| 12 ++--
> > include/xen/interface/physdev.h |4 ++--
> > include/xen/interface/version.h |2 +-
> > include/xen/interface/xen.h |6 +++---
> > 7 files changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/xen/interface.h
> > b/arch/arm/include/asm/xen/interface.h
> > index f904dcc..1d3030c 100644
> > --- a/arch/arm/include/asm/xen/interface.h
> > +++ b/arch/arm/include/asm/xen/interface.h
> > @@ -9,8 +9,11 @@
> >
> > #include
> >
> > +#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
> > +
> > #define __DEFINE_GUEST_HANDLE(name, type) \
> > - typedef type * __guest_handle_ ## name
> > + typedef struct { union { type *p; uint64_aligned_t q; }; } \
> > +__guest_handle_ ## name
> >
> > #define DEFINE_GUEST_HANDLE_STRUCT(name) \
> > __DEFINE_GUEST_HANDLE(name, struct name)
> > @@ -21,13 +24,14 @@
> > do {\
> > if (sizeof(hnd) == 8) \
> > *(uint64_t *)&(hnd) = 0;\
> > - (hnd) = val;\
> > + (hnd).p = val; \
> > } while (0)
> >
> > #ifndef __ASSEMBLY__
> > /* Explicitly size integers that represent pfns in the interface with
> > * Xen so that we can have one ABI that works for 32 and 64 bit guests. */
> > typedef uint64_t xen_pfn_t;
> > +typedef uint64_t xen_ulong_t;
> > /* Guest handles for primitive C types. */
> > __DEFINE_GUEST_HANDLE(uchar, unsigned char);
> > __DEFINE_GUEST_HANDLE(uint, unsigned int);
> > diff --git a/arch/ia64/include/asm/xen/interface.h
> > b/arch/ia64/include/asm/xen/interface.h
> > index 686464e..7c83445 100644
> > --- a/arch/ia64/include/asm/xen/interface.h
> > +++ b/arch/ia64/include/asm/xen/interface.h
> > @@ -71,6 +71,7 @@
> > * with Xen so that we could have one ABI that works for 32 and 64 bit
> > * guests. */
> > typedef unsigned long xen_pfn_t;
> > +typedef unsigned long xen_ulong_t;
> > /* Guest handles for primitive C types. */
> > __DEFINE_GUEST_HANDLE(uchar, unsigned char);
> > __DEFINE_GUEST_HANDLE(uint, unsigned int);
> > diff --git a/arch/x86/include/asm/xen/interface.h
> > b/arch/x86/include/asm/xen/interface.h
> > index 555f94d..28fc621 100644
> > --- a/arch/x86/include/asm/xen/interface.h
> > +++ b/arch/x86/include/asm/xen/interface.h
> > @@ -51,6 +51,7 @@
> > * with Xen so that on ARM we can have one ABI that works for 32 and 64
> > * bit guests. */
> > typedef unsigned long xen_pfn_t;
> > +typedef unsigned long xen_ulong_t;
> > /* Guest handles for primitive C types. */
> > __DEFINE_GUEST_HANDLE(uchar, unsigned char);
> > __DEFINE_GUEST_HANDLE(uint, unsigned int);
> > diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> > index abbbff0..b5c3098 100644
> > --- a/include/xen/interface/memory.h
> > +++ b/include/xen/interface/memory.h
> > @@ -34,7 +34,7 @@ struct xen_memory_reservation {
> > GUEST_HANDLE(xen_pfn_t) extent_start;
> >
> > /* Number of extents, and size/alignment of each (2^extent_order
> > pages). */
> > -unsigned long nr_extents;
> > +xen_ulong_t nr_extents;
> > unsigned int extent_order;
> >
> > /*
> > @@ -92,7 +92,7 @@ struct xen_memory_exchange {
> > * command will be non-zero.
> > * 5. THIS FIELD MUST BE INITIALISED TO ZERO BY THE CALLER!
> > */
> > -unsigned long nr_exchanged;
> > +xen_ulong_t nr_exchanged;
> > };
> >
> > DEFINE_GUEST_HANDLE_STRUCT(xen_memory_exchange);
> > @@ -148,8 +148,8 @@ DEFINE_GUEST_HANDLE_STRUCT(xen_machphys_mfn_list);
> > */
> > #define XENMEM_machphys_mapping 12
> > struct xen_machphys_mapping {
> > -unsigned long v_start, v_end; /* Start and end virtual addresses. */
> > -unsigned long max_mfn;/* Maximum MFN that can be looked up. */
> > +xen_ulong_t v_start, v_end; /* Start and end virtual addresses. */
> > +xen_ulong_t
Re: [PATCH v2 10/23] xen/arm: compile and run xenbus
On Tue, 7 Aug 2012, Daniel De Graaf wrote:
> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote:
> >> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not
> >> an error.
> >>
> >> If Linux is running as an HVM domain and is running as Dom0, use
> >> xenstored_local_init to initialize the xenstore page and event channel.
> >>
> >> Changes in v2:
> >>
> >> - refactor xenbus_init.
> >
> > Thank you. Lets also CC our friend at NSA who has been doing some work
> > in that area. Daniel are you OK with this change - will it still make
> > PV initial domain with with the MiniOS XenBus driver?
> >
> > Thanks.
>
> That case will work, but what this will break is launching the initial domain
> with a Xenstore stub domain already running (see below).
>
> >>
> >> Signed-off-by: Stefano Stabellini
> >> ---
> >> drivers/xen/xenbus/xenbus_comms.c |2 +-
> >> drivers/xen/xenbus/xenbus_probe.c | 62
> >> +---
> >> drivers/xen/xenbus/xenbus_xs.c|1 +
> >> 3 files changed, 45 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/xen/xenbus/xenbus_comms.c
> >> b/drivers/xen/xenbus/xenbus_comms.c
> >> index 52fe7ad..c5aa55c 100644
> >> --- a/drivers/xen/xenbus/xenbus_comms.c
> >> +++ b/drivers/xen/xenbus/xenbus_comms.c
> >> @@ -224,7 +224,7 @@ int xb_init_comms(void)
> >>int err;
> >>err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting,
> >>0, "xenbus", &xb_waitq);
> >> - if (err <= 0) {
> >> + if (err < 0) {
> >>printk(KERN_ERR "XENBUS request irq failed %i\n", err);
> >>return err;
> >>}
> >
> >> diff --git a/drivers/xen/xenbus/xenbus_probe.c
> >> b/drivers/xen/xenbus/xenbus_probe.c
> >> index b793723..a67ccc0 100644
> >> --- a/drivers/xen/xenbus/xenbus_probe.c
> >> +++ b/drivers/xen/xenbus/xenbus_probe.c
> >> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void)
> >>return err;
> >> }
> >>
> >> +enum xenstore_init {
> >> + UNKNOWN,
> >> + PV,
> >> + HVM,
> >> + LOCAL,
> >> +};
> >> static int __init xenbus_init(void)
> >> {
> >>int err = 0;
> >> + enum xenstore_init usage = UNKNOWN;
> >> + uint64_t v = 0;
> >>
> >>if (!xen_domain())
> >>return -ENODEV;
> >>
> >>xenbus_ring_ops_init();
> >>
> >> - if (xen_hvm_domain()) {
> >> - uint64_t v = 0;
> >> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> >> - if (err)
> >> - goto out_error;
> >> - xen_store_evtchn = (int)v;
> >> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> >> - if (err)
> >> - goto out_error;
> >> - xen_store_mfn = (unsigned long)v;
> >> - xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT,
> >> PAGE_SIZE);
> >> - } else {
> >> - xen_store_evtchn = xen_start_info->store_evtchn;
> >> - xen_store_mfn = xen_start_info->store_mfn;
> >> - if (xen_store_evtchn)
> >> - xenstored_ready = 1;
> >> - else {
> >> + if (xen_pv_domain())
> >> + usage = PV;
> >> + if (xen_hvm_domain())
> >> + usage = HVM;
>
> The above is correct for domUs, and is overridden for dom0s:
>
> >> + if (xen_hvm_domain() && xen_initial_domain())
> >> + usage = LOCAL;
> >> + if (xen_pv_domain() && !xen_start_info->store_evtchn)
> >> + usage = LOCAL;
>
> Instead of these checks, I think it should just be:
>
> if (!xen_start_info->store_evtchn)
> usage = LOCAL;
>
> Any domain started after xenstore will have store_evtchn set, so if you don't
> have this set, you are either going to be running xenstore locally, or will
> use the ioctl to change it later (and so should still set up everything as if
> it will be running locally).
That would be wrong for an HVM dom0 domain (at least on ARM), because
we don't have a start_info page at all.
> >> + if (xen_pv_domain() && xen_start_info->store_evtchn)
> >> + xenstored_ready = 1;
>
> This part can now just be moved unconditionally into case PV.
What about:
if (xen_pv_domain())
usage = PV;
if (xen_hvm_domain())
usage = HVM;
if (!xen_store_evtchn)
usage = LOCAL;
and moving xenstored_ready in case PV, like you suggested.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 10/23] xen/arm: compile and run xenbus
On Wed, 8 Aug 2012, Daniel De Graaf wrote:
> On 08/08/2012 12:51 PM, Stefano Stabellini wrote:
> > On Tue, 7 Aug 2012, Daniel De Graaf wrote:
> >> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote:
> >>> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote:
> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not
> an error.
>
> If Linux is running as an HVM domain and is running as Dom0, use
> xenstored_local_init to initialize the xenstore page and event channel.
>
> Changes in v2:
>
> - refactor xenbus_init.
> >>>
> >>> Thank you. Lets also CC our friend at NSA who has been doing some work
> >>> in that area. Daniel are you OK with this change - will it still make
> >>> PV initial domain with with the MiniOS XenBus driver?
> >>>
> >>> Thanks.
> >>
> >> That case will work, but what this will break is launching the initial
> >> domain
> >> with a Xenstore stub domain already running (see below).
> >>
>
> Signed-off-by: Stefano Stabellini
> ---
> drivers/xen/xenbus/xenbus_comms.c |2 +-
> drivers/xen/xenbus/xenbus_probe.c | 62
> +---
> drivers/xen/xenbus/xenbus_xs.c|1 +
> 3 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_comms.c
> b/drivers/xen/xenbus/xenbus_comms.c
> index 52fe7ad..c5aa55c 100644
> --- a/drivers/xen/xenbus/xenbus_comms.c
> +++ b/drivers/xen/xenbus/xenbus_comms.c
> @@ -224,7 +224,7 @@ int xb_init_comms(void)
> int err;
> err = bind_evtchn_to_irqhandler(xen_store_evtchn,
> wake_waiting,
> 0, "xenbus", &xb_waitq);
> -if (err <= 0) {
> +if (err < 0) {
> printk(KERN_ERR "XENBUS request irq failed
> %i\n", err);
> return err;
> }
> >>>
> diff --git a/drivers/xen/xenbus/xenbus_probe.c
> b/drivers/xen/xenbus/xenbus_probe.c
> index b793723..a67ccc0 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void)
> return err;
> }
>
> +enum xenstore_init {
> +UNKNOWN,
> +PV,
> +HVM,
> +LOCAL,
> +};
> static int __init xenbus_init(void)
> {
> int err = 0;
> +enum xenstore_init usage = UNKNOWN;
> +uint64_t v = 0;
>
> if (!xen_domain())
> return -ENODEV;
>
> xenbus_ring_ops_init();
>
> -if (xen_hvm_domain()) {
> -uint64_t v = 0;
> -err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> -if (err)
> -goto out_error;
> -xen_store_evtchn = (int)v;
> -err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> -if (err)
> -goto out_error;
> -xen_store_mfn = (unsigned long)v;
> -xen_store_interface = ioremap(xen_store_mfn <<
> PAGE_SHIFT, PAGE_SIZE);
> -} else {
> -xen_store_evtchn = xen_start_info->store_evtchn;
> -xen_store_mfn = xen_start_info->store_mfn;
> -if (xen_store_evtchn)
> -xenstored_ready = 1;
> -else {
> +if (xen_pv_domain())
> +usage = PV;
> +if (xen_hvm_domain())
> +usage = HVM;
> >>
> >> The above is correct for domUs, and is overridden for dom0s:
> >>
> +if (xen_hvm_domain() && xen_initial_domain())
> +usage = LOCAL;
> +if (xen_pv_domain() && !xen_start_info->store_evtchn)
> +usage = LOCAL;
> >>
> >> Instead of these checks, I think it should just be:
> >>
> >> if (!xen_start_info->store_evtchn)
> >>usage = LOCAL;
> >>
> >> Any domain started after xenstore will have store_evtchn set, so if you
> >> don't
> >> have this set, you are either going to be running xenstore locally, or will
> >> use the ioctl to change it later (and so should still set up everything as
> >> if
> >> it will be running locally).
> >
> > That would be wrong for an HVM dom0 domain (at least on ARM), because
> > we don't have a start_info page at all.
> >
> >
> +if (xen_pv_domain() && xen_start_info->store_evtchn)
> +xenstored_ready = 1;
> >>
> >> This part can now just be moved unconditionally into case PV.
> >
> > What about:
> >
> > if (xen_pv_domain())
> > usage = PV;
>
Re: [PATCH v2 21/23] xen: update xen_add_to_physmap interface
On Tue, 7 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 06, 2012 at 03:27:24PM +0100, Stefano Stabellini wrote:
> > Update struct xen_add_to_physmap to be in sync with Xen's version of the
> > structure.
> > The size field was introduced by:
> >
> > changeset: 24164:707d27fe03e7
> > user:Jean Guyader
> > date:Fri Nov 18 13:42:08 2011 +
> > summary: mm: New XENMEM space, XENMAPSPACE_gmfn_range
> >
> > According to the comment:
> >
> > "This new field .size is located in the 16 bits padding between .domid
> > and .space in struct xen_add_to_physmap to stay compatible with older
> > versions."
> >
> > Changes in v2:
>
> Looks good. Let me take this as in my tree to prep it for Mukesh's patches.
OK.
Beware that patch #23 is going to modify xen_add_to_physmap again to
replace .size with a union.
> > - remove erroneous comment in the commit message.
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > include/xen/interface/memory.h |3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> > index b5c3098..b66d04c 100644
> > --- a/include/xen/interface/memory.h
> > +++ b/include/xen/interface/memory.h
> > @@ -163,6 +163,9 @@ struct xen_add_to_physmap {
> > /* Which domain to change the mapping for. */
> > domid_t domid;
> >
> > +/* Number of pages to go through for gmfn_range */
> > +uint16_tsize;
> > +
> > /* Source mapping space. */
> > #define XENMAPSPACE_shared_info 0 /* shared info page */
> > #define XENMAPSPACE_grant_table 1 /* grant table page */
> > --
> > 1.7.2.5
>
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 10/23] xen/arm: compile and run xenbus
On Wed, 8 Aug 2012, Daniel De Graaf wrote:
> On 08/08/2012 01:19 PM, Stefano Stabellini wrote:
> > On Wed, 8 Aug 2012, Daniel De Graaf wrote:
> >> On 08/08/2012 12:51 PM, Stefano Stabellini wrote:
> >>> On Tue, 7 Aug 2012, Daniel De Graaf wrote:
> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote:
> >> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not
> >> an error.
> >>
> >> If Linux is running as an HVM domain and is running as Dom0, use
> >> xenstored_local_init to initialize the xenstore page and event channel.
> >>
> >> Changes in v2:
> >>
> >> - refactor xenbus_init.
> >
> > Thank you. Lets also CC our friend at NSA who has been doing some work
> > in that area. Daniel are you OK with this change - will it still make
> > PV initial domain with with the MiniOS XenBus driver?
> >
> > Thanks.
>
> That case will work, but what this will break is launching the initial
> domain
> with a Xenstore stub domain already running (see below).
>
> >>
> >> Signed-off-by: Stefano Stabellini
> >> ---
> >> drivers/xen/xenbus/xenbus_comms.c |2 +-
> >> drivers/xen/xenbus/xenbus_probe.c | 62
> >> +---
> >> drivers/xen/xenbus/xenbus_xs.c|1 +
> >> 3 files changed, 45 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/xen/xenbus/xenbus_comms.c
> >> b/drivers/xen/xenbus/xenbus_comms.c
> >> index 52fe7ad..c5aa55c 100644
> >> --- a/drivers/xen/xenbus/xenbus_comms.c
> >> +++ b/drivers/xen/xenbus/xenbus_comms.c
> >> @@ -224,7 +224,7 @@ int xb_init_comms(void)
> >>int err;
> >>err = bind_evtchn_to_irqhandler(xen_store_evtchn,
> >> wake_waiting,
> >>0, "xenbus", &xb_waitq);
> >> - if (err <= 0) {
> >> + if (err < 0) {
> >>printk(KERN_ERR "XENBUS request irq failed
> >> %i\n", err);
> >>return err;
> >>}
> >
> >> diff --git a/drivers/xen/xenbus/xenbus_probe.c
> >> b/drivers/xen/xenbus/xenbus_probe.c
> >> index b793723..a67ccc0 100644
> >> --- a/drivers/xen/xenbus/xenbus_probe.c
> >> +++ b/drivers/xen/xenbus/xenbus_probe.c
> >> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void)
> >>return err;
> >> }
> >>
> >> +enum xenstore_init {
> >> + UNKNOWN,
> >> + PV,
> >> + HVM,
> >> + LOCAL,
> >> +};
> >> static int __init xenbus_init(void)
> >> {
> >>int err = 0;
> >> + enum xenstore_init usage = UNKNOWN;
> >> + uint64_t v = 0;
> >>
> >>if (!xen_domain())
> >>return -ENODEV;
> >>
> >>xenbus_ring_ops_init();
> >>
> >> - if (xen_hvm_domain()) {
> >> - uint64_t v = 0;
> >> - err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
> >> - if (err)
> >> - goto out_error;
> >> - xen_store_evtchn = (int)v;
> >> - err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
> >> - if (err)
> >> - goto out_error;
> >> - xen_store_mfn = (unsigned long)v;
> >> - xen_store_interface = ioremap(xen_store_mfn <<
> >> PAGE_SHIFT, PAGE_SIZE);
> >> - } else {
> >> - xen_store_evtchn = xen_start_info->store_evtchn;
> >> - xen_store_mfn = xen_start_info->store_mfn;
> >> - if (xen_store_evtchn)
> >> - xenstored_ready = 1;
> >> - else {
> >> + if (xen_pv_domain())
> >> + usage = PV;
> >> + if (xen_hvm_domain())
> >> + usage = HVM;
>
> The above is correct for domUs, and is overridden for dom0s:
>
> >> + if (xen_hvm_domain() && xen_initial_domain())
> >> + usage = LOCAL;
> >> + if (xen_pv_domain() && !xen_start_info->store_evtchn)
> >> + usage = LOCAL;
>
> Instead of these checks, I think it should just be:
>
> if (!xen_start_info->store_evtchn)
> usage = LOCAL;
>
> Any domain started after xenstore will have store_evtchn set, so if you
> don't
> have this set, you are either going to be running xenstore locally, or
> will
> use the ioctl to change it later (and so should still set up everything
> as if
> it will be running locally).
> >>>
> >>> That would be wrong for an HVM dom0 domain (at least on ARM), because
> >>> we don't have a start_info page at all.
> >>>
> >>>
Re: [PATCH v2 15/23] xen/arm: receive Xen events on ARM
On Tue, 7 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 06, 2012 at 03:27:18PM +0100, Stefano Stabellini wrote:
> > Compile events.c on ARM.
> > Parse, map and enable the IRQ to get event notifications from the device
> > tree (node "/xen").
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > arch/arm/include/asm/xen/events.h | 18 ++
> > arch/arm/xen/enlighten.c | 33 +
> > arch/x86/xen/enlighten.c |1 +
> > arch/x86/xen/irq.c|1 +
> > arch/x86/xen/xen-ops.h|1 -
> > drivers/xen/events.c | 17 ++---
> > include/xen/events.h |2 ++
> > 7 files changed, 69 insertions(+), 4 deletions(-)
> > create mode 100644 arch/arm/include/asm/xen/events.h
> >
> > diff --git a/arch/arm/include/asm/xen/events.h
> > b/arch/arm/include/asm/xen/events.h
> > new file mode 100644
> > index 000..94b4e90
> > --- /dev/null
> > +++ b/arch/arm/include/asm/xen/events.h
> > @@ -0,0 +1,18 @@
> > +#ifndef _ASM_ARM_XEN_EVENTS_H
> > +#define _ASM_ARM_XEN_EVENTS_H
> > +
> > +#include
> > +
> > +enum ipi_vector {
> > + XEN_PLACEHOLDER_VECTOR,
> > +
> > + /* Xen IPIs go here */
> > + XEN_NR_IPIS,
> > +};
> > +
> > +static inline int xen_irqs_disabled(struct pt_regs *regs)
> > +{
> > + return raw_irqs_disabled_flags(regs->ARM_cpsr);
> > +}
> > +
> > +#endif /* _ASM_ARM_XEN_EVENTS_H */
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index e5e92d5..87b17f0 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -1,4 +1,5 @@
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -9,6 +10,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -33,6 +36,8 @@ EXPORT_SYMBOL_GPL(xen_have_vector_callback);
> > int xen_platform_pci_unplug = XEN_UNPLUG_ALL;
> > EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
> >
> > +static __read_mostly int xen_events_irq = -1;
> > +
>
> So this is global..
> > int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> >unsigned long addr,
> >unsigned long mfn, int nr,
> > @@ -66,6 +71,9 @@ static int __init xen_guest_init(void)
> > if (of_address_to_resource(node, GRANT_TABLE_PHYSADDR, &res))
> > return 0;
> > xen_hvm_resume_frames = res.start >> PAGE_SHIFT;
> > + xen_events_irq = irq_of_parse_and_map(node, 0);
> > + pr_info("Xen support found, events_irq=%d gnttab_frame_pfn=%lx\n",
> > + xen_events_irq, xen_hvm_resume_frames);
> > xen_domain_type = XEN_HVM_DOMAIN;
> >
> > xen_setup_features();
> > @@ -107,3 +115,28 @@ static int __init xen_guest_init(void)
> > return 0;
> > }
> > core_initcall(xen_guest_init);
> > +
> > +static irqreturn_t xen_arm_callback(int irq, void *arg)
> > +{
> > + xen_hvm_evtchn_do_upcall();
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int __init xen_init_events(void)
> > +{
> > + if (!xen_domain() || xen_events_irq < 0)
> > + return -ENODEV;
> > +
> > + xen_init_IRQ();
> > +
> > + if (request_percpu_irq(xen_events_irq, xen_arm_callback,
> > + "events", xen_vcpu)) {
>
> But here you are asking for it to be percpu? What if there are other
> interrupts on the _other_ CPUs that conflict with it?
> > + pr_err("Error requesting IRQ %d\n", xen_events_irq);
> > + return -EINVAL;
> > + }
> > +
> > + enable_percpu_irq(xen_events_irq, 0);
>
> Uh, that is bold. One global to rule them all, eh? Should you make
> it at least:
> static DEFINE_PER_CPU(int, xen_events_irq);
> ?
That is an interesting observation.
Currently Xen is using a per-cpu interrupt (a PPI, using the GIC
terminology), and it makes sense so that we can receive event
notifications on multiple vcpus independently.
The irq range 16-31 is reserved for PPIs and I am assuming that Xen will
be able to find one spare, the same one, for all vcpus.
In fact the third field corresponding to the interrupt in the DT (0xf08
in my dts) contains the cpu mask and it is set to 0xf (the maximum)
right now.
Maybe I should just BUG_ON(xen_events_irq > 31 || xen_events_irq < 16)?
The versioning of the hypervisor node on the DT is going to help us make
any changes to the interface in the future.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH v2 10/23] xen/arm: compile and run xenbus
On 08/08/2012 01:19 PM, Stefano Stabellini wrote:
> On Wed, 8 Aug 2012, Daniel De Graaf wrote:
>> On 08/08/2012 12:51 PM, Stefano Stabellini wrote:
>>> On Tue, 7 Aug 2012, Daniel De Graaf wrote:
On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote:
>> bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not
>> an error.
>>
>> If Linux is running as an HVM domain and is running as Dom0, use
>> xenstored_local_init to initialize the xenstore page and event channel.
>>
>> Changes in v2:
>>
>> - refactor xenbus_init.
>
> Thank you. Lets also CC our friend at NSA who has been doing some work
> in that area. Daniel are you OK with this change - will it still make
> PV initial domain with with the MiniOS XenBus driver?
>
> Thanks.
That case will work, but what this will break is launching the initial
domain
with a Xenstore stub domain already running (see below).
>>
>> Signed-off-by: Stefano Stabellini
>> ---
>> drivers/xen/xenbus/xenbus_comms.c |2 +-
>> drivers/xen/xenbus/xenbus_probe.c | 62
>> +---
>> drivers/xen/xenbus/xenbus_xs.c|1 +
>> 3 files changed, 45 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_comms.c
>> b/drivers/xen/xenbus/xenbus_comms.c
>> index 52fe7ad..c5aa55c 100644
>> --- a/drivers/xen/xenbus/xenbus_comms.c
>> +++ b/drivers/xen/xenbus/xenbus_comms.c
>> @@ -224,7 +224,7 @@ int xb_init_comms(void)
>> int err;
>> err = bind_evtchn_to_irqhandler(xen_store_evtchn,
>> wake_waiting,
>> 0, "xenbus", &xb_waitq);
>> -if (err <= 0) {
>> +if (err < 0) {
>> printk(KERN_ERR "XENBUS request irq failed
>> %i\n", err);
>> return err;
>> }
>
>> diff --git a/drivers/xen/xenbus/xenbus_probe.c
>> b/drivers/xen/xenbus/xenbus_probe.c
>> index b793723..a67ccc0 100644
>> --- a/drivers/xen/xenbus/xenbus_probe.c
>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>> @@ -719,37 +719,61 @@ static int __init xenstored_local_init(void)
>> return err;
>> }
>>
>> +enum xenstore_init {
>> +UNKNOWN,
>> +PV,
>> +HVM,
>> +LOCAL,
>> +};
>> static int __init xenbus_init(void)
>> {
>> int err = 0;
>> +enum xenstore_init usage = UNKNOWN;
>> +uint64_t v = 0;
>>
>> if (!xen_domain())
>> return -ENODEV;
>>
>> xenbus_ring_ops_init();
>>
>> -if (xen_hvm_domain()) {
>> -uint64_t v = 0;
>> -err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
>> -if (err)
>> -goto out_error;
>> -xen_store_evtchn = (int)v;
>> -err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
>> -if (err)
>> -goto out_error;
>> -xen_store_mfn = (unsigned long)v;
>> -xen_store_interface = ioremap(xen_store_mfn <<
>> PAGE_SHIFT, PAGE_SIZE);
>> -} else {
>> -xen_store_evtchn = xen_start_info->store_evtchn;
>> -xen_store_mfn = xen_start_info->store_mfn;
>> -if (xen_store_evtchn)
>> -xenstored_ready = 1;
>> -else {
>> +if (xen_pv_domain())
>> +usage = PV;
>> +if (xen_hvm_domain())
>> +usage = HVM;
The above is correct for domUs, and is overridden for dom0s:
>> +if (xen_hvm_domain() && xen_initial_domain())
>> +usage = LOCAL;
>> +if (xen_pv_domain() && !xen_start_info->store_evtchn)
>> +usage = LOCAL;
Instead of these checks, I think it should just be:
if (!xen_start_info->store_evtchn)
usage = LOCAL;
Any domain started after xenstore will have store_evtchn set, so if you
don't
have this set, you are either going to be running xenstore locally, or will
use the ioctl to change it later (and so should still set up everything as
if
it will be running locally).
>>>
>>> That would be wrong for an HVM dom0 domain (at least on ARM), because
>>> we don't have a start_info page at all.
>>>
>>>
>> +if (xen_pv_domain() && xen_start_info->store_evtchn)
>> +xenstored_ready = 1;
This part can now just be moved unconditionally into case PV.
>>>
>>> What about:
>>
Re: [PATCH v2 10/23] xen/arm: compile and run xenbus
On 08/08/2012 12:51 PM, Stefano Stabellini wrote:
> On Tue, 7 Aug 2012, Daniel De Graaf wrote:
>> On 08/07/2012 02:21 PM, Konrad Rzeszutek Wilk wrote:
>>> On Mon, Aug 06, 2012 at 03:27:13PM +0100, Stefano Stabellini wrote:
bind_evtchn_to_irqhandler can legitimately return 0 (irq 0): it is not
an error.
If Linux is running as an HVM domain and is running as Dom0, use
xenstored_local_init to initialize the xenstore page and event channel.
Changes in v2:
- refactor xenbus_init.
>>>
>>> Thank you. Lets also CC our friend at NSA who has been doing some work
>>> in that area. Daniel are you OK with this change - will it still make
>>> PV initial domain with with the MiniOS XenBus driver?
>>>
>>> Thanks.
>>
>> That case will work, but what this will break is launching the initial domain
>> with a Xenstore stub domain already running (see below).
>>
Signed-off-by: Stefano Stabellini
---
drivers/xen/xenbus/xenbus_comms.c |2 +-
drivers/xen/xenbus/xenbus_probe.c | 62
+---
drivers/xen/xenbus/xenbus_xs.c|1 +
3 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_comms.c
b/drivers/xen/xenbus/xenbus_comms.c
index 52fe7ad..c5aa55c 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -224,7 +224,7 @@ int xb_init_comms(void)
int err;
err = bind_evtchn_to_irqhandler(xen_store_evtchn, wake_waiting,
0, "xenbus", &xb_waitq);
- if (err <= 0) {
+ if (err < 0) {
printk(KERN_ERR "XENBUS request irq failed %i\n", err);
return err;
}
>>>
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index b793723..a67ccc0 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -719,37 +719,61 @@ static int __init xenstored_local_init(void)
return err;
}
+enum xenstore_init {
+ UNKNOWN,
+ PV,
+ HVM,
+ LOCAL,
+};
static int __init xenbus_init(void)
{
int err = 0;
+ enum xenstore_init usage = UNKNOWN;
+ uint64_t v = 0;
if (!xen_domain())
return -ENODEV;
xenbus_ring_ops_init();
- if (xen_hvm_domain()) {
- uint64_t v = 0;
- err = hvm_get_parameter(HVM_PARAM_STORE_EVTCHN, &v);
- if (err)
- goto out_error;
- xen_store_evtchn = (int)v;
- err = hvm_get_parameter(HVM_PARAM_STORE_PFN, &v);
- if (err)
- goto out_error;
- xen_store_mfn = (unsigned long)v;
- xen_store_interface = ioremap(xen_store_mfn << PAGE_SHIFT,
PAGE_SIZE);
- } else {
- xen_store_evtchn = xen_start_info->store_evtchn;
- xen_store_mfn = xen_start_info->store_mfn;
- if (xen_store_evtchn)
- xenstored_ready = 1;
- else {
+ if (xen_pv_domain())
+ usage = PV;
+ if (xen_hvm_domain())
+ usage = HVM;
>>
>> The above is correct for domUs, and is overridden for dom0s:
>>
+ if (xen_hvm_domain() && xen_initial_domain())
+ usage = LOCAL;
+ if (xen_pv_domain() && !xen_start_info->store_evtchn)
+ usage = LOCAL;
>>
>> Instead of these checks, I think it should just be:
>>
>> if (!xen_start_info->store_evtchn)
>> usage = LOCAL;
>>
>> Any domain started after xenstore will have store_evtchn set, so if you don't
>> have this set, you are either going to be running xenstore locally, or will
>> use the ioctl to change it later (and so should still set up everything as if
>> it will be running locally).
>
> That would be wrong for an HVM dom0 domain (at least on ARM), because
> we don't have a start_info page at all.
>
>
+ if (xen_pv_domain() && xen_start_info->store_evtchn)
+ xenstored_ready = 1;
>>
>> This part can now just be moved unconditionally into case PV.
>
> What about:
>
> if (xen_pv_domain())
> usage = PV;
> if (xen_hvm_domain())
> usage = HVM;
> if (!xen_store_evtchn)
> usage = LOCAL;
>
> and moving xenstored_ready in case PV, like you suggested.
>
That looks correct, but you'd need to split up the switch statement in
order to populate xen_store_evtchn before that last condition, which
ends up pretty much eliminating the usage variable.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
