Hi Christoffer,
On 17/05/2017 13:18, Christoffer Dall wrote:
> Hi Jean,
>
> On Tue, May 16, 2017 at 12:23:52PM +0100, Jean-Philippe Brucker wrote:
>> Hi,
>>
>> On 09/05/17 09:56, Christoffer Dall wrote:
>>> Instead of waiting with registering KVM iodevs until the first VCPU is
>>> run, we can actually create the iodevs when the redist base address is
>>> set. The only downside is that we must now also check if we need to do
>>> this for VCPUs which are created after creating the VGIC, because there
>>> is no enforced ordering between creating the VGIC (and setting its base
>>> addresses) and creating the VCPUs.
>>
>> This triggers a BUG(), when the order is VGIC init, then VCPU init (which
>> is what kvmtool does).
>>
>> Issuing KVM_CREATE_VCPU after VGIC intialization produces the following calls:
>>
>> kvm_vm_ioctl_create_vcpu
>> kvm_arch_vcpu_create
>> kvm_vcpu_init
>> kvm_arch_vcpu_init
>> kvm_vgic_vcpu_init
>> vgic_register_redist_iodev
>> kvm_vcpu_get_idx
>> ... no VCPU registered yet in kvm->vcpus :(
>> BUG();
>>
>> ... would later register vcpu:
>> kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu
>>
>> My quick fix is to move kvm_vgic_vcpu_init into kvm_arch_vcpu_postcreate,
>> but it discards the return value of kvm_vgic_vcpu_init, so I don't know
>> how to do it properly.
>>
>
> Would you mind giving this patch a go with your setup?
>
> commit 7370dc8eefc9004923c2454c2f01c49850c8d94b (HEAD -> vcpu_idx_redist_bugfix)
> Author: Christoffer Dall <cdall@xxxxxxxxxx>
> Date: Wed May 17 13:12:51 2017 +0200
>
> KVM: arm/arm64: Fix bug when registering redist iodevs
>
> If userspace creates the VCPUs after initializing the VGIC, then we end
> up in a situation where we trigger a bug in kvm_vcpu_get_idx(), because
> it is called prior to adding the VCPU into the vcpus array on the VM.
>
> There is no tight coupling between the VCPU index and the area of the
> redistributor region used for the VCPU, so we can simply ensure that all
> creations of redistributors are serialized per VM, and increment an
> offset when we successfully add a redistributor.
>
> The vgic_register_redist_iodev() function can be called from two paths:
> vgic_redister_all_redist_iodev() which is called via the kvm_vgic_addr()
> device attribute handler. This patch already holds the kvm->lock mutex.
>
> The other path is via kvm_vgic_vcpu_init, which is called through a
> longer chain from kvm_vm_ioctl_create_vcpu(), which releases the
> kvm->lock mutex just before calling kvm_arch_vcpu_create(), so we can
> simply take this mutex again later for our purposes.
>
> Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxx>
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 97b8d37..2304aeb 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -195,9 +195,13 @@ struct vgic_dist {
> /* either a GICv2 CPU interface */
> gpa_t vgic_cpu_base;
> /* or a number of GICv3 redistributor regions */
> - gpa_t vgic_redist_base;
> + struct {
> + gpa_t vgic_redist_base;
> + gpa_t vgic_redist_free_offset;
> + };
> };
>
> +
> /* distributor enabled */
> bool enabled;
>
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index dc68e2e..3a0b899 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -242,8 +242,11 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> * If we are creating a VCPU with a GICv3 we must also register the
> * KVM io device for the redistributor that belongs to this VCPU.
> */
> - if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> + if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> + mutex_lock(&vcpu->kvm->lock);
> ret = vgic_register_redist_iodev(vcpu);
> + mutex_unlock(&vcpu->kvm->lock);
> + }
> return ret;
> }
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 99da1a2..9b0f681 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -586,7 +586,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> if (!vgic_v3_check_base(kvm))
> return -EINVAL;
>
> - rd_base = vgic->vgic_redist_base + kvm_vcpu_get_idx(vcpu) * SZ_64K * 2;
> + rd_base = vgic->vgic_redist_base + vgic->vgic_redist_free_offset;
> sgi_base = rd_base + SZ_64K;
>
> kvm_iodevice_init(&rd_dev->dev, &kvm_io_gic_ops);
> @@ -615,11 +615,14 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
> ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, sgi_base,
> SZ_64K, &sgi_dev->dev);
> mutex_unlock(&kvm->slots_lock);
> - if (ret)
> + if (ret) {
> kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,
> &rd_dev->dev);
Not related to this patch but I noticed kvm_io_bus_unregister_dev must
also be called with slots_lock held.
Also true in vgic_unregister_redist_iodev().
Then on this very patch file, looks good to me.
Thanks
Eric
> + return ret;
> + }
>
> - return ret;
> + vgic->vgic_redist_free_offset += 2 * SZ_64K;
> + return 0;
> }
>
> static void vgic_unregister_redist_iodev(struct kvm_vcpu *vcpu)
>
>
> Thanks,
> -Christoffer
>
