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

[PATCH v2 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).

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

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

 xen/arch/x86/acpi/boot.c    |  8 ++++++--
 xen/arch/x86/time.c         | 15 +++++++--------
 xen/include/acpi/acmacros.h |  8 ++++++++
 3 files changed, 21 insertions(+), 10 deletions(-)

-- 
2.27.0

[PATCH v2 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>
---
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: artur@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

---
 xen/arch/x86/acpi/boot.c    |  8 ++++++--
 xen/arch/x86/time.c         | 15 +++++++--------
 xen/include/acpi/acmacros.h |  8 ++++++++
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index bcba52e232..8d514388b4 100644
--- 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)) {
 			pmtmr_ioport = fadt->xpm_timer_block.address;
 			pmtmr_width = fadt->xpm_timer_block.bit_width;
 		}
@@ -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);
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index d643590c0a..e9180175e0 100644
--- 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
 };
 
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.
-- 
2.27.0

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

[Jan Beulich]

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

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

[Grzegorz Uriasz]

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

On 19/06/2020 15:17, Jan Beulich wrote:
> 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.
I will add the check and correct the indentation.
>
>> @@ -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.)
>
My bad - I've misunderstood the discussion.
>> --- 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.

>
> Jan
Best Regards,
Grzegorz


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

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

[Jan Beulich]

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