Re: [PATCH v4 1/4] kvmtool: ARM: Use KVM_ARM_PREFERRED_TARGET vm ioctl to determine target cpu

[Andre Przywara <andre.przywara@arm.com>]

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@linaro.org>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> ---
>  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;
>  }
>  
>