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

[Marc Zyngier <marc.zyngier@arm.com>]

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...