[PATCH 0/1] Fix broken suspend on some machines

[PATCH 0/1] Fix broken suspend on some machines

[Grzegorz Uriasz]

Hi,

The included patch is a small subset of a bigger patch set spanning few 
projects aiming to isolate the GPU in Qubes OS to a dedicated security domain.
I'm doing this together with 3 colleagues as part of our Bachelors thesis.
While working on the project we came across 2 machines - newer gaming
laptops on which the suspend functionality on unmodified xen is completely broken.

The affected machines were able to suspend but not always resume. Even
if the resume succeeded then the kernel time was trashed in the dmesg log
and the machine never managed to suspend another time. After changing
the xen clock to hpet, suspend started working again both on stock
xen and Qubes OS - this indicates a bug in the ACPI PM Timer. After
disassembling the FADT ACPI table on the ASUS FX504GM I understood that the
reported bit width is 32 bits but the flags indicate a 24 bit PM timer.

The included patch fixes the suspend feature on ASUS FX504GM and hopefully
other laptops - Probably next week I will test this patch on my
friend's laptop where this issue also occurs(suspend is broken, trashed kernel
time after resume).

Grzegorz Uriasz (1):
  x86/acpi: Use FADT flags to determine the PMTMR width

 xen/arch/x86/acpi/boot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.27.0

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

