mbox series

[0/4] KVM: selftests: Various cleanups and fixes

Message ID 20200310091556.4701-1-drjones@redhat.com (mailing list archive)
Headers show
Series KVM: selftests: Various cleanups and fixes | expand

Message

Andrew Jones March 10, 2020, 9:15 a.m. UTC
Andrew Jones (4):
  fixup! selftests: KVM: SVM: Add vmcall test
  KVM: selftests: Share common API documentation
  KVM: selftests: Enable printf format warnings for TEST_ASSERT
  KVM: selftests: Use consistent message for test skipping

 tools/testing/selftests/kvm/.gitignore        |   5 +-
 .../selftests/kvm/demand_paging_test.c        |   6 +-
 tools/testing/selftests/kvm/dirty_log_test.c  |   3 +-
 .../testing/selftests/kvm/include/kvm_util.h  | 100 ++++++++-
 .../testing/selftests/kvm/include/test_util.h |   5 +-
 .../selftests/kvm/lib/aarch64/processor.c     |  17 --
 tools/testing/selftests/kvm/lib/assert.c      |   6 +-
 tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +-
 .../selftests/kvm/lib/kvm_util_internal.h     |  48 +++++
 .../selftests/kvm/lib/s390x/processor.c       |  74 -------
 tools/testing/selftests/kvm/lib/test_util.c   |  12 ++
 .../selftests/kvm/lib/x86_64/processor.c      | 196 ++++--------------
 tools/testing/selftests/kvm/lib/x86_64/svm.c  |   2 +-
 tools/testing/selftests/kvm/lib/x86_64/vmx.c  |   2 +-
 tools/testing/selftests/kvm/s390x/memop.c     |   2 +-
 .../selftests/kvm/s390x/sync_regs_test.c      |   2 +-
 .../kvm/x86_64/cr4_cpuid_sync_test.c          |   2 +-
 .../testing/selftests/kvm/x86_64/evmcs_test.c |   6 +-
 .../selftests/kvm/x86_64/hyperv_cpuid.c       |   8 +-
 .../selftests/kvm/x86_64/mmio_warning_test.c  |   4 +-
 .../selftests/kvm/x86_64/platform_info_test.c |   3 +-
 .../kvm/x86_64/set_memory_region_test.c       |   3 +-
 .../testing/selftests/kvm/x86_64/state_test.c |   4 +-
 .../selftests/kvm/x86_64/svm_vmcall_test.c    |   3 +-
 .../selftests/kvm/x86_64/sync_regs_test.c     |   4 +-
 .../selftests/kvm/x86_64/vmx_dirty_log_test.c |   2 +-
 .../kvm/x86_64/vmx_set_nested_state_test.c    |   4 +-
 .../selftests/kvm/x86_64/xss_msr_test.c       |   2 +-
 28 files changed, 243 insertions(+), 292 deletions(-)

Comments

Christian Borntraeger March 10, 2020, 9:45 a.m. UTC | #1
On 10.03.20 10:15, Andrew Jones wrote:
> 
> Andrew Jones (4):
>   fixup! selftests: KVM: SVM: Add vmcall test
>   KVM: selftests: Share common API documentation
>   KVM: selftests: Enable printf format warnings for TEST_ASSERT
>   KVM: selftests: Use consistent message for test skipping

This looks like a nice cleanup but this does not seem to apply
cleanly on kvm/master or linus/master. Which tree is this based on?

