[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |