[PATCH v7 0/5] dirty_log_perf_test vCPU pinning

[PATCH v7 0/5] dirty_log_perf_test vCPU pinning

[Vipin Sharma]

Pin vCPUs to a host physical CPUs (pCPUs) in dirty_log_perf_test and
optionally pin the main application thread to a physical cpu if
provided. All tests based on perf_test_util framework can take advantage
of it if needed.

While at it, I changed atoi() to atoi_paranoid(), atoi_positive,
atoi_non_negative() in other tests, sorted command line options
alphabetically in dirty_log_perf_test, and added break between -e and -g
which was missed in original commit when -e was introduced.

v7:
- Moved pinning APIs from perf_test_util.c to kvm_util.c.
- Missed one atoi() call in aarch64/debug-exceptions.c, changed it to
  atoi_positive().

v6: https://lore.kernel.org/lkml/20221021211816.1525201-1-vipinsh@google.com/
- Updated the shortlog of Patch 5.
- Changed formatting of help text of -c in dirty_log_perf_test

v5: https://lore.kernel.org/lkml/20221010220538.1154054-1-vipinsh@google.com/
- Added atoi_postive() and atoi_non_negative() APIs for string parsing.
- Using sched_getaffinity() to verify if a pCPU is allowed or not.
- Changed Suggested-by to add only person came up with original idea of
  pinning.
- Updated strings and commit messages.

v4: https://lore.kernel.org/lkml/20221006171133.372359-1-vipinsh@google.com/
- Moved boolean to check vCPUs pinning from perf_test_vcpu_args to
  perf_test_args.
- Changed assert statements to make error more descriptive.
- Modified break statement between 'e' and 'g' option in v3 by not copying
  dirty_log_manual_caps = 0 to 'e'.

v3: https://lore.kernel.org/lkml/20220826184500.1940077-1-vipinsh@google.com
- Moved atoi_paranoid() to test_util.c and replaced all atoi() usage
  with atoi_paranoid()
- Sorted command line options alphabetically.
- Instead of creating a vcpu thread on a specific pcpu the thread will
  migrate to the provided pcpu after its creation.
- Decoupled -e and -g option.

v2: https://lore.kernel.org/lkml/20220819210737.763135-1-vipinsh@google.com/
- Removed -d option.
- One cpu list passed as option, cpus for vcpus, followed by
  application thread cpu.
- Added paranoid cousin of atoi().

v1: https://lore.kernel.org/lkml/20220817152956.4056410-1-vipinsh@google.com


Vipin Sharma (5):
  KVM: selftests: Add missing break between -e and -g option in
    dirty_log_perf_test
  KVM: selftests: Put command line options in alphabetical order in
    dirty_log_perf_test
  KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()
  KVM: selftests: Add atoi_positive() and atoi_non_negative() for input
    validation
  KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs

 .../selftests/kvm/aarch64/arch_timer.c        | 25 ++------
 .../selftests/kvm/aarch64/debug-exceptions.c  |  2 +-
 .../testing/selftests/kvm/aarch64/vgic_irq.c  |  6 +-
 .../selftests/kvm/access_tracking_perf_test.c |  2 +-
 .../selftests/kvm/demand_paging_test.c        |  4 +-
 .../selftests/kvm/dirty_log_perf_test.c       | 64 +++++++++++++------
 .../selftests/kvm/include/kvm_util_base.h     |  4 ++
 .../selftests/kvm/include/perf_test_util.h    |  4 ++
 .../testing/selftests/kvm/include/test_util.h |  4 ++
 .../selftests/kvm/kvm_page_table_test.c       |  4 +-
 tools/testing/selftests/kvm/lib/kvm_util.c    | 54 ++++++++++++++++
 .../selftests/kvm/lib/perf_test_util.c        |  8 ++-
 tools/testing/selftests/kvm/lib/test_util.c   | 35 ++++++++++
 .../selftests/kvm/max_guest_memory_test.c     |  7 +-
 .../kvm/memslot_modification_stress_test.c    |  6 +-
 .../testing/selftests/kvm/memslot_perf_test.c | 24 ++-----
 .../selftests/kvm/set_memory_region_test.c    |  2 +-
 .../selftests/kvm/x86_64/nx_huge_pages_test.c |  4 +-
 18 files changed, 180 insertions(+), 79 deletions(-)

-- 
2.38.1.273.g43a17bfeac-goog

[PATCH v7 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test

[Vipin Sharma]

Passing -e option (Run VCPUs while dirty logging is being disabled) in
dirty_log_perf_test also unintentionally enables -g (Do not enable
KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2). Add break between two switch case
logic.

Fixes: cfe12e64b065 ("KVM: selftests: Add an option to run vCPUs while disabling dirty logging")
Signed-off-by: Vipin Sharma <vipinsh@google.com>
---
 tools/testing/selftests/kvm/dirty_log_perf_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index f99e39a672d3..56e08da3a87f 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -411,6 +411,7 @@ int main(int argc, char *argv[])
 		case 'e':
 			/* 'e' is for evil. */
 			run_vcpus_while_disabling_dirty_logging = true;
+			break;
 		case 'g':
 			dirty_log_manual_caps = 0;
 			break;
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH v7 2/5] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test

[Vipin Sharma]

There are 13 command line options and they are not in any order. Put
them in alphabetical order to make it easy to add new options.

No functional change intended.

Signed-off-by: Vipin Sharma <vipinsh@google.com>
Reviewed-by: Wei Wang <wei.w.wang@intel.com>
---
 .../selftests/kvm/dirty_log_perf_test.c       | 36 ++++++++++---------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index 56e08da3a87f..5bb6954b2358 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -406,50 +406,52 @@ int main(int argc, char *argv[])
 
 	guest_modes_append_default();
 