> 
>  tools/testing/selftests/kvm/.gitignore        |   5 +-
>  .../selftests/kvm/demand_paging_test.c        |   6 +-
>  tools/testing/selftests/kvm/dirty_log_test.c  |   3 +-
>  .../testing/selftests/kvm/include/kvm_util.h  | 100 ++++++++-
>  .../testing/selftests/kvm/include/test_util.h |   5 +-
>  .../selftests/kvm/lib/aarch64/processor.c     |  17 --
>  tools/testing/selftests/kvm/lib/assert.c      |   6 +-
>  tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +-
>  .../selftests/kvm/lib/kvm_util_internal.h     |  48 +++++
>  .../selftests/kvm/lib/s390x/processor.c       |  74 -------
>  tools/testing/selftests/kvm/lib/test_util.c   |  12 ++
>  .../selftests/kvm/lib/x86_64/processor.c      | 196 ++++--------------
>  tools/testing/selftests/kvm/lib/x86_64/svm.c  |   2 +-
>  tools/testing/selftests/kvm/lib/x86_64/vmx.c  |   2 +-
>  tools/testing/selftests/kvm/s390x/memop.c     |   2 +-
>  .../selftests/kvm/s390x/sync_regs_test.c      |   2 +-
>  .../kvm/x86_64/cr4_cpuid_sync_test.c          |   2 +-
>  .../testing/selftests/kvm/x86_64/evmcs_test.c |   6 +-
>  .../selftests/kvm/x86_64/hyperv_cpuid.c       |   8 +-
>  .../selftests/kvm/x86_64/mmio_warning_test.c  |   4 +-
>  .../selftests/kvm/x86_64/platform_info_test.c |   3 +-
>  .../kvm/x86_64/set_memory_region_test.c       |   3 +-
>  .../testing/selftests/kvm/x86_64/state_test.c |   4 +-
>  .../selftests/kvm/x86_64/svm_vmcall_test.c    |   3 +-
>  .../selftests/kvm/x86_64/sync_regs_test.c     |   4 +-
>  .../selftests/kvm/x86_64/vmx_dirty_log_test.c |   2 +-
>  .../kvm/x86_64/vmx_set_nested_state_test.c    |   4 +-
>  .../selftests/kvm/x86_64/xss_msr_test.c       |   2 +-
>  28 files changed, 243 insertions(+), 292 deletions(-)
>
Andrew Jones March 10, 2020, 11:58 a.m. UTC | #2
On Tue, Mar 10, 2020 at 10:45:43AM +0100, Christian Borntraeger wrote:
> On 10.03.20 10:15, Andrew Jones wrote:
> > 
> > Andrew Jones (4):
> >   fixup! selftests: KVM: SVM: Add vmcall test
> >   KVM: selftests: Share common API documentation
> >   KVM: selftests: Enable printf format warnings for TEST_ASSERT
> >   KVM: selftests: Use consistent message for test skipping
> 
> This looks like a nice cleanup but this does not seem to apply
> cleanly on kvm/master or linus/master. Which tree is this based on?

This is based on kvm/queue. Sorry, I should have mentioned that in
the cover letter.

Thanks,
drew

> 
> > 
> >  tools/testing/selftests/kvm/.gitignore        |   5 +-
> >  .../selftests/kvm/demand_paging_test.c        |   6 +-
> >  tools/testing/selftests/kvm/dirty_log_test.c  |   3 +-
> >  .../testing/selftests/kvm/include/kvm_util.h  | 100 ++++++++-
> >  .../testing/selftests/kvm/include/test_util.h |   5 +-
> >  .../selftests/kvm/lib/aarch64/processor.c     |  17 --
> >  tools/testing/selftests/kvm/lib/assert.c      |   6 +-
> >  tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +-
> >  .../selftests/kvm/lib/kvm_util_internal.h     |  48 +++++
> >  .../selftests/kvm/lib/s390x/processor.c       |  74 -------
> >  tools/testing/selftests/kvm/lib/test_util.c   |  12 ++
> >  .../selftests/kvm/lib/x86_64/processor.c      | 196 ++++--------------
> >  tools/testing/selftests/kvm/lib/x86_64/svm.c  |   2 +-
> >  tools/testing/selftests/kvm/lib/x86_64/vmx.c  |   2 +-
> >  tools/testing/selftests/kvm/s390x/memop.c     |   2 +-
> >  .../selftests/kvm/s390x/sync_regs_test.c      |   2 +-
> >  .../kvm/x86_64/cr4_cpuid_sync_test.c          |   2 +-
> >  .../testing/selftests/kvm/x86_64/evmcs_test.c |   6 +-
> >  .../selftests/kvm/x86_64/hyperv_cpuid.c       |   8 +-
> >  .../selftests/kvm/x86_64/mmio_warning_test.c  |   4 +-
> >  .../selftests/kvm/x86_64/platform_info_test.c |   3 +-
> >  .../kvm/x86_64/set_memory_region_test.c       |   3 +-
> >  .../testing/selftests/kvm/x86_64/state_test.c |   4 +-
> >  .../selftests/kvm/x86_64/svm_vmcall_test.c    |   3 +-
> >  .../selftests/kvm/x86_64/sync_regs_test.c     |   4 +-
> >  .../selftests/kvm/x86_64/vmx_dirty_log_test.c |   2 +-
> >  .../kvm/x86_64/vmx_set_nested_state_test.c    |   4 +-
> >  .../selftests/kvm/x86_64/xss_msr_test.c       |   2 +-
> >  28 files changed, 243 insertions(+), 292 deletions(-)
> > 
>
Christian Borntraeger March 10, 2020, 12:29 p.m. UTC | #3
I get the following with your patches.


