On Wed, Sep 17, 2014 at 3:43 AM, Anup Patel <apatel@xxxxxxx> wrote:
> 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.
>>
Please ignore this notice. I used wrong email in my last reply.
Regards,
Anup
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
--
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
