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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2020 20:23, Grzegorz Uriasz wrote:
> On 19/06/2020 15:17, Jan Beulich wrote:
>> On 19.06.2020 06:28, Grzegorz Uriasz wrote:
>>> --- 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.

I guessed this was the goal, but I think such adjustments, if indeed
wanted, would better go into a separate patch. The bug fix here wants
backporting, while this extra cleanup probably doesn't.

Jan