Messages in this thread | | | Date | Fri, 29 Jul 2022 21:23:08 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present() |
| |
On Wed, Jul 27, 2022 at 10:25:04AM +0200, Jean Delvare wrote: > On Tue, 26 Jul 2022 12:43:29 +0300, Andy Shevchenko wrote: > > The byte at offset 6 represent length. Don't take it and drop it immediately > > by using proper accessor, i.e. get_unaligned_be24(). > > The subject sounds like you are fixing a bug, while this is only, at > best, a minor optimization.
I don't know how to improve it, but it kinda a bug in a logic for non-prepared reader, esp. as specification suggests different thing about version offset.
...
> > - dmi_ver = get_unaligned_be32(buf + 6) & 0xFFFFFF; > > + dmi_ver = get_unaligned_be24(buf + 7);
> I admit I did not know about get_unaligned_be24(). While I agree that > it makes the source code look better, one downside is that it actually > increases the binary size on x86_64. The reason is that > get_unaligned_be32() is optimized by assembly instruction bswapl, while > get_unaligned_be24() is not. Situation appears to be the same on ia64 > and arm. Only arm64 would apparently benefit from your proposed > change.
Good to know! But here it's not a hot path, right?
> I'm not too sure what is preferred in such situations.
For cold paths I think the correctness prevail the performance.
Alternatively we might add a comment in the code explaining the trick, although I won't like to do it.
Another way is to have a subset of 24-/48-bit unaligned accessors that use the trick specifically for hot paths with a caveat that they may access data out of the 24-/48-bit boundaries.
-- With Best Regards, Andy Shevchenko
| |