Hi Andre,
On 29 August 2014 14:40, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> (resent, that was the wrong account before ...)
>
> 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).
You are absolutely right here. I was just trying to keep
KVMTOOL changes to minimum.
>
> 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.
Yes, this makes sense. In fact, QEMU does something similar
for "-cpu host -M virt" command line options.
I think I should be less lazy on this one. I will rework this
and make it more like QEMU "-cpu host" option.
Thanks,
Anup
>
> 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
