Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683587 > unrolled thread

Re: [PATCH v2 38/52] KVM: arm/arm64: GICv4: Wire init/teardown of per-VM support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 38/52] KVM: arm/arm64: GICv4: Wire init/teardown of  per-VM support Shanker Donthineni <shankerd@codeaurora.org> - 2017-07-08 13:30 +0200
    Re: [PATCH v2 38/52] KVM: arm/arm64: GICv4: Wire init/teardown of  per-VM support Marc Zyngier <marc.zyngier@arm.com> - 2017-07-10 18:40 +0200

#1683587 — Re: [PATCH v2 38/52] KVM: arm/arm64: GICv4: Wire init/teardown of per-VM support

Hi Marc,

On 06/28/2017 10:03 AM, Marc Zyngier wrote:
> Should the HW support GICv4 and an ITS being associated with this
> VM, let's init the its_vm and its_vpe structures.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-init.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 3a0b8999f011..0de1f0d986d4 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -285,8 +285,14 @@ int vgic_init(struct kvm *kvm)
>  	if (ret)
>  		goto out;
>  
> -	if (vgic_has_its(kvm))
> +	if (vgic_has_its(kvm)) {
>  		dist->msis_require_devid = true;
> +		if (kvm_vgic_global_state.has_gicv4) {
> +			ret = vgic_v4_init(kvm);
> +			if (ret)
> +				goto out;
> +		}

This is not quite right, ITS virtual device may not be initialized at the time of 
calling vgic-init(). This change breaks the existing KVM functionality with QEMU
hypervisor tool. In later patches, code assumes vgic_v4_init(kvm) was called when
vgic_has_its(kvm) returns a true value. 

The right change would be move this logic to inside vgic_its_create() something like this.

 --- a/virt/kvm/arm/vgic/vgic-init.c
 +++ b/virt/kvm/arm/vgic/vgic-init.c
 @@ -285,14 +285,8 @@ int vgic_init(struct kvm *kvm)
         if (ret)
                 goto out;
 
 -       if (vgic_has_its(kvm)) { 
 +       if (vgic_has_its(kvm))
                 dist->msis_require_devid = true;
 -               if (kvm_vgic_global_state.has_gicv4) {
 -                       ret = vgic_v4_init(kvm);
 -                       if (ret)
 -                               goto out;
 -               }
 -       }
 
         kvm_for_each_vcpu(i, vcpu, kvm)
                 kvm_vgic_vcpu_enable(vcpu);

 --- a/virt/kvm/arm/vgic/vgic-its.c
 +++ b/virt/kvm/arm/vgic/vgic-its.c
 @@ -1637,6 +1637,7 @@ static int vgic_register_its_iodev(struct kvm *kvm, struct
  static int vgic_its_create(struct kvm_device *dev, u32 type)
  {
         struct vgic_its *its;
 +       int ret;
 
         if (type != KVM_DEV_TYPE_ARM_VGIC_ITS) 
                 return -ENODEV;
 @@ -1657,6 +1658,12 @@ static int vgic_its_create(struct kvm_device *dev, u32 ty
         its->enabled = false;
         its->dev = dev;
  
 +       if (kvm_vgic_global_state.has_gicv4) {
 +               ret = vgic_v4_init(dev->kvm);
 +               if (ret)
 +                       return -ENOMEM;
 +       }
 +


> +	}
>  
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_vgic_vcpu_enable(vcpu);
> @@ -323,6 +329,9 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>  
>  	kfree(dist->spis);
>  	dist->nr_spis = 0;
> +
> +	if (kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm))
> +		vgic_v4_teardown(kvm);
>  }
>  
>  void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
> 

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [next] | [standalone]

#1684411

On 08/07/17 12:26, Shanker Donthineni wrote:
> Hi Marc,
> 
> On 06/28/2017 10:03 AM, Marc Zyngier wrote:
>> Should the HW support GICv4 and an ITS being associated with this
>> VM, let's init the its_vm and its_vpe structures.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-init.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
>> index 3a0b8999f011..0de1f0d986d4 100644
>> --- a/virt/kvm/arm/vgic/vgic-init.c
>> +++ b/virt/kvm/arm/vgic/vgic-init.c
>> @@ -285,8 +285,14 @@ int vgic_init(struct kvm *kvm)
>>  	if (ret)
>>  		goto out;
>>  
>> -	if (vgic_has_its(kvm))
>> +	if (vgic_has_its(kvm)) {
>>  		dist->msis_require_devid = true;
>> +		if (kvm_vgic_global_state.has_gicv4) {
>> +			ret = vgic_v4_init(kvm);
>> +			if (ret)
>> +				goto out;
>> +		}
> 
> This is not quite right, ITS virtual device may not be initialized at the time of 
> calling vgic-init(). This change breaks the existing KVM functionality with QEMU
> hypervisor tool. In later patches, code assumes vgic_v4_init(kvm) was called when
> vgic_has_its(kvm) returns a true value. 
> 
> The right change would be move this logic to inside vgic_its_create() something like this.
> 
>  --- a/virt/kvm/arm/vgic/vgic-init.c
>  +++ b/virt/kvm/arm/vgic/vgic-init.c
>  @@ -285,14 +285,8 @@ int vgic_init(struct kvm *kvm)
>          if (ret)
>                  goto out;
>  
>  -       if (vgic_has_its(kvm)) { 
>  +       if (vgic_has_its(kvm))
>                  dist->msis_require_devid = true;
>  -               if (kvm_vgic_global_state.has_gicv4) {
>  -                       ret = vgic_v4_init(kvm);
>  -                       if (ret)
>  -                               goto out;
>  -               }
>  -       }
>  
>          kvm_for_each_vcpu(i, vcpu, kvm)
>                  kvm_vgic_vcpu_enable(vcpu);
> 
>  --- a/virt/kvm/arm/vgic/vgic-its.c
>  +++ b/virt/kvm/arm/vgic/vgic-its.c
>  @@ -1637,6 +1637,7 @@ static int vgic_register_its_iodev(struct kvm *kvm, struct
>   static int vgic_its_create(struct kvm_device *dev, u32 type)
>   {
>          struct vgic_its *its;
>  +       int ret;
>  
>          if (type != KVM_DEV_TYPE_ARM_VGIC_ITS) 
>                  return -ENODEV;
>  @@ -1657,6 +1658,12 @@ static int vgic_its_create(struct kvm_device *dev, u32 ty
>          its->enabled = false;
>          its->dev = dev;
>   
>  +       if (kvm_vgic_global_state.has_gicv4) {
>  +               ret = vgic_v4_init(dev->kvm);
>  +               if (ret)
>  +                       return -ENOMEM;
>  +       }
>  +

This approach is pretty busted if you have more than a single virtual
ITS, as you now leak memory from the VPE array and IRQ domain allocations.

Also, there is no guarantee that we've created all vcpus by the time we
create an ITS, so this approach fails as well ("create vcpu 1, create
ITS, create vcpu 2, init vgic" is a valid sequence).

I guess we need to call vgic_v4_init() on both vgic_init() and
its_create(), and only perform the GICv4 init if the vgic has already
been initialized.

Or we simply restrict the order to be vgic_init() last, and fix
userspace to respect that order. Not enabling GICv4 won't break SW. It
may even be faster.

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web