Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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