Paths

Table of Contentst

Fix attachment of IPMI driver on ARM64
Needs RevisionPublic
Actions

Authored by allanjude on Feb 16 2021, 5:06 PM.

Details

Reviewers

emaste
mhorne
scottph
tsoome
imp
jhb
manu
gallatin
andrew

Summary

stand: Support (and prefer) the SMBIOS 64-bit Entry Point Structure
smbios: Move smbios driver out from x86 machdep code
smbios: Support (and prefer) the SMBIOS 64-bit Entry Point Structure
smbios: Use loader provided adddress for SMBIOS entry point
acpi: Add workaround for Altra I2C memory resource
iicsmb: Request the bus recursively in bread()
ipmi_smbios: Deduplicate smbios entry point discovery logic
Enable smbios in ARM64 GENERIC
ipmi: Add smbus and pci ipmi types to aarch64 build
ig4(4): Add an EMAG device type
ig4(4): Increase timeout to about 1 second
ig4(4): Use calculated value when sda_hold is zero in ACPI
ipmi_ssif: Fix inverted for the end of multi-part reads
smbios: update makefile for new location of smbios.c
ipmi: on aarch64, load dependancies to increase chance of finding ipmi device
ipmi_smbios: remove unused smbios_cksum function

Sponsored By: Ampere Computing LLC
Submitted By: Klara Inc.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 37074
Build 33963: arc lint + arc unit

Event Timeline

allanjude created this revision.Feb 16 2021, 5:06 PM

Herald added a reviewer: manu. · View Herald TranscriptFeb 16 2021, 5:06 PM

Herald added subscribers: bcran, rpokala, andrew. · View Herald Transcript

allanjude requested review of this revision.Feb 16 2021, 5:06 PM

Harbormaster completed remote builds in B37057: Diff 84022.Feb 16 2021, 5:06 PM

allanjude added a reviewer: gallatin.Feb 16 2021, 5:07 PM

manu added inline comments.Feb 16 2021, 5:32 PM

stand/efi/loader/main.c
1184	Why is another loop needed ? Also this drop the guid assignment.
sys/dev/ichiic/ig4_iic.c
273	Why ?
sys/dev/iicbus/iicsmb.c
458	Why ?

allanjude added inline comments.Feb 16 2021, 6:33 PM

stand/efi/loader/main.c
1184	The new loop checks &smbios3, and the existing loop checks &smbios (v2). Although it does look like a mistake that it doesn't refresh guid in the 2nd loop. @scottph

manu added inline comments.Feb 16 2021, 6:40 PM

stand/efi/loader/main.c
1184	But we don't need to loop twice.

manu added inline comments.Feb 16 2021, 6:47 PM

stand/libsa/smbios.c
108	Same thing here, no need to loop twice.

scottph added inline comments.Feb 16 2021, 6:50 PM

stand/efi/loader/main.c
1184	The idea is to look through the whole list for smbios3 first, and then look for smbios after that. You're right Allan, the second loop got broken in this patch.
sys/dev/ichiic/ig4_iic.c
273	Per the i2c spec, a slave device can stretch SCL idefinitely, so 25ms is a bit arbitrary in general. smbus does specify an optional timeout recovery mechanism to be done at about 25~35ms, but the IPMI SSIF spec says that BMCs don't have any obligation to implement that. The BMC on altra seems to mostly respond within 25ms, but occasionally will stretch SCL for ~300 msec. Also, the count_us mechanism seems to actually timeout around 25% earlier than it would claim (timeout really happening around 19ms instead of 25ms).
sys/dev/iicbus/iicsmb.c
458	ipmi_ssif will `smbus_request_bus` to do multiple requests smbus requests (which requests the iicbus), and then here in bread we also need to request the bus because bread takes multiple transactions. This causes deadlock as it's waiting for the bus it already has without IIC_RECURSIVE.

manu added inline comments.Feb 16 2021, 6:59 PM

sys/dev/ichiic/ig4_iic.c
273	That should be in the commit then.
sys/dev/iicbus/iicsmb.c
458	That should be in the commit then.

andrew added inline comments.Feb 16 2021, 7:39 PM

stand/efi/loader/main.c
1184	Looping twice is the easiest way to search for two entries as this code is doing. Alternatively we could set a variable to signal when each is found to only set the variables once.
stand/libsa/smbios.c
108	That will be needed if we need to search for the smbios3 signature first, then the smbios signature.

