[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [RFC PATCH 21/49] ARM: new VGIC: Add acccessor to new struct vgic_irq instance
On 13/02/18 11:18, Andre Przywara wrote:
Hi,
Hi Andre,
On 12/02/18 17:42, Julien Grall wrote:
Hi Andre,
On 09/02/18 14:39, 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>
---
xen/arch/arm/vgic/vgic.c | 107
+++++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/vgic/vgic.h | 32 ++++++++++++++
2 files changed, 139 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..3075091caa
--- /dev/null
+++ b/xen/arch/arm/vgic/vgic.c
@@ -0,0 +1,107 @@
+/*
+ * 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 <xen/sched.h>
+
+#include <asm/arm_vgic.h>
+#include "vgic.h"
Please order the include alphabetically.
Sure.
+
+/*
+ * 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 think it would be worth thinking of a different data structure here.
The number of LPIs can be quite high for the hardware domain.
Probably true. I just didn't want to waste time on this yet, as we don't
have LPIs at the moment. Having a list has the big advantage of being
easy to understand and to implement, so I consider this an optimization
that we can have later.
I am not a big fan of adding code that will never be used as it is and
just too slow. That's going to have an impact on platform such as Thunder-X.
+ return NULL;
+}
+
+void vgic_put_irq(struct domain *d, struct vgic_irq *irq)
+{
+ struct vgic_dist *dist = &d->arch.vgic;
+
+ if ( irq->intid < VGIC_MIN_LPI )
+ return;
+
+ spin_lock(&dist->lpi_list_lock);
+ if ( !atomic_dec_and_test(&irq->refcount) )
+ {
+ spin_unlock(&dist->lpi_list_lock);
+ return;
+ };
+
+ list_del(&irq->lpi_list);
I would add
ASSERT(lpi_list_count >= 1);
But it is a bit hard to know whether this code is valid given you don't
have any implementation of ITS so far.
Is it? You should not need the actual ITS code to validate this
function. In fact there are only very few users in vgic-its.c.
The main point here is that you have textbook ref-counting: *Every* time
you take a pointer to an IRQ (vgic_get_irq), you have to tell the code
when you are done with it (vgic_put_irq).
So we decided to have this ref-counting done properly even though it's
pointless for SPIs, PPIs and SGIs, as it makes the code very clear to
read and verify.
Mostly you have get and put in one function, but sometimes there is more
time between them: for instance if an interrupt goes to the ap_list. We
"get" it, add it to the list, then return. When the guest has actually
handled this interrupt, we remove it from the list and only then "put"
it again.
When I read only this series, I can't tell why refcounting is necessary
for LPIs only. This is neither said in the commit message nor in the
cover letter. Note that I know the answer, I doubt the other may know it.
So yes, the code is trivial. But without the full logic/explanation, it
is hard to tell how the ITS is going to fit in and whether what you do
know looks sensible.
+ dist->lpi_list_count--;
+ spin_unlock(&dist->lpi_list_lock);
+
+ xfree(irq);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
new file mode 100644
index 0000000000..7a15cfdd79
--- /dev/null
+++ b/xen/arch/arm/vgic/vgic.h
To be honest, I am not a big fan of headers defined in the code bits. So
I would need a reason for that to be there and not in the include you
defined in the previous patch.
What is the problem with that?
The rationale here is to gather all definitions and prototypes that are
actually VGIC *internal*. No code outside of the actual VGIC
(xen/arch/arm/vgic/) should be concerned with it, and so I consider this
good style to keep this header file local. This makes it very clear that
no generic or arch code should ever tinker with anything defined in it.
Think about it like we could actually glue all those files in this new
directory together into one glorious new-vgic.c. Then we would not need
this header. But it's terrible to read and review, so we have this nice
split-up into vgic-mmio.c and vgic.c, for instance. And now we need this
header file to link those files together, to allow the MMIO emulation to
manipulate the state of an interrupt and queue it to a VCPU, for instance.
It's totally possible that there are definitions and prototypes in here
which don't belong here. TBH I didn't review this file here very
carefully for what we still need and what not, so I am happy to take
advice on what's wrong here.
Fair enough.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
