On Mon, Mar 25, 2013 at 08:49:47AM +0000, Liu, Jinsong wrote:
> Konrad Rzeszutek Wilk wrote:
> > With the Xen ACPI stub code (CONFIG_XEN_STUB=y) enabled, the power
> > C and P states are no longer uploaded to the hypervisor.
> >
> > The reason is that the Xen CPU hotplug code: xen-acpi-cpuhotplug.c
> > and the xen-acpi-stub.c register themselves as the "processor" type
> > object.
> >
> > That means the generic processor (processor_driver.c) stops
> > working and it does not call (acpi_processor_add) which populates the
> >
> > per_cpu(processors, pr->id) = pr;
> >
> > structure. The 'pr' is gathered from the acpi_processor_get_info
> > function which does the job of finding the C-states and figuring out
> > PBLK address.
> >
> > The 'processors->pr' is then later used by xen-acpi-processor.c (the
> > one that uploads C and P states to the hypervisor). Since it is NULL,
> > we end
> > skip the gathering of _PSD, _PSS, _PCT, etc and never upload the power
> > management data.
> >
> > The end result is that enabling the CONFIG_XEN_STUB in the build
> > means that xen-acpi-processor is not working anymore.
> >
> > This temporary patch fixes it by marking the XEN_STUB driver as
> > BROKEN until this can be properly fixed.
> >
> > CC: jinsong....@intel.com
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> > ---
> > drivers/xen/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > index 5a32232..67af155 100644
> > --- a/drivers/xen/Kconfig
> > +++ b/drivers/xen/Kconfig
> > @@ -182,7 +182,7 @@ config XEN_PRIVCMD
> >
> > config XEN_STUB
> > bool "Xen stub drivers"
> > - depends on XEN && X86_64
> > + depends on XEN && X86_64 && BROKEN
> > default n
> > help
> > Allow kernel to install stub drivers, to reserve space for Xen
> > drivers,
>
>
> Yes, exactly.
>
> With xen-cpu-hotplug logic added, it's not proper to make xen Px/Cx rely on
> native processor_driver anymore.
Ahhh.
> Reasonable solution is, hooking Px/Cx logic into xen hotplug path (say,
> ops.add --> acpi_processor_add), like what native side does currently.
Right. That would require making acpi_processor_add an EXPORT symbol.
>
> So at Xen dom0 side, there are 2 issues need to be solved:
> 1. currently xen dom0 defaultly set cpu_possible_map equal to
> cpu_present_map, we need adjust it so that when cpu hotplug it can handle
> 'pr' properly;
Kind of. The xen-acpi-processor dealt with that by copying the 'pr' for the
present, but not-offline vCPUS
and figuring out which of them dom0 did not know about. Look in the
'get_max_acpi_id' and 'pr_backup'
in the code.
So it has the logic to handle dom0_max_vcpus=X, where X is less than physically
present CPUs.
> 2. hook Px/Cx parsing logic into cpu hotadd or cpu online logic, like what
> native side did;
>
> Attached is a patch to solve issue 1, basically it generates possible map
> according to MADT entries, not trims according to hypervisor online cpus.
> With it, dom0 cpu possible map reserves space for Xen cpu hotplug and
> corresponding Px/Cx, and dom0 also get a unified possible map view just like
> it runs under native platform.
Ah, didn't think of doing it that way! Thought look at
cf405ae612b0f7e2358db7ff594c0e94846137aa
as the scheduler will now try to use those.
>
> As for issue 2, it may be fixed later :-)
> (BTW, your patch to temporarily disable xen-stub is necessary even with my
> patch, only would be reverted after issue 2 got solved)
Right.
>
> Thanks,
> Jinsong
>
> ========================
> >From bb49caa523de46551c37de91754c49919bee4687 Mon Sep 17 00:00:00 2001
> From: Liu Jinsong <jinsong....@intel.com>
> Date: Mon, 25 Mar 2013 21:31:36 +0800
> Subject: [PATCH] Xen/X86: adjust dom0 cpu possible map for sake of Xen Px/Cx
>
> Currently dom0 cpu possible map defaultly equal to cpu
> present map. It works fine if not consider Xen cpu hotplug
> and corresponding Px/Cx. However, when cpu hotplug the
> possible map should reserve some space, considering it's
> static and per-cpu data-structures are allocated at init
> time, and do not expect to do this dynamically.
>
> This patch adjusts Xen dom0 cpu possible map.
> Basically it generates possible map according to MADT entries,
> not trims according to hypervisor online cpus. With it, dom0
> cpu possible map reserves space for Xen cpu hotplug and
> corresponding Px/Cx, and dom0 also get a unified possible map
> view just like it runs under native platform.
>
> Signed-off-by: Liu Jinsong <jinsong....@intel.com>
> ---
> arch/x86/xen/enlighten.c | 13 +++++++++++--
> arch/x86/xen/smp.c | 30 +++++++-----------------------
> 2 files changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index c8e1c7b..f630956 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1089,8 +1089,17 @@ void xen_setup_vcpu_info_placement(void)
> {
> int cpu;
>
> - for_each_possible_cpu(cpu)
> - xen_vcpu_setup(cpu);
> + /*
> + * Dom0 reserved more possible bits than present ones for the sake of
> + * Xen processor driver and hotplug logic. To avoid clamp_max_cpus,
> + * setup dom0 vcpus for present ones.
> + */
> + if (xen_initial_domain())
> + for_each_present_cpu(cpu)
> + xen_vcpu_setup(cpu);
> + else
> + for_each_possible_cpu(cpu)
> + xen_vcpu_setup(cpu);
>
> /* xen_vcpu_setup managed to place the vcpu_info within the
> percpu area for all cpus, so make use of it */
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index 09ea61d..9b334b9 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -192,37 +192,23 @@ static void __init xen_fill_possible_map(void)
> static void __init xen_filter_cpu_maps(void)
> {
> int i, rc;
> - unsigned int subtract = 0;
>
> if (!xen_initial_domain())
> return;
>
> num_processors = 0;
> disabled_cpus = 0;
> +
> + /* all bits have been set possible at prefill_possible_map */
> for (i = 0; i < nr_cpu_ids; i++) {
> rc = HYPERVISOR_vcpu_op(VCPUOP_is_up, i, NULL);
> - if (rc >= 0) {
> + if (rc >= 0)
> num_processors++;
> - set_cpu_possible(i, true);
> - } else {
> - set_cpu_possible(i, false);
> + else {
> + disabled_cpus++;
> set_cpu_present(i, false);
> - subtract++;
> }
> }
> -#ifdef CONFIG_HOTPLUG_CPU
> - /* This is akin to using 'nr_cpus' on the Linux command line.
> - * Which is OK as when we use 'dom0_max_vcpus=X' we can only
> - * have up to X, while nr_cpu_ids is greater than X. This
> - * normally is not a problem, except when CPU hotplugging
> - * is involved and then there might be more than X CPUs
> - * in the guest - which will not work as there is no
> - * hypercall to expand the max number of VCPUs an already
> - * running guest has. So cap it up to X. */
> - if (subtract)
> - nr_cpu_ids = nr_cpu_ids - subtract;
> -#endif
> -
This was a fix for
commit cf405ae612b0f7e2358db7ff594c0e94846137aa
Author: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
Date: Thu Apr 26 13:50:03 2012 -0400
xen/smp: Fix crash when booting with ACPI hotplug CPUs.
which I think your patch would expose again?
> }
>
> static void __init xen_smp_prepare_boot_cpu(void)
> @@ -272,15 +258,13 @@ static void __init xen_smp_prepare_cpus(unsigned int
> max_cpus)
>
> cpumask_copy(xen_cpu_initialized_map, cpumask_of(0));
>
> - /* Restrict the possible_map according to max_cpus. */
> + /* Restrict the possible/present_map according to max_cpus. */
> while ((num_possible_cpus() > 1) && (num_possible_cpus() > max_cpus)) {
> for (cpu = nr_cpu_ids - 1; !cpu_possible(cpu); cpu--)
> continue;
> set_cpu_possible(cpu, false);
> + set_cpu_present(cpu, false);
> }
> -
> - for_each_possible_cpu(cpu)
> - set_cpu_present(cpu, true);
> }
>
> static int __cpuinit
> --
> 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
