Hi
On 03/08/2016 19:48, Auger Eric wrote:
> Hi Andre, Christoffer,
>
> On 03/08/2016 19:18, Andre Przywara wrote:
>> 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@xxxxxxx>
>>>> ---
>>>> 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.
>
> Well to me this will lead to a kvm_vcpu_ioctl/KVM_RUN failure. Then the
> unregistration only happens in kvm_destroy_vm/kvm_io_bus_destroy and
> this calls iodevice destructor ops which is not implemented for our
> device so I don't think this can be removed right now.
hum no I mixed things I think: The destructor is needed if we want to
remove our io-dev explicit deallocation in vgic and vits. Anyway this
could be more elegant than what we have now. I can take this in charge
if confirmed.
sorry for the noise
Eric
>
> We shall implement this destructor ops first. In the past I attempted to
> use the destructor (early ages of new vgic) but this crashed. I think
> this crash should not happen anymore after:
> e6e3b5a64e5f15ebd569118a9af16bd4165cbd1a
>
> I tested the new version with a qemu integration looking similar to
> other vgic init:
>
> Tested-by: Eric Auger <eric.auger@xxxxxxxxxx>
>
> Cheers
>
> Eric
>
>
>>
>> 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
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
