[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH 20/49] ARM: new VGIC: Add data structure definitions



Hi Andre,

On 09/02/18 14:39, Andre Przywara wrote:
Add a new header file for the new and improved GIC implementation.
The big change is that we now have a struct vgic_irq per IRQ instead
of spreading all the information over various bitmaps in the ranks.

We include this new header conditionally from within the old header
file for the time being to avoid touching all the users.

This is based on Linux commit b18b57787f5e, written by Christoffer Dall.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
---
  xen/include/asm-arm/arm_vgic.h | 269 +++++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/domain.h   |   4 +
  xen/include/asm-arm/vgic.h     |   6 +
  3 files changed, 279 insertions(+)
  create mode 100644 xen/include/asm-arm/arm_vgic.h

diff --git a/xen/include/asm-arm/arm_vgic.h b/xen/include/asm-arm/arm_vgic.h
new file mode 100644
index 0000000000..865e9ee5bc
--- /dev/null
+++ b/xen/include/asm-arm/arm_vgic.h

arm_vgic.h is a confusing name. Can we name it vgic-new.h?

@@ -0,0 +1,269 @@
+/*
+ * Copyright (C) 2015, 2016 ARM Ltd.
+ *
+ * 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 __KVM_ARM_VGIC_H
+#define __KVM_ARM_VGIC_H

Please modify the guards based on the file name.

+
+#include <asm/atomic.h>
+#include <asm/mmio.h>
+#include <xen/spinlock.h>
+#include <xen/list.h>
+
+#define VGIC_V3_MAX_CPUS        255
+#define VGIC_V2_MAX_CPUS        8
+#define VGIC_NR_IRQS_LEGACY     256
+#define VGIC_NR_SGIS            16
+#define VGIC_NR_PPIS            16
+#define VGIC_NR_PRIVATE_IRQS    (VGIC_NR_SGIS + VGIC_NR_PPIS)
+#define VGIC_MAX_PRIVATE        (VGIC_NR_PRIVATE_IRQS - 1)
+#define VGIC_MAX_SPI            1019
+#define VGIC_MAX_RESERVED       1023
+#define VGIC_MIN_LPI            8192
+
+#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
+#define irq_is_spi(irq) ((irq) >= VGIC_NR_PRIVATE_IRQS && \
+                         (irq) <= VGIC_MAX_SPI)
+
+enum vgic_type {
+    VGIC_V2,        /* Good ol' GICv2 */
+    VGIC_V3,        /* New fancy GICv3 */
+};
+
+#define VGIC_V2_MAX_LRS         (1 << 6)
+#define VGIC_V3_MAX_LRS         16
+#define VGIC_V3_LR_INDEX(lr)    (VGIC_V3_MAX_LRS - 1 - lr)
+
+enum vgic_irq_config {
+    VGIC_CONFIG_EDGE = 0,

I don't think it is necessary to set 0 here as IIRC an enum always start at 0 if not specified.

+    VGIC_CONFIG_LEVEL
+};
+
+struct vgic_irq {
+    spinlock_t irq_lock;        /* Protects the content of the struct */
+    struct list_head lpi_list;  /* Used to link all LPIs together */
+    struct list_head ap_list;
+
+    struct vcpu *vcpu;          /*
+                                 * SGIs and PPIs: The VCPU
+                                 * SPIs and LPIs: The VCPU whose ap_list
+                                 * this is queued on.
+                                 */
+
+    struct vcpu *target_vcpu;   /*
+                                 * The VCPU that this interrupt should
+                                 * be sent to, as a result of the
+                                 * targets reg (v2) or the affinity reg (v3).
+                                 */
+
+    u32 intid;                  /* Guest visible INTID */

As we are using Xen coding style, please replace all u* with uint_*.

+    bool line_level;            /* Level only */
+    bool pending_latch;         /*
+                                 * The pending latch state used to
+                                 * calculate the pending state for both
+                                 * level and edge triggered IRQs.
+                                 */
+    bool active;                /* not used for LPIs */
+    bool enabled;
+    bool hw;                    /* Tied to HW IRQ */
+    atomic_t refcount;          /* Used for LPIs */
+    u32 hwintid;                /* HW INTID number */
+    union
+    {
+        u8 targets;             /* GICv2 target VCPUs mask */
+        u32 mpidr;              /* GICv3 target VCPU */
+    };
+    u8 source;                  /* GICv2 SGIs only */
+    u8 priority;
+    enum vgic_irq_config config;    /* Level or edge */
+};
+
+struct vgic_register_region;