-	while ((opt = getopt(argc, argv, "eghi:p:m:nb:f:v:os:x:")) != -1) {
+	while ((opt = getopt(argc, argv, "b:ef:ghi:m:nop:s:v:x:")) != -1) {
 		switch (opt) {
+		case 'b':
+			guest_percpu_mem_size = parse_size(optarg);
+			break;
 		case 'e':
 			/* 'e' is for evil. */
 			run_vcpus_while_disabling_dirty_logging = true;
 			break;
+		case 'f':
+			p.wr_fract = atoi(optarg);
+			TEST_ASSERT(p.wr_fract >= 1,
+				    "Write fraction cannot be less than one");
+			break;
 		case 'g':
 			dirty_log_manual_caps = 0;
 			break;
+		case 'h':
+			help(argv[0]);
+			break;
 		case 'i':
 			p.iterations = atoi(optarg);
 			break;
-		case 'p':
-			p.phys_offset = strtoull(optarg, NULL, 0);
-			break;
 		case 'm':
 			guest_modes_cmdline(optarg);
 			break;
 		case 'n':
 			perf_test_args.nested = true;
 			break;
-		case 'b':
-			guest_percpu_mem_size = parse_size(optarg);
+		case 'o':
+			p.partition_vcpu_memory_access = false;
 			break;
-		case 'f':
-			p.wr_fract = atoi(optarg);
-			TEST_ASSERT(p.wr_fract >= 1,
-				    "Write fraction cannot be less than one");
+		case 'p':
+			p.phys_offset = strtoull(optarg, NULL, 0);
+			break;
+		case 's':
+			p.backing_src = parse_backing_src_type(optarg);
 			break;
 		case 'v':
 			nr_vcpus = atoi(optarg);
 			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
 			break;
-		case 'o':
-			p.partition_vcpu_memory_access = false;
-			break;
-		case 's':
-			p.backing_src = parse_backing_src_type(optarg);
-			break;
 		case 'x':
 			p.slots = atoi(optarg);
 			break;
-		case 'h':
 		default:
 			help(argv[0]);
 			break;
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH v7 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()

[Vipin Sharma]

atoi() doesn't detect errors. There is no way to know that a 0 return
is correct conversion or due to an error.

Introduce atoi_paranoid() to detect errors and provide correct
conversion. Replace all atoi() calls with atoi_paranoid().

Signed-off-by: Vipin Sharma <vipinsh@google.com>
Suggested-by: David Matlack <dmatlack@google.com>
Suggested-by: Sean Christopherson <seanjc@google.com>

---
 .../selftests/kvm/aarch64/arch_timer.c        |  8 ++++----
 .../selftests/kvm/aarch64/debug-exceptions.c  |  2 +-
 .../testing/selftests/kvm/aarch64/vgic_irq.c  |  6 +++---
 .../selftests/kvm/access_tracking_perf_test.c |  2 +-
 .../selftests/kvm/demand_paging_test.c        |  2 +-
 .../selftests/kvm/dirty_log_perf_test.c       |  8 ++++----
 .../testing/selftests/kvm/include/test_util.h |  2 ++
 .../selftests/kvm/kvm_page_table_test.c       |  2 +-
 tools/testing/selftests/kvm/lib/test_util.c   | 19 +++++++++++++++++++
 .../selftests/kvm/max_guest_memory_test.c     |  6 +++---
 .../kvm/memslot_modification_stress_test.c    |  4 ++--
 .../testing/selftests/kvm/memslot_perf_test.c | 10 +++++-----
 .../selftests/kvm/set_memory_region_test.c    |  2 +-
 .../selftests/kvm/x86_64/nx_huge_pages_test.c |  4 ++--
 14 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
index 574eb73f0e90..251e7ff04883 100644
--- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -414,7 +414,7 @@ static bool parse_args(int argc, char *argv[])
 	while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) {
 		switch (opt) {
 		case 'n':
-			test_args.nr_vcpus = atoi(optarg);
+			test_args.nr_vcpus = atoi_paranoid(optarg);
 			if (test_args.nr_vcpus <= 0) {
 				pr_info("Positive value needed for -n\n");
 				goto err;
@@ -425,21 +425,21 @@ static bool parse_args(int argc, char *argv[])
 			}
 			break;
 		case 'i':
-			test_args.nr_iter = atoi(optarg);
+			test_args.nr_iter = atoi_paranoid(optarg);
 			if (test_args.nr_iter <= 0) {
 				pr_info("Positive value needed for -i\n");
 				goto err;
 			}
 			break;
 		case 'p':
-			test_args.timer_period_ms = atoi(optarg);
+			test_args.timer_period_ms = atoi_paranoid(optarg);
 			if (test_args.timer_period_ms <= 0) {
 				pr_info("Positive value needed for -p\n");
 				goto err;
 			}
 			break;
 		case 'm':
-			test_args.migration_freq_ms = atoi(optarg);
+			test_args.migration_freq_ms = atoi_paranoid(optarg);
 			if (test_args.migration_freq_ms < 0) {
 				pr_info("0 or positive value needed for -m\n");
 				goto err;
diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 947bd201435c..19fffdf19c9f 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -423,7 +423,7 @@ int main(int argc, char *argv[])
 	while ((opt = getopt(argc, argv, "i:")) != -1) {
 		switch (opt) {
 		case 'i':
-			ss_iteration = atoi(optarg);
+			ss_iteration = atoi_paranoid(optarg);
 			break;
 		case 'h':
 		default:
diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
index 17417220a083..ae90b718070a 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
@@ -824,16 +824,16 @@ int main(int argc, char **argv)
 	while ((opt = getopt(argc, argv, "hn:e:l:")) != -1) {
 		switch (opt) {
 		case 'n':
-			nr_irqs = atoi(optarg);
+			nr_irqs = atoi_paranoid(optarg);
 			if (nr_irqs > 1024 || nr_irqs % 32)
 				help(argv[0]);
 			break;
 		case 'e':
-			eoi_split = (bool)atoi(optarg);
+			eoi_split = (bool)atoi_paranoid(optarg);
 			default_args = false;
 			break;
 		case 'l':
-			level_sensitive = (bool)atoi(optarg);
+			level_sensitive = (bool)atoi_paranoid(optarg);
 			default_args = false;
 			break;
 		case 'h':
diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c
index 76c583a07ea2..c6bcc5301e2c 100644
--- a/tools/testing/selftests/kvm/access_tracking_perf_test.c
+++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c
@@ -368,7 +368,7 @@ int main(int argc, char *argv[])
 			params.vcpu_memory_bytes = parse_size(optarg);
 			break;
 		case 'v':
-			params.nr_vcpus = atoi(optarg);
+			params.nr_vcpus = atoi_paranoid(optarg);
 			break;
 		case 'o':
 			overlap_memory_access = true;
diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
index 779ae54f89c4..82597fb04146 100644
--- a/tools/testing/selftests/kvm/demand_paging_test.c
+++ b/tools/testing/selftests/kvm/demand_paging_test.c
@@ -427,7 +427,7 @@ int main(int argc, char *argv[])
 			p.src_type = parse_backing_src_type(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi(optarg);
+			nr_vcpus = atoi_paranoid(optarg);
 			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
 			break;
diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index 5bb6954b2358..ecda802b78ff 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -416,7 +416,7 @@ int main(int argc, char *argv[])
 			run_vcpus_while_disabling_dirty_logging = true;
 			break;
 		case 'f':
-			p.wr_fract = atoi(optarg);
+			p.wr_fract = atoi_paranoid(optarg);
 			TEST_ASSERT(p.wr_fract >= 1,
 				    "Write fraction cannot be less than one");
 			break;
@@ -427,7 +427,7 @@ int main(int argc, char *argv[])
 			help(argv[0]);
 			break;
 		case 'i':
-			p.iterations = atoi(optarg);
+			p.iterations = atoi_paranoid(optarg);
 			break;
 		case 'm':
 			guest_modes_cmdline(optarg);
@@ -445,12 +445,12 @@ int main(int argc, char *argv[])
 			p.backing_src = parse_backing_src_type(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi(optarg);
+			nr_vcpus = atoi_paranoid(optarg);
 			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
 			break;
 		case 'x':
-			p.slots = atoi(optarg);
+			p.slots = atoi_paranoid(optarg);
 			break;
 		default:
 			help(argv[0]);
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index befc754ce9b3..feae42863759 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -152,4 +152,6 @@ static inline void *align_ptr_up(void *x, size_t size)
 	return (void *)align_up((unsigned long)x, size);
 }
 
+int atoi_paranoid(const char *num_str);
+
 #endif /* SELFTEST_KVM_TEST_UTIL_H */
diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c
index f42c6ac6d71d..ea7feb69bb88 100644
--- a/tools/testing/selftests/kvm/kvm_page_table_test.c
+++ b/tools/testing/selftests/kvm/kvm_page_table_test.c
@@ -461,7 +461,7 @@ int main(int argc, char *argv[])
 			p.test_mem_size = parse_size(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi(optarg);
+			nr_vcpus = atoi_paranoid(optarg);
 			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
 			break;
diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index 6d23878bbfe1..ec0f070a6f21 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -334,3 +334,22 @@ long get_run_delay(void)
 
 	return val[1];
 }
+
+int atoi_paranoid(const char *num_str)
+{
+	char *end_ptr;
+	long num;
+
+	errno = 0;
+	num = strtol(num_str, &end_ptr, 10);
+	TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str);
+	TEST_ASSERT(num_str != end_ptr,
+		    "strtol(\"%s\") didn't find a valid integer.\n", num_str);
+	TEST_ASSERT(*end_ptr == '\0',
+		    "strtol(\"%s\") failed to parse trailing characters \"%s\".\n",
+		    num_str, end_ptr);
+	TEST_ASSERT(num >= INT_MIN && num <= INT_MAX,
+		    "%ld not in range of [%d, %d]", num, INT_MIN, INT_MAX);
+
+	return num;
+}
diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
index 9a6e4f3ad6b5..1595b73dc09a 100644
--- a/tools/testing/selftests/kvm/max_guest_memory_test.c
+++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
@@ -193,15 +193,15 @@ int main(int argc, char *argv[])
 	while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) {
 		switch (opt) {
 		case 'c':
-			nr_vcpus = atoi(optarg);
+			nr_vcpus = atoi_paranoid(optarg);
 			TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0");
 			break;
 		case 'm':
-			max_mem = atoi(optarg) * size_1gb;
+			max_mem = atoi_paranoid(optarg) * size_1gb;
 			TEST_ASSERT(max_mem > 0, "memory size must be >0");
 			break;
 		case 's':
-			slot_size = atoi(optarg) * size_1gb;
+			slot_size = atoi_paranoid(optarg) * size_1gb;
 			TEST_ASSERT(slot_size > 0, "slot size must be >0");
 			break;
 		case 'H':
diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
index 6ee7e1dde404..865276993ffb 100644
--- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
+++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
@@ -166,7 +166,7 @@ int main(int argc, char *argv[])
 			guest_percpu_mem_size = parse_size(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi(optarg);
+			nr_vcpus = atoi_paranoid(optarg);
 			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d",
 				    max_vcpus);
@@ -175,7 +175,7 @@ int main(int argc, char *argv[])
 			p.partition_vcpu_memory_access = false;
 			break;
 		case 'i':
-			p.nr_memslot_modifications = atoi(optarg);
+			p.nr_memslot_modifications = atoi_paranoid(optarg);
 			break;
 		case 'h':
 		default:
diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 44995446d942..4bae9e3f5ca1 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -885,21 +885,21 @@ static bool parse_args(int argc, char *argv[],
 			map_unmap_verify = true;
 			break;
 		case 's':
-			targs->nslots = atoi(optarg);
+			targs->nslots = atoi_paranoid(optarg);
 			if (targs->nslots <= 0 && targs->nslots != -1) {
 				pr_info("Slot count cap has to be positive or -1 for no cap\n");
 				return false;
 			}
 			break;
 		case 'f':
-			targs->tfirst = atoi(optarg);
+			targs->tfirst = atoi_paranoid(optarg);
 			if (targs->tfirst < 0) {
 				pr_info("First test to run has to be non-negative\n");
 				return false;
 			}
 			break;
 		case 'e':
-			targs->tlast = atoi(optarg);
+			targs->tlast = atoi_paranoid(optarg);
 			if (targs->tlast < 0 || targs->tlast >= NTESTS) {
 				pr_info("Last test to run has to be non-negative and less than %zu\n",
 					NTESTS);
@@ -907,14 +907,14 @@ static bool parse_args(int argc, char *argv[],
 			}
 			break;
 		case 'l':
-			targs->seconds = atoi(optarg);
+			targs->seconds = atoi_paranoid(optarg);
 			if (targs->seconds < 0) {
 				pr_info("Test length in seconds has to be non-negative\n");
 				return false;
 			}
 			break;
 		case 'r':
-			targs->runs = atoi(optarg);
+			targs->runs = atoi_paranoid(optarg);
 			if (targs->runs <= 0) {
 				pr_info("Runs per test has to be positive\n");
 				return false;
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index 0d55f508d595..c366949c8362 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -407,7 +407,7 @@ int main(int argc, char *argv[])
 
 #ifdef __x86_64__
 	if (argc > 1)
-		loops = atoi(argv[1]);
+		loops = atoi_paranoid(argv[1]);
 	else
 		loops = 10;
 
diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
index 59ffe7fd354f..354b6902849c 100644
--- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
+++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
@@ -241,10 +241,10 @@ int main(int argc, char **argv)
 	while ((opt = getopt(argc, argv, "hp:t:r")) != -1) {
 		switch (opt) {
 		case 'p':
-			reclaim_period_ms = atoi(optarg);
+			reclaim_period_ms = atoi_paranoid(optarg);
 			break;
 		case 't':
-			token = atoi(optarg);
+			token = atoi_paranoid(optarg);
 			break;
 		case 'r':
 			reboot_permissions = true;
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH v7 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation

[Vipin Sharma]

Many KVM selftests take command line arguments which are supposed to be
positive (>0) or non-negative (>=0). Some tests do these validation and
some missed adding the check.

Add atoi_positive() and atoi_non_negative() to validate inputs in
selftests before proceeding to use those values.

Signed-off-by: Vipin Sharma <vipinsh@google.com>
---
 .../selftests/kvm/aarch64/arch_timer.c        | 25 ++++---------------
 .../selftests/kvm/aarch64/debug-exceptions.c  |  2 +-
 .../testing/selftests/kvm/aarch64/vgic_irq.c  |  2 +-
 .../selftests/kvm/access_tracking_perf_test.c |  2 +-
 .../selftests/kvm/demand_paging_test.c        |  4 +--
 .../selftests/kvm/dirty_log_perf_test.c       | 12 ++++-----
 .../testing/selftests/kvm/include/test_util.h |  2 ++
 .../selftests/kvm/kvm_page_table_test.c       |  4 +--
 tools/testing/selftests/kvm/lib/test_util.c   | 16 ++++++++++++
 .../selftests/kvm/max_guest_memory_test.c     |  7 +++---
 .../kvm/memslot_modification_stress_test.c    |  6 ++---
 .../testing/selftests/kvm/memslot_perf_test.c | 22 ++++------------
 .../selftests/kvm/set_memory_region_test.c    |  2 +-
 13 files changed, 47 insertions(+), 59 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
index 251e7ff04883..24dffcaf7a9f 100644
--- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -414,36 +414,21 @@ static bool parse_args(int argc, char *argv[])
 	while ((opt = getopt(argc, argv, "hn:i:p:m:")) != -1) {
 		switch (opt) {
 		case 'n':
-			test_args.nr_vcpus = atoi_paranoid(optarg);
-			if (test_args.nr_vcpus <= 0) {
-				pr_info("Positive value needed for -n\n");
-				goto err;
-			} else if (test_args.nr_vcpus > KVM_MAX_VCPUS) {
+			test_args.nr_vcpus = atoi_positive(optarg);
+			if (test_args.nr_vcpus > KVM_MAX_VCPUS) {
 				pr_info("Max allowed vCPUs: %u\n",
 					KVM_MAX_VCPUS);
 				goto err;
 			}
 			break;
 		case 'i':
-			test_args.nr_iter = atoi_paranoid(optarg);
-			if (test_args.nr_iter <= 0) {
-				pr_info("Positive value needed for -i\n");
-				goto err;
-			}
+			test_args.nr_iter = atoi_positive(optarg);
 			break;
 		case 'p':
-			test_args.timer_period_ms = atoi_paranoid(optarg);
-			if (test_args.timer_period_ms <= 0) {
-				pr_info("Positive value needed for -p\n");
-				goto err;
-			}
+			test_args.timer_period_ms = atoi_positive(optarg);
 			break;
 		case 'm':
-			test_args.migration_freq_ms = atoi_paranoid(optarg);
-			if (test_args.migration_freq_ms < 0) {
-				pr_info("0 or positive value needed for -m\n");
-				goto err;
-			}
+			test_args.migration_freq_ms = atoi_non_negative(optarg);
 			break;
 		case 'h':
 		default:
diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index 19fffdf19c9f..9650e8a9bac6 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -423,7 +423,7 @@ int main(int argc, char *argv[])
 	while ((opt = getopt(argc, argv, "i:")) != -1) {
 		switch (opt) {
 		case 'i':
-			ss_iteration = atoi_paranoid(optarg);
+			ss_iteration = atoi_positive(optarg);
 			break;
 		case 'h':
 		default:
diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
index ae90b718070a..d7da5f24db35 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_irq.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_irq.c
@@ -824,7 +824,7 @@ int main(int argc, char **argv)
 	while ((opt = getopt(argc, argv, "hn:e:l:")) != -1) {
 		switch (opt) {
 		case 'n':
-			nr_irqs = atoi_paranoid(optarg);
+			nr_irqs = atoi_non_negative(optarg);
 			if (nr_irqs > 1024 || nr_irqs % 32)
 				help(argv[0]);
 			break;
diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c
index c6bcc5301e2c..b30500cc197e 100644
--- a/tools/testing/selftests/kvm/access_tracking_perf_test.c
+++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c
@@ -368,7 +368,7 @@ int main(int argc, char *argv[])
 			params.vcpu_memory_bytes = parse_size(optarg);
 			break;
 		case 'v':
-			params.nr_vcpus = atoi_paranoid(optarg);
+			params.nr_vcpus = atoi_positive(optarg);
 			break;
 		case 'o':
 			overlap_memory_access = true;
diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
index 82597fb04146..dcdb6964b1dc 100644
--- a/tools/testing/selftests/kvm/demand_paging_test.c
+++ b/tools/testing/selftests/kvm/demand_paging_test.c
@@ -427,8 +427,8 @@ int main(int argc, char *argv[])
 			p.src_type = parse_backing_src_type(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi_paranoid(optarg);
-			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
+			nr_vcpus = atoi_positive(optarg);
+			TEST_ASSERT(nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
 			break;
 		case 'o':
diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index ecda802b78ff..618598ddd993 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -416,9 +416,7 @@ int main(int argc, char *argv[])
 			run_vcpus_while_disabling_dirty_logging = true;
 			break;
 		case 'f':
-			p.wr_fract = atoi_paranoid(optarg);
-			TEST_ASSERT(p.wr_fract >= 1,
-				    "Write fraction cannot be less than one");
+			p.wr_fract = atoi_positive(optarg);
 			break;
 		case 'g':
 			dirty_log_manual_caps = 0;
@@ -427,7 +425,7 @@ int main(int argc, char *argv[])
 			help(argv[0]);
 			break;
 		case 'i':
-			p.iterations = atoi_paranoid(optarg);
+			p.iterations = atoi_positive(optarg);
 			break;
 		case 'm':
 			guest_modes_cmdline(optarg);
@@ -445,12 +443,12 @@ int main(int argc, char *argv[])
 			p.backing_src = parse_backing_src_type(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi_paranoid(optarg);
-			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
+			nr_vcpus = atoi_positive(optarg);
+			TEST_ASSERT(nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
 			break;
 		case 'x':
-			p.slots = atoi_paranoid(optarg);
+			p.slots = atoi_positive(optarg);
 			break;
 		default:
 			help(argv[0]);
diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index feae42863759..9c7b2c186a48 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -153,5 +153,7 @@ static inline void *align_ptr_up(void *x, size_t size)
 }
 
 int atoi_paranoid(const char *num_str);
+uint32_t atoi_positive(const char *num_str);
+uint32_t atoi_non_negative(const char *num_str);
 
 #endif /* SELFTEST_KVM_TEST_UTIL_H */
diff --git a/tools/testing/selftests/kvm/kvm_page_table_test.c b/tools/testing/selftests/kvm/kvm_page_table_test.c
index ea7feb69bb88..2f62e19976fd 100644
--- a/tools/testing/selftests/kvm/kvm_page_table_test.c
+++ b/tools/testing/selftests/kvm/kvm_page_table_test.c
@@ -461,8 +461,8 @@ int main(int argc, char *argv[])
 			p.test_mem_size = parse_size(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi_paranoid(optarg);
-			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
+			nr_vcpus = atoi_positive(optarg);
+			TEST_ASSERT(nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d", max_vcpus);
 			break;
 		case 's':
diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index ec0f070a6f21..210e98a49a83 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -353,3 +353,19 @@ int atoi_paranoid(const char *num_str)
 
 	return num;
 }
+
+uint32_t atoi_positive(const char *num_str)
+{
+	int num = atoi_paranoid(num_str);
+
+	TEST_ASSERT(num > 0, "%s is not a positive integer.\n", num_str);
+	return num;
+}
+
+uint32_t atoi_non_negative(const char *num_str)
+{
+	int num = atoi_paranoid(num_str);
+
+	TEST_ASSERT(num >= 0, "%s is not a non-negative integer.\n", num_str);
+	return num;
+}
diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
index 1595b73dc09a..20015de3b91c 100644
--- a/tools/testing/selftests/kvm/max_guest_memory_test.c
+++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
@@ -193,15 +193,14 @@ int main(int argc, char *argv[])
 	while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) {
 		switch (opt) {
 		case 'c':
-			nr_vcpus = atoi_paranoid(optarg);
-			TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0");
+			nr_vcpus = atoi_positive(optarg);
 			break;
 		case 'm':
-			max_mem = atoi_paranoid(optarg) * size_1gb;
+			max_mem = atoi_positive(optarg) * size_1gb;
 			TEST_ASSERT(max_mem > 0, "memory size must be >0");
 			break;
 		case 's':
-			slot_size = atoi_paranoid(optarg) * size_1gb;
+			slot_size = atoi_positive(optarg) * size_1gb;
 			TEST_ASSERT(slot_size > 0, "slot size must be >0");
 			break;
 		case 'H':
diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
index 865276993ffb..7539ee7b6e95 100644
--- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
+++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
@@ -166,8 +166,8 @@ int main(int argc, char *argv[])
 			guest_percpu_mem_size = parse_size(optarg);
 			break;
 		case 'v':
-			nr_vcpus = atoi_paranoid(optarg);
-			TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
+			nr_vcpus = atoi_positive(optarg);
+			TEST_ASSERT(nr_vcpus <= max_vcpus,
 				    "Invalid number of vcpus, must be between 1 and %d",
 				    max_vcpus);
 			break;
@@ -175,7 +175,7 @@ int main(int argc, char *argv[])
 			p.partition_vcpu_memory_access = false;
 			break;
 		case 'i':
-			p.nr_memslot_modifications = atoi_paranoid(optarg);
+			p.nr_memslot_modifications = atoi_positive(optarg);
 			break;
 		case 'h':
 		default:
diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index 4bae9e3f5ca1..8e6e2d44d002 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -892,33 +892,21 @@ static bool parse_args(int argc, char *argv[],
 			}
 			break;
 		case 'f':
-			targs->tfirst = atoi_paranoid(optarg);
-			if (targs->tfirst < 0) {
-				pr_info("First test to run has to be non-negative\n");
-				return false;
-			}
+			targs->tfirst = atoi_non_negative(optarg);
 			break;
 		case 'e':
-			targs->tlast = atoi_paranoid(optarg);
-			if (targs->tlast < 0 || targs->tlast >= NTESTS) {
+			targs->tlast = atoi_non_negative(optarg);
+			if (targs->tlast >= NTESTS) {
 				pr_info("Last test to run has to be non-negative and less than %zu\n",
 					NTESTS);
 				return false;
 			}
 			break;
 		case 'l':
-			targs->seconds = atoi_paranoid(optarg);
-			if (targs->seconds < 0) {
-				pr_info("Test length in seconds has to be non-negative\n");
-				return false;
-			}
+			targs->seconds = atoi_non_negative(optarg);
 			break;
 		case 'r':
-			targs->runs = atoi_paranoid(optarg);
-			if (targs->runs <= 0) {
-				pr_info("Runs per test has to be positive\n");
-				return false;
-			}
+			targs->runs = atoi_positive(optarg);
 			break;
 		}
 	}
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index c366949c8362..e09092110121 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -407,7 +407,7 @@ int main(int argc, char *argv[])
 
 #ifdef __x86_64__
 	if (argc > 1)
-		loops = atoi_paranoid(argv[1]);
+		loops = atoi_positive(argv[1]);
 	else
 		loops = 10;
 
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH v7 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs

[Vipin Sharma]

Add a command line option, -c, to pin vCPUs to physical CPUs (pCPUs),
i.e.  to force vCPUs to run on specific pCPUs.

Requirement to implement this feature came in discussion on the patch
"Make page tables for eager page splitting NUMA aware"
https://lore.kernel.org/lkml/YuhPT2drgqL+osLl@google.com/

This feature is useful as it provides a way to analyze performance based
on the vCPUs and dirty log worker locations, like on the different NUMA
nodes or on the same NUMA nodes.

To keep things simple, implementation is intentionally very limited,
either all of the vCPUs will be pinned followed by an optional main
thread or nothing will be pinned.

Signed-off-by: Vipin Sharma <vipinsh@google.com>
Suggested-by: David Matlack <dmatlack@google.com>
---
 .../selftests/kvm/dirty_log_perf_test.c       | 25 ++++++++-
 .../selftests/kvm/include/kvm_util_base.h     |  4 ++
 .../selftests/kvm/include/perf_test_util.h    |  4 ++
 tools/testing/selftests/kvm/lib/kvm_util.c    | 54 +++++++++++++++++++
 .../selftests/kvm/lib/perf_test_util.c        |  8 ++-
 5 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index 618598ddd993..a82fc51d57ca 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -353,7 +353,7 @@ static void help(char *name)
 	puts("");
 	printf("usage: %s [-h] [-i iterations] [-p offset] [-g] "
 	       "[-m mode] [-n] [-b vcpu bytes] [-v vcpus] [-o] [-s mem type]"
-	       "[-x memslots]\n", name);
+	       "[-x memslots] [-c physical cpus to run test on]\n", name);
 	puts("");
 	printf(" -i: specify iteration counts (default: %"PRIu64")\n",
 	       TEST_HOST_LOOP_N);
@@ -383,6 +383,17 @@ static void help(char *name)
 	backing_src_help("-s");
 	printf(" -x: Split the memory region into this number of memslots.\n"
 	       "     (default: 1)\n");
+	printf(" -c: Pin tasks to physical CPUs.  Takes a list of comma separated\n"
+	       "     values (target pCPU), one for each vCPU, plus an optional\n"
+	       "     entry for the main application task (specified via entry\n"
+	       "     <nr_vcpus + 1>).  If used, entries must be provided for all\n"
+	       "     vCPUs, i.e. pinning vCPUs is all or nothing.\n\n"
+	       "     E.g. to create 3 vCPUs, pin vCPU0=>pCPU22, vCPU1=>pCPU23,\n"
+	       "     vCPU2=>pCPU24, and pin the application task to pCPU50:\n\n"
+	       "         ./dirty_log_perf_test -v 3 -c 22,23,24,50\n\n"
+	       "     To leave the application task unpinned, drop the final entry:\n\n"
+	       "         ./dirty_log_perf_test -v 3 -c 22,23,24\n\n"
+	       "     (default: no pinning)\n");
 	puts("");
 	exit(0);
 }
@@ -390,6 +401,7 @@ static void help(char *name)
 int main(int argc, char *argv[])
 {
 	int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS);
+	const char *pcpu_list = NULL;
 	struct test_params p = {
 		.iterations = TEST_HOST_LOOP_N,
 		.wr_fract = 1,
@@ -406,11 +418,14 @@ int main(int argc, char *argv[])
 
 	guest_modes_append_default();
 
-	while ((opt = getopt(argc, argv, "b:ef:ghi:m:nop:s:v:x:")) != -1) {
+	while ((opt = getopt(argc, argv, "b:c:ef:ghi:m:nop:s:v:x:")) != -1) {
 		switch (opt) {
 		case 'b':
 			guest_percpu_mem_size = parse_size(optarg);
 			break;
+		case 'c':
+			pcpu_list = optarg;
+			break;
 		case 'e':
 			/* 'e' is for evil. */
 			run_vcpus_while_disabling_dirty_logging = true;
@@ -456,6 +471,12 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (pcpu_list) {
+		kvm_parse_vcpu_pinning(pcpu_list, perf_test_args.vcpu_to_pcpu,
+				       nr_vcpus);
+		perf_test_args.pin_vcpus = true;
+	}
+
 	TEST_ASSERT(p.iterations >= 2, "The test should have at least two iterations");
 
 	pr_info("Test iterations: %"PRIu64"\n",	p.iterations);
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index e42a09cd24a0..3bf2333ef95d 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -688,6 +688,10 @@ static inline struct kvm_vm *vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
 
 struct kvm_vcpu *vm_recreate_with_one_vcpu(struct kvm_vm *vm);
 
+void kvm_pin_this_task_to_pcpu(uint32_t pcpu);
+void kvm_parse_vcpu_pinning(const char *pcpus_string, uint32_t vcpu_to_pcpu[],
+			    int nr_vcpus);
+
 unsigned long vm_compute_max_gfn(struct kvm_vm *vm);
 unsigned int vm_calc_num_guest_pages(enum vm_guest_mode mode, size_t size);
 unsigned int vm_num_host_pages(enum vm_guest_mode mode, unsigned int num_guest_pages);
diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h
index eaa88df0555a..849c875dd0ff 100644
--- a/tools/testing/selftests/kvm/include/perf_test_util.h
+++ b/tools/testing/selftests/kvm/include/perf_test_util.h
@@ -39,6 +39,10 @@ struct perf_test_args {
 
 	/* Run vCPUs in L2 instead of L1, if the architecture supports it. */
 	bool nested;
+	/* True if all vCPUs are pinned to pCPUs */
+	bool pin_vcpus;
+	/* The vCPU=>pCPU pinning map. Only valid if pin_vcpus is true. */
+	uint32_t vcpu_to_pcpu[KVM_MAX_VCPUS];
 
 	struct perf_test_vcpu_args vcpu_args[KVM_MAX_VCPUS];
 };
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index f1cb1627161f..8292af9d7660 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -11,6 +11,7 @@
 #include "processor.h"
 
 #include <assert.h>
+#include <sched.h>
 #include <sys/mman.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -443,6 +444,59 @@ struct kvm_vcpu *vm_recreate_with_one_vcpu(struct kvm_vm *vm)
 	return vm_vcpu_recreate(vm, 0);
 }
 
+void kvm_pin_this_task_to_pcpu(uint32_t pcpu)
+{
+	cpu_set_t mask;
+	int r;
+
+	CPU_ZERO(&mask);
+	CPU_SET(pcpu, &mask);
+	r = sched_setaffinity(0, sizeof(mask), &mask);
+	TEST_ASSERT(!r, "sched_setaffinity() failed for pCPU '%u'.\n", pcpu);
+}
+
+static uint32_t parse_pcpu(const char *cpu_str, const cpu_set_t *allowed_mask)
+{
+	uint32_t pcpu = atoi_non_negative(cpu_str);
+
+	TEST_ASSERT(CPU_ISSET(pcpu, allowed_mask),
+		    "Not allowed to run on pCPU '%d', check cgroups?\n", pcpu);
+	return pcpu;
+}
+
+void kvm_parse_vcpu_pinning(const char *pcpus_string, uint32_t vcpu_to_pcpu[],
+			    int nr_vcpus)
+{
+	cpu_set_t allowed_mask;
+	char *cpu, *cpu_list;
+	char delim[2] = ",";
+	int i, r;
+
+	cpu_list = strdup(pcpus_string);
+	TEST_ASSERT(cpu_list, "strdup() allocation failed.\n");
+
+	r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask);
+	TEST_ASSERT(!r, "sched_getaffinity() failed");
+
+	cpu = strtok(cpu_list, delim);
+
+	/* 1. Get all pcpus for vcpus. */
+	for (i = 0; i < nr_vcpus; i++) {
+		TEST_ASSERT(cpu, "pCPU not provided for vCPU '%d'\n", i);
+		vcpu_to_pcpu[i] = parse_pcpu(cpu, &allowed_mask);
+		cpu = strtok(NULL, delim);
+	}
+
+	/* 2. Check if the main worker needs to be pinned. */
+	if (cpu) {
+		kvm_pin_this_task_to_pcpu(parse_pcpu(cpu, &allowed_mask));
+		cpu = strtok(NULL, delim);
+	}
+
+	TEST_ASSERT(!cpu, "pCPU list contains trailing garbage characters '%s'", cpu);
+	free(cpu_list);
+}
+
 /*
  * Userspace Memory Region Find
  *
diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c
index 9618b37c66f7..3a1d0a44419b 100644
--- a/tools/testing/selftests/kvm/lib/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
@@ -2,6 +2,8 @@
 /*
  * Copyright (C) 2020, Google LLC.
  */
+#define _GNU_SOURCE
+
 #include <inttypes.h>
 
 #include "kvm_util.h"
@@ -243,6 +245,10 @@ void __weak perf_test_setup_nested(struct kvm_vm *vm, int nr_vcpus, struct kvm_v
 static void *vcpu_thread_main(void *data)
 {
 	struct vcpu_thread *vcpu = data;
+	int vcpu_idx = vcpu->vcpu_idx;
+
+	if (perf_test_args.pin_vcpus)
+		kvm_pin_this_task_to_pcpu(perf_test_args.vcpu_to_pcpu[vcpu_idx]);
 
 	WRITE_ONCE(vcpu->running, true);
 
@@ -255,7 +261,7 @@ static void *vcpu_thread_main(void *data)
 	while (!READ_ONCE(all_vcpu_threads_running))
 		;
 
-	vcpu_thread_fn(&perf_test_args.vcpu_args[vcpu->vcpu_idx]);
+	vcpu_thread_fn(&perf_test_args.vcpu_args[vcpu_idx]);
 
 	return NULL;
 }
-- 
2.38.1.273.g43a17bfeac-goog

Re: [PATCH v7 1/5] KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test

[Sean Christopherson]

On Mon, Oct 31, 2022, Vipin Sharma wrote:
> Passing -e option (Run VCPUs while dirty logging is being disabled) in
> dirty_log_perf_test also unintentionally enables -g (Do not enable
> KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2). Add break between two switch case
> logic.
> 
> Fixes: cfe12e64b065 ("KVM: selftests: Add an option to run vCPUs while disabling dirty logging")
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v7 2/5] KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test

[Sean Christopherson]

On Mon, Oct 31, 2022, Vipin Sharma wrote:
> There are 13 command line options and they are not in any order. Put
> them in alphabetical order to make it easy to add new options.
> 
> No functional change intended.
> 
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Reviewed-by: Wei Wang <wei.w.wang@intel.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v7 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()

[Sean Christopherson]

On Mon, Oct 31, 2022, Vipin Sharma wrote:
> atoi() doesn't detect errors. There is no way to know that a 0 return
> is correct conversion or due to an error.
> 
> Introduce atoi_paranoid() to detect errors and provide correct
> conversion. Replace all atoi() calls with atoi_paranoid().
> 
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Suggested-by: David Matlack <dmatlack@google.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> 
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v7 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()

[Sean Christopherson]

On Mon, Oct 31, 2022, Vipin Sharma wrote:
> atoi() doesn't detect errors. There is no way to know that a 0 return
> is correct conversion or due to an error.
> 
> Introduce atoi_paranoid() to detect errors and provide correct
> conversion. Replace all atoi() calls with atoi_paranoid().
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index 6d23878bbfe1..ec0f070a6f21 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -334,3 +334,22 @@ long get_run_delay(void)
>  
>  	return val[1];
>  }
> +
> +int atoi_paranoid(const char *num_str)
> +{
> +	char *end_ptr;
> +	long num;
> +
> +	errno = 0;
> +	num = strtol(num_str, &end_ptr, 10);

I take back my review.  This forces specifying params in decimal, e.g. a large
hex number yields:

  strtol("0xffffffffff") failed to parse trailing characters "xffffffffff".

Obviously I'm intentionally being a bad user in this particular case, but there
will inevitably be tests that want to take hex input, e.g. an x86 test that takes
an MSR index would definitely want hex input.

Looking through all selftests, I don't think there are existing cases that would
likely want hex, but it's trivial to support since strtol() will autodetect the
format if the base is '0', i.e.

diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index 210e98a49a83..8c37cfa12edc 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -341,7 +341,7 @@ int atoi_paranoid(const char *num_str)
        long num;
 
        errno = 0;
-       num = strtol(num_str, &end_ptr, 10);
+       num = strtol(num_str, &end_ptr, 0);
        TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str);
        TEST_ASSERT(num_str != end_ptr,
                    "strtol(\"%s\") didn't find a valid integer.\n", num_str);

Re: [PATCH v7 3/5] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()

[Vipin Sharma]

On Mon, Oct 31, 2022 at 12:17 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Oct 31, 2022, Vipin Sharma wrote:
> > atoi() doesn't detect errors. There is no way to know that a 0 return
> > is correct conversion or due to an error.
> >
> > Introduce atoi_paranoid() to detect errors and provide correct
> > conversion. Replace all atoi() calls with atoi_paranoid().
> > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> > index 6d23878bbfe1..ec0f070a6f21 100644
> > --- a/tools/testing/selftests/kvm/lib/test_util.c
> > +++ b/tools/testing/selftests/kvm/lib/test_util.c
> > @@ -334,3 +334,22 @@ long get_run_delay(void)
> >
> >       return val[1];
> >  }
> > +
> > +int atoi_paranoid(const char *num_str)
> > +{
> > +     char *end_ptr;
> > +     long num;
> > +
> > +     errno = 0;
> > +     num = strtol(num_str, &end_ptr, 10);
>
> I take back my review.  This forces specifying params in decimal, e.g. a large
> hex number yields:
>
>   strtol("0xffffffffff") failed to parse trailing characters "xffffffffff".
>
> Obviously I'm intentionally being a bad user in this particular case, but there
> will inevitably be tests that want to take hex input, e.g. an x86 test that takes
> an MSR index would definitely want hex input.
>
> Looking through all selftests, I don't think there are existing cases that would
> likely want hex, but it's trivial to support since strtol() will autodetect the
> format if the base is '0', i.e.

atoi_paranoid() replaced atoi() which only works for base 10. I was
keeping it similar.

I'll wait a couple of days for any other feedback and send v8.

>
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index 210e98a49a83..8c37cfa12edc 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -341,7 +341,7 @@ int atoi_paranoid(const char *num_str)
>         long num;
>
>         errno = 0;
> -       num = strtol(num_str, &end_ptr, 10);
> +       num = strtol(num_str, &end_ptr, 0);
>         TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str);
>         TEST_ASSERT(num_str != end_ptr,
>                     "strtol(\"%s\") didn't find a valid integer.\n", num_str);

Re: [PATCH v7 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation

[Sean Christopherson]

On Mon, Oct 31, 2022, Vipin Sharma wrote:
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index ec0f070a6f21..210e98a49a83 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -353,3 +353,19 @@ int atoi_paranoid(const char *num_str)
>  
>  	return num;
>  }
> +
> +uint32_t atoi_positive(const char *num_str)

I think it makes sense to inline atoi_positive() and atoi_non_negative() in
test_util.h.  Depending on developer's setups, it might be one less layer to jump
through to look at the implementation.

> +{
> +	int num = atoi_paranoid(num_str);
> +
> +	TEST_ASSERT(num > 0, "%s is not a positive integer.\n", num_str);

Newlines aren't needed in asserts.  This applies to atoi_paranoid() in the previous
patch as well (I initially missed them).

> +	return num;
> +}
> +
> +uint32_t atoi_non_negative(const char *num_str)
> +{
> +	int num = atoi_paranoid(num_str);
> +
> +	TEST_ASSERT(num >= 0, "%s is not a non-negative integer.\n", num_str);
> +	return num;
> +}
> diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
> index 1595b73dc09a..20015de3b91c 100644
> --- a/tools/testing/selftests/kvm/max_guest_memory_test.c
> +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
> @@ -193,15 +193,14 @@ int main(int argc, char *argv[])
>  	while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) {
>  		switch (opt) {
>  		case 'c':
> -			nr_vcpus = atoi_paranoid(optarg);
> -			TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0");
> +			nr_vcpus = atoi_positive(optarg);

I know I originally made the claim that the assert would provide enough context
to offest lack of a specific message, but after actually playing around with this,
past me was wrong.  E.g. this

  Memory size must be greater than 0, got '-1'

is much more helpful than

  -1 is not a positive integer.

E.g. something like this?

  static inline uint32_t atoi_positive(const char *name, const char *num_str)
  {
	int num = atoi_paranoid(num_str);

	TEST_ASSERT(num > 0, "%s must be greater than 0, got '%s'", name, num_str);
	return num;
  }

  static inline uint32_t atoi_non_negative(const char *name, const char *num_str)
  {
	int num = atoi_paranoid(num_str);

	TEST_ASSERT(num >= 0, "%s must be non-negative, got '%s'", name, num_str);
	return num;
  }

IMO, that also makes the code slightly easier to follow as it's super obvious
what is being parsed.

  p.wr_fract = atoi_positive("Write fraction", optarg);

  p.iterations = atoi_positive("Number of iterations", optarg);

  nr_vcpus = atoi_positive("Number of vCPUs", optarg);

Last thought: my vote would be to ignore the 80 char soft limit when adding the
"name" to these calls, in every case except nr_memslot_modifications the overrun
is relatively minor and not worth wrapping.  See below for my thougts on that one.

>  			break;
>  		case 'm':
> -			max_mem = atoi_paranoid(optarg) * size_1gb;
> +			max_mem = atoi_positive(optarg) * size_1gb;
>  			TEST_ASSERT(max_mem > 0, "memory size must be >0");

This assert can be dropped, max_mem is a uint64_t so wrapping to '0' is impossible.

>  			break;
>  		case 's':
> -			slot_size = atoi_paranoid(optarg) * size_1gb;
> +			slot_size = atoi_positive(optarg) * size_1gb;

Same thing here.

>  			TEST_ASSERT(slot_size > 0, "slot size must be >0");
>  			break;
>  		case 'H':
> diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> index 865276993ffb..7539ee7b6e95 100644
> --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> @@ -175,7 +175,7 @@ int main(int argc, char *argv[])
>  			p.partition_vcpu_memory_access = false;
>  			break;

memslot_modification_delay can be converted to atoi_non_negative(), it open codes
strtoul(), but the "long" part is unnecessary because memslot_modification_delay
is an "unsigned int", not an "unsigned long".

>  		case 'i':
> -			p.nr_memslot_modifications = atoi_paranoid(optarg);
> +			p.nr_memslot_modifications = atoi_positive(optarg);

To avoid a ridiculously long line, my vote is to rename the test args.  The names
are rather odd irrespective of line length.  E.g. in a prep patch do

  s/memslot_modification_delay/delay
  s/nr_memslot_modifications/nr_iterations

which yields parsing of:

	while ((opt = getopt(argc, argv, "hm:d:b:v:oi:")) != -1) {
		switch (opt) {
		case 'm':
			guest_modes_cmdline(optarg);
			break;
		case 'd':
			p.delay = atoi_non_negative("Delay", optarg);
			break;
		case 'b':
			guest_percpu_mem_size = parse_size(optarg);
			break;
		case 'v':
			nr_vcpus = atoi_positive("Number of vCPUs", optarg);
			TEST_ASSERT(nr_vcpus <= max_vcpus,
				    "Invalid number of vcpus, must be between 1 and %d",
				    max_vcpus);
			break;
		case 'o':
			p.partition_vcpu_memory_access = false;
			break;
		case 'i':
			p.nr_iterations = atoi_positive("Number of iterations", optarg);
			break;
		case 'h':
		default:
			help(argv[0]);
			break;
		}
	}

Re: [PATCH v7 5/5] KVM: selftests: Allowing running dirty_log_perf_test on specific CPUs

[Sean Christopherson]

On Mon, Oct 31, 2022, Vipin Sharma wrote:
> Add a command line option, -c, to pin vCPUs to physical CPUs (pCPUs),
> i.e.  to force vCPUs to run on specific pCPUs.
> 
> Requirement to implement this feature came in discussion on the patch
> "Make page tables for eager page splitting NUMA aware"
> https://lore.kernel.org/lkml/YuhPT2drgqL+osLl@google.com/
> 
> This feature is useful as it provides a way to analyze performance based
> on the vCPUs and dirty log worker locations, like on the different NUMA
> nodes or on the same NUMA nodes.
> 
> To keep things simple, implementation is intentionally very limited,
> either all of the vCPUs will be pinned followed by an optional main
> thread or nothing will be pinned.
> 
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> Suggested-by: David Matlack <dmatlack@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v7 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation

[Vipin Sharma]

On Mon, Oct 31, 2022 at 12:48 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Oct 31, 2022, Vipin Sharma wrote:
> > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> > index ec0f070a6f21..210e98a49a83 100644
> > --- a/tools/testing/selftests/kvm/lib/test_util.c
> > +++ b/tools/testing/selftests/kvm/lib/test_util.c
> > @@ -353,3 +353,19 @@ int atoi_paranoid(const char *num_str)
> >
> >       return num;
> >  }
> > +
> > +uint32_t atoi_positive(const char *num_str)
>
> I think it makes sense to inline atoi_positive() and atoi_non_negative() in
> test_util.h.  Depending on developer's setups, it might be one less layer to jump
> through to look at the implementation.
>

I am not sure if this makes life much easier for developers, as
"inline" can totally be ignored by the compiler. Also, not sure how
much qualitative improvement it will add in the developer's code
browsing journey. Anyways, I will add "inline" in the next version.

> > +{
> > +     int num = atoi_paranoid(num_str);
> > +
> > +     TEST_ASSERT(num > 0, "%s is not a positive integer.\n", num_str);
>
> Newlines aren't needed in asserts.  This applies to atoi_paranoid() in the previous
> patch as well (I initially missed them).
>

Okay, I will remove it from the previous patch also.

> > +     return num;
> > +}
> > +
> > +uint32_t atoi_non_negative(const char *num_str)
> > +{
> > +     int num = atoi_paranoid(num_str);
> > +
> > +     TEST_ASSERT(num >= 0, "%s is not a non-negative integer.\n", num_str);
> > +     return num;
> > +}
> > diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
> > index 1595b73dc09a..20015de3b91c 100644
> > --- a/tools/testing/selftests/kvm/max_guest_memory_test.c
> > +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
> > @@ -193,15 +193,14 @@ int main(int argc, char *argv[])
> >       while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) {
> >               switch (opt) {
> >               case 'c':
> > -                     nr_vcpus = atoi_paranoid(optarg);
> > -                     TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0");
> > +                     nr_vcpus = atoi_positive(optarg);
>
> I know I originally made the claim that the assert would provide enough context
> to offest lack of a specific message, but after actually playing around with this,
> past me was wrong.  E.g. this
>
>   Memory size must be greater than 0, got '-1'
>
> is much more helpful than
>
>   -1 is not a positive integer.
>
> E.g. something like this?
>
>   static inline uint32_t atoi_positive(const char *name, const char *num_str)
>   {
>         int num = atoi_paranoid(num_str);
>
>         TEST_ASSERT(num > 0, "%s must be greater than 0, got '%s'", name, num_str);
>         return num;
>   }
>
>   static inline uint32_t atoi_non_negative(const char *name, const char *num_str)
>   {
>         int num = atoi_paranoid(num_str);
>
>         TEST_ASSERT(num >= 0, "%s must be non-negative, got '%s'", name, num_str);
>         return num;
>   }
>
> IMO, that also makes the code slightly easier to follow as it's super obvious
> what is being parsed.
>
>   p.wr_fract = atoi_positive("Write fraction", optarg);
>
>   p.iterations = atoi_positive("Number of iterations", optarg);
>
>   nr_vcpus = atoi_positive("Number of vCPUs", optarg);
>

I will make this change. It is indeed better.

> Last thought: my vote would be to ignore the 80 char soft limit when adding the
> "name" to these calls, in every case except nr_memslot_modifications the overrun
> is relatively minor and not worth wrapping.  See below for my thougts on that one.
>
> >                       break;
> >               case 'm':
> > -                     max_mem = atoi_paranoid(optarg) * size_1gb;
> > +                     max_mem = atoi_positive(optarg) * size_1gb;
> >                       TEST_ASSERT(max_mem > 0, "memory size must be >0");
>
> This assert can be dropped, max_mem is a uint64_t so wrapping to '0' is impossible.
>

I intentionally kept it, as it is also protecting against having
accidently making size_1gb to 0.

> >                       break;
> >               case 's':
> > -                     slot_size = atoi_paranoid(optarg) * size_1gb;
> > +                     slot_size = atoi_positive(optarg) * size_1gb;
>
> Same thing here.
>
> >                       TEST_ASSERT(slot_size > 0, "slot size must be >0");
> >                       break;
> >               case 'H':
> > diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> > index 865276993ffb..7539ee7b6e95 100644
> > --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> > +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> > @@ -175,7 +175,7 @@ int main(int argc, char *argv[])
> >                       p.partition_vcpu_memory_access = false;
> >                       break;
>
> memslot_modification_delay can be converted to atoi_non_negative(), it open codes
> strtoul(), but the "long" part is unnecessary because memslot_modification_delay
> is an "unsigned int", not an "unsigned long".
>
> >               case 'i':
> > -                     p.nr_memslot_modifications = atoi_paranoid(optarg);
> > +                     p.nr_memslot_modifications = atoi_positive(optarg);
>
> To avoid a ridiculously long line, my vote is to rename the test args.  The names
> are rather odd irrespective of line length.  E.g. in a prep patch do
>
>   s/memslot_modification_delay/delay
>   s/nr_memslot_modifications/nr_iterations
>

Okay, I will change this and any other places I find which can be shortened.

> which yields parsing of:
>
>         while ((opt = getopt(argc, argv, "hm:d:b:v:oi:")) != -1) {
>                 switch (opt) {
>                 case 'm':
>                         guest_modes_cmdline(optarg);
>                         break;
>                 case 'd':
>                         p.delay = atoi_non_negative("Delay", optarg);
>                         break;
>                 case 'b':
>                         guest_percpu_mem_size = parse_size(optarg);
>                         break;
>                 case 'v':
>                         nr_vcpus = atoi_positive("Number of vCPUs", optarg);
>                         TEST_ASSERT(nr_vcpus <= max_vcpus,
>                                     "Invalid number of vcpus, must be between 1 and %d",
>                                     max_vcpus);
>                         break;
>                 case 'o':
>                         p.partition_vcpu_memory_access = false;
>                         break;
>                 case 'i':
>                         p.nr_iterations = atoi_positive("Number of iterations", optarg);
>                         break;
>                 case 'h':
>                 default:
>                         help(argv[0]);
>                         break;
>                 }
>         }
>

Re: [PATCH v7 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation

[Sean Christopherson]

On Tue, Nov 01, 2022, Vipin Sharma wrote:
> On Mon, Oct 31, 2022 at 12:48 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Mon, Oct 31, 2022, Vipin Sharma wrote:
> > > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> > > index ec0f070a6f21..210e98a49a83 100644
> > > --- a/tools/testing/selftests/kvm/lib/test_util.c
> > > +++ b/tools/testing/selftests/kvm/lib/test_util.c
> > > @@ -353,3 +353,19 @@ int atoi_paranoid(const char *num_str)
> > >
> > >       return num;
> > >  }
> > > +
> > > +uint32_t atoi_positive(const char *num_str)
> >
> > I think it makes sense to inline atoi_positive() and atoi_non_negative() in
> > test_util.h.  Depending on developer's setups, it might be one less layer to jump
> > through to look at the implementation.
> >
> 
> I am not sure if this makes life much easier for developers, as
> "inline" can totally be ignored by the compiler. Also, not sure how
> much qualitative improvement it will add in the developer's code
> browsing journey. Anyways, I will add "inline" in the next version.

To be clear, it's not about adding "inline", it's about not having separate
declarations and definitions.  E.g. I've yet to achieve a setup that has 100%
accuracy when it comes to navigating to a definition versus a declaration.  And
when poking around code, seeing a "static inline" function provides a hint that
a function is likely a simple wrapper without even having to look at the
implementation.

These are all small things, but I can't think of a reason _not_ to inline these
trivial wrappers.

> > Last thought: my vote would be to ignore the 80 char soft limit when adding the
> > "name" to these calls, in every case except nr_memslot_modifications the overrun
> > is relatively minor and not worth wrapping.  See below for my thougts on that one.
> >
> > >                       break;
> > >               case 'm':
> > > -                     max_mem = atoi_paranoid(optarg) * size_1gb;
> > > +                     max_mem = atoi_positive(optarg) * size_1gb;
> > >                       TEST_ASSERT(max_mem > 0, "memory size must be >0");
> >
> > This assert can be dropped, max_mem is a uint64_t so wrapping to '0' is impossible.
> >
> 
> I intentionally kept it, as it is also protecting against having
> accidently making size_1gb to 0.

Heh, the test has far, far bigger problems if it screws up size_1gb.  And that's
an orthogonal concern as the test would be horrifically broken regardless of
whether or not the user specified '-m' and/or '-s'.

A better approach is to replace the homebrewed size_1gb with SZ_1G from
tools/include/linux/sizes.h.  I, and many others, completely overlooked size.h.

Re: [PATCH v7 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation

[Vipin Sharma]

On Tue, Nov 1, 2022 at 12:20 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Nov 01, 2022, Vipin Sharma wrote:
> > On Mon, Oct 31, 2022 at 12:48 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Mon, Oct 31, 2022, Vipin Sharma wrote:
> > > > diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> > > > index ec0f070a6f21..210e98a49a83 100644
> > > > --- a/tools/testing/selftests/kvm/lib/test_util.c
> > > > +++ b/tools/testing/selftests/kvm/lib/test_util.c
> > > > @@ -353,3 +353,19 @@ int atoi_paranoid(const char *num_str)
> > > >
> > > >       return num;
> > > >  }
> > > > +
> > > > +uint32_t atoi_positive(const char *num_str)
> > >
> > > I think it makes sense to inline atoi_positive() and atoi_non_negative() in
> > > test_util.h.  Depending on developer's setups, it might be one less layer to jump
> > > through to look at the implementation.
> > >
> >
> > I am not sure if this makes life much easier for developers, as
> > "inline" can totally be ignored by the compiler. Also, not sure how
> > much qualitative improvement it will add in the developer's code
> > browsing journey. Anyways, I will add "inline" in the next version.
>
> To be clear, it's not about adding "inline", it's about not having separate
> declarations and definitions.  E.g. I've yet to achieve a setup that has 100%
> accuracy when it comes to navigating to a definition versus a declaration.  And
> when poking around code, seeing a "static inline" function provides a hint that
> a function is likely a simple wrapper without even having to look at the
> implementation.
>
> These are all small things, but I can't think of a reason _not_ to inline these
> trivial wrappers.
>

Note to myself: Read the whole sentence!

I skipped "in test_util.h". Got it.

> > > Last thought: my vote would be to ignore the 80 char soft limit when adding the
> > > "name" to these calls, in every case except nr_memslot_modifications the overrun
> > > is relatively minor and not worth wrapping.  See below for my thougts on that one.
> > >
> > > >                       break;
> > > >               case 'm':
> > > > -                     max_mem = atoi_paranoid(optarg) * size_1gb;
> > > > +                     max_mem = atoi_positive(optarg) * size_1gb;
> > > >                       TEST_ASSERT(max_mem > 0, "memory size must be >0");
> > >
> > > This assert can be dropped, max_mem is a uint64_t so wrapping to '0' is impossible.
> > >
> >
> > I intentionally kept it, as it is also protecting against having
> > accidently making size_1gb to 0.
>
> Heh, the test has far, far bigger problems if it screws up size_1gb.  And that's
> an orthogonal concern as the test would be horrifically broken regardless of
> whether or not the user specified '-m' and/or '-s'.
>
> A better approach is to replace the homebrewed size_1gb with SZ_1G from
> tools/include/linux/sizes.h.  I, and many others, completely overlooked size.h.

I will replace it.