* [PATCH v3 0/1] Fix broken suspend on some machines
@ 2020-06-19 22:00 Grzegorz Uriasz
2020-06-19 22:00 ` [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz
0 siblings, 1 reply; 8+ messages in thread
From: Grzegorz Uriasz @ 2020-06-19 22:00 UTC (permalink / raw
To: xen-devel
Cc: Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz,
Jan Beulich, j.nowak26, contact, Roger Pau Monné
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
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
Grzegorz Uriasz (1):
x86/acpi: Use FADT flags to determine the PMTMR width
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(-)
--
2.27.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
2020-06-19 22:00 [PATCH v3 0/1] Fix broken suspend on some machines Grzegorz Uriasz
@ 2020-06-19 22:00 ` Grzegorz Uriasz
2020-06-22 8:49 ` Jan Beulich
2020-06-22 8:54 ` Roger Pau Monné
0 siblings, 2 replies; 8+ messages in thread
From: Grzegorz Uriasz @ 2020-06-19 22:00 UTC (permalink / raw
To: xen-devel
Cc: Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz,
Jan Beulich, j.nowak26, contact, Roger Pau Monné
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)
+ 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.
--
2.27.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
2020-06-19 22:00 ` [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz
@ 2020-06-22 8:49 ` Jan Beulich
2020-06-24 15:31 ` Ping: " Jan Beulich
2020-06-22 8:54 ` Roger Pau Monné
1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2020-06-22 8:49 UTC (permalink / raw
To: Grzegorz Uriasz
Cc: Wei Liu, Paul Durrant, jakub, Andrew Cooper, marmarek, j.nowak26,
xen-devel, contact, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
2020-06-19 22:00 ` [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz
2020-06-22 8:49 ` Jan Beulich
@ 2020-06-22 8:54 ` Roger Pau Monné
2020-06-22 8:58 ` Roger Pau Monné
2020-06-22 10:13 ` Jan Beulich
1 sibling, 2 replies; 8+ messages in thread
From: Roger Pau Monné @ 2020-06-22 8:54 UTC (permalink / raw
To: Grzegorz Uriasz
Cc: Wei Liu, jakub, Andrew Cooper, marmarek, Jan Beulich, j.nowak26,
xen-devel, contact
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
2020-06-22 8:54 ` Roger Pau Monné
@ 2020-06-22 8:58 ` Roger Pau Monné
2020-06-22 10:13 ` Jan Beulich
1 sibling, 0 replies; 8+ messages in thread
From: Roger Pau Monné @ 2020-06-22 8:58 UTC (permalink / raw
To: Grzegorz Uriasz
Cc: Wei Liu, jakub, Andrew Cooper, marmarek, Jan Beulich, j.nowak26,
xen-devel, contact
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
2020-06-22 8:54 ` Roger Pau Monné
2020-06-22 8:58 ` Roger Pau Monné
@ 2020-06-22 10:13 ` Jan Beulich
1 sibling, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2020-06-22 10:13 UTC (permalink / raw
To: Roger Pau Monné
Cc: Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz,
j.nowak26, xen-devel, contact
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
2020-06-22 8:49 ` Jan Beulich
@ 2020-06-24 15:31 ` Jan Beulich
2020-06-25 6:55 ` Paul Durrant
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2020-06-24 15:31 UTC (permalink / raw
To: Paul Durrant
Cc: Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz,
j.nowak26, xen-devel, contact, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
2020-06-24 15:31 ` Ping: " Jan Beulich
@ 2020-06-25 6:55 ` Paul Durrant
0 siblings, 0 replies; 8+ messages in thread
From: Paul Durrant @ 2020-06-25 6:55 UTC (permalink / raw
To: 'Jan Beulich'
Cc: 'Wei Liu', jakub, 'Andrew Cooper', marmarek,
'Grzegorz Uriasz', j.nowak26, xen-devel, contact,
'Roger Pau Monné'
> -----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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-25 6:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-19 22:00 [PATCH v3 0/1] Fix broken suspend on some machines Grzegorz Uriasz
2020-06-19 22:00 ` [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz
2020-06-22 8:49 ` Jan Beulich
2020-06-24 15:31 ` Ping: " Jan Beulich
2020-06-25 6:55 ` Paul Durrant
2020-06-22 8:54 ` Roger Pau Monné
2020-06-22 8:58 ` Roger Pau Monné
2020-06-22 10:13 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.