manu added inline comments.Feb 16 2021, 7:43 PM

stand/efi/loader/main.c
1184	That's what I'm suggesting, both the snprintf and smbios_detect should only appear once here.

In the future, could you upload a branch like this as a series of reviews, e.g.:

git arc create -l main..mybranch

sys/dev/acpica/acpi_resource.c
500

allanjude marked 14 inline comments as done.Feb 16 2021, 9:55 PM

stand: Support (and prefer) the SMBIOS 64-bit Entry Point Structure
smbios: Move smbios driver out from x86 machdep code
smbios: Support (and prefer) the SMBIOS 64-bit Entry Point Structure
smbios: Use loader provided adddress for SMBIOS entry point
acpi: Add workaround for Altra I2C memory resource
iicsmb: Request the bus recursively in bread()
ipmi_smbios: Deduplicate smbios entry point discovery logic
Enable smbios in ARM64 GENERIC
ipmi: Add smbus and pci ipmi types to aarch64 build
ig4(4): Add an EMAG device type
ig4(4): Increase timeout to about 1 second
ig4(4): Use calculated value when sda_hold is zero in ACPI
ipmi_ssif: Fix inverted for the end of multi-part reads
smbios: update makefile for new location of smbios.c
ipmi: on aarch64, load dependancies to increase chance of finding ipmi device
ipmi_smbios: remove unused smbios_cksum function
[squashme] Address feedback from manu and andrew

Herald added subscribers: melifaro, ae. · View Herald TranscriptFeb 16 2021, 10:33 PM

Harbormaster completed remote builds in B37073: Diff 84048.Feb 16 2021, 10:34 PM

allanjude added inline comments.Feb 16 2021, 10:35 PM

stand/efi/loader/main.c
1184	this way it should skip the 2nd loop if it found something in the first loop

Repost

Harbormaster completed remote builds in B37074: Diff 84049.Feb 16 2021, 10:36 PM

The loader part seems OK for me (I did the SMB3 support in illumos loader), the kernel part seems to make sense, but I am not familiar with those bits:)

Looks like I don't have machine with smbios table but I can confirm that at least this doesn't break anything on honeycomb.
You can add my reviewed-by for the stand patches.
The rest looks ok but I don't know enough the kernel side to validate it.

@allanjude these originated as a number of commits, right? Does it make sense to start committing earlier patches that have been reviewed (e.g. stand bits) and rebase this review?

In D28707#643366, @emaste wrote:

@allanjude these originated as a number of commits, right? Does it make sense to start committing earlier patches that have been reviewed (e.g. stand bits) and rebase this review?

stand patches can't go in without the kernel ones.
If we find a smbios v3 table we don't set the kenv of the v2.1 one so kernel won't work.

In D28707#643373, @manu wrote:

In D28707#643366, @emaste wrote:

@allanjude these originated as a number of commits, right? Does it make sense to start committing earlier patches that have been reviewed (e.g. stand bits) and rebase this review?

stand patches can't go in without the kernel ones.
If we find a smbios v3 table we don't set the kenv of the v2.1 one so kernel won't work.

Ou, but that would be major pain - it would be bad day for anyone trying to boot older kernel... so we should provide both to let older kernel to work?

In D28707#643380, @tsoome wrote:

In D28707#643373, @manu wrote:

In D28707#643366, @emaste wrote:

@allanjude these originated as a number of commits, right? Does it make sense to start committing earlier patches that have been reviewed (e.g. stand bits) and rebase this review?

stand patches can't go in without the kernel ones.
If we find a smbios v3 table we don't set the kenv of the v2.1 one so kernel won't work.

Ou, but that would be major pain - it would be bad day for anyone trying to boot older kernel... so we should provide both to let older kernel to work?

Yeah I think so, I think we should just export every smbios table in loader and let kernel choose.

This revision now requires changes to proceed.Feb 17 2021, 6:50 PM

In D28707#643382, @manu wrote:

In D28707#643380, @tsoome wrote:

In D28707#643373, @manu wrote:

stand patches can't go in without the kernel ones.
If we find a smbios v3 table we don't set the kenv of the v2.1 one so kernel won't work.

Ou, but that would be major pain - it would be bad day for anyone trying to boot older kernel... so we should provide both to let older kernel to work?

Yeah I think so, I think we should just export every smbios table in loader and let kernel choose.

