Bug 262638 - overly strict check of hpet region size
Summary: overly strict check of hpet region size
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 13.0-STABLE
Hardware: amd64 Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-03-17 23:15 UTC by John F. Carr
Modified: 2022-04-27 13:48 UTC (History)
2 users (show)

See Also:


Attachments
lower minimum region size to 512 (653 bytes, patch)
2022-03-17 23:37 UTC, John F. Carr
no flags Details | Diff
a more complicated patch (2.19 KB, patch)
2022-03-19 23:40 UTC, John F. Carr
no flags Details | Diff
another patch (2.20 KB, patch)
2022-03-22 20:32 UTC, John F. Carr
no flags Details | Diff
proposed patch (2.53 KB, patch)
2022-03-25 16:05 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John F. Carr 2022-03-17 23:15:42 UTC
FreeBSD warns during boot of my HPE Proliant DL325 (AMD EPYC 7402P):

hpet0: <High Precision Event Timer> iomem 0xfed00000-0xfed001ff on acpi0
hpet0: memory region width 512 too small
device_attach: hpet0 attach returned 6

The hpet driver requires at least 1024 bytes of address space.  This appears to be overly strict because the address space used depends on the number of counters.  See HPET_TIMER_CAP_CNF and related macros.  My 512 byte address space should be sufficient for 8 counters.  (I have not checked what the configuration registers say.  I can hack up my kernel to report them if the answer is imporant.)
Comment 1 John F. Carr 2022-03-17 23:37:33 UTC
Created attachment 232539 [details]
lower minimum region size to 512
Comment 2 John F. Carr 2022-03-17 23:38:33 UTC
Lowering the minimum region size does the trick.  Now my kernel says

hpet0: <High Precision Event Timer> iomem 0xfed00000-0xfed001ff on acpi0
hpet0: vendor 0x1022, rev 0x1, 14318180Hz, 3 timers, legacy route
Timecounter "HPET" frequency 14318180 Hz quality 950
Timecounter "ACPI-fast" frequency 3579545 Hz quality 900

With only 3 timers 512 bytes is enough address space.
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2022-03-18 16:54:41 UTC
The value of 1024 comes directly from the Intel specification, see section 2.1: https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/software-developers-hpet-spec-1-0a.pdf

Some AMD documentation for 16h processors suggests that only three timers are available, and the MMIO space for HPET registers is indeed only 512 bytes in size: https://www.amd.com/system/files/TechDocs/52740_16h_Models_30h-3Fh_BKDG.pdf

Changing the value of HPET_MEM_WIDTH doesn't seem like the right solution: hpet_identify() uses that constant to initialize the memory resource for the hpet driver if it can't get the resource size from acpi.  We could instead modify the check in hpet_attach() to first verify that the resource is large enough to read the capabilities register, then check it again once we have the number of timers.
Comment 4 John F. Carr 2022-03-19 23:40:30 UTC
Created attachment 232585 [details]
a more complicated patch

With this patch the kernel enables HPET on an AMD Opteron x3421 (which worked before) and an AMD EPYC 7402P (which did not work before).
Comment 5 Mark Johnston freebsd_committer freebsd_triage 2022-03-21 16:53:23 UTC
(In reply to John F. Carr from comment #4)
This looks like it'd work, but it's hard for a reader to understand where the value of HPET_MEM_MIN_WIDTH comes from.  Why not make it just large enough to read the general registers (0x100)?
Comment 6 John F. Carr 2022-03-22 20:32:12 UTC
Created attachment 232639 [details]
another patch
Comment 7 Mark Johnston freebsd_committer freebsd_triage 2022-03-24 16:47:13 UTC
(In reply to John F. Carr from comment #6)
Looks ok to me, thanks.
Comment 8 Mark Johnston freebsd_committer freebsd_triage 2022-03-25 16:05:26 UTC
Created attachment 232708 [details]
proposed patch

Here's a copy with the commit log message fleshed out and a style bug fixed.  If you have no objection, I'll commit it.
Comment 9 John F. Carr 2022-03-26 17:12:22 UTC
The latest patch looks good, and with it applied hpet0 is found on my AMD system.
Comment 10 commit-hook freebsd_committer freebsd_triage 2022-03-28 15:25:20 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=964bf2f902c5e05381018532e5d9d456979d4bf7

commit 964bf2f902c5e05381018532e5d9d456979d4bf7
Author:     John F. Carr <jfc@mit.edu>
AuthorDate: 2022-03-19 22:51:43 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-03-28 15:23:45 +0000

    hpet: Allow a MMIO window smaller than 1K

    Some new AMD systems provide a HPET MMIO region smaller than the 1KB
    specified, and a correspondingly small number of timers.  Handle this in
    the HPET driver rather than requiring a 1KB window.  This allows the
    HPET driver to attach on such systems.

    PR:             262638
    Reviewed by:    markj
    MFC after:      1 month

 sys/dev/acpica/acpi_hpet.c | 20 +++++++++++++++++---
 sys/dev/acpica/acpi_hpet.h |  1 +
 2 files changed, 18 insertions(+), 3 deletions(-)
Comment 11 commit-hook freebsd_committer freebsd_triage 2022-04-27 13:47:57 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=972ea9cc57fd9c13c118dab0d8797e6716fdc171

commit 972ea9cc57fd9c13c118dab0d8797e6716fdc171
Author:     John F. Carr <jfc@mit.edu>
AuthorDate: 2022-03-19 22:51:43 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-04-27 13:33:56 +0000

    hpet: Allow a MMIO window smaller than 1K

    Some new AMD systems provide a HPET MMIO region smaller than the 1KB
    specified, and a correspondingly small number of timers.  Handle this in
    the HPET driver rather than requiring a 1KB window.  This allows the
    HPET driver to attach on such systems.

    PR:             262638
    Reviewed by:    markj

    (cherry picked from commit 964bf2f902c5e05381018532e5d9d456979d4bf7)

 sys/dev/acpica/acpi_hpet.c | 20 +++++++++++++++++---
 sys/dev/acpica/acpi_hpet.h |  1 +
 2 files changed, 18 insertions(+), 3 deletions(-)