Re: [PATCH 03/11] kvmtool: AArch{32,64}: use KVM_CREATE_DEVICE & co to instanciate the GIC

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

On 26/01/15 11:26, Will Deacon wrote:
> On Fri, Jan 23, 2015 at 04:35:02PM +0000, Andre Przywara wrote:
>> From: Marc Zyngier <marc.zyngier@arm.com>
>>
>> As of 3.14, KVM/arm supports the creation/configuration of the GIC through
>> a more generic device API, which is now the preferred way to do so.
>>
>> Plumb the new API in, and allow the old code to be used as a fallback.
>>
>> [Andre: Rename some functions on the way to differentiate between
>> creation and initialisation more clearly.]
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  tools/kvm/arm/gic.c                    |   60 ++++++++++++++++++++++++++++----
>>  tools/kvm/arm/include/arm-common/gic.h |    2 +-
>>  tools/kvm/arm/kvm.c                    |    6 ++--
>>  3 files changed, 57 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/kvm/arm/gic.c b/tools/kvm/arm/gic.c
>> index 5d8cbe6..ce5f7fa 100644
>> --- a/tools/kvm/arm/gic.c
>> +++ b/tools/kvm/arm/gic.c
>> @@ -7,7 +7,41 @@
>>  #include <linux/byteorder.h>
>>  #include <linux/kvm.h>
>>  
>> -int gic__init_irqchip(struct kvm *kvm)
>> +static int gic_fd = -1;
>> +
>> +static int gic__create_device(struct kvm *kvm)
>> +{
>> +	int err;
>> +	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>> +	u64 dist_addr = ARM_GIC_DIST_BASE;
>> +	struct kvm_create_device gic_device = {
>> +		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
>> +	};
>> +	struct kvm_device_attr cpu_if_attr = {
>> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +		.attr	= KVM_VGIC_V2_ADDR_TYPE_CPU,
>> +		.addr	= (u64)(unsigned long)&cpu_if_addr,
>> +	};
>> +	struct kvm_device_attr dist_attr = {
>> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
>> +		.addr	= (u64)(unsigned long)&dist_addr,
>> +	};
>> +
>> +	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
>> +	if (err)
>> +		return err;
>> +
>> +	gic_fd = gic_device.fd;
>> +
>> +	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>> +	if (err)
>> +		return err;
>> +
>> +	return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
>> +}
>> +
>> +static int gic__create_irqchip(struct kvm *kvm)
>>  {
>>  	int err;
>>  	struct kvm_arm_device_addr gic_addr[] = {
>> @@ -23,12 +57,6 @@ int gic__init_irqchip(struct kvm *kvm)
>>  		}
>>  	};
>>  
>> -	if (kvm->nrcpus > GIC_MAX_CPUS) {
>> -		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
>> -				kvm->nrcpus, GIC_MAX_CPUS);
>> -		kvm->nrcpus = GIC_MAX_CPUS;
>> -	}
>> -
>>  	err = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
>>  	if (err)
>>  		return err;
>> @@ -41,6 +69,24 @@ int gic__init_irqchip(struct kvm *kvm)
>>  	return err;
>>  }
>>  
>> +int gic__create(struct kvm *kvm)
>> +{
>> +	int err;
>> +
>> +	if (kvm->nrcpus > GIC_MAX_CPUS) {
>> +		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
>> +				kvm->nrcpus, GIC_MAX_CPUS);
>> +		kvm->nrcpus = GIC_MAX_CPUS;
>> +	}
>> +
>> +	/* Try the new way first, and fallback on legacy method otherwise */
>> +	err = gic__create_device(kvm);
>> +	if (err)
>> +		err = gic__create_irqchip(kvm);
> 
> This fallback doesn't look safe to me:
> 
>   - gic_fd might remain initialised
>   - What does the kernel vgic driver do if you've already done
>     a successful KVM_CREATE_DEVICE and then try to use the legacy method?

Good point. I think we need to cleanup the device by closing the fd (and
resetting the variable to -1) in case any of the subsequent ioctls
return with an error (e.g. due to unaligned addresses).
I have to check what happens in the kernel in that case, though.

Cheers,
Andre.