So we want to revert back to always having both loops, and running both snprintf()/setenv()'s ?

In D28707#643366, @emaste wrote:

@allanjude these originated as a number of commits, right? Does it make sense to start committing earlier patches that have been reviewed (e.g. stand bits) and rebase this review?

It is still separate commits, but it might be possible to land some of them sooner, sure. Let me extract some of them into separate reviews.

In D28707#643373, @manu wrote:

In D28707#643366, @emaste wrote:

@allanjude these originated as a number of commits, right? Does it make sense to start committing earlier patches that have been reviewed (e.g. stand bits) and rebase this review?

stand patches can't go in without the kernel ones.
If we find a smbios v3 table we don't set the kenv of the v2.1 one so kernel won't work.

We also have a machine here that won't boot with the patches applied. Please don't commit until that gets worked out....

In D28707#643456, @allanjude wrote:

In D28707#643382, @manu wrote:

In D28707#643380, @tsoome wrote:

In D28707#643373, @manu wrote:

stand patches can't go in without the kernel ones.
If we find a smbios v3 table we don't set the kenv of the v2.1 one so kernel won't work.

Ou, but that would be major pain - it would be bad day for anyone trying to boot older kernel... so we should provide both to let older kernel to work?

Yeah I think so, I think we should just export every smbios table in loader and let kernel choose.

So we want to revert back to always having both loops, and running both snprintf()/setenv()'s ?

No, having only one loop that set both kenv is what we need.

This also has a number of intermingled changes that look like they'd be better off as separate changes (such as the copy to sys/dev/smbios being separate from the rest). Please consider breaking this down into smaller chunks. IT would also make this easier to review, since it's on the edge of being too large for phab to cope well...

sys/dev/acpica/acpi_resource.c
502	Is there some way we can limit this to known bad boards? We're starting to get rather a large number of these exceptions and have started having cases where early hardware exception causes actual bugs in real shipping hardware....
sys/dev/ichiic/ig4_acpi.c
92	Is there some way this can be part of a table of hids to matchs? that makes the device play nicer with devmatch.

No, having only one loop that set both kenv is what we need.

I've incorporated this change, and posted it separately: https://reviews.freebsd.org/D28737

Ok, I think I've gotten a handle on the git-arc tool.

I've squashed the fixups into Scott's original commits, and created separate reviews. These reviews should have already addressed the feedback given here, except for Warner's new comments:

allanjude mentioned this in D28737: stand: Support (and prefer) the SMBIOS 64-bit Entry Point Structure.Feb 17 2021, 9:41 PM

allanjude mentioned this in D28741: acpi: Add workaround for Altra I2C memory resource.Feb 17 2021, 9:44 PM

allanjude mentioned this in D28746: ig4(4): Add an EMAG device type.

In D28707#643461, @imp wrote:

We also have a machine here that won't boot with the patches applied. Please don't commit until that gets worked out....

@gallatin reports that this set of changes cherry-picked from my tree yesterday, does not hang on boot anymore.

In D28707#643659, @allanjude wrote:

In D28707#643461, @imp wrote:

We also have a machine here that won't boot with the patches applied. Please don't commit until that gets worked out....

@gallatin reports that this set of changes cherry-picked from my tree yesterday, does not hang on boot anymore.

Great, then consider obj objection satisfied.

And thanks for the split.

jkim mentioned this in D28743: ipmi_smbios: Deduplicate smbios entry point discovery logic.Feb 23 2021, 10:18 PM

Is there some interest in getting IPMI working on arm64 machines? When testing FreeBSD current on two Ampere systems it does not work. Some of the smaller patches are not yet committed. Is there an intention to get this working?

Herald added a reviewer: andrew. · View Herald TranscriptJun 13 2024, 12:28 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a subscriber: dab. · View Herald Transcript

In D28707#1039838, @tuexen wrote:

Is there some interest in getting IPMI working on arm64 machines? When testing FreeBSD current on two Ampere systems it does not work. Some of the smaller patches are not yet committed. Is there an intention to get this working?

It kinda works... I have an arm machine that needs this, so will be checking each of the differentials in the split-out reviews

Revision Contents
Changeset List

Path

Size

stand/

efi/

loader/

main.c

25 lines

libsa/

smbios.c

60 lines

sys/

amd64/

conf/

GENERIC

1 line

MINIMAL

1 line

include/

pc/

bios.h

32 lines

arm64/

conf/

