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

From: Jan Beulich <jbeulich@suse.com>
To: Grzegorz Uriasz <gorbak25@gmail.com>
Cc: "Wei Liu" <wl@xen.org>, "Paul Durrant" <paul@xen.org>,
	jakub@bartmin.ski, "Andrew Cooper" <andrew.cooper3@citrix.com>,
	marmarek@invisiblethingslab.com, j.nowak26@student.uw.edu.pl,
	xen-devel@lists.xenproject.org, contact@puzio.waw.pl,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
Date: Mon, 22 Jun 2020 10:49:04 +0200	[thread overview]
Message-ID: <5993e716-d71d-cbc0-a186-5325e2bdeba4@suse.com> (raw)
In-Reply-To: <de0e33d2be0924e66a220678a7683098df70c2b5.1592603468.git.gorbak25@gmail.com>

On 20.06.2020 00:00, Grzegorz Uriasz wrote:
> @@ -490,8 +497,12 @@ 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 < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER)
> +        printk(KERN_WARNING PREFIX "PM-Timer is too short\n");

Indentation is screwed up here.

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -457,16 +457,13 @@ 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 || (pmtmr_width != 24 && pmtmr_width != 32) )
>          return 0;
>  
> -    if ( pmtmr_width == 32 )
> -    {
> -        pts->counter_bits = 32;
> -        mask = 0xffffffff;
> -    }
> +    pts->counter_bits = pmtmr_width;
> +    mask = (1ull << pmtmr_width) - 1;

"mask" being of "u32" type, the use of 1ull is certainly a little odd
here, and one of the reasons why I think this extra "cleanup" would
better go in a separate patch.

Seeing that besides cosmetic aspects the patch looks okay now, I'd be
willing to leave the bigger adjustment mostly as is, albeit I'd
prefer the 1ull to go away, by instead switching to e.g.

    mask = 0xffffffff >> (32 - pmtmr_width);

With the adjustments (which I'd be happy to make while committing,
provided you agree)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Also Cc-ing Paul for a possible release ack (considering this is a
backporting candidate).

Jan