Re: [Qemu-devel] [PATCH v7 2/6] Implement GIC-500 base class

[Peter Maydell]

On 24 July 2015 at 10:55, Pavel Fedin <address@hidden> wrote:
> From: Shlomo Pongratz <address@hidden>
>
> This class is to be used by both software and KVM implementations of GICv3
>

> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -0,0 +1,112 @@
> +/*
> + * ARM GIC support
> + *
> + * Copyright (c) 2012 Linaro Limited
> + * Copyright (c) 2015 Huawei.
> + * Written by Peter Maydell
> + * Extended to 64 cores by Shlomo Pongratz
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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 HW_ARM_GICV3_COMMON_H
> +#define HW_ARM_GICV3_COMMON_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/intc/arm_gic_common.h"
> +
> +/* Maximum number of possible interrupts, determined by the GIC architecture 
> */
> +#define GICV3_MAXIRQ 1020

So how do LPIs work? They have IDs above 1023.

> +#define GICV3_NCPU 64

Where does '64' come from as a maximum limit?

> +
> +typedef struct gicv3_irq_state {
> +    /* The enable bits are only banked for per-cpu interrupts.  */
> +    uint64_t enabled;
> +    uint64_t pending;
> +    uint64_t active;
> +    uint64_t level;
> +    uint64_t group;

Why are these uint64_t ?

> +    bool edge_trigger; /* true: edge-triggered, false: level-triggered  */
> +} gicv3_irq_state;
> +
> +typedef struct gicv3_sgi_state {
> +    uint64_t pending[GICV3_NCPU];
> +} gicv3_sgi_state;
> +
> +typedef struct GICv3State {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    qemu_irq parent_irq[GICV3_NCPU];
> +    qemu_irq parent_fiq[GICV3_NCPU];
> +    /* GICD_CTLR; for a GIC with the security extensions the NS banked 
> version
> +     * of this register is just an alias of bit 1 of the S banked version.
> +     */
> +    uint32_t ctlr;
> +    /* Sim GICC_CTLR; again, the NS banked version is just aliases of bits of
> +     * the S banked register, so our state only needs to store the S version.
> +     */

"Sim" ?

> +    uint32_t cpu_ctlr[GICV3_NCPU];
> +    bool cpu_enabled[GICV3_NCPU];
> +
> +    gicv3_irq_state irq_state[GICV3_MAXIRQ];
> +    uint64_t irq_target[GICV3_MAXIRQ];
> +    uint8_t priority1[GIC_INTERNAL][GICV3_NCPU];
> +    uint8_t priority2[GICV3_MAXIRQ - GIC_INTERNAL];
> +    uint16_t last_active[GICV3_MAXIRQ][GICV3_NCPU];
> +    /* For each SGI on the target CPU, we store 64 bits
> +     * indicating which source CPUs have made this SGI
> +     * pending on the target CPU. These correspond to
> +     * the bytes in the GIC_SPENDSGIR* registers as
> +     * read by the target CPU.

This comment doesn't make any sense. You can't fit
64 bits into a byte, and GIC_SPENDSGIR<n> only hold
8 set-pending bits per SGI.

> +     */
> +    gicv3_sgi_state sgi_state[GIC_NR_SGIS];
> +
> +    uint16_t priority_mask[GICV3_NCPU];
> +    uint16_t running_irq[GICV3_NCPU];
> +    uint16_t running_priority[GICV3_NCPU];
> +    uint16_t current_pending[GICV3_NCPU];
> +
> +    uint32_t cpu_mask; /* For redistributor */
> +    uint32_t num_cpu;
> +    MemoryRegion iomem_dist; /* Distributor */
> +    MemoryRegion iomem_lpi; /* Redistributor */
> +    uint32_t num_irq;
> +    uint32_t revision;
> +    bool security_levels;
> +    int dev_fd; /* kvm device fd if backed by kvm vgic support */
> +} GICv3State;

This whole struct reads like "we just took the GICv2 state
and changed it as little as possible beyond bumping the
NCPU define a bit". That doesn't make me very confident
that it's actually correct for GICv3...

thanks
-- PMM