On Fri, Jan 08, 2021 at 11:48:33AM -0600, Scott Cheloha wrote:
> On Fri, Jan 08, 2021 at 05:41:24PM +0100, Mark Kettenis wrote:
> > > Date: Fri, 8 Jan 2021 10:27:38 -0600
> > > From: Scott Cheloha <[email protected]>
> > >
> > > On Wed, Jan 06, 2021 at 11:26:27PM +0100, Mark Kettenis wrote:
> > > > > Date: Wed, 6 Jan 2021 16:16:27 -0600
> > > > > From: Scott Cheloha <[email protected]>
> > > > >
> > > > > On Wed, Jan 06, 2021 at 12:16:13PM -0600, Scott Cheloha wrote:
> > > > > > As mentioned in a prior mail, tpm(4) is the last user of tvtohz(9)
> > > > > > in
> > > > > > the tree.
> > > > > >
> > > > > > However, we don't need to use tvtohz(9) in tpm(4) at all.
> > > > > > Converting
> > > > > > from milliseconds to ticks is trivial. Using an intermediary
> > > > > > timeval
> > > > > > is just pointless indirection.
> > > > > >
> > > > > > With this committed I will be able to remove both tvtohz(9) and
> > > > > > tstohz(9) from the tree in a subsequent patch.
> > > > >
> > > > > Whoops, made a mistake in the prior diff.
> > > > >
> > > > > We want to MAX() the result up to 1 to avoid dividing to zero and
> > > > > blocking indefinitely in e.g. tsleep(9). Previous diff MIN'd the
> > > > > result down to 1, which is very wrong.
> > > >
> > > > To be honest I'd just zap the function completely and instead simply do:
> > > >
> > > > to = TPM_ACCESS_TMO / 10;
> > > >
> > > > and
> > > >
> > > > to = TPM_BURST_TMO / 10;
> > >
> > > That won't work on custom kernels where HZ is not 100, no?
> >
> > HZ is irrelevant here. These TPM_XXX_TMO defines specify a timeout in
> > microseconds and DELAY(10) just delays for 10us. So you just need to
> > figure out how many times you need to do the 10us delay to reach the
> > timeout specified by TPM_XXX_TMO.
>
> Whoops, yes, you're right.
>
> > Also, this driver is only supported on amd64/i386 at this point (but
> > might show up on arm64 at some point).
> >
> > > > There is no magic happening here. The code is just doing a hardware
> > > > register poll loop in 10us steps.
> > > >
> > > > Hmm, actually it seems the code is broken and uses steps of 10
> > > > microseconds instead of milliseconds. So instead it should probably
> > > > use:
> > > >
> > > > to = TPM_ACCESS_TMO * 100;
> > > >
> > > > and
> > > >
> > > > to = TPM_BURST_TMO * 100;
> > >
> > > This problem came up in a different thread:
> > >
> > > https://marc.info/?l=openbsd-tech&m=160833962329381&w=2
> > >
> > > jcs@ said, and I paraphrase, "tpm(4) sucks, we should merge NetBSD's
> > > latest code, which is much nicer now."
> > >
> > > That sounds like a much taller order than I can fill, so I'm just
> > > trying to remove the tvtohz(9) call without changing any behavior,
> > > even if the behavior looks wrong (like using the wrong units).
> > >
> > > I don't even have a tpm(4) device to test. Until I have one I'm
> > > reluctant to do things like expanding the delay time in these loops.
> > > As of now the driver "works" for some subset of people, which is not
> > > nothing.
> >
> > But having illogical code is a problem as well. So I think we should
> > at least attempt to fix it the right way. All it takes is to find
> > someone with a laptop where the driver attaches and have them
> > suspend/resume it.
>
> We can try that, sure. Here's the patch:
>
> - Remove tpm_tmotohz().
>
> - tpm_waitfor() does DELAY(1), so in that case convert from
> milliseconds to microseconds. Note in the function argument
> that we expect milliseconds, not "tries".
>
> - In the other cases we do DELAY(10), so we only multiply by 100.
> Leave a comment explaining what we're doing.
>
> Unsure who can test. We need a suspend/resume test with this patch.
> Probably an older machine. Newer machines tend to have TPM 2.0 chips.
My x1 gen 2 seems to have a TPM 1.2 chip. Or can emulate one?
The bios is confusing. I had it disabled until now.
It seems to be able to suspend/resume just fine with this.
It shows up like this:
tpm0 at acpi0 TPM_ addr 0xfed40000/0x5000, device 0x0000104a rev 0x4e
No idea what this does, I'm going to disable it again, but let me know
if you want further tests.
OpenBSD 6.8-current (GENERIC.MP) #103: Fri Jan 8 20:11:51 CET 2021
[email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 8233394176 (7851MB)
avail mem = 7968542720 (7599MB)
random: good seed from bootblocks
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xdcd3d000 (61 entries)
bios0: vendor LENOVO version "GRET40WW (1.17 )" date 09/02/2014
bios0: LENOVO 20A7006VUS
acpi0 at bios0: ACPI 5.0
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SLIC DBGP ECDT HPET APIC MCFG SSDT SSDT SSDT SSDT SSDT
SSDT SSDT PCCT SSDT TCPA UEFI MSDM ASF! BATB FPDT UEFI DMAR
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP2(S4) XHCI(S3) EHC1(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpihpet0 at acpi0: 14318179 Hz
acpimadt0 at acpi0 addr 0xfee00000: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.27 MHz, 06-45-01
cpu0:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 99MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.16 MHz, 06-45-01
cpu1:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 1, core 0, package 0
cpu2 at mainbus0: apid 2 (application processor)
cpu2: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.16 MHz, 06-45-01
cpu2:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN
cpu2: 256KB 64b/line 8-way L2 cache
cpu2: smt 0, core 1, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.16 MHz, 06-45-01
cpu3:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN
cpu3: 256KB 64b/line 8-way L2 cache
cpu3: smt 1, core 1, package 0
ioapic0 at mainbus0: apid 2 pa 0xfec00000, version 20, 40 pins
acpimcfg0 at acpi0
acpimcfg0: addr 0xf8000000, bus 0-63
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (PEG_)
acpiprt2 at acpi0: bus 2 (EXP1)
acpiprt3 at acpi0: bus 3 (EXP2)
acpiprt4 at acpi0: bus -1 (EXP3)
dwiic0 at acpi0 I2C1 addr 0xfe105000/0x1000 irq 7
iic0 at dwiic0
"CPLM3218" at iic0 addr 0x48 not configured
acpibtn0 at acpi0: LID_
acpibtn1 at acpi0: SLPB
acpipci0 at acpi0 PCI0: 0x00000000 0x00000011 0x00000001
acpicmos0 at acpi0
tpm0 at acpi0 TPM_ addr 0xfed40000/0x5000, device 0x0000104a rev 0x4e
acpibat0 at acpi0: BAT0 model "45N1703" serial 3191 type LiP oem "SMP"
acpiac0 at acpi0: AC unit offline
acpithinkpad0 at acpi0: version 2.0
"PNP0C14" at acpi0 not configured
"PNP0C14" at acpi0 not configured
"PNP0C14" at acpi0 not configured
"INT340F" at acpi0 not configured
acpicpu0 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpicpu1 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpicpu2 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpicpu3 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33),
C1(1000@1 mwait.1), PSS
acpipwrres0 at acpi0: PUBS, resource for XHCI, EHC1
acpitz0 at acpi0: critical temperature is 200 degC
acpivideo0 at acpi0: VID_
acpivout0 at acpivideo0: LCD0
acpivideo1 at acpi0: VID_
cpu0: using VERW MDS workaround (except on vmm entry)
cpu0: Enhanced SpeedStep 798 MHz: speeds: 2701, 2700, 2600, 2400, 2300, 2100,
2000, 1800, 1700, 1600, 1400, 1300, 1100, 1000, 800, 756 MHz
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "Intel Core 4G Host" rev 0x0b
inteldrm0 at pci0 dev 2 function 0 "Intel HD Graphics" rev 0x0b
drm0 at inteldrm0
inteldrm0: msi, HASWELL, gen 7
azalia0 at pci0 dev 3 function 0 "Intel Core 4G HD Audio" rev 0x0b: msi
azalia0: No codecs found
xhci0 at pci0 dev 20 function 0 "Intel 8 Series xHCI" rev 0x04: msi, xHCI 1.0
usb0 at xhci0: USB revision 3.0
uhub0 at usb0 configuration 1 interface 0 "Intel xHCI root hub" rev 3.00/1.00
addr 1
"Intel 8 Series MEI" rev 0x04 at pci0 dev 22 function 0 not configured
puc0 at pci0 dev 22 function 3 "Intel 8 Series KT" rev 0x04: ports: 16 com
com4 at puc0 port 0 apic 2 int 17: ns16550a, 16 byte fifo
com4: probed fifo depth: 0 bytes
em0 at pci0 dev 25 function 0 "Intel I218-LM" rev 0x04: msi, address
54:ee:75:3d:fb:31
azalia1 at pci0 dev 27 function 0 "Intel 8 Series HD Audio" rev 0x04: msi
azalia1: codecs: Realtek ALC292
audio0 at azalia1
ppb0 at pci0 dev 28 function 0 "Intel 8 Series PCIE" rev 0xe4: msi
pci1 at ppb0 bus 2
ppb1 at pci0 dev 28 function 1 "Intel 8 Series PCIE" rev 0xe4: msi
pci2 at ppb1 bus 3
iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless AC 7260" rev 0x83, msi
ehci0 at pci0 dev 29 function 0 "Intel 8 Series USB" rev 0x04: apic 2 int 23
usb1 at ehci0: USB revision 2.0
uhub1 at usb1 configuration 1 interface 0 "Intel EHCI root hub" rev 2.00/1.00
addr 1
pcib0 at pci0 dev 31 function 0 "Intel 8 Series LPC" rev 0x04
ahci0 at pci0 dev 31 function 2 "Intel 8 Series AHCI" rev 0x04: msi, AHCI 1.3
ahci0: port 0: 6.0Gb/s
scsibus1 at ahci0: 32 targets
sd0 at scsibus1 targ 0 lun 0: <ATA, TOSHIBA THNSNJ51, JULA> naa.500080dc001bf1e2
sd0: 488386MB, 512 bytes/sector, 1000215216 sectors, thin
ichiic0 at pci0 dev 31 function 3 "Intel 8 Series SMBus" rev 0x04: apic 2 int 18
iic1 at ichiic0
isa0 at pcib0
isadma0 at isa0
pckbc0 at isa0 port 0x60/5 irq 1 irq 12
pckbd0 at pckbc0 (kbd slot)
wskbd0 at pckbd0: console keyboard
pms0 at pckbc0 (aux slot)
wsmouse0 at pms0 mux 0
wsmouse1 at pms0 mux 0
pms0: Synaptics clickpad, firmware 8.1, 0x1e2b1 0x940300 0x2d9240 0xd001a3
0x12e800
pcppi0 at isa0 port 0x61
spkr0 at pcppi0
vmm0 at mainbus0: VMX/EPT
umb0 at uhub0 port 4 configuration 1 interface 0 "Sierra Wireless Inc. Sierra
Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2
umodem0 at uhub0 port 4 configuration 1 interface 2 "Sierra Wireless Inc.
Sierra Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2
umodem0: data interface 3, has no CM over data, has break
umodem0: status change notification available
ucom0 at umodem0
uhub2 at uhub1 port 1 configuration 1 interface 0 "Intel Rate Matching Hub" rev
2.00/0.04 addr 2
uhidev0 at uhub2 port 5 configuration 1 interface 0 "ELAN Touchscreen" rev
2.00/0.12 addr 3
uhidev0: iclass 3/0, 68 report ids
ums0 at uhidev0 reportid 1: 1 button, tip
wsmouse2 at ums0 mux 0
uhid0 at uhidev0 reportid 2: input=64, output=0, feature=0
uhid1 at uhidev0 reportid 3: input=0, output=31, feature=0
uhid2 at uhidev0 reportid 4: input=19, output=0, feature=0
uhid3 at uhidev0 reportid 10: input=0, output=0, feature=1
ums1 at uhidev0 reportid 68
ums1: mouse has no X report
uvideo0 at uhub2 port 8 configuration 1 interface 0 "Chicony Electronics
Co.,Ltd. Integrated Camera" rev 2.00/25.09 addr 4
video0 at uvideo0
vscsi0 at root
scsibus2 at vscsi0: 256 targets
softraid0 at root
scsibus3 at softraid0: 256 targets
sd1 at scsibus3 targ 1 lun 0: <OPENBSD, SR CRYPTO, 006>
sd1: 488381MB, 512 bytes/sector, 1000206308 sectors
root on sd1a (f3054894d8af4b99.a) swap on sd1b dump on sd1b
inteldrm0: 2560x1440, 32bpp
wsdisplay0 at inteldrm0 mux 1: console (std, vt100 emulation), using wskbd0
wsdisplay0: screen 1-5 added (std, vt100 emulation)
iwm0: hw rev 0x140, fw ver 17.3216344376.0, address 5c:c5:d4:63:b3:d9
>
> Index: tpm.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/acpi/tpm.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 tpm.c
> --- tpm.c 22 May 2020 10:16:37 -0000 1.10
> +++ tpm.c 8 Jan 2021 17:43:35 -0000
> @@ -158,7 +158,6 @@ int tpm_request_locality(struct tpm_soft
> void tpm_release_locality(struct tpm_softc *);
> int tpm_getburst(struct tpm_softc *);
> uint8_t tpm_status(struct tpm_softc *);
> -int tpm_tmotohz(int);
>
> struct cfattach tpm_ca = {
> sizeof(struct tpm_softc),
> @@ -385,7 +384,7 @@ tpm_request_locality(struct tpm_softc *s
> bus_space_write_1(sc->sc_bt, sc->sc_bh, TPM_ACCESS,
> TPM_ACCESS_REQUEST_USE);
>
> - to = tpm_tmotohz(TPM_ACCESS_TMO);
> + to = TPM_ACCESS_TMO * 100; /* steps of 10 microseconds */
>
> while ((r = bus_space_read_1(sc->sc_bt, sc->sc_bh, TPM_ACCESS) &
> (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) !=
> @@ -420,7 +419,7 @@ tpm_getburst(struct tpm_softc *sc)
> {
> int burst, burst2, to;
>
> - to = tpm_tmotohz(TPM_BURST_TMO);
> + to = TPM_BURST_TMO * 100; /* steps of 10 microseconds */
>
> burst = 0;
> while (burst == 0 && to--) {
> @@ -453,29 +452,21 @@ tpm_status(struct tpm_softc *sc)
> }
>
> int
> -tpm_tmotohz(int tmo)
> -{
> - struct timeval tv;
> -
> - tv.tv_sec = tmo / 1000;
> - tv.tv_usec = 1000 * (tmo % 1000);
> -
> - return tvtohz(&tv);
> -}
> -
> -int
> -tpm_waitfor(struct tpm_softc *sc, uint8_t mask, int tries)
> +tpm_waitfor(struct tpm_softc *sc, uint8_t mask, int msecs)
> {
> + int usecs;
> uint8_t status;
>
> + usecs = msecs * 1000;
> +
> while (((status = tpm_status(sc)) & mask) != mask) {
> - if (tries == 0) {
> + if (usecs == 0) {
> DPRINTF(("%s: %s: timed out, status 0x%x != 0x%x\n",
> sc->sc_dev.dv_xname, __func__, status, mask));
> return status;
> }
>
> - tries--;
> + usecs--;
> DELAY(1);
> }
>
>
--
I'm not entirely sure you are real.
