| Message ID | dba39869b788f7f9d937fac48f0476a0443925f0.1592142369.git.gorbak25@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix broken suspend on some machines | expand |
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.
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
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.
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. >
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();
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.
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.
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
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
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.
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.
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; } } /*