Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test

[Emanuele Giuseppe Esposito]

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