* [Qemu-devel] [RFC PATCH v3 0/4] GICv3 live migration support
@ 2015-10-22 14:02 Pavel Fedin
2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information Pavel Fedin
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Pavel Fedin @ 2015-10-22 14:02 UTC (permalink / raw)
To: qemu-devel
Cc: Diana Craciun, Peter Maydell, Shlomo Pongratz, Vijay Kilari,
Shlomo Pongratz
This series introduces support for GICv3 live migration. It is based on
kernel API which is not released yet, therefore i post it as an RFC.
Kernel patches which implement this functionality are:
- [PATCH v5 0/7] KVM: arm64: Implement API for vGICv3 live migration
http://www.spinics.net/lists/kvm/msg122040.html
One more prerequisite for this series is:
- [PATCH] target-arm: Extract some external ARM CPU API
http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05269.html
The main purpose of this RFC is to agree on GICv3 state data format,
because software implementation of GICv3 is also going to use it. In order
to simplify GICv3 software emulation development, parts 1 and 2 of this
patchset can be accepted right now, without waiting for the kernel part.
v2 => v3:
- Integrated state manipulation macros from Shlomo Pongratz' GICv3 RFC
(with some changes)
- Added fields for SGI source masks
- Do not use mp-affinity property every time, cache IDs internally instead
- Removed mp-affinity property patch, now a part of prerequisite
v1 => v2:
- Use different kernel API, agreed upon by KVM developers
- Reworked state representation, do not duplicate SPI fields any more
- Added basic LPI support (PROPBASER and PENDBASER).
Pavel Fedin (4):
hw/intc/arm_gicv3_common: Add state information
kernel: Add definitions for GICv3 attributes
hw/intc/arm_gicv3_kvm: Implement get/put functions
hw/intc/arm_gicv3_common: Add vmstate descriptors
hw/intc/arm_gicv3_common.c | 185 ++++++++++++++-
hw/intc/arm_gicv3_kvm.c | 456 ++++++++++++++++++++++++++++++++++++-
hw/intc/gicv3_internal.h | 265 +++++++++++++++++++++
include/hw/intc/arm_gicv3_common.h | 93 +++++++-
include/migration/vmstate.h | 9 +
linux-headers/asm-arm64/kvm.h | 17 +-
6 files changed, 1011 insertions(+), 14 deletions(-)
create mode 100644 hw/intc/gicv3_internal.h
--
2.4.4
^ permalink raw reply [flat|nested] 14+ messages in thread* [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information 2015-10-22 14:02 [Qemu-devel] [RFC PATCH v3 0/4] GICv3 live migration support Pavel Fedin @ 2015-10-22 14:02 ` Pavel Fedin 2015-10-23 13:57 ` Peter Maydell 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 2/4] kernel: Add definitions for GICv3 attributes Pavel Fedin ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Pavel Fedin @ 2015-10-22 14:02 UTC (permalink / raw) To: qemu-devel Cc: Diana Craciun, Peter Maydell, Shlomo Pongratz, Vijay Kilari, Shlomo Pongratz 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. Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- 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)); + + 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 + */ + 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); +} + +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) +#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 +#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 + +/* 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) +#define GICD_CTLR_ARE_NS (1U << 5) +#define GICD_CTLR_DS (1U << 6) +#define GICD_CTLR_RWP (1U << 31) + +/* + * Redistributor frame offsets from RD_base + */ +#define GICR_SGI_OFFSET 0x10000 + +/* + * Re-Distributor registers, offsets from RD_base + */ +#define GICR_CTLR GICD_CTLR +#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 +#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) + +#define GICR_TYPER_PLPIS (1U << 0) +#define GICR_TYPER_VLPIS (1U << 1) +#define GICR_TYPER_LAST (1U << 4) + +#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) + +#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 + */ +#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) + +#define ICC_CTLR_CBPR (1U << 0) +#define ICC_CTLR_EOIMODE (1U << 1) +#define ICC_CTLR_PMHE (1U << 6) + +#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 */ +}; + +typedef struct GICv3SGISource GICv3SGISource; + +struct GICv3CPUState { + uint64_t affinity_id; /* Cached affinity ID of the CPU */ + + /* Redistributor */ + bool cpu_enabled; /* !GICR_WAKER_ProcessorSleep */ + uint32_t redist_ctlr; /* GICR_CTLR */ + 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 */ + 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 */ + uint16_t current_pending; + uint16_t running_irq; + uint16_t running_priority; + uint16_t last_active[GICV3_MAXIRQ]; +}; + +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. + */ + 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 */ + 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information Pavel Fedin @ 2015-10-23 13:57 ` Peter Maydell 2015-10-24 12:30 ` Shlomo Pongratz 2015-10-26 7:43 ` Pavel Fedin 0 siblings, 2 replies; 14+ messages in thread From: Peter Maydell @ 2015-10-23 13:57 UTC (permalink / raw) To: Pavel Fedin Cc: Diana Craciun, Shlomo Pongratz, Shlomo Pongratz, QEMU Developers, Vijay Kilari On 22 October 2015 at 15:02, Pavel Fedin <p.fedin@samsung.com> 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.fedin@samsung.com> > --- > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information 2015-10-23 13:57 ` Peter Maydell @ 2015-10-24 12:30 ` Shlomo Pongratz 2015-10-24 12:35 ` Peter Maydell 2015-10-26 7:43 ` Pavel Fedin 1 sibling, 1 reply; 14+ messages in thread From: Shlomo Pongratz @ 2015-10-24 12:30 UTC (permalink / raw) To: Peter Maydell Cc: Diana Craciun, Shlomo Pongratz, Pavel Fedin, QEMU Developers, Vijay Kilari [-- Attachment #1: Type: text/plain, Size: 30572 bytes --] Comment on the "workaround" see inline. On Friday, October 23, 2015, Peter Maydell <peter.maydell@linaro.org> wrote: > On 22 October 2015 at 15:02, Pavel Fedin <p.fedin@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.fedin@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 > [-- Attachment #2: Type: text/html, Size: 38221 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information 2015-10-24 12:30 ` Shlomo Pongratz @ 2015-10-24 12:35 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2015-10-24 12:35 UTC (permalink / raw) To: Shlomo Pongratz Cc: Vijay Kilari, Shlomo Pongratz, Pavel Fedin, QEMU Developers, Diana Craciun, kvmarm@lists.cs.columbia.edu On 24 October 2015 at 13:30, Shlomo Pongratz <shlomopongratz@gmail.com> wrote: > Comment on the "workaround" see inline. > >> > + /* 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. If it's a kernel bug we should just fix it in the kernel -- all this GICv3 support is still sufficiently early that it seems to me better to just fix the kernel than to carry around workarounds for bugs. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information 2015-10-23 13:57 ` Peter Maydell 2015-10-24 12:30 ` Shlomo Pongratz @ 2015-10-26 7:43 ` Pavel Fedin 2015-10-26 10:59 ` Peter Maydell 1 sibling, 1 reply; 14+ messages in thread From: Pavel Fedin @ 2015-10-26 7:43 UTC (permalink / raw) To: 'Peter Maydell' Cc: 'Diana Craciun', 'Shlomo Pongratz', 'Shlomo Pongratz', 'QEMU Developers', 'Vijay Kilari' Hello! I skipped many of comments because they are straightforward to address, decided to discuss only important ones. > 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... I remember that you told me that we are going to emulate full-blown GICv3, with HYP support and will all legacy stuff. You told me about this while merging the initial GICv3 series, so we reserved MMIO space for legacy CPU interface. So, i was pretty confident that we are going to do this over time, aren't we? > (Are we starting out with the non-legacy config that forces > system-regs and affinity-routing to always-on?) Yes, we are, but i also remember that you told me that migration data format, once implemented, is set in stone, so we should think very well. (*) So far, i added also the following legacy stuff: 1. GICC_CTLR This is currently used to store GRPEN bits. I thought that having dedicated bool's for them is too much, they are just single bits, and they have to be mirrored in GICC_CTLR, once implemented. So i just squashed them in there from the beginning. Additionally, some of its bits, like FIQBypDisGrpX and IRQBypDisGrpX, do not have analogs in system registers. So, if we even implement them, we'll have to store them in dedicated place, and now we already have this place. 2. GIC_SPENDSGIR (**) Actually this is not used by in-kernel vGIC, but this is necessary for SW emulation. Because, as far as i can understand Shlomo's code, for software emulation we need to track down which source CPUs are sending SGIs, even if we don't emulate legacy interface. Because, for example, if two CPUs send an SGI to another CPU at the same time, the destination CPU should actually get two interrupts. So, we have to track down whose interrupts are already delivered and whose are not yet. And Shlomo's code uses a bitmask for that, which i put in GICv3SGISource. 3. GICD_ITARGETSR Ok, this is actually not used yet, but again, this does not have any analog in system register interface, so once we have legacy mode, we have to store it somewhere. So i decided to reserve it too, because of (*). > > + /* 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 ?) To tell the truth i don't know, i left original Shlomo's comment. I'm not sure it’s really a bug, i think Linux relies on the firmware. All boards i know have some kind of trustzone code, even minimal one, and i believe it's its responsibility to set these things up. > > +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. Explained above. I could have bool grpen[2], but this would be two 32-bit variables, not used for anything else. And legacy_ctlr is a single 32-bit variable, which is potentially more functional, and provides better backwards compatibility for live migration data format. > > +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. This is about (**). Or do you want to tell that GICv3 with affinity routing enabled simply doesn't care about source CPUs, and if several CPUs trigger the same SGI for the same destination, the destination gets only one SGI? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information 2015-10-26 7:43 ` Pavel Fedin @ 2015-10-26 10:59 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2015-10-26 10:59 UTC (permalink / raw) To: Pavel Fedin Cc: Diana Craciun, Shlomo Pongratz, Shlomo Pongratz, QEMU Developers, Vijay Kilari On 26 October 2015 at 07:43, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > > I skipped many of comments because they are straightforward to address, decided to discuss only important ones. > >> 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... > > I remember that you told me that we are going to emulate full-blown > GICv3, with HYP support and will all legacy stuff. You told me about > this while merging the initial GICv3 series, so we reserved MMIO space > for legacy CPU interface. So, i was pretty confident that we are going > to do this over time, aren't we? Yeah, you're right -- we should at least leave ourselves the room to do that. >> (Are we starting out with the non-legacy config that forces >> system-regs and affinity-routing to always-on?) > > Yes, we are, but i also remember that you told me that migration data > format, once implemented, is set in stone, so we should think very well. You'll see from a comment I made in a later patch that we can have optional migration subsections, so we can have the data which is only needed in legacy configs be in its own subsection which won't break migration for non-legacy setups. It's the non-legacy used by KVM format we really need to get right from the start. > (*) So far, i added also the following legacy stuff: > > 1. GICC_CTLR > > This is currently used to store GRPEN bits. I thought that having > dedicated bool's for them is too much, they are just single bits, > and they have to be mirrored in GICC_CTLR, once implemented. So i > just squashed them in there from the beginning. Additionally, some > of its bits, like FIQBypDisGrpX and IRQBypDisGrpX, do not have > analogs in system registers. So, if we even implement them, we'll > have to store them in dedicated place, and now we already have this > place. The only reason not to want to do this is that it means we have legacy state in the not-legacy parts of the migration struct... > 2. GIC_SPENDSGIR (**) > > Actually this is not used by in-kernel vGIC, but this is necessary > for SW emulation. Because, as far as i can understand Shlomo's code, > for software emulation we need to track down which source CPUs are > sending SGIs, even if we don't emulate legacy interface. Because, > for example, if two CPUs send an SGI to another CPU at the same time, > the destination CPU should actually get two interrupts. So, we have > to track down whose interrupts are already delivered and whose are > not yet. And Shlomo's code uses a bitmask for that, which i put in > GICv3SGISource. I haven't looked into the details but this suggests to me that the emulation code is doing something wrong. The GICv3 registers should expose (one way or another) all the state in hardware, and a sw emulation version should not need any extra state in order to behave correctly. >> > +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. > > This is about (**). Or do you want to tell that GICv3 with affinity > routing enabled simply doesn't care about source CPUs, and if several > CPUs trigger the same SGI for the same destination, the destination > gets only one SGI? There is no hidden state in the GIC. If a non-legacy GICv3 has no register state to store information about source CPUs, then it cannot care about source CPUs. (For instance, note that the OS is not passed info about SGI source CPUs in the INTID bits [12:10] on reads of GICC_IAR if affinity routing is enabled.) One of the GICv2-to-GICv3+affinity changes is that in GICv3 SGIs were banked by both source and destination processor; in GICv3+affinity-routing, they are banked only by destination processor. thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC PATCH v3 2/4] kernel: Add definitions for GICv3 attributes 2015-10-22 14:02 [Qemu-devel] [RFC PATCH v3 0/4] GICv3 live migration support Pavel Fedin 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information Pavel Fedin @ 2015-10-22 14:02 ` Pavel Fedin 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions Pavel Fedin 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 4/4] hw/intc/arm_gicv3_common: Add vmstate descriptors Pavel Fedin 3 siblings, 0 replies; 14+ messages in thread From: Pavel Fedin @ 2015-10-22 14:02 UTC (permalink / raw) To: qemu-devel Cc: Diana Craciun, Peter Maydell, Shlomo Pongratz, Vijay Kilari, Shlomo Pongratz This temporary patch adds kernel API definitions. Use proper header update procedure after these features are released. Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- linux-headers/asm-arm64/kvm.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h index d3714c0..6e377c1 100644 --- a/linux-headers/asm-arm64/kvm.h +++ b/linux-headers/asm-arm64/kvm.h @@ -179,14 +179,14 @@ struct kvm_arch_memory_slot { KVM_REG_ARM64_SYSREG_ ## n ## _MASK) #define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \ - (KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \ - ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \ + (ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \ ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \ ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \ ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \ ARM64_SYS_REG_SHIFT_MASK(op2, OP2)) -#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64) +#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_ARM64 | \ + KVM_REG_SIZE_U64 | KVM_REG_ARM64_SYSREG) #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1) #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2) @@ -197,12 +197,21 @@ struct kvm_arch_memory_slot { #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS 1 #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS 2 #define KVM_DEV_ARM_VGIC_CPUID_SHIFT 32 -#define KVM_DEV_ARM_VGIC_CPUID_MASK (0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT) +#define KVM_DEV_ARM_VGIC_CPUID_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT) #define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0 #define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3 #define KVM_DEV_ARM_VGIC_GRP_CTRL 4 #define KVM_DEV_ARM_VGIC_CTRL_INIT 0 +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5 +#define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6 +#define KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \ + KVM_REG_ARM64_SYSREG_OP1_MASK | \ + KVM_REG_ARM64_SYSREG_CRN_MASK | \ + KVM_REG_ARM64_SYSREG_CRM_MASK | \ + KVM_REG_ARM64_SYSREG_OP2_MASK) +#define KVM_DEV_ARM_VGIC_SYSREG(op0,op1,crn,crm,op2) \ + __ARM64_SYS_REG(op0,op1,crn,crm,op2) /* KVM_IRQ_LINE irq field index values */ #define KVM_ARM_IRQ_TYPE_SHIFT 24 -- 2.4.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions 2015-10-22 14:02 [Qemu-devel] [RFC PATCH v3 0/4] GICv3 live migration support Pavel Fedin 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information Pavel Fedin 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 2/4] kernel: Add definitions for GICv3 attributes Pavel Fedin @ 2015-10-22 14:02 ` Pavel Fedin 2015-10-23 13:57 ` Peter Maydell 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 4/4] hw/intc/arm_gicv3_common: Add vmstate descriptors Pavel Fedin 3 siblings, 1 reply; 14+ messages in thread From: Pavel Fedin @ 2015-10-22 14:02 UTC (permalink / raw) To: qemu-devel Cc: Diana Craciun, Peter Maydell, Shlomo Pongratz, Vijay Kilari, Shlomo Pongratz This actually implements pre_save and post_load methods for in-kernel vGICv3. Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- hw/intc/arm_gicv3_kvm.c | 456 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 452 insertions(+), 4 deletions(-) diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c index b48f78f..ce8d2a0 100644 --- a/hw/intc/arm_gicv3_kvm.c +++ b/hw/intc/arm_gicv3_kvm.c @@ -21,8 +21,11 @@ #include "hw/intc/arm_gicv3_common.h" #include "hw/sysbus.h" +#include "migration/migration.h" +#include "qemu/error-report.h" #include "sysemu/kvm.h" #include "kvm_arm.h" +#include "gicv3_internal.h" #include "vgic_common.h" #ifdef DEBUG_GICV3_KVM @@ -41,6 +44,23 @@ #define KVM_ARM_GICV3_GET_CLASS(obj) \ OBJECT_GET_CLASS(KVMARMGICv3Class, (obj), TYPE_KVM_ARM_GICV3) +#define ICC_PMR_EL1 \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b0100, 0b0110, 0b000) +#define ICC_BPR0_EL1 \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1000, 0b011) +#define ICC_APR0_EL1(n) \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1000, 0b100 | n) +#define ICC_APR1_EL1(n) \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1001, 0b000 | n) +#define ICC_BPR1_EL1 \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b011) +#define ICC_CTLR_EL1 \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b100) +#define ICC_IGRPEN0_EL1 \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b110) +#define ICC_IGRPEN1_EL1 \ + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b111) + typedef struct KVMARMGICv3Class { ARMGICv3CommonClass parent_class; DeviceRealize parent_realize; @@ -54,16 +74,431 @@ static void kvm_arm_gicv3_set_irq(void *opaque, int irq, int level) kvm_arm_gic_set_irq(s->num_irq, irq, level); } +#define VGIC_CPUID(cpuid) ((((cpuid) & ARM_AFF3_MASK) >> 8) | \ + ((cpuid) & ARM32_AFFINITY_MASK)) +#define KVM_VGIC_ATTR(reg, cpuid) \ + ((VGIC_CPUID(cpuid) << KVM_DEV_ARM_VGIC_CPUID_SHIFT) | (reg)) + +static inline void kvm_gicd_access(GICv3State *s, int offset, int cpu, + uint64_t *val, bool write) +{ + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS, + KVM_VGIC_ATTR(offset, s->cpu[cpu].affinity_id), + val, write); +} + +static inline void kvm_gicr_access(GICv3State *s, int offset, int cpu, + uint64_t *val, bool write) +{ + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS, + KVM_VGIC_ATTR(offset, s->cpu[cpu].affinity_id), + val, write); +} + +static inline void kvm_gicc_access(GICv3State *s, uint64_t reg, int cpu, + uint64_t *val, bool write) +{ + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS, + KVM_VGIC_ATTR(reg, s->cpu[cpu].affinity_id), + val, write); +} + +/* + * Translate from the in-kernel field for an IRQ value to/from the qemu + * representation. + */ +typedef void (*vgic_translate_fn)(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel); + +/* synthetic translate function used for clear/set registers to completely + * clear a setting using a clear-register before setting the remaining bits + * using a set-register */ +static void translate_clear(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel) +{ + if (to_kernel) { + *field = ~0; + } else { + /* does not make sense: qemu model doesn't use set/clear regs */ + abort(); + } +} + +static void translate_enabled(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel) +{ + if (to_kernel) { + *field = GIC_TEST_ENABLED(irq, cpu); + } else { + GIC_REPLACE_ENABLED(irq, cpu, *field); + } +} + +static void translate_group(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel) +{ + if (to_kernel) { + *field = GIC_TEST_GROUP(irq, cpu); + } else { + GIC_REPLACE_GROUP(irq, cpu, *field); + } +} + +static void translate_trigger(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel) +{ + if (to_kernel) { + *field = GIC_TEST_EDGE_TRIGGER(irq, cpu) ? 2 : 0; + } else { + GIC_REPLACE_EDGE_TRIGGER(irq, cpu, *field & 2); + } +} + +static void translate_pending(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel) +{ + if (to_kernel) { + *field = gic_test_pending(s, irq, cpu); + } else { + GIC_REPLACE_PENDING(irq, cpu, *field); + /* TODO: Capture if level-line is held high in the kernel */ + } +} + +static void translate_active(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel) +{ + if (to_kernel) { + *field = GIC_TEST_ACTIVE(irq, cpu); + } else { + GIC_REPLACE_ACTIVE(irq, cpu, *field); + } +} + +static void translate_priority(GICv3State *s, int irq, int cpu, + uint32_t *field, bool to_kernel) +{ + if (to_kernel) { + *field = GIC_GET_PRIORITY(irq, cpu); + } else { + GIC_SET_PRIORITY(irq, cpu, *field); + } +} + +#define for_each_irq_reg(_irq, _max, _field_width) \ + for (_irq = 0; _irq < _max; _irq += (32 / _field_width)) + +/* Read a register group from the kernel VGIC */ +static void kvm_dist_get(GICv3State *s, uint32_t offset, int width, + vgic_translate_fn translate_fn) +{ + uint64_t reg; + int j; + int irq, cpu, maxcpu; + uint32_t field; + int regsz = 32 / width; /* irqs per kernel register */ + + for_each_irq_reg(irq, s->num_irq, width) { + maxcpu = irq < GIC_INTERNAL ? s->num_cpu : 1; + for (cpu = 0; cpu < maxcpu; cpu++) { + /* In GICv3 SGI/PPIs are stored in redistributor + * Offsets in SGI area are the same as in distributor + */ + if (irq < GIC_INTERNAL) { + kvm_gicr_access(s, offset + GICR_SGI_OFFSET, cpu, ®, false); + } else { + kvm_gicd_access(s, offset, cpu, ®, false); + } + for (j = 0; j < regsz; j++) { + field = extract32(reg, j * width, width); + translate_fn(s, irq + j, cpu, &field, false); + } + } + offset += 4; + } +} + +/* Write a register group to the kernel VGIC */ +static void kvm_dist_put(GICv3State *s, uint32_t offset, int width, + vgic_translate_fn translate_fn) +{ + uint64_t reg; + int j; + int irq, cpu, maxcpu; + uint32_t field; + int regsz = 32 / width; /* irqs per kernel register */ + + for_each_irq_reg(irq, s->num_irq, width) { + maxcpu = irq < GIC_INTERNAL ? s->num_cpu : 1; + for (cpu = 0; cpu < maxcpu; cpu++) { + reg = 0; + for (j = 0; j < regsz; j++) { + translate_fn(s, irq + j, cpu, &field, true); + reg = deposit32(reg, j * width, width, field); + } + /* In GICv3 SGI/PPIs are stored in redistributor + * Offsets in SGI area are the same as in distributor + */ + if (irq < GIC_INTERNAL) { + kvm_gicr_access(s, offset + GICR_SGI_OFFSET, cpu, ®, true); + } else { + kvm_gicd_access(s, offset, cpu, ®, true); + } + } + offset += 4; + } +} + +static void kvm_arm_gicv3_check(GICv3State *s) +{ + uint64_t reg; + uint32_t num_irq; + + /* Sanity checking s->num_irq */ + kvm_gicd_access(s, GICD_TYPER, 0, ®, false); + num_irq = ((reg & 0x1f) + 1) * 32; + + if (num_irq < s->num_irq) { + error_report("Model requests %u IRQs, but kernel supports max %u\n", + s->num_irq, num_irq); + abort(); + } + + /* TODO: Consider checking compatibility with the IIDR ? */ +} + static void kvm_arm_gicv3_put(GICv3State *s) { - /* TODO */ - DPRINTF("Cannot put kernel gic state, no kernel interface\n"); + uint64_t reg, redist_typer; + int ncpu, i; + + kvm_arm_gicv3_check(s); + + kvm_gicr_access(s, GICR_TYPER, 0, &redist_typer, false); + + /***************************************************************** + * (Re)distributor State + */ + + reg = s->ctlr; + kvm_gicd_access(s, GICD_CTLR, 0, ®, true); + + if (redist_typer & GICR_TYPER_PLPIS) { + /* Set base addresses before LPIs are enabled by GICR_CTLR write */ + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { + GICv3CPUState *c = &s->cpu[ncpu]; + + reg = c->propbaser & (GICR_PROPBASER_OUTER_CACHEABILITY_MASK | + GICR_PROPBASER_ADDR_MASK | + GICR_PROPBASER_SHAREABILITY_MASK | + GICR_PROPBASER_CACHEABILITY_MASK | + GICR_PROPBASER_IDBITS_MASK); + kvm_gicr_access(s, GICR_PROPBASER, ncpu, ®, true); + + reg = c->pendbaser & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | + GICR_PENDBASER_ADDR_MASK | + GICR_PENDBASER_SHAREABILITY_MASK | + GICR_PENDBASER_CACHEABILITY_MASK); + if (!c->redist_ctlr & GICR_CTLR_ENABLE_LPIS) { + reg |= GICR_PENDBASER_PTZ; + } + kvm_gicr_access(s, GICR_PENDBASER, ncpu, ®, true); + } + } + + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { + GICv3CPUState *c = &s->cpu[ncpu]; + + reg = c->redist_ctlr & (GICR_CTLR_ENABLE_LPIS | GICR_CTLR_DPG0 | + GICR_CTLR_DPG1NS | GICR_CTLR_DPG1S); + kvm_gicr_access(s, GICR_CTLR, ncpu, ®, true); + + reg = c->cpu_enabled ? 0 : GICR_WAKER_ProcessorSleep; + kvm_gicr_access(s, GICR_WAKER, ncpu, ®, true); + } + + /* irq_state[n].enabled -> GICD_ISENABLERn */ + kvm_dist_put(s, GICD_ICENABLER, 1, translate_clear); + kvm_dist_put(s, GICD_ISENABLER, 1, translate_enabled); + + /* irq_state[n].group -> GICD_IGROUPRn */ + kvm_dist_put(s, GICD_IGROUPR, 1, translate_group); + + /* Restore targets before pending to ensure the pending state is set on + * the appropriate CPU interfaces in the kernel */ + + /* s->route[irq] -> GICD_IROUTERn */ + for (i = GIC_INTERNAL; i < s->num_irq; i++) { + uint32_t offset = GICD_IROUTER + (sizeof(reg) * i); + + reg = s->irq_route[i - GIC_INTERNAL]; + kvm_gicd_access(s, offset, 0, ®, true); + } + + /* irq_state[n].trigger -> GICD_ICFGRn + * (restore configuration registers before pending IRQs so we treat + * level/edge correctly) */ + kvm_dist_put(s, GICD_ICFGR, 2, translate_trigger); + + /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */ + kvm_dist_put(s, GICD_ICPENDR, 1, translate_clear); + kvm_dist_put(s, GICD_ISPENDR, 1, translate_pending); + + /* irq_state[n].active -> GICD_ISACTIVERn */ + kvm_dist_put(s, GICD_ICACTIVER, 1, translate_clear); + kvm_dist_put(s, GICD_ISACTIVER, 1, translate_active); + + /* s->priorityX[irq] -> ICD_IPRIORITYRn */ + kvm_dist_put(s, GICD_IPRIORITYR, 8, translate_priority); + + /***************************************************************** + * CPU Interface(s) State + */ + + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { + GICv3CPUState *c = &s->cpu[ncpu]; + + reg = c->ctlr[1] & (ICC_CTLR_CBPR | ICC_CTLR_EOIMODE | ICC_CTLR_PMHE); + kvm_gicc_access(s, ICC_CTLR_EL1, ncpu, ®, true); + + reg = gicv3_get_igrpen0(s, ncpu); + kvm_gicc_access(s, ICC_IGRPEN0_EL1, ncpu, ®, true); + + reg = gicv3_get_igrpen1(s, ncpu); + kvm_gicc_access(s, ICC_IGRPEN1_EL1, ncpu, ®, true); + + reg = c->priority_mask; + kvm_gicc_access(s, ICC_PMR_EL1, ncpu, ®, true); + + reg = c->bpr[0]; + kvm_gicc_access(s, ICC_BPR0_EL1, ncpu, ®, true); + + reg = c->bpr[1]; + kvm_gicc_access(s, ICC_BPR1_EL1, ncpu, ®, true); + + for (i = 0; i < 4; i++) { + reg = c->apr[i][0]; + kvm_gicc_access(s, ICC_APR0_EL1(i), ncpu, ®, true); + } + + for (i = 0; i < 4; i++) { + reg = c->apr[i][1]; + kvm_gicc_access(s, ICC_APR1_EL1(i), ncpu, ®, true); + } + } } static void kvm_arm_gicv3_get(GICv3State *s) { - /* TODO */ - DPRINTF("Cannot get kernel gic state, no kernel interface\n"); + uint64_t reg, redist_typer; + int ncpu, i; + + kvm_arm_gicv3_check(s); + + kvm_gicr_access(s, GICR_TYPER, 0, &redist_typer, false); + + /***************************************************************** + * (Re)distributor State + */ + + /* GICD_CTLR -> s->ctlr */ + kvm_gicd_access(s, GICD_CTLR, 0, ®, false); + s->ctlr = reg; + + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { + GICv3CPUState *c = &s->cpu[ncpu]; + + kvm_gicr_access(s, GICR_CTLR, ncpu, ®, false); + c->redist_ctlr = reg & (GICR_CTLR_ENABLE_LPIS | GICR_CTLR_DPG0 | + GICR_CTLR_DPG1NS | GICR_CTLR_DPG1S); + + kvm_gicr_access(s, GICR_WAKER, ncpu, ®, false); + c->cpu_enabled = !(reg & GICR_WAKER_ProcessorSleep); + } + + if (redist_typer & GICR_TYPER_PLPIS) { + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { + GICv3CPUState *c = &s->cpu[ncpu]; + + kvm_gicr_access(s, GICR_PROPBASER, ncpu, ®, false); + c->propbaser = reg & (GICR_PROPBASER_OUTER_CACHEABILITY_MASK | + GICR_PROPBASER_ADDR_MASK | + GICR_PROPBASER_SHAREABILITY_MASK | + GICR_PROPBASER_CACHEABILITY_MASK | + GICR_PROPBASER_IDBITS_MASK); + + kvm_gicr_access(s, GICR_PENDBASER, ncpu, ®, false); + c->pendbaser = reg & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | + GICR_PENDBASER_ADDR_MASK | + GICR_PENDBASER_SHAREABILITY_MASK | + GICR_PENDBASER_CACHEABILITY_MASK); + } + } + + /* GICD_IIDR -> ? */ + /* kvm_gicd_access(s, GICD_IIDR, 0, ®, false); */ + + /* GICD_IGROUPRn -> irq_state[n].group */ + kvm_dist_get(s, GICD_IGROUPR, 1, translate_group); + + /* GICD_ISENABLERn -> irq_state[n].enabled */ + kvm_dist_get(s, GICD_ISENABLER, 1, translate_enabled); + + /* GICD_ISPENDRn -> irq_state[n].pending + irq_state[n].level */ + kvm_dist_get(s, GICD_ISPENDR, 1, translate_pending); + + /* GICD_ISACTIVERn -> irq_state[n].active */ + kvm_dist_get(s, GICD_ISACTIVER, 1, translate_active); + + /* GICD_ICFRn -> irq_state[n].trigger */ + kvm_dist_get(s, GICD_ICFGR, 2, translate_trigger); + + /* GICD_IPRIORITYRn -> s->priorityX[irq] */ + kvm_dist_get(s, GICD_IPRIORITYR, 8, translate_priority); + + /* GICD_IROUTERn -> s->route[irq] */ + for (i = GIC_INTERNAL; i < s->num_irq; i++) { + uint32_t offset = GICD_IROUTER + (sizeof(reg) * i); + + kvm_gicd_access(s, offset, 0, ®, false); + s->irq_route[i - GIC_INTERNAL] = reg; + } + + /***************************************************************** + * CPU Interface(s) State + */ + + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { + GICv3CPUState *c = &s->cpu[ncpu]; + + kvm_gicc_access(s, ICC_CTLR_EL1, ncpu, ®, false); + c->ctlr[1] = reg & (ICC_CTLR_CBPR | ICC_CTLR_EOIMODE | ICC_CTLR_PMHE); + + kvm_gicc_access(s, ICC_IGRPEN0_EL1, ncpu, ®, false); + gicv3_set_igrpen0(s, ncpu, reg); + + kvm_gicc_access(s, ICC_IGRPEN1_EL1, ncpu, ®, false); + gicv3_set_igrpen1(s, ncpu, reg); + + kvm_gicc_access(s, ICC_PMR_EL1, ncpu, ®, false); + c->priority_mask = reg & ICC_PMR_PRIORITY_MASK; + + kvm_gicc_access(s, ICC_BPR0_EL1, ncpu, ®, false); + c->bpr[0] = reg & ICC_BPR_BINARYPOINT_MASK; + + kvm_gicc_access(s, ICC_BPR1_EL1, ncpu, ®, false); + c->bpr[1] = reg & ICC_BPR_BINARYPOINT_MASK; + + for (i = 0; i < 4; i++) { + kvm_gicc_access(s, ICC_APR0_EL1(i), ncpu, ®, false); + c->apr[i][0] = reg; + } + + for (i = 0; i < 4; i++) { + kvm_gicc_access(s, ICC_APR1_EL1(i), ncpu, ®, false); + c->apr[i][1] = reg; + } + } } static void kvm_arm_gicv3_reset(DeviceState *dev) @@ -74,6 +509,12 @@ static void kvm_arm_gicv3_reset(DeviceState *dev) DPRINTF("Reset\n"); kgc->parent_reset(dev); + + if (s->migration_blocker) { + DPRINTF("Cannot put kernel gic state, no kernel interface\n"); + return; + } + kvm_arm_gicv3_put(s); } @@ -117,6 +558,13 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd); kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR, KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd); + + if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS, + GICD_CTLR)) { + error_setg(&s->migration_blocker, "This operating system kernel does " + "not support vGICv3 migration"); + migrate_add_blocker(s->migration_blocker); + } } static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data) -- 2.4.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions Pavel Fedin @ 2015-10-23 13:57 ` Peter Maydell 2015-10-26 7:59 ` Pavel Fedin 0 siblings, 1 reply; 14+ messages in thread From: Peter Maydell @ 2015-10-23 13:57 UTC (permalink / raw) To: Pavel Fedin Cc: Diana Craciun, Shlomo Pongratz, Shlomo Pongratz, QEMU Developers, Vijay Kilari On 22 October 2015 at 15:02, Pavel Fedin <p.fedin@samsung.com> wrote: > This actually implements pre_save and post_load methods for in-kernel > vGICv3. > > Signed-off-by: Pavel Fedin <p.fedin@samsung.com> > --- > hw/intc/arm_gicv3_kvm.c | 456 +++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 452 insertions(+), 4 deletions(-) > > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c > index b48f78f..ce8d2a0 100644 > --- a/hw/intc/arm_gicv3_kvm.c > +++ b/hw/intc/arm_gicv3_kvm.c > @@ -21,8 +21,11 @@ > > #include "hw/intc/arm_gicv3_common.h" > #include "hw/sysbus.h" > +#include "migration/migration.h" > +#include "qemu/error-report.h" > #include "sysemu/kvm.h" > #include "kvm_arm.h" > +#include "gicv3_internal.h" > #include "vgic_common.h" > > #ifdef DEBUG_GICV3_KVM > @@ -41,6 +44,23 @@ > #define KVM_ARM_GICV3_GET_CLASS(obj) \ > OBJECT_GET_CLASS(KVMARMGICv3Class, (obj), TYPE_KVM_ARM_GICV3) > > +#define ICC_PMR_EL1 \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b0100, 0b0110, 0b000) > +#define ICC_BPR0_EL1 \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1000, 0b011) > +#define ICC_APR0_EL1(n) \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1000, 0b100 | n) > +#define ICC_APR1_EL1(n) \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1001, 0b000 | n) > +#define ICC_BPR1_EL1 \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b011) > +#define ICC_CTLR_EL1 \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b100) > +#define ICC_IGRPEN0_EL1 \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b110) > +#define ICC_IGRPEN1_EL1 \ > + KVM_DEV_ARM_VGIC_SYSREG(0b11, 0b000, 0b1100, 0b1100, 0b111) Nice, but I'm not sure binary literals are supported by all the compilers we use (we don't use them anywhere else in qemu as far as I can see), and the other code we have in helper.c for system register numbers consistently uses decimal, so probably better to stick with that. > + > typedef struct KVMARMGICv3Class { > ARMGICv3CommonClass parent_class; > DeviceRealize parent_realize; > @@ -54,16 +74,431 @@ static void kvm_arm_gicv3_set_irq(void *opaque, int irq, int level) > kvm_arm_gic_set_irq(s->num_irq, irq, level); > } > > +#define VGIC_CPUID(cpuid) ((((cpuid) & ARM_AFF3_MASK) >> 8) | \ > + ((cpuid) & ARM32_AFFINITY_MASK)) > +#define KVM_VGIC_ATTR(reg, cpuid) \ > + ((VGIC_CPUID(cpuid) << KVM_DEV_ARM_VGIC_CPUID_SHIFT) | (reg)) > + > +static inline void kvm_gicd_access(GICv3State *s, int offset, int cpu, > + uint64_t *val, bool write) > +{ > + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS, > + KVM_VGIC_ATTR(offset, s->cpu[cpu].affinity_id), > + val, write); > +} > + > +static inline void kvm_gicr_access(GICv3State *s, int offset, int cpu, > + uint64_t *val, bool write) > +{ > + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS, > + KVM_VGIC_ATTR(offset, s->cpu[cpu].affinity_id), > + val, write); > +} > + > +static inline void kvm_gicc_access(GICv3State *s, uint64_t reg, int cpu, > + uint64_t *val, bool write) > +{ > + kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS, > + KVM_VGIC_ATTR(reg, s->cpu[cpu].affinity_id), > + val, write); > +} > + > +/* > + * Translate from the in-kernel field for an IRQ value to/from the qemu > + * representation. > + */ > +typedef void (*vgic_translate_fn)(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel); > + > +/* synthetic translate function used for clear/set registers to completely > + * clear a setting using a clear-register before setting the remaining bits > + * using a set-register */ > +static void translate_clear(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel) > +{ > + if (to_kernel) { > + *field = ~0; > + } else { > + /* does not make sense: qemu model doesn't use set/clear regs */ > + abort(); > + } > +} > + > +static void translate_enabled(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel) > +{ > + if (to_kernel) { > + *field = GIC_TEST_ENABLED(irq, cpu); > + } else { > + GIC_REPLACE_ENABLED(irq, cpu, *field); > + } > +} > + > +static void translate_group(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel) > +{ > + if (to_kernel) { > + *field = GIC_TEST_GROUP(irq, cpu); > + } else { > + GIC_REPLACE_GROUP(irq, cpu, *field); > + } > +} > + > +static void translate_trigger(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel) > +{ > + if (to_kernel) { > + *field = GIC_TEST_EDGE_TRIGGER(irq, cpu) ? 2 : 0; > + } else { > + GIC_REPLACE_EDGE_TRIGGER(irq, cpu, *field & 2); > + } > +} > + > +static void translate_pending(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel) > +{ > + if (to_kernel) { > + *field = gic_test_pending(s, irq, cpu); > + } else { > + GIC_REPLACE_PENDING(irq, cpu, *field); > + /* TODO: Capture if level-line is held high in the kernel */ > + } > +} > + > +static void translate_active(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel) > +{ > + if (to_kernel) { > + *field = GIC_TEST_ACTIVE(irq, cpu); > + } else { > + GIC_REPLACE_ACTIVE(irq, cpu, *field); > + } > +} > + > +static void translate_priority(GICv3State *s, int irq, int cpu, > + uint32_t *field, bool to_kernel) > +{ > + if (to_kernel) { > + *field = GIC_GET_PRIORITY(irq, cpu); > + } else { > + GIC_SET_PRIORITY(irq, cpu, *field); > + } > +} > + > +#define for_each_irq_reg(_irq, _max, _field_width) \ > + for (_irq = 0; _irq < _max; _irq += (32 / _field_width)) > + > +/* Read a register group from the kernel VGIC */ > +static void kvm_dist_get(GICv3State *s, uint32_t offset, int width, > + vgic_translate_fn translate_fn) > +{ > + uint64_t reg; > + int j; > + int irq, cpu, maxcpu; > + uint32_t field; > + int regsz = 32 / width; /* irqs per kernel register */ > + > + for_each_irq_reg(irq, s->num_irq, width) { > + maxcpu = irq < GIC_INTERNAL ? s->num_cpu : 1; > + for (cpu = 0; cpu < maxcpu; cpu++) { > + /* In GICv3 SGI/PPIs are stored in redistributor > + * Offsets in SGI area are the same as in distributor > + */ > + if (irq < GIC_INTERNAL) { > + kvm_gicr_access(s, offset + GICR_SGI_OFFSET, cpu, ®, false); > + } else { > + kvm_gicd_access(s, offset, cpu, ®, false); > + } This looks very odd. Rather than saying "if this is a GIC internal interrupt then the state lives in the redistributor, otherwise it's in the distributor", we should just first transfer all the distributor state (once), and then transfer the redistributor state for each CPU. (This also relates to my comment on patch 1 about not wanting a generic "GIC_GET/SET_FOO" set of macros which don't care about where the state is.) > + for (j = 0; j < regsz; j++) { > + field = extract32(reg, j * width, width); > + translate_fn(s, irq + j, cpu, &field, false); > + } > + } > + offset += 4; > + } > +} > + > +/* Write a register group to the kernel VGIC */ > +static void kvm_dist_put(GICv3State *s, uint32_t offset, int width, > + vgic_translate_fn translate_fn) > +{ > + uint64_t reg; > + int j; > + int irq, cpu, maxcpu; > + uint32_t field; > + int regsz = 32 / width; /* irqs per kernel register */ > + > + for_each_irq_reg(irq, s->num_irq, width) { > + maxcpu = irq < GIC_INTERNAL ? s->num_cpu : 1; > + for (cpu = 0; cpu < maxcpu; cpu++) { > + reg = 0; > + for (j = 0; j < regsz; j++) { > + translate_fn(s, irq + j, cpu, &field, true); > + reg = deposit32(reg, j * width, width, field); > + } > + /* In GICv3 SGI/PPIs are stored in redistributor > + * Offsets in SGI area are the same as in distributor > + */ > + if (irq < GIC_INTERNAL) { > + kvm_gicr_access(s, offset + GICR_SGI_OFFSET, cpu, ®, true); > + } else { > + kvm_gicd_access(s, offset, cpu, ®, true); > + } > + } > + offset += 4; > + } > +} > + > +static void kvm_arm_gicv3_check(GICv3State *s) > +{ > + uint64_t reg; > + uint32_t num_irq; > + > + /* Sanity checking s->num_irq */ > + kvm_gicd_access(s, GICD_TYPER, 0, ®, false); > + num_irq = ((reg & 0x1f) + 1) * 32; > + > + if (num_irq < s->num_irq) { > + error_report("Model requests %u IRQs, but kernel supports max %u\n", > + s->num_irq, num_irq); > + abort(); > + } > + > + /* TODO: Consider checking compatibility with the IIDR ? */ > +} > + > static void kvm_arm_gicv3_put(GICv3State *s) > { > - /* TODO */ > - DPRINTF("Cannot put kernel gic state, no kernel interface\n"); > + uint64_t reg, redist_typer; > + int ncpu, i; > + > + kvm_arm_gicv3_check(s); > + > + kvm_gicr_access(s, GICR_TYPER, 0, &redist_typer, false); > + > + /***************************************************************** > + * (Re)distributor State > + */ > + > + reg = s->ctlr; > + kvm_gicd_access(s, GICD_CTLR, 0, ®, true); > + > + if (redist_typer & GICR_TYPER_PLPIS) { > + /* Set base addresses before LPIs are enabled by GICR_CTLR write */ > + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { > + GICv3CPUState *c = &s->cpu[ncpu]; > + > + reg = c->propbaser & (GICR_PROPBASER_OUTER_CACHEABILITY_MASK | > + GICR_PROPBASER_ADDR_MASK | > + GICR_PROPBASER_SHAREABILITY_MASK | > + GICR_PROPBASER_CACHEABILITY_MASK | > + GICR_PROPBASER_IDBITS_MASK); > + kvm_gicr_access(s, GICR_PROPBASER, ncpu, ®, true); > + > + reg = c->pendbaser & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | > + GICR_PENDBASER_ADDR_MASK | > + GICR_PENDBASER_SHAREABILITY_MASK | > + GICR_PENDBASER_CACHEABILITY_MASK); > + if (!c->redist_ctlr & GICR_CTLR_ENABLE_LPIS) { > + reg |= GICR_PENDBASER_PTZ; > + } Why does the state of the pendbaser register depend on state in the redist_ctlr ? Worth a comment, whatever the answer is. > + kvm_gicr_access(s, GICR_PENDBASER, ncpu, ®, true); > + } > + } > + > + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { > + GICv3CPUState *c = &s->cpu[ncpu]; > + > + reg = c->redist_ctlr & (GICR_CTLR_ENABLE_LPIS | GICR_CTLR_DPG0 | > + GICR_CTLR_DPG1NS | GICR_CTLR_DPG1S); > + kvm_gicr_access(s, GICR_CTLR, ncpu, ®, true); > + > + reg = c->cpu_enabled ? 0 : GICR_WAKER_ProcessorSleep; > + kvm_gicr_access(s, GICR_WAKER, ncpu, ®, true); > + } > + > + /* irq_state[n].enabled -> GICD_ISENABLERn */ > + kvm_dist_put(s, GICD_ICENABLER, 1, translate_clear); > + kvm_dist_put(s, GICD_ISENABLER, 1, translate_enabled); > + > + /* irq_state[n].group -> GICD_IGROUPRn */ > + kvm_dist_put(s, GICD_IGROUPR, 1, translate_group); > + > + /* Restore targets before pending to ensure the pending state is set on > + * the appropriate CPU interfaces in the kernel */ > + > + /* s->route[irq] -> GICD_IROUTERn */ > + for (i = GIC_INTERNAL; i < s->num_irq; i++) { > + uint32_t offset = GICD_IROUTER + (sizeof(reg) * i); > + > + reg = s->irq_route[i - GIC_INTERNAL]; > + kvm_gicd_access(s, offset, 0, ®, true); > + } > + > + /* irq_state[n].trigger -> GICD_ICFGRn > + * (restore configuration registers before pending IRQs so we treat > + * level/edge correctly) */ > + kvm_dist_put(s, GICD_ICFGR, 2, translate_trigger); > + > + /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */ > + kvm_dist_put(s, GICD_ICPENDR, 1, translate_clear); > + kvm_dist_put(s, GICD_ISPENDR, 1, translate_pending); > + > + /* irq_state[n].active -> GICD_ISACTIVERn */ > + kvm_dist_put(s, GICD_ICACTIVER, 1, translate_clear); > + kvm_dist_put(s, GICD_ISACTIVER, 1, translate_active); > + > + /* s->priorityX[irq] -> ICD_IPRIORITYRn */ > + kvm_dist_put(s, GICD_IPRIORITYR, 8, translate_priority); > + > + /***************************************************************** > + * CPU Interface(s) State > + */ > + > + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { > + GICv3CPUState *c = &s->cpu[ncpu]; > + > + reg = c->ctlr[1] & (ICC_CTLR_CBPR | ICC_CTLR_EOIMODE | ICC_CTLR_PMHE); > + kvm_gicc_access(s, ICC_CTLR_EL1, ncpu, ®, true); > + > + reg = gicv3_get_igrpen0(s, ncpu); > + kvm_gicc_access(s, ICC_IGRPEN0_EL1, ncpu, ®, true); > + > + reg = gicv3_get_igrpen1(s, ncpu); > + kvm_gicc_access(s, ICC_IGRPEN1_EL1, ncpu, ®, true); > + > + reg = c->priority_mask; > + kvm_gicc_access(s, ICC_PMR_EL1, ncpu, ®, true); > + > + reg = c->bpr[0]; > + kvm_gicc_access(s, ICC_BPR0_EL1, ncpu, ®, true); > + > + reg = c->bpr[1]; > + kvm_gicc_access(s, ICC_BPR1_EL1, ncpu, ®, true); > + > + for (i = 0; i < 4; i++) { > + reg = c->apr[i][0]; > + kvm_gicc_access(s, ICC_APR0_EL1(i), ncpu, ®, true); > + } > + > + for (i = 0; i < 4; i++) { > + reg = c->apr[i][1]; > + kvm_gicc_access(s, ICC_APR1_EL1(i), ncpu, ®, true); > + } > + } > } > > static void kvm_arm_gicv3_get(GICv3State *s) > { > - /* TODO */ > - DPRINTF("Cannot get kernel gic state, no kernel interface\n"); > + uint64_t reg, redist_typer; > + int ncpu, i; > + > + kvm_arm_gicv3_check(s); > + > + kvm_gicr_access(s, GICR_TYPER, 0, &redist_typer, false); > + > + /***************************************************************** > + * (Re)distributor State > + */ > + > + /* GICD_CTLR -> s->ctlr */ > + kvm_gicd_access(s, GICD_CTLR, 0, ®, false); > + s->ctlr = reg; > + > + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { > + GICv3CPUState *c = &s->cpu[ncpu]; > + > + kvm_gicr_access(s, GICR_CTLR, ncpu, ®, false); > + c->redist_ctlr = reg & (GICR_CTLR_ENABLE_LPIS | GICR_CTLR_DPG0 | > + GICR_CTLR_DPG1NS | GICR_CTLR_DPG1S); > + > + kvm_gicr_access(s, GICR_WAKER, ncpu, ®, false); > + c->cpu_enabled = !(reg & GICR_WAKER_ProcessorSleep); If you take my suggestion in patch 1 of just having GICR_WAKER in the state struct, this code becomes simpler. > + } > + > + if (redist_typer & GICR_TYPER_PLPIS) { > + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { > + GICv3CPUState *c = &s->cpu[ncpu]; > + > + kvm_gicr_access(s, GICR_PROPBASER, ncpu, ®, false); > + c->propbaser = reg & (GICR_PROPBASER_OUTER_CACHEABILITY_MASK | > + GICR_PROPBASER_ADDR_MASK | > + GICR_PROPBASER_SHAREABILITY_MASK | > + GICR_PROPBASER_CACHEABILITY_MASK | > + GICR_PROPBASER_IDBITS_MASK); > + > + kvm_gicr_access(s, GICR_PENDBASER, ncpu, ®, false); > + c->pendbaser = reg & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | > + GICR_PENDBASER_ADDR_MASK | > + GICR_PENDBASER_SHAREABILITY_MASK | > + GICR_PENDBASER_CACHEABILITY_MASK); Why do we need to mask these values? > + } > + } > + > + /* GICD_IIDR -> ? */ > + /* kvm_gicd_access(s, GICD_IIDR, 0, ®, false); */ > + > + /* GICD_IGROUPRn -> irq_state[n].group */ > + kvm_dist_get(s, GICD_IGROUPR, 1, translate_group); These comments about 'irq_state[n]' don't match where the state actually is in the struct definitions from patch 1. > + > + /* GICD_ISENABLERn -> irq_state[n].enabled */ > + kvm_dist_get(s, GICD_ISENABLER, 1, translate_enabled); > + > + /* GICD_ISPENDRn -> irq_state[n].pending + irq_state[n].level */ > + kvm_dist_get(s, GICD_ISPENDR, 1, translate_pending); > + > + /* GICD_ISACTIVERn -> irq_state[n].active */ > + kvm_dist_get(s, GICD_ISACTIVER, 1, translate_active); > + > + /* GICD_ICFRn -> irq_state[n].trigger */ > + kvm_dist_get(s, GICD_ICFGR, 2, translate_trigger); > + > + /* GICD_IPRIORITYRn -> s->priorityX[irq] */ > + kvm_dist_get(s, GICD_IPRIORITYR, 8, translate_priority); > + > + /* GICD_IROUTERn -> s->route[irq] */ > + for (i = GIC_INTERNAL; i < s->num_irq; i++) { > + uint32_t offset = GICD_IROUTER + (sizeof(reg) * i); > + > + kvm_gicd_access(s, offset, 0, ®, false); > + s->irq_route[i - GIC_INTERNAL] = reg; > + } Missing code to transfer GICD_IGRPMODR<n> ? > + > + /***************************************************************** > + * CPU Interface(s) State > + */ > + > + for (ncpu = 0; ncpu < s->num_cpu; ncpu++) { > + GICv3CPUState *c = &s->cpu[ncpu]; > + > + kvm_gicc_access(s, ICC_CTLR_EL1, ncpu, ®, false); > + c->ctlr[1] = reg & (ICC_CTLR_CBPR | ICC_CTLR_EOIMODE | ICC_CTLR_PMHE); > + > + kvm_gicc_access(s, ICC_IGRPEN0_EL1, ncpu, ®, false); > + gicv3_set_igrpen0(s, ncpu, reg); > + > + kvm_gicc_access(s, ICC_IGRPEN1_EL1, ncpu, ®, false); > + gicv3_set_igrpen1(s, ncpu, reg); > + > + kvm_gicc_access(s, ICC_PMR_EL1, ncpu, ®, false); > + c->priority_mask = reg & ICC_PMR_PRIORITY_MASK; > + > + kvm_gicc_access(s, ICC_BPR0_EL1, ncpu, ®, false); > + c->bpr[0] = reg & ICC_BPR_BINARYPOINT_MASK; > + > + kvm_gicc_access(s, ICC_BPR1_EL1, ncpu, ®, false); > + c->bpr[1] = reg & ICC_BPR_BINARYPOINT_MASK; Do we need to mask these out? We could just trust that the state the kernel has is valid... > + > + for (i = 0; i < 4; i++) { > + kvm_gicc_access(s, ICC_APR0_EL1(i), ncpu, ®, false); > + c->apr[i][0] = reg; > + } > + > + for (i = 0; i < 4; i++) { > + kvm_gicc_access(s, ICC_APR1_EL1(i), ncpu, ®, false); > + c->apr[i][1] = reg; > + } Do we not transfer ICC_SRE_EL1 because it's implemented as RO? (I think that's right for no-irq/fiq-bypass, sysregs only.) > + } > } > > static void kvm_arm_gicv3_reset(DeviceState *dev) > @@ -74,6 +509,12 @@ static void kvm_arm_gicv3_reset(DeviceState *dev) > DPRINTF("Reset\n"); > > kgc->parent_reset(dev); > + > + if (s->migration_blocker) { > + DPRINTF("Cannot put kernel gic state, no kernel interface\n"); > + return; > + } > + > kvm_arm_gicv3_put(s); > } > > @@ -117,6 +558,13 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) > KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd); > kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR, > KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd); > + > + if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_DIST_REGS, > + GICD_CTLR)) { > + error_setg(&s->migration_blocker, "This operating system kernel does " > + "not support vGICv3 migration"); > + migrate_add_blocker(s->migration_blocker); > + } > } > > static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data) > -- > 2.4.4 thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions 2015-10-23 13:57 ` Peter Maydell @ 2015-10-26 7:59 ` Pavel Fedin 2015-10-26 11:09 ` Peter Maydell 0 siblings, 1 reply; 14+ messages in thread From: Pavel Fedin @ 2015-10-26 7:59 UTC (permalink / raw) To: 'Peter Maydell' Cc: 'Diana Craciun', 'Shlomo Pongratz', 'Shlomo Pongratz', 'QEMU Developers', 'Vijay Kilari' Hello! > > + reg = c->pendbaser & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | > > + GICR_PENDBASER_ADDR_MASK | > > + GICR_PENDBASER_SHAREABILITY_MASK | > > + GICR_PENDBASER_CACHEABILITY_MASK); > > + if (!c->redist_ctlr & GICR_CTLR_ENABLE_LPIS) { > > + reg |= GICR_PENDBASER_PTZ; > > + } > > Why does the state of the pendbaser register depend on state in the > redist_ctlr ? PTZ bit is write-only, we cannot read it back. And spec says that setting PTZ is adviced while LPIs are not enabled, because it shortens down the time of GIC initialization. So, i had to implement this small heuristics here. Is this approach OK? > Worth a comment, whatever the answer is. I will. > > + kvm_gicr_access(s, GICR_PENDBASER, ncpu, ®, false); > > + c->pendbaser = reg & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | > > + GICR_PENDBASER_ADDR_MASK | > > + GICR_PENDBASER_SHAREABILITY_MASK | > > + GICR_PENDBASER_CACHEABILITY_MASK); > > Why do we need to mask these values? I decided to do this at least for the case of KVM->TCG migration (as far as i understand, such things are possible). In this case i think we should not pollute our state with read-only bits, which get added by the emulation code itself. > Do we not transfer ICC_SRE_EL1 because it's implemented as RO? > (I think that's right for no-irq/fiq-bypass, sysregs only.) Yes, also because looks like KVM is not going to implement GICv3 with non-SRE mode, instead, if we want to run a legacy guest, we just configure our host to provide GICv2 for it. I actually migrate only those CPU interface registers, which are saved by the kernel code as part of guest's context. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions 2015-10-26 7:59 ` Pavel Fedin @ 2015-10-26 11:09 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2015-10-26 11:09 UTC (permalink / raw) To: Pavel Fedin Cc: Diana Craciun, Shlomo Pongratz, Shlomo Pongratz, QEMU Developers, Vijay Kilari On 26 October 2015 at 07:59, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> > + reg = c->pendbaser & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | >> > + GICR_PENDBASER_ADDR_MASK | >> > + GICR_PENDBASER_SHAREABILITY_MASK | >> > + GICR_PENDBASER_CACHEABILITY_MASK); >> > + if (!c->redist_ctlr & GICR_CTLR_ENABLE_LPIS) { >> > + reg |= GICR_PENDBASER_PTZ; >> > + } >> >> Why does the state of the pendbaser register depend on state in the >> redist_ctlr ? > > PTZ bit is write-only, we cannot read it back. And spec says that setting PTZ is adviced while LPIs are not enabled, because it shortens down the time of GIC initialization. So, i had to implement this small heuristics here. Is this approach OK? OK, with a comment to say that's what we're doing. (I assume that when we support LPIs we'll then set PTZ appropriately, so this code will change later.) >> Worth a comment, whatever the answer is. > > I will. > >> > + kvm_gicr_access(s, GICR_PENDBASER, ncpu, ®, false); >> > + c->pendbaser = reg & (GICR_PENDBASER_OUTER_CACHEABILITY_MASK | >> > + GICR_PENDBASER_ADDR_MASK | >> > + GICR_PENDBASER_SHAREABILITY_MASK | >> > + GICR_PENDBASER_CACHEABILITY_MASK); >> >> Why do we need to mask these values? > > I decided to do this at least for the case of KVM->TCG migration (as far as i understand, such things are possible). In this case i think we should not pollute our state with read-only bits, which get added by the emulation code itself. We don't do this for other system registers which might contain RO bits, so I think for consistency we shouldn't mask bits out here either. (Transferring RO bits in migration state gives the destination an opportunity in theory to reject a migration which is for a config it can't handle. And reserved bits may end up having a use in a future GIC version, so it's nice not to have to do a QEMU update just to remove them from the mask.) thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC PATCH v3 4/4] hw/intc/arm_gicv3_common: Add vmstate descriptors 2015-10-22 14:02 [Qemu-devel] [RFC PATCH v3 0/4] GICv3 live migration support Pavel Fedin ` (2 preceding siblings ...) 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions Pavel Fedin @ 2015-10-22 14:02 ` Pavel Fedin 2015-10-23 13:57 ` Peter Maydell 3 siblings, 1 reply; 14+ messages in thread From: Pavel Fedin @ 2015-10-22 14:02 UTC (permalink / raw) To: qemu-devel Cc: Diana Craciun, Peter Maydell, Shlomo Pongratz, Vijay Kilari, Shlomo Pongratz Add state structure descriptors and actually enable live migration. In order to describe fixed-size bitmaps, VMSTATE_BITMAP_STATIC() macro is introduced. Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- hw/intc/arm_gicv3_common.c | 58 ++++++++++++++++++++++++++++++++++++++++++++- include/migration/vmstate.h | 9 +++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index 2082d05..12d9de1 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -45,11 +45,67 @@ static int gicv3_post_load(void *opaque, int version_id) return 0; } +static const VMStateDescription vmstate_gicv3_sgi = { + .name = "arm_gicv3_sgi", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_BITMAP(pending, GICv3SGISource, 0, size), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_gicv3_cpu = { + .name = "arm_gicv3_cpu", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_BOOL(cpu_enabled, GICv3CPUState), + VMSTATE_UINT32(redist_ctlr, GICv3CPUState), + VMSTATE_UINT32(group, GICv3CPUState), + VMSTATE_UINT32(enabled, GICv3CPUState), + VMSTATE_UINT32(pending, GICv3CPUState), + VMSTATE_UINT32(active, GICv3CPUState), + VMSTATE_UINT32(edge_trigger, GICv3CPUState), + VMSTATE_UINT8_ARRAY(priority, GICv3CPUState, GIC_INTERNAL), + VMSTATE_UINT64(propbaser, GICv3CPUState), + VMSTATE_UINT64(pendbaser, GICv3CPUState), + VMSTATE_UINT32_ARRAY(ctlr, GICv3CPUState, 2), + VMSTATE_UINT32(priority_mask, GICv3CPUState), + VMSTATE_UINT32_ARRAY(bpr, GICv3CPUState, 2), + VMSTATE_UINT32_2DARRAY(apr, GICv3CPUState, 4, 2), + VMSTATE_UINT32(legacy_ctlr, GICv3CPUState), + VMSTATE_STRUCT_ARRAY(sgi, GICv3CPUState, GIC_NR_SGIS, 1, + vmstate_gicv3_sgi, GICv3SGISource), + VMSTATE_UINT16(current_pending, GICv3CPUState), + VMSTATE_UINT16(running_irq, GICv3CPUState), + VMSTATE_UINT16(running_priority, GICv3CPUState), + VMSTATE_UINT16_ARRAY(last_active, GICv3CPUState, GICV3_MAXIRQ), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_gicv3 = { .name = "arm_gicv3", - .unmigratable = 1, + .version_id = 1, + .minimum_version_id = 1, .pre_save = gicv3_pre_save, .post_load = gicv3_post_load, + .fields = (VMStateField[]) { + VMSTATE_UINT32(ctlr, GICv3State), + VMSTATE_BITMAP_STATIC(group, GICv3State , 0, GICV3_MAXSPI), + VMSTATE_BITMAP_STATIC(enabled, GICv3State , 0, GICV3_MAXSPI), + VMSTATE_BITMAP_STATIC(pending, GICv3State , 0, GICV3_MAXSPI), + VMSTATE_BITMAP_STATIC(active, GICv3State , 0, GICV3_MAXSPI), + VMSTATE_BITMAP_STATIC(level, GICv3State , 0, GICV3_MAXSPI), + VMSTATE_BITMAP_STATIC(edge_trigger, GICv3State , 0, GICV3_MAXSPI), + VMSTATE_UINT8_ARRAY(priority, GICv3State, GICV3_MAXSPI), + VMSTATE_UINT8_ARRAY(irq_target, GICv3State, GICV3_MAXSPI), + VMSTATE_UINT64_ARRAY(irq_route, GICv3State, GICV3_MAXSPI), + VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, GICv3State, num_cpu, + vmstate_gicv3_cpu, GICv3CPUState), + VMSTATE_END_OF_LIST() + } }; void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 9a65522..7d060e9 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -537,6 +537,15 @@ extern const VMStateInfo vmstate_info_bitmap; .offset = offsetof(_state, _field), \ } +#define VMSTATE_BITMAP_STATIC(_field, _state, _version, _size) { \ + .name = (stringify(_field)), \ + .version_id = (_version), \ + .size = (_size), \ + .info = &vmstate_info_bitmap, \ + .flags = VMS_BUFFER, \ + .offset = offsetof(_state, _field), \ +} + /* _f : field name _f_n : num of elements field_name _n : num of elements -- 2.4.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v3 4/4] hw/intc/arm_gicv3_common: Add vmstate descriptors 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 4/4] hw/intc/arm_gicv3_common: Add vmstate descriptors Pavel Fedin @ 2015-10-23 13:57 ` Peter Maydell 0 siblings, 0 replies; 14+ messages in thread From: Peter Maydell @ 2015-10-23 13:57 UTC (permalink / raw) To: Pavel Fedin Cc: Diana Craciun, Shlomo Pongratz, Shlomo Pongratz, QEMU Developers, Vijay Kilari On 22 October 2015 at 15:02, Pavel Fedin <p.fedin@samsung.com> wrote: > Add state structure descriptors and actually enable live migration. > > In order to describe fixed-size bitmaps, VMSTATE_BITMAP_STATIC() macro is > introduced. > > Signed-off-by: Pavel Fedin <p.fedin@samsung.com> > --- > hw/intc/arm_gicv3_common.c | 58 ++++++++++++++++++++++++++++++++++++++++++++- > include/migration/vmstate.h | 9 +++++++ > 2 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index 2082d05..12d9de1 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -45,11 +45,67 @@ static int gicv3_post_load(void *opaque, int version_id) > return 0; > } > > +static const VMStateDescription vmstate_gicv3_sgi = { > + .name = "arm_gicv3_sgi", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_BITMAP(pending, GICv3SGISource, 0, size), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_gicv3_cpu = { > + .name = "arm_gicv3_cpu", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_BOOL(cpu_enabled, GICv3CPUState), > + VMSTATE_UINT32(redist_ctlr, GICv3CPUState), > + VMSTATE_UINT32(group, GICv3CPUState), > + VMSTATE_UINT32(enabled, GICv3CPUState), > + VMSTATE_UINT32(pending, GICv3CPUState), > + VMSTATE_UINT32(active, GICv3CPUState), > + VMSTATE_UINT32(edge_trigger, GICv3CPUState), > + VMSTATE_UINT8_ARRAY(priority, GICv3CPUState, GIC_INTERNAL), > + VMSTATE_UINT64(propbaser, GICv3CPUState), > + VMSTATE_UINT64(pendbaser, GICv3CPUState), > + VMSTATE_UINT32_ARRAY(ctlr, GICv3CPUState, 2), > + VMSTATE_UINT32(priority_mask, GICv3CPUState), > + VMSTATE_UINT32_ARRAY(bpr, GICv3CPUState, 2), > + VMSTATE_UINT32_2DARRAY(apr, GICv3CPUState, 4, 2), > + VMSTATE_UINT32(legacy_ctlr, GICv3CPUState), I think we should keep all the legacy-configs-only state in its own VMStateDescription and have that only be included in the migration state if the GIC has legacy enabled (by defining a suitable .needed function for it). That will let us keep the legacy-emulation state (which might have to change as we implement that emulation) away from the state that we care about for KVM VMs (which are non-legacy-only), reducing the chances we end up with an awkward migration compat break in future. (Maybe we could do the same with state which is only for "if the GIC supports the security extensions", and "if the GIC supports virtualization". Haven't thought too hard about this yet.) > + VMSTATE_STRUCT_ARRAY(sgi, GICv3CPUState, GIC_NR_SGIS, 1, > + vmstate_gicv3_sgi, GICv3SGISource), > + VMSTATE_UINT16(current_pending, GICv3CPUState), > + VMSTATE_UINT16(running_irq, GICv3CPUState), > + VMSTATE_UINT16(running_priority, GICv3CPUState), > + VMSTATE_UINT16_ARRAY(last_active, GICv3CPUState, GICV3_MAXIRQ), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_gicv3 = { > .name = "arm_gicv3", > - .unmigratable = 1, > + .version_id = 1, > + .minimum_version_id = 1, > .pre_save = gicv3_pre_save, > .post_load = gicv3_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(ctlr, GICv3State), > + VMSTATE_BITMAP_STATIC(group, GICv3State , 0, GICV3_MAXSPI), > + VMSTATE_BITMAP_STATIC(enabled, GICv3State , 0, GICV3_MAXSPI), > + VMSTATE_BITMAP_STATIC(pending, GICv3State , 0, GICV3_MAXSPI), > + VMSTATE_BITMAP_STATIC(active, GICv3State , 0, GICV3_MAXSPI), > + VMSTATE_BITMAP_STATIC(level, GICv3State , 0, GICV3_MAXSPI), > + VMSTATE_BITMAP_STATIC(edge_trigger, GICv3State , 0, GICV3_MAXSPI), > + VMSTATE_UINT8_ARRAY(priority, GICv3State, GICV3_MAXSPI), > + VMSTATE_UINT8_ARRAY(irq_target, GICv3State, GICV3_MAXSPI), > + VMSTATE_UINT64_ARRAY(irq_route, GICv3State, GICV3_MAXSPI), > + VMSTATE_STRUCT_VARRAY_POINTER_UINT32(cpu, GICv3State, num_cpu, > + vmstate_gicv3_cpu, GICv3CPUState), > + VMSTATE_END_OF_LIST() > + } > }; > > void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler, > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 9a65522..7d060e9 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -537,6 +537,15 @@ extern const VMStateInfo vmstate_info_bitmap; > .offset = offsetof(_state, _field), \ > } > > +#define VMSTATE_BITMAP_STATIC(_field, _state, _version, _size) { \ > + .name = (stringify(_field)), \ > + .version_id = (_version), \ > + .size = (_size), \ > + .info = &vmstate_info_bitmap, \ > + .flags = VMS_BUFFER, \ > + .offset = offsetof(_state, _field), \ > +} > + > /* _f : field name > _f_n : num of elements field_name > _n : num of elements > -- > 2.4.4 thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-10-26 11:09 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-22 14:02 [Qemu-devel] [RFC PATCH v3 0/4] GICv3 live migration support Pavel Fedin 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 1/4] hw/intc/arm_gicv3_common: Add state information Pavel Fedin 2015-10-23 13:57 ` Peter Maydell 2015-10-24 12:30 ` Shlomo Pongratz 2015-10-24 12:35 ` Peter Maydell 2015-10-26 7:43 ` Pavel Fedin 2015-10-26 10:59 ` Peter Maydell 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 2/4] kernel: Add definitions for GICv3 attributes Pavel Fedin 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 3/4] hw/intc/arm_gicv3_kvm: Implement get/put functions Pavel Fedin 2015-10-23 13:57 ` Peter Maydell 2015-10-26 7:59 ` Pavel Fedin 2015-10-26 11:09 ` Peter Maydell 2015-10-22 14:02 ` [Qemu-devel] [RFC PATCH v3 4/4] hw/intc/arm_gicv3_common: Add vmstate descriptors Pavel Fedin 2015-10-23 13:57 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).