Re: [PATCH v2 4/9] KVM: arm/arm64: vgic-its: Invalidate MSI-LPI translation cache on specific commands

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

 



On 23/07/2019 13:47, Auger Eric wrote:
> Hi Marc,
> 
> On 7/23/19 2:43 PM, Marc Zyngier wrote:
>> On 23/07/2019 13:25, Auger Eric wrote:
>>> Hi Marc,
>>>
>>> On 7/22/19 12:54 PM, Marc Zyngier wrote:
>>>> Hi Eric,
>>>>
>>>> On 01/07/2019 13:38, Auger Eric wrote:
>>>>> Hi Marc,
>>>>>
>>>>> On 6/11/19 7:03 PM, Marc Zyngier wrote:
>>>>>> The LPI translation cache needs to be discarded when an ITS command
>>>>>> may affect the translation of an LPI (DISCARD and MAPD with V=0) or
>>>>>> the routing of an LPI to a redistributor with disabled LPIs (MOVI,
>>>>>> MOVALL).
>>>>>>
>>>>>> We decide to perform a full invalidation of the cache, irrespective
>>>>>> of the LPI that is affected. Commands are supposed to be rare enough
>>>>>> that it doesn't matter.
>>>>>>
>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>>>> ---
>>>>>>  virt/kvm/arm/vgic/vgic-its.c | 8 ++++++++
>>>>>>  1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>>>>> index 9b6b66204b97..5254bb762e1b 100644
>>>>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>>>>> @@ -733,6 +733,8 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
>>>>>>  		 * don't bother here since we clear the ITTE anyway and the
>>>>>>  		 * pending state is a property of the ITTE struct.
>>>>>>  		 */
>>>>>> +		vgic_its_invalidate_cache(kvm);
>>>>>> +
>>>>>>  		its_free_ite(kvm, ite);
>>>>>>  		return 0;
>>>>>>  	}
>>>>>> @@ -768,6 +770,8 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>>>>>>  	ite->collection = collection;
>>>>>>  	vcpu = kvm_get_vcpu(kvm, collection->target_addr);
>>>>>>  
>>>>>> +	vgic_its_invalidate_cache(kvm);
>>>>>> +
>>>>>>  	return update_affinity(ite->irq, vcpu);
>>>>>>  }
>>>>>>  
>>>>>> @@ -996,6 +1000,8 @@ static void vgic_its_free_device(struct kvm *kvm, struct its_device *device)
>>>>>>  	list_for_each_entry_safe(ite, temp, &device->itt_head, ite_list)
>>>>>>  		its_free_ite(kvm, ite);
>>>>>>  
>>>>>> +	vgic_its_invalidate_cache(kvm);
>>>>>> +
>>>>>>  	list_del(&device->dev_list);
>>>>>>  	kfree(device);
>>>>>>  }
>>>>>> @@ -1249,6 +1255,8 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
>>>>>>  		vgic_put_irq(kvm, irq);
>>>>>>  	}
>>>>>>  
>>>>>> +	vgic_its_invalidate_cache(kvm);
>>>>> All the commands are executed with the its_lock held. Now we don't take
>>>>> it anymore on the fast cache injection path. Don't we have a window
>>>>> where the move has been applied at table level and the cache is not yet
>>>>> invalidated? Same question for vgic_its_free_device().
>>>>
>>>> There is definitely a race, but that race is invisible from the guest's
>>>> perspective. The guest can only assume that the command has taken effect
>>>> once a SYNC command has been executed, and it cannot observe that the
>>>> SYNC command has been executed before we have invalidated the cache.
>>>>
>>>> Does this answer your question?
>>>
>>> OK make sense. Thank you for the clarification
>>>
>>> Another question, don't we need to invalidate the cache on  MAPC V=0 as
>>> well? Removing the mapping of the collection results in interrupts
>>> belonging to that collection being ignored. If we don't flush the
>>> pending bit will be set?
>>
>> Yup, that's a good point. I think i had that at some point, and ended up 
>> dropping it, probably missing the point that the interrupt would be made 
>> pending.
>>
>> I'll add this:
>>
>> @@ -1218,6 +1218,7 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
>>  
>>  	if (!valid) {
>>  		vgic_its_free_collection(its, coll_id);
>> +		vgic_its_invalidate_cache(kvm);
>>  	} else {
>>  		collection = find_collection(its, coll_id);
>>  
> Yep, with that change feel free to add my R-b
> 
> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>

Thanks!

	M.
-- 
Jazz is not dead. It just smells funny...



[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