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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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
>