Comment on the "workaround" see inline.
On Friday, October 23, 2015, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 22 October 2015 at 15:02, Pavel Fedin <p.fe...@samsung.com
> <javascript:;>> 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 <p.fe...@samsung.com <javascript:;>>
> > ---
> > 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 ?)
>
I think it is a bug in Linux as I explain in the comment, as far as I know
the kernel should not assume anything, that is if it wants ti use group one
it should initialize it, but since I don't have a board with a GICv3
(GIC500) I can't be sure.
My purpose is to make the "virt" virtual machine work. Maybe this
initialization should go there.
>
> > + 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
>
