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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.