Re: [Xen-devel] [PATCH] XENPF_set_processor_pminfo XEN_PM_CX overflows states array

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.12 at 15:07, eric <eric.chanudet@xxxxxxxxxx> wrote:
> Calling XENPF_set_processor_pminfo with XEN_PM_CX could cause states 
> array in "struct acpi_processor_power" to exceed its limit.
> 
> The array used to be reset (by function cpuidle_init_cpu()) for each 
> hypercall. The patch puts it back that way and adds an assertion to make 
> it clear in case that happens again.
> 
> Signed-off-by: Eric Chanudet <eric.chanudet@xxxxxxxxxxxxx>
> 
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -622,19 +622,19 @@ static int cpuidle_init_cpu(int cpu)
> 
>          for ( i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++ )
>              acpi_power->states[i].idx = i;
> -
> -        acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1;
> -        acpi_power->states[ACPI_STATE_C1].entry_method = ACPI_CSTATE_EM_HALT;
> -
> -        acpi_power->states[ACPI_STATE_C0].valid = 1;
> -        acpi_power->states[ACPI_STATE_C1].valid = 1;
> -
> -        acpi_power->count = 2;
> -        acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1];
> -        acpi_power->cpu = cpu;
>          processor_powers[cpu] = acpi_power;
>      }
> 
> +    acpi_power->states[ACPI_STATE_C1].type = ACPI_STATE_C1;
> +    acpi_power->states[ACPI_STATE_C1].entry_method = ACPI_CSTATE_EM_HALT;
> +
> +    acpi_power->states[ACPI_STATE_C0].valid = 1;
> +    acpi_power->states[ACPI_STATE_C1].valid = 1;
> +
> +    acpi_power->count = 2;
> +    acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1];
> +    acpi_power->cpu = cpu;

Is there any reason to move all of these assignments, rather than
just the assignment of ->count and maybe ->states[ACPI_STATE_C1]?
Everything else should not get modified anywhere else.

> +
>      if ( cpu == 0 )
>      {
>          if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
> @@ -870,11 +870,10 @@ static void set_cx(
> 
>      if ( xen_cx->type == ACPI_STATE_C1 )
>          cx = &acpi_power->states[1];
> -    else
> -        cx = &acpi_power->states[acpi_power->count];
> -
> -    if ( !cx->valid )
> -        acpi_power->count++;
> +    else {

Please put the brace on a new line.

> +        ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER);
> +        cx = &acpi_power->states[acpi_power->count++];
> +    }

To be on the safe side, you should probably also check for
ACPI_STATE_C0 (and ignore its data), otherwise (other than before
your patch) ->count would get incremented for this too. So I'd
suggest converting the if() here to a switch().

Jan

> 
>      cx->valid    = 1;
>      cx->type     = xen_cx->type;
> 
> -- 
> Eric Chanudet
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 




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