Re: [Qemu-devel] [PATCH 06/23] hw/intc/arm_gicv3: Add state information

[Shannon Zhao]

On 2016/5/10 1:29, Peter Maydell wrote:
> From: Pavel Fedin <address@hidden>
> 
> Add state information to GICv3 object structure and implement
> arm_gicv3_common_reset().
> 
> This commit includes accessor functions for the fields which are
> stored as bitmaps in uint32_t arrays.
> 
> Signed-off-by: Pavel Fedin <address@hidden>
> [PMM: significantly overhauled:
>  * Add missing qom/cpu.h include
>  * Remove legacy-only state fields (we can add them later if/when we add
>    legacy emulation)
>  * Use arrays of uint32_t to store the various distributor bitmaps,
>    and provide accessor functions for the various set/test/etc operations
>  * Add various missing register offset #defines
>  * Accessor macros which combine distributor and redistributor behaviour
>    removed
>  * Fields in state structures renamed to match architectural register names
>  * Corrected the reset value for GICR_IENABLER0 since we don't support
>    legacy mode
>  * Added ARM_LINUX_BOOT_IF interface for "we are directly booting a kernel in
>    non-secure" so that we can fake up the firmware-mandated reconfiguration
>    only when we need it
> ]
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/intc/arm_gicv3_common.c         | 140 ++++++++++++++++++++++++++-
>  hw/intc/gicv3_internal.h           | 172 +++++++++++++++++++++++++++++++++
>  include/hw/intc/arm_gicv3_common.h | 189 
> ++++++++++++++++++++++++++++++++++++-
>  3 files changed, 496 insertions(+), 5 deletions(-)
>  create mode 100644 hw/intc/gicv3_internal.h
> 
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index b9d3824..9ee4412 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -3,8 +3,9 @@
>   *
>   * Copyright (c) 2012 Linaro Limited
>   * Copyright (c) 2015 Huawei.
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
>   * Written by Peter Maydell
> - * Extended to 64 cores by Shlomo Pongratz
> + * Reworked for GICv3 by Shlomo Pongratz and Pavel Fedin
>   *
>   * 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
> @@ -22,7 +23,10 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qom/cpu.h"
>  #include "hw/intc/arm_gicv3_common.h"
> +#include "gicv3_internal.h"
> +#include "hw/arm/linux-boot-if.h"
>  
>  static void gicv3_pre_save(void *opaque)
>  {
> @@ -90,6 +94,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, 
> qemu_irq_handler handler,
>  static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
>  {
>      GICv3State *s = ARM_GICV3_COMMON(dev);
> +    int i;
>  
>      /* revision property is actually reserved and currently used only in 
> order
>       * to keep the interface compatible with GICv2 code, avoiding extra
> @@ -100,11 +105,136 @@ static void arm_gicv3_common_realize(DeviceState *dev, 
> Error **errp)
>          error_setg(errp, "unsupported GIC revision %d", s->revision);
>          return;
>      }
> +
> +    if (s->num_irq > GICV3_MAXIRQ) {
> +        error_setg(errp,
> +                   "requested %u interrupt lines exceeds GIC maximum %d",
> +                   s->num_irq, GICV3_MAXIRQ);
> +        return;
> +    }
> +
Does it need to check if s->num_irq is less than 32?

> +    s->cpu = g_new0(GICv3CPUState, s->num_cpu);
> +
And check if s->num_cpu is greater than 0?

> +    for (i = 0; i < s->num_cpu; i++) {
> +        CPUState *cpu = qemu_get_cpu(i);
> +        uint64_t cpu_affid;
> +        int last;
> +
> +        s->cpu[i].cpu = cpu;
> +        s->cpu[i].gic = s;
> +
> +        /* Pre-construct the GICR_TYPER:
> +         * For our implementation:
> +         *  Top 32 bits are the affinity value of the associated CPU
> +         *  CommonLPIAff == 01 (redistributors with same Aff3 share LPI 
> table)
> +         *  Processor_Number == CPU index starting from 0
> +         *  DPGS == 0 (GICR_CTLR.DPG* not supported)
> +         *  Last == 1 if this is the last redistributor in a series of
> +         *            contiguous redistributor pages
> +         *  DirectLPI == 0 (direct injection of LPIs not supported)
> +         *  VLPIS == 0 (virtual LPIs not supported)
> +         *  PLPIS == 0 (physical LPIs not supported)
> +         */
> +        cpu_affid = object_property_get_int(OBJECT(cpu), "mp-affinity", 
> NULL);
> +        last = (i == s->num_cpu - 1);
> +
> +        /* The CPU mp-affinity property is in MPIDR register format; squash
> +         * the affinity bytes into 32 bits as the GICR_TYPER has them.
> +         */
> +        cpu_affid = (cpu_affid & 0xFF00000000ULL >> 8) | (cpu_affid & 
> 0xFFFFFF);
> +        s->cpu[i].gicr_typer = (cpu_affid << 32) |
> +            (1 << 24) |
> +            (i << 8) |
> +            (last << 4);
> +    }
>  }
>  
>  static void arm_gicv3_common_reset(DeviceState *dev)
>  {
> -    /* TODO */
> +    GICv3State *s = ARM_GICV3_COMMON(dev);
> +    int i;
> +
> +    for (i = 0; i < s->num_cpu; i++) {
> +        GICv3CPUState *cs = &s->cpu[i];
> +
> +        cs->level = 0;
> +        cs->gicr_ctlr = 0;
> +        cs->gicr_statusr[GICV3_S] = 0;
> +        cs->gicr_statusr[GICV3_NS] = 0;
> +        cs->gicr_waker = GICR_WAKER_ProcessorSleep | 
> GICR_WAKER_ChildrenAsleep;
> +        cs->gicr_propbaser = 0;
> +        cs->gicr_pendbaser = 0;
> +        /* If we're resetting a TZ-aware GIC as if secure firmware
> +         * had set it up ready to start a kernel in non-secure, we
> +         * need to set interrupts to group 1 so the kernel can use them.
> +         * Otherwise they reset to group 0 like the hardware.
> +         */
> +        if (s->irq_reset_nonsecure) {
> +            cs->gicr_igroupr0 = 0xffffffff;
> +        } else {
> +            cs->gicr_igroupr0 = 0;
> +        }
> +
> +        cs->gicr_ienabler0 = 0;
> +        cs->gicr_ipendr0 = 0;
> +        cs->gicr_iactiver0 = 0;
> +        cs->edge_trigger = 0xffff;
> +        cs->gicr_igrpmodr0 = 0;
> +        cs->gicr_nsacr = 0;
> +        memset(cs->gicr_ipriorityr, 0, sizeof(cs->gicr_ipriorityr));
> +
> +        /* State in the CPU interface must *not* be reset here, because it
> +         * is part of the CPU's reset domain, not the GIC device's.
> +         */
> +    }
> +
> +    /* For our implementation affinity routing is always enabled */
> +    if (s->security_extn) {
> +        s->gicd_ctlr = GICD_CTLR_ARE_S | GICD_CTLR_ARE_NS;
> +    } else {
> +        s->gicd_ctlr = GICD_CTLR_DS | GICD_CTLR_ARE;
> +    }
> +
I'm a little confused with the no security support case, and in that
case, this GICv3 implementation supports only a single Security state,
right? If so, the SPEC says the DS bit is "Disable Security. This field
is RAO/WI." So why do you set the DS bit here?

[...]
> diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
> new file mode 100644
> index 0000000..d23524b
> --- /dev/null
> +++ b/hw/intc/gicv3_internal.h
> @@ -0,0 +1,172 @@
> +/*
> + * ARM GICv3 support - internal interfaces
> + *
> + * Copyright (c) 2012 Linaro Limited
> + * Copyright (c) 2015 Huawei.
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
> + * Written by Peter Maydell
> + * Reworked for GICv3 by Shlomo Pongratz and Pavel Fedin
> + *
> + * 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 QEMU_ARM_GICV3_INTERNAL_H
> +#define QEMU_ARM_GICV3_INTERNAL_H
> +
> +#include "hw/intc/arm_gicv3_common.h"
> +
> +/* Distributor registers, as offsets from the distributor base address */
> +#define GICD_CTLR            0x0000
> +#define GICD_TYPER           0x0004
> +#define GICD_IIDR            0x0008
> +#define GICD_STATUSR         0x0010
> +#define GICD_SETSPI_NSR      0x0040
> +#define GICD_CLRSPI_NSR      0x0048
> +#define GICD_SETSPI_SR       0x0050
> +#define GICD_CLRSPI_SR       0x0058
> +#define GICD_SEIR            0x0068
> +#define GICD_IGROUPR         0x0080
> +#define GICD_ISENABLER       0x0100
> +#define GICD_ICENABLER       0x0180
> +#define GICD_ISPENDR         0x0200
> +#define GICD_ICPENDR         0x0280
> +#define GICD_ISACTIVER       0x0300
> +#define GICD_ICACTIVER       0x0380
> +#define GICD_IPRIORITYR      0x0400
> +#define GICD_ITARGETSR       0x0800
> +#define GICD_ICFGR           0x0C00
> +#define GICD_IGRPMODR        0x0D00
> +#define GICD_NSACR           0x0E00
> +#define GICD_SGIR            0x0F00
> +#define GICD_CPENDSGIR       0x0F10
> +#define GICD_SPENDSGIR       0x0F20
> +#define GICD_IROUTER         0x6000
This should be 0x6100 or gicd_irouter[GICV3_MAXSPI] should use
GIC_MAXIRQ in struct GICv3State. Otherwise gicd_read_irouter() will be
wrong because s->gicd_irouter[irq] will be off normal upper if irq is
e.g. GIC_MAXIRQ - 1.

[...]

> +#define GICR_PROPBASER_OUTER_CACHEABILITY_MASK (7ULL << 56)
> +#define GICR_PROPBASER_ADDR_MASK               (0xfffffffffULL << 12)
> +#define GICR_PROPBASER_SHAREABILITY_MASK       (3U << 10)
> +#define GICR_PROPBASER_CACHEABILITY_MASK       (7U << 7)
> +#define GICR_PROPBASER_IDBITS_MASK             (0x1f)
Use (0x1f << 0) to keep consistent?

Thanks,
-- 
Shannon