Re: [PATCH 09/13] KVM: arm64: handle pending bit for LPIs in ITS emulation

[Eric Auger <eric.auger@linaro.org>]

On 06/11/2015 06:01 PM, Marc Zyngier wrote:
> On 11/06/15 16:46, Andre Przywara wrote:
>> Salut Eric,
>>
>> On 06/09/2015 04:59 PM, Eric Auger wrote:
>>> On 05/29/2015 11:53 AM, Andre Przywara wrote:
>>>> As the actual LPI number in a guest can be quite high, but is mostly
>>>> assigned using a very sparse allocation scheme, bitmaps and arrays
>>>> for storing the virtual interrupt status are a waste of memory.
>>>> We use our equivalent of the "Interrupt Translation Table Entry"
>>>> (ITTE) to hold this extra status information for a virtual LPI.
>>>> As the normal VGIC code cannot use it's fancy bitmaps to manage
>>>> pending interrupts, we provide a hook in the VGIC code to let the
>>>> ITS emulation handle the list register queueing itself.
>>>> LPIs are located in a separate number range (>=8192), so
>>>> distinguishing them is easy. With LPIs being only edge-triggered, we
>>>> get away with a less complex IRQ handling.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>  include/kvm/arm_vgic.h      |  2 ++
>>>>  virt/kvm/arm/its-emul.c     | 66 +++++++++++++++++++++++++++++++++++++++++++
>>>>  virt/kvm/arm/its-emul.h     |  3 ++
>>>>  virt/kvm/arm/vgic-v3-emul.c |  2 ++
>>>>  virt/kvm/arm/vgic.c         | 68 +++++++++++++++++++++++++++++++++------------
>>>>  5 files changed, 124 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>> index fa17df6..de19c34 100644
>>>> --- a/include/kvm/arm_vgic.h
>>>> +++ b/include/kvm/arm_vgic.h
>>>> @@ -147,6 +147,8 @@ struct vgic_vm_ops {
>>>>  	int	(*init_model)(struct kvm *);
>>>>  	void	(*destroy_model)(struct kvm *);
>>>>  	int	(*map_resources)(struct kvm *, const struct vgic_params *);
>>>> +	bool	(*queue_lpis)(struct kvm_vcpu *);
>>>> +	void	(*unqueue_lpi)(struct kvm_vcpu *, int irq);
>>>>  };
>>>>  
>>>>  struct vgic_io_device {
>>>> diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
>>>> index f0f4a9c..f75fb9e 100644
>>>> --- a/virt/kvm/arm/its-emul.c
>>>> +++ b/virt/kvm/arm/its-emul.c
>>>> @@ -50,8 +50,26 @@ struct its_itte {
>>>>  	struct its_collection *collection;
>>>>  	u32 lpi;
>>>>  	u32 event_id;
>>>> +	bool enabled;
>>>> +	unsigned long *pending;
>>> allocated in later patch. does not ease the review of the life cycle but
>>> I guess it is accepted/acceptable.
>>
>> I tried to move some bits around a bit and ran into several issues, so I
>> guess we have to live with that.
>>
>>> Isn't it somehow redundant to have a bitmap here where the collection
>>> already indicates the target cpu id on which the LPI is pending?
>>
>> Unfortunately only "somewhat", as Marc taught me the other day ;-)
>> First, the spec shows that the pending bitmap is allocated _per CPU_, so
>> we have to model this here appropriately.
>> Second, you could have an LPI pending on one distributor, then change
>> the associated collection to another distributor and trigger that
>> interrupt again. This would make it pending on two VCPUs.
>> Admittedly not the most prominent use case, but possible.
> 
> The exact scenario is related to the MOVI command, which changes the
> affinity of the interrupt and also move any pending state from another
> CPU. There is no guarantee that these two actions are completed
> atomically w.r.t the delivery of interrupts to CPUs.
> 
> We *could* make it atomic, but that would be quite heavy handed.

OK thanks,

The ITS command chapter is my next one ;-)

Best Regards

Eric
> 
> 	M.
>