| Message ID | 20190712133324.3934659-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] x86: kvm: avoid -Wsometimes-uninitized warning | expand |
On Fri, Jul 12, 2019 at 03:32:43PM +0200, Arnd Bergmann wrote: > clang points out that running a 64-bit guest on a 32-bit host > would lead to uninitialized variables: > > arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] > if (!longmode) { > ^~~~~~~~~ > arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here > trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa); > ^~~~~ > arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true > if (!longmode) { > ^~~~~~~~~~~~~~~ > arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning > u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS; > ^ > = 0 > arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] > arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] > > Since that combination is not supported anyway, change the condition > to tell the compiler how the code is actually executed. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > v2: make the change inside of is_64_bit_mode(). > --- > arch/x86/kvm/hyperv.c | 6 ++---- > arch/x86/kvm/x86.h | 4 ++++ > 2 files changed, 6 insertions(+), 4 deletions(-) Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> However I still think the log message could state it more explicitly that it was the compiler's fault, and the patch is a workaround for it. Otherwise later on someone may decide to restore the similarity of is_64_bit_mode() to other inlines in this file, and will be extremely unlikely to test clang 32-bit build... Roman.
On Fri, Jul 12, 2019 at 3:54 PM Roman Kagan <rkagan@virtuozzo.com> wrote: > > On Fri, Jul 12, 2019 at 03:32:43PM +0200, Arnd Bergmann wrote: > > clang points out that running a 64-bit guest on a 32-bit host > > would lead to uninitialized variables: > > > > arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] > > if (!longmode) { > > ^~~~~~~~~ > > arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here > > trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa); > > ^~~~~ > > arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true > > if (!longmode) { > > ^~~~~~~~~~~~~~~ > > arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning > > u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS; > > ^ > > = 0 > > arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] > > arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] > > > > Since that combination is not supported anyway, change the condition > > to tell the compiler how the code is actually executed. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > v2: make the change inside of is_64_bit_mode(). > > --- > > arch/x86/kvm/hyperv.c | 6 ++---- > > arch/x86/kvm/x86.h | 4 ++++ > > 2 files changed, 6 insertions(+), 4 deletions(-) > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > > However I still think the log message could state it more explicitly > that it was the compiler's fault, and the patch is a workaround for it. > > Otherwise later on someone may decide to restore the similarity of > is_64_bit_mode() to other inlines in this file, and will be extremely > unlikely to test clang 32-bit build... Fair enough. I've reworded the changelog, as well as the patch to document this now, in a way that should make it harder to introduce the warning again by accident. Unfortunately, that #ifdef check cannot be turned into an if(IS_ENABLED()) because kvm_r8_read() is not defined on i386. Note that the 0-day bot now tests with clang as well, so you would definitely hear about a warning appearing. v3 coming. Arnd
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index a39e38f13029..4c1c423a3d08 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1594,7 +1594,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) { u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS; uint16_t code, rep_idx, rep_cnt; - bool fast, longmode, rep; + bool fast, rep; /* * hypercall generates UD from non zero cpl and real mode @@ -1605,9 +1605,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) return 1; } - longmode = is_64_bit_mode(vcpu); - - if (!longmode) { + if (!is_64_bit_mode(vcpu)) { param = ((u64)kvm_rdx_read(vcpu) << 32) | (kvm_rax_read(vcpu) & 0xffffffff); ingpa = ((u64)kvm_rbx_read(vcpu) << 32) | diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index e08a12892e8b..b457b3267296 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -90,6 +90,7 @@ static inline int is_long_mode(struct kvm_vcpu *vcpu) #endif } +#ifdef CONFIG_X86_64 static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu) { int cs_db, cs_l; @@ -99,6 +100,9 @@ static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu) kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l); return cs_l; } +#else +#define is_64_bit_mode(vcpu) false +#endif static inline bool is_la57_mode(struct kvm_vcpu *vcpu) {
clang points out that running a 64-bit guest on a 32-bit host would lead to uninitialized variables: arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] if (!longmode) { ^~~~~~~~~ arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa); ^~~~~ arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true if (!longmode) { ^~~~~~~~~~~~~~~ arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS; ^ = 0 arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] Since that combination is not supported anyway, change the condition to tell the compiler how the code is actually executed. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- v2: make the change inside of is_64_bit_mode(). --- arch/x86/kvm/hyperv.c | 6 ++---- arch/x86/kvm/x86.h | 4 ++++ 2 files changed, 6 insertions(+), 4 deletions(-) -- 2.20.0