[Grzegorz Uriasz]

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>
Tested-by: Grzegorz Uriasz <gorbak25@gmail.com>
---
 xen/arch/x86/acpi/boot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index bcba52e232..2ad3eb4abc 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
 		if (fadt->xpm_timer_block.space_id ==
 		    ACPI_ADR_SPACE_SYSTEM_IO) {
 			pmtmr_ioport = fadt->xpm_timer_block.address;
-			pmtmr_width = fadt->xpm_timer_block.bit_width;
+			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
+				pmtmr_width = 32;
+			else
+				pmtmr_width = 24;
 		}
 	}
 	/*
-- 
2.27.0

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

[Roger Pau Monné]

On Sun, Jun 14, 2020 at 02:36:28PM +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>
> Tested-by: Grzegorz Uriasz <gorbak25@gmail.com>

Thanks for the patch! I'm not sure it's required to add your
Tested-by, I usually assume a patch has been tested by it's author
unless told otherwise.

> ---
>  xen/arch/x86/acpi/boot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
> index bcba52e232..2ad3eb4abc 100644
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>  		if (fadt->xpm_timer_block.space_id ==
>  		    ACPI_ADR_SPACE_SYSTEM_IO) {
>  			pmtmr_ioport = fadt->xpm_timer_block.address;
> -			pmtmr_width = fadt->xpm_timer_block.bit_width;
> +			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
> +				pmtmr_width = 32;
> +			else
> +				pmtmr_width = 24;

I think there's also a block below that you need to fix, when
xpm_timer_block is not set the data is fetched from pm_timer_block
instead, which AFAICT also needs to take ACPI_FADT_32BIT_TIMER into
account in order to use the correct size.

FWIW, I would set pmtmr_width only once after pmtmr_ioport has been
set, since regardless of whether the port is discovered using
xpm_timer_block or pm_timer_block the size will always be derived from
the flags field.

Thanks, Roger.

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

[Jan Beulich]

On 14.06.2020 16:36, Grzegorz Uriasz wrote:
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>  		if (fadt->xpm_timer_block.space_id ==
>  		    ACPI_ADR_SPACE_SYSTEM_IO) {
>  			pmtmr_ioport = fadt->xpm_timer_block.address;
> -			pmtmr_width = fadt->xpm_timer_block.bit_width;
> +			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
> +				pmtmr_width = 32;
> +			else
> +				pmtmr_width = 24;

I think disagreement of the two wants logging, and you want to
default to using the smaller of the two (or even to ignoring the
timer altogether). Then there wants to be a way to override
(unless we already have one) our defaulting, in case it's wrong.

Also I'd prefer if you used a conditional operator for this
assignment.

Jan

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

[Roger Pau Monné]

On Tue, Jun 16, 2020 at 10:07:05AM +0200, Jan Beulich wrote:
> On 14.06.2020 16:36, Grzegorz Uriasz wrote:
> > --- a/xen/arch/x86/acpi/boot.c
> > +++ b/xen/arch/x86/acpi/boot.c
> > @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
> >  		if (fadt->xpm_timer_block.space_id ==
> >  		    ACPI_ADR_SPACE_SYSTEM_IO) {
> >  			pmtmr_ioport = fadt->xpm_timer_block.address;
> > -			pmtmr_width = fadt->xpm_timer_block.bit_width;
> > +			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
> > +				pmtmr_width = 32;
> > +			else
> > +				pmtmr_width = 24;
> 
> I think disagreement of the two wants logging, and you want to
> default to using the smaller of the two (or even to ignoring the
> timer altogether). Then there wants to be a way to override
> (unless we already have one) our defaulting, in case it's wrong.

TBH, I presume timer_block will always return 32bits, because that's
the size of the register. Then the timer can implement less bits than
the full size of the register, and that's what gets signaled using the
ACPI flags. What we care about here is the number of bits used by the
timer, not the size of the register.

I think we should only ignore the timer if pm_timer_block.bit_width <
pmtmr_width.

Printing a (debug) message when those values disagree is fine, but I
bet it's going to trigger always when the implemented timer is only
using 24bits.

Roger.

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

[Jan Beulich]

On 16.06.2020 12:32, Roger Pau Monné wrote:
> On Tue, Jun 16, 2020 at 10:07:05AM +0200, Jan Beulich wrote:
>> On 14.06.2020 16:36, Grzegorz Uriasz wrote:
>>> --- a/xen/arch/x86/acpi/boot.c
>>> +++ b/xen/arch/x86/acpi/boot.c
>>> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>>  		if (fadt->xpm_timer_block.space_id ==
>>>  		    ACPI_ADR_SPACE_SYSTEM_IO) {
>>>  			pmtmr_ioport = fadt->xpm_timer_block.address;
>>> -			pmtmr_width = fadt->xpm_timer_block.bit_width;
>>> +			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
>>> +				pmtmr_width = 32;
>>> +			else
>>> +				pmtmr_width = 24;
>>
>> I think disagreement of the two wants logging, and you want to
>> default to using the smaller of the two (or even to ignoring the
>> timer altogether). Then there wants to be a way to override
>> (unless we already have one) our defaulting, in case it's wrong.
> 
> TBH, I presume timer_block will always return 32bits, because that's
> the size of the register. Then the timer can implement less bits than
> the full size of the register, and that's what gets signaled using the
> ACPI flags. What we care about here is the number of bits used by the
> timer, not the size of the register.

The first random system I checked this on reports 24 bits as bit_width
(and the flag clear, i.e. both are consistent). The flag, aiui, is
really important only in the ACPI v1 case, where the size of the
register was a byte-granular value. The spec isn't helpful in
clarifying applicability of the flag though, i.e. one can interpret it
either way imo.

Jan

> I think we should only ignore the timer if pm_timer_block.bit_width <
> pmtmr_width.
> 
> Printing a (debug) message when those values disagree is fine, but I
> bet it's going to trigger always when the implemented timer is only
> using 24bits.
> 
> Roger.
> 

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

[Jan Beulich]

On 16.06.2020 12:32, Roger Pau Monné wrote:
> On Tue, Jun 16, 2020 at 10:07:05AM +0200, Jan Beulich wrote:
>> On 14.06.2020 16:36, Grzegorz Uriasz wrote:
>>> --- a/xen/arch/x86/acpi/boot.c
>>> +++ b/xen/arch/x86/acpi/boot.c
>>> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>>  		if (fadt->xpm_timer_block.space_id ==
>>>  		    ACPI_ADR_SPACE_SYSTEM_IO) {
>>>  			pmtmr_ioport = fadt->xpm_timer_block.address;
>>> -			pmtmr_width = fadt->xpm_timer_block.bit_width;
>>> +			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
>>> +				pmtmr_width = 32;
>>> +			else
>>> +				pmtmr_width = 24;
>>
>> I think disagreement of the two wants logging, and you want to
>> default to using the smaller of the two (or even to ignoring the
>> timer altogether). Then there wants to be a way to override
>> (unless we already have one) our defaulting, in case it's wrong.
> 
> TBH, I presume timer_block will always return 32bits, because that's
> the size of the register. Then the timer can implement less bits than
> the full size of the register, and that's what gets signaled using the
> ACPI flags. What we care about here is the number of bits used by the
> timer, not the size of the register.
> 
> I think we should only ignore the timer if pm_timer_block.bit_width <
> pmtmr_width.
> 
> Printing a (debug) message when those values disagree is fine, but I
> bet it's going to trigger always when the implemented timer is only
> using 24bits.

The 2nd system I tried on would trigger it, so maybe there's no point
in logging indeed. How about the below as a basis?

Jan

--- unstable.orig/xen/arch/x86/acpi/boot.c
+++ unstable/xen/arch/x86/acpi/boot.c
@@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
 	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 ||
+		     fadt->xpm_timer_block.access_width == 3)) {
 			pmtmr_ioport = fadt->xpm_timer_block.address;
 			pmtmr_width = fadt->xpm_timer_block.bit_width;
 		}
@@ -492,8 +494,10 @@ static int __init acpi_parse_fadt(struct
  	 */
 	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);
