From: Peter Xu <peterx@redhat.com>
To: Andrew Jones <drjones@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
Yan Zhao <yan.y.zhao@intel.com>, Jason Wang <jasowang@redhat.com>,
Alex Williamson <alex.williamson@redhat.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
Christophe de Dinechin <dinechin@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
Kevin Tian <kevin.tian@intel.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [PATCH v6 10/14] KVM: selftests: Use a single binary for dirty/clear log test
Date: Wed, 11 Mar 2020 13:43:24 -0400 [thread overview]
Message-ID: <20200311174324.GH479302@xz-x1> (raw)
In-Reply-To: <20200310081002.unxq6kwlevmr6m3b@kamzik.brq.redhat.com>
On Tue, Mar 10, 2020 at 09:10:02AM +0100, Andrew Jones wrote:
> On Mon, Mar 09, 2020 at 06:25:19PM -0400, Peter Xu wrote:
> > Remove the clear_dirty_log test, instead merge it into the existing
> > dirty_log_test. It should be cleaner to use this single binary to do
> > both tests, also it's a preparation for the upcoming dirty ring test.
> >
> > The default behavior will run all the modes in sequence.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > tools/testing/selftests/kvm/Makefile | 2 -
> > .../selftests/kvm/clear_dirty_log_test.c | 2 -
> > tools/testing/selftests/kvm/dirty_log_test.c | 169 +++++++++++++++---
> > 3 files changed, 146 insertions(+), 27 deletions(-)
> > delete mode 100644 tools/testing/selftests/kvm/clear_dirty_log_test.c
> >
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index d91c53b726e6..941bfcd48eaa 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -27,11 +27,9 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_set_nested_state_test
> > TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
> > TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
> > TEST_GEN_PROGS_x86_64 += x86_64/svm_vmcall_test
> > -TEST_GEN_PROGS_x86_64 += clear_dirty_log_test
> > TEST_GEN_PROGS_x86_64 += dirty_log_test
> > TEST_GEN_PROGS_x86_64 += kvm_create_max_vcpus
> >
> > -TEST_GEN_PROGS_aarch64 += clear_dirty_log_test
> > TEST_GEN_PROGS_aarch64 += dirty_log_test
> > TEST_GEN_PROGS_aarch64 += kvm_create_max_vcpus
> >
> > diff --git a/tools/testing/selftests/kvm/clear_dirty_log_test.c b/tools/testing/selftests/kvm/clear_dirty_log_test.c
> > deleted file mode 100644
> > index 749336937d37..000000000000
> > --- a/tools/testing/selftests/kvm/clear_dirty_log_test.c
> > +++ /dev/null
> > @@ -1,2 +0,0 @@
> > -#define USE_CLEAR_DIRTY_LOG
> > -#include "dirty_log_test.c"
> > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> > index 3c0ffd34b3b0..642886394e34 100644
> > --- a/tools/testing/selftests/kvm/dirty_log_test.c
> > +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> > @@ -128,6 +128,73 @@ static uint64_t host_dirty_count;
> > static uint64_t host_clear_count;
> > static uint64_t host_track_next_count;
> >
> > +enum log_mode_t {
> > + /* Only use KVM_GET_DIRTY_LOG for logging */
> > + LOG_MODE_DIRTY_LOG = 0,
> > +
> > + /* Use both KVM_[GET|CLEAR]_DIRTY_LOG for logging */
> > + LOG_MODE_CLEAR_LOG = 1,
> > +
> > + LOG_MODE_NUM,
> > +
> > + /* Run all supported modes */
> > + LOG_MODE_ALL = LOG_MODE_NUM,
> > +};
> > +
> > +/* Mode of logging to test. Default is to run all supported modes */
> > +static enum log_mode_t host_log_mode_option = LOG_MODE_ALL;
> > +/* Logging mode for current run */
> > +static enum log_mode_t host_log_mode;
> > +
> > +static bool clear_log_supported(void)
> > +{
> > + return kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2);
> > +}
> > +
> > +static void clear_log_create_vm_done(struct kvm_vm *vm)
> > +{
> > + struct kvm_enable_cap cap = {};
> > +
> > + cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2;
> > + cap.args[0] = 1;
> > + vm_enable_cap(vm, &cap);
> > +}
> > +
> > +static void dirty_log_collect_dirty_pages(struct kvm_vm *vm, int slot,
> > + void *bitmap, uint32_t num_pages)
> > +{
> > + kvm_vm_get_dirty_log(vm, slot, bitmap);
> > +}
> > +
> > +static void clear_log_collect_dirty_pages(struct kvm_vm *vm, int slot,
> > + void *bitmap, uint32_t num_pages)
> > +{
> > + kvm_vm_get_dirty_log(vm, slot, bitmap);
> > + kvm_vm_clear_dirty_log(vm, slot, bitmap, 0, num_pages);
> > +}
> > +
> > +struct log_mode {
> > + const char *name;
> > + /* Return true if this mode is supported, otherwise false */
> > + bool (*supported)(void);
> > + /* Hook when the vm creation is done (before vcpu creation) */
> > + void (*create_vm_done)(struct kvm_vm *vm);
> > + /* Hook to collect the dirty pages into the bitmap provided */
> > + void (*collect_dirty_pages) (struct kvm_vm *vm, int slot,
> > + void *bitmap, uint32_t num_pages);
> > +} log_modes[LOG_MODE_NUM] = {
> > + {
> > + .name = "dirty-log",
> > + .collect_dirty_pages = dirty_log_collect_dirty_pages,
> > + },
> > + {
> > + .name = "clear-log",
> > + .supported = clear_log_supported,
> > + .create_vm_done = clear_log_create_vm_done,
> > + .collect_dirty_pages = clear_log_collect_dirty_pages,
> > + },
> > +};
> > +
> > /*
> > * We use this bitmap to track some pages that should have its dirty
> > * bit set in the _next_ iteration. For example, if we detected the
> > @@ -137,6 +204,43 @@ static uint64_t host_track_next_count;
> > */
> > static unsigned long *host_bmap_track;
> >
> > +static void log_modes_dump(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < LOG_MODE_NUM; i++)
> > + printf("%s, ", log_modes[i].name);
> > + puts("\b\b \b\b");
>
> This will be ugly when the output is redirected to a file.
> How about just
>
> printf("%s", log_modes[0].name);
> for (i = 1; i < LOG_MODE_NUM; i++)
> printf(", %s", log_modes[i].name);
> printf("\n");
Will do.
>
> > +}
> > +
> > +static bool log_mode_supported(void)
> > +{
> > + struct log_mode *mode = &log_modes[host_log_mode];
> > +
> > + if (mode->supported)
> > + return mode->supported();
> > +
> > + return true;
> > +}
> > +
> > +static void log_mode_create_vm_done(struct kvm_vm *vm)
> > +{
> > + struct log_mode *mode = &log_modes[host_log_mode];
> > +
> > + if (mode->create_vm_done)
> > + mode->create_vm_done(vm);
> > +}
> > +
> > +static void log_mode_collect_dirty_pages(struct kvm_vm *vm, int slot,
> > + void *bitmap, uint32_t num_pages)
> > +{
> > + struct log_mode *mode = &log_modes[host_log_mode];
> > +
> > + TEST_ASSERT(mode->collect_dirty_pages != NULL,
> > + "collect_dirty_pages() is required for any log mode!");
> > + mode->collect_dirty_pages(vm, slot, bitmap, num_pages);
> > +}
> > +
> > static void generate_random_array(uint64_t *guest_array, uint64_t size)
> > {
> > uint64_t i;
> > @@ -257,6 +361,7 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, uint32_t vcpuid,
> > #ifdef __x86_64__
> > vm_create_irqchip(vm);
> > #endif
> > + log_mode_create_vm_done(vm);
> > vm_vcpu_add_default(vm, vcpuid, guest_code);
> > return vm;
> > }
> > @@ -271,6 +376,12 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
> > struct kvm_vm *vm;
> > unsigned long *bmap;
> >
> > + if (!log_mode_supported()) {
> > + fprintf(stderr, "Log mode '%s' not supported, skip\n",
> > + log_modes[host_log_mode].name);
>
> I think kvm selftests needs a skip_test() function that outputs a more
> consistent test skip message. It seems we mostly do
Yep, I can introduce one.
>
> fprintf(stderr, "%s, skipping test\n", custom_message);
>
> but here we have ', skip'. Also, I see a few places were we output
> skipping to stderr and others to stdout. I think I like stdout better.
Sure.
>
> > + return;
> > + }
> > +
> > /*
> > * We reserve page table for 2 times of extra dirty mem which
> > * will definitely cover the original (1G+) test range. Here
> > @@ -316,14 +427,6 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
> > bmap = bitmap_alloc(host_num_pages);
> > host_bmap_track = bitmap_alloc(host_num_pages);
> >
> > -#ifdef USE_CLEAR_DIRTY_LOG
> > - struct kvm_enable_cap cap = {};
> > -
> > - cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2;
> > - cap.args[0] = 1;
> > - vm_enable_cap(vm, &cap);
> > -#endif
> > -
> > /* Add an extra memory slot for testing dirty logging */
> > vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> > guest_test_phys_mem,
> > @@ -364,11 +467,8 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
> > while (iteration < iterations) {
> > /* Give the vcpu thread some time to dirty some pages */
> > usleep(interval * 1000);
> > - kvm_vm_get_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap);
> > -#ifdef USE_CLEAR_DIRTY_LOG
> > - kvm_vm_clear_dirty_log(vm, TEST_MEM_SLOT_INDEX, bmap, 0,
> > - host_num_pages);
> > -#endif
> > + log_mode_collect_dirty_pages(vm, TEST_MEM_SLOT_INDEX,
> > + bmap, host_num_pages);
> > vm_dirty_log_verify(bmap);
> > iteration++;
> > sync_global_to_guest(vm, iteration);
> > @@ -413,6 +513,9 @@ static void help(char *name)
> > TEST_HOST_LOOP_INTERVAL);
> > printf(" -p: specify guest physical test memory offset\n"
> > " Warning: a low offset can conflict with the loaded test code.\n");
> > + printf(" -M: specify the host logging mode "
> > + "(default: run all log modes). Supported modes: \n\t");
> > + log_modes_dump();
> > printf(" -m: specify the guest mode ID to test "
> > "(default: test all supported modes)\n"
> > " This option may be used multiple times.\n"
> > @@ -432,18 +535,11 @@ int main(int argc, char *argv[])
> > bool mode_selected = false;
> > uint64_t phys_offset = 0;
> > unsigned int mode;
> > - int opt, i;
> > + int opt, i, j;
> > #ifdef __aarch64__
> > unsigned int host_ipa_limit;
> > #endif
> >
> > -#ifdef USE_CLEAR_DIRTY_LOG
> > - if (!kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2)) {
> > - fprintf(stderr, "KVM_CLEAR_DIRTY_LOG not available, skipping tests\n");
> > - exit(KSFT_SKIP);
> > - }
> > -#endif
> > -
> > #ifdef __x86_64__
> > vm_guest_mode_params_init(VM_MODE_PXXV48_4K, true, true);
> > #endif
> > @@ -463,7 +559,7 @@ int main(int argc, char *argv[])
> > vm_guest_mode_params_init(VM_MODE_P40V48_4K, true, true);
> > #endif
> >
> > - while ((opt = getopt(argc, argv, "hi:I:p:m:")) != -1) {
> > + while ((opt = getopt(argc, argv, "hi:I:p:m:M:")) != -1) {
> > switch (opt) {
> > case 'i':
> > iterations = strtol(optarg, NULL, 10);
> > @@ -485,6 +581,22 @@ int main(int argc, char *argv[])
> > "Guest mode ID %d too big", mode);
> > vm_guest_mode_params[mode].enabled = true;
> > break;
> > + case 'M':
>
> Can also add
>
> if (!strcmp(optarg, "all"))
> host_log_mode_option = LOG_MODE_ALL;
Sure.
>
> > + for (i = 0; i < LOG_MODE_NUM; i++) {
> > + if (!strcmp(optarg, log_modes[i].name)) {
> > + DEBUG("Setting log mode to: '%s'\n",
> > + optarg);
>
> Basing this on kvm/queue won't work as DEBUG() no longer exists. This
> looks like a pr_info().
I'll rebase to kvm/queue and see...
>
> > + host_log_mode_option = i;
> > + break;
> > + }
> > + }
> > + if (i == LOG_MODE_NUM) {
> > + printf("Log mode '%s' is invalid. "
> > + "Please choose from: ", optarg);
> > + log_modes_dump();
> > + exit(-1);
>
> Exit code of 255? Probably just want exit(1);
Sure. Thanks!
--
Peter Xu
next prev parent reply other threads:[~2020-03-11 17:43 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-09 21:44 [PATCH v6 00/14] KVM: Dirty ring interface Peter Xu
2020-03-09 21:44 ` [PATCH v6 01/14] KVM: X86: Change parameter for fast_page_fault tracepoint Peter Xu
2020-03-09 21:44 ` [PATCH v6 02/14] KVM: Cache as_id in kvm_memory_slot Peter Xu
2020-03-10 14:48 ` Sean Christopherson
2020-03-09 21:44 ` [PATCH v6 03/14] KVM: X86: Don't track dirty for KVM_SET_[TSS_ADDR|IDENTITY_MAP_ADDR] Peter Xu
2020-03-10 15:06 ` Sean Christopherson
2020-03-11 1:10 ` kbuild test robot
[not found] ` <20200311163906.GG479302@xz-x1>
2020-03-11 17:09 ` Sean Christopherson
2020-03-18 20:04 ` Peter Xu
2020-03-09 21:44 ` [PATCH v6 04/14] KVM: Pass in kvm pointer into mark_page_dirty_in_slot() Peter Xu
2020-03-09 21:44 ` [PATCH v6 05/14] KVM: X86: Implement ring-based dirty memory tracking Peter Xu
2020-03-09 22:24 ` [PATCH v6 06/14] KVM: Make dirty ring exclusive to dirty bitmap log Peter Xu
2020-03-09 22:25 ` [PATCH v6 07/14] KVM: Don't allocate dirty bitmap if dirty ring is enabled Peter Xu
2020-03-09 22:25 ` [PATCH v6 08/14] KVM: selftests: Always clear dirty bitmap after iteration Peter Xu
2020-03-09 22:25 ` [PATCH v6 09/14] KVM: selftests: Sync uapi/linux/kvm.h to tools/ Peter Xu
2020-03-09 22:25 ` [PATCH v6 10/14] KVM: selftests: Use a single binary for dirty/clear log test Peter Xu
2020-03-10 8:10 ` Andrew Jones
2020-03-11 17:43 ` Peter Xu [this message]
2020-03-11 18:47 ` Andrew Jones
2020-03-09 22:25 ` [PATCH v6 11/14] KVM: selftests: Introduce after_vcpu_run hook for dirty " Peter Xu
2020-03-09 22:25 ` [PATCH v6 12/14] KVM: selftests: Add dirty ring buffer test Peter Xu
2020-03-10 8:18 ` Andrew Jones
2020-03-11 17:44 ` Peter Xu
2020-03-09 22:25 ` [PATCH v6 13/14] KVM: selftests: Let dirty_log_test async for dirty ring test Peter Xu
2020-03-10 8:27 ` Andrew Jones
2020-03-11 17:45 ` Peter Xu
2020-03-09 22:25 ` [PATCH v6 14/14] KVM: selftests: Add "-c" parameter to dirty log test Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200311174324.GH479302@xz-x1 \
--to=peterx@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=dgilbert@redhat.com \
--cc=dinechin@redhat.com \
--cc=drjones@redhat.com \
--cc=jasowang@redhat.com \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=sean.j.christopherson@intel.com \
--cc=vkuznets@redhat.com \
--cc=yan.y.zhao@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox