On Thu, Sep 11, 2014 at 9:24 PM, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> Hi Anup,
>
> On 08/09/14 09:17, 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.
>>
>> If KVM_ARM_PREFERRED_TARGET vm ioctl succeeds but the returned
>> target type is not known to KVMTOOL then we forcefully init
>> VCPU with target type returned by KVM_ARM_PREFERRED_TARGET vm ioctl.
>>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>
>> Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxx>
>> ---
>> tools/kvm/arm/kvm-cpu.c | 52 +++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 41 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c
>> index aeaa4cf..ba7a762 100644
>> --- a/tools/kvm/arm/kvm-cpu.c
>> +++ b/tools/kvm/arm/kvm-cpu.c
>> @@ -33,7 +33,8 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>> struct kvm_arm_target *target;
>> struct kvm_cpu *vcpu;
>> int coalesced_offset, mmap_size, err = -1;
>> - unsigned int i;
>> + unsigned int i, target_type;
>> + struct kvm_vcpu_init preferred_init;
>> struct kvm_vcpu_init vcpu_init = {
>> .features = ARM_VCPU_FEATURE_FLAGS(kvm, cpu_id)
>> };
>> @@ -55,19 +56,47 @@ 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;
>> - err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
>> - if (!err)
>> - break;
>> + /*
>> + * 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];
>> + target_type = kvm_arm_targets[i]->id;
>> + break;
>> + }
>> + }
>> + if (!target) {
>> + target = kvm_arm_targets[0];
>
> I think you missed the part of the patch which adds the now magic zero
> member of kvm_arm_targets[]. A simple static initializer should work.
Yes, good catch. I will fix this.
>
>> + target_type = preferred_init.target;
>
> Can't you move that out of the loop, in front of it actually? Then you
> can get rid of the line above setting the target_type also, since you
> always use the same value now, regardless whether you found that CPU in
> the list or not.
Sure, I'll rearrange this.
>
>> + }
>> + } 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];
>> + target_type = target->id;
>> + vcpu_init.target = target_type;
>> + err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
>> + if (!err)
>> + break;
>> + }
>> + if (err)
>> + die("Unable to find matching target");
>> }
>>
>> + vcpu_init.target = target_type;
>> + err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
>
> You should do this only in the if-branch above, since you (try to) call
> KVM_ARM_VCPU_INIT already in the else branch before. Otherwise in the
> latter case you would do it twice.
I was trying to follow the old code which was doing KVM_ARM_VCPU_INIT
twice for the same VCPU but it makes sense to do it only once.
Regards,
Anup
>
> Regards,
> Andre.
>
>> if (err || target->init(vcpu))
>> - die("Unable to initialise ARM vcpu");
>> + die("Unable to initialise vcpu");
>>
>> coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION,
>> KVM_CAP_COALESCED_MMIO);
>> @@ -81,6 +110,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;
>> }
>>
>>
> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
> is for the sole use of the intended recipient(s) and contains information
> that is confidential and proprietary to Applied Micro Circuits Corporation or its subsidiaries.
> It is to be used solely for the purpose of furthering the parties' business relationship.
> All unauthorized review, use, disclosure or distribution is prohibited.
> If you are not the intended recipient, please contact the sender by reply e-mail
> and destroy all copies of the original message.
>
--
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