In file included from s390x/sync_regs_test.c:21:
s390x/sync_regs_test.c: In function ‘compare_sregs’:
s390x/sync_regs_test.c:41:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
   41 |       "Register " #reg \
      |       ^~~~~~~~~~~
   42 |       " values did not match: 0x%llx, 0x%llx\n", \
   43 |       left->reg, right->reg)
      |       ~~~~~~~~~~~~~~~~~~~~~~
   44 | 
      |        
   45 | static void compare_regs(struct kvm_regs *left, struct kvm_sync_regs *right)
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   46 | {
      | ~      
   47 |  int i;
      |  ~~~~~~
   48 | 
      |        
   49 |  for (i = 0; i < 16; i++)
      |  ~~~~~~~~~~~~~~~~~~~~~~~~
   50 |   REG_COMPARE(gprs[i]);
      |   ~~~~~~~~~~~~~~~~~~~~~
   51 | }
      | ~      
   52 | 
      |        
   53 | static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   54 | {
      | ~      
   55 |  int i;
      |  ~~~~~~
   56 | 
      |        
   57 |  for (i = 0; i < 16; i++)
      |  ~~~~~~~~~~~~~~~~~~~~~~~~
   58 |   REG_COMPARE(acrs[i]);
      |   ~~~~~~~~~~~~~~~~~~~
      |                   |
      |                   __u32 {aka unsigned int}
include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
   46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
      |                                           ^~~
s390x/sync_regs_test.c:58:3: note: in expansion of macro ‘REG_COMPARE’
   58 |   REG_COMPARE(acrs[i]);
      |   ^~~~~~~~~~~
s390x/sync_regs_test.c:41:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 7 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
   41 |       "Register " #reg \
      |       ^~~~~~~~~~~
   42 |       " values did not match: 0x%llx, 0x%llx\n", \
   43 |       left->reg, right->reg)
      |                  ~~~~~~~~~~~
   44 | 
      |        
   45 | static void compare_regs(struct kvm_regs *left, struct kvm_sync_regs *right)
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   46 | {
      | ~      
   47 |  int i;
      |  ~~~~~~
   48 | 
      |        
   49 |  for (i = 0; i < 16; i++)
      |  ~~~~~~~~~~~~~~~~~~~~~~~~
   50 |   REG_COMPARE(gprs[i]);
      |   ~~~~~~~~~~~~~~~~~~~~~
   51 | }
      | ~      
   52 | 
      |        
   53 | static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   54 | {
      | ~      
   55 |  int i;
      |  ~~~~~~
   56 | 
      |        
   57 |  for (i = 0; i < 16; i++)
      |  ~~~~~~~~~~~~~~~~~~~~~~~~
   58 |   REG_COMPARE(acrs[i]);
      |   ~~~~~~~~~~~~~~~~~~~
      |                   |
      |                   __u32 {aka unsigned int}
include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
   46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
      |                                           ^~~
s390x/sync_regs_test.c:58:3: note: in expansion of macro ‘REG_COMPARE’
   58 |   REG_COMPARE(acrs[i]);
      |   ^~~~~~~~~~~
s390x/sync_regs_test.c: In function ‘main’:
s390x/sync_regs_test.c:158:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
  158 |       "acr0 sync regs value incorrect 0x%llx.",
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  159 |       run->s.regs.acrs[0]);
      |       ~~~~~~~~~~~~~~~~~~~
      |                       |
      |                       __u32 {aka unsigned int}
include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
   46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
      |                                           ^~~
s390x/sync_regs_test.c:158:44: note: format string is defined here
  158 |       "acr0 sync regs value incorrect 0x%llx.",
      |                                         ~~~^
      |                                            |
      |                                            long long unsigned int
      |      