Do we really need the forward declaration here?

+struct vgic_its;
+
+enum iodev_type {
+    IODEV_CPUIF,

I don't think this one is necessary.

+    IODEV_DIST,
+    IODEV_REDIST,
+    IODEV_ITS
+};
+
+struct vgic_io_device {
+    paddr_t base_addr;
+    union
+    {
+        struct vcpu *redist_vcpu;
+        struct vgic_its *its;
+    };
+    const struct vgic_register_region *regions;
+    enum iodev_type iodev_type;
+    int nr_regions;

unsigned int please.

+};
+
+struct vgic_its {
+    /* The base address of the ITS control register frame */
+    paddr_t     vgic_its_base;
+
+    bool        enabled;
+    struct vgic_io_device       iodev;
+
+    /* These registers correspond to GITS_BASER{0,1} */
+    u64         baser_device_table;
+    u64         baser_coll_table;
+
+    /* Protects the command queue */
+    spinlock_t  cmd_lock;
+    u64         cbaser;
+    u32         creadr;
+    u32         cwriter;
+
+    /* migration ABI revision in use */
+    u32         abi_rev;
+
+    /* Protects the device and collection lists */
+    spinlock_t  its_lock;
+    struct list_head            device_list;
+    struct list_head            collection_list;
+};
+
+struct vgic_dist {
+    bool                ready;
+    bool                initialized;
+
+    /* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
+    u32                 version;
+
+    /* Do injected MSIs require an additional device ID? */
+    bool                msis_require_devid;
+
+    int                 nr_spis;

Ditto.

+
+    /* base addresses in guest physical address space: */
+    paddr_t             vgic_dist_base;     /* distributor */
+    union
+    {
+        /* either a GICv2 CPU interface */
+        paddr_t         vgic_cpu_base;
+        /* or a number of GICv3 redistributor regions */
+        struct
+        {
+            paddr_t     vgic_redist_base;
+            paddr_t     vgic_redist_free_offset;
+        };
+    };
+
+    /* distributor enabled */
+    bool                enabled;
+
+    struct vgic_irq     *spis;
+    unsigned long       *allocated_irqs; /* bitmap of IRQs allocated */
+
+    struct vgic_io_device   dist_iodev;
+
+    bool                has_its;
+
+    /*
+     * Contains the attributes and gpa of the LPI configuration table.
+     * Since we report GICR_TYPER.CommonLPIAff as 0b00, we can share
+     * one address across all redistributors.
+     * GICv3 spec: 6.1.2 "LPI Configuration tables"
+     */
+    u64                 propbaser;
+
+    /* Protects the lpi_list and the count value below. */
+    spinlock_t          lpi_list_lock;
+    struct list_head    lpi_list_head;
+    int                 lpi_list_count;

Ditto.

+};
+
+struct vgic_v2_cpu_if {
+    u32     vgic_hcr;
+    u32     vgic_vmcr;
+    u64     vgic_elrsr; /* Saved only */
+    u32     vgic_apr;
+    u32     vgic_lr[VGIC_V2_MAX_LRS];
+};
+
+struct vgic_v3_cpu_if {
+    u32     vgic_hcr;
+    u32     vgic_vmcr;
+    u32     vgic_sre;   /* Restored only, change ignored */
+    u32     vgic_elrsr; /* Saved only */
+    u32     vgic_ap0r[4];
+    u32     vgic_ap1r[4];
+    u64     vgic_lr[VGIC_V3_MAX_LRS];
+};
+
+struct vgic_cpu {
+    /* CPU vif control registers for world switch */
+    union
+    {
+        struct vgic_v2_cpu_if   vgic_v2;
+        struct vgic_v3_cpu_if   vgic_v3;
+    };
+
+    unsigned int used_lrs;
+    struct vgic_irq private_irqs[VGIC_NR_PRIVATE_IRQS];
+
+    spinlock_t ap_list_lock;    /* Protects the ap_list */
+
+    /*
+     * List of IRQs that this VCPU should consider because they are either
+     * Active or Pending (hence the name; AP list), or because they recently
+     * were one of the two and need to be migrated off this list to another
+     * VCPU.
+     */
+    struct list_head ap_list_head;
+
+    /*
+     * Members below are used with GICv3 emulation only and represent
+     * parts of the redistributor.
+     */
+    struct vgic_io_device   rd_iodev;
+    struct vgic_io_device   sgi_iodev;
+
+    /* Contains the attributes and gpa of the LPI pending tables. */
+    u64 pendbaser;
+
+    bool lpis_enabled;
+
+    /* Cache guest priority bits */
+    u32 num_pri_bits;
+
+    /* Cache guest interrupt ID bits */
+    u32 num_id_bits;
+};
+
+extern struct static_key_false vgic_v2_cpuif_trap;
+extern struct static_key_false vgic_v3_cpuif_trap;