GENERIC

1 line

conf/

files

1 line

files.x86

1 line

dev/

acpica/

acpi_resource.c

7 lines

ichiic/

ig4_acpi.c

12 lines

ig4_iic.c

15 lines

ig4_var.h

1 line

iicbus/

iicsmb.c

2 lines

ipmi/

1 line

1 line

66 lines

5 lines

2 lines

	dev/	smbios/	smbios.h/
	amd64/	include/	pc/


	bios.h

80 lines

	dev/	smbios/
	x86/	bios/

smbios.c

134 lines

i386/

conf/

GENERIC

1 line

MINIMAL

1 line

include/

pc/

bios.h

32 lines

modules/

bios/

smbios/

Makefile

2 lines

ipmi/

Makefile

11 lines

x86/

bios/

smbios.c

Commit	Tree	Parents	Author	Summary	Date
51cc7369d642	0950fd07534c	1a3d9184c27d	Allan Jude	[squashme] Address feedback from manu and andrew	Feb 16 2021, 9:44 PM
1a3d9184c27d	379959e3ec3e	36439bf4097e	Allan Jude	ipmi_smbios: remove unused smbios_cksum function (Show More…)	Feb 16 2021, 5:02 PM
36439bf4097e	5bb2f1feb2c5	f8ad49320212	Allan Jude	ipmi: on aarch64, load dependancies to increase chance of finding ipmi device (Show More…)	Feb 16 2021, 4:56 PM
f8ad49320212	644130c73252	7717d7c5f17c	Allan Jude	smbios: update makefile for new location of smbios.c (Show More…)	Feb 13 2021, 12:18 AM
7717d7c5f17c	356d946e57e2	ff6bcb9553ed	D Scott Phillips	ipmi_ssif: Fix inverted for the end of multi-part reads (Show More…)	Feb 12 2021, 1:29 AM
ff6bcb9553ed	725a938536c2	f2f3b7399bea	D Scott Phillips	ig4(4): Use calculated value when sda_hold is zero in ACPI (Show More…)	Feb 12 2021, 1:24 AM
f2f3b7399bea	53bf90b6e30c	169ff7cbfbcc	D Scott Phillips	ig4(4): Increase timeout to about 1 second (Show More…)	Feb 12 2021, 1:21 AM
169ff7cbfbcc	1b9baebe86a6	eebffc396e54	D Scott Phillips	ig4(4): Add an EMAG device type (Show More…)	Feb 12 2021, 1:19 AM
eebffc396e54	21ee8b7eac6a	f13b0c3aff47	D Scott Phillips	ipmi: Add smbus and pci ipmi types to aarch64 build (Show More…)	Feb 10 2021, 7:43 AM
f13b0c3aff47	544d738e0953	037dddf0324f	D Scott Phillips	Enable smbios in ARM64 GENERIC (Show More…)	Feb 9 2021, 9:59 PM
037dddf0324f	cc6414873027	91d7d3111285	D Scott Phillips	ipmi_smbios: Deduplicate smbios entry point discovery logic (Show More…)	Feb 10 2021, 7:35 AM
91d7d3111285	0bf91dea0c10	7ba4fe0f3ad6	D Scott Phillips	iicsmb: Request the bus recursively in bread() (Show More…)	Feb 10 2021, 6:54 AM
7ba4fe0f3ad6	4d38c415fa55	a2bb3b5a7910	D Scott Phillips	acpi: Add workaround for Altra I2C memory resource (Show More…)	Feb 10 2021, 6:51 AM
a2bb3b5a7910	2937936f4fb9	20efb782590e	D Scott Phillips	smbios: Use loader provided adddress for SMBIOS entry point (Show More…)	Feb 9 2021, 9:57 PM
20efb782590e	b00e755698bc	69619c0ca2ff	D Scott Phillips	smbios: Support (and prefer) the SMBIOS 64-bit Entry Point Structure (Show More…)	Feb 9 2021, 9:49 PM
69619c0ca2ff	a477af2369a4	2de2a07b0e8a	D Scott Phillips	smbios: Move smbios driver out from x86 machdep code (Show More…)	Feb 9 2021, 9:48 PM
2de2a07b0e8a	3aa8a5fcc763	2fe5a79425c7	D Scott Phillips	stand: Support (and prefer) the SMBIOS 64-bit Entry Point Structure (Show More…)	Feb 9 2021, 9:09 PM