Re: [PATCH v5 19/22] KVM: arm64: vgic-its: ITT save and restore

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,
On 04/05/2017 10:23, Christoffer Dall wrote:
> On Thu, May 04, 2017 at 09:40:35AM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 04/05/2017 09:31, Christoffer Dall wrote:
>>> On Wed, May 03, 2017 at 11:55:34PM +0200, Auger Eric wrote:
>>>> Hi Christoffer,
>>>>
>>>> On 03/05/2017 18:37, Christoffer Dall wrote:
>>>>> On Wed, May 03, 2017 at 06:08:58PM +0200, Auger Eric wrote:
>>>>>> Hi Christoffer,
>>>>>>
>>>>>> On 30/04/2017 22:14, Christoffer Dall wrote:
>>>>>>> On Fri, Apr 14, 2017 at 12:15:31PM +0200, Eric Auger wrote:
>>>>>>>> Introduce routines to save and restore device ITT and their
>>>>>>>> interrupt table entries (ITE).
>>>>>>>>
>>>>>>>> The routines will be called on device table save and
>>>>>>>> restore. They will become static in subsequent patches.
>>>>>>>
>>>>>>> Why this bottom-up approach?  Couldn't you start by having the patch
>>>>>>> that restores the device table and define the static functions that
>>>>>>> return an error there
>>>>>> done
>>>>>> , and then fill them in with subsequent patches
>>>>>>> (liek this one)?
>>>>>>>
>>>>>>> That would have the added benefit of being able to tell how things are
>>>>>>> designed to be called.
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> v4 -> v5:
>>>>>>>> - ITE are now sorted by eventid on the flush
>>>>>>>> - rename *flush* into *save*
>>>>>>>> - use macros for shits and masks
>>>>>>>> - pass ite_esz to vgic_its_save_ite
>>>>>>>>
>>>>>>>> v3 -> v4:
>>>>>>>> - lookup_table and compute_next_eventid_offset become static in this
>>>>>>>>   patch
>>>>>>>> - remove static along with vgic_its_flush/restore_itt to avoid
>>>>>>>>   compilation warnings
>>>>>>>> - next field only computed with a shift (mask removed)
>>>>>>>> - handle the case where the last element has not been found
>>>>>>>>
>>>>>>>> v2 -> v3:
>>>>>>>> - add return 0 in vgic_its_restore_ite (was in subsequent patch)
>>>>>>>>
>>>>>>>> v2: creation
>>>>>>>> ---
>>>>>>>>  virt/kvm/arm/vgic/vgic-its.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>>  virt/kvm/arm/vgic/vgic.h     |   4 ++
>>>>>>>>  2 files changed, 129 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>>>>>>> index 35b2ca1..b02fc3f 100644
>>>>>>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>>>>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>>>>>>> @@ -23,6 +23,7 @@
>>>>>>>>  #include <linux/interrupt.h>
>>>>>>>>  #include <linux/list.h>
>>>>>>>>  #include <linux/uaccess.h>
>>>>>>>> +#include <linux/list_sort.h>
>>>>>>>>  
>>>>>>>>  #include <linux/irqchip/arm-gic-v3.h>
>>>>>>>>  
>>>>>>>> @@ -1695,7 +1696,7 @@ u32 compute_next_devid_offset(struct list_head *h, struct its_device *dev)
>>>>>>>>  	return min_t(u32, next_offset, VITS_DTE_MAX_DEVID_OFFSET);
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> -u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>>>>>>>> +static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>>>>>>>>  {
>>>>>>>>  	struct list_head *e = &ite->ite_list;
>>>>>>>>  	struct its_ite *next;
>>>>>>>> @@ -1737,8 +1738,8 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>>>>>>>>   *
>>>>>>>>   * Return: < 0 on error, 1 if last element identified, 0 otherwise
>>>>>>>>   */
>>>>>>>> -int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>>>>>>> -		 int start_id, entry_fn_t fn, void *opaque)
>>>>>>>> +static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>>>>>>> +			int start_id, entry_fn_t fn, void *opaque)
>>>>>>>>  {
>>>>>>>>  	void *entry = kzalloc(esz, GFP_KERNEL);
>>>>>>>>  	struct kvm *kvm = its->dev->kvm;
>>>>>>>> @@ -1773,6 +1774,127 @@ int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>>>>>>>  }
>>>>>>>>  
>>>>>>>>  /**
>>>>>>>> + * vgic_its_save_ite - Save an interrupt translation entry at @gpa
>>>>>>>> + */
>>>>>>>> +static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>>>>>>>> +			      struct its_ite *ite, gpa_t gpa, int ite_esz)
>>>>>>>> +{
>>>>>>>> +	struct kvm *kvm = its->dev->kvm;
>>>>>>>> +	u32 next_offset;
>>>>>>>> +	u64 val;
>>>>>>>> +
>>>>>>>> +	next_offset = compute_next_eventid_offset(&dev->itt_head, ite);
>>>>>>>> +	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
>>>>>>>> +	       ((u64)ite->lpi << KVM_ITS_ITE_PINTID_SHIFT) |
>>>>>>>> +		ite->collection->collection_id;
>>>>>>>> +	val = cpu_to_le64(val);
>>>>>>>> +	return kvm_write_guest(kvm, gpa, &val, ite_esz);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * vgic_its_restore_ite - restore an interrupt translation entry
>>>>>>>> + * @event_id: id used for indexing
>>>>>>>> + * @ptr: pointer to the ITE entry
>>>>>>>> + * @opaque: pointer to the its_device
>>>>>>>> + * @next: id offset to the next entry
>>>>>>>> + */
>>>>>>>> +static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>>>>>>>> +				void *ptr, void *opaque, u32 *next)
>>>>>>>> +{
>>>>>>>> +	struct its_device *dev = (struct its_device *)opaque;
>>>>>>>> +	struct its_collection *collection;
>>>>>>>> +	struct kvm *kvm = its->dev->kvm;
>>>>>>>> +	u64 val, *p = (u64 *)ptr;
>>>>>>>
>>>>>>> nit: initializations on separate line (and possible do that just above
>>>>>>> assigning val).
>>>>>> done
>>>>>>>
>>>>>>>> +	struct vgic_irq *irq;
>>>>>>>> +	u32 coll_id, lpi_id;
>>>>>>>> +	struct its_ite *ite;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	val = *p;
>>>>>>>> +	*next = 1;
>>>>>>>> +
>>>>>>>> +	val = le64_to_cpu(val);
>>>>>>>> +
>>>>>>>> +	coll_id = val & KVM_ITS_ITE_ICID_MASK;
>>>>>>>> +	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
>>>>>>>> +
>>>>>>>> +	if (!lpi_id)
>>>>>>>> +		return 0;
>>>>>>>
>>>>>>> are all non-zero LPI IDs valid?  Don't we have a wrapper that tests if
>>>>>>> the ID is valid?
>>>>>> no, lpi_id must be >= GIC_MIN_LPI=8192; added that check.
>>>>>> ABI Doc says lpi_id==0 is interpreted as invalid. Other values <
>>>>>> GIC_MIN_LPI cause an -EINVAL error
>>>>>>>
>>>>>>> (looks like it's possible to add LPIs with the INTID range of SPIs, SGIs
>>>>>>> and PPIs here)
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +	*next = val >> KVM_ITS_ITE_NEXT_SHIFT;
>>>>>>>
>>>>>>> Don't we need to validate this somehow since it will presumably be used
>>>>>>> to forward a pointer somehow by the caller?
>>>>>> checked against max number of eventids supported by the device
>>>>>>>
>>>>>>>> +
>>>>>>>> +	collection = find_collection(its, coll_id);
>>>>>>>> +	if (!collection)
>>>>>>>> +		return -EINVAL;
>>>>>>>> +
>>>>>>>> +	ret = vgic_its_alloc_ite(dev, &ite, collection,
>>>>>>>> +				  lpi_id, event_id);
>>>>>>>> +	if (ret)
>>>>>>>> +		return ret;
>>>>>>>> +
>>>>>>>> +	irq = vgic_add_lpi(kvm, lpi_id);
>>>>>>>> +	if (IS_ERR(irq))
>>>>>>>> +		return PTR_ERR(irq);
>>>>>>>> +	ite->irq = irq;
>>>>>>>> +
>>>>>>>> +	/* restore the configuration of the LPI */
>>>>>>>> +	ret = update_lpi_config(kvm, irq, NULL);
>>>>>>>> +	if (ret)
>>>>>>>> +		return ret;
>>>>>>>> +
>>>>>>>> +	update_affinity_ite(kvm, ite);
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int vgic_its_ite_cmp(void *priv, struct list_head *a,
>>>>>>>> +			    struct list_head *b)
>>>>>>>> +{
>>>>>>>> +	struct its_ite *itea = container_of(a, struct its_ite, ite_list);
>>>>>>>> +	struct its_ite *iteb = container_of(b, struct its_ite, ite_list);
>>>>>>>> +
>>>>>>>> +	if (itea->event_id < iteb->event_id)
>>>>>>>> +		return -1;
>>>>>>>> +	else
>>>>>>>> +		return 1;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
>>>>>>>> +{
>>>>>>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>>>>>>>> +	gpa_t base = device->itt_addr;
>>>>>>>> +	struct its_ite *ite;
>>>>>>>> +	int ret, ite_esz = abi->ite_esz;
>>>>>>>
>>>>>>> nit: initializations on separate line
>>>>>> OK
>>>>>>>
>>>>>>>> +
>>>>>>>> +	list_sort(NULL, &device->itt_head, vgic_its_ite_cmp);
>>>>>>>> +
>>>>>>>> +	list_for_each_entry(ite, &device->itt_head, ite_list) {
>>>>>>>> +		gpa_t gpa = base + ite->event_id * ite_esz;
>>>>>>>> +
>>>>>>>> +		ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
>>>>>>>> +		if (ret)
>>>>>>>> +			return ret;
>>>>>>>> +	}
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
>>>>>>>> +{
>>>>>>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>>>>>>>> +	gpa_t base = dev->itt_addr;
>>>>>>>> +	int ret, ite_esz = abi->ite_esz;
>>>>>>>> +	size_t max_size = BIT_ULL(dev->nb_eventid_bits) * ite_esz;
>>>>>>>
>>>>>>> nit: initializations on separate line
>>>>>> OK
>>>>>>>
>>>>>>>> +
>>>>>>>> +	ret =  lookup_table(its, base, max_size, ite_esz, 0,
>>>>>>>> +			    vgic_its_restore_ite, dev);
>>>>>>>
>>>>>>> nit: extra white space
>>>>>>>
>>>>>>>> +
>>>>>>>> +	if (ret < 0)
>>>>>>>> +		return ret;
>>>>>>>> +
>>>>>>>> +	/* if the last element has not been found we are in trouble */
>>>>>>>> +	return ret ? 0 : -EINVAL;
>>>>>>>
>>>>>>> hmm, these are values potentially created by the guest in guest RAM,
>>>>>>> right?  So do we really abort migration and return an error to userspace
>>>>>>> in this case?
>>>>>> So we discussed with Peter/dave we shouldn't abort() in qemu in case of
>>>>>> such error. The restore table IOCTL will return an error. Up to qemu to
>>>>>> print the error. Destination guest will not be functional though.
>>>>>>
>>>>>
>>>>> ok, I'm just wondering if userspace can make a qualified decision based
>>>>> on this error code.  EINVAL typically means that userspace provided
>>>>> something incorrect, which I suppose in a sense is true, but this should
>>>>> be the only case where we return EINVAL here.
>>>>   Userspace must be able to
>>>>> tell the cases apart where the guest programmed bogus into memory before
>>>>> migration started, in which case we should ignore-and-resume, and where
>>>>> QEMU errornously provide some bogus value where the machine state
>>>>> becomes unreliable and must be powered down.
>>>> guest does not feed much besides few registers the ITS table restore
>>>> depends on. In case we want a more subtle error management at userspace
>>>> level all the error codes need to be revisited I am afraid. My plan was
>>>> to be more rough at the beginning and ignore & resume if ITS table
>>>> restore fails.
>>>>
>>>
>>> Do we require that the VM is quiesced the entire time between saving the
>>> ITS state to memory and copying all memory over the wire and capturing
>>> all register state?  If so, then an error to restore would be because of
>>> userspace doing something wrong and handling that accordingly is fine.
>>
>> yes the ITS table save into RAM starts when we have a guarantee that all
>> the VCPUS are stopped (we take all locks). 
> 
> The important bit is whether or not userspace is allowed to start any
> VCPUs again before copying over all RAM etc.  I suppose not.
no it is not meant to happen.
> 
>> The restore happens before
>> the VM gets resumed. At least this is the QEMU integration as of today.
>>
> 
> Does our ABI mandate this behavior (document it somewhere) ?
I will add this

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux