arch/x86/kvm/svm.c:3600: possible bad operator ?

arch/x86/kvm/svm.c:3600: possible bad operator ?

[David Binderman]

Hello there,

arch/x86/kvm/svm.c:3600:45: warning: logical ‘and’ applied to
non-boolean constant [-Wlogical-op]

Source code is

    u32 index = svm->vmcb->control.exit_info_2 && 0xFF;

Maybe better code

    u32 index = svm->vmcb->control.exit_info_2 & 0xFF;


Regards

David Binderman

Re: arch/x86/kvm/svm.c:3600: possible bad operator ?

[Paolo Bonzini]

On 23/05/2016 13:22, David Binderman wrote:
> Hello there,
> 
> arch/x86/kvm/svm.c:3600:45: warning: logical ‘and’ applied to
> non-boolean constant [-Wlogical-op]
> 
> Source code is
> 
>     u32 index = svm->vmcb->control.exit_info_2 && 0xFF;
> 
> Maybe better code
> 
>     u32 index = svm->vmcb->control.exit_info_2 & 0xFF;

Yes, there's a bug there.  However it's only affecting a tracepoint,
which is why it probably wasn't noticed.

Thanks,

Paolo

Re: arch/x86/kvm/svm.c:3600: possible bad operator ?

[Thomas Huth]

On 23.05.2016 13:42, Paolo Bonzini wrote:
> 
> On 23/05/2016 13:22, David Binderman wrote:
>> Hello there,
>>
>> arch/x86/kvm/svm.c:3600:45: warning: logical ‘and’ applied to
>> non-boolean constant [-Wlogical-op]
>>
>> Source code is
>>
>>     u32 index = svm->vmcb->control.exit_info_2 && 0xFF;
>>
>> Maybe better code
>>
>>     u32 index = svm->vmcb->control.exit_info_2 & 0xFF;
> 
> Yes, there's a bug there.  However it's only affecting a tracepoint,
> which is why it probably wasn't noticed.

This define in the same file also looks quite suspicious:

#define AVIC_HPA_MASK	~((0xFFFULL << 52) || 0xFFF)

Strange that you did not get a warning on that one, too?

 Thomas

Re: arch/x86/kvm/svm.c:3600: possible bad operator ?

[David Binderman]

Hello there,

On Mon, May 23, 2016 at 3:52 PM, Thomas Huth <thuth@redhat.com> wrote:
> This define in the same file also looks quite suspicious:
>
> #define AVIC_HPA_MASK   ~((0xFFFULL << 52) || 0xFFF)
>
> Strange that you did not get a warning on that one, too?

I did, but the warnings were buried in a morass of other warnings, so
I didn't notice it ;-<

[linux-next/arch/x86/kvm/svm.c:1075]: (style) Boolean result is used
in bitwise operation. Clarify expression with parentheses.
[linux-next/arch/x86/kvm/svm.c:1076]: (style) Boolean result is used
in bitwise operation. Clarify expression with parentheses.
[linux-next/arch/x86/kvm/svm.c:1077]: (style) Boolean result is used
in bitwise operation. Clarify expression with parentheses.

Three warnings, one for each use of the macro AVIC_HPA_MASK.

My apologies for not spotting this sooner. In order to avoid further
public embarrassment, here are a couple of other minor things I
found in x86/kvm/*.c, which might be worth fixing.

arch/x86/kvm/cpuid.c:867]: (style) Variable 'function' is assigned a
value that is never used.
arch/x86/kvm/x86.c:2142]: (style) Variable 'gpa_offset' is assigned a
value that is never used.


Regards

David Binderman

Re: arch/x86/kvm/svm.c:3600: possible bad operator ?

[Paolo Bonzini]

On 23/05/2016 17:05, David Binderman wrote:
> Hello there,
> 
> On Mon, May 23, 2016 at 3:52 PM, Thomas Huth <thuth@redhat.com> wrote:
>> This define in the same file also looks quite suspicious:
>>
>> #define AVIC_HPA_MASK   ~((0xFFFULL << 52) || 0xFFF)
>>
>> Strange that you did not get a warning on that one, too?
> 
> I did, but the warnings were buried in a morass of other warnings, so
> I didn't notice it ;-<
> 
> [linux-next/arch/x86/kvm/svm.c:1075]: (style) Boolean result is used
> in bitwise operation. Clarify expression with parentheses.
> [linux-next/arch/x86/kvm/svm.c:1076]: (style) Boolean result is used
> in bitwise operation. Clarify expression with parentheses.
> [linux-next/arch/x86/kvm/svm.c:1077]: (style) Boolean result is used
> in bitwise operation. Clarify expression with parentheses.
> 
> Three warnings, one for each use of the macro AVIC_HPA_MASK.

Thanks, I'll squash these in:

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 2eb615e400e3..1163e8173e5a 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -84,7 +84,7 @@ MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id);
 #define TSC_RATIO_MIN		0x0000000000000001ULL
 #define TSC_RATIO_MAX		0x000000ffffffffffULL

-#define AVIC_HPA_MASK	~((0xFFFULL << 52) || 0xFFF)
+#define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)

 /*
  * 0xff is broadcast, so the max index allowed for physical APIC ID


Paolo

> My apologies for not spotting this sooner. In order to avoid further
> public embarrassment, here are a couple of other minor things I
> found in x86/kvm/*.c, which might be worth fixing.
> 
> arch/x86/kvm/cpuid.c:867]: (style) Variable 'function' is assigned a
> value that is never used.
> arch/x86/kvm/x86.c:2142]: (style) Variable 'gpa_offset' is assigned a
> value that is never used.
> 
> 
> Regards
> 
> David Binderman
>