Re: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation

[Eric Auger]

Hi Andre, Pavel
On 10/12/2015 05:18 PM, Pavel Fedin wrote:
>  Hello!
> 
>> Also what is the status of Eric's IRQ routing support? Should this go in
>> first now?
> 
>  I'd say without vITS there's nothing to use IRQ routing with. It could go in 
> and just lay around
> silently, so that it's not forgotten, but for example current qemu just knows 
> that with GICv2m it
> should use hardcoded linear MSI->SPI mapping.
Currently the gsi routing applies on top of ITS emulation series. I am
going to rebase it soon. It can go in 4.5 with ITS emulation series.

Best Regards

Eric
> 
> 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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Pavel Fedin]

 Hello!

> Currently the gsi routing applies on top of ITS emulation series. I am
> going to rebase it soon. It can go in 4.5 with ITS emulation series.

 Ah, yes, of course, because it reuses API definitions. I forgot this.

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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Pavel Fedin]

 Hello!

 I already suggested one bunch of fixes on top of vITS series, and here is 
another one. It reconciles it with spurious interrupt
fix, and adds missing check in vgic_retire_disabled_irqs(), which was removed 
in original v3 series.
---
>From bdbedc35a4dc9bc258b21792cf734aa3b2383dff Mon Sep 17 00:00:00 2001
From: Pavel Fedin 
Date: Tue, 13 Oct 2015 15:24:19 +0300
Subject: [PATCH] KVM: arm/arm64: Fix LPI loss

compute_pending_for_cpu() should return true if there's something pending
on the given vCPU. This is used in order to correctly set
dist->irq_pending_on_cpu flag. However, the function knows nothing about
LPIs, this can contribute to LPI loss.

This patch fixes it by introducing vits_check_lpis() function, which
returns true if there's any pending LPI. Also, some refactoring done,
wrapping some repeated checks into helper functions.

Additionally, vgic_retire_disabled_irqs() is fixed to correctly skip LPIs.

Signed-off-by: Pavel Fedin 
---
 include/kvm/arm_vgic.h  |  1 +
 virt/kvm/arm/its-emul.c | 46 +++--
 virt/kvm/arm/its-emul.h |  1 +
 virt/kvm/arm/vgic-v3-emul.c |  1 +
 virt/kvm/arm/vgic.c | 19 +--
 5 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 39113b9..21c8427 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -148,6 +148,7 @@ struct vgic_vm_ops {
int (*map_resources)(struct kvm *, const struct vgic_params *);
bool(*queue_lpis)(struct kvm_vcpu *);
void(*unqueue_lpi)(struct kvm_vcpu *, int irq);
+   bool(*check_lpis)(struct kvm_vcpu *);
int (*inject_msi)(struct kvm *, struct kvm_msi *);
 };
 
diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
index b1d61df..2fcd844 100644
--- a/virt/kvm/arm/its-emul.c
+++ b/virt/kvm/arm/its-emul.c
@@ -381,6 +381,18 @@ out_unlock:
return ret;
 }
 
