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