Messages in this thread | | | Subject | Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test | From | Emanuele Giuseppe Esposito <> | Date | Fri, 19 Mar 2021 09:34:40 +0100 |
| |
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 >
| |