linux-kernel - Re: [PATCH v3] KVM/x86: Move definition of _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X+EqJB/k1kxSWE7m@google.com>
Date:   Mon, 21 Dec 2020 15:05:08 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Krish Sadhukhan <krish.sadhukhan@...cle.com>
Cc:     Uros Bizjak <ubizjak@...il.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v3] KVM/x86: Move definition of __ex to x86.h

On Mon, Dec 21, 2020, Krish Sadhukhan wrote:
> 
> On 12/21/20 11:48 AM, Uros Bizjak wrote:
> > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > index c5ee0f5ce0f1..5b16d2b5c3bc 100644
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -8,6 +8,30 @@
> >   #include "kvm_cache_regs.h"
> >   #include "kvm_emulate.h"
> > +asmlinkage void kvm_spurious_fault(void);
> > +
> > +/*
> > + * Handle a fault on a hardware virtualization (VMX or SVM) instruction.
> > + *
> > + * Hardware virtualization extension instructions may fault if a reboot turns
> > + * off virtualization while processes are running.  Usually after catching the
> > + * fault we just panic; during reboot instead the instruction is ignored.
> > + */
> > +#define __ex(insn)							\
> 
> 
> While the previous name was too elaborate, this new name is very cryptic. 
> Unless we are saving for space,it's better to give a somewhat descriptive
> name.

We are saving for space in a way.  Not so much to actually save lines of code,
but to avoid stealing the focus from the code that matters.  __ex() is cryptic
for the completely unfamiliar, but I'm worried that anything more verbose will
harm the readability of the code where it is used, which is usually what's more
important in the long run.  

__ex() does have some meaning, as it's connected to the various ex_handler_*()
helpers.  ex_handle() is the best semi-verbose alternative that I can think of,
but even that is too long for my liking when reading the inline asm flows.  And
it's not like ex_handle() tells the whole story; the reader still has to go to
the definition to understand what it does, or worse, will make incorrect
assumptions about how exceptions are handled.

E.g. with the short version, my eyes gravitate toward vmxoff/vmsave without
getting stuck on the verbose wrapper.

	asm volatile (__ex("vmxoff"));

	asm volatile(__ex("vmsave") : : "a" (__sme_page_pa(sd->save_area)) : "memory");

vs.

	asm volatile (ex_handle("vmxoff"));

	asm volatile(ex_handle("vmsave") : : "a" (__sme_page_pa(sd->save_area)) : "memory");