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 = ¤t_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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
