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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 06/03/18 17:46, Julien Grall wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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.

But I consider this more readable, especially since this is a bool now.
So it's immediately obvious that EDGE is 0.
Shall I replace this with:
#define VGIC_CONFIG_EDGE false
#define VGIC_CONFIG_LEVEL true
now? Or using 0 and 1?

The enum vgic_irq_config type is now not used anywhere.

> 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?

No, we don't. I think I removed this, but it got reintroduced over a rebase.

>> +
>> +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?

Indeed, good catch.
K is the new D ;-)
But as we don't use those macros anyway, I will just remove them.

Cheers,
Andre.

>> +
>> +#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,
> 

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