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