static_key_false does not exist on Xen.

+
+#define vgic_initialized(k) ((k)->arch.vgic.initialized)
+#define vgic_ready(k)       ((k)->arch.vgic.ready)
+#define vgic_valid_spi(k, i)    (((i) >= VGIC_NR_PRIVATE_IRQS) && \
+            ((i) < (k)->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS))
+
+bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
+bool vgic_v3_emulate(struct cpu_user_regs *regs, union hsr hsr);

While I am ok with the structure been defined up front, I would rather prefer to see the prototype defined with the actual implementation. This is much easier to see if you miss a prototype. And technically, this patch only add "data structure definitions".

+
+void vgic_clear_pending_irqs(struct vcpu *v);
+
+void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
+                      paddr_t vbase, void __iomem *hbase,
+                      uint32_t aliased_offset);
+struct rdist_region;
+void vgic_v3_setup_hw(paddr_t dbase, unsigned int nr_rdist_regions,
+                      const struct rdist_region *regions,
+                      unsigned int intid_bits);

Please don't re-define prototypes existing between the two implementation.

+
+#endif /* __KVM_ARM_VGIC_H */

Please fix the name here.

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 968ffb0c81..e8f2917140 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -198,7 +198,11 @@ struct arch_vcpu
      union gic_state_data gic;
      uint64_t lr_mask;
+#ifdef CONFIG_NEW_VGIC
+    struct vgic_cpu vgic_cpu;

Hmmm. Is there any point of the naming? You are in arch_vcpu so likely it will describe the vGIC CPU interface.

If you real want to do the renaming, then do it for the current vGIC as well.

+#else
      struct vgic_cpu vgic;
+#endif
/* Timer registers */
      uint32_t cntkctl;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 85ad2aca79..96b99f5c85 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -18,6 +18,10 @@
  #ifndef __ASM_ARM_VGIC_H__
  #define __ASM_ARM_VGIC_H__
+#ifdef CONFIG_NEW_VGIC
+#include <asm/arm_vgic.h>
+#else
+
  #include <xen/bitops.h>
  #include <xen/radix-tree.h>
  #include <xen/rbtree.h>
@@ -313,6 +317,8 @@ void vgic_v3_setup_hw(paddr_t dbase,
                        unsigned int intid_bits);
  #endif
+#endif /* !CONFIG_NEW_VGIC */
+
  /*** Common VGIC functions used by Xen arch code ****/
/*


Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.