lkml.org 
[lkml]   [2021]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test
From
Date


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
>

\
 
 \ /
  Last update: 2021-03-19 09:36    [W:0.083 / U:0.976 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site