Re: [Xen-devel] [PATCH] xen/acpi-stub: Disable it b/c the acpi_processor_add is no longer called.

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

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.liu@xxxxxxxxx
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  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.
Reasonable solution is, hooking Px/Cx logic into xen hotplug path (say, ops.add 
--> acpi_processor_add), like what native side does currently.

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;
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.

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)

Thanks,
Jinsong

========================
>From bb49caa523de46551c37de91754c49919bee4687 Mon Sep 17 00:00:00 2001
From: Liu Jinsong <jinsong.liu@xxxxxxxxx>
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.liu@xxxxxxxxx>
---
 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
-
 }
 
 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

Attachment: 0001-Xen-X86-adjust-dom0-cpu-possible-map-for-sake-of-Xen.patch
Description: 0001-Xen-X86-adjust-dom0-cpu-possible-map-for-sake-of-Xen.patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel