[PATCH v2 3/3] efi: x86: Wire up IBT annotation in memory attributes table

[Mark Rutland]

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 at 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 at lakrids:~]% gcc args.c -o args
| [mark at lakrids:~]% ./args            
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: YES
|   other: 0x0
| foo:
|   enable: NO
|   other: 0xdead

Mark.