Re: [PATCH v7 15/17] KVM: arm64: implement ITS command queue command handlers

[Andre Przywara <andre.przywara@arm.com>]

Hi Diana,

thanks for having such an elaborate look!

On 30/06/16 12:22, Diana Madalina Craciun wrote:
> Hi Andre,
> 
> On 06/28/2016 03:32 PM, Andre Przywara wrote:

...

>> +/* The MAPC command maps collection IDs to redistributors. */
>> +static int vits_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
>> +				u64 *its_cmd)
>> +{
>> +	u16 coll_id;
>> +	u32 target_addr;
>> +	struct its_collection *collection;
>> +	bool valid;
>> +	int ret = 0;
>> +
>> +	valid = its_cmd_get_validbit(its_cmd);
>> +	coll_id = its_cmd_get_collection(its_cmd);
>> +	target_addr = its_cmd_get_target_addr(its_cmd);
>> +
>> +	if (target_addr >= atomic_read(&kvm->online_vcpus))
>> +		return E_ITS_MAPC_PROCNUM_OOR;
>> +
>> +	mutex_lock(&its->its_lock);
>> +
>> +	collection = find_collection(its, coll_id);
>> +
>> +	if (!valid) {
>> +		struct its_device *device;
>> +		struct its_itte *itte;
>> +		/*
>> +		 * Clearing the mapping for that collection ID removes the
>> +		 * entry from the list. If there wasn't any before, we can
>> +		 * go home early.
>> +		 */
>> +		if (!collection)
>> +			goto out_unlock;
>> +
>> +		for_each_lpi_its(device, itte, its)
>> +			if (itte->collection &&
>> +			    itte->collection->collection_id == coll_id)
>> +				itte->collection = NULL;
>> +
>> +		list_del(&collection->coll_list);
>> +		kfree(collection);
>> +	} else {
>> +		if (!collection) {
>> +			collection = kzalloc(sizeof(struct its_collection),
>> +					     GFP_KERNEL);
>> +			if (!collection) {
>> +				ret = -ENOMEM;
>> +				goto out_unlock;
>> +			}
>> +		}
>> +
>> +		vits_init_collection(its, collection, coll_id);
>> +		collection->target_addr = target_addr;
> 
> Why initializing the collection also in the case it was previously
> found? Can't we end up adding a collection with the same ID twice to the
> collection list?

Oh right, the vits_init_collection() call has to move inside the if
clause. Good catch!

>> +		update_affinity_collection(kvm, its, collection);
> 
> In case the collection was newly allocated it has no interrupts mapped.
> So, I guess, it is no use iterating through the ITTE list because we
> will not find any interrupt.

Yes, though it doesn't hurt to do it anyway. I will try to create an
else clause and see how that looks like.

>> +	}
>> +
>> +out_unlock:
>> +	mutex_unlock(&its->its_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +/* The CLEAR command removes the pending state for a particular LPI. */
>> +static int vits_cmd_handle_clear(struct kvm *kvm, struct vgic_its *its,
>> +				 u64 *its_cmd)
>> +{
>> +	u32 device_id;
>> +	u32 event_id;
>> +	struct its_itte *itte;
>> +	int ret = 0;
>> +
>> +	device_id = its_cmd_get_deviceid(its_cmd);
>> +	event_id = its_cmd_get_id(its_cmd);
>> +
>> +	mutex_lock(&its->its_lock);
>> +
>> +	itte = find_itte(its, device_id, event_id);
>> +	if (!itte) {
>> +		ret = E_ITS_CLEAR_UNMAPPED_INTERRUPT;
>> +		goto out_unlock;
>> +	}
>> +
>> +	itte->irq->pending = false;
>> +
>> +out_unlock:
>> +	mutex_unlock(&its->its_lock);
>> +	return ret;
>> +}
>> +
>> +/* The INV command syncs the configuration bits from the memory table. */
>> +static int vits_cmd_handle_inv(struct kvm *kvm, struct vgic_its *its,
>> +			       u64 *its_cmd)
>> +{
>> +	u32 device_id;
>> +	u32 event_id;
>> +	struct its_itte *itte;
>> +	int ret;
>> +
>> +	device_id = its_cmd_get_deviceid(its_cmd);
>> +	event_id = its_cmd_get_id(its_cmd);
>> +
>> +	mutex_lock(&its->its_lock);
>> +
>> +	itte = find_itte(its, device_id, event_id);
>> +	if (!itte) {
>> +		ret = E_ITS_INV_UNMAPPED_INTERRUPT;
>> +		goto out_unlock;
>> +	}
>> +
>> +	ret = update_lpi_config(kvm, itte->irq);
>> +
>> +out_unlock:
>> +	mutex_unlock(&its->its_lock);
>> +	return ret;
>> +}
>> +
>> +/*
>> + * The INVALL command requests flushing of all IRQ data in this collection.
>> + * Find the VCPU mapped to that collection, then iterate over the VM's list
>> + * of mapped LPIs and update the configuration for each IRQ which targets
>> + * the specified vcpu. The configuration will be read from the in-memory
>> + * configuration table.
>> + */
>> +static int vits_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its,
>> +				  u64 *its_cmd)
>> +{
>> +	u32 coll_id = its_cmd_get_collection(its_cmd);
>> +	struct its_collection *collection;
>> +	struct kvm_vcpu *vcpu;
>> +	struct vgic_irq *irq;
>> +	u32 *intids;
>> +	int irq_count, i;
>> +
>> +	mutex_lock(&its->its_lock);
>> +
>> +	collection = find_collection(its, coll_id);
>> +	if (!its_is_collection_mapped(collection))
>> +		return E_ITS_INVALL_UNMAPPED_COLLECTION;
>> +
>> +	vcpu = kvm_get_vcpu(kvm, collection->target_addr);
>> +
>> +	irq_count = vits_copy_lpi_list(kvm, &intids);
>> +	if (irq_count < 0)
>> +		return irq_count;
>> +
>> +	for (i = 0; i < irq_count; i++) {
>> +		irq = vgic_get_irq(kvm, NULL, intids[i]);
>> +		if (!irq)
>> +			continue;
>> +		update_lpi_config_filtered(kvm, irq, vcpu);
>> +		vgic_put_irq_locked(kvm, irq);
>> +	}
>> +
>> +	kfree(intids);
>> +
>> +	mutex_unlock(&its->its_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * The MOVALL command moves the pending state of all IRQs targeting one
>> + * redistributor to another. We don't hold the pending state in the VCPUs,
>> + * but in the IRQs instead, so there is really not much to do for us here.
>> + * However the spec says that no IRQ must target the old redistributor
>> + * afterwards, so we make sure that no LPI is using the associated target_vcpu.
>> + * This command affects all LPIs in the system.
> 
> I am not sure I understand what "This command affects all LPIs in the
> system" means. Only the LPIs that are targeting redistributor 1 are
> affected.

Yes, but not only those LPIs that are mapped on _this_ ITS - that's why
we have to iterate the per-VM LPI list instead of just the ITS data
structures.
I think I will refine the comment, thanks for mentioning this.

> 
>> + */
>> +static int vits_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
>> +				  u64 *its_cmd)
>> +{
> 
> I am not sure I understand the spec correctly. So, after the movall
> instruction the target for all the interrupts targeting redistributor 1
> changed. However, what happens with the collection the interrupts are
> mapped to? I see that the target CPU for the collection does not change.
> The spec says: "In particular, an implementation might choose to remap
> all affected collections to RDbase2 ." I guess that the user should use
> mapc - movall combination for mapping the collection to another
> redistributor. Is my understanding correct?

Yes, movall alone is not sufficient to move LPIs from one redist to
another, it's just meant to move the _pending state_. The spec is indeed
not very clear about it, for instance "pending state" is only mentioned
in the pseudo code (that's why I also got it wrong in v5). And indeed I
was also reading the sentence you mentioned when implementing v5.
Frankly I am not sure we actually need this moving in our
implementation, but as the spec explicitly mentions this I thought it
would be a good idea to be sure of that.

Thanks again to the review!

Cheers,
Andre.

>> +	struct vgic_dist *dist = &kvm->arch.vgic;
>> +	u32 target1_addr = its_cmd_get_target_addr(its_cmd);
>> +	u32 target2_addr = its_cmd_mask_field(its_cmd, 3, 16, 32);
>> +	struct kvm_vcpu *vcpu1, *vcpu2;
>> +	struct vgic_irq *irq;
>> +
>> +	if (target1_addr >= atomic_read(&kvm->online_vcpus) ||
>> +	    target2_addr >= atomic_read(&kvm->online_vcpus))
>> +		return E_ITS_MOVALL_PROCNUM_OOR;
>> +
>> +	if (target1_addr == target2_addr)
>> +		return 0;
>> +
>> +	vcpu1 = kvm_get_vcpu(kvm, target1_addr);
>> +	vcpu2 = kvm_get_vcpu(kvm, target2_addr);
>> +
>> +	spin_lock(&dist->lpi_list_lock);
>> +
>> +	list_for_each_entry(irq, &dist->lpi_list_head, lpi_entry) {
>> +		spin_lock(&irq->irq_lock);
>> +
>> +		if (irq->target_vcpu == vcpu1)
>> +			irq->target_vcpu = vcpu2;
>> +
>> +		spin_unlock(&irq->irq_lock);
>> +	}
>> +
>> +	spin_unlock(&dist->lpi_list_lock);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * This function is called with the its_cmd lock held, but the ITS data
>> + * structure lock dropped. It is within the responsibility of the actual
>> + * command handlers to take care of proper locking when needed.
>> + */
>>  static int vits_handle_command(struct kvm *kvm, struct vgic_its *its,
>>  			       u64 *its_cmd)
>>  {
>> -	return -ENODEV;
>> +	u8 cmd = its_cmd_get_command(its_cmd);
>> +	int ret = -ENODEV;
>> +
>> +	switch (cmd) {
>> +	case GITS_CMD_MAPD:
>> +		ret = vits_cmd_handle_mapd(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_MAPC:
>> +		ret = vits_cmd_handle_mapc(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_MAPI:
>> +		ret = vits_cmd_handle_mapi(kvm, its, its_cmd, cmd);
>> +		break;
>> +	case GITS_CMD_MAPTI:
>> +		ret = vits_cmd_handle_mapi(kvm, its, its_cmd, cmd);
>> +		break;
>> +	case GITS_CMD_MOVI:
>> +		ret = vits_cmd_handle_movi(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_DISCARD:
>> +		ret = vits_cmd_handle_discard(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_CLEAR:
>> +		ret = vits_cmd_handle_clear(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_MOVALL:
>> +		ret = vits_cmd_handle_movall(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_INV:
>> +		ret = vits_cmd_handle_inv(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_INVALL:
>> +		ret = vits_cmd_handle_invall(kvm, its, its_cmd);
>> +		break;
>> +	case GITS_CMD_SYNC:
>> +		/* we ignore this command: we are in sync all of the time */
>> +		ret = 0;
>> +		break;
>> +	}
>> +
>> +	return ret;
>>  }
>>  
>>  static u64 vgic_sanitise_its_baser(u64 reg)
> 
> Thanks,
> 
> Diana
> 
> 
>