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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Grzegorz Uriasz <gorbak25@xxxxxxxxx>
  • Date: Fri, 19 Jun 2020 20:23:22 +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, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Fri, 19 Jun 2020 18:23:39 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 19/06/2020 15:17, Jan Beulich wrote:
> On 19.06.2020 06:28, Grzegorz Uriasz wrote:
>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -478,7 +478,9 @@ static int __init acpi_parse_fadt(struct 
>> acpi_table_header *table)
>>      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 ||
>> +                ACPI_ACCESS_BIT_WIDTH(fadt->xpm_timer_block.access_width) 
>> == 32)) {
> Thinking about it again, since we're already tightening this
> check, I think we would better also verify bit_offset == 0.
>
> There also looks to be an indenting blank missing here.
I will add the check and correct the indentation.
>
>> @@ -490,8 +492,10 @@ static int __init acpi_parse_fadt(struct 
>> acpi_table_header *table)
>>       */
>>      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);
> I thought we had agreed to log at the very least the case where
> the FADT flag is set but the width is less than 32 bits. (And I
> agree that perhaps there's not much more to log, unless we'd
> suspect e.g. strange access widths to be present somewhere.)
>
My bad - I've misunderstood the discussion.
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -457,16 +457,16 @@ static u64 read_pmtimer_count(void)
>>  static s64 __init init_pmtimer(struct platform_timesource *pts)
>>  {
>>      u64 start;
>> -    u32 count, target, mask = 0xffffff;
>> +    u32 count, target, mask;
>>  
>> -    if ( !pmtmr_ioport || !pmtmr_width )
>> +    if ( !pmtmr_ioport )
>>          return 0;
>>  
>> -    if ( pmtmr_width == 32 )
>> -    {
>> -        pts->counter_bits = 32;
>> -        mask = 0xffffffff;
>> -    }
>> +    if ( pmtmr_width != 24 && pmtmr_width != 32 )
>> +        return 0;
>> +
>> +    pts->counter_bits = (int) pmtmr_width;
>> +    mask = (1ull << pmtmr_width) - 1;
>>  
>>      count = inl(pmtmr_ioport) & mask;
>>      start = rdtsc_ordered();
>> @@ -486,7 +486,6 @@ static struct platform_timesource __initdata plt_pmtimer 
>> =
>>      .name = "ACPI PM Timer",
>>      .frequency = ACPI_PM_FREQUENCY,
>>      .read_counter = read_pmtimer_count,
>> -    .counter_bits = 24,
>>      .init = init_pmtimer
>>  };
> I'm struggling a little to see why this code churn is needed / wanted.
> The change I had suggested was quite a bit less intrusive. I'm not
> entirely opposed though, but at the very least please drop the odd
> (int) cast. If anything we want the struct field changed to unsigned
> int (but unlikely in this very patch).
>
> If you want to stick to this larger change, then also please fold the
> two if()s at the beginning of the function.

I wanted to avoid hard coding the masks -> Linux has a nice macro for
generating the masks but I haven't found a similar macro in xen.
Additionally this version sets the counter width in a single place
instead of two.

>
> Jan
Best Regards,
Grzegorz

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®.