Page MenuHomeFreeBSD

Fix attachment of IPMI driver on ARM64
Needs RevisionPublic

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

Details

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 37074
Build 33963: arc lint + arc unit

Event Timeline

stand/efi/loader/main.c
1183

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 ?

stand/efi/loader/main.c
1183

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

stand/efi/loader/main.c
1183

But we don't need to loop twice.

stand/libsa/smbios.c
108

Same thing here, no need to loop twice.

stand/efi/loader/main.c
1183

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.

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.

stand/efi/loader/main.c
1183

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.

stand/efi/loader/main.c
1183

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
499
  • 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
stand/efi/loader/main.c
1183

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

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?

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

@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 requested changes to this revision.Feb 17 2021, 6:50 PM
In D28707#643373, @manu 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#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 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:

@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#643382, @manu 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

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