linux-kernel - Re: [Patch v3 12/13] KVM: selftests: Make vCPU exit reason test assertion common.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y5PlW+U2MblIObHT@google.com>
Date:   Sat, 10 Dec 2022 01:48:11 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     David Matlack <dmatlack@...gle.com>
Cc:     Vipin Sharma <vipinsh@...gle.com>, pbonzini@...hat.com,
        vkuznets@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Patch v3 12/13] KVM: selftests: Make vCPU exit reason test
 assertion common.

On Fri, Dec 09, 2022, David Matlack wrote:
> On Mon, Dec 05, 2022 at 11:14:29AM -0800, Vipin Sharma wrote:
> > Make ASSERT_EXIT_REASON() macro and replace all exit reason test assert
> > statements with it.
> > 
> > No functional changes intended.
> > 
> > Suggested-by: David Matlack <dmatlack@...gle.com>
> > Signed-off-by: Vipin Sharma <vipinsh@...gle.com>
> 
> Reviewed-by: David Matlack <dmatlack@...gle.com>
> 
> > ---
> >  .../testing/selftests/kvm/aarch64/psci_test.c |  4 +--
> >  .../testing/selftests/kvm/include/test_util.h | 10 ++++++++
> [...]
> >  .../selftests/kvm/x86_64/xapic_ipi_test.c     |  6 +----
> >  .../selftests/kvm/x86_64/xen_shinfo_test.c    |  7 +-----
> >  .../selftests/kvm/x86_64/xen_vmcall_test.c    |  5 +---
> >  44 files changed, 71 insertions(+), 293 deletions(-)
> 
> Nice diff stat :)

I like the diffstat too, but I think we need a slightly different name for the
macro.  "EXIT_REASON" can be interpreted as "hardware exit reason" or "KVM exit
(to userspace) reason".  Most usage will be fairly obvious, but I'd like to avoid
confusion when swapping between selftests and KVM-unit-tests (which often asserts
on the hardware exit reason).   The name will be a bit longer, but I don't think
line length with ever be a problem.  And most people will just copy+paste so finger
fatigue likeliy won't be an issue either.

I also vote to prefix it with TEST, i.e. TEST_ASSERT_KVM_EXIT_REASON(), for
consistency and to make it very clear it's a TEST_ASSERT() wrapper.  ASSERT_EQ()
makes me twitch every time I see it.  This is definitely a personal preference 
(or problem) I can survive with though :-)