Fix attachment of IPMI driver on ARM64

[allanjude]

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.

[manu]

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]

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]

stand/efi/loader/main.c

1184

But we don't need to loop twice.

[manu]

stand/libsa/smbios.c

108

Same thing here, no need to loop twice.

[scottph]

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]

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]

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]

stand/efi/loader/main.c

1184

That's what I'm suggesting, both the snprintf and smbios_detect should only appear once here.

[jhb]

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]

• 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

[allanjude]

stand/efi/loader/main.c

1184

this way it should skip the 2nd loop if it found something in the first loop

[allanjude]

Repost

[tsoome]

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:)

[manu]

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.

[emaste]

@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?

[manu]

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.

[tsoome]

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?

[manu]

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.

[allanjude]

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 ?

[allanjude]

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.

[imp]

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....

[manu]

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.

[imp]

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.

[allanjude]

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

[allanjude]

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:

https://reviews.freebsd.org/D28737
https://reviews.freebsd.org/D28738
https://reviews.freebsd.org/D28739
https://reviews.freebsd.org/D28740
https://reviews.freebsd.org/D28741
https://reviews.freebsd.org/D28742
https://reviews.freebsd.org/D28743
https://reviews.freebsd.org/D28744
https://reviews.freebsd.org/D28745
https://reviews.freebsd.org/D28746
https://reviews.freebsd.org/D28747
https://reviews.freebsd.org/D28748
https://reviews.freebsd.org/D28749
https://reviews.freebsd.org/D28750
https://reviews.freebsd.org/D28751

[allanjude]

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.

[imp]

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.

[tuexen]

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?

[imp]

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