* [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test
2021-03-18 15:16 [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl Emanuele Giuseppe Esposito
@ 2021-03-18 15:16 ` Emanuele Giuseppe Esposito
2021-03-18 16:20 ` Andrew Jones
2021-03-18 15:39 ` [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl Paolo Bonzini
2021-03-18 16:23 ` Andrew Jones
2 siblings, 1 reply; 7+ messages in thread
From: Emanuele Giuseppe Esposito @ 2021-03-18 15:16 UTC (permalink / raw)
To: linux-kselftest
Cc: Paolo Bonzini, Shuah Khan, Andrew Jones, Peter Xu,
Vitaly Kuznetsov, linux-kernel, kvm
Test for the KVM_SET_BOOT_CPU_ID ioctl.
Check that it correctly allows to change the BSP vcpu.
v1 -> v2:
- remove unnecessary printf
- move stage for loop inside run_vcpu
- test EBUSY when calling KVM_SET_BOOT_CPU_ID after vcpu
creation and execution
- introduce _vm_ioctl
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
tools/testing/selftests/kvm/.gitignore | 1 +
tools/testing/selftests/kvm/Makefile | 1 +
.../selftests/kvm/x86_64/set_boot_cpu_id.c | 166 ++++++++++++++++++
3 files changed, 168 insertions(+)
create mode 100644 tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 32b87cc77c8e..43b8aa82aefe 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -5,6 +5,7 @@
/s390x/resets
/s390x/sync_regs_test
/x86_64/cr4_cpuid_sync_test
+/x86_64/set_boot_cpu_id
/x86_64/debug_regs
/x86_64/evmcs_test
/x86_64/get_cpuid_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index a6d61f451f88..e7b62666e06e 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -39,6 +39,7 @@ LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
+TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
TEST_GEN_PROGS_x86_64 += x86_64/get_cpuid_test
TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
diff --git a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
new file mode 100644
index 000000000000..12c558fc8074
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test that KVM_SET_BOOT_CPU_ID works as intended
+ *
+ * Copyright (C) 2020, Red Hat, Inc.
+ */
+#define _GNU_SOURCE /* for program_invocation_name */
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+
+#define N_VCPU 2
+#define VCPU_ID0 0
+#define VCPU_ID1 1
+
+static uint32_t get_bsp_flag(void)
+{
+ return rdmsr(MSR_IA32_APICBASE) & MSR_IA32_APICBASE_BSP;
+}
+
+static void guest_bsp_vcpu(void *arg)
+{
+ GUEST_SYNC(1);
+
+ GUEST_ASSERT(get_bsp_flag() != 0);
+
+ GUEST_DONE();
+}
+
+static void guest_not_bsp_vcpu(void *arg)
+{
+ GUEST_SYNC(1);
+
+ GUEST_ASSERT(get_bsp_flag() == 0);
+
+ GUEST_DONE();
+}
+
+static void test_set_boot_busy(struct kvm_vm *vm)
+{
+ int res;
+
+ res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID0);
+ TEST_ASSERT(res == -1 && errno == EBUSY,
+ "KVM_SET_BOOT_CPU_ID set while running vm");
+}
+
+static void run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
+{
+ struct ucall uc;
+ int stage;
+
+ for (stage = 0; stage < 2; stage++) {
+
+ vcpu_run(vm, vcpuid);
+
+ switch (get_ucall(vm, vcpuid, &uc)) {
+ case UCALL_SYNC:
+ TEST_ASSERT(!strcmp((const char *)uc.args[0], "hello") &&
+ uc.args[1] == stage + 1,
+ "Stage %d: Unexpected register values vmexit, got %lx",
+ stage + 1, (ulong)uc.args[1]);
+ test_set_boot_busy(vm);
+ break;
+ case UCALL_DONE:
+ TEST_ASSERT(stage == 1,
+ "Expected GUEST_DONE in stage 2, got stage %d",
+ stage);
+ break;
+ case UCALL_ABORT:
+ TEST_ASSERT(false, "%s at %s:%ld\n\tvalues: %#lx, %#lx",
+ (const char *)uc.args[0], __FILE__,
+ uc.args[1], uc.args[2], uc.args[3]);
+ default:
+ TEST_ASSERT(false, "Unexpected exit: %s",
+ exit_reason_str(vcpu_state(vm, vcpuid)->exit_reason));
+ }
+ }
+}
+
+static struct kvm_vm *create_vm(void)
+{
+ struct kvm_vm *vm;
+ uint64_t vcpu_pages = (DEFAULT_STACK_PGS) * 2;
+ uint64_t extra_pg_pages = vcpu_pages / PTES_PER_MIN_PAGE * N_VCPU;
+ uint64_t pages = DEFAULT_GUEST_PHY_PAGES + vcpu_pages + extra_pg_pages;
+
+ pages = vm_adjust_num_guest_pages(VM_MODE_DEFAULT, pages);
+ vm = vm_create(VM_MODE_DEFAULT, pages, O_RDWR);
+
+ kvm_vm_elf_load(vm, program_invocation_name, 0, 0);
+ vm_create_irqchip(vm);
+
+ return vm;
+}
+
+static void add_x86_vcpu(struct kvm_vm *vm, uint32_t vcpuid, bool bsp_code)
+{
+ if (bsp_code)
+ vm_vcpu_add_default(vm, vcpuid, guest_bsp_vcpu);
+ else
+ vm_vcpu_add_default(vm, vcpuid, guest_not_bsp_vcpu);
+
+ vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
+}
+
+static void run_vm_bsp(uint32_t bsp_vcpu)
+{
+ struct kvm_vm *vm;
+ bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;
+
+ vm = create_vm();
+
+ if (is_bsp_vcpu1)
+ vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
+
+ add_x86_vcpu(vm, VCPU_ID0, !is_bsp_vcpu1);
+ add_x86_vcpu(vm, VCPU_ID1, is_bsp_vcpu1);
+
+ run_vcpu(vm, VCPU_ID0);
+ run_vcpu(vm, VCPU_ID1);
+
+ kvm_vm_free(vm);
+}
+
+static void check_set_bsp_busy(void)
+{
+ struct kvm_vm *vm;
+ int res;
+
+ vm = create_vm();
+
+ add_x86_vcpu(vm, VCPU_ID0, true);
+ add_x86_vcpu(vm, VCPU_ID1, false);
+
+ res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
+ TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set after adding vcpu");
+
+ run_vcpu(vm, VCPU_ID0);
+ run_vcpu(vm, VCPU_ID1);
+
+ res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
+ TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set to a terminated vcpu");
+
+ kvm_vm_free(vm);
+}
+
+int main(int argc, char *argv[])
+{
+ if (!kvm_check_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
+ print_skip("set_boot_cpu_id not available");
+ return 0;
+ }
+
+ run_vm_bsp(VCPU_ID0);
+ run_vm_bsp(VCPU_ID1);
+ run_vm_bsp(VCPU_ID0);
+
+ check_set_bsp_busy();
+}
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test
2021-03-18 15:16 ` [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test Emanuele Giuseppe Esposito
@ 2021-03-18 16:20 ` Andrew Jones
2021-03-19 8:34 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Jones @ 2021-03-18 16:20 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: linux-kselftest, Paolo Bonzini, Shuah Khan, Peter Xu,
Vitaly Kuznetsov, linux-kernel, kvm
On Thu, Mar 18, 2021 at 04:16:24PM +0100, Emanuele Giuseppe Esposito wrote:
> Test for the KVM_SET_BOOT_CPU_ID ioctl.
> Check that it correctly allows to change the BSP vcpu.
>
> v1 -> v2:
> - remove unnecessary printf
> - move stage for loop inside run_vcpu
> - test EBUSY when calling KVM_SET_BOOT_CPU_ID after vcpu
> creation and execution
> - introduce _vm_ioctl
This information should be in the cover-letter. Or, for a single patch (no
cover-letter needed submission), then it should go below the '---' under
your s-o-b.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> tools/testing/selftests/kvm/.gitignore | 1 +
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/x86_64/set_boot_cpu_id.c | 166 ++++++++++++++++++
> 3 files changed, 168 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
>
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 32b87cc77c8e..43b8aa82aefe 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -5,6 +5,7 @@
> /s390x/resets
> /s390x/sync_regs_test
> /x86_64/cr4_cpuid_sync_test
> +/x86_64/set_boot_cpu_id
> /x86_64/debug_regs
> /x86_64/evmcs_test
> /x86_64/get_cpuid_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index a6d61f451f88..e7b62666e06e 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -39,6 +39,7 @@ LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
> LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>
> TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> +TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
We like the above two test lists (Makefile and .gitignore) to be in
alphabetical order.
> TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
> TEST_GEN_PROGS_x86_64 += x86_64/get_cpuid_test
> TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
> diff --git a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
> new file mode 100644
> index 000000000000..12c558fc8074
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test that KVM_SET_BOOT_CPU_ID works as intended
> + *
> + * Copyright (C) 2020, Red Hat, Inc.
> + */
> +#define _GNU_SOURCE /* for program_invocation_name */
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "processor.h"
> +
> +#define N_VCPU 2
> +#define VCPU_ID0 0
> +#define VCPU_ID1 1
> +
> +static uint32_t get_bsp_flag(void)
> +{
> + return rdmsr(MSR_IA32_APICBASE) & MSR_IA32_APICBASE_BSP;
> +}
> +
> +static void guest_bsp_vcpu(void *arg)
> +{
> + GUEST_SYNC(1);
> +
> + GUEST_ASSERT(get_bsp_flag() != 0);
> +
> + GUEST_DONE();
> +}
> +
> +static void guest_not_bsp_vcpu(void *arg)
> +{
> + GUEST_SYNC(1);
> +
> + GUEST_ASSERT(get_bsp_flag() == 0);
> +
> + GUEST_DONE();
> +}
> +
> +static void test_set_boot_busy(struct kvm_vm *vm)
> +{
> + int res;
> +
> + res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID0);
> + TEST_ASSERT(res == -1 && errno == EBUSY,
> + "KVM_SET_BOOT_CPU_ID set while running vm");
> +}
> +
> +static void run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
> +{
> + struct ucall uc;
> + int stage;
> +
> + for (stage = 0; stage < 2; stage++) {
> +
> + vcpu_run(vm, vcpuid);
> +
> + switch (get_ucall(vm, vcpuid, &uc)) {
> + case UCALL_SYNC:
> + TEST_ASSERT(!strcmp((const char *)uc.args[0], "hello") &&
> + uc.args[1] == stage + 1,
> + "Stage %d: Unexpected register values vmexit, got %lx",
> + stage + 1, (ulong)uc.args[1]);
> + test_set_boot_busy(vm);
> + break;
> + case UCALL_DONE:
> + TEST_ASSERT(stage == 1,
> + "Expected GUEST_DONE in stage 2, got stage %d",
> + stage);
> + break;
> + case UCALL_ABORT:
> + TEST_ASSERT(false, "%s at %s:%ld\n\tvalues: %#lx, %#lx",
> + (const char *)uc.args[0], __FILE__,
> + uc.args[1], uc.args[2], uc.args[3]);
> + default:
> + TEST_ASSERT(false, "Unexpected exit: %s",
> + exit_reason_str(vcpu_state(vm, vcpuid)->exit_reason));
> + }
> + }
> +}
> +
> +static struct kvm_vm *create_vm(void)
> +{
> + struct kvm_vm *vm;
> + uint64_t vcpu_pages = (DEFAULT_STACK_PGS) * 2;
^ should be N_VCPU
> + uint64_t extra_pg_pages = vcpu_pages / PTES_PER_MIN_PAGE * N_VCPU;
^
should be 2
> + uint64_t pages = DEFAULT_GUEST_PHY_PAGES + vcpu_pages + extra_pg_pages;
> +
> + pages = vm_adjust_num_guest_pages(VM_MODE_DEFAULT, pages);
> + vm = vm_create(VM_MODE_DEFAULT, pages, O_RDWR);
> +
> + kvm_vm_elf_load(vm, program_invocation_name, 0, 0);
> + vm_create_irqchip(vm);
> +
> + return vm;
> +}
> +
> +static void add_x86_vcpu(struct kvm_vm *vm, uint32_t vcpuid, bool bsp_code)
> +{
> + if (bsp_code)
> + vm_vcpu_add_default(vm, vcpuid, guest_bsp_vcpu);
> + else
> + vm_vcpu_add_default(vm, vcpuid, guest_not_bsp_vcpu);
> +
> + vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
> +}
> +
> +static void run_vm_bsp(uint32_t bsp_vcpu)
I think the 'bsp' suffixes and prefixes make the purpose of this function
and its argument more confusing. Just
static void run_vm(uint32_t vcpu)
would be more clear to me.
> +{
> + struct kvm_vm *vm;
> + bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;
Could add another define
#define BSP_VCPU VCPU_ID1
And then instead of creating the above bool, just do
if (vcpu == BSP_VCPU)
> +
> + vm = create_vm();
> +
> + if (is_bsp_vcpu1)
> + vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
Does this ioctl need to be called before creating the vcpus? The
documentation in Documentation/virt/kvm/api.rst doesn't say that.
If it can be called after creating the vcpus, then
vm_create_default_with_vcpus() can be used and there's no need
for the create_vm() and add_x86_vcpu() functions. Just use the
same guest code for both, but pass the cpu index to the guest
code function allowing something like
if (cpu == BSP_VCPU)
GUEST_ASSERT(get_bsp_flag() != 0);
else
GUEST_ASSERT(get_bsp_flag() == 0);
> +
> + add_x86_vcpu(vm, VCPU_ID0, !is_bsp_vcpu1);
> + add_x86_vcpu(vm, VCPU_ID1, is_bsp_vcpu1);
> +
> + run_vcpu(vm, VCPU_ID0);
> + run_vcpu(vm, VCPU_ID1);
> +
> + kvm_vm_free(vm);
> +}
> +
> +static void check_set_bsp_busy(void)
> +{
> + struct kvm_vm *vm;
> + int res;
> +
> + vm = create_vm();
> +
> + add_x86_vcpu(vm, VCPU_ID0, true);
> + add_x86_vcpu(vm, VCPU_ID1, false);
> +
> + res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
> + TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set after adding vcpu");
> +
> + run_vcpu(vm, VCPU_ID0);
> + run_vcpu(vm, VCPU_ID1);
> +
> + res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
> + TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set to a terminated vcpu");
> +
> + kvm_vm_free(vm);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + if (!kvm_check_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
> + print_skip("set_boot_cpu_id not available");
> + return 0;
Should be exit(KSFT_SKIP);
> + }
> +
> + run_vm_bsp(VCPU_ID0);
> + run_vm_bsp(VCPU_ID1);
> + run_vm_bsp(VCPU_ID0);
> +
> + check_set_bsp_busy();
Don't you get a compiler warning here saying there's no return from a
function that returns int?
> +}
> --
> 2.29.2
>
Thanks,
drew
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test
2021-03-18 16:20 ` Andrew Jones
@ 2021-03-19 8:34 ` Emanuele Giuseppe Esposito
2021-03-19 9:11 ` Andrew Jones
0 siblings, 1 reply; 7+ messages in thread
From: Emanuele Giuseppe Esposito @ 2021-03-19 8:34 UTC (permalink / raw)
To: Andrew Jones
Cc: linux-kselftest, Paolo Bonzini, Shuah Khan, Peter Xu,
Vitaly Kuznetsov, linux-kernel, kvm
On 18/03/2021 17:20, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 04:16:24PM +0100, Emanuele Giuseppe Esposito wrote:
>> Test for the KVM_SET_BOOT_CPU_ID ioctl.
>> Check that it correctly allows to change the BSP vcpu.
>>
>> v1 -> v2:
>> - remove unnecessary printf
>> - move stage for loop inside run_vcpu
>> - test EBUSY when calling KVM_SET_BOOT_CPU_ID after vcpu
>> creation and execution
>> - introduce _vm_ioctl
>
> This information should be in the cover-letter. Or, for a single patch (no
> cover-letter needed submission), then it should go below the '---' under
> your s-o-b.
>
>>
>> +static void add_x86_vcpu(struct kvm_vm *vm, uint32_t vcpuid, bool bsp_code)
>> +{
>> + if (bsp_code)
>> + vm_vcpu_add_default(vm, vcpuid, guest_bsp_vcpu);
>> + else
>> + vm_vcpu_add_default(vm, vcpuid, guest_not_bsp_vcpu);
>> +
>> + vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
>> +}
>> +
>> +static void run_vm_bsp(uint32_t bsp_vcpu)
>
> I think the 'bsp' suffixes and prefixes make the purpose of this function
> and its argument more confusing. Just
>
> static void run_vm(uint32_t vcpu)
>
> would be more clear to me.
The idea here was "run vm with this vcpu as BSP", implicitly assuming
that there are alwasy 2 vcpu inside, so we are picking one as BSP.
Maybe
run_vm_2_vcpu(uint32_t bsp_vcpid)
is better?
>
>> +{
>> + struct kvm_vm *vm;
>> + bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;
>
> Could add another define
>
> #define BSP_VCPU VCPU_ID1
>
> And then instead of creating the above bool, just do
>
> if (vcpu == BSP_VCPU)
I think it will be even more confusing to have BSP_VCPU fixed to
VCPU_ID1, because in the tests before and after I use VCPU_ID0 as BSP.
run_vm_bsp(VCPU_ID0);
run_vm_bsp(VCPU_ID1);
run_vm_bsp(VCPU_ID0);
>
>> +
>> + vm = create_vm();
>> +
>> + if (is_bsp_vcpu1)
>> + vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
>
> Does this ioctl need to be called before creating the vcpus? The
> documentation in Documentation/virt/kvm/api.rst doesn't say that.
Yes, it has to be called before creating the vcpus, as also shown in the
test function "check_set_bsp_busy". KVM checks that created_vcpus is 0
before setting the bsp field.
arch/x86/kvm/x86.c
case KVM_SET_BOOT_CPU_ID:
...
if (kvm->created_vcpus)
r = -EBUSY;
else
kvm->arch.bsp_vcpu_id = arg;
I will update the documentation to include also this information.
> If it can be called after creating the vcpus, then
> vm_create_default_with_vcpus() can be used and there's no need
> for the create_vm() and add_x86_vcpu() functions.
Just use the
> same guest code for both, but pass the cpu index to the guest
> code function allowing something like
>
> if (cpu == BSP_VCPU)
> GUEST_ASSERT(get_bsp_flag() != 0);
> else
> GUEST_ASSERT(get_bsp_flag() == 0);
>
I might be wrong, but there seems not to be an easy way to pass
arguments to the guest function.
Thank you,
Emanuele
>
>> +
>> + add_x86_vcpu(vm, VCPU_ID0, !is_bsp_vcpu1);
>> + add_x86_vcpu(vm, VCPU_ID1, is_bsp_vcpu1);
>> +
>> + run_vcpu(vm, VCPU_ID0);
>> + run_vcpu(vm, VCPU_ID1);
>> +
>> + kvm_vm_free(vm);
>> +}
>> +
>> +static void check_set_bsp_busy(void)
>> +{
>> + struct kvm_vm *vm;
>> + int res;
>> +
>> + vm = create_vm();
>> +
>> + add_x86_vcpu(vm, VCPU_ID0, true);
>> + add_x86_vcpu(vm, VCPU_ID1, false);
>> +
>> + res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
>> + TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set after adding vcpu");
>> +
>> + run_vcpu(vm, VCPU_ID0);
>> + run_vcpu(vm, VCPU_ID1);
>> +
>> + res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
>> + TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set to a terminated vcpu");
>> +
>> + kvm_vm_free(vm);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + if (!kvm_check_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
>> + print_skip("set_boot_cpu_id not available");
>> + return 0;
>
> Should be exit(KSFT_SKIP);
>
>> + }
>> +
>> + run_vm_bsp(VCPU_ID0);
>> + run_vm_bsp(VCPU_ID1);
>> + run_vm_bsp(VCPU_ID0);
>> +
>> + check_set_bsp_busy();
>
> Don't you get a compiler warning here saying there's no return from a
> function that returns int?
>
>> +}
>> --
>> 2.29.2
>>
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test
2021-03-19 8:34 ` Emanuele Giuseppe Esposito
@ 2021-03-19 9:11 ` Andrew Jones
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Jones @ 2021-03-19 9:11 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: linux-kselftest, Paolo Bonzini, Shuah Khan, Peter Xu,
Vitaly Kuznetsov, linux-kernel, kvm
On Fri, Mar 19, 2021 at 09:34:40AM +0100, Emanuele Giuseppe Esposito wrote:
> On 18/03/2021 17:20, Andrew Jones wrote:
> > On Thu, Mar 18, 2021 at 04:16:24PM +0100, Emanuele Giuseppe Esposito wrote:
> > > +static void run_vm_bsp(uint32_t bsp_vcpu)
> >
> > I think the 'bsp' suffixes and prefixes make the purpose of this function
> > and its argument more confusing. Just
> >
> > static void run_vm(uint32_t vcpu)
> >
> > would be more clear to me.
>
> The idea here was "run vm with this vcpu as BSP", implicitly assuming that
> there are alwasy 2 vcpu inside, so we are picking one as BSP.
>
> Maybe
>
> run_vm_2_vcpu(uint32_t bsp_vcpid)
>
> is better?
run_vm() is probably still sufficient for the function name. I better
understand the naming of the function's argument now, so the bsp prefix
does make sense.
>
> >
> > > +{
> > > + struct kvm_vm *vm;
> > > + bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;
> >
> > Could add another define
> >
> > #define BSP_VCPU VCPU_ID1
> >
> > And then instead of creating the above bool, just do
> >
> > if (vcpu == BSP_VCPU)
>
> I think it will be even more confusing to have BSP_VCPU fixed to VCPU_ID1,
> because in the tests before and after I use VCPU_ID0 as BSP.
>
> run_vm_bsp(VCPU_ID0);
> run_vm_bsp(VCPU_ID1);
> run_vm_bsp(VCPU_ID0);
Agreed. I hadn't read that far down and hadn't grasped the purpose
of run_vm[_bsp]'s argument before. But, can we get rid of the
is_bsp_vcpu1 boolean anyway? 'if (bsp_vcpu == VCPU_ID1)' is terse
enough, and it better exposes the logic.
>
> >
> > > +
> > > + vm = create_vm();
> > > +
> > > + if (is_bsp_vcpu1)
> > > + vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
> >
> > Does this ioctl need to be called before creating the vcpus? The
> > documentation in Documentation/virt/kvm/api.rst doesn't say that.
>
> Yes, it has to be called before creating the vcpus, as also shown in the
> test function "check_set_bsp_busy". KVM checks that created_vcpus is 0
> before setting the bsp field.
>
> arch/x86/kvm/x86.c
> case KVM_SET_BOOT_CPU_ID:
> ...
> if (kvm->created_vcpus)
> r = -EBUSY;
> else
> kvm->arch.bsp_vcpu_id = arg;
>
> I will update the documentation to include also this information.
Great!
And I'll try to improve the KVM selftests API to better support these
types of tests without having to copy so much code.
>
> > If it can be called after creating the vcpus, then
> > vm_create_default_with_vcpus() can be used and there's no need
> > for the create_vm() and add_x86_vcpu() functions.
>
> Just use the
> > same guest code for both, but pass the cpu index to the guest
> > code function allowing something like
> >
> > if (cpu == BSP_VCPU)
> > GUEST_ASSERT(get_bsp_flag() != 0);
> > else
> > GUEST_ASSERT(get_bsp_flag() == 0);
> >
> I might be wrong, but there seems not to be an easy way to pass arguments to
> the guest function.
There are several tests that pass the CPU index to the guest function
which you can use as examples. Take a look at e.g. steal_time.c. The
trick is to set the argument(s) with vcpu_args_set().
Thanks,
drew
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl
2021-03-18 15:16 [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl Emanuele Giuseppe Esposito
2021-03-18 15:16 ` [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test Emanuele Giuseppe Esposito
@ 2021-03-18 15:39 ` Paolo Bonzini
2021-03-18 16:23 ` Andrew Jones
2 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2021-03-18 15:39 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, linux-kselftest
Cc: Shuah Khan, Andrew Jones, Peter Xu, Vitaly Kuznetsov, linux-kernel, kvm
On 18/03/21 16:16, Emanuele Giuseppe Esposito wrote:
> As in kvm_ioctl and _kvm_ioctl, add
> the respective _vm_ioctl for vm_ioctl.
>
> _vm_ioctl invokes an ioctl using the vm fd,
> leaving the caller to test the result.
Slightly better subject: "selftests/kvm: add _vm_ioctl".
Queued both, but next time please include a cover letter too.
Thanks!
Paolo
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> tools/testing/selftests/kvm/include/kvm_util.h | 1 +
> tools/testing/selftests/kvm/lib/kvm_util.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 2d7eb6989e83..d53a5f7cad61 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -133,6 +133,7 @@ void vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
> int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
> void *arg);
> void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg);
> void kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
> int _kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
> void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index e5fbf16f725b..b8849a1aca79 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1697,11 +1697,16 @@ void vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
> {
> int ret;
>
> - ret = ioctl(vm->fd, cmd, arg);
> + ret = _vm_ioctl(vm, cmd, arg);
> TEST_ASSERT(ret == 0, "vm ioctl %lu failed, rc: %i errno: %i (%s)",
> cmd, ret, errno, strerror(errno));
> }
>
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
> +{
> + return ioctl(vm->fd, cmd, arg);
> +}
> +
> /*
> * KVM system ioctl
> *
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl
2021-03-18 15:16 [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl Emanuele Giuseppe Esposito
2021-03-18 15:16 ` [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test Emanuele Giuseppe Esposito
2021-03-18 15:39 ` [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl Paolo Bonzini
@ 2021-03-18 16:23 ` Andrew Jones
2 siblings, 0 replies; 7+ messages in thread
From: Andrew Jones @ 2021-03-18 16:23 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: linux-kselftest, Paolo Bonzini, Shuah Khan, Peter Xu,
Vitaly Kuznetsov, linux-kernel, kvm
On Thu, Mar 18, 2021 at 04:16:23PM +0100, Emanuele Giuseppe Esposito wrote:
> As in kvm_ioctl and _kvm_ioctl, add
> the respective _vm_ioctl for vm_ioctl.
>
> _vm_ioctl invokes an ioctl using the vm fd,
> leaving the caller to test the result.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> tools/testing/selftests/kvm/include/kvm_util.h | 1 +
> tools/testing/selftests/kvm/lib/kvm_util.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 2d7eb6989e83..d53a5f7cad61 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -133,6 +133,7 @@ void vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
> int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
> void *arg);
> void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg);
> void kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
> int _kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
> void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index e5fbf16f725b..b8849a1aca79 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1697,11 +1697,16 @@ void vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
> {
> int ret;
>
> - ret = ioctl(vm->fd, cmd, arg);
> + ret = _vm_ioctl(vm, cmd, arg);
> TEST_ASSERT(ret == 0, "vm ioctl %lu failed, rc: %i errno: %i (%s)",
> cmd, ret, errno, strerror(errno));
> }
>
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
> +{
> + return ioctl(vm->fd, cmd, arg);
> +}
> +
> /*
> * KVM system ioctl
> *
> --
> 2.29.2
>
With the summary prefix change suggested by Paolo, or even better
'KVM: selftests:' since that's what the majority of patches in KVM
selftests have
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread