Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width

[Grzegorz Uriasz <gorbak25@xxxxxxxxx>]

I'm wondering why support for 32 bit acpi pm timers was introduced to xen.
Linux doesn't bother and just crops it to 24 bits:
https://github.com/torvalds/linux/blob/a5dc8300df75e8b8384b4c82225f1e4a0b4d9b55/drivers/clocksource/acpi_pm.c#L37

Best regards,
Grzegorz Uriasz

On 16/06/2020 16:59, Roger Pau Monné wrote:
> On Tue, Jun 16, 2020 at 03:25:42PM +0200, Jan Beulich wrote:
>> On 16.06.2020 12:32, Roger Pau Monné wrote:
>>> On Tue, Jun 16, 2020 at 10:07:05AM +0200, Jan Beulich wrote:
>>>> On 14.06.2020 16:36, Grzegorz Uriasz wrote:
>>>>> --- a/xen/arch/x86/acpi/boot.c
>>>>> +++ b/xen/arch/x86/acpi/boot.c
>>>>> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct 
>>>>> acpi_table_header *table)
>>>>>           if (fadt->xpm_timer_block.space_id ==
>>>>>               ACPI_ADR_SPACE_SYSTEM_IO) {
>>>>>                   pmtmr_ioport = fadt->xpm_timer_block.address;
>>>>> -                 pmtmr_width = fadt->xpm_timer_block.bit_width;
>>>>> +                 if (fadt->flags & ACPI_FADT_32BIT_TIMER)
>>>>> +                         pmtmr_width = 32;
>>>>> +                 else
>>>>> +                         pmtmr_width = 24;
>>>> I think disagreement of the two wants logging, and you want to
>>>> default to using the smaller of the two (or even to ignoring the
>>>> timer altogether). Then there wants to be a way to override
>>>> (unless we already have one) our defaulting, in case it's wrong.
>>> TBH, I presume timer_block will always return 32bits, because that's
>>> the size of the register. Then the timer can implement less bits than
>>> the full size of the register, and that's what gets signaled using the
>>> ACPI flags. What we care about here is the number of bits used by the
>>> timer, not the size of the register.
>>>
>>> I think we should only ignore the timer if pm_timer_block.bit_width <
>>> pmtmr_width.
>>>
>>> Printing a (debug) message when those values disagree is fine, but I
>>> bet it's going to trigger always when the implemented timer is only
>>> using 24bits.
>> The 2nd system I tried on would trigger it, so maybe there's no point
>> in logging indeed. How about the below as a basis?
>>
>> Jan
>>
>> --- unstable.orig/xen/arch/x86/acpi/boot.c
>> +++ unstable/xen/arch/x86/acpi/boot.c
>> @@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
>>      if (fadt->header.revision >= FADT2_REVISION_ID) {
>>              /* FADT rev. 2 */
>>              if (fadt->xpm_timer_block.space_id ==
>> -                ACPI_ADR_SPACE_SYSTEM_IO) {
>> +                ACPI_ADR_SPACE_SYSTEM_IO &&
>> +                (fadt->xpm_timer_block.access_width == 0 ||
>> +                 fadt->xpm_timer_block.access_width == 3)) {
> We should really have defines for those values, or else they seem to
> imply actual access sizes. What about adding
> ACPI_ADDR_ACCESS_{LEGACY,BYTE,WORD,DWORD,QWORD}?
>
> Also the check for the access size seems kind of unrelated to the
> patch itself? (not that I'm opposed to it)
>
>>                      pmtmr_ioport = fadt->xpm_timer_block.address;
>>                      pmtmr_width = fadt->xpm_timer_block.bit_width;
>>              }
>> @@ -492,8 +494,10 @@ static int __init acpi_parse_fadt(struct
>>       */
>>      if (!pmtmr_ioport) {
>>              pmtmr_ioport = fadt->pm_timer_block;
>> -            pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0;
>> +            pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0;
>>      }
>> +    if (pmtmr_width > 24 && !(fadt->flags & ACPI_FADT_32BIT_TIMER))
>> +            pmtmr_width = 24;
>>      if (pmtmr_ioport)
>>              printk(KERN_INFO PREFIX "PM-Timer IO Port: %#x (%u bits)\n",
>>                     pmtmr_ioport, pmtmr_width);
>> --- unstable.orig/xen/arch/x86/time.c
>> +++ unstable/xen/arch/x86/time.c
>> @@ -465,7 +465,7 @@ static s64 __init init_pmtimer(struct pl
>>      u64 start;
>>      u32 count, target, mask = 0xffffff;
>>  
>> -    if ( !pmtmr_ioport || !pmtmr_width )
>> +    if ( !pmtmr_ioport )
>>          return 0;
>>  
>>      if ( pmtmr_width == 32 )
>> @@ -473,6 +473,8 @@ static s64 __init init_pmtimer(struct pl
>>          pts->counter_bits = 32;
>>          mask = 0xffffffff;
>>      }
>> +    else if ( pmtmr_width != pts->counter_bits )
>> +        return 0;
>>  
>>      count = inl(pmtmr_ioport) & mask;
>>      start = rdtsc_ordered();
> The rest LGTM.
>
> Thanks, Roger.

Attachment: signature.asc
Description: OpenPGP digital signature