Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-23 Thread Andrew Jones 
On Mon, May 20, 2019 at 12:03:08PM +0200, Paolo Bonzini wrote:
> On 17/05/19 11:04, Thomas Huth wrote:
> > So far the KVM selftests are compiled without any compiler warnings
> > enabled. That's quite bad, since we miss a lot of possible bugs this
> > way. Let's enable at least "-Wall" and some other useful warning flags
> > now, and fix at least the trivial problems in the code (like unused
> > variables).
> > 
> > Signed-off-by: Thomas Huth 
> > ---
> >  v2:
> >  - Rebased to kvm/queue
> >  - Fix warnings in state_test.c and evmcs_test.c, too
> > 
> >  tools/testing/selftests/kvm/Makefile   | 4 +++-
> >  tools/testing/selftests/kvm/dirty_log_test.c   | 6 +-
> >  tools/testing/selftests/kvm/lib/kvm_util.c | 3 ---
> >  tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +---
> >  tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c   | 1 +
> >  tools/testing/selftests/kvm/x86_64/evmcs_test.c| 7 +--
> >  tools/testing/selftests/kvm/x86_64/platform_info_test.c| 1 -
> >  tools/testing/selftests/kvm/x86_64/smm_test.c  | 3 +--
> >  tools/testing/selftests/kvm/x86_64/state_test.c| 7 +--
> >  .../selftests/kvm/x86_64/vmx_close_while_nested_test.c | 5 +
> >  tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c   | 5 ++---
> >  11 files changed, 16 insertions(+), 30 deletions(-)
> 
> Queued, with a squashed fix to kvm_get_supported_hv_cpuid.
>

I've done the fixups needed to keep aarch64 compiling and will send
the patch shortly.

drew

Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-20 Thread Paolo Bonzini 
On 17/05/19 12:07, Thomas Huth wrote:
> 
> lib/ucall.c: In function ‘get_ucall’:
> lib/ucall.c:145:3: warning: dereferencing type-punned pointer will break
> strict-aliasing rules [-Wstrict-aliasing]
>gva = *(vm_vaddr_t *)run->mmio.data;
> 
> x86_64/vmx_set_nested_state_test.c: In function
> ‘set_revision_id_for_vmcs12’:
> x86_64/vmx_set_nested_state_test.c:78:2: warning: dereferencing
> type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>   *(u32 *)(state->data) = vmcs12_revision;
> 
> ... how do we want to handle such spots in the kvm selftest code?
> Compile with -fno-strict-aliasing? Or fix it with type-punning through
> unions?

I would use memcpy.  I'll send a patch shortly.

Paolo

Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-20 Thread Paolo Bonzini 
On 17/05/19 11:04, Thomas Huth wrote:
> So far the KVM selftests are compiled without any compiler warnings
> enabled. That's quite bad, since we miss a lot of possible bugs this
> way. Let's enable at least "-Wall" and some other useful warning flags
> now, and fix at least the trivial problems in the code (like unused
> variables).
> 
> Signed-off-by: Thomas Huth 
> ---
>  v2:
>  - Rebased to kvm/queue
>  - Fix warnings in state_test.c and evmcs_test.c, too
> 
>  tools/testing/selftests/kvm/Makefile   | 4 +++-
>  tools/testing/selftests/kvm/dirty_log_test.c   | 6 +-
>  tools/testing/selftests/kvm/lib/kvm_util.c | 3 ---
>  tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +---
>  tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c   | 1 +
>  tools/testing/selftests/kvm/x86_64/evmcs_test.c| 7 +--
>  tools/testing/selftests/kvm/x86_64/platform_info_test.c| 1 -
>  tools/testing/selftests/kvm/x86_64/smm_test.c  | 3 +--
>  tools/testing/selftests/kvm/x86_64/state_test.c| 7 +--
>  .../selftests/kvm/x86_64/vmx_close_while_nested_test.c | 5 +
>  tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c   | 5 ++---
>  11 files changed, 16 insertions(+), 30 deletions(-)

