Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 25 May 2022 17:32:28 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Jon Kohler <jon@nutanix.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Waiman Long <longman@redhat.com>,
        Kees Cook <keescook@chromium.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] KVM: VMX: do not disable interception for
 MSR_IA32_SPEC_CTRL on eIBRS
Message-ID: <Yo5oLAzdYurlMQFm@google.com>
References: <20220520204115.67580-1-jon@nutanix.com>
 <Yo5hmcdRvE1UrI4y@google.com>
 <3C8F5313-2830-46E3-A512-CFA4A24C24D7@nutanix.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <3C8F5313-2830-46E3-A512-CFA4A24C24D7@nutanix.com>
Precedence: bulk

On Wed, May 25, 2022, Jon Kohler wrote:
> > On May 25, 2022, at 1:04 PM, Sean Christopherson <seanjc@google.com> wrote:
> > the code.  Again, it's all about whether eIBRS is advertised to the guest.  With
> > some other minor tweaking to wrangle the comment to 80 chars...
> 
> RE 80 chars - quick question (and forgive the silly question here), but how are you
> counting that? I’ve got my editor cutting at 79 cols, where tab size is accounted
> for as 4 cols, so the longest line on my side for this patch is 72-73 or so.

Tabs are 8 cols in the kernel.  FWIW, your patch was totally fine with respect to
wrapping, my comment was purely to give the heads up that I arbitrarily tweaked
other bits of the comment to adjust for my suggested reword.

> These also pass the checkpatch.pl script as well, so I just want to make sure
> going forward I’m formatting them appropriately.

For future reference, checkpatch.pl only yells if a line length exceeds 100 chars.
The 80 char limit is a soft limit, with 100 chars being a mostly-firm limit.
checkpatch used to yell at 80, but was changed because too many people were
interpreting 80 chars as a hard limit and blindly wrapping code to make checkpatch
happy at the cost of yielding less readable code.

Whether or not to run over the soft limit is definitely subjective, just try to
use common sense.  E.g. overflowing by 1-2 chars, not a big deal, especially if
the statement would otherwise fit on a single line, i.e. doesn't already wrap.

A decent example is the SGX MSRs, which allows the SGX_LC_ENABLED check to run
over a little, but wraps the data update.  The reasoning is that

		if (!msr_info->host_initiated &&
		    (!guest_cpuid_has(vcpu, X86_FEATURE_SGX_LC) ||
		    ((vmx->msr_ia32_feature_control & FEAT_CTL_LOCKED) &&
		    !(vmx->msr_ia32_feature_control & FEAT_CTL_SGX_LC_ENABLED))))
			return 1;

is much more readable than 

		if (!msr_info->host_initiated &&
		    (!guest_cpuid_has(vcpu, X86_FEATURE_SGX_LC) ||
		    ((vmx->msr_ia32_feature_control & FEAT_CTL_LOCKED) &&
		    !(vmx->msr_ia32_feature_control &
		      FEAT_CTL_SGX_LC_ENABLED))))
			return 1;

but this

		vmx->msr_ia32_sgxlepubkeyhash
			[msr_index - MSR_IA32_SGXLEPUBKEYHASH0] = data;

is only isn't _that_ much worse than running the line way out, and ~93 chars gets
to be a bit too long.

		vmx->msr_ia32_sgxlepubkeyhash[msr_index - MSR_IA32_SGXLEPUBKEYHASH0] = data;