[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state
|
From: |
Peter Maydell |
|
Subject: |
Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information |
|
Date: |
Fri, 23 Oct 2015 14:57:42 +0100 |
On 22 October 2015 at 15:02, Pavel Fedin <address@hidden> wrote:
> Add state information to GICv3 object structure and implement
> arm_gicv3_common_reset(). Also, add some functions for registers which are
> not stored directly but simulated.
>
> State information includes not only pure GICv3 data, but also some legacy
> registers. This will be useful for implementing software emulation of GICv3
> with v2 backwards compatilibity mode.
So we're going to (potentially) emulate:
* non-system-register config
* non-affinity-routing config
? OK, but are we really sure we want to do that? Legacy config
is deprecated and it's really going to complicate the model...
(Are we starting out with the non-legacy config that forces
system-regs and affinity-routing to always-on?)
> Signed-off-by: Pavel Fedin <address@hidden>
> ---
> hw/intc/arm_gicv3_common.c | 127 +++++++++++++++++-
> hw/intc/gicv3_internal.h | 265
> +++++++++++++++++++++++++++++++++++++
> include/hw/intc/arm_gicv3_common.h | 93 ++++++++++++-
> 3 files changed, 480 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 032ece2..2082d05 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
> @@ -21,6 +22,7 @@
> */
>
> #include "hw/intc/arm_gicv3_common.h"
> +#include "gicv3_internal.h"
>
> static void gicv3_pre_save(void *opaque)
> {
> @@ -88,6 +90,8 @@ 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);
> + Object *cpu;
> + unsigned int i, j;
>
> /* revision property is actually reserved and currently used only in
> order
> * to keep the interface compatible with GICv2 code, avoiding extra
> @@ -98,11 +102,130 @@ 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;
> + }
> +
> + s->cpu = g_malloc(s->num_cpu * sizeof(GICv3CPUState));
g_new0(GICv3CPUState, s->num_cpu);
> +
> + for (i = 0; i < s->num_cpu; i++) {
> + for (j = 0; j < GIC_NR_SGIS; j++) {
> + s->cpu[i].sgi[j].pending = g_malloc(BITS_TO_LONGS(s->num_cpu));
> + s->cpu[i].sgi[j].size = s->num_cpu;
> + }
> +
> + cpu = OBJECT(qemu_get_cpu(i));
> + s->cpu[i].affinity_id = object_property_get_int(cpu, "mp-affinity",
> + NULL);
> + }
> }
>
> static void arm_gicv3_common_reset(DeviceState *dev)
> {
> - /* TODO */
> + GICv3State *s = ARM_GICV3_COMMON(dev);
> + unsigned int i, j;
> +
> + for (i = 0; i < s->num_cpu; i++) {
> + GICv3CPUState *c = &s->cpu[i];
> +
> + c->cpu_enabled = false;
> + c->redist_ctlr = 0;
> + c->propbaser = GICR_PROPBASER_InnerShareable|GICR_PROPBASER_WaWb;
> + c->pendbaser = GICR_PENDBASER_InnerShareable|GICR_PENDBASER_WaWb;
> + /* Workaround!
> + * Linux (drivers/irqchip/irq-gic-v3.c) is enabling only group one,
> + * in gic_cpu_sys_reg_init it calls gic_write_grpen1(1);
> + * but it doesn't conigure any interrupt to be in group one.
> + * The same for SPIs below
> + */
Is this a bug in Linux, or is it just expecting that firmware configures
all interrupts into group 1 for it? (ie do we need some variation on
commit 8ff41f3995ad2d for gicv3 ?)
> + c->group = 0xffffffff;
> + /* GIC-500 comment 'j' SGI are always enabled */
> + c->enabled = 0x0000ffff;
> + c->pending = 0;
> + c->active = 0;
> + c->level = 0;
> + c->edge_trigger = 0x0000ffff;
> + memset(c->priority, 0, sizeof(c->priority));
> + for (j = 0; j < GIC_NR_SGIS; j++) {
> + bitmap_zero(c->sgi[j].pending, s->num_cpu);
> + }
> +
> + c->ctlr[0] = 0;
> + c->ctlr[1] = 0;
> + c->legacy_ctlr = 0;
> + c->priority_mask = 0;
> + c->bpr[0] = GIC_MIN_BPR0;
> + c->bpr[1] = GIC_MIN_BPR1;
> + memset(c->apr, 0, sizeof(c->apr));
> +
> + c->current_pending = 1023;
> + c->running_irq = 1023;
> + c->running_priority = 0x100;
> + memset(c->last_active, 0, sizeof(c->last_active));
> + }
> +
> + memset(s->group, 0, sizeof(s->group));
> + memset(s->enabled, 0, sizeof(s->enabled));
> + memset(s->pending, 0, sizeof(s->pending));
> + memset(s->active, 0, sizeof(s->active));
> + memset(s->level, 0, sizeof(s->level));
> + memset(s->edge_trigger, 0, sizeof(s->edge_trigger));
> +
> + /* Workaround! (the same as c->group above) */
> + for (i = GIC_INTERNAL; i < s->num_irq; i++) {
> + set_bit(i - GIC_INTERNAL, s->group);
> + }
> +
> + /* By default all interrupts always target CPU #0 */
> + for (i = 0; i < GICV3_MAXSPI; i++) {
> + s->irq_target[i] = 1;
> + }
> + memset(s->irq_route, 0, sizeof(s->irq_route));
> + memset(s->priority, 0, sizeof(s->priority));
> +
> + /* With all configuration we don't support GICv2 backwards computability
> */
> + if (s->security_extn) {
> + /* GICv3 5.3.20 With two security So DS is RAZ/WI ARE_NS is RAO/WI
> + * and ARE_S is RAO/WI
> + */
> + s->ctlr = GICD_CTLR_ARE_S | GICD_CTLR_ARE_NS;
> + } else {
> + /* GICv3 5.3.20 With one security So DS is RAO/WI ARE is RAO/WI
> + */
> + s->ctlr = GICD_CTLR_DS | GICD_CTLR_ARE;
> + }
> +}
> +
> +uint32_t gicv3_get_igrpen0(GICv3State *s, int cpuindex)
> +{
> + GICv3CPUState *c = &s->cpu[cpuindex];
> +
> + return extract32(c->legacy_ctlr, GICC_CTLR_EN_GRP0_SHIFT, 1);
> +}
My gut feeling is that if we alias legacy and system register
state, then we should do it by having the 'master' copy be
the system register format.
> +
> +void gicv3_set_igrpen0(GICv3State *s, int cpuindex, uint32_t val)
> +{
> + GICv3CPUState *c = &s->cpu[cpuindex];
> +
> + c->legacy_ctlr = deposit32(c->legacy_ctlr, GICC_CTLR_EN_GRP0_SHIFT, 1,
> val);
> +}
> +
> +uint32_t gicv3_get_igrpen1(GICv3State *s, int cpuindex)
> +{
> + GICv3CPUState *c = &s->cpu[cpuindex];
> +
> + return extract32(c->legacy_ctlr, GICC_CTLR_EN_GRP1_SHIFT, 1);
> +}
> +
> +void gicv3_set_igrpen1(GICv3State *s, int cpuindex, uint32_t val)
> +{
> + GICv3CPUState *c = &s->cpu[cpuindex];
> +
> + c->legacy_ctlr = deposit32(c->legacy_ctlr, GICC_CTLR_EN_GRP1_SHIFT, 1,
> val);
> }
>
> static Property arm_gicv3_common_properties[] = {
> diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
> new file mode 100644
> index 0000000..f1694b3
> --- /dev/null
> +++ b/hw/intc/gicv3_internal.h
> @@ -0,0 +1,265 @@
> +/*
> + * 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"
> +
> +/*
> + * Field manipulations
> + */
> +#define GIC_CLEAR_BIT(irq, ncpu, field) \
> + do { \
> + if ((irq) < GIC_INTERNAL) { \
> + s->cpu[(ncpu)].field &= ~(1 << (irq)); \
> + } else { \
> + clear_bit((irq) - GIC_INTERNAL, s->field); \
> + } \
> + } while (0)
> +#define GIC_SET_BIT(irq, ncpu, field) \
> + do { \
> + if ((irq) < GIC_INTERNAL) { \
> + s->cpu[(ncpu)].field |= 1 << (irq); \
> + } else { \
> + set_bit((irq) - GIC_INTERNAL, s->field); \
> + } \
> + } while (0)
Special-casing irq numbers less than GIC_INTERNAL like this looks a bit
odd. Since in general the difference is that the state for the
< GIC_INTERNAL irqs is in the distributor, whereas state for the others
is in the redistributor, I wouldn't expect code to generally want to
update the state without caring whether it's in the distributor or the
redistributor. Where we do need to do something different for these
cases I think it will be clearer to have the "internal irqs are different"
check in the calling code, not hidden in the accessors.
(This will also allow you to use proper functions rather than
macros, generally.)
> +#define GIC_REPLACE_BIT(irq, ncpu, field, val) \
> + do { \
> + if (val) { \
> + GIC_SET_BIT(irq, ncpu, field); \
> + } else { \
> + GIC_CLEAR_BIT(irq, ncpu, field); \
> + } \
> + } while (0)
> +#define GIC_TEST_BIT(irq, ncpu, field) \
> + (((irq) < GIC_INTERNAL) ? (s->cpu[(ncpu)].field >> (irq)) & 1 : \
> + test_bit((irq) - GIC_INTERNAL, s->field))
> +
> +#define GIC_SET_PRIORITY(irq, ncpu, pri) \
> + do { \
> + if ((irq) < GIC_INTERNAL) { \
> + s->cpu[(ncpu)].priority[(irq)] = (pri); \
> + } else { \
> + s->priority[(irq) - GIC_INTERNAL] = (pri); \
> + } \
> + } while (0)
> +#define GIC_GET_PRIORITY(irq, ncpu) \
> + (((irq) < GIC_INTERNAL) ? s->cpu[(ncpu)].priority[irq] : \
> + s->priority[(irq) - GIC_INTERNAL])
> +
> +#define GIC_GET_TARGET(irq, ncpu) \
> + (((irq) < GIC_INTERNAL) ? (1 << (ncpu)) : \
> + s->irq_target[(irq) - GIC_INTERNAL])
> +#define GIC_SET_TARGET(irq, cm) \
> + do { \
> + if ((irq) >= GIC_INTERNAL) { \
> + s->irq_target[(irq - GIC_INTERNAL)] = (cm); \
> + } \
> + } while (0)
> +
> +#define GIC_SET_GROUP(irq, cpu) GIC_SET_BIT(irq, cpu, group)
> +#define GIC_CLEAR_GROUP(irq, cpu) GIC_CLEAR_BIT(irq, cpu, group)
> +#define GIC_REPLACE_GROUP(irq, cpu, val) GIC_REPLACE_BIT(irq, cpu, group,
> val)
> +#define GIC_TEST_GROUP(irq, cpu) GIC_TEST_BIT(irq, cpu, group)
> +#define GIC_SET_ENABLED(irq, cpu) GIC_SET_BIT(irq, cpu, enabled)
> +#define GIC_CLEAR_ENABLED(irq, cpu) GIC_CLEAR_BIT(irq, cpu, enabled)
> +#define GIC_REPLACE_ENABLED(irq, cpu, val) \
> + GIC_REPLACE_BIT(irq, cpu, enabled,
> val)
> +#define GIC_TEST_ENABLED(irq, cpu) GIC_TEST_BIT(irq, cpu, enabled)
> +#define GIC_SET_PENDING(irq, cpu) GIC_SET_BIT(irq, cpu, pending)
> +#define GIC_CLEAR_PENDING(irq, cpu) GIC_CLEAR_BIT(irq, cpu, pending)
> +#define GIC_REPLACE_PENDING(irq, cpu, val) \
> + GIC_REPLACE_BIT(irq, cpu, pending,
> val)
> +#define GIC_TEST_PENDING(irq, cpu) GIC_TEST_BIT(irq, cpu, pending)
> +#define GIC_SET_ACTIVE(irq, cpu) GIC_SET_BIT(irq, cpu, active)
> +#define GIC_CLEAR_ACTIVE(irq, cpu) GIC_CLEAR_BIT(irq, cpu, active)
> +#define GIC_REPLACE_ACTIVE(irq, cpu, val) GIC_REPLACE_BIT(irq, cpu, active,
> val)
> +#define GIC_TEST_ACTIVE(irq, cpu) GIC_TEST_BIT(irq, cpu, active)
> +#define GIC_SET_LEVEL(irq, cpu) GIC_SET_BIT(irq, cpu, level)
> +#define GIC_CLEAR_LEVEL(irq, cpu) GIC_CLEAR_BIT(irq, cpu, level)
> +#define GIC_REPLACE_LEVEL(irq, cpu, val) GIC_REPLACE_BIT(irq, cpu, level,
> val)
> +#define GIC_TEST_LEVEL(irq, cpu) GIC_TEST_BIT(irq, cpu, level)
> +#define GIC_SET_EDGE_TRIGGER(irq, cpu) GIC_SET_BIT(irq, cpu, edge_trigger)
> +#define GIC_CLEAR_EDGE_TRIGGER(irq, cpu) GIC_CLEAR_BIT(irq, cpu,
> edge_trigger)
> +#define GIC_REPLACE_EDGE_TRIGGER(irq, cpu, val) \
> + GIC_REPLACE_BIT(irq, cpu, edge_trigger, val)
> +#define GIC_TEST_EDGE_TRIGGER(irq, cpu) GIC_TEST_BIT(irq, cpu, edge_trigger)
> +
> +static inline bool gic_test_pending(GICv3State *s, int irq, int cpu)
> +{
> + /* Edge-triggered interrupts are marked pending on a rising edge, but
> + * level-triggered interrupts are either considered pending when the
> + * level is active or if software has explicitly written to
> + * GICD_ISPENDR to set the state pending.
> + */
> + return GIC_TEST_PENDING(irq, cpu) ||
> + (!GIC_TEST_EDGE_TRIGGER(irq, cpu) && GIC_TEST_LEVEL(irq, cpu));
> +}
> +
> +
> +#define GICD_CTLR 0x0000
This block of GICD_ constants is missing a header comment.
> +#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_CPENDSGIR 0x0F10
> +#define GICD_SPENDSGIR 0x0F20
> +#define GICD_IROUTER 0x6000
> +#define GICD_PIDR2 0xFFE8
Missing GICD_IGRPMODR, GICD_NASCR, GICD_SGIR ?
> +
> +/* GICD_CTLR fields */
> +#define GICD_CTLR_EN_GRP0 (1U << 0)
> +#define GICD_CTLR_EN_GRP1NS (1U << 1) /* GICv3 5.3.20 */
> +#define GICD_CTLR_EN_GRP1S (1U << 2)
> +#define GICD_CTLR_EN_GRP1_ALL (GICD_CTLR_EN_GRP1NS |
> GICD_CTLR_EN_GRP1S)
> +#define GICD_CTLR_ARE (1U << 4)
> +#define GICD_CTLR_ARE_S (1U << 4)
Comment about why we're defining two bits with the same value?
> +#define GICD_CTLR_ARE_NS (1U << 5)
> +#define GICD_CTLR_DS (1U << 6)
> +#define GICD_CTLR_RWP (1U << 31)
Might as well add GICD_CTLR_E1NWF while we're here.
> +
> +/*
> + * Redistributor frame offsets from RD_base
> + */
> +#define GICR_SGI_OFFSET 0x10000
> +
> +/*
> + * Re-Distributor registers, offsets from RD_base
> + */
> +#define GICR_CTLR GICD_CTLR
The redistributor register layout has nothing to do with the
distributor register layout, so it doesn't make sense to define
the GICR_ constants in terms of the GICD_ ones.
> +#define GICR_IIDR 0x0004
> +#define GICR_TYPER 0x0008
> +#define GICR_STATUSR GICD_STATUSR
> +#define GICR_WAKER 0x0014
> +#define GICR_SETLPIR 0x0040
> +#define GICR_CLRLPIR 0x0048
> +#define GICR_SEIR GICD_SEIR
> +#define GICR_PROPBASER 0x0070
> +#define GICR_PENDBASER 0x0078
> +#define GICR_INVLPIR 0x00A0
> +#define GICR_INVALLR 0x00B0
> +#define GICR_SYNCR 0x00C0
> +#define GICR_MOVLPIR 0x0100
> +#define GICR_MOVALLR 0x0110
My copy of the GICv3 spec defines 0x100 and 0x110 as IMPDEF.
> +#define GICR_PIDR2 GICD_PIDR2
> +
> +#define GICR_CTLR_DPG1S (1U << 26)
> +#define GICR_CTLR_DPG1NS (1U << 25)
> +#define GICR_CTLR_DPG0 (1U << 24)
> +#define GICR_CTLR_ENABLE_LPIS (1U << 0)
Missing UWP and RWP.
Why is this set of constants defined from the highest bit
working down, when the GICD_CTLR constants were defined from
the lowest bit working up?
> +
> +#define GICR_TYPER_PLPIS (1U << 0)
> +#define GICR_TYPER_VLPIS (1U << 1)
> +#define GICR_TYPER_LAST (1U << 4)
Also missing various bits.
> +
> +#define GICR_WAKER_ProcessorSleep (1U << 1)
> +#define GICR_WAKER_ChildrenAsleep (1U << 2)
> +
> +#define GICR_PROPBASER_OUTER_AsInner (0ULL << 56)
> +#define GICR_PROPBASER_OUTER_nC (1ULL << 56)
> +#define GICR_PROPBASER_OUTER_RaWt (2ULL << 56)
> +#define GICR_PROPBASER_OUTER_RaWb (3ULL << 56)
> +#define GICR_PROPBASER_OUTER_WaWt (4ULL << 56)
> +#define GICR_PROPBASER_OUTER_WaWb (5ULL << 56)
> +#define GICR_PROPBASER_OUTER_RaWaWt (6ULL << 56)
> +#define GICR_PROPBASER_OUTER_RaWaWb (7ULL << 56)
> +#define GICR_PROPBASER_OUTER_CACHEABILITY_MASK (7ULL << 56)
> +#define GICR_PROPBASER_ADDR_MASK (0xfffffffffULL << 12)
> +#define GICR_PROPBASER_NonShareable (0U << 10)
> +#define GICR_PROPBASER_InnerShareable (1U << 10)
> +#define GICR_PROPBASER_OuterShareable (2U << 10)
> +#define GICR_PROPBASER_SHAREABILITY_MASK (3U << 10)
> +#define GICR_PROPBASER_nCnB (0U << 7)
> +#define GICR_PROPBASER_nC (1U << 7)
> +#define GICR_PROPBASER_RaWt (2U << 7)
> +#define GICR_PROPBASER_RaWb (3U << 7)
> +#define GICR_PROPBASER_WaWt (4U << 7)
> +#define GICR_PROPBASER_WaWb (5U << 7)
> +#define GICR_PROPBASER_RaWaWt (6U << 7)
> +#define GICR_PROPBASER_RaWaWb (7U << 7)
> +#define GICR_PROPBASER_CACHEABILITY_MASK (7U << 7)
> +#define GICR_PROPBASER_IDBITS_MASK (0x1f)
Do we really need all the defines for different values in the
InnerCache/OuterCache/Shareability fields, given that QEMU
doesn't model any of these memory attributes at all and so our
GICv3 emulation will just ignore whatever the guest writes here?
(If we do want them it seems better to define constants for the
various field values 0 through 7, plus a constant for the location
of the field, since the fields here and in PENDBASER are all
basically the same encoding.)
> +
> +#define GICR_PENDBASER_PTZ (1ULL << 62)
> +#define GICR_PENDBASER_OUTER_AsInner (0ULL << 56)
> +#define GICR_PENDBASER_OUTER_nC (1ULL << 56)
> +#define GICR_PENDBASER_OUTER_RaWt (2ULL << 56)
> +#define GICR_PENDBASER_OUTER_RaWb (3ULL << 56)
> +#define GICR_PENDBASER_OUTER_WaWt (4ULL << 56)
> +#define GICR_PENDBASER_OUTER_WaWb (5ULL << 56)
> +#define GICR_PENDBASER_OUTER_RaWaWt (6ULL << 56)
> +#define GICR_PENDBASER_OUTER_RaWaWb (7ULL << 56)
> +#define GICR_PENDBASER_OUTER_CACHEABILITY_MASK (7ULL << 56)
> +#define GICR_PENDBASER_ADDR_MASK (0xffffffffULL << 16)
> +#define GICR_PENDBASER_NonShareable (0U << 10)
> +#define GICR_PENDBASER_InnerShareable (1U << 10)
> +#define GICR_PENDBASER_OuterShareable (2U << 10)
> +#define GICR_PENDBASER_SHAREABILITY_MASK (3U << 10)
> +#define GICR_PENDBASER_nCnB (0U << 7)
> +#define GICR_PENDBASER_nC (1U << 7)
> +#define GICR_PENDBASER_RaWt (2U << 7)
> +#define GICR_PENDBASER_RaWb (3U << 7)
> +#define GICR_PENDBASER_WaWt (4U << 7)
> +#define GICR_PENDBASER_WaWb (5U << 7)
> +#define GICR_PENDBASER_RaWaWt (6U << 7)
> +#define GICR_PENDBASER_RaWaWb (7U << 7)
> +#define GICR_PENDBASER_CACHEABILITY_MASK (7U << 7)
> +
> +/*
> + * Simulated system registers
> + */
Not sure what "simulated system registers" means here.
> +#define GICC_CTLR_EN_GRP0_SHIFT 0
> +#define GICC_CTLR_EN_GRP0 (1U << GICC_CTLR_EN_GRP0_SHIFT)
> +#define GICC_CTLR_EN_GRP1_SHIFT 1
> +#define GICC_CTLR_EN_GRP1 (1U << GICC_CTLR_EN_GRP1_SHIFT)
> +#define GICC_CTLR_ACK_CTL (1U << 2)
> +#define GICC_CTLR_FIQ_EN (1U << 3)
> +#define GICC_CTLR_CBPR (1U << 4) /* GICv1: SBPR */
> +#define GICC_CTLR_EOIMODE (1U << 9)
> +#define GICC_CTLR_EOIMODE_NS (1U << 10)
We haven't defined a GICC_CTLR register offset, so why define
the bit fields for it?
> +
> +#define ICC_CTLR_CBPR (1U << 0)
> +#define ICC_CTLR_EOIMODE (1U << 1)
> +#define ICC_CTLR_PMHE (1U << 6)
Missing a few fields.
> +
> +#define ICC_PMR_PRIORITY_MASK 0xff
> +#define ICC_BPR_BINARYPOINT_MASK 0x07
> +#define ICC_IGRPEN_ENABLE 0x01
> +
> +#endif /* !QEMU_ARM_GIC_INTERNAL_H */
> diff --git a/include/hw/intc/arm_gicv3_common.h
> b/include/hw/intc/arm_gicv3_common.h
> index c2fd8da..c128622 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -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
> @@ -26,7 +27,66 @@
> #include "hw/sysbus.h"
> #include "hw/intc/arm_gic_common.h"
>
> -typedef struct GICv3State {
> +/*
> + * Maximum number of possible interrupts, determined by the GIC architecture.
> + * Note that this does not include LPIs. When implemented, these should be
> + * dealt with separately.
> + */
> +#define GICV3_MAXIRQ 1020
> +#define GICV3_MAXSPI (GICV3_MAXIRQ - GIC_INTERNAL)
> +
> +#define GIC_MIN_BPR0 0
> +#define GIC_MIN_BPR1 (GIC_MIN_BPR0 + 1)
> +
> +struct GICv3SGISource {
> + /* For each SGI on the target CPU, we store bit mask
> + * 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.
> + */
> + unsigned long *pending;
> + int32_t size; /* Bitmap size for migration */
> +};
This doesn't look right. There is one GICD_SPENDSGIR* register set
for each CPU, but each CPU has only 8 pending bits per SGI.
(That's because this is only relevant for legacy operation
with affinity-routing disabled, and the limitations on
legacy operation apply.) So you don't need a huge bitmap here.
I would default to modelling this as uint32_t spendsgir[4]
unless there's a good reason to do something else.
> +
> +typedef struct GICv3SGISource GICv3SGISource;
> +
> +struct GICv3CPUState {
> + uint64_t affinity_id; /* Cached affinity ID of the CPU */
> +
> + /* Redistributor */
> + bool cpu_enabled; /* !GICR_WAKER_ProcessorSleep */
Why not just have a field for GICR_WAKER, and read the relevant bit
as needed? (I'm assuming we won't in practice implement all the
GICv3 power-down signalling logic anyway.)
> + uint32_t redist_ctlr; /* GICR_CTLR */
If you named all these fields with their architectural names you
wouldn't have to have all these comments...
> + uint32_t group; /* GICR_IGROUPR0 */
> + uint32_t enabled; /* GICR_ISENABLER0 */
> + uint32_t pending; /* GICR_ISPENDR0 */
> + uint32_t active; /* GICR_ISACTIVER0 */
> + uint32_t level; /* Current IRQ level */
> + uint32_t edge_trigger; /* GICR_ICFGR */
GICR_ICFGR0 and GICR_ICFGR1 are both 32-bit registers, but we seem
to only have 32 bits of state here.
> + uint8_t priority[GIC_INTERNAL]; /* GICR_IPRIORITYR */
> + uint64_t propbaser;
> + uint64_t pendbaser;
> +
> + /* CPU interface */
> + uint32_t ctlr[2]; /* ICC_CTLR_EL1, banked */
> + uint32_t priority_mask; /* ICC_PMR_EL1 */
> + uint32_t bpr[2];
> + uint32_t apr[4][2];
> +
> + /* Legacy CPU interface */
> + uint32_t legacy_ctlr; /* GICC_CTLR */
> + GICv3SGISource sgi[GIC_NR_SGIS]; /* GIC_SPENDSGIR */
> +
> + /* Internal state */
There should in general be no internal state, unless it is purely
a cached representation of some architecturally visible state
(ie an optimisation for speed).
> + uint16_t current_pending;
> + uint16_t running_irq;
> + uint16_t running_priority;
> + uint16_t last_active[GICV3_MAXIRQ];
This last_active array is definitely wrong, as is running_irq. I
fixed the GICv2 not to try to handle priorities in this way in a
set of commits in September; the GICv3 implementation should work
similarly to how GICv2 does it now.
running_priority is not internal state, it is ICC_RPR_EL1.
current_pending is not internal state, it is ICC_HPPIR0_EL1 and
ICC_HPPIR1_EL1.
> +};
> +
> +typedef struct GICv3CPUState GICv3CPUState;
> +
> +struct GICv3State {
> /*< private >*/
> SysBusDevice parent_obj;
> /*< public >*/
> @@ -43,7 +103,28 @@ typedef struct GICv3State {
> bool security_extn;
>
> int dev_fd; /* kvm device fd if backed by kvm vgic support */
> -} GICv3State;
> + Error *migration_blocker;
> +
> + /* Distributor */
> +
> + /* 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.
> + */
This comment needs updating, because in GICv3 the NS banked version has
more than just one bit (though they are all still aliases of various bits
in the S version).
> + uint32_t ctlr; /* GICD_CTLR */
> + DECLARE_BITMAP(group, GICV3_MAXSPI); /* GICD_IGROUPR */
> + DECLARE_BITMAP(enabled, GICV3_MAXSPI); /* GICD_ISENABLER */
> + DECLARE_BITMAP(pending, GICV3_MAXSPI); /* GICD_ISPENDR */
> + DECLARE_BITMAP(active, GICV3_MAXSPI); /* GICD_ISACTIVER */
> + DECLARE_BITMAP(level, GICV3_MAXSPI); /* Current level */
> + DECLARE_BITMAP(edge_trigger, GICV3_MAXSPI); /* GICD_ICFGR */
GICD_ICFGR has two bits of state per interrupt, not one.
> + uint8_t priority[GICV3_MAXSPI]; /* GICD_IPRIORITYR */
> + uint8_t irq_target[GICV3_MAXSPI]; /* GICD_ITARGETSR */
> + uint64_t irq_route[GICV3_MAXSPI]; /* GICD_IROUTER */
> +
> + GICv3CPUState *cpu;
> +};
> +
> +typedef struct GICv3State GICv3State;
>
> #define TYPE_ARM_GICV3_COMMON "arm-gicv3-common"
> #define ARM_GICV3_COMMON(obj) \
> @@ -65,4 +146,10 @@ typedef struct ARMGICv3CommonClass {
> void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
> const MemoryRegionOps *ops);
>
> +/* Accessors for simulated system registers */
> +uint32_t gicv3_get_igrpen0(GICv3State *s, int cpuindex);
> +void gicv3_set_igrpen0(GICv3State *s, int cpuindex, uint32_t val);
> +uint32_t gicv3_get_igrpen1(GICv3State *s, int cpuindex);
> +void gicv3_set_igrpen1(GICv3State *s, int cpuindex, uint32_t val);
> +
> #endif
> --
> 2.4.4
thanks
-- PMM
[Qemu-devel] [RFC PATCH v3 2/4] kernel: Add definitions for GICv3 attributes, Pavel Fedin, 2015/10/22
[Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions, Pavel Fedin, 2015/10/22
[Qemu-devel] [RFC PATCH v3 4/4] hw/intc/arm_gicv3_common: Add vmstate descriptors, Pavel Fedin, 2015/10/22