[v3,1/1] x86/acpi: Use FADT flags to determine the PMTMR width
diff mbox series

Message ID de0e33d2be0924e66a220678a7683098df70c2b5.1592603468.git.gorbak25@gmail.com
State New
Headers show
Series
  • Fix broken suspend on some machines
Related show

Commit Message

Grzegorz Uriasz June 19, 2020, 10 p.m. UTC
On some computers the bit width of the PM Timer as reported
by ACPI is 32 bits when in fact the FADT flags report correctly
that the timer is 24 bits wide. On affected machines such as the
ASUS FX504GM and never gaming laptops this results in the inability
to resume the machine from suspend. Without this patch suspend is
broken on affected machines and even if a machine manages to resume
correctly then the kernel time and xen timers are trashed.

Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: marmarek@invisiblethingslab.com
Cc: contact@puzio.waw.pl
Cc: jakub@bartmin.ski
Cc: j.nowak26@student.uw.edu.pl

Changes in v2:
- Check pm timer access width
- Proper timer width is set when xpm block is not present
- Cleanup timer initialization

Changes in v3:
- Check pm timer bit offset
- Warn about acpi firmware bugs
- Drop int cast in init_pmtimer
- Merge if's in init_pmtimer
---
 xen/arch/x86/acpi/boot.c    | 19 +++++++++++++++----
 xen/arch/x86/time.c         | 12 ++++--------
 xen/include/acpi/acmacros.h |  8 ++++++++
 3 files changed, 27 insertions(+), 12 deletions(-)

Comments

