* [PATCH 0/1] Fix broken suspend on some machines @ 2020-06-14 14:36 Grzegorz Uriasz 2020-06-14 14:36 ` [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz 0 siblings, 1 reply; 13+ messages in thread From: Grzegorz Uriasz @ 2020-06-14 14:36 UTC (permalink / raw) To: xen-devel Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz, Jan Beulich, j.nowak26, 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). 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-14 14:36 [PATCH 0/1] Fix broken suspend on some machines Grzegorz Uriasz @ 2020-06-14 14:36 ` Grzegorz Uriasz 2020-06-15 7:05 ` Roger Pau Monné 2020-06-16 8:07 ` Jan Beulich 0 siblings, 2 replies; 13+ messages in thread From: Grzegorz Uriasz @ 2020-06-14 14:36 UTC (permalink / raw) To: xen-devel Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz, Jan Beulich, j.nowak26, 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> 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-14 14:36 ` [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz @ 2020-06-15 7:05 ` Roger Pau Monné 2020-06-16 8:07 ` Jan Beulich 1 sibling, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2020-06-15 7:05 UTC (permalink / raw) To: Grzegorz Uriasz Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Jan Beulich, j.nowak26, xen-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-14 14:36 ` [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz 2020-06-15 7:05 ` Roger Pau Monné @ 2020-06-16 8:07 ` Jan Beulich 2020-06-16 10:32 ` Roger Pau Monné 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2020-06-16 8:07 UTC (permalink / raw) To: Grzegorz Uriasz Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, j.nowak26, xen-devel, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 8:07 ` Jan Beulich @ 2020-06-16 10:32 ` Roger Pau Monné 2020-06-16 12:31 ` Jan Beulich 2020-06-16 13:25 ` Jan Beulich 0 siblings, 2 replies; 13+ messages in thread From: Roger Pau Monné @ 2020-06-16 10:32 UTC (permalink / raw) To: Jan Beulich Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz, j.nowak26, xen-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 10:32 ` Roger Pau Monné @ 2020-06-16 12:31 ` Jan Beulich 2020-06-16 13:25 ` Jan Beulich 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2020-06-16 12:31 UTC (permalink / raw) To: Roger Pau Monné Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz, j.nowak26, xen-devel 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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 10:32 ` Roger Pau Monné 2020-06-16 12:31 ` Jan Beulich @ 2020-06-16 13:25 ` Jan Beulich 2020-06-16 14:59 ` Roger Pau Monné 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2020-06-16 13:25 UTC (permalink / raw) To: Roger Pau Monné, Grzegorz Uriasz Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, j.nowak26, xen-devel 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(); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 13:25 ` Jan Beulich @ 2020-06-16 14:59 ` Roger Pau Monné 2020-06-16 15:10 ` Grzegorz Uriasz 2020-06-16 15:11 ` Jan Beulich 0 siblings, 2 replies; 13+ messages in thread From: Roger Pau Monné @ 2020-06-16 14:59 UTC (permalink / raw) To: Jan Beulich Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz, j.nowak26, xen-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 14:59 ` Roger Pau Monné @ 2020-06-16 15:10 ` Grzegorz Uriasz 2020-06-16 15:13 ` Jan Beulich 2020-06-16 15:11 ` Jan Beulich 1 sibling, 1 reply; 13+ messages in thread From: Grzegorz Uriasz @ 2020-06-16 15:10 UTC (permalink / raw) To: Roger Pau Monné, Jan Beulich Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, j.nowak26, xen-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 15:10 ` Grzegorz Uriasz @ 2020-06-16 15:13 ` Jan Beulich 0 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2020-06-16 15:13 UTC (permalink / raw) To: Grzegorz Uriasz Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, j.nowak26, xen-devel, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 14:59 ` Roger Pau Monné 2020-06-16 15:10 ` Grzegorz Uriasz @ 2020-06-16 15:11 ` Jan Beulich 2020-06-16 15:25 ` Roger Pau Monné 1 sibling, 1 reply; 13+ messages in thread From: Jan Beulich @ 2020-06-16 15:11 UTC (permalink / raw) To: Roger Pau Monné Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz, j.nowak26, xen-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 15:11 ` Jan Beulich @ 2020-06-16 15:25 ` Roger Pau Monné 2020-06-17 6:16 ` Grzegorz Uriasz 0 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2020-06-16 15:25 UTC (permalink / raw) To: Jan Beulich Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, Grzegorz Uriasz, j.nowak26, xen-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width 2020-06-16 15:25 ` Roger Pau Monné @ 2020-06-17 6:16 ` Grzegorz Uriasz 0 siblings, 0 replies; 13+ messages in thread From: Grzegorz Uriasz @ 2020-06-17 6:16 UTC (permalink / raw) To: Roger Pau Monné, Jan Beulich Cc: artur, Wei Liu, jakub, Andrew Cooper, marmarek, j.nowak26, xen-devel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-06-17 6:17 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-14 14:36 [PATCH 0/1] Fix broken suspend on some machines Grzegorz Uriasz 2020-06-14 14:36 ` [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width Grzegorz Uriasz 2020-06-15 7:05 ` Roger Pau Monné 2020-06-16 8:07 ` Jan Beulich 2020-06-16 10:32 ` Roger Pau Monné 2020-06-16 12:31 ` Jan Beulich 2020-06-16 13:25 ` Jan Beulich 2020-06-16 14:59 ` Roger Pau Monné 2020-06-16 15:10 ` Grzegorz Uriasz 2020-06-16 15:13 ` Jan Beulich 2020-06-16 15:11 ` Jan Beulich 2020-06-16 15:25 ` Roger Pau Monné 2020-06-17 6:16 ` Grzegorz Uriasz
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.