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

[Grzegorz Uriasz <gorbak25@xxxxxxxxx>]

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