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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.06.19 at 18:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 23/05/2019 13:18, Jan Beulich wrote:
>> At least for more recent CPUs, following what BKDG / PPR suggest for the
>> BIOS to surface via ACPI we can make ourselves independent of Dom0
>> uploading respective data.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> TBD: Can we set local_apic_timer_c2_ok to true? I can't seem to find any
>>      statement in the BKDG / PPR as to whether the LAPIC timer continues
>>      running in CC6.
> 
> This ought to be easy to determine.  Given the description of CC6
> flushing the cache and power gating the core, I'd say there is a
> reasonable chance that the LAPIC timer stops in CC6.

But "reasonable chance" isn't enough for my taste here. And from
what you deduce, the answer to the question would be "no", and
hence simply no change to be made anywhere. (I do think though
that it's more complicated than this, because iirc much also depends
on what the firmware actually does.)

>> TBD: We may want to verify that HLT indeed is configured to enter CC6.
> 
> I can't actually spot anything which talks about HLT directly.  The
> closest I can post is CFOH (cache flush on halt) which is an
> auto-transition from CC1 to CC6 after a specific timeout, but the
> wording suggests that mwait would also take this path.

Well, I had come across a section describing how HLT can be
configured to be the same action as the I/O port read from one
of the three ports involved in C-state management
(CStateBaseAddr+0...2). But I can't seem to find this again.

As to MWAIT behaving the same, I don't think I can spot proof
of your interpretation or proof of Brian's.

>> --- a/xen/arch/x86/acpi/cpu_idle.c
>> +++ b/xen/arch/x86/acpi/cpu_idle.c
>> @@ -1283,6 +1288,98 @@ 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,
>> +            .latency = 1,
>> +        },
>> +        {
>> +            .type = ACPI_STATE_C2,
>> +            .entry_method = ACPI_CSTATE_EM_HALT,
>> +            .latency = 400,
>> +        },
>> +    };
>> +
>> +    if ( pm_idle_save && pm_idle != acpi_processor_idle )
>> +        return;
>> +
>> +    if ( vendor_override < 0 )
>> +        return;
>> +
>> +    switch ( c->x86 )
>> +    {
>> +    case 0x17:
> 
> With Hygon in the mix, this should be expanded to Fam18h.

But only once we get a guarantee from AMD that they won't use
family 18h. Otherwise we'd have to use vendor checks here.
Anyway this series predates the merging of the Hygon one. But
yes, I can easily do this for v2.

Jan



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