Re: [PATCH 09/12] KVM: arm/arm64: prepare GICv2 emulation to be handled by kvm_io_bus

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

Hej,

On 14/03/15 14:30, Christoffer Dall wrote:
> On Fri, Mar 13, 2015 at 04:10:09PM +0000, Andre Przywara wrote:
>> Using the framework provided by the recent vgic.c changes we register
>> a kvm_io_bus device when initializing the virtual GICv2.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/arm_vgic.h      |    1 +
>>  virt/kvm/arm/vgic-v2-emul.c |   13 +++++++++----
>>  virt/kvm/arm/vgic.c         |   16 ++++++++++++++++
>>  3 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 4bfc6a3..74a4ac4 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -245,6 +245,7 @@ struct vgic_dist {
>>  	unsigned long		*irq_pending_on_cpu;
>>  
>>  	struct vgic_vm_ops	vm_ops;
>> +	struct vgic_io_device	dist_iodev;
>>  };
>>  
>>  struct vgic_v2_cpu_if {
>> diff --git a/virt/kvm/arm/vgic-v2-emul.c b/virt/kvm/arm/vgic-v2-emul.c
>> index 0defac6..6f685c9 100644
>> --- a/virt/kvm/arm/vgic-v2-emul.c
>> +++ b/virt/kvm/arm/vgic-v2-emul.c
>> @@ -490,6 +490,7 @@ static bool vgic_v2_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>  static int vgic_v2_map_resources(struct kvm *kvm,
>>  				 const struct vgic_params *params)
>>  {
>> +	struct vgic_dist *dist = &kvm->arch.vgic;
>>  	int ret = 0;
>>  
>>  	if (!irqchip_in_kernel(kvm))
>> @@ -500,13 +501,17 @@ static int vgic_v2_map_resources(struct kvm *kvm,
>>  	if (vgic_ready(kvm))
>>  		goto out;
>>  
>> -	if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) ||
>> -	    IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) {
>> +	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
>> +	    IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) {
>>  		kvm_err("Need to set vgic cpu and dist addresses first\n");
>>  		ret = -ENXIO;
>>  		goto out;
>>  	}
>>  
>> +	vgic_register_kvm_io_dev(kvm, dist->vgic_dist_base,
>> +				 KVM_VGIC_V2_DIST_SIZE,
>> +				 vgic_dist_ranges, -1, &dist->dist_iodev);
>> +
>>  	/*
>>  	 * Initialize the vgic if this hasn't already been done on demand by
>>  	 * accessing the vgic state from userspace.
>> @@ -517,7 +522,7 @@ static int vgic_v2_map_resources(struct kvm *kvm,
>>  		goto out;
>>  	}
>>  
>> -	ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
>> +	ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base,
>>  				    params->vcpu_base, KVM_VGIC_V2_CPU_SIZE,
>>  				    true);
>>  	if (ret) {
>> @@ -525,7 +530,7 @@ static int vgic_v2_map_resources(struct kvm *kvm,
>>  		goto out;
>>  	}
>>  
>> -	kvm->arch.vgic.ready = true;
>> +	dist->ready = true;
>>  out:
>>  	if (ret)
>>  		kvm_vgic_destroy(kvm);
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 71389b8..b1dd717 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -908,6 +908,20 @@ int vgic_register_kvm_io_dev(struct kvm *kvm, gpa_t base, int len,
>>  	return 0;
>>  }
>>  
>> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
>> +{
>> +	struct vgic_dist *dist = &kvm->arch.vgic;
>> +
>> +	if (!dist || !kvm->buses[KVM_MMIO_BUS])
>> +		return;
>> +
>> +	mutex_lock(&kvm->slots_lock);
>> +	if (dist->dist_iodev.dev.ops)
>> +		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
>> +					  &dist->dist_iodev.dev);
> 
> why are we only unregisttering the iodev when we have ops?

Because vgic_unregister_kvm_io_dev() is called by kvm_vgic_destroy(),
which we call on some occasions during vgic_init() when we encounter an
error. There is quite a window of cases where the kvm_io_bus devices
haven't been registered yet, so we shouldn't try to unregister them at
this place. Using the .ops parameter seemed like an elegant way to
detect this case.
Does that make sense? Or have I missed something?

Cheers,
Andre.

> 
>> +	mutex_unlock(&kvm->slots_lock);
>> +}
>> +
>>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>>  {
>>  	return dist->nr_irqs - VGIC_NR_PRIVATE_IRQS;
>> @@ -1546,6 +1560,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>>  	struct kvm_vcpu *vcpu;
>>  	int i;
>>  
>> +	vgic_unregister_kvm_io_dev(kvm);
>> +
>>  	kvm_for_each_vcpu(i, vcpu, kvm)
>>  		kvm_vgic_vcpu_destroy(vcpu);
>>  
>> -- 
>> 1.7.9.5
>>
>