| Message ID | 1366831695-13214-2-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2013-04-24 at 20:28 +0100, Stefano Stabellini wrote: > Map vcpu_info using VCPUOP_register_vcpu_info on secondary cpus. > > Call enable_percpu_irq on every cpu. > > Changed in v2: > - move the percpu variable argument fix to a separate patch; > - remove unused variable. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > arch/arm/xen/enlighten.c | 38 +++++++++++++++++++++++++++++++++++++- > 1 files changed, 37 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > index 99ce189..94bbf3b 100644 > --- a/arch/arm/xen/enlighten.c > +++ b/arch/arm/xen/enlighten.c > @@ -2,6 +2,7 @@ > #include <xen/events.h> > #include <xen/grant_table.h> > #include <xen/hvm.h> > +#include <xen/interface/vcpu.h> > #include <xen/interface/xen.h> > #include <xen/interface/memory.h> > #include <xen/interface/hvm/params.h> > @@ -32,6 +33,7 @@ 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); > +DEFINE_PER_CPU(struct vcpu_info, xen_vcpu_info); > /* These are unused until we support booting "pre-ballooned" */ > unsigned long xen_released_pages; > @@ -148,6 +150,32 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma, > } > EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range); > > +static int __init xen_secondary_init(unsigned int cpu) > +{ > + struct vcpu_register_vcpu_info info; > + struct vcpu_info *vcpup; > + int err; > + > + if (cpu == 0) > + return 0; > + > + pr_info("Xen: initializing cpu%d\n", cpu); > + vcpup = &per_cpu(xen_vcpu_info, cpu); > + > + info.mfn = __pa(vcpup) >> PAGE_SHIFT; > + info.offset = offset_in_page(vcpup); Do you need to somehow guarantee it's not going to cross a page? Maybe standard C alignment rules make that the case? > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > * is required to use VCPUOP_register_vcpu_info to place vcpu info > * for secondary CPUs as they are brought up. */ > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > + for_each_online_cpu(i) > + xen_secondary_init(i); > > gnttab_init(); > if (!xen_initial_domain()) [...] > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > return -EINVAL; > } > > - enable_percpu_irq(xen_events_irq, 0); > + on_each_cpu(xen_percpu_enable_events, NULL, 0); It feels like there ought to be some sort of per-cpu bringup callback which takes care of these dynamically. Maybe that doesn't matter until we get vcpu hotplug going? > > return 0; > }
On Thu, 25 Apr 2013, Ian Campbell wrote: > On Wed, 2013-04-24 at 20:28 +0100, Stefano Stabellini wrote: > > Map vcpu_info using VCPUOP_register_vcpu_info on secondary cpus. > > > > Call enable_percpu_irq on every cpu. > > > > Changed in v2: > > - move the percpu variable argument fix to a separate patch; > > - remove unused variable. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > arch/arm/xen/enlighten.c | 38 +++++++++++++++++++++++++++++++++++++- > > 1 files changed, 37 insertions(+), 1 deletions(-) > > > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > > index 99ce189..94bbf3b 100644 > > --- a/arch/arm/xen/enlighten.c > > +++ b/arch/arm/xen/enlighten.c > > @@ -2,6 +2,7 @@ > > #include <xen/events.h> > > #include <xen/grant_table.h> > > #include <xen/hvm.h> > > +#include <xen/interface/vcpu.h> > > #include <xen/interface/xen.h> > > #include <xen/interface/memory.h> > > #include <xen/interface/hvm/params.h> > > @@ -32,6 +33,7 @@ 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); > > +DEFINE_PER_CPU(struct vcpu_info, xen_vcpu_info); > > > /* These are unused until we support booting "pre-ballooned" */ > > unsigned long xen_released_pages; > > @@ -148,6 +150,32 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma, > > } > > EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range); > > > > +static int __init xen_secondary_init(unsigned int cpu) > > +{ > > + struct vcpu_register_vcpu_info info; > > + struct vcpu_info *vcpup; > > + int err; > > + > > + if (cpu == 0) > > + return 0; > > + > > + pr_info("Xen: initializing cpu%d\n", cpu); > > + vcpup = &per_cpu(xen_vcpu_info, cpu); > > + > > + info.mfn = __pa(vcpup) >> PAGE_SHIFT; > > + info.offset = offset_in_page(vcpup); > > Do you need to somehow guarantee it's not going to cross a page? Maybe > standard C alignment rules make that the case? That is a good question, I don't think that DEFINE_PER_CPU makes any alignment guarantees (standard C alignment aside). I'll switch to __alloc_percpu that allows me to specify an alignment and has the advantage of being dynamically allocated. > > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > > * is required to use VCPUOP_register_vcpu_info to place vcpu info > > * for secondary CPUs as they are brought up. */ > > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > > + for_each_online_cpu(i) > > + xen_secondary_init(i); > > > > gnttab_init(); > > if (!xen_initial_domain()) > [...] > > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > > return -EINVAL; > > } > > > > - enable_percpu_irq(xen_events_irq, 0); > > + on_each_cpu(xen_percpu_enable_events, NULL, 0); > > It feels like there ought to be some sort of per-cpu bringup callback > which takes care of these dynamically. Maybe that doesn't matter until > we get vcpu hotplug going? I suspect there isn't one, considering that on_each_cpu is also used by kvm_vgic_hyp_init, kvm_timer_hyp_init and others.
> > > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > > > * is required to use VCPUOP_register_vcpu_info to place vcpu info > > > * for secondary CPUs as they are brought up. */ > > > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > > > + for_each_online_cpu(i) > > > + xen_secondary_init(i); > > > > > > gnttab_init(); > > > if (!xen_initial_domain()) > > [...] > > > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > > > return -EINVAL; > > > } > > > > > > - enable_percpu_irq(xen_events_irq, 0); > > > + on_each_cpu(xen_percpu_enable_events, NULL, 0); > > > > It feels like there ought to be some sort of per-cpu bringup callback > > which takes care of these dynamically. Maybe that doesn't matter until > > we get vcpu hotplug going? > > I suspect there isn't one, considering that on_each_cpu is also used by > kvm_vgic_hyp_init, kvm_timer_hyp_init and others. Could we use cpu_notifiers for this?
On Thu, 25 Apr 2013, Ian Campbell wrote: > > > > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > > > > * is required to use VCPUOP_register_vcpu_info to place vcpu info > > > > * for secondary CPUs as they are brought up. */ > > > > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > > > > + for_each_online_cpu(i) > > > > + xen_secondary_init(i); > > > > > > > > gnttab_init(); > > > > if (!xen_initial_domain()) > > > [...] > > > > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > > > > return -EINVAL; > > > > } > > > > > > > > - enable_percpu_irq(xen_events_irq, 0); > > > > + on_each_cpu(xen_percpu_enable_events, NULL, 0); > > > > > > It feels like there ought to be some sort of per-cpu bringup callback > > > which takes care of these dynamically. Maybe that doesn't matter until > > > we get vcpu hotplug going? > > > > I suspect there isn't one, considering that on_each_cpu is also used by > > kvm_vgic_hyp_init, kvm_timer_hyp_init and others. > > Could we use cpu_notifiers for this? cpu_notifiers are for cpu hotplug, not for secondary cpu bringup
On Thu, 2013-04-25 at 19:45 +0100, Stefano Stabellini wrote: > On Thu, 25 Apr 2013, Ian Campbell wrote: > > > > > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > > > > > * is required to use VCPUOP_register_vcpu_info to place vcpu info > > > > > * for secondary CPUs as they are brought up. */ > > > > > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > > > > > + for_each_online_cpu(i) > > > > > + xen_secondary_init(i); > > > > > > > > > > gnttab_init(); > > > > > if (!xen_initial_domain()) > > > > [...] > > > > > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > > > > > return -EINVAL; > > > > > } > > > > > > > > > > - enable_percpu_irq(xen_events_irq, 0); > > > > > + on_each_cpu(xen_percpu_enable_events, NULL, 0); > > > > > > > > It feels like there ought to be some sort of per-cpu bringup callback > > > > which takes care of these dynamically. Maybe that doesn't matter until > > > > we get vcpu hotplug going? > > > > > > I suspect there isn't one, considering that on_each_cpu is also used by > > > kvm_vgic_hyp_init, kvm_timer_hyp_init and others. > > > > Could we use cpu_notifiers for this? > > cpu_notifiers are for cpu hotplug, not for secondary cpu bringup Are you sure they don't also trigger during bringup, because the distinction is a little bit academic... Ian.
On Fri, 26 Apr 2013, Ian Campbell wrote: > On Thu, 2013-04-25 at 19:45 +0100, Stefano Stabellini wrote: > > On Thu, 25 Apr 2013, Ian Campbell wrote: > > > > > > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > > > > > > * is required to use VCPUOP_register_vcpu_info to place vcpu info > > > > > > * for secondary CPUs as they are brought up. */ > > > > > > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > > > > > > + for_each_online_cpu(i) > > > > > > + xen_secondary_init(i); > > > > > > > > > > > > gnttab_init(); > > > > > > if (!xen_initial_domain()) > > > > > [...] > > > > > > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > > > - enable_percpu_irq(xen_events_irq, 0); > > > > > > + on_each_cpu(xen_percpu_enable_events, NULL, 0); > > > > > > > > > > It feels like there ought to be some sort of per-cpu bringup callback > > > > > which takes care of these dynamically. Maybe that doesn't matter until > > > > > we get vcpu hotplug going? > > > > > > > > I suspect there isn't one, considering that on_each_cpu is also used by > > > > kvm_vgic_hyp_init, kvm_timer_hyp_init and others. > > > > > > Could we use cpu_notifiers for this? > > > > cpu_notifiers are for cpu hotplug, not for secondary cpu bringup > > Are you sure they don't also trigger during bringup, because the > distinction is a little bit academic... My mistake, they do run on secondary cpus, but not in our case because xen_guest_init is called *after* cpu_notifiers are called. So, they are too early for Xen.
On Fri, 2013-04-26 at 11:27 +0100, Stefano Stabellini wrote: > On Fri, 26 Apr 2013, Ian Campbell wrote: > > On Thu, 2013-04-25 at 19:45 +0100, Stefano Stabellini wrote: > > > On Thu, 25 Apr 2013, Ian Campbell wrote: > > > > > > > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > > > > > > > * is required to use VCPUOP_register_vcpu_info to place vcpu info > > > > > > > * for secondary CPUs as they are brought up. */ > > > > > > > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > > > > > > > + for_each_online_cpu(i) > > > > > > > + xen_secondary_init(i); > > > > > > > > > > > > > > gnttab_init(); > > > > > > > if (!xen_initial_domain()) > > > > > > [...] > > > > > > > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > > > > > > > return -EINVAL; > > > > > > > } > > > > > > > > > > > > > > - enable_percpu_irq(xen_events_irq, 0); > > > > > > > + on_each_cpu(xen_percpu_enable_events, NULL, 0); > > > > > > > > > > > > It feels like there ought to be some sort of per-cpu bringup callback > > > > > > which takes care of these dynamically. Maybe that doesn't matter until > > > > > > we get vcpu hotplug going? > > > > > > > > > > I suspect there isn't one, considering that on_each_cpu is also used by > > > > > kvm_vgic_hyp_init, kvm_timer_hyp_init and others. > > > > > > > > Could we use cpu_notifiers for this? > > > > > > cpu_notifiers are for cpu hotplug, not for secondary cpu bringup > > > > Are you sure they don't also trigger during bringup, because the > > distinction is a little bit academic... > > My mistake, they do run on secondary cpus, but not in our case because > xen_guest_init is called *after* cpu_notifiers are called. > So, they are too early for Xen. Another reason to consider calling xen_guest_init much earlier then IMHO. although we can live with the solution you have now I suppose. Ian.
On Fri, 26 Apr 2013, Ian Campbell wrote: > On Fri, 2013-04-26 at 11:27 +0100, Stefano Stabellini wrote: > > On Fri, 26 Apr 2013, Ian Campbell wrote: > > > On Thu, 2013-04-25 at 19:45 +0100, Stefano Stabellini wrote: > > > > On Thu, 25 Apr 2013, Ian Campbell wrote: > > > > > > > > @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) > > > > > > > > * is required to use VCPUOP_register_vcpu_info to place vcpu info > > > > > > > > * for secondary CPUs as they are brought up. */ > > > > > > > > per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; > > > > > > > > + for_each_online_cpu(i) > > > > > > > > + xen_secondary_init(i); > > > > > > > > > > > > > > > > gnttab_init(); > > > > > > > > if (!xen_initial_domain()) > > > > > > > [...] > > > > > > > > @@ -244,7 +280,7 @@ static int __init xen_init_events(void) > > > > > > > > return -EINVAL; > > > > > > > > } > > > > > > > > > > > > > > > > - enable_percpu_irq(xen_events_irq, 0); > > > > > > > > + on_each_cpu(xen_percpu_enable_events, NULL, 0); > > > > > > > > > > > > > > It feels like there ought to be some sort of per-cpu bringup callback > > > > > > > which takes care of these dynamically. Maybe that doesn't matter until > > > > > > > we get vcpu hotplug going? > > > > > > > > > > > > I suspect there isn't one, considering that on_each_cpu is also used by > > > > > > kvm_vgic_hyp_init, kvm_timer_hyp_init and others. > > > > > > > > > > Could we use cpu_notifiers for this? > > > > > > > > cpu_notifiers are for cpu hotplug, not for secondary cpu bringup > > > > > > Are you sure they don't also trigger during bringup, because the > > > distinction is a little bit academic... > > > > My mistake, they do run on secondary cpus, but not in our case because > > xen_guest_init is called *after* cpu_notifiers are called. > > So, they are too early for Xen. > > Another reason to consider calling xen_guest_init much earlier then > IMHO. although we can live with the solution you have now I suppose. Yeah, moving the call to xen_guest_init earlier might be a good idea, but I wouldn't want to do it in this patch series, at this point of the Linux release cycle.
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 99ce189..94bbf3b 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -2,6 +2,7 @@ #include <xen/events.h> #include <xen/grant_table.h> #include <xen/hvm.h> +#include <xen/interface/vcpu.h> #include <xen/interface/xen.h> #include <xen/interface/memory.h> #include <xen/interface/hvm/params.h> @@ -32,6 +33,7 @@ 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); +DEFINE_PER_CPU(struct vcpu_info, xen_vcpu_info); /* These are unused until we support booting "pre-ballooned" */ unsigned long xen_released_pages; @@ -148,6 +150,32 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma, } EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range); +static int __init xen_secondary_init(unsigned int cpu) +{ + struct vcpu_register_vcpu_info info; + struct vcpu_info *vcpup; + int err; + + if (cpu == 0) + return 0; + + pr_info("Xen: initializing cpu%d\n", cpu); + vcpup = &per_cpu(xen_vcpu_info, cpu); + + info.mfn = __pa(vcpup) >> PAGE_SHIFT; + info.offset = offset_in_page(vcpup); + + err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, cpu, &info); + if (err) { + pr_debug("register_vcpu_info failed: err=%d\n", err); + } else { + /* This cpu is using the registered vcpu info, even if + later ones fail to. */ + per_cpu(xen_vcpu, cpu) = vcpup; + } + return 0; +} + /* * see Documentation/devicetree/bindings/arm/xen.txt for the * documentation of the Xen Device Tree format. @@ -163,6 +191,7 @@ static int __init xen_guest_init(void) const char *version = NULL; const char *xen_prefix = "xen,xen-"; struct resource res; + int i; node = of_find_compatible_node(NULL, NULL, "xen,xen"); if (!node) { @@ -216,6 +245,8 @@ static int __init xen_guest_init(void) * is required to use VCPUOP_register_vcpu_info to place vcpu info * for secondary CPUs as they are brought up. */ per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0]; + for_each_online_cpu(i) + xen_secondary_init(i); gnttab_init(); if (!xen_initial_domain()) @@ -231,6 +262,11 @@ static irqreturn_t xen_arm_callback(int irq, void *arg) return IRQ_HANDLED; } +static __init void xen_percpu_enable_events(void *unused) +{ + enable_percpu_irq(xen_events_irq, 0); +} + static int __init xen_init_events(void) { if (!xen_domain() || xen_events_irq < 0) @@ -244,7 +280,7 @@ static int __init xen_init_events(void) return -EINVAL; } - enable_percpu_irq(xen_events_irq, 0); + on_each_cpu(xen_percpu_enable_events, NULL, 0); return 0; }
Map vcpu_info using VCPUOP_register_vcpu_info on secondary cpus. Call enable_percpu_irq on every cpu. Changed in v2: - move the percpu variable argument fix to a separate patch; - remove unused variable. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- arch/arm/xen/enlighten.c | 38 +++++++++++++++++++++++++++++++++++++- 1 files changed, 37 insertions(+), 1 deletions(-)