[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • From: Grzegorz Uriasz <gorbak25@xxxxxxxxx>
  • Date: Tue, 16 Jun 2020 17:10:11 +0200
  • Autocrypt: addr=gorbak25@xxxxxxxxx; prefer-encrypt=mutual; keydata= mQENBFyZgUUBCACo21Uf58hRRgX0uQd3kRJUqXp8/kJsC58tKZG9ENy8rvmcq15AmblqOQnD k6Pmh2lhh31A+m4ONF+TT0vlFYv9sN0kA1llvHPH95LYhROXt7UwSZBQTnQlLZFVqeGa3R6M pJwaAMQP/lyYgINt1pvBdCWtcNP/wKuNI/efnZuBOPDKSeBH/V0ZmmZxlSsx05/ktgtR6ibP FZXPBgx5DY0DxPm/jb8CqkXO5291wyYCP2VDy85oqG8EgsfFOOPZNwBQCKS7cWLZDMEsVNnB WyU3zJZBvEVK/5BwIzm1+AL9lR6RRLaOpC19k2ZqbyhEG9EhsR+/DIF0znBd9Nhjokd5ABEB AAG0JEdyemVnb3J6IFVyaWFzeiA8Z29yYmFrMjVAZ21haWwuY29tPokBVwQTAQgAQQIbAwUJ A8JnAAULCQgHAgYVCgkICwIEFgIDAQIeAQIXgBYhBHDkRnQjCGTnaTo/LngD9fcXMA1pBQJd /5CUAhkBAAoJEHgD9fcXMA1pb9YIAICTmZOrEGho9MC7z851jw507EamIqaAXQAADKpbxSGH yVxALYZv6cqR84rsQuML0N8ai4rdCJ7PviMYyaWVX3P09kJoFxSzKhjxbwZMkRF8O+9tIhNO 29h8v5cmv7Sp4NpssITFMZAms4LleJtdkivDeRxSCyJnHceZyGiD6pPTkopyuISGZIS+4axF zarn+JM+uiTm1QHdCbcvK3sor//QvHr9kKjLKTxMwiTZOGQzF8+oHpVxxwX8Bz3UbFwRX/Io rErT92f9WOsFWnvZHuLGcRLSlG0h9xljIuhP7mvGiudTgNoPt9YFtAG8KT/2HnUZ4XxJKi+B 8Ilet++3m4m5AQ0EXJmBRQEIANBww0bVhYwP1g4AMux/Fjp7KUjYj7Q8ej+t71ShZkAzvPQT 00ULdnYEa62uKM9YwXqOu0XJsnBveJKO1q3nfJuazAbsC0B3v0bYlUUQoTRxCUs3v22UOVxe kaTtN3KDdnSTq7QkkZBZFvV+vAwKGlqECzsYtZ86CiIEOgmUeVA82AzyMx306l1th90OdHYt LKkHxreEPWInN9jptOaKNwvB5cR6PxFfVRtVteN121tVJRK5geA0RVjHn35ig97ycDP5FcwP HuuuKfr+07ANXrtFM/QLGmIvEaMEgpPmzyGkXGhbsDpMikHOkXvQCRTesJ38r8DRUffinYXI vAw0IsMAEQEAAYkBPAQYAQgAJhYhBHDkRnQjCGTnaTo/LngD9fcXMA1pBQJcmYFFAhsMBQkD wmcAAAoJEHgD9fcXMA1p3y8H/2nftQbUcb2oKtpyePStdmdN45A1OWjorj6iBRvTOhoig6y7 PbveJ9Zj35IP0Zy4W77Ke4ayK/PiBkh7bRrdQAwTAc7EiYw3j+foO32JA/4bANMgSuRBxO/d xoloRPoaRE6eGUkA3N65V5WlWkinKxzSGDgSOz7Tit5QY8fGJYWeLTzFj605Y9iUu0MSLpfs LQQby+I9gETWh5TUMz1uNytIB80UdMpzqcC36zCMk7wIy1g8YhbehJq1zU1+ZpDrggrN3ucH 0NFFvHq5uwEkR8Llj29nDcyKuWMlnCMpM/iJcTde7N8UVdtN9yBGol4+yAZT0w5RP87pw3oD fuZMcoY=
  • Cc: artur@xxxxxxxxxxxx, Wei Liu <wl@xxxxxxx>, jakub@xxxxxxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, marmarek@xxxxxxxxxxxxxxxxxxxxxx, j.nowak26@xxxxxxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 16 Jun 2020 15:10:28 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.