smbios: handle smbios3 for arm64
ClosedPublic
Actions

Authored by gallatin on Nov 14 2023, 12:40 AM.

Details

Reviewers

allanjude
imp
jhb

Commits

rGba0e4d7971e0: smbios: handle smbios3 for arm64

Summary

Get smbios working on arm64 where it seems to be exclusively smbios version 3.x.

The "interesting" thing here is that the smbios table seems to be RAM in the EFI runtime services table. This makes it owned by "ram0", and not io memory. That prevents bus_alloc_resource() from being able to claim it, since ram0 already owns it. According to @jhb this is working as intended, as bus_alloc_resource() is meant to be used with IO memory, not physical RAM. He suggested I convert the driver to simply use pmap_mapbios(), which I've done.

This is based on initial work by Allan Jude in https://reviews.freebsd.org/D28739 and is a prerequisite for getting IPMI to attach via the SSIF method on arm64. Note that all IPMI devices I've seen on arm64 use the SSIF attachment.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

gallatin requested review of this revision.Nov 14 2023, 12:40 AM

gallatin created this revision.

imp added inline comments.Nov 14 2023, 12:57 AM

sys/dev/smbios/smbios.c
89	Max of the v2 and v3 sizes?
97–102	I'd be tempted to set the map size here too...
108	And here
114	No errors?
115	I'd still che k the signature to see which one you found.
122	I'd be tempted to say it's smbv2 explicitly rather than yhe length assertion
145–146	I think this return is wrong. We are just fixing the length sometimes... but when we don't we still need to add the child.
199	I'd be tempted to store the length in the softc... to limit interface abuse for this rman that might not be quite right for the address we smuggle in.
292	Space before ( This is how you should yell the different cases apart.

gallatin added inline comments.Nov 14 2023, 2:42 PM

sys/dev/smbios/smbios.c
89	Didn't want to do that, because that assumes placement is perfect. Eg, if you have a v2 thats sizeof (*eps) from the top of ram, mapping the v3 size might fail.
97–102	Its set to the default v2 size above.
108	Its set to the default v2 size above.
114	The function returns void.. not sure what you mean.
145–146	This is old code. The only change I made was to avoid leaking the mapped address. I'd prefer not to touch it.
292	No clue what you're trying to say. The style in this file is AFU, and I tried to match it.

gallatin added inline comments.Nov 14 2023, 3:01 PM

sys/dev/smbios/smbios.c
122	The original code did a len check against a magic number that represented the mapping size, i'm doing it "better" by using the map size. There is no point to come here if the length is not equal to the mapping size, because its apparently some kind of fixup for a weird older smbios version.

So there's one, maybe two issues. First issue is the pmap issue I raised in the BIOS case. Second is 'We were told a smbios ptr, but it was bogus' seems to be something we should whine about. The second isn't as important, but will be a silent failure. The other no error cases are for legacy BIOS where SMBIOS might not be there (though the platforms where that is still the case are maybe 25 years old).

sys/dev/smbios/smbios.c
108	fair enough (here and above). I have a conern though about this addr vs others.... This addr is a VA that's passed through BIOS_PADDRTOVADDR already, so it doesn't need the pmap_mapbios(). Will that done below screw things up since KERNBASE is added to the PA?
114	We found an address, but couldn't map it. Shouldn't we report it?
122	OK. That makes sense.
145–146	fair enough.
292	Yea, the style is AFU... Didn't notice the rest of the file was that way.

imp added inline comments.Nov 14 2023, 5:57 PM

sys/dev/smbios/smbios.c
173	The pa here is good for the EFI case, but not the BIOS case.... So the above comment about bios_sigsearch returning a VA is more important here.

I misssed the return((u_int32_t)(uintptr_t)BIOS_VADDRTOPADDR(sp)); at the end of bios_sigsearch, so it does return a pa, so ignore my mismatch comments.

So I'd have done some things differently... but what's there is fine to commit...

This revision is now accepted and ready to land.Nov 14 2023, 6:47 PM

gallatin added inline comments.Nov 14 2023, 8:18 PM

sys/dev/smbios/smbios.c
108	The code for this has not changed with this update. The old code did the same thing, so I think this comment is out of scope for these changes. Note that bios_sigsearch() returns a physical, not virtual, address. return((u_int32_t)(uintptr_t)BIOS_VADDRTOPADDR(sp)); In any case, a single physical address can have multiple virtual mappings, so I don't think it is a problem anyway.
173	No, it returns a VA.
199	That's not a bad idea.