On 10.03.20 12:58, Andrew Jones wrote:
> On Tue, Mar 10, 2020 at 10:45:43AM +0100, Christian Borntraeger wrote:
>> On 10.03.20 10:15, Andrew Jones wrote:
>>>
>>> Andrew Jones (4):
>>>   fixup! selftests: KVM: SVM: Add vmcall test
>>>   KVM: selftests: Share common API documentation
>>>   KVM: selftests: Enable printf format warnings for TEST_ASSERT
>>>   KVM: selftests: Use consistent message for test skipping
>>
>> This looks like a nice cleanup but this does not seem to apply
>> cleanly on kvm/master or linus/master. Which tree is this based on?
> 
> This is based on kvm/queue. Sorry, I should have mentioned that in
> the cover letter.
> 
> Thanks,
> drew
> 
>>
>>>
>>>  tools/testing/selftests/kvm/.gitignore        |   5 +-
>>>  .../selftests/kvm/demand_paging_test.c        |   6 +-
>>>  tools/testing/selftests/kvm/dirty_log_test.c  |   3 +-
>>>  .../testing/selftests/kvm/include/kvm_util.h  | 100 ++++++++-
>>>  .../testing/selftests/kvm/include/test_util.h |   5 +-
>>>  .../selftests/kvm/lib/aarch64/processor.c     |  17 --
>>>  tools/testing/selftests/kvm/lib/assert.c      |   6 +-
>>>  tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +-
>>>  .../selftests/kvm/lib/kvm_util_internal.h     |  48 +++++
>>>  .../selftests/kvm/lib/s390x/processor.c       |  74 -------
>>>  tools/testing/selftests/kvm/lib/test_util.c   |  12 ++
>>>  .../selftests/kvm/lib/x86_64/processor.c      | 196 ++++--------------
>>>  tools/testing/selftests/kvm/lib/x86_64/svm.c  |   2 +-
>>>  tools/testing/selftests/kvm/lib/x86_64/vmx.c  |   2 +-
>>>  tools/testing/selftests/kvm/s390x/memop.c     |   2 +-
>>>  .../selftests/kvm/s390x/sync_regs_test.c      |   2 +-
>>>  .../kvm/x86_64/cr4_cpuid_sync_test.c          |   2 +-
>>>  .../testing/selftests/kvm/x86_64/evmcs_test.c |   6 +-
>>>  .../selftests/kvm/x86_64/hyperv_cpuid.c       |   8 +-
>>>  .../selftests/kvm/x86_64/mmio_warning_test.c  |   4 +-
>>>  .../selftests/kvm/x86_64/platform_info_test.c |   3 +-
>>>  .../kvm/x86_64/set_memory_region_test.c       |   3 +-
>>>  .../testing/selftests/kvm/x86_64/state_test.c |   4 +-
>>>  .../selftests/kvm/x86_64/svm_vmcall_test.c    |   3 +-
>>>  .../selftests/kvm/x86_64/sync_regs_test.c     |   4 +-
>>>  .../selftests/kvm/x86_64/vmx_dirty_log_test.c |   2 +-
>>>  .../kvm/x86_64/vmx_set_nested_state_test.c    |   4 +-
>>>  .../selftests/kvm/x86_64/xss_msr_test.c       |   2 +-
>>>  28 files changed, 243 insertions(+), 292 deletions(-)
>>>
>>
>
Christian Borntraeger March 10, 2020, 12:34 p.m. UTC | #4
On 10.03.20 13:29, Christian Borntraeger wrote:
> I get the following with your patches.

And those errors have been there before. 
I will provide fixups for sync_regs_test.c and reset.c. So you patch set really has value.


