Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 21 Dec 2020 15:05:08 -0800
From:   Sean Christopherson <seanjc@google.com>
To:     Krish Sadhukhan <krish.sadhukhan@oracle.com>
Cc:     Uros Bizjak <ubizjak@gmail.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v3] KVM/x86: Move definition of __ex to x86.h
Message-ID: <X+EqJB/k1kxSWE7m@google.com>
References: <20201221194800.46962-1-ubizjak@gmail.com>
 <a773afca-7f28-2392-74ad-0895da3f75ca@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a773afca-7f28-2392-74ad-0895da3f75ca@oracle.com>
Precedence: bulk

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");