linux-kernel - Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210319091114.agftfyhb2ispaktv@kamzik.brq.redhat.com>
Date:   Fri, 19 Mar 2021 10:11:14 +0100
From:   Andrew Jones <drjones@...hat.com>
To:     Emanuele Giuseppe Esposito <eesposit@...hat.com>
Cc:     linux-kselftest@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Shuah Khan <shuah@...nel.org>, Peter Xu <peterx@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test

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