> 
> 
> In file included from s390x/sync_regs_test.c:21:
> s390x/sync_regs_test.c: In function ‘compare_sregs’:
> s390x/sync_regs_test.c:41:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
>    41 |       "Register " #reg \
>       |       ^~~~~~~~~~~
>    42 |       " values did not match: 0x%llx, 0x%llx\n", \
>    43 |       left->reg, right->reg)
>       |       ~~~~~~~~~~~~~~~~~~~~~~
>    44 | 
>       |        
>    45 | static void compare_regs(struct kvm_regs *left, struct kvm_sync_regs *right)
>       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    46 | {
>       | ~      
>    47 |  int i;
>       |  ~~~~~~
>    48 | 
>       |        
>    49 |  for (i = 0; i < 16; i++)
>       |  ~~~~~~~~~~~~~~~~~~~~~~~~
>    50 |   REG_COMPARE(gprs[i]);
>       |   ~~~~~~~~~~~~~~~~~~~~~
>    51 | }
>       | ~      
>    52 | 
>       |        
>    53 | static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
>       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    54 | {
>       | ~      
>    55 |  int i;
>       |  ~~~~~~
>    56 | 
>       |        
>    57 |  for (i = 0; i < 16; i++)
>       |  ~~~~~~~~~~~~~~~~~~~~~~~~
>    58 |   REG_COMPARE(acrs[i]);
>       |   ~~~~~~~~~~~~~~~~~~~
>       |                   |
>       |                   __u32 {aka unsigned int}
> include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
>    46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>       |                                           ^~~
> s390x/sync_regs_test.c:58:3: note: in expansion of macro ‘REG_COMPARE’
>    58 |   REG_COMPARE(acrs[i]);
>       |   ^~~~~~~~~~~
> s390x/sync_regs_test.c:41:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 7 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
>    41 |       "Register " #reg \
>       |       ^~~~~~~~~~~
>    42 |       " values did not match: 0x%llx, 0x%llx\n", \
>    43 |       left->reg, right->reg)
>       |                  ~~~~~~~~~~~
>    44 | 
>       |        
>    45 | static void compare_regs(struct kvm_regs *left, struct kvm_sync_regs *right)
>       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    46 | {
>       | ~      
>    47 |  int i;
>       |  ~~~~~~
>    48 | 
>       |        
>    49 |  for (i = 0; i < 16; i++)
>       |  ~~~~~~~~~~~~~~~~~~~~~~~~
>    50 |   REG_COMPARE(gprs[i]);
>       |   ~~~~~~~~~~~~~~~~~~~~~
>    51 | }
>       | ~      
>    52 | 
>       |        
>    53 | static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
>       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    54 | {
>       | ~      
>    55 |  int i;
>       |  ~~~~~~
>    56 | 
>       |        
>    57 |  for (i = 0; i < 16; i++)
>       |  ~~~~~~~~~~~~~~~~~~~~~~~~
>    58 |   REG_COMPARE(acrs[i]);
>       |   ~~~~~~~~~~~~~~~~~~~
>       |                   |
>       |                   __u32 {aka unsigned int}
> include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
>    46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>       |                                           ^~~
> s390x/sync_regs_test.c:58:3: note: in expansion of macro ‘REG_COMPARE’
>    58 |   REG_COMPARE(acrs[i]);
>       |   ^~~~~~~~~~~
> s390x/sync_regs_test.c: In function ‘main’:
> s390x/sync_regs_test.c:158:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
>   158 |       "acr0 sync regs value incorrect 0x%llx.",
>       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   159 |       run->s.regs.acrs[0]);
>       |       ~~~~~~~~~~~~~~~~~~~
>       |                       |
>       |                       __u32 {aka unsigned int}
> include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
>    46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
>       |                                           ^~~
> s390x/sync_regs_test.c:158:44: note: format string is defined here
>   158 |       "acr0 sync regs value incorrect 0x%llx.",
>       |                                         ~~~^
>       |                                            |
>       |                                            long long unsigned int
>       |      
> 
> 
> 
> On 10.03.20 12:58, Andrew Jones wrote:
>> On Tue, Mar 10, 2020 at 10:45:43AM +0100, Christian Borntraeger wrote:
>>> On 10.03.20 10:15, Andrew Jones wrote:
>>>>
>>>> Andrew Jones (4):
>>>>   fixup! selftests: KVM: SVM: Add vmcall test
>>>>   KVM: selftests: Share common API documentation
>>>>   KVM: selftests: Enable printf format warnings for TEST_ASSERT
>>>>   KVM: selftests: Use consistent message for test skipping
>>>
>>> This looks like a nice cleanup but this does not seem to apply
>>> cleanly on kvm/master or linus/master. Which tree is this based on?
>>
>> This is based on kvm/queue. Sorry, I should have mentioned that in
>> the cover letter.
>>
>> Thanks,
>> drew
>>
>>>
>>>>
>>>>  tools/testing/selftests/kvm/.gitignore        |   5 +-
>>>>  .../selftests/kvm/demand_paging_test.c        |   6 +-
>>>>  tools/testing/selftests/kvm/dirty_log_test.c  |   3 +-
>>>>  .../testing/selftests/kvm/include/kvm_util.h  | 100 ++++++++-
>>>>  .../testing/selftests/kvm/include/test_util.h |   5 +-
>>>>  .../selftests/kvm/lib/aarch64/processor.c     |  17 --
>>>>  tools/testing/selftests/kvm/lib/assert.c      |   6 +-
>>>>  tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +-
>>>>  .../selftests/kvm/lib/kvm_util_internal.h     |  48 +++++
>>>>  .../selftests/kvm/lib/s390x/processor.c       |  74 -------
>>>>  tools/testing/selftests/kvm/lib/test_util.c   |  12 ++
>>>>  .../selftests/kvm/lib/x86_64/processor.c      | 196 ++++--------------
>>>>  tools/testing/selftests/kvm/lib/x86_64/svm.c  |   2 +-
>>>>  tools/testing/selftests/kvm/lib/x86_64/vmx.c  |   2 +-
>>>>  tools/testing/selftests/kvm/s390x/memop.c     |   2 +-
>>>>  .../selftests/kvm/s390x/sync_regs_test.c      |   2 +-
>>>>  .../kvm/x86_64/cr4_cpuid_sync_test.c          |   2 +-
>>>>  .../testing/selftests/kvm/x86_64/evmcs_test.c |   6 +-
>>>>  .../selftests/kvm/x86_64/hyperv_cpuid.c       |   8 +-
>>>>  .../selftests/kvm/x86_64/mmio_warning_test.c  |   4 +-
>>>>  .../selftests/kvm/x86_64/platform_info_test.c |   3 +-
>>>>  .../kvm/x86_64/set_memory_region_test.c       |   3 +-
>>>>  .../testing/selftests/kvm/x86_64/state_test.c |   4 +-
>>>>  .../selftests/kvm/x86_64/svm_vmcall_test.c    |   3 +-
>>>>  .../selftests/kvm/x86_64/sync_regs_test.c     |   4 +-
>>>>  .../selftests/kvm/x86_64/vmx_dirty_log_test.c |   2 +-
>>>>  .../kvm/x86_64/vmx_set_nested_state_test.c    |   4 +-
>>>>  .../selftests/kvm/x86_64/xss_msr_test.c       |   2 +-
>>>>  28 files changed, 243 insertions(+), 292 deletions(-)
>>>>
>>>
>>
>
Andrew Jones March 10, 2020, 1:18 p.m. UTC | #5
On Tue, Mar 10, 2020 at 01:34:54PM +0100, Christian Borntraeger wrote:
> On 10.03.20 13:29, Christian Borntraeger wrote:
> > I get the following with your patches.
> 
> And those errors have been there before. 
> I will provide fixups for sync_regs_test.c and reset.c. So you patch set really has value.

