[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
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.
+
+/*
+ * 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.
+ {
+ if ( irq->intid != intid )
+ continue;
+
+ /*
+ * This increases the refcount, the caller is expected to
+ * call vgic_put_irq() later once it's finished with the IRQ.
+ */
+ vgic_get_irq_kref(irq);
+ goto out_unlock;
+ }
+ irq = NULL;
+
+out_unlock:
+ spin_unlock(&dist->lpi_list_lock);
+
+ return irq;
+}
+
+/*
+ * This looks up the virtual interrupt ID to get the corresponding
+ * struct vgic_irq. It also increases the refcount, so any caller is expected
+ * to call vgic_put_irq() once it's finished with this IRQ.
+ */
+struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
+ u32 intid)
Indentation.
+{
+ /* SGIs and PPIs */
+ if ( intid <= VGIC_MAX_PRIVATE )
+ return &vcpu->arch.vgic_cpu.private_irqs[intid];
+
+ /* SPIs */
+ if ( intid <= VGIC_MAX_SPI )
+ return &d->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
+
+ /* LPIs */
+ if ( intid >= VGIC_MIN_LPI )
+ return vgic_get_lpi(d, intid);
+
+ WARN();
Newline here please.
I would turn into an ASSERT_UNREACHABLE() so it is only happening in
debug build and avoid to worry about a guest exploiting that :).
+ 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.
+ 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.
@@ -0,0 +1,32 @@
+/*
+ * 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/>.
+ */
+#ifndef __XEN_ARM_VGIC_NEW_H__
+#define __XEN_ARM_VGIC_NEW_H__
This does not match the filename/path.
+
+struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
+ u32 intid);
+void vgic_put_irq(struct domain *d, struct vgic_irq *irq);
+
+static inline void vgic_get_irq_kref(struct vgic_irq *irq)
+{
+ if ( irq->intid < VGIC_MIN_LPI )
+ return;
+
+ atomic_inc(&irq->refcount);
+}
+
+#endif
Missing emacs magic.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