Jan Beulich June 22, 2020, 8:49 a.m. UTC | #1
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
Roger Pau Monné June 22, 2020, 8:54 a.m. UTC | #2
On Fri, Jun 19, 2020 at 10:00:16PM +0000, Grzegorz Uriasz wrote:
> On some computers the bit width of the PM Timer as reported
> by ACPI is 32 bits when in fact the FADT flags report correctly
> that the timer is 24 bits wide. On affected machines such as the
> ASUS FX504GM and never gaming laptops this results in the inability
> to resume the machine from suspend. Without this patch suspend is
> broken on affected machines and even if a machine manages to resume
> correctly then the kernel time and xen timers are trashed.
> 
> Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: marmarek@invisiblethingslab.com
> Cc: contact@puzio.waw.pl
> Cc: jakub@bartmin.ski
> Cc: j.nowak26@student.uw.edu.pl
> 
> Changes in v2:
> - Check pm timer access width
> - Proper timer width is set when xpm block is not present
> - Cleanup timer initialization
> 
> Changes in v3:
> - Check pm timer bit offset
> - Warn about acpi firmware bugs
> - Drop int cast in init_pmtimer
> - Merge if's in init_pmtimer
> ---
>  xen/arch/x86/acpi/boot.c    | 19 +++++++++++++++----
>  xen/arch/x86/time.c         | 12 ++++--------
>  xen/include/acpi/acmacros.h |  8 ++++++++
>  3 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
> index bcba52e232..24fd236eb5 100644
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -475,10 +475,17 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>  
>  #ifdef CONFIG_X86_PM_TIMER
>  	/* detect the location of the ACPI PM Timer */
> -	if (fadt->header.revision >= FADT2_REVISION_ID) {
> +	if (fadt->header.revision >= FADT2_REVISION_ID &&
> +	    fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
>  		/* FADT rev. 2 */
> -		if (fadt->xpm_timer_block.space_id ==
> -		    ACPI_ADR_SPACE_SYSTEM_IO) {
> +		if (fadt->xpm_timer_block.access_width != 0 &&
> +		    ACPI_ACCESS_BIT_WIDTH(fadt->xpm_timer_block.access_width) != 32)
> +			printk(KERN_WARNING PREFIX "PM-Timer has invalid access width(%u)\n",
> +			       fadt->xpm_timer_block.access_width);
> +		else if (fadt->xpm_timer_block.bit_offset != 0)

This should be a plain if instead of an else if, it's possible the
block has both the access_width and the bit_offset wrong.

> +			printk(KERN_WARNING PREFIX "PM-Timer has invalid bit offset(%u)\n",
> +			       fadt->xpm_timer_block.bit_offset);
> +		else {
>  			pmtmr_ioport = fadt->xpm_timer_block.address;
>  			pmtmr_width = fadt->xpm_timer_block.bit_width;
>  		}
> @@ -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. Also this should be a fatal error likely? You should set
pmtmr_ioport = 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);
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index d643590c0a..8a45514908 100644
> --- 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;
>  
>      count = inl(pmtmr_ioport) & mask;
>      start = rdtsc_ordered();
> @@ -486,7 +483,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
>  };
>  
> diff --git a/xen/include/acpi/acmacros.h b/xen/include/acpi/acmacros.h
> index 6765535053..86c503c20f 100644
> --- a/xen/include/acpi/acmacros.h
> +++ b/xen/include/acpi/acmacros.h
> @@ -121,6 +121,14 @@
>  #define ACPI_COMPARE_NAME(a,b)          (!ACPI_STRNCMP (ACPI_CAST_PTR (char,(a)), ACPI_CAST_PTR (char,(b)), ACPI_NAME_SIZE))
>  #endif
>  
> +/*
> + * Algorithm to obtain access bit or byte width.
> + * Can be used with access_width of struct acpi_generic_address and access_size of
> + * struct acpi_resource_generic_register.
> + */
> +#define ACPI_ACCESS_BIT_WIDTH(size)     (1 << ((size) + 2))
> +#define ACPI_ACCESS_BYTE_WIDTH(size)    (1 << ((size) - 1))

Note sure I would introduce this last one, since it's not used by the
code.

Thanks, Roger.
Roger Pau Monné June 22, 2020, 8:58 a.m. UTC | #3
On Mon, Jun 22, 2020 at 10:54:00AM +0200, Roger Pau Monné wrote:
> On Fri, Jun 19, 2020 at 10:00:16PM +0000, Grzegorz Uriasz wrote:
> > On some computers the bit width of the PM Timer as reported
> > by ACPI is 32 bits when in fact the FADT flags report correctly
> > that the timer is 24 bits wide. On affected machines such as the
> > ASUS FX504GM and never gaming laptops this results in the inability
> > to resume the machine from suspend. Without this patch suspend is
> > broken on affected machines and even if a machine manages to resume
> > correctly then the kernel time and xen timers are trashed.
> > 
> > Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > Cc: marmarek@invisiblethingslab.com
> > Cc: contact@puzio.waw.pl
> > Cc: jakub@bartmin.ski
> > Cc: j.nowak26@student.uw.edu.pl
> > 
> > Changes in v2:
> > - Check pm timer access width
> > - Proper timer width is set when xpm block is not present
> > - Cleanup timer initialization
> > 
> > Changes in v3:
> > - Check pm timer bit offset
> > - Warn about acpi firmware bugs
> > - Drop int cast in init_pmtimer
> > - Merge if's in init_pmtimer
> > ---
> >  xen/arch/x86/acpi/boot.c    | 19 +++++++++++++++----
> >  xen/arch/x86/time.c         | 12 ++++--------
> >  xen/include/acpi/acmacros.h |  8 ++++++++
> >  3 files changed, 27 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
> > index bcba52e232..24fd236eb5 100644
> > --- a/xen/arch/x86/acpi/boot.c
> > +++ b/xen/arch/x86/acpi/boot.c
> > @@ -475,10 +475,17 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
> >  
> >  #ifdef CONFIG_X86_PM_TIMER
> >  	/* detect the location of the ACPI PM Timer */
> > -	if (fadt->header.revision >= FADT2_REVISION_ID) {
> > +	if (fadt->header.revision >= FADT2_REVISION_ID &&
> > +	    fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> >  		/* FADT rev. 2 */
> > -		if (fadt->xpm_timer_block.space_id ==
> > -		    ACPI_ADR_SPACE_SYSTEM_IO) {
> > +		if (fadt->xpm_timer_block.access_width != 0 &&
> > +		    ACPI_ACCESS_BIT_WIDTH(fadt->xpm_timer_block.access_width) != 32)
> > +			printk(KERN_WARNING PREFIX "PM-Timer has invalid access width(%u)\n",
> > +			       fadt->xpm_timer_block.access_width);
> > +		else if (fadt->xpm_timer_block.bit_offset != 0)
> 
> This should be a plain if instead of an else if, it's possible the
> block has both the access_width and the bit_offset wrong.

My bad, realized this avoids setting pmtmr_ioport.

Roger.
Jan Beulich June 22, 2020, 10:13 a.m. UTC | #4
On 22.06.2020 10:54, Roger Pau Monné wrote:
> On Fri, Jun 19, 2020 at 10:00:16PM +0000, 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. Also this should be a fatal error likely? You should set
> pmtmr_ioport = 0?

Not sure, to be honest. It's entirely fuzzy what mode to use in this
case, yet assuming a working 24-bit timer looks valid in this case.
And indeed we'd use the timer (with a 24-bit mask) exactly when
pmtmr_width == 24 (alongside the bogus set ACPI_FADT_32BIT_TIMER bit).

What I do notice only now though is that inside the if() there's a
pair of parentheses missing.

Jan
Jan Beulich June 24, 2020, 3:31 p.m. UTC | #5
On 22.06.2020 10:49, Jan Beulich wrote:
> 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).

Paul?

Thanks, Jan
Paul Durrant June 25, 2020, 6:55 a.m. UTC | #6
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 24 June 2020 16:32
> To: Paul Durrant <paul@xen.org>
> Cc: Grzegorz Uriasz <gorbak25@gmail.com>; Wei Liu <wl@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: Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
> 
> On 22.06.2020 10:49, Jan Beulich wrote:
> > 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).
> 
> Paul?
> 

Looks ok.

Release-acked-by: Paul Durrant <paul@xen.org>

> Thanks, Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index bcba52e232..24fd236eb5 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -475,10 +475,17 @@  static int __init acpi_parse_fadt(struct acpi_table_header *table)
 
 #ifdef CONFIG_X86_PM_TIMER
 	/* detect the location of the ACPI PM Timer */
-	if (fadt->header.revision >= FADT2_REVISION_ID) {
+	if (fadt->header.revision >= FADT2_REVISION_ID &&
+	    fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
 		/* FADT rev. 2 */
-		if (fadt->xpm_timer_block.space_id ==
-		    ACPI_ADR_SPACE_SYSTEM_IO) {
+		if (fadt->xpm_timer_block.access_width != 0 &&
+		    ACPI_ACCESS_BIT_WIDTH(fadt->xpm_timer_block.access_width) != 32)
+			printk(KERN_WARNING PREFIX "PM-Timer has invalid access width(%u)\n",
+			       fadt->xpm_timer_block.access_width);
+		else if (fadt->xpm_timer_block.bit_offset != 0)
+			printk(KERN_WARNING PREFIX "PM-Timer has invalid bit offset(%u)\n",
+			       fadt->xpm_timer_block.bit_offset);
+		else {
 			pmtmr_ioport = fadt->xpm_timer_block.address;
 			pmtmr_width = fadt->xpm_timer_block.bit_width;
 		}
@@ -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");
+	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);
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index d643590c0a..8a45514908 100644
--- 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;
 
     count = inl(pmtmr_ioport) & mask;
     start = rdtsc_ordered();
@@ -486,7 +483,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
 };
 
diff --git a/xen/include/acpi/acmacros.h b/xen/include/acpi/acmacros.h
index 6765535053..86c503c20f 100644
--- a/xen/include/acpi/acmacros.h
+++ b/xen/include/acpi/acmacros.h
@@ -121,6 +121,14 @@ 
 #define ACPI_COMPARE_NAME(a,b)          (!ACPI_STRNCMP (ACPI_CAST_PTR (char,(a)), ACPI_CAST_PTR (char,(b)), ACPI_NAME_SIZE))
 #endif
 
+/*
+ * Algorithm to obtain access bit or byte width.
+ * Can be used with access_width of struct acpi_generic_address and access_size of
+ * struct acpi_resource_generic_register.
+ */
+#define ACPI_ACCESS_BIT_WIDTH(size)     (1 << ((size) + 2))
+#define ACPI_ACCESS_BYTE_WIDTH(size)    (1 << ((size) - 1))
+
 /*
  * Macros for moving data around to/from buffers that are possibly unaligned.
  * If the hardware supports the transfer of unaligned data, just do the store.