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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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.)

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

Jan