RE: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation
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
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
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(&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)
+ 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(&its->lock);
}
+bool vits_check_lpis(struct kvm_vcpu *vcpu)
+{
+ struct vgic_its *its = &vcpu->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(&its->lock);
+ for_each_lpi(device, itte, vcpu->kvm) {
+ ret = lpi_is_pending(itte, vcpu->vcpu_id);
+ if (ret)
+ goto out;
+ }
+
+out:
+ spin_unlock(&its->lock);
+ return ret;
+}
+
static void its_free_itte(struct its_itte *itte)
{
list_del(&itte->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 --git a/virt/kv
RE: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation
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
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
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 4.3
RE: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation
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
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
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
Re: [PATCH v3 00/16] KVM: arm64: GICv3 ITS emulation
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
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