Queued, with a squashed fix to kvm_get_supported_hv_cpuid.

Paolo

Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-17 Thread Vitaly Kuznetsov 
Thomas Huth  writes:

>
> x86_64/vmx_set_nested_state_test.c: In function
> ‘set_revision_id_for_vmcs12’:
> x86_64/vmx_set_nested_state_test.c:78:2: warning: dereferencing
> type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>   *(u32 *)(state->data) = vmcs12_revision;
>
> ... how do we want to handle such spots in the kvm selftest code?
> Compile with -fno-strict-aliasing? Or fix it with type-punning through
> unions?

These are tests, let's keep code simple. Casting my vote for the former
option)

-- 
Vitaly

Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-17 Thread Thomas Huth 
On 17/05/2019 11.41, Vitaly Kuznetsov wrote:
> Peter Xu  writes:
> 
>> On Fri, May 17, 2019 at 11:04:45AM +0200, Thomas Huth wrote:
>>> So far the KVM selftests are compiled without any compiler warnings
>>> enabled. That's quite bad, since we miss a lot of possible bugs this
>>> way. Let's enable at least "-Wall" and some other useful warning flags
>>> now, and fix at least the trivial problems in the code (like unused
>>> variables).
>>>
>>> Signed-off-by: Thomas Huth 
>>> ---
>>>  v2:
>>>  - Rebased to kvm/queue
>>>  - Fix warnings in state_test.c and evmcs_test.c, too
>>
>> I still see these warnings (probably because the hyperv_cpuid.c is a
>> new test):
>>
>> In file included from x86_64/hyperv_cpuid.c:18:
>> x86_64/hyperv_cpuid.c: In function ‘test_hv_cpuid’:
>> x86_64/hyperv_cpuid.c:61:33: warning: suggest parentheses around comparison 
>> in operand of ‘==’ [-Wparentheses]
>>TEST_ASSERT(entry->padding[0] == entry->padding[1]
>>~~^~~~
>> include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’
>>   test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>>^
>> x86_64/hyperv_cpuid.c:62:8: warning: suggest parentheses around comparison 
>> in operand of ‘==’ [-Wparentheses]
>>TEST_ASSERT(entry->padding[0] == entry->padding[1]
>>~~
>> == entry->padding[2] == 0,
>> ^~~~
>> include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’
>>   test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
> 
> There's a fix from Dan Carpenter on the list:
> https://marc.info/?l=kernel-janitors&m=155783012012532&w=2

Right, that fix should preferably be committed first.

>> x86_64/hyperv_cpuid.c: In function ‘kvm_get_supported_hv_cpuid’:
>> x86_64/hyperv_cpuid.c:93:6: warning: unused variable ‘ret’ 
>> [-Wunused-variable]
>>   int ret;
>>   ^~~

... but I obviously missed this one here again :-/

There are also these compiler warnings left:

lib/ucall.c: In function ‘get_ucall’:
lib/ucall.c:145:3: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
   gva = *(vm_vaddr_t *)run->mmio.data;

x86_64/vmx_set_nested_state_test.c: In function
‘set_revision_id_for_vmcs12’:
x86_64/vmx_set_nested_state_test.c:78:2: warning: dereferencing
type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  *(u32 *)(state->data) = vmcs12_revision;

... how do we want to handle such spots in the kvm selftest code?
Compile with -fno-strict-aliasing? Or fix it with type-punning through
unions?

 Thomas

Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-17 Thread Vitaly Kuznetsov 
Peter Xu  writes:

> On Fri, May 17, 2019 at 11:04:45AM +0200, Thomas Huth wrote:
>> So far the KVM selftests are compiled without any compiler warnings
>> enabled. That's quite bad, since we miss a lot of possible bugs this
>> way. Let's enable at least "-Wall" and some other useful warning flags
>> now, and fix at least the trivial problems in the code (like unused
>> variables).
>> 
>> Signed-off-by: Thomas Huth 
>> ---
>>  v2:
>>  - Rebased to kvm/queue
>>  - Fix warnings in state_test.c and evmcs_test.c, too
>
> I still see these warnings (probably because the hyperv_cpuid.c is a
> new test):
>
> In file included from x86_64/hyperv_cpuid.c:18:
> x86_64/hyperv_cpuid.c: In function ‘test_hv_cpuid’:
> x86_64/hyperv_cpuid.c:61:33: warning: suggest parentheses around comparison 
> in operand of ‘==’ [-Wparentheses]
>TEST_ASSERT(entry->padding[0] == entry->padding[1]
>~~^~~~
> include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’
>   test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>^
> x86_64/hyperv_cpuid.c:62:8: warning: suggest parentheses around comparison in 
> operand of ‘==’ [-Wparentheses]
>TEST_ASSERT(entry->padding[0] == entry->padding[1]
>~~
> == entry->padding[2] == 0,
> ^~~~
> include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’
>   test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)

There's a fix from Dan Carpenter on the list:
https://marc.info/?l=kernel-janitors&m=155783012012532&w=2

>^
> x86_64/hyperv_cpuid.c: In function ‘kvm_get_supported_hv_cpuid’:
> x86_64/hyperv_cpuid.c:93:6: warning: unused variable ‘ret’ [-Wunused-variable]
>   int ret;
>   ^~~
>
> The first two seem to be real bugs in the test code, and the 3rd one
> might need a cleanup too.

-- 
Vitaly

Re: [PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-17 Thread Peter Xu 
On Fri, May 17, 2019 at 11:04:45AM +0200, Thomas Huth wrote:
> So far the KVM selftests are compiled without any compiler warnings
> enabled. That's quite bad, since we miss a lot of possible bugs this
> way. Let's enable at least "-Wall" and some other useful warning flags
> now, and fix at least the trivial problems in the code (like unused
> variables).
> 
> Signed-off-by: Thomas Huth 
> ---
>  v2:
>  - Rebased to kvm/queue
>  - Fix warnings in state_test.c and evmcs_test.c, too

I still see these warnings (probably because the hyperv_cpuid.c is a
new test):

In file included from x86_64/hyperv_cpuid.c:18:
x86_64/hyperv_cpuid.c: In function ‘test_hv_cpuid’:
x86_64/hyperv_cpuid.c:61:33: warning: suggest parentheses around comparison in 
operand of ‘==’ [-Wparentheses]
   TEST_ASSERT(entry->padding[0] == entry->padding[1]
   ~~^~~~
include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’
  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
   ^
x86_64/hyperv_cpuid.c:62:8: warning: suggest parentheses around comparison in 
operand of ‘==’ [-Wparentheses]
   TEST_ASSERT(entry->padding[0] == entry->padding[1]
   ~~
== entry->padding[2] == 0,
^~~~
include/test_util.h:32:15: note: in definition of macro ‘TEST_ASSERT’
  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
   ^
x86_64/hyperv_cpuid.c: In function ‘kvm_get_supported_hv_cpuid’:
x86_64/hyperv_cpuid.c:93:6: warning: unused variable ‘ret’ [-Wunused-variable]
  int ret;
  ^~~

The first two seem to be real bugs in the test code, and the 3rd one
might need a cleanup too.

Thanks,

-- 
Peter Xu

[PATCH v2] KVM: selftests: Compile code with warnings enabled

2019-05-17 Thread Thomas Huth 
So far the KVM selftests are compiled without any compiler warnings
enabled. That's quite bad, since we miss a lot of possible bugs this
way. Let's enable at least "-Wall" and some other useful warning flags
now, and fix at least the trivial problems in the code (like unused
variables).

Signed-off-by: Thomas Huth 
---
 v2:
 - Rebased to kvm/queue
 - Fix warnings in state_test.c and evmcs_test.c, too

 tools/testing/selftests/kvm/Makefile   | 4 +++-
 tools/testing/selftests/kvm/dirty_log_test.c   | 6 +-
 tools/testing/selftests/kvm/lib/kvm_util.c | 3 ---
 tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +---
 tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c   | 1 +
 tools/testing/selftests/kvm/x86_64/evmcs_test.c| 7 +--
 tools/testing/selftests/kvm/x86_64/platform_info_test.c| 1 -
 tools/testing/selftests/kvm/x86_64/smm_test.c  | 3 +--
 tools/testing/selftests/kvm/x86_64/state_test.c| 7 +--
 .../selftests/kvm/x86_64/vmx_close_while_nested_test.c | 5 +
 tools/testing/selftests/kvm/x86_64/vmx_tsc_adjust_test.c   | 5 ++---
 11 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/tools/testing/selftests/kvm/Makefile 
b/tools/testing/selftests/kvm/Makefile
index 79c524395ebe..d113eaf2d570 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -34,7 +34,9 @@ LIBKVM += $(LIBKVM_$(UNAME_M))
 INSTALL_HDR_PATH = $(top_srcdir)/usr
 LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/
 LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include
-CFLAGS += -O2 -g -std=gnu99 -fno-stack-protector -fno-PIE 
-I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(page_size;
 
@@ -1334,7 +1332,6 @@ void vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid, 
struct kvm_sregs *sregs)
 int _vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs 
*sregs)
 {
struct vcpu *vcpu = vcpu_find(vm, vcpuid);
-   int ret;
 
TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
 
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c 
b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index dc7fae9fa424..21f3040d90cb 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -229,8 +229,6 @@ void sregs_dump(FILE *stream, struct kvm_sregs *sregs,
 
 void virt_pgd_alloc(struct kvm_vm *vm, uint32_t pgd_memslot)
 {
-   int rc;
-
TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
 
@@ -549,7 +547,6 @@ vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
struct pageDirectoryPointerEntry *pdpe;
struct pageDirectoryEntry *pde;
struct pageTableEntry *pte;
-   void *hva;
 
TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
@@ -582,6 +579,7 @@ vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
 unmapped_gva:
TEST_ASSERT(false, "No mapping for vm virtual address, "
"gva: 0x%lx", gva);
+   exit(EXIT_FAILURE);
 }
 
 static void kvm_setup_gdt(struct kvm_vm *vm, struct kvm_dtable *dt, int 
gdt_memslot,
diff --git a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c 
b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
index 7c2c4d4055a8..63cc9c3f5ab6 100644
--- a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
+++ b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
@@ -87,6 +87,7 @@ int main(int argc, char *argv[])
while (1) {
rc = _vcpu_run(vm, VCPU_ID);
 
+   TEST_ASSERT(rc == 0, "vcpu_run failed: %d\n", rc);
TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
"Unexpected exit reason: %u (%s),\n",
run->exit_reason,
diff --git a/tools/testing/selftests/kvm/x86_64/evmcs_test.c 
b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
index 36669684eca5..b38260e29775 100644
--- a/tools/testing/selftests/kvm/x86_64/evmcs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
@@ -19,8 +19,6 @@
 
 #define VCPU_ID5
 
-static bool have_nested_state;
-
 void l2_guest_code(void)
 {
GUEST_SYNC(6);
@@ -73,7 +71,6 @@ void guest_code(struct vmx_pages *vmx_pages)
 
 int main(int argc, char *argv[])
 {
-   struct vmx_pages *vmx_pages = NULL;
vm_vaddr_t vmx_pages_gva = 0;
 
struct kvm_regs regs1, regs2;
@@ -88,8 +85,6 @@ int main(int argc, char *argv[])
 .args[0] = (unsigned long)&evmcs_ver
};
 
-   struct kvm_cpuid_entry2 *entry = kvm_get_supported_cpuid_entry(1);
-
/* Create VM */
vm = vm_create_default(VCPU_ID, 0, guest_code);
 
@@ -113,7 +108,7 @@ int main(int argc, char *argv[])
 
vcpu_regs_get(vm, VCPU_ID,