On 30 September 2014 14:26, Andre Przywara <andre.przywara@xxxxxxx> wrote:
> Hi Anup,
>
> thanks for the re-spin and sorry for the delay.
>
> Looks better now, some minor comments below.
>
> On 19/09/14 00:57, 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/aarch32/arm-cpu.c | 9 ++++++-
>> tools/kvm/arm/aarch64/arm-cpu.c | 10 ++++++--
>> tools/kvm/arm/kvm-cpu.c | 52 ++++++++++++++++++++++++++++++---------
>> 3 files changed, 57 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/kvm/arm/aarch32/arm-cpu.c b/tools/kvm/arm/aarch32/arm-cpu.c
>> index 71b98fe..0d2ff11 100644
>> --- a/tools/kvm/arm/aarch32/arm-cpu.c
>> +++ b/tools/kvm/arm/aarch32/arm-cpu.c
>> @@ -22,6 +22,12 @@ static int arm_cpu__vcpu_init(struct kvm_cpu *vcpu)
>> return 0;
>> }
>>
>> +static struct kvm_arm_target target_generic_v7 = {
>> + .id = UINT_MAX,
>> + .compatible = "arm,arm-v7",
>> + .init = arm_cpu__vcpu_init,
>> +};
>> +
>> static struct kvm_arm_target target_cortex_a15 = {
>> .id = KVM_ARM_TARGET_CORTEX_A15,
>> .compatible = "arm,cortex-a15",
>> @@ -36,7 +42,8 @@ static struct kvm_arm_target target_cortex_a7 = {
>>
>> static int arm_cpu__core_init(struct kvm *kvm)
>> {
>> - return (kvm_cpu__register_kvm_arm_target(&target_cortex_a15) ||
>> + return (kvm_cpu__register_kvm_arm_target(&target_generic_v7) ||
>> + kvm_cpu__register_kvm_arm_target(&target_cortex_a15) ||
>> kvm_cpu__register_kvm_arm_target(&target_cortex_a7));
>> }
>
> I wonder if you could avoid the registration of this target and instead
> reference it later directly (instead of using a magic 0 index)?
> This way you wouldn't need to care about avoiding accidental .id matches
> with the UINT_MAX above.
>
>> core_init(arm_cpu__core_init);
>> diff --git a/tools/kvm/arm/aarch64/arm-cpu.c b/tools/kvm/arm/aarch64/arm-cpu.c
>> index ce5ea2f..9ee3da3 100644
>> --- a/tools/kvm/arm/aarch64/arm-cpu.c
>> +++ b/tools/kvm/arm/aarch64/arm-cpu.c
>> @@ -16,13 +16,18 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>> timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>> }
>>
>> -
>> static int arm_cpu__vcpu_init(struct kvm_cpu *vcpu)
>> {
>> vcpu->generate_fdt_nodes = generate_fdt_nodes;
>> return 0;
>> }
>>
>> +static struct kvm_arm_target target_generic_v8 = {
>> + .id = UINT_MAX,
>> + .compatible = "arm,arm-v8",
>> + .init = arm_cpu__vcpu_init,
>> +};
>> +
>> static struct kvm_arm_target target_aem_v8 = {
>> .id = KVM_ARM_TARGET_AEM_V8,
>> .compatible = "arm,arm-v8",
>> @@ -43,7 +48,8 @@ static struct kvm_arm_target target_cortex_a57 = {
>>
>> static int arm_cpu__core_init(struct kvm *kvm)
>> {
>> - return (kvm_cpu__register_kvm_arm_target(&target_aem_v8) ||
>> + return (kvm_cpu__register_kvm_arm_target(&target_generic_v8) ||
>> + kvm_cpu__register_kvm_arm_target(&target_aem_v8) ||
>> kvm_cpu__register_kvm_arm_target(&target_foundation_v8) ||
>> kvm_cpu__register_kvm_arm_target(&target_cortex_a57));
>> }
>
> (same thing like for v7 here)
>
>> diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c
>> index aeaa4cf..6de5344 100644
>> --- a/tools/kvm/arm/kvm-cpu.c
>> +++ b/tools/kvm/arm/kvm-cpu.c
>> @@ -13,7 +13,7 @@ int kvm_cpu__get_debug_fd(void)
>> return debug_fd;
>> }
>>
>> -static struct kvm_arm_target *kvm_arm_targets[KVM_ARM_NUM_TARGETS];
>> +static struct kvm_arm_target *kvm_arm_targets[KVM_ARM_NUM_TARGETS+1];
>
> w/s issue
>
>> int kvm_cpu__register_kvm_arm_target(struct kvm_arm_target *target)
>> {
>> unsigned int i = 0;
>> @@ -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;
>> + /*
>> + * If preferred target ioctl successful then use preferred target
>
> the is
>
>> + * 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];
>
> As hinted above, I'd rather see a name for the magic 0 here or the
> default target_generic referenced directly.
>
>> + target_type = preferred_init.target;
>> + }
>> + vcpu_init.target = target_type;
>
> I think you can get rid of the target_type variable at all:
>
> if (!target) {
> target = &target_generic_v8;
> vcpu_init.target = preferred_init.target;
> } else
> vcpu_init.target = target->id;
>
>> err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
>> - if (!err)
>> - break;
>> + } 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;
>
> even more obvious here.
>
>> + err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
>> + if (!err)
>> + break;
>> + }
>> + if (err)
>> + die("Unable to find matching target");
>> }
>>
>> 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;
>
> If I am not mistaken, this value is bogus when we use PREFERRED_TARGET.
> Please use vcpu_init.target here instead.
>
> Regards,
> Andre.
>
>> vcpu->cpu_compatible = target->compatible;
>> vcpu->is_running = true;
>> +
>> return vcpu;
>> }
>>
>>
Sure, will incorporate all your suggestions.
Regards,
Anup
--
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
