| Message ID | 1390920842-21886-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 28 Jan 2014, Julien Grall wrote: > Event channels driver needs to be initialized very early. Until now, Xen > initialization was done after all CPUs was bring up. > > We can safely move the initialization to an early initcall. > > Also use a cpu notifier to: > - Register the VCPU when the CPU is prepared > - Enable event channel IRQ when the CPU is running > > Signed-off-by: Julien Grall <julien.grall@linaro.org> Did you test this patch in Dom0 as well as in DomUs? > arch/arm/xen/enlighten.c | 84 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 55 insertions(+), 29 deletions(-) > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > index 293eeea..39b668e 100644 > --- a/arch/arm/xen/enlighten.c > +++ b/arch/arm/xen/enlighten.c > @@ -23,6 +23,7 @@ > #include <linux/of_address.h> > #include <linux/cpuidle.h> > #include <linux/cpufreq.h> > +#include <linux/cpu.h> > > #include <linux/mm.h> > > @@ -154,12 +155,11 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma, > } > EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range); > > -static void __init xen_percpu_init(void *unused) > +static void xen_percpu_init(int cpu) > { > struct vcpu_register_vcpu_info info; > struct vcpu_info *vcpup; > int err; > - int cpu = get_cpu(); > > pr_info("Xen: initializing cpu%d\n", cpu); > vcpup = per_cpu_ptr(xen_vcpu_info, cpu); > @@ -170,9 +170,11 @@ static void __init xen_percpu_init(void *unused) > err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, cpu, &info); > BUG_ON(err); > per_cpu(xen_vcpu, cpu) = vcpup; > +} > > +static void xen_interrupt_init(void) > +{ > enable_percpu_irq(xen_events_irq, 0); > - put_cpu(); > } > > static void xen_restart(enum reboot_mode reboot_mode, const char *cmd) > @@ -193,6 +195,36 @@ static void xen_power_off(void) > BUG(); > } > > +static irqreturn_t xen_arm_callback(int irq, void *arg) > +{ > + xen_hvm_evtchn_do_upcall(); > + return IRQ_HANDLED; > +} > + > +static int xen_cpu_notification(struct notifier_block *self, > + unsigned long action, > + void *hcpu) > +{ > + int cpu = (long)hcpu; > + > + switch (action) { > + case CPU_UP_PREPARE: > + xen_percpu_init(cpu); > + break; > + case CPU_STARTING: > + xen_interrupt_init(); > + break; Is CPU_STARTING guaranteed to be called on the new cpu only? If so, why not call both xen_percpu_init and xen_interrupt_init on CPU_STARTING? As it stands I think you introduced a subtle change (that might be OK but I think is unintentional): xen_percpu_init might not be called from the same cpu as its target anymore. > + default: > + break; > + } > + > + return NOTIFY_OK; > +} > + > +static struct notifier_block xen_cpu_notifier = { > + .notifier_call = xen_cpu_notification, > +}; > + > /* > * see Documentation/devicetree/bindings/arm/xen.txt for the > * documentation of the Xen Device Tree format. > @@ -209,6 +241,7 @@ static int __init xen_guest_init(void) > const char *xen_prefix = "xen,xen-"; > struct resource res; > phys_addr_t grant_frames; > + int cpu; > > node = of_find_compatible_node(NULL, NULL, "xen,xen"); > if (!node) { > @@ -281,9 +314,27 @@ static int __init xen_guest_init(void) > disable_cpuidle(); > disable_cpufreq(); > > + xen_init_IRQ(); > + > + if (xen_events_irq < 0) > + return -ENODEV; Since you are moving this code to xen_guest_init, you can check for xen_events_irq earlier on, where we parse the irq from device tree. > + if (request_percpu_irq(xen_events_irq, xen_arm_callback, > + "events", &xen_vcpu)) { > + pr_err("Error request IRQ %d\n", xen_events_irq); > + return -EINVAL; > + } > + > + cpu = get_cpu(); > + xen_percpu_init(cpu); > + xen_interrupt_init(); > + put_cpu(); > + > + register_cpu_notifier(&xen_cpu_notifier); > + > return 0; > } > -core_initcall(xen_guest_init); > +early_initcall(xen_guest_init); > > static int __init xen_pm_init(void) > { > @@ -297,31 +348,6 @@ static int __init xen_pm_init(void) > } > late_initcall(xen_pm_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)) { > - pr_err("Error requesting IRQ %d\n", xen_events_irq); > - return -EINVAL; > - } > - > - on_each_cpu(xen_percpu_init, NULL, 0); > - > - return 0; > -} > -postcore_initcall(xen_init_events); > - > /* In the hypervisor.S file. */ > EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op); > EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op); > -- > 1.7.10.4 >
On 01/28/2014 05:13 PM, Stefano Stabellini wrote: > On Tue, 28 Jan 2014, Julien Grall wrote: >> Event channels driver needs to be initialized very early. Until now, Xen >> initialization was done after all CPUs was bring up. >> >> We can safely move the initialization to an early initcall. >> >> Also use a cpu notifier to: >> - Register the VCPU when the CPU is prepared >> - Enable event channel IRQ when the CPU is running >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Did you test this patch in Dom0 as well as in DomUs? > Only try dom0. I will try domU. > >> arch/arm/xen/enlighten.c | 84 ++++++++++++++++++++++++++++++---------------- >> 1 file changed, 55 insertions(+), 29 deletions(-) >> >> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c >> index 293eeea..39b668e 100644 >> --- a/arch/arm/xen/enlighten.c >> +++ b/arch/arm/xen/enlighten.c >> @@ -23,6 +23,7 @@ >> #include <linux/of_address.h> >> #include <linux/cpuidle.h> >> #include <linux/cpufreq.h> >> +#include <linux/cpu.h> >> >> #include <linux/mm.h> >> >> @@ -154,12 +155,11 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma, >> } >> EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range); >> >> -static void __init xen_percpu_init(void *unused) >> +static void xen_percpu_init(int cpu) >> { >> struct vcpu_register_vcpu_info info; >> struct vcpu_info *vcpup; >> int err; >> - int cpu = get_cpu(); >> >> pr_info("Xen: initializing cpu%d\n", cpu); >> vcpup = per_cpu_ptr(xen_vcpu_info, cpu); >> @@ -170,9 +170,11 @@ static void __init xen_percpu_init(void *unused) >> err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, cpu, &info); >> BUG_ON(err); >> per_cpu(xen_vcpu, cpu) = vcpup; >> +} >> >> +static void xen_interrupt_init(void) >> +{ >> enable_percpu_irq(xen_events_irq, 0); >> - put_cpu(); >> } >> >> static void xen_restart(enum reboot_mode reboot_mode, const char *cmd) >> @@ -193,6 +195,36 @@ static void xen_power_off(void) >> BUG(); >> } >> >> +static irqreturn_t xen_arm_callback(int irq, void *arg) >> +{ >> + xen_hvm_evtchn_do_upcall(); >> + return IRQ_HANDLED; >> +} >> + >> +static int xen_cpu_notification(struct notifier_block *self, >> + unsigned long action, >> + void *hcpu) >> +{ >> + int cpu = (long)hcpu; >> + >> + switch (action) { >> + case CPU_UP_PREPARE: >> + xen_percpu_init(cpu); >> + break; >> + case CPU_STARTING: >> + xen_interrupt_init(); >> + break; > > Is CPU_STARTING guaranteed to be called on the new cpu only? Yes. > If so, why not call both xen_percpu_init and xen_interrupt_init on > CPU_STARTING? Just in case that xen_vcpu is used somewhere else by a cpu notifier callback CPU_STARTING. We don't know which callback is called first. > As it stands I think you introduced a subtle change (that might be OK > but I think is unintentional): xen_percpu_init might not be called from > the same cpu as its target anymore. No, xen_percpu_init and xen_interrupt_init are called on the boot cpu at the end of xen_guest_init. > > >> + default: >> + break; >> + } >> + >> + return NOTIFY_OK; >> +} >> + >> +static struct notifier_block xen_cpu_notifier = { >> + .notifier_call = xen_cpu_notification, >> +}; >> + >> /* >> * see Documentation/devicetree/bindings/arm/xen.txt for the >> * documentation of the Xen Device Tree format. >> @@ -209,6 +241,7 @@ static int __init xen_guest_init(void) >> const char *xen_prefix = "xen,xen-"; >> struct resource res; >> phys_addr_t grant_frames; >> + int cpu; >> >> node = of_find_compatible_node(NULL, NULL, "xen,xen"); >> if (!node) { >> @@ -281,9 +314,27 @@ static int __init xen_guest_init(void) >> disable_cpuidle(); >> disable_cpufreq(); >> >> + xen_init_IRQ(); >> + >> + if (xen_events_irq < 0) >> + return -ENODEV; > > Since you are moving this code to xen_guest_init, you can check for > xen_events_irq earlier on, where we parse the irq from device tree. Will do.
On Tue, 28 Jan 2014, Julien Grall wrote: > >> +static int xen_cpu_notification(struct notifier_block *self, > >> + unsigned long action, > >> + void *hcpu) > >> +{ > >> + int cpu = (long)hcpu; > >> + > >> + switch (action) { > >> + case CPU_UP_PREPARE: > >> + xen_percpu_init(cpu); > >> + break; > >> + case CPU_STARTING: > >> + xen_interrupt_init(); > >> + break; > > > > Is CPU_STARTING guaranteed to be called on the new cpu only? > > Yes. > > > If so, why not call both xen_percpu_init and xen_interrupt_init on > > CPU_STARTING? > > Just in case that xen_vcpu is used somewhere else by a cpu notifier > callback CPU_STARTING. We don't know which callback is called first. Could you please elaborate a bit more on the problem you are trying to describe? > > As it stands I think you introduced a subtle change (that might be OK > > but I think is unintentional): xen_percpu_init might not be called from > > the same cpu as its target anymore. > > No, xen_percpu_init and xen_interrupt_init are called on the boot cpu at > the end of xen_guest_init. Is CPU_UP_PREPARE guaranteed to be called on the target cpu? I think not, therefore you would be executing xen_percpu_init for cpu1 on cpu0.
On 01/28/2014 05:46 PM, Stefano Stabellini wrote: > On Tue, 28 Jan 2014, Julien Grall wrote: >>>> +static int xen_cpu_notification(struct notifier_block *self, >>>> + unsigned long action, >>>> + void *hcpu) >>>> +{ >>>> + int cpu = (long)hcpu; >>>> + >>>> + switch (action) { >>>> + case CPU_UP_PREPARE: >>>> + xen_percpu_init(cpu); >>>> + break; >>>> + case CPU_STARTING: >>>> + xen_interrupt_init(); >>>> + break; >>> >>> Is CPU_STARTING guaranteed to be called on the new cpu only? >> >> Yes. >> >>> If so, why not call both xen_percpu_init and xen_interrupt_init on >>> CPU_STARTING? >> >> Just in case that xen_vcpu is used somewhere else by a cpu notifier >> callback CPU_STARTING. We don't know which callback is called first. > > Could you please elaborate a bit more on the problem you are trying to > describe? We want to make sure that the vcpu is registered correctly. If not, we can't skip it and avoid xen to have a "dead" VCPU to schedule due to BUG_ON. I agree that now we have a BUG_ON in the middle of xen_percpu_init, but it's possible to return an error. In this case Linux will skip this cpu and continue to boot. >>> As it stands I think you introduced a subtle change (that might be OK >>> but I think is unintentional): xen_percpu_init might not be called from >>> the same cpu as its target anymore. >> >> No, xen_percpu_init and xen_interrupt_init are called on the boot cpu at >> the end of xen_guest_init. > > Is CPU_UP_PREPARE guaranteed to be called on the target cpu? I think > not, therefore you would be executing xen_percpu_init for cpu1 on cpu0. > I don't see any issue to execute xen_percpu_init for cpu1 on cpu0, all the code is taking directly the vcpu ID to initialize.
On Tue, 28 Jan 2014, Julien Grall wrote: > On 01/28/2014 05:46 PM, Stefano Stabellini wrote: > > On Tue, 28 Jan 2014, Julien Grall wrote: > >>>> +static int xen_cpu_notification(struct notifier_block *self, > >>>> + unsigned long action, > >>>> + void *hcpu) > >>>> +{ > >>>> + int cpu = (long)hcpu; > >>>> + > >>>> + switch (action) { > >>>> + case CPU_UP_PREPARE: > >>>> + xen_percpu_init(cpu); > >>>> + break; > >>>> + case CPU_STARTING: > >>>> + xen_interrupt_init(); > >>>> + break; > >>> > >>> Is CPU_STARTING guaranteed to be called on the new cpu only? > >> > >> Yes. > >> > >>> If so, why not call both xen_percpu_init and xen_interrupt_init on > >>> CPU_STARTING? > >> > >> Just in case that xen_vcpu is used somewhere else by a cpu notifier > >> callback CPU_STARTING. We don't know which callback is called first. > > > > Could you please elaborate a bit more on the problem you are trying to > > describe? > > We want to make sure that the vcpu is registered correctly. If not, we > can't skip it and avoid xen to have a "dead" VCPU to schedule due to BUG_ON. > > I agree that now we have a BUG_ON in the middle of xen_percpu_init, but > it's possible to return an error. In this case Linux will skip this cpu > and continue to boot. I think there are no benefits in having two separate functions (xen_percpu_init and xen_interrupt_init) called at two different points (CPU_UP_PREPARE and CPU_STARTING). I would simply have one. Simpler is better. > >>> As it stands I think you introduced a subtle change (that might be OK > >>> but I think is unintentional): xen_percpu_init might not be called from > >>> the same cpu as its target anymore. > >> > >> No, xen_percpu_init and xen_interrupt_init are called on the boot cpu at > >> the end of xen_guest_init. > > > > Is CPU_UP_PREPARE guaranteed to be called on the target cpu? I think > > not, therefore you would be executing xen_percpu_init for cpu1 on cpu0. > > > > I don't see any issue to execute xen_percpu_init for cpu1 on cpu0, all > the code is taking directly the vcpu ID to initialize. Me neither, I was simply pointing out that you made this change without writing it in the commit message (therefore I assume it might be unintended).
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index 293eeea..39b668e 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -23,6 +23,7 @@ #include <linux/of_address.h> #include <linux/cpuidle.h> #include <linux/cpufreq.h> +#include <linux/cpu.h> #include <linux/mm.h> @@ -154,12 +155,11 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma, } EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range); -static void __init xen_percpu_init(void *unused) +static void xen_percpu_init(int cpu) { struct vcpu_register_vcpu_info info; struct vcpu_info *vcpup; int err; - int cpu = get_cpu(); pr_info("Xen: initializing cpu%d\n", cpu); vcpup = per_cpu_ptr(xen_vcpu_info, cpu); @@ -170,9 +170,11 @@ static void __init xen_percpu_init(void *unused) err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, cpu, &info); BUG_ON(err); per_cpu(xen_vcpu, cpu) = vcpup; +} +static void xen_interrupt_init(void) +{ enable_percpu_irq(xen_events_irq, 0); - put_cpu(); } static void xen_restart(enum reboot_mode reboot_mode, const char *cmd) @@ -193,6 +195,36 @@ static void xen_power_off(void) BUG(); } +static irqreturn_t xen_arm_callback(int irq, void *arg) +{ + xen_hvm_evtchn_do_upcall(); + return IRQ_HANDLED; +} + +static int xen_cpu_notification(struct notifier_block *self, + unsigned long action, + void *hcpu) +{ + int cpu = (long)hcpu; + + switch (action) { + case CPU_UP_PREPARE: + xen_percpu_init(cpu); + break; + case CPU_STARTING: + xen_interrupt_init(); + break; + default: + break; + } + + return NOTIFY_OK; +} + +static struct notifier_block xen_cpu_notifier = { + .notifier_call = xen_cpu_notification, +}; + /* * see Documentation/devicetree/bindings/arm/xen.txt for the * documentation of the Xen Device Tree format. @@ -209,6 +241,7 @@ static int __init xen_guest_init(void) const char *xen_prefix = "xen,xen-"; struct resource res; phys_addr_t grant_frames; + int cpu; node = of_find_compatible_node(NULL, NULL, "xen,xen"); if (!node) { @@ -281,9 +314,27 @@ static int __init xen_guest_init(void) disable_cpuidle(); disable_cpufreq(); + xen_init_IRQ(); + + if (xen_events_irq < 0) + return -ENODEV; + + if (request_percpu_irq(xen_events_irq, xen_arm_callback, + "events", &xen_vcpu)) { + pr_err("Error request IRQ %d\n", xen_events_irq); + return -EINVAL; + } + + cpu = get_cpu(); + xen_percpu_init(cpu); + xen_interrupt_init(); + put_cpu(); + + register_cpu_notifier(&xen_cpu_notifier); + return 0; } -core_initcall(xen_guest_init); +early_initcall(xen_guest_init); static int __init xen_pm_init(void) { @@ -297,31 +348,6 @@ static int __init xen_pm_init(void) } late_initcall(xen_pm_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)) { - pr_err("Error requesting IRQ %d\n", xen_events_irq); - return -EINVAL; - } - - on_each_cpu(xen_percpu_init, NULL, 0); - - return 0; -} -postcore_initcall(xen_init_events); - /* In the hypervisor.S file. */ EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op); EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
Event channels driver needs to be initialized very early. Until now, Xen initialization was done after all CPUs was bring up. We can safely move the initialization to an early initcall. Also use a cpu notifier to: - Register the VCPU when the CPU is prepared - Enable event channel IRQ when the CPU is running Signed-off-by: Julien Grall <julien.grall@linaro.org> --- arch/arm/xen/enlighten.c | 84 ++++++++++++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 29 deletions(-)