Yeah, another thing I intended to say in the cover-letter, but forgot,
was that I hadn't tested on s390x, and that there was a chance
TEST_ASSERT warnings would be found.

Thanks for fixing them!

drew

> 
> 
> > 
> > 
> > In file included from s390x/sync_regs_test.c:21:
> > s390x/sync_regs_test.c: In function ‘compare_sregs’:
> > s390x/sync_regs_test.c:41:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
> >    41 |       "Register " #reg \
> >       |       ^~~~~~~~~~~
> >    42 |       " values did not match: 0x%llx, 0x%llx\n", \
> >    43 |       left->reg, right->reg)
> >       |       ~~~~~~~~~~~~~~~~~~~~~~
> >    44 | 
> >       |        
> >    45 | static void compare_regs(struct kvm_regs *left, struct kvm_sync_regs *right)
> >       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    46 | {
> >       | ~      
> >    47 |  int i;
> >       |  ~~~~~~
> >    48 | 
> >       |        
> >    49 |  for (i = 0; i < 16; i++)
> >       |  ~~~~~~~~~~~~~~~~~~~~~~~~
> >    50 |   REG_COMPARE(gprs[i]);
> >       |   ~~~~~~~~~~~~~~~~~~~~~
> >    51 | }
> >       | ~      
> >    52 | 
> >       |        
> >    53 | static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
> >       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    54 | {
> >       | ~      
> >    55 |  int i;
> >       |  ~~~~~~
> >    56 | 
> >       |        
> >    57 |  for (i = 0; i < 16; i++)
> >       |  ~~~~~~~~~~~~~~~~~~~~~~~~
> >    58 |   REG_COMPARE(acrs[i]);
> >       |   ~~~~~~~~~~~~~~~~~~~
> >       |                   |
> >       |                   __u32 {aka unsigned int}
> > include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
> >    46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
> >       |                                           ^~~
> > s390x/sync_regs_test.c:58:3: note: in expansion of macro ‘REG_COMPARE’
> >    58 |   REG_COMPARE(acrs[i]);
> >       |   ^~~~~~~~~~~
> > s390x/sync_regs_test.c:41:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 7 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
> >    41 |       "Register " #reg \
> >       |       ^~~~~~~~~~~
> >    42 |       " values did not match: 0x%llx, 0x%llx\n", \
> >    43 |       left->reg, right->reg)
> >       |                  ~~~~~~~~~~~
> >    44 | 
> >       |        
> >    45 | static void compare_regs(struct kvm_regs *left, struct kvm_sync_regs *right)
> >       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    46 | {
> >       | ~      
> >    47 |  int i;
> >       |  ~~~~~~
> >    48 | 
> >       |        
> >    49 |  for (i = 0; i < 16; i++)
> >       |  ~~~~~~~~~~~~~~~~~~~~~~~~
> >    50 |   REG_COMPARE(gprs[i]);
> >       |   ~~~~~~~~~~~~~~~~~~~~~
> >    51 | }
> >       | ~      
> >    52 | 
> >       |        
> >    53 | static void compare_sregs(struct kvm_sregs *left, struct kvm_sync_regs *right)
> >       | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    54 | {
> >       | ~      
> >    55 |  int i;
> >       |  ~~~~~~
> >    56 | 
> >       |        
> >    57 |  for (i = 0; i < 16; i++)
> >       |  ~~~~~~~~~~~~~~~~~~~~~~~~
> >    58 |   REG_COMPARE(acrs[i]);
> >       |   ~~~~~~~~~~~~~~~~~~~
> >       |                   |
> >       |                   __u32 {aka unsigned int}
> > include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
> >    46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
> >       |                                           ^~~
> > s390x/sync_regs_test.c:58:3: note: in expansion of macro ‘REG_COMPARE’
> >    58 |   REG_COMPARE(acrs[i]);
> >       |   ^~~~~~~~~~~
> > s390x/sync_regs_test.c: In function ‘main’:
> > s390x/sync_regs_test.c:158:7: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 6 has type ‘__u32’ {aka ‘unsigned int’} [-Wformat=]
> >   158 |       "acr0 sync regs value incorrect 0x%llx.",
> >       |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   159 |       run->s.regs.acrs[0]);
> >       |       ~~~~~~~~~~~~~~~~~~~
> >       |                       |
> >       |                       __u32 {aka unsigned int}
> > include/test_util.h:46:43: note: in definition of macro ‘TEST_ASSERT’
> >    46 |  test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
> >       |                                           ^~~
> > s390x/sync_regs_test.c:158:44: note: format string is defined here
> >   158 |       "acr0 sync regs value incorrect 0x%llx.",
> >       |                                         ~~~^
> >       |                                            |
> >       |                                            long long unsigned int
> >       |      
> > 
> > 
> > 
> > On 10.03.20 12:58, Andrew Jones wrote:
> >> On Tue, Mar 10, 2020 at 10:45:43AM +0100, Christian Borntraeger wrote:
> >>> On 10.03.20 10:15, Andrew Jones wrote:
> >>>>
> >>>> Andrew Jones (4):
> >>>>   fixup! selftests: KVM: SVM: Add vmcall test
> >>>>   KVM: selftests: Share common API documentation
> >>>>   KVM: selftests: Enable printf format warnings for TEST_ASSERT
> >>>>   KVM: selftests: Use consistent message for test skipping
> >>>
> >>> This looks like a nice cleanup but this does not seem to apply
> >>> cleanly on kvm/master or linus/master. Which tree is this based on?
> >>
> >> This is based on kvm/queue. Sorry, I should have mentioned that in
> >> the cover letter.
> >>
> >> Thanks,
> >> drew
> >>
> >>>
> >>>>
> >>>>  tools/testing/selftests/kvm/.gitignore        |   5 +-
> >>>>  .../selftests/kvm/demand_paging_test.c        |   6 +-
> >>>>  tools/testing/selftests/kvm/dirty_log_test.c  |   3 +-
> >>>>  .../testing/selftests/kvm/include/kvm_util.h  | 100 ++++++++-
> >>>>  .../testing/selftests/kvm/include/test_util.h |   5 +-
> >>>>  .../selftests/kvm/lib/aarch64/processor.c     |  17 --
> >>>>  tools/testing/selftests/kvm/lib/assert.c      |   6 +-
> >>>>  tools/testing/selftests/kvm/lib/kvm_util.c    |  10 +-
> >>>>  .../selftests/kvm/lib/kvm_util_internal.h     |  48 +++++
> >>>>  .../selftests/kvm/lib/s390x/processor.c       |  74 -------
> >>>>  tools/testing/selftests/kvm/lib/test_util.c   |  12 ++
> >>>>  .../selftests/kvm/lib/x86_64/processor.c      | 196 ++++--------------
> >>>>  tools/testing/selftests/kvm/lib/x86_64/svm.c  |   2 +-
> >>>>  tools/testing/selftests/kvm/lib/x86_64/vmx.c  |   2 +-
> >>>>  tools/testing/selftests/kvm/s390x/memop.c     |   2 +-
> >>>>  .../selftests/kvm/s390x/sync_regs_test.c      |   2 +-
> >>>>  .../kvm/x86_64/cr4_cpuid_sync_test.c          |   2 +-
> >>>>  .../testing/selftests/kvm/x86_64/evmcs_test.c |   6 +-
> >>>>  .../selftests/kvm/x86_64/hyperv_cpuid.c       |   8 +-
> >>>>  .../selftests/kvm/x86_64/mmio_warning_test.c  |   4 +-
> >>>>  .../selftests/kvm/x86_64/platform_info_test.c |   3 +-
> >>>>  .../kvm/x86_64/set_memory_region_test.c       |   3 +-
> >>>>  .../testing/selftests/kvm/x86_64/state_test.c |   4 +-
> >>>>  .../selftests/kvm/x86_64/svm_vmcall_test.c    |   3 +-
> >>>>  .../selftests/kvm/x86_64/sync_regs_test.c     |   4 +-
> >>>>  .../selftests/kvm/x86_64/vmx_dirty_log_test.c |   2 +-
> >>>>  .../kvm/x86_64/vmx_set_nested_state_test.c    |   4 +-
> >>>>  .../selftests/kvm/x86_64/xss_msr_test.c       |   2 +-
> >>>>  28 files changed, 243 insertions(+), 292 deletions(-)
> >>>>
> >>>
> >>
> > 
>
Peter Xu March 11, 2020, 7:29 p.m. UTC | #6
On Tue, Mar 10, 2020 at 10:15:52AM +0100, Andrew Jones wrote:
> 
> Andrew Jones (4):
>   fixup! selftests: KVM: SVM: Add vmcall test
>   KVM: selftests: Share common API documentation
>   KVM: selftests: Enable printf format warnings for TEST_ASSERT
>   KVM: selftests: Use consistent message for test skipping

Reviewed-by: Peter Xu <peterx@redhat.com>

I'll rebase my further tests against this too.  Thanks!
Paolo Bonzini March 14, 2020, 11:44 a.m. UTC | #7
On 11/03/20 20:29, Peter Xu wrote:
> On Tue, Mar 10, 2020 at 10:15:52AM +0100, Andrew Jones wrote:
>>
>> Andrew Jones (4):
>>   fixup! selftests: KVM: SVM: Add vmcall test
>>   KVM: selftests: Share common API documentation
>>   KVM: selftests: Enable printf format warnings for TEST_ASSERT
>>   KVM: selftests: Use consistent message for test skipping
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> I'll rebase my further tests against this too.  Thanks!
> 

Queued, thanks.

Paolo