--- unstable.orig/xen/arch/x86/time.c
+++ unstable/xen/arch/x86/time.c
@@ -465,7 +465,7 @@ static s64 __init init_pmtimer(struct pl
     u64 start;
     u32 count, target, mask = 0xffffff;
 
-    if ( !pmtmr_ioport || !pmtmr_width )
+    if ( !pmtmr_ioport )
         return 0;
 
     if ( pmtmr_width == 32 )
@@ -473,6 +473,8 @@ static s64 __init init_pmtimer(struct pl
         pts->counter_bits = 32;
         mask = 0xffffffff;
     }
+    else if ( pmtmr_width != pts->counter_bits )
+        return 0;
 
     count = inl(pmtmr_ioport) & mask;
     start = rdtsc_ordered();

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

[Roger Pau Monné]

On Tue, Jun 16, 2020 at 03:25:42PM +0200, Jan Beulich wrote:
> On 16.06.2020 12:32, Roger Pau Monné wrote:
> > On Tue, Jun 16, 2020 at 10:07:05AM +0200, Jan Beulich wrote:
> >> On 14.06.2020 16:36, Grzegorz Uriasz wrote:
> >>> --- a/xen/arch/x86/acpi/boot.c
> >>> +++ b/xen/arch/x86/acpi/boot.c
> >>> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
> >>>  		if (fadt->xpm_timer_block.space_id ==
> >>>  		    ACPI_ADR_SPACE_SYSTEM_IO) {
> >>>  			pmtmr_ioport = fadt->xpm_timer_block.address;
> >>> -			pmtmr_width = fadt->xpm_timer_block.bit_width;
> >>> +			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
> >>> +				pmtmr_width = 32;
> >>> +			else
> >>> +				pmtmr_width = 24;
> >>
> >> I think disagreement of the two wants logging, and you want to
> >> default to using the smaller of the two (or even to ignoring the
> >> timer altogether). Then there wants to be a way to override
> >> (unless we already have one) our defaulting, in case it's wrong.
> > 
> > TBH, I presume timer_block will always return 32bits, because that's
> > the size of the register. Then the timer can implement less bits than
> > the full size of the register, and that's what gets signaled using the
> > ACPI flags. What we care about here is the number of bits used by the
> > timer, not the size of the register.
> > 
> > I think we should only ignore the timer if pm_timer_block.bit_width <
> > pmtmr_width.
> > 
> > Printing a (debug) message when those values disagree is fine, but I
> > bet it's going to trigger always when the implemented timer is only
> > using 24bits.
> 
> The 2nd system I tried on would trigger it, so maybe there's no point
> in logging indeed. How about the below as a basis?
> 
> Jan
> 
> --- unstable.orig/xen/arch/x86/acpi/boot.c
> +++ unstable/xen/arch/x86/acpi/boot.c
> @@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
>  	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 ||
> +		     fadt->xpm_timer_block.access_width == 3)) {

We should really have defines for those values, or else they seem to
imply actual access sizes. What about adding
ACPI_ADDR_ACCESS_{LEGACY,BYTE,WORD,DWORD,QWORD}?

Also the check for the access size seems kind of unrelated to the
patch itself? (not that I'm opposed to it)

>  			pmtmr_ioport = fadt->xpm_timer_block.address;
>  			pmtmr_width = fadt->xpm_timer_block.bit_width;
>  		}
> @@ -492,8 +494,10 @@ static int __init acpi_parse_fadt(struct
>   	 */
>  	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);
> --- unstable.orig/xen/arch/x86/time.c
> +++ unstable/xen/arch/x86/time.c
> @@ -465,7 +465,7 @@ static s64 __init init_pmtimer(struct pl
>      u64 start;
>      u32 count, target, mask = 0xffffff;
>  
> -    if ( !pmtmr_ioport || !pmtmr_width )
> +    if ( !pmtmr_ioport )
>          return 0;
>  
>      if ( pmtmr_width == 32 )
> @@ -473,6 +473,8 @@ static s64 __init init_pmtimer(struct pl
>          pts->counter_bits = 32;
>          mask = 0xffffffff;
>      }
> +    else if ( pmtmr_width != pts->counter_bits )
> +        return 0;
>  
>      count = inl(pmtmr_ioport) & mask;
>      start = rdtsc_ordered();

The rest LGTM.

Thanks, Roger.

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

[Grzegorz Uriasz]

[-- Attachment #1.1: Type: text/plain, Size: 4172 bytes --]

I'm wondering why support for 32 bit acpi pm timers was introduced to xen.
Linux doesn't bother and just crops it to 24 bits:
https://github.com/torvalds/linux/blob/a5dc8300df75e8b8384b4c82225f1e4a0b4d9b55/drivers/clocksource/acpi_pm.c#L37

Best regards,
Grzegorz Uriasz

On 16/06/2020 16:59, Roger Pau Monné wrote:
> On Tue, Jun 16, 2020 at 03:25:42PM +0200, Jan Beulich wrote:
>> On 16.06.2020 12:32, Roger Pau Monné wrote:
>>> On Tue, Jun 16, 2020 at 10:07:05AM +0200, Jan Beulich wrote:
>>>> On 14.06.2020 16:36, Grzegorz Uriasz wrote:
>>>>> --- a/xen/arch/x86/acpi/boot.c
>>>>> +++ b/xen/arch/x86/acpi/boot.c
>>>>> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>>>>  		if (fadt->xpm_timer_block.space_id ==
>>>>>  		    ACPI_ADR_SPACE_SYSTEM_IO) {
>>>>>  			pmtmr_ioport = fadt->xpm_timer_block.address;
>>>>> -			pmtmr_width = fadt->xpm_timer_block.bit_width;
>>>>> +			if (fadt->flags & ACPI_FADT_32BIT_TIMER)
>>>>> +				pmtmr_width = 32;
>>>>> +			else
>>>>> +				pmtmr_width = 24;
>>>> I think disagreement of the two wants logging, and you want to
>>>> default to using the smaller of the two (or even to ignoring the
>>>> timer altogether). Then there wants to be a way to override
>>>> (unless we already have one) our defaulting, in case it's wrong.
>>> TBH, I presume timer_block will always return 32bits, because that's
>>> the size of the register. Then the timer can implement less bits than
>>> the full size of the register, and that's what gets signaled using the
>>> ACPI flags. What we care about here is the number of bits used by the
>>> timer, not the size of the register.
>>>
>>> I think we should only ignore the timer if pm_timer_block.bit_width <
>>> pmtmr_width.
>>>
>>> Printing a (debug) message when those values disagree is fine, but I
>>> bet it's going to trigger always when the implemented timer is only
>>> using 24bits.
>> The 2nd system I tried on would trigger it, so maybe there's no point
>> in logging indeed. How about the below as a basis?
>>
>> Jan
>>
>> --- unstable.orig/xen/arch/x86/acpi/boot.c
>> +++ unstable/xen/arch/x86/acpi/boot.c
>> @@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
>>  	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 ||
>> +		     fadt->xpm_timer_block.access_width == 3)) {
> We should really have defines for those values, or else they seem to
> imply actual access sizes. What about adding
> ACPI_ADDR_ACCESS_{LEGACY,BYTE,WORD,DWORD,QWORD}?
>
> Also the check for the access size seems kind of unrelated to the
> patch itself? (not that I'm opposed to it)
>
>>  			pmtmr_ioport = fadt->xpm_timer_block.address;
>>  			pmtmr_width = fadt->xpm_timer_block.bit_width;
>>  		}
>> @@ -492,8 +494,10 @@ static int __init acpi_parse_fadt(struct
>>   	 */
>>  	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);
>> --- unstable.orig/xen/arch/x86/time.c
>> +++ unstable/xen/arch/x86/time.c
>> @@ -465,7 +465,7 @@ static s64 __init init_pmtimer(struct pl
>>      u64 start;
>>      u32 count, target, mask = 0xffffff;
>>  
>> -    if ( !pmtmr_ioport || !pmtmr_width )
>> +    if ( !pmtmr_ioport )
>>          return 0;
>>  
>>      if ( pmtmr_width == 32 )
>> @@ -473,6 +473,8 @@ static s64 __init init_pmtimer(struct pl
>>          pts->counter_bits = 32;
>>          mask = 0xffffffff;
>>      }
>> +    else if ( pmtmr_width != pts->counter_bits )
>> +        return 0;
>>  
>>      count = inl(pmtmr_ioport) & mask;
>>      start = rdtsc_ordered();
> The rest LGTM.
>
> Thanks, Roger.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jan Beulich]

On 16.06.2020 16:59, Roger Pau Monné wrote:
> On Tue, Jun 16, 2020 at 03:25:42PM +0200, Jan Beulich wrote:
>> --- unstable.orig/xen/arch/x86/acpi/boot.c
>> +++ unstable/xen/arch/x86/acpi/boot.c
>> @@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
>>  	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 ||
>> +		     fadt->xpm_timer_block.access_width == 3)) {
> 
> We should really have defines for those values, or else they seem to
> imply actual access sizes. What about adding
> ACPI_ADDR_ACCESS_{LEGACY,BYTE,WORD,DWORD,QWORD}?

If there were such defines, I'd have used them. Adding them is
inappropriate though, as this would modify an imported header in a
Xen-specific way. We could leverage ACPI_ACCESS_BIT_WIDTH() here,
though.

> Also the check for the access size seems kind of unrelated to the
> patch itself? (not that I'm opposed to it)

It's related, but could also live in its own patch.

Jan

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

[Jan Beulich]

On 16.06.2020 17:10, Grzegorz Uriasz wrote:
> I'm wondering why support for 32 bit acpi pm timers was introduced to xen.

The handling of the timer wrapping is less overhead is a wider timer can
be used.

Jan

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

[Roger Pau Monné]

On Tue, Jun 16, 2020 at 05:11:58PM +0200, Jan Beulich wrote:
> On 16.06.2020 16:59, Roger Pau Monné wrote:
> > On Tue, Jun 16, 2020 at 03:25:42PM +0200, Jan Beulich wrote:
> >> --- unstable.orig/xen/arch/x86/acpi/boot.c
> >> +++ unstable/xen/arch/x86/acpi/boot.c
> >> @@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
> >>  	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 ||
> >> +		     fadt->xpm_timer_block.access_width == 3)) {
> > 
> > We should really have defines for those values, or else they seem to
> > imply actual access sizes. What about adding
> > ACPI_ADDR_ACCESS_{LEGACY,BYTE,WORD,DWORD,QWORD}?
> 
> If there were such defines, I'd have used them. Adding them is
> inappropriate though, as this would modify an imported header in a
> Xen-specific way. We could leverage ACPI_ACCESS_BIT_WIDTH() here,
> though.

Yes, that would be better IMO.

Thanks, Roger.

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

[Grzegorz Uriasz]

[-- Attachment #1.1: Type: text/plain, Size: 1233 bytes --]

I will submit the v2 patch today.

Best Regards,
Grzegorz

On 16/06/2020 17:25, Roger Pau Monné wrote:
> On Tue, Jun 16, 2020 at 05:11:58PM +0200, Jan Beulich wrote:
>> On 16.06.2020 16:59, Roger Pau Monné wrote:
>>> On Tue, Jun 16, 2020 at 03:25:42PM +0200, Jan Beulich wrote:
>>>> --- unstable.orig/xen/arch/x86/acpi/boot.c
>>>> +++ unstable/xen/arch/x86/acpi/boot.c
>>>> @@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
>>>>  	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 ||
>>>> +		     fadt->xpm_timer_block.access_width == 3)) {
>>> We should really have defines for those values, or else they seem to
>>> imply actual access sizes. What about adding
>>> ACPI_ADDR_ACCESS_{LEGACY,BYTE,WORD,DWORD,QWORD}?
>> If there were such defines, I'd have used them. Adding them is
>> inappropriate though, as this would modify an imported header in a
>> Xen-specific way. We could leverage ACPI_ACCESS_BIT_WIDTH() here,
>> though.
> Yes, that would be better IMO.
>
> Thanks, Roger.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]