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
