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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.12 at 18:46, Eric Chanudet <eric.chanudet@xxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/acpi/cpu_idle.c    Tue Mar 06 15:51:33 2012 +0100
> +++ b/xen/arch/x86/acpi/cpu_idle.c    Wed Mar 07 17:39:34 2012 +0000
> @@ -645,12 +645,12 @@
>       
>          acpi_power->states[ACPI_STATE_C0].valid = 1;
>          acpi_power->states[ACPI_STATE_C1].valid = 1;
> +        acpi_power->cpu = cpu;
> +        processor_powers[cpu] = acpi_power;
> +    }
>       
>          acpi_power->count = 2;
>          acpi_power->safe_state = &acpi_power->states[ACPI_STATE_C1];

Please fix the indentation here. Further, I'd really like to see the two
C1-related lines (setting .type and .entry_method) moved out of the
if() as well - if we're getting fresh data from Dom0, we shouldn't make
assumptions on the validity of the old data.

> -        acpi_power->cpu = cpu;
> -        processor_powers[cpu] = acpi_power;
> -    }
>  
>      if ( cpu == 0 )
>      {

Further, as you're adjusting this anyway, I think this whole if() should
move into the earlier if()'s body.

> @@ -885,16 +885,20 @@
>      if ( check_cx(acpi_power, xen_cx) != 0 )
>          return;
>  
> -    if ( xen_cx->type == ACPI_STATE_C1 )
> +    switch (xen_cx->type)
> +    {
> +    case ACPI_STATE_C0:
> +        return;
> +    case ACPI_STATE_C1:
>          cx = &acpi_power->states[1];
> -    else
> -        cx = &acpi_power->states[acpi_power->count];
> +        break;
> +    default:
> +        ASSERT(acpi_power->count < ACPI_PROCESSOR_MAX_POWER);

After some more thinking about this, I don't see ASSERT() as a
proper action here. Rather than crashing the debug hypervisor
and silently corrupting data on a non-debug one, print a warning
here and bail out.

> +        cx = &acpi_power->states[acpi_power->count++];
> +        cx->valid = 1;
> +        cx->type = xen_cx->type;
> +    }
>  
> -    if ( !cx->valid )
> -        acpi_power->count++;
> -
> -    cx->valid    = 1;
> -    cx->type     = xen_cx->type;
>      cx->address  = xen_cx->reg.address;
>  
>      switch ( xen_cx->reg.space_id )

As the patch is sufficiently different from the previous version, I think
you ought to re-submit with a full patch description and s-o-b line. Or
maybe I should just take your patch as a basis and do the adjustments
myself...

Jan


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