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

[Christoffer Dall]

On Wed, Aug 03, 2016 at 06:18:48PM +0100, 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 at 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.

I think that the other architectures and places in KVM don't do this
type of cleanup (I argued for this the last time around too), and this
code here is particularly painful (not your fault, just the way it is),
so I prefer getting rid of it.

Can you argue for why we'd need it?  Stuff failed, so even if userspace
tries to do more stuff with a broken VM, what do we care if a few
devices linger around registered on the IO bus?  That shouldn't be able
to break anything.

In fact, is it not similar to initializing a few ITSes with the current
implementation and getting a failure on a later one?

Thanks,
-Christoffer