[PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

[Andre Przywara]

Hi,

On 03/08/16 18:11, Christoffer Dall wrote:
> On Wed, Aug 03, 2016 at 03:57:45PM +0100, Andre Przywara wrote:
>> Currently we register ITS devices upon userland issuing the CTRL_INIT
>> ioctl to mark initialization of the ITS as done.
>> This deviates from the initialization sequence of the existing GIC
>> devices and does not play well with the way QEMU handles things.
>> To be more in line with what we are used to, register the ITS(es) just
>> before the first VCPU is about to run, so in the map_resources() call.
>> This involves iterating through the list of KVM devices and handle each
>> ITS that we find.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>> Hi,
>>
>> this is based upon next-20160728 plus Christoffer's kvm_device locking
>> fix from today. Please let me know what tree I should base upon and I
>> will rebase.
>> Eric, Christoffer: does that do what you expect? Can QEMU live with that?
>>
>> Cheers,
>> Andre.
>>
>>  virt/kvm/arm/vgic/vgic-its.c | 56 ++++++++++++++++++++++++++++++++++++--------
>>  virt/kvm/arm/vgic/vgic-v3.c  |  6 +++++
>>  virt/kvm/arm/vgic/vgic.h     |  6 +++++
>>  3 files changed, 58 insertions(+), 10 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 07411cf..e677a60 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1288,13 +1288,13 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
>>  		its_sync_lpi_pending_table(vcpu);
>>  }
>>  
>> -static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
>> +static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
>>  {
>>  	struct vgic_io_device *iodev = &its->iodev;
>>  	int ret;
>>  
>> -	if (its->initialized)
>> -		return 0;
>> +	if (!its->initialized)
>> +		return -EBUSY;
>>  
>>  	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
>>  		return -ENXIO;
>> @@ -1311,9 +1311,6 @@ static int vgic_its_init_its(struct kvm *kvm, struct vgic_its *its)
>>  				      KVM_VGIC_V3_ITS_SIZE, &iodev->dev);
>>  	mutex_unlock(&kvm->slots_lock);
>>  
>> -	if (!ret)
>> -		its->initialized = true;
>> -
>>  	return ret;
>>  }
>>  
>> @@ -1435,9 +1432,6 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>>  		if (type != KVM_VGIC_ITS_ADDR_TYPE)
>>  			return -ENODEV;
>>  
>> -		if (its->initialized)
>> -			return -EBUSY;
>> -
>>  		if (copy_from_user(&addr, uaddr, sizeof(addr)))
>>  			return -EFAULT;
>>  
>> @@ -1453,7 +1447,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>>  	case KVM_DEV_ARM_VGIC_GRP_CTRL:
>>  		switch (attr->attr) {
>>  		case KVM_DEV_ARM_VGIC_CTRL_INIT:
>> -			return vgic_its_init_its(dev->kvm, its);
>> +			its->initialized = true;
>> +
>> +			return 0;
>>  		}
>>  		break;
>>  	}
>> @@ -1498,3 +1494,43 @@ int kvm_vgic_register_its_device(void)
>>  	return kvm_register_device_ops(&kvm_arm_vgic_its_ops,
>>  				       KVM_DEV_TYPE_ARM_VGIC_ITS);
>>  }
>> +
>> +/*
>> + * Registers all ITSes with the kvm_io_bus framework.
>> + * To follow the existing VGIC initialization sequence, this has to be
>> + * done as late as possible, just before the first VCPU runs.
>> + */
>> +int vgic_register_its_iodevs(struct kvm *kvm)
>> +{
>> +	struct kvm_device *dev;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&kvm->devices_lock);
>> +
>> +	list_for_each_entry(dev, &kvm->devices, vm_node) {
>> +		if (dev->ops != &kvm_arm_vgic_its_ops)
>> +			continue;
>> +
>> +		ret = vgic_register_its_iodev(kvm, dev->private);
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	if (ret) {
>> +		/* Iterate backwards to roll back previous registrations. */
>> +		for (dev = list_prev_entry(dev, vm_node);
>> +		     &dev->vm_node != &kvm->devices;
>> +		     dev = list_prev_entry(dev, vm_node)) {
>> +			struct vgic_its *its = dev->private;
>> +
>> +			if (dev->ops != &kvm_arm_vgic_its_ops)
>> +				continue;
>> +
>> +			kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
>> +						  &its->iodev.dev);
>> +		}
>> +	}
> 
> is the unregister really necessary?

I was wondering the same, but we do it for the GICv3 redistributors as
well (though that was introduced by the same stupid author).
That being said I would be too happy to remove both these dodgy routines
if we agree that a failure will ultimately lead to a VM teardown and is
thus not needed.

Cheers,
Andre.

> 
>> +
>> +	mutex_unlock(&kvm->devices_lock);
>> +	return ret;
>> +}
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 0506543..f0d9b2b 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -289,6 +289,12 @@ int vgic_v3_map_resources(struct kvm *kvm)
>>  		goto out;
>>  	}
>>  
>> +	ret = vgic_register_its_iodevs(kvm);
>> +	if (ret) {
>> +		kvm_err("Unable to register VGIC ITS MMIO regions\n");
>> +		goto out;
>> +	}
>> +
>>  	dist->ready = true;
>>  
>>  out:
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index 1d8e21d..6c4625c 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -84,6 +84,7 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu);
>>  int vgic_v3_probe(const struct gic_kvm_info *info);
>>  int vgic_v3_map_resources(struct kvm *kvm);
>>  int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address);
>> +int vgic_register_its_iodevs(struct kvm *kvm);
>>  bool vgic_has_its(struct kvm *kvm);
>>  int kvm_vgic_register_its_device(void);
>>  void vgic_enable_lpis(struct kvm_vcpu *vcpu);
>> @@ -140,6 +141,11 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm,
>>  	return -ENODEV;
>>  }
>>  
>> +static inline int vgic_register_its_iodevs(struct kvm *kvm)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>>  static inline bool vgic_has_its(struct kvm *kvm)
>>  {
>>  	return false;
>> -- 
>> 2.9.0
>>
> 
> Otherwise this looks good to me.
> 
> Can someone provide a tested-by ?
> 
> Thanks,
> -Christoffer
>