Re: [Xen-devel] [PATCH v2 3/5] x86/AMD: make C-state handling independent of Dom0

[Jan Beulich <JBeulich@xxxxxxxx>]

On 16.07.2019 16:26, Roger Pau Monné  wrote:
> On Wed, Jul 03, 2019 at 01:01:48PM +0000, Jan Beulich wrote:
>> --- a/xen/arch/x86/acpi/cpu_idle.c
>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>> @@ -110,6 +110,8 @@ boolean_param("lapic_timer_c2_ok", local
>>    
>>    struct acpi_processor_power *__read_mostly processor_powers[NR_CPUS];
>>    
>> +static int8_t __read_mostly vendor_override;
> 
> AFAICT from the code below this is a tri-state (-1, 0, 1). Could it
> maybe be turned into an enum with definitions of the different
> states?
> 
> I think it would make the usage clearer.

Well, personally I prefer doing it this way for tristates. I'll
wait to see what others think.

>> @@ -1286,6 +1291,103 @@ long set_cx_pminfo(uint32_t acpi_id, str
>>        return 0;
>>    }
>>    
>> +static void amd_cpuidle_init(struct acpi_processor_power *power)
>> +{
>> +    unsigned int i, nr = 0;
>> +    const struct cpuinfo_x86 *c = &current_cpu_data;
>> +    const unsigned int ecx_req = CPUID5_ECX_EXTENSIONS_SUPPORTED |
>> +                                 CPUID5_ECX_INTERRUPT_BREAK;
>> +    const struct acpi_processor_cx *cx = NULL;
>> +    static const struct acpi_processor_cx fam17[] = {
>> +        {
>> +            .type = ACPI_STATE_C1,
>> +            .entry_method = ACPI_CSTATE_EM_FFH,
>> +            .address = 0,
> 
> address field will already get set to 0 by default.

Hmm, yes. I'm never really sure whether adding explicit zero
initializers for fields where they aren't don't-cares is better.
Nor (mostly for that reason) am I really consistent in this. I
guess I'll drop the line.

>> +            .latency = 1,
>> +        },
>> +        {
>> +            .type = ACPI_STATE_C2,
>> +            .entry_method = ACPI_CSTATE_EM_HALT,
>> +            .latency = 400,
> 
> Maybe the latency values could be added to cpuidle.h as defines?

I'd rather not, as such constants wouldn't be used in more than one
place. See xen/arch/x86/cpu/mwait-idle.c's respective tables.

>> +        },
>> +    };
>> +
>> +    if ( pm_idle_save && pm_idle != acpi_processor_idle )
>> +        return;
>> +
>> +    if ( vendor_override < 0 )
>> +        return;
>> +
>> +    switch ( c->x86 )
>> +    {
>> +    case 0x18:
>> +        if ( boot_cpu_data.x86_vendor != X86_VENDOR_HYGON )
>> +        {
>> +    default:
>> +            vendor_override = -1;
>> +            return;
>> +        }
>> +        /* fall through */
>> +    case 0x17:
>> +        if ( cpu_has_monitor && c->cpuid_level >= CPUID_MWAIT_LEAF &&
>> +             (cpuid_ecx(CPUID_MWAIT_LEAF) & ecx_req) == ecx_req )
>> +        {
>> +            cx = fam17;
>> +            nr = ARRAY_SIZE(fam17);
>> +            local_apic_timer_c2_ok = true;
>> +            break;
>> +        }
>> +        /* fall through */
>> +    case 0x15:
>> +    case 0x16:
>> +        cx = &fam17[1];
>> +        nr = ARRAY_SIZE(fam17) - 1;
>> +        break;
>> +    }
>> +
>> +    power->flags.has_cst = true;
>> +
>> +    for ( i = 0; i < nr; ++i )
>> +    {
>> +        if ( cx[i].type > max_cstate )
>> +            break;
>> +        power->states[i + 1] = cx[i];
>> +        power->states[i + 1].idx = i + 1;
>> +        power->states[i + 1].target_residency = cx[i].latency * 
>> latency_factor;
> 
> You could set target_residency as part of the initialization, but I
> guess latency_factor being non-const that would move fam17 outside of
> .rodata?

The static being function local - yes, I could, but besides the array
moving out of .rodata I'd then also need to duplicate the latency
values (and as said above I'd like to avoid introducing #define-s for
them).

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel