RE: [PATCH v2 11/15] KVM: arm64: handle pending bit for LPIs in ITS emulation

[Pavel Fedin]

 Hello!

> +/* Called with the distributor lock held by the caller. */
> +void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int lpi)
> +{
> + struct vgic_its *its = >kvm->arch.vgic.its;
> + struct its_itte *itte;
> +
> + spin_lock(>lock);
> +
> + /* Find the right ITTE and put the pending state back in there */
> + itte = find_itte_by_lpi(vcpu->kvm, lpi);
> + if (itte)
> + __set_bit(vcpu->vcpu_id, itte->pending);
> +
> + spin_unlock(>lock);
> +}

 I am working on implementing live migration for the ITS. And here i have one 
fundamental problem.
vits_unqueue_lpi() processes only PENDING state. And looks like this 
corresponds to the HW
implementation, which has only bitwise pending table. But, in terms of 
migration, we can actually
have LPI in active state, while it's being processed.
 The question is - how can we handle it? Should we have one more bitwise table 
for active LPIs, or
is it enough to remember only a single, currently active LPI? Can LPIs be 
preempted on a real
hardware, or not?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/15] KVM: arm64: handle pending bit for LPIs in ITS emulation

[Christoffer Dall]

On Wed, Oct 07, 2015 at 11:39:30AM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > +/* Called with the distributor lock held by the caller. */
> > +void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int lpi)
> > +{
> > +   struct vgic_its *its = >kvm->arch.vgic.its;
> > +   struct its_itte *itte;
> > +
> > +   spin_lock(>lock);
> > +
> > +   /* Find the right ITTE and put the pending state back in there */
> > +   itte = find_itte_by_lpi(vcpu->kvm, lpi);
> > +   if (itte)
> > +   __set_bit(vcpu->vcpu_id, itte->pending);
> > +
> > +   spin_unlock(>lock);
> > +}
> 
>  I am working on implementing live migration for the ITS. And here i have one 
> fundamental problem.
> vits_unqueue_lpi() processes only PENDING state. And looks like this 
> corresponds to the HW
> implementation, which has only bitwise pending table. But, in terms of 
> migration, we can actually
> have LPI in active state, while it's being processed.

I thought LPIs had strict fire-and-forget semantics, not allowing any
active state, and that they are either pending or inactive?


>  The question is - how can we handle it? Should we have one more bitwise 
> table for active LPIs, or
> is it enough to remember only a single, currently active LPI? Can LPIs be 
> preempted on a real
> hardware, or not?
> 
Perhaps you're asking if LPIs have active state semantics on real
hardware and thus supports threaded interrupt handling for LPIs?

That is not supported on real hardware, which I think addresses your
concerns.

Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 11/15] KVM: arm64: handle pending bit for LPIs in ITS emulation

[Eric Auger]

On 08/25/2015 04:34 PM, Andre Przywara wrote:
> Hi Eric,
> 
> On 14/08/15 12:58, Eric Auger wrote:
>> On 07/10/2015 04:21 PM, 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 
>>> ---
>>>  include/kvm/arm_vgic.h  |  2 ++
>>>  virt/kvm/arm/its-emul.c | 71 
>>> 
>>>  virt/kvm/arm/its-emul.h |  3 ++
>>>  virt/kvm/arm/vgic-v3-emul.c |  2 ++
>>>  virt/kvm/arm/vgic.c | 72 
>>> ++---
>>>  5 files changed, 133 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index 1648668..2a67a10 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 7f217fa..b9c40d7 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;
>>>  };
>>>
>>> +#define for_each_lpi(dev, itte, kvm) \
>>> + list_for_each_entry(dev, &(kvm)->arch.vgic.its.device_list, dev_list) 
>>> \
>>> + list_for_each_entry(itte, &(dev)->itt, itte_list)
>>> +
>> You have a checkpatch error here:
>>
>> ERROR: Macros with complex values should be enclosed in parentheses
>> #52: FILE: virt/kvm/arm/its-emul.c:57:
>> +#define for_each_lpi(dev, itte, kvm) \
>> +   list_for_each_entry(dev, &(kvm)->arch.vgic.its.device_list, 
>> dev_list) \
>> +   list_for_each_entry(itte, &(dev)->itt, itte_list)
> 
> I know about that one. The problem is that if I add the parentheses it
> breaks the usage below due to the curly brackets. But the definition
> above is just so convenient and I couldn't find another neat solution so
> far. If you are concerned about that I can give it another try,
> otherwise I tend to just ignore checkpatch here.
OK maybe you can add a comment then?
> 
>>> +static struct its_itte *find_itte_by_lpi(struct kvm *kvm, int lpi)
>>> +{
>> can't we have the same LPI present in different interrupt translation
>> tables? I don't know it is a sensible setting but I did not succeed in
>> finding it was not possible.
> 
> Thanks to Marc I am happy (and relieved!) to point you to 6.1.1 LPI INTIDs:
> "The behavior of the GIC is UNPREDICTABLE if software:
> - Maps multiple EventID/DeviceID combinations to the same physical LPI
> INTID."
> 
> So I exercise the freedom of UNPREDICTABLE here ;-)

OK
> 
>>> + struct its_device *device;
>>> + struct its_itte *itte;
>>> +
>>> + for_each_lpi(device, itte, kvm) {
>>> + if (itte->lpi == lpi)
>>> + return itte;
>>> + }
>>> + return NULL;
>>> +}
>>> +
>>>  #define BASER_BASE_ADDRESS(x) ((x) & 0xf000ULL)
>>>
>>>  /* The distributor lock is held by the VGIC MMIO handler. */
>>> @@ -145,6 +163,59 @@ static bool handle_mmio_gits_idregs(struct kvm_vcpu 
>>> *vcpu,
>>>   return false;
>>>  }
>>>
>>> +/*
>>> + * Find all enabled and pending LPIs and queue them into the list
>>> + * registers.
>>> + * The dist lock is held by the caller.
>>> + */
>>> +bool vits_queue_lpis(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct vgic_its *its = >kvm->arch.vgic.its;
>>> + struct its_device *device;
>>> + struct its_itte *itte;
>>> + bool ret = true;
>>> +
>>> + if (!vgic_has_its(vcpu->kvm))
>>> + return true;
>>> + if (!its->enabled || !vcpu->kvm->arch.vgic.lpis_enabled)
>>> + return true;
>>> +
>>> + spin_lock(>lock);
>>> + for_each_lpi(device, itte, vcpu->kvm) {
>>> + if (!itte->enabled || !test_bit(vcpu->vcpu_id, itte->pending))
>>> + continue;
>>> +
>>> + if (!itte->collection)
>>> +   

Re: [PATCH v2 11/15] KVM: arm64: handle pending bit for LPIs in ITS emulation

[Andre Przywara]

Hi Eric,

On 14/08/15 12:58, Eric Auger wrote:
 On 07/10/2015 04:21 PM, 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.przyw...@arm.com
 ---
  include/kvm/arm_vgic.h  |  2 ++
  virt/kvm/arm/its-emul.c | 71 
 
  virt/kvm/arm/its-emul.h |  3 ++
  virt/kvm/arm/vgic-v3-emul.c |  2 ++
  virt/kvm/arm/vgic.c | 72 
 ++---
  5 files changed, 133 insertions(+), 17 deletions(-)

 diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
 index 1648668..2a67a10 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 7f217fa..b9c40d7 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;
  };

 +#define for_each_lpi(dev, itte, kvm) \
 + list_for_each_entry(dev, (kvm)-arch.vgic.its.device_list, dev_list) \
 + list_for_each_entry(itte, (dev)-itt, itte_list)
 +
 You have a checkpatch error here:
 
 ERROR: Macros with complex values should be enclosed in parentheses
 #52: FILE: virt/kvm/arm/its-emul.c:57:
 +#define for_each_lpi(dev, itte, kvm) \
 +   list_for_each_entry(dev, (kvm)-arch.vgic.its.device_list, dev_list) 
 \
 +   list_for_each_entry(itte, (dev)-itt, itte_list)

I know about that one. The problem is that if I add the parentheses it
breaks the usage below due to the curly brackets. But the definition
above is just so convenient and I couldn't find another neat solution so
far. If you are concerned about that I can give it another try,
otherwise I tend to just ignore checkpatch here.

 +static struct its_itte *find_itte_by_lpi(struct kvm *kvm, int lpi)
 +{
 can't we have the same LPI present in different interrupt translation
 tables? I don't know it is a sensible setting but I did not succeed in
 finding it was not possible.

Thanks to Marc I am happy (and relieved!) to point you to 6.1.1 LPI INTIDs:
The behavior of the GIC is UNPREDICTABLE if software:
- Maps multiple EventID/DeviceID combinations to the same physical LPI
INTID.

So I exercise the freedom of UNPREDICTABLE here ;-)

 + struct its_device *device;
 + struct its_itte *itte;
 +
 + for_each_lpi(device, itte, kvm) {
 + if (itte-lpi == lpi)
 + return itte;
 + }
 + return NULL;
 +}
 +
  #define BASER_BASE_ADDRESS(x) ((x)  0xf000ULL)

  /* The distributor lock is held by the VGIC MMIO handler. */
 @@ -145,6 +163,59 @@ static bool handle_mmio_gits_idregs(struct kvm_vcpu 
 *vcpu,
   return false;
  }

 +/*
 + * Find all enabled and pending LPIs and queue them into the list
 + * registers.
 + * The dist lock is held by the caller.
 + */
 +bool vits_queue_lpis(struct kvm_vcpu *vcpu)
 +{
 + struct vgic_its *its = vcpu-kvm-arch.vgic.its;
 + struct its_device *device;
 + struct its_itte *itte;
 + bool ret = true;
 +
 + if (!vgic_has_its(vcpu-kvm))
 + return true;
 + if (!its-enabled || !vcpu-kvm-arch.vgic.lpis_enabled)
 + return true;
 +
 + spin_lock(its-lock);
 + for_each_lpi(device, itte, vcpu-kvm) {
 + if (!itte-enabled || !test_bit(vcpu-vcpu_id, itte-pending))
 + continue;
 +
 + if (!itte-collection)
 + continue;
 +
 + if (itte-collection-target_addr != vcpu-vcpu_id)
 + continue;
 +
 + __clear_bit(vcpu-vcpu_id, itte-pending);
 +
 + ret = vgic_queue_irq(vcpu, 0, itte-lpi);
 what if the vgic_queue_irq fails since no LR can be found, the
 itte-pending was cleared so we forget that LPI? shouldn't we restore
 the pending state in ITT? in vgic_queue_hwirq the state change only is
 performed if the 

Re: [PATCH v2 11/15] KVM: arm64: handle pending bit for LPIs in ITS emulation

[Eric Auger]

On 07/10/2015 04:21 PM, 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.przyw...@arm.com
 ---
  include/kvm/arm_vgic.h  |  2 ++
  virt/kvm/arm/its-emul.c | 71 
  virt/kvm/arm/its-emul.h |  3 ++
  virt/kvm/arm/vgic-v3-emul.c |  2 ++
  virt/kvm/arm/vgic.c | 72 
 ++---
  5 files changed, 133 insertions(+), 17 deletions(-)
 
 diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
 index 1648668..2a67a10 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 7f217fa..b9c40d7 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;
  };
  
 +#define for_each_lpi(dev, itte, kvm) \
 + list_for_each_entry(dev, (kvm)-arch.vgic.its.device_list, dev_list) \
 + list_for_each_entry(itte, (dev)-itt, itte_list)
 +
You have a checkpatch error here:

ERROR: Macros with complex values should be enclosed in parentheses
#52: FILE: virt/kvm/arm/its-emul.c:57:
+#define for_each_lpi(dev, itte, kvm) \
+   list_for_each_entry(dev, (kvm)-arch.vgic.its.device_list, dev_list) \
+   list_for_each_entry(itte, (dev)-itt, itte_list)

 +static struct its_itte *find_itte_by_lpi(struct kvm *kvm, int lpi)
 +{
can't we have the same LPI present in different interrupt translation
tables? I don't know it is a sensible setting but I did not succeed in
finding it was not possible.
 + struct its_device *device;
 + struct its_itte *itte;
 +
 + for_each_lpi(device, itte, kvm) {
 + if (itte-lpi == lpi)
 + return itte;
 + }
 + return NULL;
 +}
 +
  #define BASER_BASE_ADDRESS(x) ((x)  0xf000ULL)
  
  /* The distributor lock is held by the VGIC MMIO handler. */
 @@ -145,6 +163,59 @@ static bool handle_mmio_gits_idregs(struct kvm_vcpu 
 *vcpu,
   return false;
  }
  
 +/*
 + * Find all enabled and pending LPIs and queue them into the list
 + * registers.
 + * The dist lock is held by the caller.
 + */
 +bool vits_queue_lpis(struct kvm_vcpu *vcpu)
 +{
 + struct vgic_its *its = vcpu-kvm-arch.vgic.its;
 + struct its_device *device;
 + struct its_itte *itte;
 + bool ret = true;
 +
 + if (!vgic_has_its(vcpu-kvm))
 + return true;
 + if (!its-enabled || !vcpu-kvm-arch.vgic.lpis_enabled)
 + return true;
 +
 + spin_lock(its-lock);
 + for_each_lpi(device, itte, vcpu-kvm) {
 + if (!itte-enabled || !test_bit(vcpu-vcpu_id, itte-pending))
 + continue;
 +
 + if (!itte-collection)
 + continue;
 +
 + if (itte-collection-target_addr != vcpu-vcpu_id)
 + continue;
 +
 + __clear_bit(vcpu-vcpu_id, itte-pending);
 +
 + ret = vgic_queue_irq(vcpu, 0, itte-lpi);
what if the vgic_queue_irq fails since no LR can be found, the
itte-pending was cleared so we forget that LPI? shouldn't we restore
the pending state in ITT? in vgic_queue_hwirq the state change only is
performed if the vgic_queue_irq succeeds
 + }
 +
 + spin_unlock(its-lock);
 + return ret;
 +}
 +
 +/* Called with the distributor lock held by the caller. */
 +void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int lpi)
I was a bit confused by the name of the function, with regard to
existing vgic_unqueue_irqs which restores the states in accordance to
what we have in LR. Wouldn't it make sense to call it
vits_lpi_set_pending(vcpu, lpi) or something that looks more similar to
vgic_dist_irq_set_pending setter which I think it mirrors.

 +{
 + struct vgic_its *its = vcpu-kvm-arch.vgic.its;
 + struct its_itte *itte;
 +
 +