Re: [PATCH 27/57] ARM: new VGIC: Add data structure definitions

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 05/03/18 16:03, 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@linaro.org>
> ---
> Changelog RFC ... v1:
> - rename header file to new_vgic.h
> - drop unneeded data structures (vgic_its, vgic_v<x>_cpu_if)
> - reorder members in vgic_irq to avoid padding
> - move flags members into bool bitfields
> - drop prototypes
> - use unsigned and uint<x>_t data types
> - keep arch_vcpu member name as "vgic"
> 
>   xen/include/asm-arm/new_vgic.h | 198 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/vgic.h     |   6 ++
>   2 files changed, 204 insertions(+)
>   create mode 100644 xen/include/asm-arm/new_vgic.h
> 
> diff --git a/xen/include/asm-arm/new_vgic.h b/xen/include/asm-arm/new_vgic.h
> new file mode 100644
> index 0000000000..54be5aa3eb
> --- /dev/null
> +++ b/xen/include/asm-arm/new_vgic.h
> @@ -0,0 +1,198 @@
> +/*
> + * 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 __ASM_ARM_NEW_VGIC_H
> +#define __ASM_ARM_NEW_VGIC_H
> +
> +#include <asm/atomic.h>
> +#include <asm/mmio.h>
> +#include <xen/list.h>
> +#include <xen/mm.h>
> +#include <xen/spinlock.h>
> +
> +#define VGIC_V3_MAX_CPUS        255
> +#define VGIC_V2_MAX_CPUS        8
> +#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,

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

Also, you might want to add a comment that this enum can only contain 
two values because of the way you store it (see bool config:1).

> +    VGIC_CONFIG_LEVEL
> +};
> +
> +struct vgic_irq {
> +    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).
> +                                 */
> +
> +    spinlock_t irq_lock;        /* Protects the content of the struct */
> +    uint32_t intid;             /* Guest visible INTID */
> +    atomic_t refcount;          /* Used for LPIs */
> +    uint32_t hwintid;           /* HW INTID number */
> +    union
> +    {
> +        struct {
> +            uint8_t targets;    /* GICv2 target VCPUs mask */
> +            uint8_t source;     /* GICv2 SGIs only */
> +        };
> +        uint32_t mpidr;         /* GICv3 target VCPU */
> +    };
> +    uint8_t priority;
> +    bool line_level:1;          /* Level only */
> +    bool pending_latch:1;       /*
> +                                 * The pending latch state used to
> +                                 * calculate the pending state for both
> +                                 * level and edge triggered IRQs.
> +                                 */
> +    bool active:1;              /* not used for LPIs */
> +    bool enabled:1;
> +    bool hw:1;                  /* Tied to HW IRQ */
> +    bool config:1;              /* Level or edge */
> +    struct list_head lpi_list;  /* Used to link all LPIs together */
> +};
> +
> +struct vgic_register_region;

Again, do we really need the forward declaration here?

> +
> +enum iodev_type {
> +    IODEV_DIST,
> +    IODEV_REDIST,
> +};
> +
> +struct vgic_io_device {
> +    gfn_t base_fn;
> +    struct vcpu *redist_vcpu;
> +    const struct vgic_register_region *regions;
> +    enum iodev_type iodev_type;
> +    unsigned int nr_regions;
> +};
> +
> +struct vgic_dist {
> +    bool                ready;
> +    bool                initialized;
> +
> +    /* vGIC model the kernel emulates for the guest (GICv2 or GICv3) */
> +    uint32_t            version;
> +
> +    /* Do injected MSIs require an additional device ID? */
> +    bool                msis_require_devid;
> +
> +    unsigned int        nr_spis;
> +
> +    /* 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"
> +     */
> +    uint64_t            propbaser;
> +
> +    /* Protects the lpi_list and the count value below. */
> +    spinlock_t          lpi_list_lock;
> +    struct list_head    lpi_list_head;
> +    unsigned int        lpi_list_count;
> +};
> +
> +struct vgic_cpu {
> +    struct vgic_irq private_irqs[VGIC_NR_PRIVATE_IRQS];
> +
> +    struct list_head ap_list_head;
> +    spinlock_t ap_list_lock;    /* Protects the ap_list */
> +
> +    unsigned int used_lrs;
> +
> +    /*
> +     * 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.
> +     */
> +
> +    /*
> +     * 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. */
> +    uint64_t pendbaser;
> +
> +    bool lpis_enabled;
> +
> +    /* Cache guest priority bits */
> +    uint32_t num_pri_bits;
> +
> +    /* Cache guest interrupt ID bits */
> +    uint32_t num_id_bits;
> +};
> +
> +#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))

What does k stands for? Shouldn't it be 'd' for domain?

> +
> +#endif /* __ASM_ARM_NEW_VGIC_H */

Missing emacs magic.

> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 84d82e6eb3..b28b8f8df7 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/new_vgic.h>
> +#else
> +
>   #include <xen/bitops.h>
>   #include <xen/radix-tree.h>
>   #include <xen/rbtree.h>
> @@ -299,6 +303,8 @@ extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
>                           const struct sgi_target *target);
>   extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
>   
> +#endif /* !CONFIG_NEW_VGIC */
> +
>   /*** Common VGIC functions used by Xen arch code ****/
>   
>   /*
> 

Cheers,

-- 
Julien Grall

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