[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 28/57] ARM: new VGIC: Add acccessor to new struct vgic_irq instance
On 03/19/2018 05:32 PM, Andre Przywara wrote:
Hi,
Hi,
On 06/03/18 18:13, Julien Grall wrote:
Hi Andre,
On 05/03/18 16:03, Andre Przywara wrote:
The new VGIC implementation centers around a struct vgic_irq instance
per virtual IRQ.
Provide a function to retrieve the right instance for a given IRQ
number and (in case of private interrupts) the right VCPU.
This also includes the corresponding put function, which does nothing
for private interrupts and SPIs, but handles the ref-counting for LPIs.
This is based on Linux commit 64a959d66e47, written by Christoffer Dall.
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
---
Changelog RFC ... v1:
- add kernel-doc comments to exported functions
- adapt to previous changes (new_vgic.h, arch_vcpu member name)
- use ASSERT_UNREACHABLE
xen/arch/arm/vgic/vgic.c | 124
+++++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/vgic/vgic.h | 41 ++++++++++++++++
2 files changed, 165 insertions(+)
create mode 100644 xen/arch/arm/vgic/vgic.c
create mode 100644 xen/arch/arm/vgic/vgic.h
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
new file mode 100644
index 0000000000..ace30f78d0
--- /dev/null
+++ b/xen/arch/arm/vgic/vgic.c
@@ -0,0 +1,124 @@
+/*
+ * Copyright (C) 2015, 2016 ARM Ltd.
+ * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <asm/bug.h>
+#include <asm/new_vgic.h>
+#include <xen/sched.h>
+
+#include "vgic.h"
+
+/*
+ * Iterate over the VM's list of mapped LPIs to find the one with a
+ * matching interrupt ID and return a reference to the IRQ structure.
+ */
+static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid)
+{
+ struct vgic_dist *dist = &d->arch.vgic;
+ struct vgic_irq *irq = NULL;
+
+ spin_lock(&dist->lpi_list_lock);
+
+ list_for_each_entry( irq, &dist->lpi_list_head, lpi_list )
I am still not a big fan of the list solution. Strictly speaking nobody
is populating that list and likely going to be too slow in Xen case (I
am thinking for the hardware domain). So I think I would prefer to see
the LPI related code disappear for this cut. This could easily be added
back as they are standalone.
I was thinking about that, but dismissed the idea:
Considering LPIs as first class citizens is a crucial property of the
new VGIC and actually the main driver for its creation: the refcounting
is solely in for that purpose, and the per-IRQ data structure and its
lock are mainly driven by it. So removing the LPI support completely
will make the refcounting look very awkward (since useless and unused).
Consequently one would need to remove that as well. In the worst case we
run into issues with unused functions, like vgic_get_irq_kref().
I already removed a lot of code from the KVM VGIC, which I feel we will
need back one day. So I really like to keep this one in, and be it just
for documentation how the refcounting is supposed to be used.
I will add a comment to this respect.
And I believe replacing the list_for_each_entry() with something more
sophisticated is the least of our problems later on.
Using a list for LPIs means the search is O(n). That search will be done
each time you want to get the LPI information.
While in guest you may have limited LPIs, this will not be the case for
the hardware domain. Indeed that domain has access to all devices and
likely going to use a lot of MSIs.
So I still can't understand why you dismiss that problem.
To be honest, I don't mind to keep the refcounting around. What I don't
want to see in Xen is using a data structure that we know will not fit
Xen (even for well-behave guests).
You may remove a lot of code, and re-add the code later but that's the
price to convert KVM to Xen code. It makes much more sense to get all
the LPIs infrastructure together.
And to be clear, what I am asking for is implementing vgic_get_lpi with
return NULL + a comment.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