+static bool its_is_enabled(struct kvm *kvm)
+{
+   return vgic_has_its(kvm) && kvm->arch.vgic.its.enabled &&
+  kvm->arch.vgic.lpis_enabled;
+}
+
+static bool lpi_is_pending(struct its_itte *itte, u32 vcpu_id)
+{
+   return itte->enabled && test_bit(vcpu_id, itte->pending) &&
+  itte->collection && (itte->collection->target_addr == vcpu_id);
+}
+
 /*
  * Find all enabled and pending LPIs and queue them into the list
  * registers.
@@ -393,20 +405,12 @@ bool vits_queue_lpis(struct kvm_vcpu *vcpu)
struct its_itte *itte;
bool ret = true;
 
-   if (!vgic_has_its(vcpu->kvm))
-   return true;
-   if (!its->enabled || !vcpu->kvm->arch.vgic.lpis_enabled)
+   if (!its_is_enabled(vcpu->kvm))
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)
-   continue;
-
-   if (itte->collection->target_addr != vcpu->vcpu_id)
+   if (!lpi_is_pending(itte, vcpu->vcpu_id))
continue;
 
 
@@ -436,6 +440,28 @@ void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int lpi)
spin_unlock(>lock);
 }
 
+bool vits_check_lpis(struct kvm_vcpu *vcpu)
+{
+   struct vgic_its *its = >kvm->arch.vgic.its;
+   struct its_device *device;
+   struct its_itte *itte;
+   bool ret = false;
+
+   if (!its_is_enabled(vcpu->kvm))
+   return false;
+
+   spin_lock(>lock);
+   for_each_lpi(device, itte, vcpu->kvm) {
+   ret = lpi_is_pending(itte, vcpu->vcpu_id);
+   if (ret)
+   goto out;
+   }
+
+out:
+   spin_unlock(>lock);
+   return ret;
+}
+
 static void its_free_itte(struct its_itte *itte)
 {
list_del(>itte_list);
diff --git a/virt/kvm/arm/its-emul.h b/virt/kvm/arm/its-emul.h
index 236f153..f7fa5f8 100644
--- a/virt/kvm/arm/its-emul.h
+++ b/virt/kvm/arm/its-emul.h
@@ -41,6 +41,7 @@ int vits_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
 
 bool vits_queue_lpis(struct kvm_vcpu *vcpu);
 void vits_unqueue_lpi(struct kvm_vcpu *vcpu, int irq);
+bool vits_check_lpis(struct kvm_vcpu *vcpu);
 
 #define E_ITS_MOVI_UNMAPPED_INTERRUPT  0x010107
 #define E_ITS_MOVI_UNMAPPED_COLLECTION 0x010109
diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
index 798f256..25463d0 100644
--- a/virt/kvm/arm/vgic-v3-emul.c
+++ b/virt/kvm/arm/vgic-v3-emul.c
@@ -966,6 +966,7 @@ void vgic_v3_init_emulation(struct kvm *kvm)
dist->vm_ops.inject_msi = vits_inject_msi;
dist->vm_ops.queue_lpis = vits_queue_lpis;
dist->vm_ops.unqueue_lpi = vits_unqueue_lpi;
+   dist->vm_ops.check_lpis = vits_check_lpis;
 
dist->vgic_dist_base = VGIC_ADDR_UNDEF;
dist->vgic_redist_base = VGIC_ADDR_UNDEF;
diff 

Re: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation

[Andre Przywara]

Hej,

On 10/10/15 16:37, Christoffer Dall wrote:
> Hi Andre,
> 
> 
> On Wed, Oct 07, 2015 at 03:55:10PM +0100, Andre Przywara wrote:
>> Hi,
>>
>> another respin and rebase of the ITS emulation series.
>> Major changes compared to v2 (beside some minor things like added
>> comments and function renames) are the rebasing and adaption to 4.3-rc
>> and Christoffer's timer rework series. Also the locking has been
>> reworked to cope with the dependencies of the its and the dist lock
>> in connection with the PROPBASER/PENDBASER and the command handling.
>> For a more detailed changelog see below or look at the respective
>> commit messages.
>>
>> This should address most of the comments I got on the list.
>> Many thanks to the diligent reviewers!
>> I didn't bother to fine-tune patch 01/16 too much, as I guess there
>> will be more discussion around this based on Pavel's latest post.
>>
>> These patches go on top of Christoffer's timer rework series [1],
>> which itself is on top of 4.3-rc2.
>> You can find all of this code in the its-emul/v3 branch of my
>> repository [2].
> 
> Thanks for rebasing the series!
> 
> Just a heads up that I may not be able to review this series for the
> next 1-2 weeks, so I'm afraid it's not going to make it in for v4.4,
> sorry.
> 
> Please let me know if this breaks expectations from everyone.

No worries, I wasn't expecting this for 4.4 anyway.
I'd rather see the prerequisites like your timer series going upstream
first, I will then rebase it on top of 4.4-rc1 (with fixes from newer
review comments incorporated).
Maybe we can take Pavel's cleanup (replacing my 1/16 and 2/16) for 4.4
already? (I will reply on those soon)
Also what is the status of Eric's IRQ routing support? Should this go in
first now?

Cheers,
Andre.

> Othersie, I will try review it with due dilligence so it makes it in for
> v4.5.
> 
> Best,
> -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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Pavel Fedin]

 Hello!

> Also what is the status of Eric's IRQ routing support? Should this go in
> first now?

 I'd say without vITS there's nothing to use IRQ routing with. It could go in 
and just lay around
silently, so that it's not forgotten, but for example current qemu just knows 
that with GICv2m it
should use hardcoded linear MSI->SPI mapping.

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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Christoffer Dall]

Hi Andre,


On Wed, Oct 07, 2015 at 03:55:10PM +0100, Andre Przywara wrote:
> Hi,
> 
> another respin and rebase of the ITS emulation series.
> Major changes compared to v2 (beside some minor things like added
> comments and function renames) are the rebasing and adaption to 4.3-rc
> and Christoffer's timer rework series. Also the locking has been
> reworked to cope with the dependencies of the its and the dist lock
> in connection with the PROPBASER/PENDBASER and the command handling.
> For a more detailed changelog see below or look at the respective
> commit messages.
> 
> This should address most of the comments I got on the list.
> Many thanks to the diligent reviewers!
> I didn't bother to fine-tune patch 01/16 too much, as I guess there
> will be more discussion around this based on Pavel's latest post.
> 
> These patches go on top of Christoffer's timer rework series [1],
> which itself is on top of 4.3-rc2.
> You can find all of this code in the its-emul/v3 branch of my
> repository [2].

Thanks for rebasing the series!

Just a heads up that I may not be able to review this series for the
next 1-2 weeks, so I'm afraid it's not going to make it in for v4.4,
sorry.

Please let me know if this breaks expectations from everyone.

Othersie, I will try review it with due dilligence so it makes it in for
v4.5.

Best,
-Christoffer

> 
> Changelog v2..v3:
> - adapt to 4.3-rc and Christoffer's timer rework
> - adapt spin locks on handling PROPBASER/PENDBASER registers
> - rework locking in ITS command handling (dropping dist where needed)
> - only clear LPI pending bit if LPI could actually be queued
> - simplify GICR_CTLR handling
> - properly free ITTEs (including our pending bitmap)
> - fix corner cases with unmapped collections
> - keep retire_lr() around
> - rename vgic_handle_base_register to vgic_reg64_access()
> - use kcalloc instead of kmalloc
> - minor fixes, renames and added comments
> 
> Changelog v1..v2
> - fix issues when using non-ITS GICv3 emulation
> - streamline frame address initialization (new patch 05/15)
> - preallocate buffer memory for reading from guest's memory
> - move locking into the actual command handlers
> -   preallocate memory for new structures if needed
> - use non-atomic __set_bit() and __clear_bit() when under the lock
> - add INT command handler to allow LPI injection from the guest
> - rewrite CWRITER handler to align with new locking scheme
> - remove unneeded CONFIG_HAVE_KVM_MSI #ifdefs
> - check memory table size against our LPI limit (65536 interrupts)
> - observe initial gap of 1024 interrupts in pending table
> - use term "configuration table" to be in line with the spec
> - clarify and extend documentation on API extensions
> - introduce new KVM_CAP_MSI_DEVID capability to advertise device ID 
> requirement
> - update, fix and add many comments
> - minor style changes as requested by reviewers
> 
> ---
> 
> The GICv3 ITS (Interrupt Translation Service) is a part of the
> ARM GICv3 interrupt controller [4] used for implementing MSIs.
> It specifies a new kind of interrupts (LPIs), which are mapped to
> establish a connection between a device, its MSI payload value and
> the target processor the IRQ is eventually delivered to.
> In order to allow using MSIs in an ARM64 KVM guest, we emulate this
> ITS widget in the kernel.
> The ITS works by reading commands written by software (from the guest
> in our case) into a (guest allocated) memory region and establishing
> the mapping between a device, the MSI payload and the target CPU.
> We parse these commands and update our internal data structures to
> reflect those changes. On an MSI injection we iterate those
> structures to learn the LPI number we have to inject.
> For the time being we use simple lists to hold the data, this is
> good enough for the small number of entries each of the components
> currently have. Should this become a performance bottleneck in the
> future, those can be extended to arrays or trees if needed.
> 
> Most of the code lives in a separate source file (its-emul.c), though
> there are some changes necessary both in vgic.c and vgic-v3-emul.c.
> 
> Patch 01/16 gets rid of the internal tracking of the used LR for
> an injected IRQ, see the commit message for more details.
> Patch 03/16 extends the KVM MSI ioctl to hold a device ID.
> Patch 04-06 make small changes to the existing VGIC code which make
> adaptions to the ITS later easier.
> The rest of the patches implement the ITS functionality step by step.
> For more details see the respective commit messages.
> 
> For the time being this series gives us the ability to use emulated
> PCI devices that can use MSIs in the guest. Those have to be
> triggered by letting the userland device emulation simulate the MSI
> write with the KVM_SIGNAL_MSI ioctl. This will be translated into
> the proper LPI by the ITS emulation and injected into the guest in
> the usual way (just with a higher IRQ number).
> 
> This series is based on 

RE: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation

[Pavel Fedin]

 Hello!

 Sorry for taking up your time, and thank you very much for the explanation.

> I'd appreciate if you could try to read and understand the architecture
> spec instead of randomly googling and quoting various bits of
> irrelevant information.

 I give my apologizes for not having time to read the whole specs from 
beginning to the end. Can
only add that it's quite weird to have these important things in "Terminology" 
section. I would
expect them to be in 6.1, for example. That was the part i read, but failed to 
find the exact
answer:
--- cut ---
LPIs do not have an active state, and transition to the inactive state on being 
acknowledged by a PE
--- cut ---

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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Marc Zyngier]

On Wed, 7 Oct 2015 21:09:07 +0300
Pavel Fedin  wrote:

>  Hello!
> 
> > LPIs do not have an active state, at the redistributor or otherwise.
> 
>  Then what do they become after they were ACK'ed and before EOI'ed?

Nothing. They are gone. What is left at the CPU interface is the active
priority.

>  I tried to google up this thing, and came up with this email:
> http://www.spinics.net/lists/kvm-arm/msg16032.html. It says that "SW must 
> issue a write to EOI to
> clear the active priorities register, hence the CPU interface still requires 
> an active state for
> LPIs". They give a link to some document which seems to be top-secret and 
> never published, because
> my arch reference manual does not have section 4.8.3 named "Properties of 
> LPI".

Your architecture document has a section 1.2.1 which contains the
sentence: "LPIs do not have an active state, and therefore do not
require explicit deactivation.". It also has 1.2.2 ("Interrupt states")
that repeatedly states the same thing. Finally, the email you quote is
about priority drop vs deactivation, not about the active state of an
LPI.

>  And another thread,
> http://lists.xen.org/archives/html/xen-devel/2014-09/msg01141.html,
> says that virtual LPIs actually do have active state in LR.

Or not. Read again. The only case where something vaguely relevant
happens is when you inject a virtual SPI backed by a HW LPI. In that
case, the LR does have an active state (of course, this is an SPI). Or
when you inject a virtual LPI backed by a HW SPI (in which case the
relevant active state is in the physical distributor, not in the LR).

I'd appreciate if you could try to read and understand the architecture
spec instead of randomly googling and quoting various bits of
irrelevant information.

If something is unclear in the architecture specification (yes, this
is complicated and sometimes confusing), please ask relevant questions.
At the moment, you're just asserting fallacies, and I'd rather spend
time doing something useful instead of setting the record straight
again and again.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny.
--
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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Marc Zyngier]

On 07/10/15 17:05, Pavel Fedin wrote:
>  Hello!
> 
>  One more concern about the whole thing. I already replied to the previous 
> series, but looks like my
> reply was missed.
>  Your implementation does not care about live migration at all. And there's 
> one fundamental issue
> with it. In the redistributor LPIs can be only pending, but in the CPU 
> interface they still can be
> active. And they have priorities, therefore they can be preempted, so we can 
> have even more than one
> active LPI at once. How to migrate this state?
>  Here i am trying to prototype this by leaving active interrupts in LRs and 
> allowing the userland to
> read/write them. This looks a bit stupid, additionally this will create 
> problems if we are e. g.
> migrating from host with 8 LRs to host with 4 LRs, while having 6 active 
> LPIs. Can anybody suggest
> better solution?
>  Technically LPI pending table has unused bits from 0 to 8191, and we have 
> 8192 LPIs, so we could
> push active state there, just for migration. Would this be a big violation of 
> specification? It says
> nothing about these bits at all.

LPIs do not have an active state, at the redistributor or otherwise.

M.
-- 
Jazz is not dead. It just smells funny...
--
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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Pavel Fedin]

 Hello!

 One more concern about the whole thing. I already replied to the previous 
series, but looks like my
reply was missed.
 Your implementation does not care about live migration at all. And there's one 
fundamental issue
with it. In the redistributor LPIs can be only pending, but in the CPU 
interface they still can be
active. And they have priorities, therefore they can be preempted, so we can 
have even more than one
active LPI at once. How to migrate this state?
 Here i am trying to prototype this by leaving active interrupts in LRs and 
allowing the userland to
read/write them. This looks a bit stupid, additionally this will create 
problems if we are e. g.
migrating from host with 8 LRs to host with 4 LRs, while having 6 active LPIs. 
Can anybody suggest
better solution?
 Technically LPI pending table has unused bits from 0 to 8191, and we have 8192 
LPIs, so we could
push active state there, just for migration. Would this be a big violation of 
specification? It says
nothing about these bits at all.

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 v3 00/16] KVM: arm64: GICv3 ITS emulation

[Pavel Fedin]

 Hello!

> LPIs do not have an active state, at the redistributor or otherwise.

 Then what do they become after they were ACK'ed and before EOI'ed?
 I tried to google up this thing, and came up with this email:
http://www.spinics.net/lists/kvm-arm/msg16032.html. It says that "SW must issue 
a write to EOI to
clear the active priorities register, hence the CPU interface still requires an 
active state for
LPIs". They give a link to some document which seems to be top-secret and never 
published, because
my arch reference manual does not have section 4.8.3 named "Properties of LPI".
 And another thread, 
http://lists.xen.org/archives/html/xen-devel/2014-09/msg01141.html, says that
virtual LPIs actually do have active state in LR.

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