| Message ID | 20240617210432.1642542-4-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: SEV-ES save area fix+cleanups | expand |
On Mon, Jun 17, 2024 at 02:04:32PM -0700, Sean Christopherson wrote: > When reading MSR_TSC_AUX, which is effectively a 32-bit value, use a > compound literal to consume/ignore the upper 32 bits instead of a "maybe" > unused local variable. While (ab)using a compound literal to discard an > unused output is unusual, it's perfectly legal as compound literals are > valid, modifiable l-values in C99 and beyond. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/svm/svm.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index cf285472fea6..69c0dea5cc0d 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -678,11 +678,8 @@ static int svm_hardware_enable(void) > * Since Linux does not change the value of TSC_AUX once set, prime the > * TSC_AUX field now to avoid a RDMSR on every vCPU run. > */ > - if (boot_cpu_has(X86_FEATURE_V_TSC_AUX)) { > - u32 __maybe_unused msr_hi; > - > - rdmsr(MSR_TSC_AUX, sev_es_host_save_area(sd)->tsc_aux, msr_hi); > - } > + if (boot_cpu_has(X86_FEATURE_V_TSC_AUX)) You can switch to cpu_feature_enabled() while you're here. > + rdmsr(MSR_TSC_AUX, sev_es_host_save_area(sd)->tsc_aux, (u32){0}); > > return 0; > } > -- Right, slick, but I'd argue that before it was better because __maybe_unused was actually documenting why that var is there. (u32){} looks a bit more cryptic but meh, ok, I guess. With those three applied, I only get arch/x86/kvm/kvm.o: warning: objtool: .text+0x0: unreachable instruction arch/x86/kvm/kvm.o: warning: objtool: .text+0x4: unreachable instruction now on a randconfig which is attached. When built with clang tho: $ clang --version Ubuntu clang version 14.0.0-1ubuntu1.1 gcc is fine. Thx.
On Tue, Jun 18, 2024, Borislav Petkov wrote: > > + rdmsr(MSR_TSC_AUX, sev_es_host_save_area(sd)->tsc_aux, (u32){0}); > > > > return 0; > > } > > -- > > Right, slick, but I'd argue that before it was better because __maybe_unused was > actually documenting why that var is there. > > (u32){} looks a bit more cryptic but meh, ok, I guess. Yeah, I'm 50/50 on whether it's too clever. I'm totally fine keeping msr_hi, what I really want to do is rewrite the rdmsr() macros to return values :-/
On Tue, Jun 18, 2024 at 06:44:40AM -0700, Sean Christopherson wrote: > Yeah, I'm 50/50 on whether it's too clever. I'm totally fine keeping msr_hi, > what I really want to do is rewrite the rdmsr() macros to return values :-/ ... and make them inline functions while at it? And perhaps unify that mess: rdmsr, rdmsrl, rdmsr_safe, __rdmsr... And, and, ... And, and, ... Yeah, it certainly needs scrubbing. Once we agree on the approach we can split the work so that it can get done gradually, without any humongous patchsets converting the world and flag days and what not breakage... Thx.
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index cf285472fea6..69c0dea5cc0d 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -678,11 +678,8 @@ static int svm_hardware_enable(void) * Since Linux does not change the value of TSC_AUX once set, prime the * TSC_AUX field now to avoid a RDMSR on every vCPU run. */ - if (boot_cpu_has(X86_FEATURE_V_TSC_AUX)) { - u32 __maybe_unused msr_hi; - - rdmsr(MSR_TSC_AUX, sev_es_host_save_area(sd)->tsc_aux, msr_hi); - } + if (boot_cpu_has(X86_FEATURE_V_TSC_AUX)) + rdmsr(MSR_TSC_AUX, sev_es_host_save_area(sd)->tsc_aux, (u32){0}); return 0; }
When reading MSR_TSC_AUX, which is effectively a 32-bit value, use a compound literal to consume/ignore the upper 32 bits instead of a "maybe" unused local variable. While (ab)using a compound literal to discard an unused output is unusual, it's perfectly legal as compound literals are valid, modifiable l-values in C99 and beyond. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/svm.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)