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 37057
Build 33946: 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
1176	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
1176	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
1176	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
1176	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
1176	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
1176	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
1176	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–101	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

11 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
24bf69c32b08	10e730e1182d	addd97ae8f7b	Allan Jude	ipmi_smbios: remove unused smbios_cksum function (Show More…)	Feb 16 2021, 5:02 PM
addd97ae8f7b	6e116a55a2a5	b5755e4c8af7	Allan Jude	ipmi: on aarch64, load dependancies to increase chance of finding ipmi device (Show More…)	Feb 16 2021, 4:56 PM
b5755e4c8af7	b06ab0aaab41	424b60ecab34	Allan Jude	smbios: update makefile for new location of smbios.c (Show More…)	Feb 13 2021, 12:18 AM
424b60ecab34	44f8542a1743	9283fe7feb1b	D Scott Phillips	ipmi_ssif: Fix inverted for the end of multi-part reads (Show More…)	Feb 12 2021, 1:29 AM
9283fe7feb1b	a349e71168a6	e1b2fd2f108d	D Scott Phillips	ig4(4): Use calculated value when sda_hold is zero in ACPI (Show More…)	Feb 12 2021, 1:24 AM
e1b2fd2f108d	c3ae51dcf310	95bb9c83d645	D Scott Phillips	ig4(4): Increase timeout to about 1 second (Show More…)	Feb 12 2021, 1:21 AM
95bb9c83d645	416a7381fc57	4fb39048fe3d	D Scott Phillips	ig4(4): Add an EMAG device type (Show More…)	Feb 12 2021, 1:19 AM
4fb39048fe3d	b80683e63975	e92a7ff39e3d	D Scott Phillips	ipmi: Add smbus and pci ipmi types to aarch64 build (Show More…)	Feb 10 2021, 7:43 AM
e92a7ff39e3d	05d3a7b4270a	d8bccc2f6b05	D Scott Phillips	Enable smbios in ARM64 GENERIC (Show More…)	Feb 9 2021, 9:59 PM
d8bccc2f6b05	4051d0cca629	18ccfc87ed77	D Scott Phillips	ipmi_smbios: Deduplicate smbios entry point discovery logic (Show More…)	Feb 10 2021, 7:35 AM
18ccfc87ed77	44edd6ce4eb7	d1f00db9a2d6	D Scott Phillips	iicsmb: Request the bus recursively in bread() (Show More…)	Feb 10 2021, 6:54 AM
d1f00db9a2d6	64254fe491fa	e464b13b13cc	D Scott Phillips	acpi: Add workaround for Altra I2C memory resource (Show More…)	Feb 10 2021, 6:51 AM
e464b13b13cc	77d556c3e374	d88d0b61af32	D Scott Phillips	smbios: Use loader provided adddress for SMBIOS entry point (Show More…)	Feb 9 2021, 9:57 PM
d88d0b61af32	72867c2b02d7	71644c3701c0	D Scott Phillips	smbios: Support (and prefer) the SMBIOS 64-bit Entry Point Structure (Show More…)	Feb 9 2021, 9:49 PM
71644c3701c0	a09beb4a249a	2fcb418bfb62	D Scott Phillips	smbios: Move smbios driver out from x86 machdep code (Show More…)	Feb 9 2021, 9:48 PM
2fcb418bfb62	e196b6066fb3	97527e9c4fd3	D Scott Phillips	stand: Support (and prefer) the SMBIOS 64-bit Entry Point Structure (Show More…)	Feb 9 2021, 9:09 PM