On Thu, 9 Feb 2023 at 17:13, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Wed, Feb 08, 2023 at 08:55:19PM +0000, Mark Rutland wrote:
> > On Wed, Feb 08, 2023 at 09:14:53PM +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 08, 2023 at 07:17:15AM -0800, Dave Hansen wrote:
> > > > On 2/6/23 04:49, Ard Biesheuvel wrote:
> > > > > --- a/arch/x86/kernel/apm_32.c
> > > > > +++ b/arch/x86/kernel/apm_32.c
> > > > > @@ -609,7 +609,7 @@ static long __apm_bios_call(void *_call)
> > > > >
> > > > > apm_irq_save(flags);
> > > > > firmware_restrict_branch_speculation_start();
> > > > > - ibt = ibt_save();
> > > > > + ibt = ibt_save(true);
> > > >
> > > > My only nit with these is the bare use of 'true'/'false'. It's
> > > > impossible to tell at the call-site what the 'true' means. So, if you
> > > > happen to respin these and see a nice way to remedy this I'd appreciate it.
> > >
> > > I've often wished for a named argument extention to C, much like named
> > > initializers, such that one can write:
> > >
> > > ibt_save(.disable = true);
> > >
> > > Because the thing you mention is very common with boolean arguments, the
> > > what gets lost in the argument name and true/false just isn't very
> > > telling.
> > >
> > > But yeah, even if by some miracle all compiler guys were like, YES! and
> > > implemented it tomorrow, we couldn't use it for a good few years anyway
> > > :-/
> >
> > Well... ;)
> >
> > | [mark@lakrids:~]% cat args.c
> > | #include <stdbool.h>
> > | #include <stdio.h>
> > |
> > | struct foo_args {
> > | bool enable;
> > | unsigned long other;
> > | };
> > |
> > | void __foo(struct foo_args args)
> > | {
> > | printf("foo:\n"
> > | " enable: %s\n"
> > | " other: 0x%lx\n",
> > | args.enable ? "YES" : "NO",
> > | args.other);
> > | }
> > |
> > | #define foo(args...) \
> > | __foo((struct foo_args) { args })
> > |
> > |
> > | int main(int argc, char *argv[])
> > | {
> > | foo(true);
> > | foo(.enable = true);
> > | foo(false, .other=0xdead);
> > | }
> > | [mark@lakrids:~]% gcc args.c -o args
> > | [mark@lakrids:~]% ./args
> > | foo:
> > | enable: YES
> > | other: 0x0
> > | foo:
> > | enable: YES
> > | other: 0x0
> > | foo:
> > | enable: NO
> > | other: 0xdead
>
> I am horrified and delighted.
+1
> And the resulting codegen is identical:
> https://godbolt.org/z/eKTMPYc17
>
> Without this fancy solution, what I'd seen is just using an enum:
>
> enum do_the_thing {
> THING_DISABLE = 0,
> THING_ENABLE,
> };
>
> void foo(enum do_the_thing enable)
> {
> if (enable) { ... }
> }
>
> foo(THING_ENABLE);
>
I have no strong preference one way or the other, but given that
apm_32.c is not the epicenter of new development, and the call from
EFI code is self-documenting already ('
ibt_save(efi_disable_ibt_for_runtime)', I'm inclined to just queue the
patch as-is, and leave it to whoever feels inclined to spend more free
time on this to come up with some nice polish to put on top.
Unless anyone minds?
