[PATCH] KVM: arm64: ITS: move ITS registration into first VCPU run

[christoffer.dall@linaro.org (Christoffer Dall)]

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@arm.com>
> >>> ---
> >>> 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