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@xxxxxxx>
>>
>> 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@xxxxxxx>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> 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.
--
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
