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