On Wed, Aug 03, 2016 at 07:48:15PM +0200, 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.
Then how are these things removed when you just shut down your VM?
To be clear: I don't care if the VM is functional or not after having
returned an error to userspace. I just care that we're not leaking
resources or are (host kernel) vulnerable to attacks.
Thanks,
-Christoffer
--
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
