Hi Anup,
On 26/08/14 10:22, Anup Patel wrote:
> Instead, of trying out each and every target type we should
> use KVM_ARM_PREFERRED_TARGET vm ioctl to determine target type
> for KVM ARM/ARM64.
>
> If KVM_ARM_PREFERRED_TARGET vm ioctl fails then we fallback to
> old method of trying all known target types.
So as the algorithm currently works, it does not give us much
improvement over the current behaviour. We still need to list each
supported MPIDR both in kvmtool and in the kernel.
Looking more closely at the code, beside the target id we only need the
kvm_target_arm[] list for the compatible string and the init() function.
The latter is (currently) the same for all supported type, so we could
use that as a standard fallback function.
The compatible string seems to be completely ignored by the ARM64
kernel, so we could as well pass "arm,armv8" all the time.
In ARM(32) kernels we seem to not make any real use of it for CPUs which
we care for (with virtualisation extensions).
So what about the following:
We keep the list as it is, but not extend it for future CPUs, expect
those in need for a special compatible string or a specific init
function. Instead we rely on PREFFERED_TARGET for all current and
upcoming CPUs (meaning unsupported CPUs must use a 3.12 kernel or higher).
If PREFERRED_TARGET works, we scan the list anyway (to find CPUs needing
special treatment), but on failure of finding something in the list we
just go ahead:
- with the target ID the kernel returned,
- an "arm,armv8" compatible string (for arm64, not sure about arm) and
- call the standard kvmtool init function
This should relief us from the burden of adding each supported CPU to
kvmtool.
Does that make sense of am I missing something?
I will hack something up to prove that it works.
Also there is now a race on big.LITTLE systems: if the PREFERRED_TARGET
ioctl is executed on one cluster, while the KVM_ARM_VCPU_INIT call is
done on another core with a different MPIDR, then the kernel will refuse
to init the CPU. I don't know of a good solution for this (except the
sledgehammer pinning with sched_setaffinity to the current core, which
is racy, too, but should at least work somehow ;-)
Any ideas?
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>
> Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxx>
> ---
> tools/kvm/arm/kvm-cpu.c | 46 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c
> index aeaa4cf..c010e9c 100644
> --- a/tools/kvm/arm/kvm-cpu.c
> +++ b/tools/kvm/arm/kvm-cpu.c
> @@ -34,6 +34,7 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
> struct kvm_cpu *vcpu;
> int coalesced_offset, mmap_size, err = -1;
> unsigned int i;
> + struct kvm_vcpu_init preferred_init;
> struct kvm_vcpu_init vcpu_init = {
> .features = ARM_VCPU_FEATURE_FLAGS(kvm, cpu_id)
> };
> @@ -55,20 +56,42 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
> if (vcpu->kvm_run == MAP_FAILED)
> die("unable to mmap vcpu fd");
>
> - /* Find an appropriate target CPU type. */
> - for (i = 0; i < ARRAY_SIZE(kvm_arm_targets); ++i) {
> - if (!kvm_arm_targets[i])
> - continue;
> - target = kvm_arm_targets[i];
> - vcpu_init.target = target->id;
> + /*
> + * If preferred target ioctl successful then use preferred target
> + * else try each and every target type.
> + */
> + err = ioctl(kvm->vm_fd, KVM_ARM_PREFERRED_TARGET, &preferred_init);
> + if (!err) {
> + /* Match preferred target CPU type. */
> + target = NULL;
> + for (i = 0; i < ARRAY_SIZE(kvm_arm_targets); ++i) {
> + if (!kvm_arm_targets[i])
> + continue;
> + if (kvm_arm_targets[i]->id == preferred_init.target) {
> + target = kvm_arm_targets[i];
> + break;
> + }
> + }
> +
> + vcpu_init.target = preferred_init.target;
> err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
> - if (!err)
> - break;
> + if (err || target->init(vcpu))
> + die("Unable to initialise vcpu for preferred target");
So that segfaults if the CPU is not in kvmtools list (as target is still
NULL). In the current implementation we should bail out (but better use
the algorithm described above).
Also these two line can be moved outside of the loop and joined with the
last two lines from the else clause ...
> + } else {
> + /* Find an appropriate target CPU type. */
> + for (i = 0; i < ARRAY_SIZE(kvm_arm_targets); ++i) {
> + if (!kvm_arm_targets[i])
> + continue;
> + target = kvm_arm_targets[i];
> + vcpu_init.target = target->id;
> + err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
> + if (!err)
> + break;
> + }
> + if (err || target->init(vcpu))
> + die("Unable to initialise vcpu");
> }
.. to appear here.
Cheers,
Andre.
>
> - if (err || target->init(vcpu))
> - die("Unable to initialise ARM vcpu");
> -
> coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION,
> KVM_CAP_COALESCED_MMIO);
> if (coalesced_offset)
> @@ -81,6 +104,7 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
> vcpu->cpu_type = target->id;
> vcpu->cpu_compatible = target->compatible;
> vcpu->is_running = true;
> +
> return vcpu;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
