* [PATCH v3 0/8] GICv3 LPI and ITS feature implementation
@ 2021-04-29 23:41 Shashi Mallela
2021-04-29 23:41 ` [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework Shashi Mallela
` (10 more replies)
0 siblings, 11 replies; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:41 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
This patchset implements qemu device model for enabling physical
LPI support and ITS functionality in GIC as per GICv3 specification.
Both flat table and 2 level tables are implemented.The ITS commands
for adding/deleting ITS table entries,trigerring LPI interrupts are
implemented.Translated LPI interrupt ids are processed by redistributor
to determine priority and set pending state appropriately before
forwarding the same to cpu interface.
The ITS feature support has been added to sbsa-ref platform as well as
virt platform,wherein the emulated functionality co-exists with kvm
kernel functionality.
Changes in v3:
- review comments addressed
Shashi Mallela (8):
hw/intc: GICv3 ITS initial framework
hw/intc: GICv3 ITS register definitions added
hw/intc: GICv3 ITS command queue framework
hw/intc: GICv3 ITS Command processing
hw/intc: GICv3 ITS Feature enablement
hw/intc: GICv3 redistributor ITS processing
hw/arm/sbsa-ref: add ITS support in SBSA GIC
hw/arm/virt: add ITS support in virt GIC
hw/arm/sbsa-ref.c | 26 +-
hw/arm/virt.c | 27 +-
hw/intc/arm_gicv3.c | 6 +
hw/intc/arm_gicv3_common.c | 13 +
hw/intc/arm_gicv3_cpuif.c | 20 +-
hw/intc/arm_gicv3_dist.c | 21 +-
hw/intc/arm_gicv3_its.c | 1247 ++++++++++++++++++++++++
hw/intc/arm_gicv3_its_common.c | 11 +-
hw/intc/arm_gicv3_its_kvm.c | 2 +-
hw/intc/arm_gicv3_redist.c | 163 +++-
hw/intc/gicv3_internal.h | 186 +++-
hw/intc/meson.build | 1 +
include/hw/arm/virt.h | 2 +
include/hw/intc/arm_gicv3_common.h | 6 +
include/hw/intc/arm_gicv3_its_common.h | 40 +-
target/arm/kvm_arm.h | 4 +-
16 files changed, 1738 insertions(+), 37 deletions(-)
create mode 100644 hw/intc/arm_gicv3_its.c
--
2.27.0
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
@ 2021-04-29 23:41 ` Shashi Mallela
2021-05-18 13:52 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 2/8] hw/intc: GICv3 ITS register definitions added Shashi Mallela
` (9 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:41 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Added register definitions relevant to ITS,implemented overall
ITS device framework with stubs for ITS control and translater
regions read/write,extended ITS common to handle mmio init between
existing kvm device and newer qemu device.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/intc/arm_gicv3_its.c | 239 +++++++++++++++++++++++++
hw/intc/arm_gicv3_its_common.c | 11 +-
hw/intc/arm_gicv3_its_kvm.c | 2 +-
hw/intc/gicv3_internal.h | 88 +++++++--
hw/intc/meson.build | 1 +
include/hw/intc/arm_gicv3_its_common.h | 10 +-
6 files changed, 331 insertions(+), 20 deletions(-)
create mode 100644 hw/intc/arm_gicv3_its.c
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
new file mode 100644
index 0000000000..7b11330e01
--- /dev/null
+++ b/hw/intc/arm_gicv3_its.c
@@ -0,0 +1,239 @@
+/*
+ * ITS emulation for a GICv3-based system
+ *
+ * Copyright Linaro.org 2021
+ *
+ * Authors:
+ * Shashi Mallela <shashi.mallela@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version. See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/qdev-properties.h"
+#include "hw/intc/arm_gicv3_its_common.h"
+#include "gicv3_internal.h"
+#include "qom/object.h"
+
+typedef struct GICv3ITSClass GICv3ITSClass;
+/* This is reusing the GICv3ITSState typedef from ARM_GICV3_ITS_COMMON */
+DECLARE_OBJ_CHECKERS(GICv3ITSState, GICv3ITSClass,
+ ARM_GICV3_ITS, TYPE_ARM_GICV3_ITS)
+
+struct GICv3ITSClass {
+ GICv3ITSCommonClass parent_class;
+ void (*parent_reset)(DeviceState *dev);
+};
+
+static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
+ uint64_t data, unsigned size, MemTxAttrs attrs)
+{
+ MemTxResult result = MEMTX_OK;
+
+ return result;
+}
+
+static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,
+ uint64_t value, MemTxAttrs attrs)
+{
+ MemTxResult result = MEMTX_OK;
+
+ return result;
+}
+
+static MemTxResult its_readl(GICv3ITSState *s, hwaddr offset,
+ uint64_t *data, MemTxAttrs attrs)
+{
+ MemTxResult result = MEMTX_OK;
+
+ return result;
+}
+
+static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,
+ uint64_t value, MemTxAttrs attrs)
+{
+ MemTxResult result = MEMTX_OK;
+
+ return result;
+}
+
+static MemTxResult its_readll(GICv3ITSState *s, hwaddr offset,
+ uint64_t *data, MemTxAttrs attrs)
+{
+ MemTxResult result = MEMTX_OK;
+
+ return result;
+}
+
+static MemTxResult gicv3_its_read(void *opaque, hwaddr offset, uint64_t *data,
+ unsigned size, MemTxAttrs attrs)
+{
+ GICv3ITSState *s = (GICv3ITSState *)opaque;
+ MemTxResult result;
+
+ switch (size) {
+ case 4:
+ result = its_readl(s, offset, data, attrs);
+ break;
+ case 8:
+ result = its_readll(s, offset, data, attrs);
+ break;
+ default:
+ result = MEMTX_ERROR;
+ break;
+ }
+
+ if (result == MEMTX_ERROR) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: invalid guest read at offset " TARGET_FMT_plx
+ "size %u\n", __func__, offset, size);
+ /*
+ * The spec requires that reserved registers are RAZ/WI;
+ * so use MEMTX_ERROR returns from leaf functions as a way to
+ * trigger the guest-error logging but don't return it to
+ * the caller, or we'll cause a spurious guest data abort.
+ */
+ result = MEMTX_OK;
+ *data = 0;
+ }
+ return result;
+}
+
+static MemTxResult gicv3_its_write(void *opaque, hwaddr offset, uint64_t data,
+ unsigned size, MemTxAttrs attrs)
+{
+ GICv3ITSState *s = (GICv3ITSState *)opaque;
+ MemTxResult result;
+
+ switch (size) {
+ case 4:
+ result = its_writel(s, offset, data, attrs);
+ break;
+ case 8:
+ result = its_writell(s, offset, data, attrs);
+ break;
+ default:
+ result = MEMTX_ERROR;
+ break;
+ }
+
+ if (result == MEMTX_ERROR) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: invalid guest write at offset " TARGET_FMT_plx
+ "size %u\n", __func__, offset, size);
+ /*
+ * The spec requires that reserved registers are RAZ/WI;
+ * so use MEMTX_ERROR returns from leaf functions as a way to
+ * trigger the guest-error logging but don't return it to
+ * the caller, or we'll cause a spurious guest data abort.
+ */
+ result = MEMTX_OK;
+ }
+ return result;
+}
+
+static const MemoryRegionOps gicv3_its_control_ops = {
+ .read_with_attrs = gicv3_its_read,
+ .write_with_attrs = gicv3_its_write,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 8,
+ .impl.min_access_size = 4,
+ .impl.max_access_size = 8,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const MemoryRegionOps gicv3_its_translation_ops = {
+ .write_with_attrs = gicv3_its_translation_write,
+ .valid.min_access_size = 2,
+ .valid.max_access_size = 4,
+ .impl.min_access_size = 2,
+ .impl.max_access_size = 4,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)
+{
+ GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
+
+ gicv3_its_init_mmio(s, &gicv3_its_control_ops, &gicv3_its_translation_ops);
+
+ if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
+ /* set the ITS default features supported */
+ s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL,
+ GITS_TYPE_PHYSICAL);
+ s->typer = FIELD_DP64(s->typer, GITS_TYPER, ITT_ENTRY_SIZE,
+ ITS_ITT_ENTRY_SIZE - 1);
+ s->typer = FIELD_DP64(s->typer, GITS_TYPER, IDBITS, ITS_IDBITS);
+ s->typer = FIELD_DP64(s->typer, GITS_TYPER, DEVBITS, ITS_DEVBITS);
+ s->typer = FIELD_DP64(s->typer, GITS_TYPER, CIL, 1);
+ s->typer = FIELD_DP64(s->typer, GITS_TYPER, CIDBITS, ITS_CIDBITS);
+ }
+}
+
+static void gicv3_its_reset(DeviceState *dev)
+{
+ GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
+ GICv3ITSClass *c = ARM_GICV3_ITS_GET_CLASS(s);
+
+ if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
+ c->parent_reset(dev);
+
+ /* Quiescent bit reset to 1 */
+ s->ctlr = FIELD_DP32(s->ctlr, GITS_CTLR, QUIESCENT, 1);
+
+ /*
+ * setting GITS_BASER0.Type = 0b001 (Device)
+ * GITS_BASER1.Type = 0b100 (Collection Table)
+ * GITS_BASER<n>.Type,where n = 3 to 7 are 0b00 (Unimplemented)
+ * GITS_BASER<0,1>.Page_Size = 64KB
+ * and default translation table entry size to 16 bytes
+ */
+ s->baser[0] = FIELD_DP64(s->baser[0], GITS_BASER, TYPE,
+ GITS_ITT_TYPE_DEVICE);
+ s->baser[0] = FIELD_DP64(s->baser[0], GITS_BASER, PAGESIZE,
+ GITS_BASER_PAGESIZE_64K);
+ s->baser[0] = FIELD_DP64(s->baser[0], GITS_BASER, ENTRYSIZE,
+ GITS_DTE_SIZE - 1);
+
+ s->baser[1] = FIELD_DP64(s->baser[1], GITS_BASER, TYPE,
+ GITS_ITT_TYPE_COLLECTION);
+ s->baser[1] = FIELD_DP64(s->baser[1], GITS_BASER, PAGESIZE,
+ GITS_BASER_PAGESIZE_64K);
+ s->baser[1] = FIELD_DP64(s->baser[1], GITS_BASER, ENTRYSIZE,
+ GITS_CTE_SIZE - 1);
+ }
+}
+
+static Property gicv3_its_props[] = {
+ DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "arm-gicv3",
+ GICv3State *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void gicv3_its_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ GICv3ITSClass *ic = ARM_GICV3_ITS_CLASS(klass);
+
+ dc->realize = gicv3_arm_its_realize;
+ device_class_set_props(dc, gicv3_its_props);
+ device_class_set_parent_reset(dc, gicv3_its_reset, &ic->parent_reset);
+}
+
+static const TypeInfo gicv3_its_info = {
+ .name = TYPE_ARM_GICV3_ITS,
+ .parent = TYPE_ARM_GICV3_ITS_COMMON,
+ .instance_size = sizeof(GICv3ITSState),
+ .class_init = gicv3_its_class_init,
+ .class_size = sizeof(GICv3ITSClass),
+};
+
+static void gicv3_its_register_types(void)
+{
+ type_register_static(&gicv3_its_info);
+}
+
+type_init(gicv3_its_register_types)
diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index 66c4c6a188..b4dddb16b8 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -50,12 +50,13 @@ static int gicv3_its_post_load(void *opaque, int version_id)
static const VMStateDescription vmstate_its = {
.name = "arm_gicv3_its",
+ .version_id = 1,
+ .minimum_version_id = 1,
.pre_save = gicv3_its_pre_save,
.post_load = gicv3_its_post_load,
.priority = MIG_PRI_GICV3_ITS,
.fields = (VMStateField[]) {
VMSTATE_UINT32(ctlr, GICv3ITSState),
- VMSTATE_UINT32(iidr, GICv3ITSState),
VMSTATE_UINT64(cbaser, GICv3ITSState),
VMSTATE_UINT64(cwriter, GICv3ITSState),
VMSTATE_UINT64(creadr, GICv3ITSState),
@@ -99,15 +100,16 @@ static const MemoryRegionOps gicv3_its_trans_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-void gicv3_its_init_mmio(GICv3ITSState *s, const MemoryRegionOps *ops)
+void gicv3_its_init_mmio(GICv3ITSState *s, const MemoryRegionOps *ops,
+ const MemoryRegionOps *tops)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(s);
memory_region_init_io(&s->iomem_its_cntrl, OBJECT(s), ops, s,
"control", ITS_CONTROL_SIZE);
memory_region_init_io(&s->iomem_its_translation, OBJECT(s),
- &gicv3_its_trans_ops, s,
- "translation", ITS_TRANS_SIZE);
+ tops ? tops : &gicv3_its_trans_ops, s,
+ "translation", ITS_TRANS_SIZE);
/* Our two regions are always adjacent, therefore we now combine them
* into a single one in order to make our users' life easier.
@@ -129,7 +131,6 @@ static void gicv3_its_common_reset(DeviceState *dev)
s->cbaser = 0;
s->cwriter = 0;
s->creadr = 0;
- s->iidr = 0;
memset(&s->baser, 0, sizeof(s->baser));
}
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index b554d2ede0..0b4cbed28b 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -106,7 +106,7 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
kvm_arm_register_device(&s->iomem_its_cntrl, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
KVM_VGIC_ITS_ADDR_TYPE, s->dev_fd, 0);
- gicv3_its_init_mmio(s, NULL);
+ gicv3_its_init_mmio(s, NULL, NULL);
if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
GITS_CTLR)) {
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 05303a55c8..e0b06930a7 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -24,6 +24,7 @@
#ifndef QEMU_ARM_GICV3_INTERNAL_H
#define QEMU_ARM_GICV3_INTERNAL_H
+#include "hw/registerfields.h"
#include "hw/intc/arm_gicv3_common.h"
/* Distributor registers, as offsets from the distributor base address */
@@ -67,6 +68,9 @@
#define GICD_CTLR_E1NWF (1U << 7)
#define GICD_CTLR_RWP (1U << 31)
+/* 16 bits EventId */
+#define GICD_TYPER_IDBITS 0xf
+
/*
* Redistributor frame offsets from RD_base
*/
@@ -122,18 +126,6 @@
#define GICR_WAKER_ProcessorSleep (1U << 1)
#define GICR_WAKER_ChildrenAsleep (1U << 2)
-#define GICR_PROPBASER_OUTER_CACHEABILITY_MASK (7ULL << 56)
-#define GICR_PROPBASER_ADDR_MASK (0xfffffffffULL << 12)
-#define GICR_PROPBASER_SHAREABILITY_MASK (3U << 10)
-#define GICR_PROPBASER_CACHEABILITY_MASK (7U << 7)
-#define GICR_PROPBASER_IDBITS_MASK (0x1f)
-
-#define GICR_PENDBASER_PTZ (1ULL << 62)
-#define GICR_PENDBASER_OUTER_CACHEABILITY_MASK (7ULL << 56)
-#define GICR_PENDBASER_ADDR_MASK (0xffffffffULL << 16)
-#define GICR_PENDBASER_SHAREABILITY_MASK (3U << 10)
-#define GICR_PENDBASER_CACHEABILITY_MASK (7U << 7)
-
#define ICC_CTLR_EL1_CBPR (1U << 0)
#define ICC_CTLR_EL1_EOIMODE (1U << 1)
#define ICC_CTLR_EL1_PMHE (1U << 6)
@@ -239,6 +231,78 @@
#define ICH_VTR_EL2_PREBITS_SHIFT 26
#define ICH_VTR_EL2_PRIBITS_SHIFT 29
+/* ITS Registers */
+
+FIELD(GITS_BASER, SIZE, 0, 8)
+FIELD(GITS_BASER, PAGESIZE, 8, 2)
+FIELD(GITS_BASER, SHAREABILITY, 10, 2)
+FIELD(GITS_BASER, PHYADDR, 12, 36)
+FIELD(GITS_BASER, PHYADDRL_64K, 16, 32)
+FIELD(GITS_BASER, PHYADDRH_64K, 48, 4)
+FIELD(GITS_BASER, ENTRYSIZE, 48, 5)
+FIELD(GITS_BASER, OUTERCACHE, 53, 3)
+FIELD(GITS_BASER, TYPE, 56, 3)
+FIELD(GITS_BASER, INNERCACHE, 59, 3)
+FIELD(GITS_BASER, INDIRECT, 62, 1)
+FIELD(GITS_BASER, VALID, 63, 1)
+
+FIELD(GITS_CTLR, QUIESCENT, 31, 1)
+
+FIELD(GITS_TYPER, PHYSICAL, 0, 1)
+FIELD(GITS_TYPER, ITT_ENTRY_SIZE, 4, 4)
+FIELD(GITS_TYPER, IDBITS, 8, 5)
+FIELD(GITS_TYPER, DEVBITS, 13, 5)
+FIELD(GITS_TYPER, SEIS, 18, 1)
+FIELD(GITS_TYPER, PTA, 19, 1)
+FIELD(GITS_TYPER, CIDBITS, 32, 4)
+FIELD(GITS_TYPER, CIL, 36, 1)
+
+#define GITS_BASER_PAGESIZE_4K 0
+#define GITS_BASER_PAGESIZE_16K 1
+#define GITS_BASER_PAGESIZE_64K 2
+
+#define GITS_ITT_TYPE_DEVICE 1ULL
+#define GITS_ITT_TYPE_COLLECTION 4ULL
+
+/**
+ * Default features advertised by this version of ITS
+ */
+/* Physical LPIs supported */
+#define GITS_TYPE_PHYSICAL (1U << 0)
+
+/*
+ * 12 bytes Interrupt translation Table Entry size
+ * ITE Lower 8 Bytes
+ * Valid = 1 bit,InterruptType = 1 bit,
+ * Size of LPI number space[considering max 24 bits],
+ * Size of LPI number space[considering max 24 bits],
+ * ITE Higher 4 Bytes
+ * ICID = 16 bits,
+ * vPEID = 16 bits
+ */
+#define ITS_ITT_ENTRY_SIZE 0xC
+
+/* 16 bits EventId */
+#define ITS_IDBITS GICD_TYPER_IDBITS
+
+/* 16 bits DeviceId */
+#define ITS_DEVBITS 0xF
+
+/* 16 bits CollectionId */
+#define ITS_CIDBITS 0xF
+
+/*
+ * 8 bytes Device Table Entry size
+ * Valid = 1 bit,ITTAddr = 44 bits,Size = 5 bits
+ */
+#define GITS_DTE_SIZE (0x8ULL)
+
+/*
+ * 8 bytes Collection Table Entry size
+ * Valid = 1 bit,RDBase = 36 bits(considering max RDBASE)
+ */
+#define GITS_CTE_SIZE (0x8ULL)
+
/* Special interrupt IDs */
#define INTID_SECURE 1020
#define INTID_NONSECURE 1021
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index 1c299039f6..53472239f0 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -8,6 +8,7 @@ softmmu_ss.add(when: 'CONFIG_ARM_GIC', if_true: files(
'arm_gicv3_dist.c',
'arm_gicv3_its_common.c',
'arm_gicv3_redist.c',
+ 'arm_gicv3_its.c',
))
softmmu_ss.add(when: 'CONFIG_ETRAXFS', if_true: files('etraxfs_pic.c'))
softmmu_ss.add(when: 'CONFIG_HEATHROW_PIC', if_true: files('heathrow_pic.c'))
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 5a0952b404..03fc3963f3 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -25,17 +25,22 @@
#include "hw/intc/arm_gicv3_common.h"
#include "qom/object.h"
+#define TYPE_ARM_GICV3_ITS "arm-gicv3-its"
+
#define ITS_CONTROL_SIZE 0x10000
#define ITS_TRANS_SIZE 0x10000
#define ITS_SIZE (ITS_CONTROL_SIZE + ITS_TRANS_SIZE)
#define GITS_CTLR 0x0
#define GITS_IIDR 0x4
+#define GITS_TYPER 0x8
#define GITS_CBASER 0x80
#define GITS_CWRITER 0x88
#define GITS_CREADR 0x90
#define GITS_BASER 0x100
+#define GITS_TRANSLATER 0x0040
+
struct GICv3ITSState {
SysBusDevice parent_obj;
@@ -51,7 +56,7 @@ struct GICv3ITSState {
/* Registers */
uint32_t ctlr;
- uint32_t iidr;
+ uint64_t typer;
uint64_t cbaser;
uint64_t cwriter;
uint64_t creadr;
@@ -62,7 +67,8 @@ struct GICv3ITSState {
typedef struct GICv3ITSState GICv3ITSState;
-void gicv3_its_init_mmio(GICv3ITSState *s, const MemoryRegionOps *ops);
+void gicv3_its_init_mmio(GICv3ITSState *s, const MemoryRegionOps *ops,
+ const MemoryRegionOps *tops);
#define TYPE_ARM_GICV3_ITS_COMMON "arm-gicv3-its-common"
typedef struct GICv3ITSCommonClass GICv3ITSCommonClass;
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 2/8] hw/intc: GICv3 ITS register definitions added
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
2021-04-29 23:41 ` [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework Shashi Mallela
@ 2021-04-29 23:41 ` Shashi Mallela
2021-05-18 14:27 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 3/8] hw/intc: GICv3 ITS command queue framework Shashi Mallela
` (8 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:41 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Defined descriptors for ITS device table,collection table and ITS
command queue entities.Implemented register read/write functions,
extract ITS table parameters and command queue parameters,extended
gicv3 common to capture qemu address space(which host the ITS table
platform memories required for subsequent ITS processing) and
initialize the same in ITS device.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/intc/arm_gicv3_its.c | 333 +++++++++++++++++++++++++
hw/intc/gicv3_internal.h | 23 ++
include/hw/intc/arm_gicv3_common.h | 3 +
include/hw/intc/arm_gicv3_its_common.h | 30 +++
4 files changed, 389 insertions(+)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 7b11330e01..a7ccb38a89 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -28,6 +28,162 @@ struct GICv3ITSClass {
void (*parent_reset)(DeviceState *dev);
};
+static bool extract_table_params(GICv3ITSState *s)
+{
+ bool result = true;
+ uint16_t num_pages = 0;
+ uint8_t page_sz_type;
+ uint8_t type;
+ uint32_t page_sz = 0;
+ uint64_t value;
+
+ for (int i = 0; i < 8; i++) {
+ value = s->baser[i];
+
+ if (!value) {
+ continue;
+ }
+
+ page_sz_type = FIELD_EX64(value, GITS_BASER, PAGESIZE);
+
+ switch (page_sz_type) {
+ case 0:
+ page_sz = GITS_ITT_PAGE_SIZE_0;
+ break;
+
+ case 1:
+ page_sz = GITS_ITT_PAGE_SIZE_1;
+ break;
+
+ case 2:
+ case 3:
+ page_sz = GITS_ITT_PAGE_SIZE_2;
+ break;
+
+ default:
+ result = false;
+ break;
+ }
+
+ if (result) {
+ num_pages = FIELD_EX64(value, GITS_BASER, SIZE);
+
+ type = FIELD_EX64(value, GITS_BASER, TYPE);
+
+ switch (type) {
+
+ case GITS_ITT_TYPE_DEVICE:
+ memset(&s->dt, 0 , sizeof(s->dt));
+ s->dt.valid = FIELD_EX64(value, GITS_BASER, VALID);
+
+ if (s->dt.valid) {
+ s->dt.page_sz = page_sz;
+ s->dt.indirect = FIELD_EX64(value, GITS_BASER, INDIRECT);
+ s->dt.entry_sz = FIELD_EX64(value, GITS_BASER, ENTRYSIZE);
+
+ if (!s->dt.indirect) {
+ s->dt.max_entries = ((num_pages + 1) * page_sz) /
+ s->dt.entry_sz;
+ } else {
+ s->dt.max_entries = ((((num_pages + 1) * page_sz) /
+ L1TABLE_ENTRY_SIZE) *
+ (page_sz / s->dt.entry_sz));
+ }
+
+ s->dt.max_devids = (1UL << (FIELD_EX64(s->typer, GITS_TYPER,
+ DEVBITS) + 1));
+
+ if ((page_sz == GITS_ITT_PAGE_SIZE_0) ||
+ (page_sz == GITS_ITT_PAGE_SIZE_1)) {
+ s->dt.base_addr = FIELD_EX64(value, GITS_BASER,
+ PHYADDR);
+ s->dt.base_addr <<= R_GITS_BASER_PHYADDR_SHIFT;
+ } else if (page_sz == GITS_ITT_PAGE_SIZE_2) {
+ s->dt.base_addr = FIELD_EX64(value, GITS_BASER,
+ PHYADDRL_64K) <<
+ R_GITS_BASER_PHYADDRL_64K_SHIFT;
+ s->dt.base_addr |= ((value >>
+ R_GITS_BASER_PHYADDR_SHIFT) &
+ R_GITS_BASER_PHYADDRH_64K_MASK) <<
+ R_GITS_BASER_PHYADDRH_64K_SHIFT;
+ }
+ }
+ break;
+
+ case GITS_ITT_TYPE_COLLECTION:
+ memset(&s->ct, 0 , sizeof(s->ct));
+ s->ct.valid = FIELD_EX64(value, GITS_BASER, VALID);
+
+ /*
+ * GITS_TYPER.HCC is 0 for this implementation
+ * hence writes are discarded if ct.valid is 0
+ */
+ if (s->ct.valid) {
+ s->ct.page_sz = page_sz;
+ s->ct.indirect = FIELD_EX64(value, GITS_BASER, INDIRECT);
+ s->ct.entry_sz = FIELD_EX64(value, GITS_BASER, ENTRYSIZE);
+
+ if (!s->ct.indirect) {
+ s->ct.max_entries = ((num_pages + 1) * page_sz) /
+ s->ct.entry_sz;
+ } else {
+ s->ct.max_entries = ((((num_pages + 1) * page_sz) /
+ L1TABLE_ENTRY_SIZE) *
+ (page_sz / s->ct.entry_sz));
+ }
+
+ if (FIELD_EX64(s->typer, GITS_TYPER, CIL)) {
+ s->ct.max_collids = (1UL << (FIELD_EX64(s->typer,
+ GITS_TYPER, CIDBITS) + 1));
+ } else {
+ /* 16-bit CollectionId supported when CIL == 0 */
+ s->ct.max_collids = (1UL << 16);
+ }
+
+ if ((page_sz == GITS_ITT_PAGE_SIZE_0) ||
+ (page_sz == GITS_ITT_PAGE_SIZE_1)) {
+ s->ct.base_addr = FIELD_EX64(value, GITS_BASER,
+ PHYADDR);
+ s->ct.base_addr <<= R_GITS_BASER_PHYADDR_SHIFT;
+ } else if (page_sz == GITS_ITT_PAGE_SIZE_2) {
+ s->ct.base_addr = FIELD_EX64(value, GITS_BASER,
+ PHYADDRL_64K) <<
+ R_GITS_BASER_PHYADDRL_64K_SHIFT;
+ s->ct.base_addr |= ((value >>
+ R_GITS_BASER_PHYADDR_SHIFT) &
+ R_GITS_BASER_PHYADDRH_64K_MASK) <<
+ R_GITS_BASER_PHYADDRH_64K_SHIFT;
+ }
+ }
+ break;
+
+ default:
+ break;
+ }
+ }
+ }
+ return result;
+}
+
+static void extract_cmdq_params(GICv3ITSState *s)
+{
+ uint16_t num_pages = 0;
+ uint64_t value = s->cbaser;
+
+ num_pages = FIELD_EX64(value, GITS_CBASER, SIZE);
+
+ memset(&s->cq, 0 , sizeof(s->cq));
+ s->cq.valid = FIELD_EX64(value, GITS_CBASER, VALID);
+
+ if (s->cq.valid) {
+ s->cq.max_entries = ((num_pages + 1) * GITS_ITT_PAGE_SIZE_0) /
+ GITS_CMDQ_ENTRY_SIZE;
+ s->cq.base_addr = FIELD_EX64(value, GITS_CBASER, PHYADDR);
+ s->cq.base_addr <<= R_GITS_CBASER_PHYADDR_SHIFT;
+ }
+ return;
+}
+
static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
uint64_t data, unsigned size, MemTxAttrs attrs)
{
@@ -40,7 +196,70 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,
uint64_t value, MemTxAttrs attrs)
{
MemTxResult result = MEMTX_OK;
+ int index;
+ uint64_t temp = 0;
+ switch (offset) {
+ case GITS_CTLR:
+ s->ctlr |= (value & ~(s->ctlr));
+
+ if (s->ctlr & ITS_CTLR_ENABLED) {
+ if (!extract_table_params(s)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: error extracting GITS_BASER parameters "
+ TARGET_FMT_plx "\n", __func__, offset);
+ } else {
+ extract_cmdq_params(s);
+ s->creadr = 0;
+ }
+ }
+ break;
+ case GITS_CBASER:
+ /*
+ * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
+ * already enabled
+ */
+ if (!(s->ctlr & ITS_CTLR_ENABLED)) {
+ s->cbaser = deposit64(s->cbaser, 0, 32, value);
+ s->creadr = 0;
+ }
+ break;
+ case GITS_CBASER + 4:
+ /*
+ * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
+ * already enabled
+ */
+ if (!(s->ctlr & ITS_CTLR_ENABLED)) {
+ s->cbaser = deposit64(s->cbaser, 32, 32, value);
+ }
+ break;
+ case GITS_CWRITER:
+ s->cwriter = deposit64(s->cwriter, 0, 32, value);
+ break;
+ case GITS_CWRITER + 4:
+ s->cwriter = deposit64(s->cwriter, 32, 32, value);
+ break;
+ case GITS_BASER ... GITS_BASER + 0x3f:
+ /*
+ * IMPDEF choice:- GITS_BASERn register becomes RO if ITS is
+ * already enabled
+ */
+ if (!(s->ctlr & ITS_CTLR_ENABLED)) {
+ index = (offset - GITS_BASER) / 8;
+
+ if (offset & 7) {
+ temp = s->baser[index];
+ temp = deposit64(temp, 32, 32, (value & ~GITS_BASER_VAL_MASK));
+ s->baser[index] |= temp;
+ } else {
+ s->baser[index] = deposit64(s->baser[index], 0, 32, value);
+ }
+ }
+ break;
+ default:
+ result = MEMTX_ERROR;
+ break;
+ }
return result;
}
@@ -48,7 +267,54 @@ static MemTxResult its_readl(GICv3ITSState *s, hwaddr offset,
uint64_t *data, MemTxAttrs attrs)
{
MemTxResult result = MEMTX_OK;
+ int index;
+ switch (offset) {
+ case GITS_CTLR:
+ *data = s->ctlr;
+ break;
+ case GITS_IIDR:
+ *data = gicv3_iidr();
+ break;
+ case GITS_PIDR2:
+ *data = gicv3_idreg(offset - GITS_PIDR2);
+ break;
+ case GITS_TYPER:
+ *data = extract64(s->typer, 0, 32);
+ break;
+ case GITS_TYPER + 4:
+ *data = extract64(s->typer, 32, 32);
+ break;
+ case GITS_CBASER:
+ *data = extract64(s->cbaser, 0, 32);
+ break;
+ case GITS_CBASER + 4:
+ *data = extract64(s->cbaser, 32, 32);
+ break;
+ case GITS_CREADR:
+ *data = extract64(s->creadr, 0, 32);
+ break;
+ case GITS_CREADR + 4:
+ *data = extract64(s->creadr, 32, 32);
+ break;
+ case GITS_CWRITER:
+ *data = extract64(s->cwriter, 0, 32);
+ break;
+ case GITS_CWRITER + 4:
+ *data = extract64(s->cwriter, 32, 32);
+ break;
+ case GITS_BASER ... GITS_BASER + 0x3f:
+ index = (offset - GITS_BASER) / 8;
+ if (offset & 7) {
+ *data = s->baser[index] >> 32;
+ } else {
+ *data = (uint32_t)s->baser[index];
+ }
+ break;
+ default:
+ result = MEMTX_ERROR;
+ break;
+ }
return result;
}
@@ -56,7 +322,35 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,
uint64_t value, MemTxAttrs attrs)
{
MemTxResult result = MEMTX_OK;
+ int index;
+ switch (offset) {
+ case GITS_BASER ... GITS_BASER + 0x3f:
+ /*
+ * IMPDEF choice:- GITS_BASERn register becomes RO if ITS is
+ * already enabled
+ */
+ if (!(s->ctlr & ITS_CTLR_ENABLED)) {
+ index = (offset - GITS_BASER) / 8;
+ s->baser[index] |= (value & ~GITS_BASER_VAL_MASK);
+ }
+ break;
+ case GITS_CBASER:
+ /*
+ * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
+ * already enabled
+ */
+ if (!(s->ctlr & ITS_CTLR_ENABLED)) {
+ s->cbaser = value;
+ }
+ break;
+ case GITS_CWRITER:
+ s->cwriter = value;
+ break;
+ default:
+ result = MEMTX_ERROR;
+ break;
+ }
return result;
}
@@ -64,7 +358,29 @@ static MemTxResult its_readll(GICv3ITSState *s, hwaddr offset,
uint64_t *data, MemTxAttrs attrs)
{
MemTxResult result = MEMTX_OK;
+ int index;
+ switch (offset) {
+ case GITS_TYPER:
+ *data = s->typer;
+ break;
+ case GITS_BASER ... GITS_BASER + 0x3f:
+ index = (offset - GITS_BASER) / 8;
+ *data = s->baser[index];
+ break;
+ case GITS_CBASER:
+ *data = s->cbaser;
+ break;
+ case GITS_CREADR:
+ *data = s->creadr;
+ break;
+ case GITS_CWRITER:
+ *data = s->cwriter;
+ break;
+ default:
+ result = MEMTX_ERROR;
+ break;
+ }
return result;
}
@@ -161,6 +477,9 @@ static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)
gicv3_its_init_mmio(s, &gicv3_its_control_ops, &gicv3_its_translation_ops);
if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
+ address_space_init(&s->gicv3->dma_as, s->gicv3->dma,
+ "gicv3-its-sysmem");
+
/* set the ITS default features supported */
s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL,
GITS_TYPE_PHYSICAL);
@@ -207,6 +526,18 @@ static void gicv3_its_reset(DeviceState *dev)
}
}
+static void gicv3_its_post_load(GICv3ITSState *s)
+{
+ if (s->ctlr & ITS_CTLR_ENABLED) {
+ if (!extract_table_params(s)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: error extracting GITS_BASER parameters\n", __func__);
+ } else {
+ extract_cmdq_params(s);
+ }
+ }
+}
+
static Property gicv3_its_props[] = {
DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "arm-gicv3",
GICv3State *),
@@ -217,10 +548,12 @@ static void gicv3_its_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
GICv3ITSClass *ic = ARM_GICV3_ITS_CLASS(klass);
+ GICv3ITSCommonClass *icc = ARM_GICV3_ITS_COMMON_CLASS(klass);
dc->realize = gicv3_arm_its_realize;
device_class_set_props(dc, gicv3_its_props);
device_class_set_parent_reset(dc, gicv3_its_reset, &ic->parent_reset);
+ icc->post_load = gicv3_its_post_load;
}
static const TypeInfo gicv3_its_info = {
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index e0b06930a7..bfabd5ad62 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -246,6 +246,14 @@ FIELD(GITS_BASER, INNERCACHE, 59, 3)
FIELD(GITS_BASER, INDIRECT, 62, 1)
FIELD(GITS_BASER, VALID, 63, 1)
+FIELD(GITS_CBASER, SIZE, 0, 8)
+FIELD(GITS_CBASER, SHAREABILITY, 10, 2)
+FIELD(GITS_CBASER, PHYADDR, 12, 40)
+FIELD(GITS_CBASER, OUTERCACHE, 53, 3)
+FIELD(GITS_CBASER, INNERCACHE, 59, 3)
+FIELD(GITS_CBASER, VALID, 63, 1)
+
+FIELD(GITS_CTLR, ENABLED, 0, 1)
FIELD(GITS_CTLR, QUIESCENT, 31, 1)
FIELD(GITS_TYPER, PHYSICAL, 0, 1)
@@ -257,6 +265,13 @@ FIELD(GITS_TYPER, PTA, 19, 1)
FIELD(GITS_TYPER, CIDBITS, 32, 4)
FIELD(GITS_TYPER, CIL, 36, 1)
+#define GITS_PIDR2 0xFFE8
+
+#define ITS_CTLR_ENABLED (1U) /* ITS Enabled */
+
+#define GITS_BASER_VAL_MASK (R_GITS_BASER_ENTRYSIZE_MASK | \
+ R_GITS_BASER_TYPE_MASK)
+
#define GITS_BASER_PAGESIZE_4K 0
#define GITS_BASER_PAGESIZE_16K 1
#define GITS_BASER_PAGESIZE_64K 2
@@ -264,6 +279,14 @@ FIELD(GITS_TYPER, CIL, 36, 1)
#define GITS_ITT_TYPE_DEVICE 1ULL
#define GITS_ITT_TYPE_COLLECTION 4ULL
+#define GITS_ITT_PAGE_SIZE_0 0x1000
+#define GITS_ITT_PAGE_SIZE_1 0x4000
+#define GITS_ITT_PAGE_SIZE_2 0x10000
+
+#define L1TABLE_ENTRY_SIZE 8
+
+#define GITS_CMDQ_ENTRY_SIZE 32
+
/**
* Default features advertised by this version of ITS
*/
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 91491a2f66..1fd5cedbbd 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -226,6 +226,9 @@ struct GICv3State {
int dev_fd; /* kvm device fd if backed by kvm vgic support */
Error *migration_blocker;
+ MemoryRegion *dma;
+ AddressSpace dma_as;
+
/* Distributor */
/* for a GIC with the security extensions the NS banked version of this
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 03fc3963f3..140b3ad2be 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -41,6 +41,32 @@
#define GITS_TRANSLATER 0x0040
+typedef struct {
+ bool valid;
+ bool indirect;
+ uint16_t entry_sz;
+ uint32_t page_sz;
+ uint32_t max_entries;
+ uint32_t max_devids;
+ uint64_t base_addr;
+} DevTableDesc;
+
+typedef struct {
+ bool valid;
+ bool indirect;
+ uint16_t entry_sz;
+ uint32_t page_sz;
+ uint32_t max_entries;
+ uint32_t max_collids;
+ uint64_t base_addr;
+} CollTableDesc;
+
+typedef struct {
+ bool valid;
+ uint32_t max_entries;
+ uint64_t base_addr;
+} CmdQDesc;
+
struct GICv3ITSState {
SysBusDevice parent_obj;
@@ -62,6 +88,10 @@ struct GICv3ITSState {
uint64_t creadr;
uint64_t baser[8];
+ DevTableDesc dt;
+ CollTableDesc ct;
+ CmdQDesc cq;
+
Error *migration_blocker;
};
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 3/8] hw/intc: GICv3 ITS command queue framework
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
2021-04-29 23:41 ` [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework Shashi Mallela
2021-04-29 23:41 ` [PATCH v3 2/8] hw/intc: GICv3 ITS register definitions added Shashi Mallela
@ 2021-04-29 23:41 ` Shashi Mallela
2021-05-18 15:43 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 4/8] hw/intc: GICv3 ITS Command processing Shashi Mallela
` (7 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:41 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Added functionality to trigger ITS command queue processing on
write to CWRITE register and process each command queue entry to
identify the command type and handle commands like MAPD,MAPC,SYNC.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/intc/arm_gicv3_its.c | 327 +++++++++++++++++++++++++++++++++++++++
hw/intc/gicv3_internal.h | 41 +++++
2 files changed, 368 insertions(+)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index a7ccb38a89..7cb465813a 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -28,6 +28,327 @@ struct GICv3ITSClass {
void (*parent_reset)(DeviceState *dev);
};
+static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint64_t rdbase;
+ uint64_t value;
+ MemTxResult res = MEMTX_OK;
+
+ offset += NUM_BYTES_IN_DW;
+ offset += NUM_BYTES_IN_DW;
+
+ value = address_space_ldq_le(as, s->cq.base_addr + offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ rdbase = (value >> RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
+
+ if (rdbase < (s->gicv3->num_cpu)) {
+ /*
+ * Current implementation makes a blocking synchronous call
+ * for every command issued earlier,hence the internal state
+ * is already consistent by the time SYNC command is executed.
+ */
+ }
+
+ offset += NUM_BYTES_IN_DW;
+ return res;
+}
+
+static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
+ uint64_t rdbase)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint64_t value;
+ uint64_t l2t_addr;
+ bool valid_l2t;
+ uint32_t l2t_id;
+ uint32_t max_l2_entries;
+ uint64_t cte = 0;
+ MemTxResult res = MEMTX_OK;
+
+ if (s->ct.valid) {
+ if (valid) {
+ /* add mapping entry to collection table */
+ cte = (valid & VALID_MASK) |
+ ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
+ }
+ } else {
+ return res;
+ }
+
+ /*
+ * The specification defines the format of level 1 entries of a
+ * 2-level table, but the format of level 2 entries and the format
+ * of flat-mapped tables is IMPDEF.
+ */
+ if (s->ct.indirect) {
+ l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
+
+ value = address_space_ldq_le(as,
+ s->ct.base_addr +
+ (l2t_id * L1TABLE_ENTRY_SIZE),
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
+
+ if (valid_l2t) {
+ max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
+
+ l2t_addr = value & ((1ULL << 51) - 1);
+
+ address_space_stq_le(as, l2t_addr +
+ ((icid % max_l2_entries) * GITS_CTE_SIZE),
+ cte, MEMTXATTRS_UNSPECIFIED, &res);
+ }
+ } else {
+ /* Flat level table */
+ address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE),
+ cte, MEMTXATTRS_UNSPECIFIED, &res);
+ }
+ return res;
+}
+
+static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint16_t icid;
+ uint64_t rdbase;
+ bool valid;
+ MemTxResult res = MEMTX_OK;
+ uint64_t value;
+
+ offset += NUM_BYTES_IN_DW;
+ offset += NUM_BYTES_IN_DW;
+
+ value = address_space_ldq_le(as, s->cq.base_addr + offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ icid = value & ICID_MASK;
+
+ rdbase = (value >> RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
+
+ valid = (value >> VALID_SHIFT) & VALID_MASK;
+
+ if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "ITS MAPC: invalid collection table attributes "
+ "icid %d rdbase %lu\n", icid, rdbase);
+ /*
+ * in this implementation,in case of error
+ * we ignore this command and move onto the next
+ * command in the queue
+ */
+ } else {
+ res = update_cte(s, icid, valid, rdbase);
+ }
+
+ offset += NUM_BYTES_IN_DW;
+ offset += NUM_BYTES_IN_DW;
+
+ return res;
+}
+
+static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
+ uint8_t size, uint64_t itt_addr)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint64_t value;
+ uint64_t l2t_addr;
+ bool valid_l2t;
+ uint32_t l2t_id;
+ uint32_t max_l2_entries;
+ uint64_t dte = 0;
+ MemTxResult res = MEMTX_OK;
+
+ if (s->dt.valid) {
+ if (valid) {
+ /* add mapping entry to device table */
+ dte = (valid & VALID_MASK) |
+ ((size & SIZE_MASK) << 1U) |
+ ((itt_addr & ITTADDR_MASK) << 6ULL);
+ }
+ } else {
+ return res;
+ }
+
+ /*
+ * The specification defines the format of level 1 entries of a
+ * 2-level table, but the format of level 2 entries and the format
+ * of flat-mapped tables is IMPDEF.
+ */
+ if (s->dt.indirect) {
+ l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE);
+
+ value = address_space_ldq_le(as,
+ s->dt.base_addr +
+ (l2t_id * L1TABLE_ENTRY_SIZE),
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
+
+ if (valid_l2t) {
+ max_l2_entries = s->dt.page_sz / s->dt.entry_sz;
+
+ l2t_addr = value & ((1ULL << 51) - 1);
+
+ address_space_stq_le(as, l2t_addr +
+ ((devid % max_l2_entries) * GITS_DTE_SIZE),
+ dte, MEMTXATTRS_UNSPECIFIED, &res);
+ }
+ } else {
+ /* Flat level table */
+ address_space_stq_le(as, s->dt.base_addr + (devid * GITS_DTE_SIZE),
+ dte, MEMTXATTRS_UNSPECIFIED, &res);
+ }
+ return res;
+}
+
+static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value,
+ uint32_t offset)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint32_t devid;
+ uint8_t size;
+ uint64_t itt_addr;
+ bool valid;
+ MemTxResult res = MEMTX_OK;
+
+ devid = (value >> DEVID_SHIFT) & DEVID_MASK;
+
+ offset += NUM_BYTES_IN_DW;
+ value = address_space_ldq_le(as, s->cq.base_addr + offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ size = (value & SIZE_MASK);
+
+ offset += NUM_BYTES_IN_DW;
+ value = address_space_ldq_le(as, s->cq.base_addr + offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK;
+
+ valid = (value >> VALID_SHIFT) & VALID_MASK;
+
+ if ((devid > s->dt.max_devids) ||
+ (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "ITS MAPD: invalid device table attributes "
+ "devid %d or size %d\n", devid, size);
+ /*
+ * in this implementation,in case of error
+ * we ignore this command and move onto the next
+ * command in the queue
+ */
+ } else {
+ if (res == MEMTX_OK) {
+ res = update_dte(s, devid, valid, size, itt_addr);
+ }
+ }
+
+ offset += NUM_BYTES_IN_DW;
+ offset += NUM_BYTES_IN_DW;
+
+ return res;
+}
+
+/*
+ * Current implementation blocks until all
+ * commands are processed
+ */
+static MemTxResult process_cmdq(GICv3ITSState *s)
+{
+ uint32_t wr_offset = 0;
+ uint32_t rd_offset = 0;
+ uint32_t cq_offset = 0;
+ uint64_t data;
+ AddressSpace *as = &s->gicv3->dma_as;
+ MemTxResult res = MEMTX_OK;
+ uint8_t cmd;
+
+ if (!(s->ctlr & ITS_CTLR_ENABLED)) {
+ return res;
+ }
+
+ wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET);
+
+ if (wr_offset > s->cq.max_entries) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: invalid write offset "
+ "%d\n", __func__, wr_offset);
+ res = MEMTX_ERROR;
+ return res;
+ }
+
+ rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET);
+
+ while (wr_offset != rd_offset) {
+ cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE);
+ data = address_space_ldq_le(as, s->cq.base_addr + cq_offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+ cmd = (data & CMD_MASK);
+
+ switch (cmd) {
+ case GITS_CMD_INT:
+ break;
+ case GITS_CMD_CLEAR:
+ break;
+ case GITS_CMD_SYNC:
+ res = process_sync(s, cq_offset);
+ break;
+ case GITS_CMD_MAPD:
+ res = process_mapd(s, data, cq_offset);
+ break;
+ case GITS_CMD_MAPC:
+ res = process_mapc(s, cq_offset);
+ break;
+ case GITS_CMD_MAPTI:
+ break;
+ case GITS_CMD_MAPI:
+ break;
+ case GITS_CMD_DISCARD:
+ break;
+ default:
+ break;
+ }
+ if (res == MEMTX_OK) {
+ rd_offset++;
+ rd_offset %= s->cq.max_entries;
+ s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset);
+ } else {
+ /*
+ * in this implementation,in case of dma read/write error
+ * we stall the command processing
+ */
+ s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1);
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: %x cmd processing failed!!\n", __func__, cmd);
+ break;
+ }
+ }
+ return res;
+}
+
static bool extract_table_params(GICv3ITSState *s)
{
bool result = true;
@@ -235,6 +556,9 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,
break;
case GITS_CWRITER:
s->cwriter = deposit64(s->cwriter, 0, 32, value);
+ if (s->cwriter != s->creadr) {
+ result = process_cmdq(s);
+ }
break;
case GITS_CWRITER + 4:
s->cwriter = deposit64(s->cwriter, 32, 32, value);
@@ -346,6 +670,9 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,
break;
case GITS_CWRITER:
s->cwriter = value;
+ if (s->cwriter != s->creadr) {
+ result = process_cmdq(s);
+ }
break;
default:
result = MEMTX_ERROR;
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index bfabd5ad62..3b8e1e85c6 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -253,6 +253,12 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3)
FIELD(GITS_CBASER, INNERCACHE, 59, 3)
FIELD(GITS_CBASER, VALID, 63, 1)
+FIELD(GITS_CREADR, STALLED, 0, 1)
+FIELD(GITS_CREADR, OFFSET, 5, 15)
+
+FIELD(GITS_CWRITER, RETRY, 0, 1)
+FIELD(GITS_CWRITER, OFFSET, 5, 15)
+
FIELD(GITS_CTLR, ENABLED, 0, 1)
FIELD(GITS_CTLR, QUIESCENT, 31, 1)
@@ -286,6 +292,41 @@ FIELD(GITS_TYPER, CIL, 36, 1)
#define L1TABLE_ENTRY_SIZE 8
#define GITS_CMDQ_ENTRY_SIZE 32
+#define NUM_BYTES_IN_DW 8
+
+#define CMD_MASK 0xff
+
+/* ITS Commands */
+#define GITS_CMD_CLEAR 0x04
+#define GITS_CMD_DISCARD 0x0F
+#define GITS_CMD_INT 0x03
+#define GITS_CMD_MAPC 0x09
+#define GITS_CMD_MAPD 0x08
+#define GITS_CMD_MAPI 0x0B
+#define GITS_CMD_MAPTI 0x0A
+#define GITS_CMD_SYNC 0x05
+
+/* MAPC command fields */
+#define ICID_LENGTH 16
+#define ICID_MASK ((1U << ICID_LENGTH) - 1)
+#define RDBASE_LENGTH 32
+#define RDBASE_SHIFT 16
+#define RDBASE_MASK ((1ULL << RDBASE_LENGTH) - 1)
+#define RDBASE_PROCNUM_LENGTH 16
+#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) - 1)
+
+#define DEVID_SHIFT 32
+#define DEVID_LENGTH 32
+#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1)
+
+/* MAPD command fields */
+#define ITTADDR_LENGTH 44
+#define ITTADDR_SHIFT 8
+#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1)
+#define SIZE_MASK 0x1f
+
+#define VALID_SHIFT 63
+#define VALID_MASK 0x1
/**
* Default features advertised by this version of ITS
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 4/8] hw/intc: GICv3 ITS Command processing
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (2 preceding siblings ...)
2021-04-29 23:41 ` [PATCH v3 3/8] hw/intc: GICv3 ITS command queue framework Shashi Mallela
@ 2021-04-29 23:41 ` Shashi Mallela
2021-05-18 15:49 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 5/8] hw/intc: GICv3 ITS Feature enablement Shashi Mallela
` (6 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:41 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Added ITS command queue handling for MAPTI,MAPI commands,handled ITS
translation which triggers an LPI via INT command as well as write
to GITS_TRANSLATER register,defined enum to differentiate between ITS
command interrupt trigger and GITS_TRANSLATER based interrupt trigger.
Each of these commands make use of other functionalities implemented to
get device table entry,collection table entry or interrupt translation
table entry required for their processing.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/intc/arm_gicv3_its.c | 346 ++++++++++++++++++++++++++++-
hw/intc/gicv3_internal.h | 12 +
include/hw/intc/arm_gicv3_common.h | 2 +
3 files changed, 359 insertions(+), 1 deletion(-)
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 7cb465813a..98c984dd22 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -28,6 +28,156 @@ struct GICv3ITSClass {
void (*parent_reset)(DeviceState *dev);
};
+typedef enum ItsCmdType {
+ NONE = 0, /* internal indication for GITS_TRANSLATER write */
+ CLEAR = 1,
+ DISCARD = 2,
+ INT = 3,
+} ItsCmdType;
+
+static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte,
+ MemTxResult *res)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint64_t l2t_addr;
+ uint64_t value;
+ bool valid_l2t;
+ uint32_t l2t_id;
+ uint32_t max_l2_entries;
+ bool status = false;
+
+ if (s->ct.indirect) {
+ l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
+
+ value = address_space_ldq_le(as,
+ s->ct.base_addr +
+ (l2t_id * L1TABLE_ENTRY_SIZE),
+ MEMTXATTRS_UNSPECIFIED, res);
+
+ if (*res == MEMTX_OK) {
+ valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
+
+ if (valid_l2t) {
+ max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
+
+ l2t_addr = value & ((1ULL << 51) - 1);
+
+ *cte = address_space_ldq_le(as, l2t_addr +
+ ((icid % max_l2_entries) * GITS_CTE_SIZE),
+ MEMTXATTRS_UNSPECIFIED, res);
+ }
+ }
+ } else {
+ /* Flat level table */
+ *cte = address_space_ldq_le(as, s->ct.base_addr +
+ (icid * GITS_CTE_SIZE),
+ MEMTXATTRS_UNSPECIFIED, res);
+ }
+
+ if (*cte & VALID_MASK) {
+ status = true;
+ }
+
+ return status;
+}
+
+static MemTxResult update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
+ uint64_t itel, uint32_t iteh)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint64_t itt_addr;
+ MemTxResult res = MEMTX_OK;
+
+ itt_addr = (dte >> 6ULL) & ITTADDR_MASK;
+ itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
+
+ address_space_stq_le(as, itt_addr + (eventid * sizeof(uint64_t)),
+ itel, MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res == MEMTX_OK) {
+ address_space_stl_le(as, itt_addr + ((eventid + sizeof(uint64_t)) *
+ sizeof(uint32_t)), iteh, MEMTXATTRS_UNSPECIFIED,
+ &res);
+ }
+ return res;
+}
+
+static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
+ uint16_t *icid, uint32_t *pIntid, MemTxResult *res)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint64_t itt_addr;
+ bool status = false;
+ uint64_t itel = 0;
+ uint32_t iteh = 0;
+
+ itt_addr = (dte >> 6ULL) & ITTADDR_MASK;
+ itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
+
+ itel = address_space_ldq_le(as, itt_addr + (eventid * sizeof(uint64_t)),
+ MEMTXATTRS_UNSPECIFIED, res);
+
+ if (*res == MEMTX_OK) {
+ iteh = address_space_ldl_le(as, itt_addr + ((eventid +
+ sizeof(uint64_t)) * sizeof(uint32_t)),
+ MEMTXATTRS_UNSPECIFIED, res);
+
+ if (*res == MEMTX_OK) {
+ if (itel & VALID_MASK) {
+ if ((itel >> ITE_ENTRY_INTTYPE_SHIFT) & GITS_TYPE_PHYSICAL) {
+ *pIntid = (itel >> ITE_ENTRY_INTID_SHIFT) &
+ ITE_ENTRY_INTID_MASK;
+ *icid = iteh & ITE_ENTRY_ICID_MASK;
+ status = true;
+ }
+ }
+ }
+ }
+ return status;
+}
+
+static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint64_t l2t_addr;
+ uint64_t value;
+ bool valid_l2t;
+ uint32_t l2t_id;
+ uint32_t max_l2_entries;
+
+ if (s->dt.indirect) {
+ l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE);
+
+ value = address_space_ldq_le(as,
+ s->dt.base_addr +
+ (l2t_id * L1TABLE_ENTRY_SIZE),
+ MEMTXATTRS_UNSPECIFIED, res);
+
+ if (*res == MEMTX_OK) {
+ valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
+
+ if (valid_l2t) {
+ max_l2_entries = s->dt.page_sz / s->dt.entry_sz;
+
+ l2t_addr = value & ((1ULL << 51) - 1);
+
+ value = 0;
+ value = address_space_ldq_le(as, l2t_addr +
+ ((devid % max_l2_entries) * GITS_DTE_SIZE),
+ MEMTXATTRS_UNSPECIFIED, res);
+ }
+ }
+ } else {
+ /* Flat level table */
+ value = 0;
+ value = address_space_ldq_le(as, s->dt.base_addr +
+ (devid * GITS_DTE_SIZE),
+ MEMTXATTRS_UNSPECIFIED, res);
+ }
+
+ return value;
+}
+
static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
{
AddressSpace *as = &s->gicv3->dma_as;
@@ -55,6 +205,182 @@ static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
return res;
}
+static MemTxResult process_int(GICv3ITSState *s, uint64_t value,
+ uint32_t offset, ItsCmdType cmd)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint32_t devid, eventid;
+ MemTxResult res = MEMTX_OK;
+ bool dte_valid;
+ uint64_t dte = 0;
+ uint32_t max_eventid;
+ uint16_t icid = 0;
+ uint32_t pIntid = 0;
+ bool ite_valid = false;
+ uint64_t cte = 0;
+ bool cte_valid = false;
+ uint64_t itel = 0;
+ uint32_t iteh = 0;
+
+ if (cmd == NONE) {
+ devid = offset;
+ } else {
+ devid = (value >> DEVID_SHIFT) & DEVID_MASK;
+
+ offset += NUM_BYTES_IN_DW;
+ value = address_space_ldq_le(as, s->cq.base_addr + offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+ }
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ eventid = (value & EVENTID_MASK);
+
+ dte = get_dte(s, devid, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+ dte_valid = dte & VALID_MASK;
+
+ if (dte_valid) {
+ max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1));
+
+ ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ if (ite_valid) {
+ cte_valid = get_cte(s, icid, &cte, &res);
+ }
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+ }
+
+ if ((devid > s->dt.max_devids) || !dte_valid || !ite_valid ||
+ !cte_valid || (eventid > max_eventid)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: invalid interrupt translation table attributes "
+ "devid %d or eventid %d\n",
+ __func__, devid, eventid);
+ /*
+ * in this implementation,in case of error
+ * we ignore this command and move onto the next
+ * command in the queue
+ */
+ } else {
+ /*
+ * Current implementation only supports rdbase == procnum
+ * Hence rdbase physical address is ignored
+ */
+ if (cmd == DISCARD) {
+ /* remove mapping from interrupt translation table */
+ res = update_ite(s, eventid, dte, itel, iteh);
+ }
+ }
+
+ if (cmd != NONE) {
+ offset += NUM_BYTES_IN_DW;
+ offset += NUM_BYTES_IN_DW;
+ }
+
+ return res;
+}
+
+static MemTxResult process_mapti(GICv3ITSState *s, uint64_t value,
+ uint32_t offset, bool ignore_pInt)
+{
+ AddressSpace *as = &s->gicv3->dma_as;
+ uint32_t devid, eventid;
+ uint32_t pIntid = 0;
+ uint32_t max_eventid, max_Intid;
+ bool dte_valid;
+ MemTxResult res = MEMTX_OK;
+ uint16_t icid = 0;
+ uint64_t dte = 0;
+ uint64_t itel = 0;
+ uint32_t iteh = 0;
+ uint32_t int_spurious = INTID_SPURIOUS;
+
+ devid = (value >> DEVID_SHIFT) & DEVID_MASK;
+ offset += NUM_BYTES_IN_DW;
+ value = address_space_ldq_le(as, s->cq.base_addr + offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ eventid = (value & EVENTID_MASK);
+
+ if (!ignore_pInt) {
+ pIntid = (value >> pINTID_OFFSET) & pINTID_MASK;
+ }
+
+ offset += NUM_BYTES_IN_DW;
+ value = address_space_ldq_le(as, s->cq.base_addr + offset,
+ MEMTXATTRS_UNSPECIFIED, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+
+ icid = value & ICID_MASK;
+
+ dte = get_dte(s, devid, &res);
+
+ if (res != MEMTX_OK) {
+ return res;
+ }
+ dte_valid = dte & VALID_MASK;
+
+ max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1));
+
+ if (!ignore_pInt) {
+ max_Intid = (1UL << (FIELD_EX64(s->typer, GITS_TYPER, IDBITS) + 1));
+ }
+
+ if ((devid > s->dt.max_devids) || (icid > s->ct.max_collids) ||
+ !dte_valid || (eventid > max_eventid) ||
+ (!ignore_pInt && ((pIntid < GICV3_LPI_INTID_START) ||
+ (pIntid > max_Intid)))) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: invalid interrupt translation table attributes "
+ "devid %d or icid %d or eventid %d or pIntid %d\n",
+ __func__, devid, icid, eventid, pIntid);
+ /*
+ * in this implementation,in case of error
+ * we ignore this command and move onto the next
+ * command in the queue
+ */
+ } else {
+ /* add ite entry to interrupt translation table */
+ itel = (dte_valid & VALID_MASK) | (GITS_TYPE_PHYSICAL <<
+ ITE_ENTRY_INTTYPE_SHIFT);
+
+ if (ignore_pInt) {
+ itel |= (eventid << ITE_ENTRY_INTID_SHIFT);
+ } else {
+ itel |= (pIntid << ITE_ENTRY_INTID_SHIFT);
+ }
+ itel |= (int_spurious << ITE_ENTRY_INTSP_SHIFT);
+ iteh |= icid;
+
+ res = update_ite(s, eventid, dte, itel, iteh);
+ }
+
+ offset += NUM_BYTES_IN_DW;
+ offset += NUM_BYTES_IN_DW;
+
+ return res;
+}
+
static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
uint64_t rdbase)
{
@@ -217,7 +543,7 @@ static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
}
static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value,
- uint32_t offset)
+ uint32_t offset)
{
AddressSpace *as = &s->gicv3->dma_as;
uint32_t devid;
@@ -310,8 +636,10 @@ static MemTxResult process_cmdq(GICv3ITSState *s)
switch (cmd) {
case GITS_CMD_INT:
+ res = process_int(s, data, cq_offset, INT);
break;
case GITS_CMD_CLEAR:
+ res = process_int(s, data, cq_offset, CLEAR);
break;
case GITS_CMD_SYNC:
res = process_sync(s, cq_offset);
@@ -323,10 +651,13 @@ static MemTxResult process_cmdq(GICv3ITSState *s)
res = process_mapc(s, cq_offset);
break;
case GITS_CMD_MAPTI:
+ res = process_mapti(s, data, cq_offset, false);
break;
case GITS_CMD_MAPI:
+ res = process_mapti(s, data, cq_offset, true);
break;
case GITS_CMD_DISCARD:
+ res = process_int(s, data, cq_offset, DISCARD);
break;
default:
break;
@@ -508,7 +839,20 @@ static void extract_cmdq_params(GICv3ITSState *s)
static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
uint64_t data, unsigned size, MemTxAttrs attrs)
{
+ GICv3ITSState *s = (GICv3ITSState *)opaque;
MemTxResult result = MEMTX_OK;
+ uint32_t devid = 0;
+
+ switch (offset) {
+ case GITS_TRANSLATER:
+ if (s->ctlr & ITS_CTLR_ENABLED) {
+ devid = attrs.requester_id;
+ result = process_int(s, data, devid, NONE);
+ }
+ break;
+ default:
+ break;
+ }
return result;
}
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 3b8e1e85c6..e49370224f 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -325,6 +325,13 @@ FIELD(GITS_TYPER, CIL, 36, 1)
#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1)
#define SIZE_MASK 0x1f
+/* MAPI command fields */
+#define EVENTID_MASK ((1ULL << 32) - 1)
+
+/* MAPTI command fields */
+#define pINTID_OFFSET 32
+#define pINTID_MASK ((1ULL << 32) - 1)
+
#define VALID_SHIFT 63
#define VALID_MASK 0x1
@@ -345,6 +352,11 @@ FIELD(GITS_TYPER, CIL, 36, 1)
* vPEID = 16 bits
*/
#define ITS_ITT_ENTRY_SIZE 0xC
+#define ITE_ENTRY_INTTYPE_SHIFT 1
+#define ITE_ENTRY_INTID_SHIFT 2
+#define ITE_ENTRY_INTID_MASK ((1ULL << 24) - 1)
+#define ITE_ENTRY_INTSP_SHIFT 26
+#define ITE_ENTRY_ICID_MASK ((1ULL << 16) - 1)
/* 16 bits EventId */
#define ITS_IDBITS GICD_TYPER_IDBITS
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 1fd5cedbbd..0715b0bc2a 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -36,6 +36,8 @@
#define GICV3_MAXIRQ 1020
#define GICV3_MAXSPI (GICV3_MAXIRQ - GIC_INTERNAL)
+#define GICV3_LPI_INTID_START 8192
+
#define GICV3_REDIST_SIZE 0x20000
/* Number of SGI target-list bits */
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 5/8] hw/intc: GICv3 ITS Feature enablement
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (3 preceding siblings ...)
2021-04-29 23:41 ` [PATCH v3 4/8] hw/intc: GICv3 ITS Command processing Shashi Mallela
@ 2021-04-29 23:41 ` Shashi Mallela
2021-05-18 15:58 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 6/8] hw/intc: GICv3 redistributor ITS processing Shashi Mallela
` (5 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:41 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Added properties to enable ITS feature and define qemu system
address space memory in gicv3 common,setup distributor and
redistributor registers to indicate LPI support.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/intc/arm_gicv3_common.c | 13 +++++++++++++
hw/intc/arm_gicv3_dist.c | 21 +++++++++++++++++++--
hw/intc/arm_gicv3_redist.c | 30 +++++++++++++++++++++++++-----
hw/intc/gicv3_internal.h | 17 +++++++++++++++++
include/hw/intc/arm_gicv3_common.h | 1 +
5 files changed, 75 insertions(+), 7 deletions(-)
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 58ef65f589..a55e91071a 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -381,6 +381,16 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
(1 << 24) |
(i << 8) |
(last << 4);
+
+ if (s->lpi_enable) {
+ s->cpu[i].gicr_typer |= GICR_TYPER_PLPIS;
+
+ if (!s->dma) {
+ error_setg(errp,
+ "Redist-ITS: Guest 'sysmem' reference link not set");
+ return;
+ }
+ }
}
}
@@ -494,9 +504,12 @@ static Property arm_gicv3_common_properties[] = {
DEFINE_PROP_UINT32("num-cpu", GICv3State, num_cpu, 1),
DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
+ DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0),
DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
DEFINE_PROP_ARRAY("redist-region-count", GICv3State, nb_redist_regions,
redist_region_count, qdev_prop_uint32, uint32_t),
+ DEFINE_PROP_LINK("sysmem", GICv3State, dma, TYPE_MEMORY_REGION,
+ MemoryRegion *),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index b65f56f903..43e0ea4367 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -366,12 +366,15 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
return MEMTX_OK;
case GICD_TYPER:
{
+ bool lpi_supported = false;
/* For this implementation:
* No1N == 1 (1-of-N SPI interrupts not supported)
* A3V == 1 (non-zero values of Affinity level 3 supported)
* IDbits == 0xf (we support 16-bit interrupt identifiers)
* DVIS == 0 (Direct virtual LPI injection not supported)
- * LPIS == 0 (LPIs not supported)
+ * LPIS == 1 (LPIs are supported if affinity routing is enabled)
+ * num_LPIs == 0b00000 (bits [15:11],Number of LPIs as indicated
+ * by GICD_TYPER.IDbits)
* MBIS == 0 (message-based SPIs not supported)
* SecurityExtn == 1 if security extns supported
* CPUNumber == 0 since for us ARE is always 1
@@ -385,8 +388,22 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
*/
bool sec_extn = !(s->gicd_ctlr & GICD_CTLR_DS);
+ /*
+ * With securityextn on, LPIs are supported when affinity routing
+ * is enabled for non-secure state and if off LPIs are supported
+ * when affinity routing is enabled.
+ */
+ if (s->lpi_enable) {
+ if (sec_extn) {
+ lpi_supported = (s->gicd_ctlr & GICD_CTLR_ARE_NS);
+ } else {
+ lpi_supported = (s->gicd_ctlr & GICD_CTLR_ARE);
+ }
+ }
+
*data = (1 << 25) | (1 << 24) | (sec_extn << 10) |
- (0xf << 19) | itlinesnumber;
+ (lpi_supported << GICD_TYPER_LPIS_OFFSET) | (GICD_TYPER_IDBITS <<
+ GICD_TYPER_IDBITS_OFFSET) | itlinesnumber;
return MEMTX_OK;
}
case GICD_IIDR:
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 8645220d61..7604ccdc83 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -244,14 +244,22 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset,
static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
uint64_t value, MemTxAttrs attrs)
{
+ uint64_t data;
+
switch (offset) {
case GICR_CTLR:
/* For our implementation, GICR_TYPER.DPGS is 0 and so all
* the DPG bits are RAZ/WI. We don't do anything asynchronously,
- * so UWP and RWP are RAZ/WI. And GICR_TYPER.LPIS is 0 (we don't
- * implement LPIs) so Enable_LPIs is RES0. So there are no writable
- * bits for us.
+ * so UWP and RWP are RAZ/WI. GICR_TYPER.LPIS is 1 (we
+ * implement LPIs) so Enable_LPIs is programmable.
*/
+ if (cs->gicr_typer & GICR_TYPER_PLPIS) {
+ if (value & GICR_CTLR_ENABLE_LPIS) {
+ cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
+ } else {
+ cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
+ }
+ }
return MEMTX_OK;
case GICR_STATUSR:
/* RAZ/WI for our implementation */
@@ -275,7 +283,12 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
cs->gicr_waker = value;
return MEMTX_OK;
case GICR_PROPBASER:
- cs->gicr_propbaser = deposit64(cs->gicr_propbaser, 0, 32, value);
+ data = value;
+ if (FIELD_EX64(data, GICR_PROPBASER, IDBITS) > GICD_TYPER_IDBITS) {
+ data &= ~R_GICR_PROPBASER_IDBITS_MASK;
+ data |= GICD_TYPER_IDBITS;
+ }
+ cs->gicr_propbaser = deposit64(cs->gicr_propbaser, 0, 32, data);
return MEMTX_OK;
case GICR_PROPBASER + 4:
cs->gicr_propbaser = deposit64(cs->gicr_propbaser, 32, 32, value);
@@ -395,9 +408,16 @@ static MemTxResult gicr_readll(GICv3CPUState *cs, hwaddr offset,
static MemTxResult gicr_writell(GICv3CPUState *cs, hwaddr offset,
uint64_t value, MemTxAttrs attrs)
{
+ uint64_t data;
+
switch (offset) {
case GICR_PROPBASER:
- cs->gicr_propbaser = value;
+ data = value;
+ if (FIELD_EX64(data, GICR_PROPBASER, IDBITS) > GICD_TYPER_IDBITS) {
+ data &= ~R_GICR_PROPBASER_IDBITS_MASK;
+ data |= GICD_TYPER_IDBITS;
+ }
+ cs->gicr_propbaser = data;
return MEMTX_OK;
case GICR_PENDBASER:
cs->gicr_pendbaser = value;
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index e49370224f..c99a05461e 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -68,6 +68,9 @@
#define GICD_CTLR_E1NWF (1U << 7)
#define GICD_CTLR_RWP (1U << 31)
+#define GICD_TYPER_LPIS_OFFSET 17
+#define GICD_TYPER_IDBITS_OFFSET 19
+#define GICD_TYPER_IDBITS_MASK 0x1f
/* 16 bits EventId */
#define GICD_TYPER_IDBITS 0xf
@@ -126,6 +129,20 @@
#define GICR_WAKER_ProcessorSleep (1U << 1)
#define GICR_WAKER_ChildrenAsleep (1U << 2)
+FIELD(GICR_PROPBASER, IDBITS, 0, 5)
+FIELD(GICR_PROPBASER, INNERCACHE, 7, 3)
+FIELD(GICR_PROPBASER, SHAREABILITY, 10, 2)
+FIELD(GICR_PROPBASER, PHYADDR, 12, 40)
+FIELD(GICR_PROPBASER, OUTERCACHE, 56, 3)
+
+#define GICR_PROPBASER_IDBITS_THRESHOLD 0xd
+
+FIELD(GICR_PENDBASER, INNERCACHE, 7, 3)
+FIELD(GICR_PENDBASER, SHAREABILITY, 10, 2)
+FIELD(GICR_PENDBASER, PHYADDR, 16, 36)
+FIELD(GICR_PENDBASER, OUTERCACHE, 56, 3)
+FIELD(GICR_PENDBASER, PTZ, 62, 1)
+
#define ICC_CTLR_EL1_CBPR (1U << 0)
#define ICC_CTLR_EL1_EOIMODE (1U << 1)
#define ICC_CTLR_EL1_PMHE (1U << 6)
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 0715b0bc2a..c1348cc60a 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -221,6 +221,7 @@ struct GICv3State {
uint32_t num_cpu;
uint32_t num_irq;
uint32_t revision;
+ bool lpi_enable;
bool security_extn;
bool irq_reset_nonsecure;
bool gicd_no_migration_shift_bug;
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 6/8] hw/intc: GICv3 redistributor ITS processing
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (4 preceding siblings ...)
2021-04-29 23:41 ` [PATCH v3 5/8] hw/intc: GICv3 ITS Feature enablement Shashi Mallela
@ 2021-04-29 23:41 ` Shashi Mallela
2021-05-20 11:01 ` Peter Maydell
2021-04-29 23:42 ` [PATCH v3 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC Shashi Mallela
` (4 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:41 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Implemented lpi processing at redistributor to get lpi config info
from lpi configuration table,determine priority,set pending state in
lpi pending table and forward the lpi to cpuif.Added logic to invoke
redistributor lpi processing with translated LPI which set/clear LPI
from ITS device as part of ITS INT,CLEAR,DISCARD command and
GITS_TRANSLATER processing.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/intc/arm_gicv3.c | 6 ++
hw/intc/arm_gicv3_cpuif.c | 20 ++++--
hw/intc/arm_gicv3_its.c | 12 ++--
hw/intc/arm_gicv3_redist.c | 133 +++++++++++++++++++++++++++++++++++++
hw/intc/gicv3_internal.h | 9 +++
5 files changed, 171 insertions(+), 9 deletions(-)
diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
index 66eaa97198..618fa1af95 100644
--- a/hw/intc/arm_gicv3.c
+++ b/hw/intc/arm_gicv3.c
@@ -166,6 +166,12 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)
cs->hppi.grp = gicv3_irq_group(cs->gic, cs, cs->hppi.irq);
}
+ if (cs->gic->lpi_enable) {
+ if (gicv3_redist_update_lpi(cs)) {
+ seenbetter = true;
+ }
+ }
+
/* If the best interrupt we just found would preempt whatever
* was the previous best interrupt before this update, then
* we know it's definitely the best one now.
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 43ef1d7a84..11b1df5b6b 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -899,9 +899,14 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq)
cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0);
gicv3_redist_update(cs);
} else {
- gicv3_gicd_active_set(cs->gic, irq);
- gicv3_gicd_pending_clear(cs->gic, irq);
- gicv3_update(cs->gic, irq, 1);
+ if (irq >= GICV3_LPI_INTID_START) {
+ gicv3_redist_lpi_pending(cs, irq, 0);
+ gicv3_redist_update(cs);
+ } else {
+ gicv3_gicd_active_set(cs->gic, irq);
+ gicv3_gicd_pending_clear(cs->gic, irq);
+ gicv3_update(cs->gic, irq, 1);
+ }
}
}
@@ -1328,7 +1333,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
}
}
- if (irq >= cs->gic->num_irq) {
+ if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable &&
+ (irq >= GICV3_LPI_INTID_START)))) {
/* This handles two cases:
* 1. If software writes the ID of a spurious interrupt [ie 1020-1023]
* to the GICC_EOIR, the GIC ignores that write.
@@ -1348,7 +1354,11 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
if (!icc_eoi_split(env, cs)) {
/* Priority drop and deactivate not split: deactivate irq now */
- icc_deactivate_irq(cs, irq);
+ if (irq >= GICV3_LPI_INTID_START) {
+ gicv3_update(cs->gic, irq, 1);
+ } else {
+ icc_deactivate_irq(cs, irq);
+ }
}
}
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 98c984dd22..28da2d1d77 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -219,6 +219,7 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value,
bool ite_valid = false;
uint64_t cte = 0;
bool cte_valid = false;
+ uint64_t rdbase;
uint64_t itel = 0;
uint32_t iteh = 0;
@@ -275,10 +276,13 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value,
* command in the queue
*/
} else {
- /*
- * Current implementation only supports rdbase == procnum
- * Hence rdbase physical address is ignored
- */
+ rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK;
+ if ((cmd == CLEAR) || (cmd == DISCARD)) {
+ gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0);
+ } else {
+ gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1);
+ }
+
if (cmd == DISCARD) {
/* remove mapping from interrupt translation table */
res = update_ite(s, eventid, dte, itel, iteh);
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 7604ccdc83..82ca9d71e5 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -256,6 +256,8 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
if (cs->gicr_typer & GICR_TYPER_PLPIS) {
if (value & GICR_CTLR_ENABLE_LPIS) {
cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
+ /* Check for any pending interr in pending table */
+ gicv3_redist_update(cs);
} else {
cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
}
@@ -546,6 +548,137 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,
return r;
}
+bool gicv3_redist_update_lpi(GICv3CPUState *cs)
+{
+ /*
+ * This function scans the LPI pending table and for each pending
+ * LPI, reads the corresponding entry from LPI configuration table
+ * to extract the priority info and determine if the LPI priority
+ * is lower than the current high priority interrupt.If yes, update
+ * high priority pending interrupt to that of LPI.
+ */
+ AddressSpace *as = &cs->gic->dma_as;
+ uint64_t lpict_baddr, lpipt_baddr;
+ uint32_t pendt_size = 0;
+ uint8_t lpite;
+ uint8_t prio, pend;
+ int i;
+ bool seenbetter = false;
+
+ if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
+ !cs->gicr_pendbaser || (FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
+ IDBITS) < GICR_PROPBASER_IDBITS_THRESHOLD)) {
+ return seenbetter;
+ }
+
+ lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, PHYADDR);
+ lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
+
+ lpipt_baddr = FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER, PHYADDR);
+ lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
+
+ /* Determine the highest priority pending interrupt among LPIs */
+ pendt_size = (1UL << (FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
+ IDBITS) - 1));
+
+ for (i = 0; i < pendt_size; i++) {
+ address_space_read(as, lpipt_baddr +
+ (((GICV3_LPI_INTID_START + i) / 8) * sizeof(pend)),
+ MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
+
+ if ((1 << ((GICV3_LPI_INTID_START + i) % 8)) & pend) {
+ address_space_read(as, lpict_baddr + (i * sizeof(lpite)),
+ MEMTXATTRS_UNSPECIFIED, &lpite, sizeof(lpite));
+
+ prio = ((lpite >> LPI_CTE_PRIORITY_OFFSET) &
+ LPI_CTE_PRIORITY_MASK);
+ prio &= LPI_PRIORITY_MASK;
+
+ if (prio < cs->hppi.prio) {
+ cs->hppi.irq = GICV3_LPI_INTID_START + i;
+ cs->hppi.prio = prio;
+ /* LPIs are always non-secure Grp1 interrupts */
+ cs->hppi.grp = GICV3_G1NS;
+ seenbetter = true;
+ }
+ }
+ }
+ return seenbetter;
+}
+
+void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level)
+{
+ AddressSpace *as = &cs->gic->dma_as;
+ uint64_t lpipt_baddr;
+ bool ispend = false;
+ uint8_t pend;
+
+ /*
+ * get the bit value corresponding to this irq in the
+ * lpi pending table
+ */
+ lpipt_baddr = FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER, PHYADDR);
+ lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
+
+ address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),
+ MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
+ ispend = ((pend >> (irq % 8)) & 0x1);
+
+ if (ispend) {
+ if (!level) {
+ /*
+ * clear the pending bit and update the lpi pending table
+ */
+ pend &= ~(1 << (irq % 8));
+
+ address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),
+ MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
+ }
+ } else {
+ if (level) {
+ /*
+ * if pending bit is not already set for this irq,turn-on the
+ * pending bit and update the lpi pending table
+ */
+ pend |= (1 << (irq % 8));
+
+ address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),
+ MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
+ }
+ }
+}
+
+void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)
+{
+ AddressSpace *as = &cs->gic->dma_as;
+ uint64_t lpict_baddr;
+ uint8_t lpite;
+
+ if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
+ !cs->gicr_pendbaser || (FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
+ IDBITS) < GICR_PROPBASER_IDBITS_THRESHOLD)) {
+ return;
+ }
+
+ lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, PHYADDR);
+ lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
+
+ /* get the lpi config table entry corresponding to this irq */
+ address_space_read(as, lpict_baddr + ((irq - GICV3_LPI_INTID_START) *
+ sizeof(lpite)), MEMTXATTRS_UNSPECIFIED,
+ &lpite, sizeof(lpite));
+
+ /* check if this irq is enabled before proceeding further */
+ if (!(lpite & LPI_CTE_ENABLED)) {
+ return;
+ }
+
+ /* set/clear the pending bit for this irq */
+ gicv3_redist_lpi_pending(cs, irq, level);
+
+ gicv3_redist_update(cs);
+}
+
void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level)
{
/* Update redistributor state for a change in an external PPI input line */
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index c99a05461e..dcdad8d0af 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -308,6 +308,12 @@ FIELD(GITS_TYPER, CIL, 36, 1)
#define L1TABLE_ENTRY_SIZE 8
+#define LPI_CTE_ENABLE_OFFSET 0
+#define LPI_CTE_ENABLED VALID_MASK
+#define LPI_CTE_PRIORITY_OFFSET 2
+#define LPI_CTE_PRIORITY_MASK ((1U << 6) - 1)
+#define LPI_PRIORITY_MASK 0xfc
+
#define GITS_CMDQ_ENTRY_SIZE 32
#define NUM_BYTES_IN_DW 8
@@ -453,6 +459,9 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,
unsigned size, MemTxAttrs attrs);
void gicv3_dist_set_irq(GICv3State *s, int irq, int level);
void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level);
+void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level);
+void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level);
+bool gicv3_redist_update_lpi(GICv3CPUState *cs);
void gicv3_redist_send_sgi(GICv3CPUState *cs, int grp, int irq, bool ns);
void gicv3_init_cpuif(GICv3State *s);
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (5 preceding siblings ...)
2021-04-29 23:41 ` [PATCH v3 6/8] hw/intc: GICv3 redistributor ITS processing Shashi Mallela
@ 2021-04-29 23:42 ` Shashi Mallela
2021-05-18 16:03 ` Peter Maydell
2021-04-29 23:42 ` [PATCH v3 8/8] hw/arm/virt: add ITS support in virt GIC Shashi Mallela
` (3 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:42 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Included creation of ITS as part of SBSA platform GIC
initialization.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/arm/sbsa-ref.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 88dfb2284c..d05cbcae48 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -35,7 +35,7 @@
#include "hw/boards.h"
#include "hw/ide/internal.h"
#include "hw/ide/ahci_internal.h"
-#include "hw/intc/arm_gicv3_common.h"
+#include "hw/intc/arm_gicv3_its_common.h"
#include "hw/loader.h"
#include "hw/pci-host/gpex.h"
#include "hw/qdev-properties.h"
@@ -65,6 +65,7 @@ enum {
SBSA_CPUPERIPHS,
SBSA_GIC_DIST,
SBSA_GIC_REDIST,
+ SBSA_GIC_ITS,
SBSA_SECURE_EC,
SBSA_GWDT,
SBSA_GWDT_REFRESH,
@@ -108,6 +109,7 @@ static const MemMapEntry sbsa_ref_memmap[] = {
[SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 },
[SBSA_GIC_DIST] = { 0x40060000, 0x00010000 },
[SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 },
+ [SBSA_GIC_ITS] = { 0x44090000, 0x00020000 },
[SBSA_SECURE_EC] = { 0x50000000, 0x00001000 },
[SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 },
[SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 },
@@ -378,7 +380,20 @@ static void create_secure_ram(SBSAMachineState *sms,
memory_region_add_subregion(secure_sysmem, base, secram);
}
-static void create_gic(SBSAMachineState *sms)
+static void create_its(SBSAMachineState *sms)
+{
+ DeviceState *dev;
+
+ dev = qdev_new(TYPE_ARM_GICV3_ITS);
+ SysBusDevice *s = SYS_BUS_DEVICE(dev);
+
+ object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(sms->gic),
+ &error_abort);
+ sysbus_realize_and_unref(s, &error_fatal);
+ sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base);
+}
+
+static void create_gic(SBSAMachineState *sms, MemoryRegion *mem)
{
unsigned int smp_cpus = MACHINE(sms)->smp.cpus;
SysBusDevice *gicbusdev;
@@ -405,6 +420,10 @@ static void create_gic(SBSAMachineState *sms)
qdev_prop_set_uint32(sms->gic, "len-redist-region-count", 1);
qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", redist0_count);
+ object_property_set_link(OBJECT(sms->gic), "sysmem", OBJECT(mem),
+ &error_fatal);
+ qdev_prop_set_bit(sms->gic, "has-lpi", true);
+
gicbusdev = SYS_BUS_DEVICE(sms->gic);
sysbus_realize_and_unref(gicbusdev, &error_fatal);
sysbus_mmio_map(gicbusdev, 0, sbsa_ref_memmap[SBSA_GIC_DIST].base);
@@ -451,6 +470,7 @@ static void create_gic(SBSAMachineState *sms)
sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus,
qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
}
+ create_its(sms);
}
static void create_uart(const SBSAMachineState *sms, int uart,
@@ -763,7 +783,7 @@ static void sbsa_ref_init(MachineState *machine)
create_secure_ram(sms, secure_sysmem);
- create_gic(sms);
+ create_gic(sms, sysmem);
create_uart(sms, SBSA_UART, sysmem, serial_hd(0));
create_uart(sms, SBSA_SECURE_UART, secure_sysmem, serial_hd(1));
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 8/8] hw/arm/virt: add ITS support in virt GIC
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (6 preceding siblings ...)
2021-04-29 23:42 ` [PATCH v3 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC Shashi Mallela
@ 2021-04-29 23:42 ` Shashi Mallela
2021-05-18 16:06 ` Peter Maydell
2021-05-18 13:41 ` [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Peter Maydell
` (2 subsequent siblings)
10 siblings, 1 reply; 25+ messages in thread
From: Shashi Mallela @ 2021-04-29 23:42 UTC (permalink / raw)
To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel
Included creation of ITS as part of virt platform GIC
initialization.This Emulated ITS model now co-exists with kvm
ITS and is enabled in absence of kvm irq kernel support in a
platform.
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
hw/arm/virt.c | 27 +++++++++++++++++++++++++--
include/hw/arm/virt.h | 2 ++
target/arm/kvm_arm.h | 4 ++--
3 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9f01d9041b..8f581747bc 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -585,6 +585,12 @@ static void create_its(VirtMachineState *vms)
const char *itsclass = its_class_name();
DeviceState *dev;
+ if (!strcmp(itsclass, "arm-gicv3-its")) {
+ if (!vms->tcg_its) {
+ itsclass = NULL;
+ }
+ }
+
if (!itsclass) {
/* Do nothing if not supported */
return;
@@ -622,7 +628,7 @@ static void create_v2m(VirtMachineState *vms)
vms->msi_controller = VIRT_MSI_CTRL_GICV2M;
}
-static void create_gic(VirtMachineState *vms)
+static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
{
MachineState *ms = MACHINE(vms);
/* We create a standalone GIC */
@@ -656,6 +662,14 @@ static void create_gic(VirtMachineState *vms)
nb_redist_regions);
qdev_prop_set_uint32(vms->gic, "redist-region-count[0]", redist0_count);
+ if (!kvm_irqchip_in_kernel()) {
+ if (vms->tcg_its) {
+ object_property_set_link(OBJECT(vms->gic), "sysmem",
+ OBJECT(mem), &error_fatal);
+ qdev_prop_set_bit(vms->gic, "has-lpi", true);
+ }
+ }
+
if (nb_redist_regions == 2) {
uint32_t redist1_capacity =
vms->memmap[VIRT_HIGH_GIC_REDIST2].size / GICV3_REDIST_SIZE;
@@ -2039,7 +2053,7 @@ static void machvirt_init(MachineState *machine)
virt_flash_fdt(vms, sysmem, secure_sysmem ?: sysmem);
- create_gic(vms);
+ create_gic(vms, sysmem);
virt_cpu_post_init(vms, sysmem);
@@ -2718,6 +2732,12 @@ static void virt_instance_init(Object *obj)
} else {
/* Default allows ITS instantiation */
vms->its = true;
+
+ if (vmc->no_tcg_its) {
+ vms->tcg_its = false;
+ } else {
+ vms->tcg_its = true;
+ }
}
/* Default disallows iommu instantiation */
@@ -2759,6 +2779,9 @@ type_init(machvirt_machine_init);
static void virt_machine_6_0_options(MachineClass *mc)
{
+ VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+ /* qemu ITS was introduced with 6.1 */
+ vmc->no_tcg_its = true;
}
DEFINE_VIRT_MACHINE_AS_LATEST(6, 0)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 921416f918..f873ab9068 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -120,6 +120,7 @@ struct VirtMachineClass {
MachineClass parent;
bool disallow_affinity_adjustment;
bool no_its;
+ bool no_tcg_its;
bool no_pmu;
bool claim_edge_triggered_timers;
bool smbios_old_sys_ver;
@@ -141,6 +142,7 @@ struct VirtMachineState {
bool highmem;
bool highmem_ecam;
bool its;
+ bool tcg_its;
bool virt;
bool ras;
bool mte;
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 34f8daa377..0613454975 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -525,8 +525,8 @@ static inline const char *its_class_name(void)
/* KVM implementation requires this capability */
return kvm_direct_msi_enabled() ? "arm-its-kvm" : NULL;
} else {
- /* Software emulation is not implemented yet */
- return NULL;
+ /* Software emulation based model */
+ return "arm-gicv3-its";
}
}
--
2.27.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v3 0/8] GICv3 LPI and ITS feature implementation
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (7 preceding siblings ...)
2021-04-29 23:42 ` [PATCH v3 8/8] hw/arm/virt: add ITS support in virt GIC Shashi Mallela
@ 2021-05-18 13:41 ` Peter Maydell
2021-05-18 14:46 ` Peter Maydell
2021-05-25 18:26 ` Alex Bennée
10 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 13:41 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> This patchset implements qemu device model for enabling physical
> LPI support and ITS functionality in GIC as per GICv3 specification.
> Both flat table and 2 level tables are implemented.The ITS commands
> for adding/deleting ITS table entries,trigerring LPI interrupts are
> implemented.Translated LPI interrupt ids are processed by redistributor
> to determine priority and set pending state appropriately before
> forwarding the same to cpu interface.
> The ITS feature support has been added to sbsa-ref platform as well as
> virt platform,wherein the emulated functionality co-exists with kvm
> kernel functionality.
>
> Changes in v3:
> - review comments addressed
>
> Shashi Mallela (8):
> hw/intc: GICv3 ITS initial framework
> hw/intc: GICv3 ITS register definitions added
> hw/intc: GICv3 ITS command queue framework
> hw/intc: GICv3 ITS Command processing
> hw/intc: GICv3 ITS Feature enablement
> hw/intc: GICv3 redistributor ITS processing
> hw/arm/sbsa-ref: add ITS support in SBSA GIC
> hw/arm/virt: add ITS support in virt GIC
This fails to build with clang, which has spotted a
missing set of brackets in two places:
../../hw/intc/arm_gicv3_redist.c:568:10: error: logical not is only
applied to the left hand side of this bitwise operator
[-Werror,-Wlogical-not-parentheses]
if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
^ ~
../../hw/intc/arm_gicv3_redist.c:568:10: note: add parentheses after
the '!' to evaluate the bitwise operator first
if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
^
( )
../../hw/intc/arm_gicv3_redist.c:568:10: note: add parentheses around
left hand side expression to silence this warning
if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
^
( )
../../hw/intc/arm_gicv3_redist.c:657:10: error: logical not is only
applied to the left hand side of this bitwise operator
[-Werror,-Wlogical-not-parentheses]
if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
^ ~
../../hw/intc/arm_gicv3_redist.c:657:10: note: add parentheses after
the '!' to evaluate the bitwise operator first
if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
^
( )
../../hw/intc/arm_gicv3_redist.c:657:10: note: add parentheses around
left hand side expression to silence this warning
if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
^
( )
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework
2021-04-29 23:41 ` [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework Shashi Mallela
@ 2021-05-18 13:52 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 13:52 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Added register definitions relevant to ITS,implemented overall
> ITS device framework with stubs for ITS control and translater
> regions read/write,extended ITS common to handle mmio init between
> existing kvm device and newer qemu device.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 239 +++++++++++++++++++++++++
> hw/intc/arm_gicv3_its_common.c | 11 +-
> hw/intc/arm_gicv3_its_kvm.c | 2 +-
> hw/intc/gicv3_internal.h | 88 +++++++--
> hw/intc/meson.build | 1 +
> include/hw/intc/arm_gicv3_its_common.h | 10 +-
> 6 files changed, 331 insertions(+), 20 deletions(-)
> create mode 100644 hw/intc/arm_gicv3_its.c
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> new file mode 100644
> index 0000000000..7b11330e01
> --- /dev/null
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -0,0 +1,239 @@
> +/*
> + * ITS emulation for a GICv3-based system
> + *
> + * Copyright Linaro.org 2021
> + *
> + * Authors:
> + * Shashi Mallela <shashi.mallela@linaro.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
> + * option) any later version. See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/intc/arm_gicv3_its_common.h"
> +#include "gicv3_internal.h"
> +#include "qom/object.h"
> +
> +typedef struct GICv3ITSClass GICv3ITSClass;
> +/* This is reusing the GICv3ITSState typedef from ARM_GICV3_ITS_COMMON */
> +DECLARE_OBJ_CHECKERS(GICv3ITSState, GICv3ITSClass,
> + ARM_GICV3_ITS, TYPE_ARM_GICV3_ITS)
> +
> +struct GICv3ITSClass {
> + GICv3ITSCommonClass parent_class;
> + void (*parent_reset)(DeviceState *dev);
> +};
> +
> +static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
> + uint64_t data, unsigned size, MemTxAttrs attrs)
Your indentation on function prototypes is still messed up; please fix it.
> + if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
> + /* set the ITS default features supported */
> + s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL,
> + GITS_TYPE_PHYSICAL);
More odd indentation. Second lines of function calls etc should line
up with the first character after the '(' on the first line.
Please fix this through the whole patchset.
> +type_init(gicv3_its_register_types)
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index 66c4c6a188..b4dddb16b8 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -50,12 +50,13 @@ static int gicv3_its_post_load(void *opaque, int version_id)
>
> static const VMStateDescription vmstate_its = {
> .name = "arm_gicv3_its",
> + .version_id = 1,
> + .minimum_version_id = 1,
> .pre_save = gicv3_its_pre_save,
> .post_load = gicv3_its_post_load,
> .priority = MIG_PRI_GICV3_ITS,
> .fields = (VMStateField[]) {
> VMSTATE_UINT32(ctlr, GICv3ITSState),
> - VMSTATE_UINT32(iidr, GICv3ITSState),
> VMSTATE_UINT64(cbaser, GICv3ITSState),
> VMSTATE_UINT64(cwriter, GICv3ITSState),
> VMSTATE_UINT64(creadr, GICv3ITSState),
You can't change the vmstate like this, you break migration compatibility.
Why are you deleting the 'iidr' field anyway ?
If we do need to remove the 'iidr', there is a mechanism for saying
"ignore the UINT64 we get from a migration source, we don't need it".
If we need to do that it should be a standalone patch at the start of
the series.
> @@ -51,7 +56,7 @@ struct GICv3ITSState {
>
> /* Registers */
> uint32_t ctlr;
> - uint32_t iidr;
> + uint64_t typer;
> uint64_t cbaser;
> uint64_t cwriter;
> uint64_t creadr;
This will break compilation of the KVM support code on aarch64 hosts,
which is still using the 'iidr' field.
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/8] hw/intc: GICv3 ITS register definitions added
2021-04-29 23:41 ` [PATCH v3 2/8] hw/intc: GICv3 ITS register definitions added Shashi Mallela
@ 2021-05-18 14:27 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 14:27 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Defined descriptors for ITS device table,collection table and ITS
> command queue entities.Implemented register read/write functions,
> extract ITS table parameters and command queue parameters,extended
> gicv3 common to capture qemu address space(which host the ITS table
> platform memories required for subsequent ITS processing) and
> initialize the same in ITS device.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 333 +++++++++++++++++++++++++
> hw/intc/gicv3_internal.h | 23 ++
> include/hw/intc/arm_gicv3_common.h | 3 +
> include/hw/intc/arm_gicv3_its_common.h | 30 +++
> 4 files changed, 389 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index 7b11330e01..a7ccb38a89 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -28,6 +28,162 @@ struct GICv3ITSClass {
> void (*parent_reset)(DeviceState *dev);
> };
>
> +static bool extract_table_params(GICv3ITSState *s)
> +{
> + bool result = true;
> + uint16_t num_pages = 0;
> + uint8_t page_sz_type;
> + uint8_t type;
> + uint32_t page_sz = 0;
> + uint64_t value;
> +
> + for (int i = 0; i < 8; i++) {
> + value = s->baser[i];
> +
> + if (!value) {
> + continue;
> + }
> +
> + page_sz_type = FIELD_EX64(value, GITS_BASER, PAGESIZE);
> +
> + switch (page_sz_type) {
> + case 0:
> + page_sz = GITS_ITT_PAGE_SIZE_0;
> + break;
> +
> + case 1:
> + page_sz = GITS_ITT_PAGE_SIZE_1;
> + break;
> +
> + case 2:
> + case 3:
> + page_sz = GITS_ITT_PAGE_SIZE_2;
> + break;
> +
> + default:
> + result = false;
> + break;
g_assert_unreachable(), because the field is 2 bits and can't have
any values that we don't handle explicitly.
> + }
> +
> + if (result) {
This can never be false, so no point checking it.
(Plus it reduces the level of indentation and makes the rest of
the function less cramped against the right margin.)
> + num_pages = FIELD_EX64(value, GITS_BASER, SIZE);
> +
> + type = FIELD_EX64(value, GITS_BASER, TYPE);
> +
> + switch (type) {
> +
> + case GITS_ITT_TYPE_DEVICE:
> + memset(&s->dt, 0 , sizeof(s->dt));
> + s->dt.valid = FIELD_EX64(value, GITS_BASER, VALID);
> +
> + if (s->dt.valid) {
> + s->dt.page_sz = page_sz;
> + s->dt.indirect = FIELD_EX64(value, GITS_BASER, INDIRECT);
> + s->dt.entry_sz = FIELD_EX64(value, GITS_BASER, ENTRYSIZE);
> +
> + if (!s->dt.indirect) {
> + s->dt.max_entries = ((num_pages + 1) * page_sz) /
> + s->dt.entry_sz;
> + } else {
> + s->dt.max_entries = ((((num_pages + 1) * page_sz) /
> + L1TABLE_ENTRY_SIZE) *
> + (page_sz / s->dt.entry_sz));
> + }
> +
> + s->dt.max_devids = (1UL << (FIELD_EX64(s->typer, GITS_TYPER,
> + DEVBITS) + 1));
> +
> + if ((page_sz == GITS_ITT_PAGE_SIZE_0) ||
> + (page_sz == GITS_ITT_PAGE_SIZE_1)) {
Use a "switch (page_sz)", not an if-ladder.
> + s->dt.base_addr = FIELD_EX64(value, GITS_BASER,
> + PHYADDR);
> + s->dt.base_addr <<= R_GITS_BASER_PHYADDR_SHIFT;
These two lines are more clearly written
s->dt.base_addr = value & R_GITS_BASER_PHYADDR_MASK;
I think.
> + } else if (page_sz == GITS_ITT_PAGE_SIZE_2) {
> + s->dt.base_addr = FIELD_EX64(value, GITS_BASER,
> + PHYADDRL_64K) <<
> + R_GITS_BASER_PHYADDRL_64K_SHIFT;
> + s->dt.base_addr |= ((value >>
> + R_GITS_BASER_PHYADDR_SHIFT) &
> + R_GITS_BASER_PHYADDRH_64K_MASK) <<
> + R_GITS_BASER_PHYADDRH_64K_SHIFT;
Similarly:
s->dt.base_addr = value & R_GITS_BASER_PHYADDRL_MASK;
s->dt.base_addr |= FIELD_EX64(value,
GITS_BASER, PHYADDRH_64K) << 48;
You'll also need to fix the error in the definition of PHYADDRH_64K in the
previous patch:
FIELD(GITS_BASER, PHYADDRH_64K, 48, 4)
as the high part of the address is in bits [15:12] of the register, not [52:48].
Calculating the base_addr from the value given the page_sz is a moderately
complicated bit of code that's duplicated here in both parts of this
function -- you should factor it out into its own function, something
like "uint64_t baser_base_addr(unit64_t value, uint32_t page_sz)".
> + }
> + }
> + break;
> +
> + case GITS_ITT_TYPE_COLLECTION:
> + memset(&s->ct, 0 , sizeof(s->ct));
> + s->ct.valid = FIELD_EX64(value, GITS_BASER, VALID);
> +
> + /*
> + * GITS_TYPER.HCC is 0 for this implementation
> + * hence writes are discarded if ct.valid is 0
> + */
> + if (s->ct.valid) {
> + s->ct.page_sz = page_sz;
> + s->ct.indirect = FIELD_EX64(value, GITS_BASER, INDIRECT);
> + s->ct.entry_sz = FIELD_EX64(value, GITS_BASER, ENTRYSIZE);
> +
> + if (!s->ct.indirect) {
> + s->ct.max_entries = ((num_pages + 1) * page_sz) /
> + s->ct.entry_sz;
> + } else {
> + s->ct.max_entries = ((((num_pages + 1) * page_sz) /
> + L1TABLE_ENTRY_SIZE) *
> + (page_sz / s->ct.entry_sz));
> + }
> +
> + if (FIELD_EX64(s->typer, GITS_TYPER, CIL)) {
> + s->ct.max_collids = (1UL << (FIELD_EX64(s->typer,
> + GITS_TYPER, CIDBITS) + 1));
> + } else {
> + /* 16-bit CollectionId supported when CIL == 0 */
> + s->ct.max_collids = (1UL << 16);
> + }
> +
> + if ((page_sz == GITS_ITT_PAGE_SIZE_0) ||
> + (page_sz == GITS_ITT_PAGE_SIZE_1)) {
> + s->ct.base_addr = FIELD_EX64(value, GITS_BASER,
> + PHYADDR);
> + s->ct.base_addr <<= R_GITS_BASER_PHYADDR_SHIFT;
> + } else if (page_sz == GITS_ITT_PAGE_SIZE_2) {
> + s->ct.base_addr = FIELD_EX64(value, GITS_BASER,
> + PHYADDRL_64K) <<
> + R_GITS_BASER_PHYADDRL_64K_SHIFT;
> + s->ct.base_addr |= ((value >>
> + R_GITS_BASER_PHYADDR_SHIFT) &
> + R_GITS_BASER_PHYADDRH_64K_MASK) <<
> + R_GITS_BASER_PHYADDRH_64K_SHIFT;
> + }
> + }
> + break;
> +
> + default:
> + break;
> + }
> + }
> + }
> + return result;
This is always true, so there's no point having the function return
a bool; make it 'void'.
> +}
> +
> +static void extract_cmdq_params(GICv3ITSState *s)
> +{
> + uint16_t num_pages = 0;
> + uint64_t value = s->cbaser;
> +
> + num_pages = FIELD_EX64(value, GITS_CBASER, SIZE);
> +
> + memset(&s->cq, 0 , sizeof(s->cq));
> + s->cq.valid = FIELD_EX64(value, GITS_CBASER, VALID);
> +
> + if (s->cq.valid) {
> + s->cq.max_entries = ((num_pages + 1) * GITS_ITT_PAGE_SIZE_0) /
> + GITS_CMDQ_ENTRY_SIZE;
> + s->cq.base_addr = FIELD_EX64(value, GITS_CBASER, PHYADDR);
> + s->cq.base_addr <<= R_GITS_CBASER_PHYADDR_SHIFT;
> + }
> + return;
Don't write code like
if (something) {
lots of processing;
}
return;
-- it hides the fact that the "!something" case is just "early exit" and
means the common-case path is indented by an extra level and more likely
to bump up against the right margin. Prefer
if (!something) {
return;
}
lots of processing;
(You can do the same in the previous function with
if (!s->dt.valid) {
return;
}
etc, again improving the indentation situation, and in the code below that's
checking ITS_CTLR_ENABLED, though there with 'break' rather than 'return'.)
Also, you don't need to have an explicit "return" at the end of a void function.
> +}
> +
> static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
> uint64_t data, unsigned size, MemTxAttrs attrs)
> {
> @@ -40,7 +196,70 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,
> uint64_t value, MemTxAttrs attrs)
> {
> MemTxResult result = MEMTX_OK;
> + int index;
> + uint64_t temp = 0;
>
> + switch (offset) {
> + case GITS_CTLR:
> + s->ctlr |= (value & ~(s->ctlr));
> +
> + if (s->ctlr & ITS_CTLR_ENABLED) {
> + if (!extract_table_params(s)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: error extracting GITS_BASER parameters "
> + TARGET_FMT_plx "\n", __func__, offset);
> + } else {
> + extract_cmdq_params(s);
> + s->creadr = 0;
> + }
> + }
> + break;
> + case GITS_CBASER:
> + /*
> + * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
> + * already enabled
> + */
> + if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> + s->cbaser = deposit64(s->cbaser, 0, 32, value);
> + s->creadr = 0;
> + }
> + break;
> + case GITS_CBASER + 4:
> + /*
> + * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
> + * already enabled
> + */
> + if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> + s->cbaser = deposit64(s->cbaser, 32, 32, value);
> + }
> + break;
> + case GITS_CWRITER:
> + s->cwriter = deposit64(s->cwriter, 0, 32, value);
> + break;
> + case GITS_CWRITER + 4:
> + s->cwriter = deposit64(s->cwriter, 32, 32, value);
> + break;
> + case GITS_BASER ... GITS_BASER + 0x3f:
> + /*
> + * IMPDEF choice:- GITS_BASERn register becomes RO if ITS is
> + * already enabled
> + */
> + if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> + index = (offset - GITS_BASER) / 8;
> +
> + if (offset & 7) {
> + temp = s->baser[index];
> + temp = deposit64(temp, 32, 32, (value & ~GITS_BASER_VAL_MASK));
> + s->baser[index] |= temp;
You don't need to use a temp here.
> + } else {
> + s->baser[index] = deposit64(s->baser[index], 0, 32, value);
> + }
GITS_BASER_VAL_MASK is the mask of valid bits for the whole 64-bit
register, so for this 32-bit write code you need to mask value
with the appropriate 32-bit half of it (and there should be masking
code in both the "low half" and "high half" if() branches).
> + }
> + break;
> + default:
> + result = MEMTX_ERROR;
> + break;
> + }
> return result;
> }
>
> @@ -48,7 +267,54 @@ static MemTxResult its_readl(GICv3ITSState *s, hwaddr offset,
> uint64_t *data, MemTxAttrs attrs)
> {
> MemTxResult result = MEMTX_OK;
> + int index;
>
> + switch (offset) {
> + case GITS_CTLR:
> + *data = s->ctlr;
> + break;
> + case GITS_IIDR:
> + *data = gicv3_iidr();
> + break;
> + case GITS_PIDR2:
> + *data = gicv3_idreg(offset - GITS_PIDR2);
> + break;
(We discussed PIDR2 in another thread.)
> + case GITS_TYPER:
> + *data = extract64(s->typer, 0, 32);
> + break;
> + case GITS_TYPER + 4:
> + *data = extract64(s->typer, 32, 32);
> + break;
> + case GITS_CBASER:
> + *data = extract64(s->cbaser, 0, 32);
> + break;
> + case GITS_CBASER + 4:
> + *data = extract64(s->cbaser, 32, 32);
> + break;
> + case GITS_CREADR:
> + *data = extract64(s->creadr, 0, 32);
> + break;
> + case GITS_CREADR + 4:
> + *data = extract64(s->creadr, 32, 32);
> + break;
> + case GITS_CWRITER:
> + *data = extract64(s->cwriter, 0, 32);
> + break;
> + case GITS_CWRITER + 4:
> + *data = extract64(s->cwriter, 32, 32);
> + break;
> + case GITS_BASER ... GITS_BASER + 0x3f:
> + index = (offset - GITS_BASER) / 8;
> + if (offset & 7) {
> + *data = s->baser[index] >> 32;
> + } else {
> + *data = (uint32_t)s->baser[index];
Use extract64() for GITS_BASER please, just for consistency with the
other registers.
> + }
> + break;
> + default:
> + result = MEMTX_ERROR;
> + break;
> + }
> return result;
> }
>
> @@ -56,7 +322,35 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,
> uint64_t value, MemTxAttrs attrs)
> {
> MemTxResult result = MEMTX_OK;
> + int index;
>
> + switch (offset) {
> + case GITS_BASER ... GITS_BASER + 0x3f:
> + /*
> + * IMPDEF choice:- GITS_BASERn register becomes RO if ITS is
> + * already enabled
> + */
> + if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> + index = (offset - GITS_BASER) / 8;
> + s->baser[index] |= (value & ~GITS_BASER_VAL_MASK);
> + }
> + break;
> + case GITS_CBASER:
> + /*
> + * IMPDEF choice:- GITS_CBASER register becomes RO if ITS is
> + * already enabled
> + */
> + if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> + s->cbaser = value;
> + }
> + break;
> + case GITS_CWRITER:
> + s->cwriter = value;
> + break;
> + default:
> + result = MEMTX_ERROR;
> + break;
> + }
> return result;
> }
>
> @@ -64,7 +358,29 @@ static MemTxResult its_readll(GICv3ITSState *s, hwaddr offset,
> uint64_t *data, MemTxAttrs attrs)
> {
> MemTxResult result = MEMTX_OK;
> + int index;
>
> + switch (offset) {
> + case GITS_TYPER:
> + *data = s->typer;
> + break;
> + case GITS_BASER ... GITS_BASER + 0x3f:
> + index = (offset - GITS_BASER) / 8;
> + *data = s->baser[index];
> + break;
> + case GITS_CBASER:
> + *data = s->cbaser;
> + break;
> + case GITS_CREADR:
> + *data = s->creadr;
> + break;
> + case GITS_CWRITER:
> + *data = s->cwriter;
> + break;
> + default:
> + result = MEMTX_ERROR;
> + break;
> + }
> return result;
> }
>
> @@ -161,6 +477,9 @@ static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)
> gicv3_its_init_mmio(s, &gicv3_its_control_ops, &gicv3_its_translation_ops);
>
> if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
> + address_space_init(&s->gicv3->dma_as, s->gicv3->dma,
> + "gicv3-its-sysmem");
"gicv3-its-dma"
> +
> /* set the ITS default features supported */
> s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL,
> GITS_TYPE_PHYSICAL);
> @@ -207,6 +526,18 @@ static void gicv3_its_reset(DeviceState *dev)
> }
> }
>
> +static void gicv3_its_post_load(GICv3ITSState *s)
> +{
> + if (s->ctlr & ITS_CTLR_ENABLED) {
> + if (!extract_table_params(s)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: error extracting GITS_BASER parameters\n", __func__);
> + } else {
> + extract_cmdq_params(s);
> + }
The correct response to "incoming migration data is bad" is not
to log a guest error but to fail the post-load function.
Luckily we get to avoid having to fix up the base class to allow
that, because in fact extract_table_params() can never fail.
> + }
> +}
> +
> static Property gicv3_its_props[] = {
> DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "arm-gicv3",
> GICv3State *),
> @@ -217,10 +548,12 @@ static void gicv3_its_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> GICv3ITSClass *ic = ARM_GICV3_ITS_CLASS(klass);
> + GICv3ITSCommonClass *icc = ARM_GICV3_ITS_COMMON_CLASS(klass);
>
> dc->realize = gicv3_arm_its_realize;
> device_class_set_props(dc, gicv3_its_props);
> device_class_set_parent_reset(dc, gicv3_its_reset, &ic->parent_reset);
> + icc->post_load = gicv3_its_post_load;
> }
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 0/8] GICv3 LPI and ITS feature implementation
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (8 preceding siblings ...)
2021-05-18 13:41 ` [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Peter Maydell
@ 2021-05-18 14:46 ` Peter Maydell
2021-06-02 17:55 ` Shashi Mallela
2021-05-25 18:26 ` Alex Bennée
10 siblings, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 14:46 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> This patchset implements qemu device model for enabling physical
> LPI support and ITS functionality in GIC as per GICv3 specification.
> Both flat table and 2 level tables are implemented.The ITS commands
> for adding/deleting ITS table entries,trigerring LPI interrupts are
> implemented.Translated LPI interrupt ids are processed by redistributor
> to determine priority and set pending state appropriately before
> forwarding the same to cpu interface.
> The ITS feature support has been added to sbsa-ref platform as well as
> virt platform,wherein the emulated functionality co-exists with kvm
> kernel functionality.
>
> Changes in v3:
> - review comments addressed
>
> Shashi Mallela (8):
> hw/intc: GICv3 ITS initial framework
> hw/intc: GICv3 ITS register definitions added
> hw/intc: GICv3 ITS command queue framework
> hw/intc: GICv3 ITS Command processing
> hw/intc: GICv3 ITS Feature enablement
> hw/intc: GICv3 redistributor ITS processing
> hw/arm/sbsa-ref: add ITS support in SBSA GIC
> hw/arm/virt: add ITS support in virt GIC
Something in here breaks "make check":
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-from-laptop/qemu/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-aarch64 tests/qtest/bios-tables-test
--tap -k
Looking for expected file 'tests/data/acpi/virt/FACP'
Using expected file 'tests/data/acpi/virt/FACP'
Looking for expected file 'tests/data/acpi/virt/APIC'
Using expected file 'tests/data/acpi/virt/APIC'
Looking for expected file 'tests/data/acpi/virt/GTDT'
Using expected file 'tests/data/acpi/virt/GTDT'
Looking for expected file 'tests/data/acpi/virt/MCFG'
Using expected file 'tests/data/acpi/virt/MCFG'
Looking for expected file 'tests/data/acpi/virt/SPCR'
Using expected file 'tests/data/acpi/virt/SPCR'
Looking for expected file 'tests/data/acpi/virt/IORT'
**
ERROR:../../tests/qtest/bios-tables-test.c:385:load_expected_aml:
assertion failed: (exp_sdt.aml_file)
ERROR qtest-aarch64/bios-tables-test - Bail out!
ERROR:../../tests/qtest/bios-tables-test.c:385:load_expected_aml:
assertion failed: (exp_sdt.aml_file)
(and then it hangs)
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 3/8] hw/intc: GICv3 ITS command queue framework
2021-04-29 23:41 ` [PATCH v3 3/8] hw/intc: GICv3 ITS command queue framework Shashi Mallela
@ 2021-05-18 15:43 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 15:43 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Added functionality to trigger ITS command queue processing on
> write to CWRITE register and process each command queue entry to
> identify the command type and handle commands like MAPD,MAPC,SYNC.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 327 +++++++++++++++++++++++++++++++++++++++
> hw/intc/gicv3_internal.h | 41 +++++
> 2 files changed, 368 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index a7ccb38a89..7cb465813a 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -28,6 +28,327 @@ struct GICv3ITSClass {
> void (*parent_reset)(DeviceState *dev);
> };
>
> +static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint64_t rdbase;
> + uint64_t value;
> + MemTxResult res = MEMTX_OK;
> +
> + offset += NUM_BYTES_IN_DW;
> + offset += NUM_BYTES_IN_DW;
> +
> + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> + MEMTXATTRS_UNSPECIFIED, &res);
> +
> + rdbase = (value >> RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
> +
> + if (rdbase < (s->gicv3->num_cpu)) {
> + /*
> + * Current implementation makes a blocking synchronous call
> + * for every command issued earlier,hence the internal state
> + * is already consistent by the time SYNC command is executed.
> + */
> + }
If we don't need to do anything for SYNC, then just don't do anything.
You don't need to read the word, extract rdbase and then have an
empty if(). You could just have the 'case GITS_CMD_SYNC' in
process_cmdq() be a brief explanatory comment and then 'break'.
> + offset += NUM_BYTES_IN_DW;
> + return res;
> +}
> +
> +static MemTxResult update_cte(GICv3ITSState *s, uint16_t icid, bool valid,
> + uint64_t rdbase)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint64_t value;
> + uint64_t l2t_addr;
> + bool valid_l2t;
> + uint32_t l2t_id;
> + uint32_t max_l2_entries;
> + uint64_t cte = 0;
> + MemTxResult res = MEMTX_OK;
> +
> + if (s->ct.valid) {
> + if (valid) {
> + /* add mapping entry to collection table */
> + cte = (valid & VALID_MASK) |
> + ((rdbase & RDBASE_PROCNUM_MASK) << 1ULL);
> + }
> + } else {
> + return res;
> + }
Another case where writing
if (!s->ct.valid) {
return res;
}
[normal case here]
would be clearer.
> +
> + /*
> + * The specification defines the format of level 1 entries of a
> + * 2-level table, but the format of level 2 entries and the format
> + * of flat-mapped tables is IMPDEF.
> + */
> + if (s->ct.indirect) {
> + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> +
> + value = address_space_ldq_le(as,
> + s->ct.base_addr +
> + (l2t_id * L1TABLE_ENTRY_SIZE),
> + MEMTXATTRS_UNSPECIFIED, &res);
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> +
> + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
If you're going to hand-define shift and mask constants can
you keep the semantics the same as the ones you get from the
FIELD macros, please? The MASK should be the in-place mask,
not one that's at the bottom of a word.
> +
> + if (valid_l2t) {
> + max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> +
> + l2t_addr = value & ((1ULL << 51) - 1);
> +
> + address_space_stq_le(as, l2t_addr +
> + ((icid % max_l2_entries) * GITS_CTE_SIZE),
> + cte, MEMTXATTRS_UNSPECIFIED, &res);
> + }
> + } else {
> + /* Flat level table */
> + address_space_stq_le(as, s->ct.base_addr + (icid * GITS_CTE_SIZE),
> + cte, MEMTXATTRS_UNSPECIFIED, &res);
> + }
> + return res;
> +}
> +
> +static MemTxResult process_mapc(GICv3ITSState *s, uint32_t offset)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint16_t icid;
> + uint64_t rdbase;
> + bool valid;
> + MemTxResult res = MEMTX_OK;
> + uint64_t value;
> +
> + offset += NUM_BYTES_IN_DW;
> + offset += NUM_BYTES_IN_DW;
> +
> + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> + MEMTXATTRS_UNSPECIFIED, &res);
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> +
> + icid = value & ICID_MASK;
> +
> + rdbase = (value >> RDBASE_SHIFT) & RDBASE_PROCNUM_MASK;
> +
> + valid = (value >> VALID_SHIFT) & VALID_MASK;
> +
> + if ((icid > s->ct.max_collids) || (rdbase > s->gicv3->num_cpu)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "ITS MAPC: invalid collection table attributes "
> + "icid %d rdbase %lu\n", icid, rdbase);
> + /*
> + * in this implementation,in case of error
> + * we ignore this command and move onto the next
> + * command in the queue
> + */
> + } else {
> + res = update_cte(s, icid, valid, rdbase);
> + }
> +
> + offset += NUM_BYTES_IN_DW;
> + offset += NUM_BYTES_IN_DW;
What are these increments for? The value is dead at this point.
> +
> + return res;
> +}
> +
> +static MemTxResult update_dte(GICv3ITSState *s, uint32_t devid, bool valid,
> + uint8_t size, uint64_t itt_addr)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint64_t value;
> + uint64_t l2t_addr;
> + bool valid_l2t;
> + uint32_t l2t_id;
> + uint32_t max_l2_entries;
> + uint64_t dte = 0;
> + MemTxResult res = MEMTX_OK;
> +
> + if (s->dt.valid) {
> + if (valid) {
> + /* add mapping entry to device table */
> + dte = (valid & VALID_MASK) |
> + ((size & SIZE_MASK) << 1U) |
> + ((itt_addr & ITTADDR_MASK) << 6ULL);
> + }
> + } else {
> + return res;
> + }
> +
> + /*
> + * The specification defines the format of level 1 entries of a
> + * 2-level table, but the format of level 2 entries and the format
> + * of flat-mapped tables is IMPDEF.
> + */
> + if (s->dt.indirect) {
> + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE);
> +
> + value = address_space_ldq_le(as,
> + s->dt.base_addr +
> + (l2t_id * L1TABLE_ENTRY_SIZE),
> + MEMTXATTRS_UNSPECIFIED, &res);
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> +
> + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> +
> + if (valid_l2t) {
> + max_l2_entries = s->dt.page_sz / s->dt.entry_sz;
> +
> + l2t_addr = value & ((1ULL << 51) - 1);
> +
> + address_space_stq_le(as, l2t_addr +
> + ((devid % max_l2_entries) * GITS_DTE_SIZE),
> + dte, MEMTXATTRS_UNSPECIFIED, &res);
> + }
> + } else {
> + /* Flat level table */
> + address_space_stq_le(as, s->dt.base_addr + (devid * GITS_DTE_SIZE),
> + dte, MEMTXATTRS_UNSPECIFIED, &res);
> + }
> + return res;
> +}
> +
> +static MemTxResult process_mapd(GICv3ITSState *s, uint64_t value,
> + uint32_t offset)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint32_t devid;
> + uint8_t size;
> + uint64_t itt_addr;
> + bool valid;
> + MemTxResult res = MEMTX_OK;
> +
> + devid = (value >> DEVID_SHIFT) & DEVID_MASK;
> +
> + offset += NUM_BYTES_IN_DW;
> + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> + MEMTXATTRS_UNSPECIFIED, &res);
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> +
> + size = (value & SIZE_MASK);
> +
> + offset += NUM_BYTES_IN_DW;
> + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> + MEMTXATTRS_UNSPECIFIED, &res);
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> +
> + itt_addr = (value >> ITTADDR_SHIFT) & ITTADDR_MASK;
> +
> + valid = (value >> VALID_SHIFT) & VALID_MASK;
> +
> + if ((devid > s->dt.max_devids) ||
> + (size > FIELD_EX64(s->typer, GITS_TYPER, IDBITS))) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "ITS MAPD: invalid device table attributes "
> + "devid %d or size %d\n", devid, size);
> + /*
> + * in this implementation,in case of error
Spaces after commas in text, please. (There's another one of these below.)
> + * we ignore this command and move onto the next
> + * command in the queue
> + */
> + } else {
> + if (res == MEMTX_OK) {
This condition can never be false, because you did an early-return
for the != MEMTX_OK case already.
> + res = update_dte(s, devid, valid, size, itt_addr);
> + }
> + }
> +
> + offset += NUM_BYTES_IN_DW;
> + offset += NUM_BYTES_IN_DW;
> +
> + return res;
> +}
> +
> +/*
> + * Current implementation blocks until all
> + * commands are processed
> + */
> +static MemTxResult process_cmdq(GICv3ITSState *s)
> +{
> + uint32_t wr_offset = 0;
> + uint32_t rd_offset = 0;
> + uint32_t cq_offset = 0;
> + uint64_t data;
> + AddressSpace *as = &s->gicv3->dma_as;
> + MemTxResult res = MEMTX_OK;
> + uint8_t cmd;
> +
> + if (!(s->ctlr & ITS_CTLR_ENABLED)) {
> + return res;
> + }
> +
> + wr_offset = FIELD_EX64(s->cwriter, GITS_CWRITER, OFFSET);
> +
> + if (wr_offset > s->cq.max_entries) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: invalid write offset "
> + "%d\n", __func__, wr_offset);
> + res = MEMTX_ERROR;
> + return res;
> + }
> +
> + rd_offset = FIELD_EX64(s->creadr, GITS_CREADR, OFFSET);
> +
> + while (wr_offset != rd_offset) {
> + cq_offset = (rd_offset * GITS_CMDQ_ENTRY_SIZE);
> + data = address_space_ldq_le(as, s->cq.base_addr + cq_offset,
> + MEMTXATTRS_UNSPECIFIED, &res);
> + cmd = (data & CMD_MASK);
> +
> + switch (cmd) {
> + case GITS_CMD_INT:
> + break;
> + case GITS_CMD_CLEAR:
> + break;
> + case GITS_CMD_SYNC:
> + res = process_sync(s, cq_offset);
> + break;
> + case GITS_CMD_MAPD:
> + res = process_mapd(s, data, cq_offset);
> + break;
> + case GITS_CMD_MAPC:
> + res = process_mapc(s, cq_offset);
> + break;
> + case GITS_CMD_MAPTI:
> + break;
> + case GITS_CMD_MAPI:
> + break;
> + case GITS_CMD_DISCARD:
> + break;
> + default:
> + break;
> + }
> + if (res == MEMTX_OK) {
> + rd_offset++;
> + rd_offset %= s->cq.max_entries;
> + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, OFFSET, rd_offset);
> + } else {
> + /*
> + * in this implementation,in case of dma read/write error
> + * we stall the command processing
> + */
> + s->creadr = FIELD_DP64(s->creadr, GITS_CREADR, STALLED, 1);
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: %x cmd processing failed!!\n", __func__, cmd);
> + break;
> + }
> + }
> + return res;
You can't just return the MEMTX_* value from process_cmdq() to
the caller, because that will cause an external abort for the
guest's write to CWRITER. You need to handle errors inside
process_cmdq() using one of the ways that the spec permits
for handling command errors. process_cmdq() itself should return
void.
> +}
> +
> static bool extract_table_params(GICv3ITSState *s)
> {
> bool result = true;
> @@ -235,6 +556,9 @@ static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,
> break;
> case GITS_CWRITER:
> s->cwriter = deposit64(s->cwriter, 0, 32, value);
> + if (s->cwriter != s->creadr) {
> + result = process_cmdq(s);
> + }
> break;
> case GITS_CWRITER + 4:
> s->cwriter = deposit64(s->cwriter, 32, 32, value);
> @@ -346,6 +670,9 @@ static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,
> break;
> case GITS_CWRITER:
> s->cwriter = value;
> + if (s->cwriter != s->creadr) {
> + result = process_cmdq(s);
> + }
You need to also allow for the guest writing the Retry bit. Retry always
reads-as-zero and its only purpose is to prod the ITS into retrying a
stalled command queue, so for us we can just mask out the 'retry' bit
on writes. That's probably best done in the earlier patch where we added
the basic CWRITER support, rather than in this one.
> break;
> default:
> result = MEMTX_ERROR;
> diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
> index bfabd5ad62..3b8e1e85c6 100644
> --- a/hw/intc/gicv3_internal.h
> +++ b/hw/intc/gicv3_internal.h
> @@ -253,6 +253,12 @@ FIELD(GITS_CBASER, OUTERCACHE, 53, 3)
> FIELD(GITS_CBASER, INNERCACHE, 59, 3)
> FIELD(GITS_CBASER, VALID, 63, 1)
>
> +FIELD(GITS_CREADR, STALLED, 0, 1)
> +FIELD(GITS_CREADR, OFFSET, 5, 15)
> +
> +FIELD(GITS_CWRITER, RETRY, 0, 1)
> +FIELD(GITS_CWRITER, OFFSET, 5, 15)
> +
> FIELD(GITS_CTLR, ENABLED, 0, 1)
> FIELD(GITS_CTLR, QUIESCENT, 31, 1)
>
> @@ -286,6 +292,41 @@ FIELD(GITS_TYPER, CIL, 36, 1)
> #define L1TABLE_ENTRY_SIZE 8
>
> #define GITS_CMDQ_ENTRY_SIZE 32
> +#define NUM_BYTES_IN_DW 8
> +
> +#define CMD_MASK 0xff
> +
> +/* ITS Commands */
> +#define GITS_CMD_CLEAR 0x04
> +#define GITS_CMD_DISCARD 0x0F
> +#define GITS_CMD_INT 0x03
> +#define GITS_CMD_MAPC 0x09
> +#define GITS_CMD_MAPD 0x08
> +#define GITS_CMD_MAPI 0x0B
> +#define GITS_CMD_MAPTI 0x0A
> +#define GITS_CMD_SYNC 0x05
> +
> +/* MAPC command fields */
> +#define ICID_LENGTH 16
> +#define ICID_MASK ((1U << ICID_LENGTH) - 1)
> +#define RDBASE_LENGTH 32
> +#define RDBASE_SHIFT 16
> +#define RDBASE_MASK ((1ULL << RDBASE_LENGTH) - 1)
I think you'd probably be better off using the FIELD macro
rather than hand-rolling macros to do the same thing.
> +#define RDBASE_PROCNUM_LENGTH 16
> +#define RDBASE_PROCNUM_MASK ((1ULL << RDBASE_PROCNUM_LENGTH) - 1)
> +
> +#define DEVID_SHIFT 32
> +#define DEVID_LENGTH 32
> +#define DEVID_MASK ((1ULL << DEVID_LENGTH) - 1)
> +
> +/* MAPD command fields */
> +#define ITTADDR_LENGTH 44
> +#define ITTADDR_SHIFT 8
> +#define ITTADDR_MASK ((1ULL << ITTADDR_LENGTH) - 1)
> +#define SIZE_MASK 0x1f
> +
> +#define VALID_SHIFT 63
> +#define VALID_MASK 0x1
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 4/8] hw/intc: GICv3 ITS Command processing
2021-04-29 23:41 ` [PATCH v3 4/8] hw/intc: GICv3 ITS Command processing Shashi Mallela
@ 2021-05-18 15:49 ` Peter Maydell
2021-05-25 17:57 ` shashi.mallela
0 siblings, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 15:49 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Added ITS command queue handling for MAPTI,MAPI commands,handled ITS
> translation which triggers an LPI via INT command as well as write
> to GITS_TRANSLATER register,defined enum to differentiate between ITS
> command interrupt trigger and GITS_TRANSLATER based interrupt trigger.
> Each of these commands make use of other functionalities implemented to
> get device table entry,collection table entry or interrupt translation
> table entry required for their processing.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
> hw/intc/arm_gicv3_its.c | 346 ++++++++++++++++++++++++++++-
> hw/intc/gicv3_internal.h | 12 +
> include/hw/intc/arm_gicv3_common.h | 2 +
> 3 files changed, 359 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index 7cb465813a..98c984dd22 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -28,6 +28,156 @@ struct GICv3ITSClass {
> void (*parent_reset)(DeviceState *dev);
> };
>
> +typedef enum ItsCmdType {
> + NONE = 0, /* internal indication for GITS_TRANSLATER write */
> + CLEAR = 1,
> + DISCARD = 2,
> + INT = 3,
> +} ItsCmdType;
> +
> +static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t *cte,
> + MemTxResult *res)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint64_t l2t_addr;
> + uint64_t value;
> + bool valid_l2t;
> + uint32_t l2t_id;
> + uint32_t max_l2_entries;
> + bool status = false;
> +
> + if (s->ct.indirect) {
> + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> +
> + value = address_space_ldq_le(as,
> + s->ct.base_addr +
> + (l2t_id * L1TABLE_ENTRY_SIZE),
> + MEMTXATTRS_UNSPECIFIED, res);
> +
> + if (*res == MEMTX_OK) {
> + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> +
> + if (valid_l2t) {
> + max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> +
> + l2t_addr = value & ((1ULL << 51) - 1);
> +
> + *cte = address_space_ldq_le(as, l2t_addr +
> + ((icid % max_l2_entries) * GITS_CTE_SIZE),
> + MEMTXATTRS_UNSPECIFIED, res);
> + }
> + }
> + } else {
> + /* Flat level table */
> + *cte = address_space_ldq_le(as, s->ct.base_addr +
> + (icid * GITS_CTE_SIZE),
> + MEMTXATTRS_UNSPECIFIED, res);
> + }
> +
> + if (*cte & VALID_MASK) {
> + status = true;
> + }
> +
> + return status;
> +}
> +
> +static MemTxResult update_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
> + uint64_t itel, uint32_t iteh)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint64_t itt_addr;
> + MemTxResult res = MEMTX_OK;
> +
> + itt_addr = (dte >> 6ULL) & ITTADDR_MASK;
> + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
> +
> + address_space_stq_le(as, itt_addr + (eventid * sizeof(uint64_t)),
> + itel, MEMTXATTRS_UNSPECIFIED, &res);
> +
> + if (res == MEMTX_OK) {
> + address_space_stl_le(as, itt_addr + ((eventid + sizeof(uint64_t)) *
> + sizeof(uint32_t)), iteh, MEMTXATTRS_UNSPECIFIED,
> + &res);
> + }
> + return res;
> +}
> +
> +static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t dte,
> + uint16_t *icid, uint32_t *pIntid, MemTxResult *res)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint64_t itt_addr;
> + bool status = false;
> + uint64_t itel = 0;
> + uint32_t iteh = 0;
> +
> + itt_addr = (dte >> 6ULL) & ITTADDR_MASK;
> + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
> +
> + itel = address_space_ldq_le(as, itt_addr + (eventid * sizeof(uint64_t)),
> + MEMTXATTRS_UNSPECIFIED, res);
> +
> + if (*res == MEMTX_OK) {
> + iteh = address_space_ldl_le(as, itt_addr + ((eventid +
> + sizeof(uint64_t)) * sizeof(uint32_t)),
> + MEMTXATTRS_UNSPECIFIED, res);
> +
> + if (*res == MEMTX_OK) {
> + if (itel & VALID_MASK) {
> + if ((itel >> ITE_ENTRY_INTTYPE_SHIFT) & GITS_TYPE_PHYSICAL) {
> + *pIntid = (itel >> ITE_ENTRY_INTID_SHIFT) &
> + ITE_ENTRY_INTID_MASK;
> + *icid = iteh & ITE_ENTRY_ICID_MASK;
> + status = true;
> + }
> + }
> + }
> + }
> + return status;
> +}
> +
> +static uint64_t get_dte(GICv3ITSState *s, uint32_t devid, MemTxResult *res)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint64_t l2t_addr;
> + uint64_t value;
> + bool valid_l2t;
> + uint32_t l2t_id;
> + uint32_t max_l2_entries;
> +
> + if (s->dt.indirect) {
> + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE);
> +
> + value = address_space_ldq_le(as,
> + s->dt.base_addr +
> + (l2t_id * L1TABLE_ENTRY_SIZE),
> + MEMTXATTRS_UNSPECIFIED, res);
> +
> + if (*res == MEMTX_OK) {
> + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> +
> + if (valid_l2t) {
> + max_l2_entries = s->dt.page_sz / s->dt.entry_sz;
> +
> + l2t_addr = value & ((1ULL << 51) - 1);
> +
> + value = 0;
This assignment is pointless because we assign again to value immediately
afterwards.
> + value = address_space_ldq_le(as, l2t_addr +
> + ((devid % max_l2_entries) * GITS_DTE_SIZE),
> + MEMTXATTRS_UNSPECIFIED, res);
> + }
> + }
> + } else {
> + /* Flat level table */
> + value = 0;
Ditto.
> + value = address_space_ldq_le(as, s->dt.base_addr +
> + (devid * GITS_DTE_SIZE),
> + MEMTXATTRS_UNSPECIFIED, res);
> + }
> +
> + return value;
> +}
> +
> static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
> {
> AddressSpace *as = &s->gicv3->dma_as;
> @@ -55,6 +205,182 @@ static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
> return res;
> }
>
> +static MemTxResult process_int(GICv3ITSState *s, uint64_t value,
> + uint32_t offset, ItsCmdType cmd)
> +{
> + AddressSpace *as = &s->gicv3->dma_as;
> + uint32_t devid, eventid;
> + MemTxResult res = MEMTX_OK;
> + bool dte_valid;
> + uint64_t dte = 0;
> + uint32_t max_eventid;
> + uint16_t icid = 0;
> + uint32_t pIntid = 0;
> + bool ite_valid = false;
> + uint64_t cte = 0;
> + bool cte_valid = false;
> + uint64_t itel = 0;
> + uint32_t iteh = 0;
> +
> + if (cmd == NONE) {
> + devid = offset;
> + } else {
> + devid = (value >> DEVID_SHIFT) & DEVID_MASK;
> +
> + offset += NUM_BYTES_IN_DW;
> + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> + MEMTXATTRS_UNSPECIFIED, &res);
> + }
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> +
> + eventid = (value & EVENTID_MASK);
> +
> + dte = get_dte(s, devid, &res);
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> + dte_valid = dte & VALID_MASK;
> +
> + if (dte_valid) {
> + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1));
> +
> + ite_valid = get_ite(s, eventid, dte, &icid, &pIntid, &res);
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> +
> + if (ite_valid) {
> + cte_valid = get_cte(s, icid, &cte, &res);
> + }
> +
> + if (res != MEMTX_OK) {
> + return res;
> + }
> + }
> +
> + if ((devid > s->dt.max_devids) || !dte_valid || !ite_valid ||
> + !cte_valid || (eventid > max_eventid)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: invalid interrupt translation table attributes "
> + "devid %d or eventid %d\n",
> + __func__, devid, eventid);
> + /*
> + * in this implementation,in case of error
> + * we ignore this command and move onto the next
> + * command in the queue
> + */
...but we could just return an error from this function and
get the 'stall' behaviour. It would be more consistent to just
stall for everything, if we're going to be stalling for various
kinds of "failed to read memory" anyway. (Same for some instances
of this in the previous patches.)
> + } else {
> + /*
> + * Current implementation only supports rdbase == procnum
> + * Hence rdbase physical address is ignored
> + */
> + if (cmd == DISCARD) {
> + /* remove mapping from interrupt translation table */
> + res = update_ite(s, eventid, dte, itel, iteh);
> + }
> + }
> +
> + if (cmd != NONE) {
> + offset += NUM_BYTES_IN_DW;
> + offset += NUM_BYTES_IN_DW;
More dead increments.
> + }
> +
> + return res;
> +}
> +
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 5/8] hw/intc: GICv3 ITS Feature enablement
2021-04-29 23:41 ` [PATCH v3 5/8] hw/intc: GICv3 ITS Feature enablement Shashi Mallela
@ 2021-05-18 15:58 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 15:58 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Added properties to enable ITS feature and define qemu system
> address space memory in gicv3 common,setup distributor and
> redistributor registers to indicate LPI support.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
> hw/intc/arm_gicv3_common.c | 13 +++++++++++++
> hw/intc/arm_gicv3_dist.c | 21 +++++++++++++++++++--
> hw/intc/arm_gicv3_redist.c | 30 +++++++++++++++++++++++++-----
> hw/intc/gicv3_internal.h | 17 +++++++++++++++++
> include/hw/intc/arm_gicv3_common.h | 1 +
> 5 files changed, 75 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 58ef65f589..a55e91071a 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -381,6 +381,16 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
> (1 << 24) |
> (i << 8) |
> (last << 4);
> +
> + if (s->lpi_enable) {
> + s->cpu[i].gicr_typer |= GICR_TYPER_PLPIS;
> +
> + if (!s->dma) {
> + error_setg(errp,
> + "Redist-ITS: Guest 'sysmem' reference link not set");
> + return;
> + }
> + }
Can you put the "if (s->lpi_enable && !s->dma)" error-exit further
up in the function with all the other error-checks, please? That way
we do all our error-handling before we start allocating memory and
doing other things.
> }
> }
>
> @@ -494,9 +504,12 @@ static Property arm_gicv3_common_properties[] = {
> DEFINE_PROP_UINT32("num-cpu", GICv3State, num_cpu, 1),
> DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
> DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
> + DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0),
> DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
> DEFINE_PROP_ARRAY("redist-region-count", GICv3State, nb_redist_regions,
> redist_region_count, qdev_prop_uint32, uint32_t),
> + DEFINE_PROP_LINK("sysmem", GICv3State, dma, TYPE_MEMORY_REGION,
> + MemoryRegion *),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
> index b65f56f903..43e0ea4367 100644
> --- a/hw/intc/arm_gicv3_dist.c
> +++ b/hw/intc/arm_gicv3_dist.c
> @@ -366,12 +366,15 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
> return MEMTX_OK;
> case GICD_TYPER:
> {
> + bool lpi_supported = false;
> /* For this implementation:
> * No1N == 1 (1-of-N SPI interrupts not supported)
> * A3V == 1 (non-zero values of Affinity level 3 supported)
> * IDbits == 0xf (we support 16-bit interrupt identifiers)
> * DVIS == 0 (Direct virtual LPI injection not supported)
> - * LPIS == 0 (LPIs not supported)
> + * LPIS == 1 (LPIs are supported if affinity routing is enabled)
> + * num_LPIs == 0b00000 (bits [15:11],Number of LPIs as indicated
> + * by GICD_TYPER.IDbits)
> * MBIS == 0 (message-based SPIs not supported)
> * SecurityExtn == 1 if security extns supported
> * CPUNumber == 0 since for us ARE is always 1
> @@ -385,8 +388,22 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
> */
> bool sec_extn = !(s->gicd_ctlr & GICD_CTLR_DS);
>
> + /*
> + * With securityextn on, LPIs are supported when affinity routing
> + * is enabled for non-secure state and if off LPIs are supported
> + * when affinity routing is enabled.
> + */
> + if (s->lpi_enable) {
> + if (sec_extn) {
> + lpi_supported = (s->gicd_ctlr & GICD_CTLR_ARE_NS);
> + } else {
> + lpi_supported = (s->gicd_ctlr & GICD_CTLR_ARE);
> + }
> + }
For our implementation, affinity routing is always enabled, so you
don't need to make a distinction between s->lpi_enable and
lpi_supported.
> +
> *data = (1 << 25) | (1 << 24) | (sec_extn << 10) |
> - (0xf << 19) | itlinesnumber;
> + (lpi_supported << GICD_TYPER_LPIS_OFFSET) | (GICD_TYPER_IDBITS <<
> + GICD_TYPER_IDBITS_OFFSET) | itlinesnumber;
Don't break the line after << like this, please; it's much easier to read
if you break after '|' instead, because then the (...) bracketed
expression stays on one line rather than being split.
> return MEMTX_OK;
> }
> case GICD_IIDR:
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index 8645220d61..7604ccdc83 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -244,14 +244,22 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr offset,
> static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
> uint64_t value, MemTxAttrs attrs)
> {
> + uint64_t data;
> +
> switch (offset) {
> case GICR_CTLR:
> /* For our implementation, GICR_TYPER.DPGS is 0 and so all
> * the DPG bits are RAZ/WI. We don't do anything asynchronously,
> - * so UWP and RWP are RAZ/WI. And GICR_TYPER.LPIS is 0 (we don't
> - * implement LPIs) so Enable_LPIs is RES0. So there are no writable
> - * bits for us.
> + * so UWP and RWP are RAZ/WI. GICR_TYPER.LPIS is 1 (we
> + * implement LPIs) so Enable_LPIs is programmable.
> */
> + if (cs->gicr_typer & GICR_TYPER_PLPIS) {
> + if (value & GICR_CTLR_ENABLE_LPIS) {
> + cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
> + } else {
> + cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
> + }
> + }
> return MEMTX_OK;
> case GICR_STATUSR:
> /* RAZ/WI for our implementation */
> @@ -275,7 +283,12 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
> cs->gicr_waker = value;
> return MEMTX_OK;
> case GICR_PROPBASER:
> - cs->gicr_propbaser = deposit64(cs->gicr_propbaser, 0, 32, value);
> + data = value;
> + if (FIELD_EX64(data, GICR_PROPBASER, IDBITS) > GICD_TYPER_IDBITS) {
> + data &= ~R_GICR_PROPBASER_IDBITS_MASK;
> + data |= GICD_TYPER_IDBITS;
> + }
> + cs->gicr_propbaser = deposit64(cs->gicr_propbaser, 0, 32, data);
This is still wrong. On v2 I said:
# This isn't what the spec says happens. It says that if the value the
guest writes
# in this field is larger than GICD_TYPER.IDbits, then the
GICD_TYPER.IDBits value
# applies. That doesn't mean the value reads back as GICD_TYPER.IDBits.
#
# How you want to handle this depends on what's going on, but it probably mostly
# looks like "code that cares about GICR_PROPBASER.IDBits will do
# MIN(field_value, GICD_TYPER_IDBITS) to find the effective value of the field".
> return MEMTX_OK;
> case GICR_PROPBASER + 4:
> cs->gicr_propbaser = deposit64(cs->gicr_propbaser, 32, 32, value);
> @@ -395,9 +408,16 @@ static MemTxResult gicr_readll(GICv3CPUState *cs, hwaddr offset,
> static MemTxResult gicr_writell(GICv3CPUState *cs, hwaddr offset,
> uint64_t value, MemTxAttrs attrs)
> {
> + uint64_t data;
> +
> switch (offset) {
> case GICR_PROPBASER:
> - cs->gicr_propbaser = value;
> + data = value;
> + if (FIELD_EX64(data, GICR_PROPBASER, IDBITS) > GICD_TYPER_IDBITS) {
> + data &= ~R_GICR_PROPBASER_IDBITS_MASK;
> + data |= GICD_TYPER_IDBITS;
> + }
> + cs->gicr_propbaser = data;
Ditto.
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC
2021-04-29 23:42 ` [PATCH v3 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC Shashi Mallela
@ 2021-05-18 16:03 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 16:03 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Included creation of ITS as part of SBSA platform GIC
> initialization.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 8/8] hw/arm/virt: add ITS support in virt GIC
2021-04-29 23:42 ` [PATCH v3 8/8] hw/arm/virt: add ITS support in virt GIC Shashi Mallela
@ 2021-05-18 16:06 ` Peter Maydell
0 siblings, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2021-05-18 16:06 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Included creation of ITS as part of virt platform GIC
> initialization.This Emulated ITS model now co-exists with kvm
Missing space after '.'.
> ITS and is enabled in absence of kvm irq kernel support in a
> platform.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 6/8] hw/intc: GICv3 redistributor ITS processing
2021-04-29 23:41 ` [PATCH v3 6/8] hw/intc: GICv3 redistributor ITS processing Shashi Mallela
@ 2021-05-20 11:01 ` Peter Maydell
2021-05-25 17:58 ` shashi.mallela
0 siblings, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2021-05-20 11:01 UTC (permalink / raw)
To: Shashi Mallela
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Implemented lpi processing at redistributor to get lpi config info
> from lpi configuration table,determine priority,set pending state in
> lpi pending table and forward the lpi to cpuif.Added logic to invoke
> redistributor lpi processing with translated LPI which set/clear LPI
> from ITS device as part of ITS INT,CLEAR,DISCARD command and
> GITS_TRANSLATER processing.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
> hw/intc/arm_gicv3.c | 6 ++
> hw/intc/arm_gicv3_cpuif.c | 20 ++++--
> hw/intc/arm_gicv3_its.c | 12 ++--
> hw/intc/arm_gicv3_redist.c | 133 +++++++++++++++++++++++++++++++++++++
> hw/intc/gicv3_internal.h | 9 +++
> 5 files changed, 171 insertions(+), 9 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> index 66eaa97198..618fa1af95 100644
> --- a/hw/intc/arm_gicv3.c
> +++ b/hw/intc/arm_gicv3.c
> @@ -166,6 +166,12 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)
> cs->hppi.grp = gicv3_irq_group(cs->gic, cs, cs->hppi.irq);
> }
>
> + if (cs->gic->lpi_enable) {
> + if (gicv3_redist_update_lpi(cs)) {
> + seenbetter = true;
> + }
> + }
> +
I'm not sure if this call is in the right place. This function
(gicv3_redist_update_noirqset()) is specifically for when state
in the *redistributor* has changed, and it is trying to be a fast
path for "we know that only the redistributor state has changed,
so we might be able to find the new best interrupt by looking only
at the redistributor state". It has a fallback case at the bottom
of the function for "the redistributor state changed such that
we have to actually look at the whole of the GIC state to find
the new best interrupt".
> /* If the best interrupt we just found would preempt whatever
> * was the previous best interrupt before this update, then
> * we know it's definitely the best one now.
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 43ef1d7a84..11b1df5b6b 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -899,9 +899,14 @@ static void icc_activate_irq(GICv3CPUState *cs, int irq)
> cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0);
> gicv3_redist_update(cs);
> } else {
> - gicv3_gicd_active_set(cs->gic, irq);
> - gicv3_gicd_pending_clear(cs->gic, irq);
> - gicv3_update(cs->gic, irq, 1);
> + if (irq >= GICV3_LPI_INTID_START) {
> + gicv3_redist_lpi_pending(cs, irq, 0);
> + gicv3_redist_update(cs);
> + } else {
> + gicv3_gicd_active_set(cs->gic, irq);
> + gicv3_gicd_pending_clear(cs->gic, irq);
> + gicv3_update(cs->gic, irq, 1);
> + }
Don't nest this if(), instead write the whole ladder at the same
nesting depth:
if (irq < GIC_INTERNAL) {
handle internal irq;
} else if (irq < GICV3_LPI_INTID_START) {
handle normal irq;
} else {
handle LPI;
}
> }
> }
>
> @@ -1328,7 +1333,8 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
> }
> }
>
> - if (irq >= cs->gic->num_irq) {
> + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable &&
> + (irq >= GICV3_LPI_INTID_START)))) {
> /* This handles two cases:
> * 1. If software writes the ID of a spurious interrupt [ie 1020-1023]
> * to the GICC_EOIR, the GIC ignores that write.
> @@ -1348,7 +1354,11 @@ static void icc_eoir_write(CPUARMState *env, const ARMCPRegInfo *ri,
>
> if (!icc_eoi_split(env, cs)) {
> /* Priority drop and deactivate not split: deactivate irq now */
> - icc_deactivate_irq(cs, irq);
> + if (irq >= GICV3_LPI_INTID_START) {
> + gicv3_update(cs->gic, irq, 1);
This doesn't look right. You're not actually doing anything here for
an LPI interrupt, so you shouldn't need to call gicv3_update().
> + } else {
> + icc_deactivate_irq(cs, irq);
> + }
> }
> }
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> index 98c984dd22..28da2d1d77 100644
> --- a/hw/intc/arm_gicv3_its.c
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -219,6 +219,7 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value,
> bool ite_valid = false;
> uint64_t cte = 0;
> bool cte_valid = false;
> + uint64_t rdbase;
> uint64_t itel = 0;
> uint32_t iteh = 0;
>
> @@ -275,10 +276,13 @@ static MemTxResult process_int(GICv3ITSState *s, uint64_t value,
> * command in the queue
> */
> } else {
> - /*
> - * Current implementation only supports rdbase == procnum
> - * Hence rdbase physical address is ignored
> - */
> + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK;
> + if ((cmd == CLEAR) || (cmd == DISCARD)) {
> + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 0);
> + } else {
> + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase], pIntid, 1);
> + }
You need to check that rdbase is actually within the range of the
number of CPUs here, otherwise you might access the cpu[] array
out of bounds.
> +
> if (cmd == DISCARD) {
> /* remove mapping from interrupt translation table */
> res = update_ite(s, eventid, dte, itel, iteh);
> diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> index 7604ccdc83..82ca9d71e5 100644
> --- a/hw/intc/arm_gicv3_redist.c
> +++ b/hw/intc/arm_gicv3_redist.c
> @@ -256,6 +256,8 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
> if (cs->gicr_typer & GICR_TYPER_PLPIS) {
> if (value & GICR_CTLR_ENABLE_LPIS) {
> cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
> + /* Check for any pending interr in pending table */
> + gicv3_redist_update(cs);
> } else {
> cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
> }
> @@ -546,6 +548,137 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,
> return r;
> }
>
> +bool gicv3_redist_update_lpi(GICv3CPUState *cs)
> +{
> + /*
> + * This function scans the LPI pending table and for each pending
> + * LPI, reads the corresponding entry from LPI configuration table
> + * to extract the priority info and determine if the LPI priority
> + * is lower than the current high priority interrupt.If yes, update
> + * high priority pending interrupt to that of LPI.
> + */
I would still like to see some profiling of whether we spend a significant
amount of time in this function...
> + AddressSpace *as = &cs->gic->dma_as;
> + uint64_t lpict_baddr, lpipt_baddr;
> + uint32_t pendt_size = 0;
> + uint8_t lpite;
> + uint8_t prio, pend;
> + int i;
> + bool seenbetter = false;
> +
> + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
> + !cs->gicr_pendbaser || (FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
> + IDBITS) < GICR_PROPBASER_IDBITS_THRESHOLD)) {
> + return seenbetter;
> + }
> +
> + lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, PHYADDR);
> + lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
> +
> + lpipt_baddr = FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER, PHYADDR);
> + lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
> +
> + /* Determine the highest priority pending interrupt among LPIs */
> + pendt_size = (1UL << (FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
> + IDBITS) - 1));
> +
> + for (i = 0; i < pendt_size; i++) {
> + address_space_read(as, lpipt_baddr +
> + (((GICV3_LPI_INTID_START + i) / 8) * sizeof(pend)),
> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
> +
> + if ((1 << ((GICV3_LPI_INTID_START + i) % 8)) & pend) {
> + address_space_read(as, lpict_baddr + (i * sizeof(lpite)),
> + MEMTXATTRS_UNSPECIFIED, &lpite, sizeof(lpite));
> +
> + prio = ((lpite >> LPI_CTE_PRIORITY_OFFSET) &
> + LPI_CTE_PRIORITY_MASK);
> + prio &= LPI_PRIORITY_MASK;
The priority field interpretation depends on whether GICD_CTLR.DS is 0 or 1,
(see section 5.1.1 in the GICv3 spec).
> +
> + if (prio < cs->hppi.prio) {
> + cs->hppi.irq = GICV3_LPI_INTID_START + i;
> + cs->hppi.prio = prio;
> + /* LPIs are always non-secure Grp1 interrupts */
> + cs->hppi.grp = GICV3_G1NS;
> + seenbetter = true;
> + }
> + }
> + }
> + return seenbetter;
> +}
> +
> +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level)
> +{
> + AddressSpace *as = &cs->gic->dma_as;
> + uint64_t lpipt_baddr;
> + bool ispend = false;
> + uint8_t pend;
> +
> + /*
> + * get the bit value corresponding to this irq in the
> + * lpi pending table
> + */
> + lpipt_baddr = FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER, PHYADDR);
> + lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
You can write
lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK;
because the register is laid out deliberately so that bits [51:16] in the
register specify bits [51:16] of the address.
> +
> + address_space_read(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),
> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
> + ispend = ((pend >> (irq % 8)) & 0x1);
> +
> + if (ispend) {
> + if (!level) {
> + /*
> + * clear the pending bit and update the lpi pending table
> + */
> + pend &= ~(1 << (irq % 8));
> +
> + address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),
> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
> + }
> + } else {
> + if (level) {
> + /*
> + * if pending bit is not already set for this irq,turn-on the
> + * pending bit and update the lpi pending table
> + */
> + pend |= (1 << (irq % 8));
> +
> + address_space_write(as, lpipt_baddr + ((irq / 8) * sizeof(pend)),
> + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
> + }
> + }
> +}
> +
> +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)
> +{
> + AddressSpace *as = &cs->gic->dma_as;
> + uint64_t lpict_baddr;
> + uint8_t lpite;
> +
> + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs->gicr_propbaser ||
> + !cs->gicr_pendbaser || (FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
> + IDBITS) < GICR_PROPBASER_IDBITS_THRESHOLD)) {
> + return;
> + }
Something needs to be checking for "attempt to work with an interrupt
number that is too large according to either GICR_PROPBASER.IDbits or
GICD_TYPER.IDbits". You could do it either in this function or in the
callers; doing it here is probably as good as anywhere.
> + lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER, PHYADDR);
> + lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
> +
> + /* get the lpi config table entry corresponding to this irq */
> + address_space_read(as, lpict_baddr + ((irq - GICV3_LPI_INTID_START) *
> + sizeof(lpite)), MEMTXATTRS_UNSPECIFIED,
> + &lpite, sizeof(lpite));
> +
> + /* check if this irq is enabled before proceeding further */
> + if (!(lpite & LPI_CTE_ENABLED)) {
> + return;
> + }
> +
> + /* set/clear the pending bit for this irq */
> + gicv3_redist_lpi_pending(cs, irq, level);
> +
> + gicv3_redist_update(cs);
> +}
> +
> void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level)
> {
> /* Update redistributor state for a change in an external PPI input line */
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 4/8] hw/intc: GICv3 ITS Command processing
2021-05-18 15:49 ` Peter Maydell
@ 2021-05-25 17:57 ` shashi.mallela
0 siblings, 0 replies; 25+ messages in thread
From: shashi.mallela @ 2021-05-25 17:57 UTC (permalink / raw)
To: Peter Maydell
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
Have taken care of all comments ,except one with my inline response
On Tue, 2021-05-18 at 16:49 +0100, Peter Maydell wrote:
> On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <
> shashi.mallela@linaro.org> wrote:
> > Added ITS command queue handling for MAPTI,MAPI commands,handled
> > ITS
> > translation which triggers an LPI via INT command as well as write
> > to GITS_TRANSLATER register,defined enum to differentiate between
> > ITS
> > command interrupt trigger and GITS_TRANSLATER based interrupt
> > trigger.
> > Each of these commands make use of other functionalities
> > implemented to
> > get device table entry,collection table entry or interrupt
> > translation
> > table entry required for their processing.
> >
> > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> > ---
> > hw/intc/arm_gicv3_its.c | 346
> > ++++++++++++++++++++++++++++-
> > hw/intc/gicv3_internal.h | 12 +
> > include/hw/intc/arm_gicv3_common.h | 2 +
> > 3 files changed, 359 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index 7cb465813a..98c984dd22 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -28,6 +28,156 @@ struct GICv3ITSClass {
> > void (*parent_reset)(DeviceState *dev);
> > };
> >
> > +typedef enum ItsCmdType {
> > + NONE = 0, /* internal indication for GITS_TRANSLATER write */
> > + CLEAR = 1,
> > + DISCARD = 2,
> > + INT = 3,
> > +} ItsCmdType;
> > +
> > +static bool get_cte(GICv3ITSState *s, uint16_t icid, uint64_t
> > *cte,
> > + MemTxResult *res)
> > +{
> > + AddressSpace *as = &s->gicv3->dma_as;
> > + uint64_t l2t_addr;
> > + uint64_t value;
> > + bool valid_l2t;
> > + uint32_t l2t_id;
> > + uint32_t max_l2_entries;
> > + bool status = false;
> > +
> > + if (s->ct.indirect) {
> > + l2t_id = icid / (s->ct.page_sz / L1TABLE_ENTRY_SIZE);
> > +
> > + value = address_space_ldq_le(as,
> > + s->ct.base_addr +
> > + (l2t_id *
> > L1TABLE_ENTRY_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, res);
> > +
> > + if (*res == MEMTX_OK) {
> > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > + if (valid_l2t) {
> > + max_l2_entries = s->ct.page_sz / s->ct.entry_sz;
> > +
> > + l2t_addr = value & ((1ULL << 51) - 1);
> > +
> > + *cte = address_space_ldq_le(as, l2t_addr +
> > + ((icid % max_l2_entries) *
> > GITS_CTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, res);
> > + }
> > + }
> > + } else {
> > + /* Flat level table */
> > + *cte = address_space_ldq_le(as, s->ct.base_addr +
> > + (icid * GITS_CTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED,
> > res);
> > + }
> > +
> > + if (*cte & VALID_MASK) {
> > + status = true;
> > + }
> > +
> > + return status;
> > +}
> > +
> > +static MemTxResult update_ite(GICv3ITSState *s, uint32_t eventid,
> > uint64_t dte,
> > + uint64_t itel, uint32_t iteh)
> > +{
> > + AddressSpace *as = &s->gicv3->dma_as;
> > + uint64_t itt_addr;
> > + MemTxResult res = MEMTX_OK;
> > +
> > + itt_addr = (dte >> 6ULL) & ITTADDR_MASK;
> > + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
> > +
> > + address_space_stq_le(as, itt_addr + (eventid *
> > sizeof(uint64_t)),
> > + itel, MEMTXATTRS_UNSPECIFIED, &res);
> > +
> > + if (res == MEMTX_OK) {
> > + address_space_stl_le(as, itt_addr + ((eventid +
> > sizeof(uint64_t)) *
> > + sizeof(uint32_t)), iteh,
> > MEMTXATTRS_UNSPECIFIED,
> > + &res);
> > + }
> > + return res;
> > +}
> > +
> > +static bool get_ite(GICv3ITSState *s, uint32_t eventid, uint64_t
> > dte,
> > + uint16_t *icid, uint32_t *pIntid,
> > MemTxResult *res)
> > +{
> > + AddressSpace *as = &s->gicv3->dma_as;
> > + uint64_t itt_addr;
> > + bool status = false;
> > + uint64_t itel = 0;
> > + uint32_t iteh = 0;
> > +
> > + itt_addr = (dte >> 6ULL) & ITTADDR_MASK;
> > + itt_addr <<= ITTADDR_SHIFT; /* 256 byte aligned */
> > +
> > + itel = address_space_ldq_le(as, itt_addr + (eventid *
> > sizeof(uint64_t)),
> > + MEMTXATTRS_UNSPECIFIED, res);
> > +
> > + if (*res == MEMTX_OK) {
> > + iteh = address_space_ldl_le(as, itt_addr + ((eventid +
> > + sizeof(uint64_t)) *
> > sizeof(uint32_t)),
> > + MEMTXATTRS_UNSPECIFIED, res);
> > +
> > + if (*res == MEMTX_OK) {
> > + if (itel & VALID_MASK) {
> > + if ((itel >> ITE_ENTRY_INTTYPE_SHIFT) &
> > GITS_TYPE_PHYSICAL) {
> > + *pIntid = (itel >> ITE_ENTRY_INTID_SHIFT) &
> > + ITE_ENTRY_INTID_MASK;
> > + *icid = iteh & ITE_ENTRY_ICID_MASK;
> > + status = true;
> > + }
> > + }
> > + }
> > + }
> > + return status;
> > +}
> > +
> > +static uint64_t get_dte(GICv3ITSState *s, uint32_t devid,
> > MemTxResult *res)
> > +{
> > + AddressSpace *as = &s->gicv3->dma_as;
> > + uint64_t l2t_addr;
> > + uint64_t value;
> > + bool valid_l2t;
> > + uint32_t l2t_id;
> > + uint32_t max_l2_entries;
> > +
> > + if (s->dt.indirect) {
> > + l2t_id = devid / (s->dt.page_sz / L1TABLE_ENTRY_SIZE);
> > +
> > + value = address_space_ldq_le(as,
> > + s->dt.base_addr +
> > + (l2t_id *
> > L1TABLE_ENTRY_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, res);
> > +
> > + if (*res == MEMTX_OK) {
> > + valid_l2t = (value >> VALID_SHIFT) & VALID_MASK;
> > +
> > + if (valid_l2t) {
> > + max_l2_entries = s->dt.page_sz / s->dt.entry_sz;
> > +
> > + l2t_addr = value & ((1ULL << 51) - 1);
> > +
> > + value = 0;
>
> This assignment is pointless because we assign again to value
> immediately
> afterwards.
>
> > + value = address_space_ldq_le(as, l2t_addr +
> > + ((devid % max_l2_entries) *
> > GITS_DTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, res);
> > + }
> > + }
> > + } else {
> > + /* Flat level table */
> > + value = 0;
>
> Ditto.
>
> > + value = address_space_ldq_le(as, s->dt.base_addr +
> > + (devid *
> > GITS_DTE_SIZE),
> > + MEMTXATTRS_UNSPECIFIED, res);
> > + }
> > +
> > + return value;
> > +}
> > +
> > static MemTxResult process_sync(GICv3ITSState *s, uint32_t offset)
> > {
> > AddressSpace *as = &s->gicv3->dma_as;
> > @@ -55,6 +205,182 @@ static MemTxResult process_sync(GICv3ITSState
> > *s, uint32_t offset)
> > return res;
> > }
> >
> > +static MemTxResult process_int(GICv3ITSState *s, uint64_t value,
> > + uint32_t offset, ItsCmdType cmd)
> > +{
> > + AddressSpace *as = &s->gicv3->dma_as;
> > + uint32_t devid, eventid;
> > + MemTxResult res = MEMTX_OK;
> > + bool dte_valid;
> > + uint64_t dte = 0;
> > + uint32_t max_eventid;
> > + uint16_t icid = 0;
> > + uint32_t pIntid = 0;
> > + bool ite_valid = false;
> > + uint64_t cte = 0;
> > + bool cte_valid = false;
> > + uint64_t itel = 0;
> > + uint32_t iteh = 0;
> > +
> > + if (cmd == NONE) {
> > + devid = offset;
> > + } else {
> > + devid = (value >> DEVID_SHIFT) & DEVID_MASK;
> > +
> > + offset += NUM_BYTES_IN_DW;
> > + value = address_space_ldq_le(as, s->cq.base_addr + offset,
> > + MEMTXATTRS_UNSPECIFIED, &res);
> > + }
> > +
> > + if (res != MEMTX_OK) {
> > + return res;
> > + }
> > +
> > + eventid = (value & EVENTID_MASK);
> > +
> > + dte = get_dte(s, devid, &res);
> > +
> > + if (res != MEMTX_OK) {
> > + return res;
> > + }
> > + dte_valid = dte & VALID_MASK;
> > +
> > + if (dte_valid) {
> > + max_eventid = (1UL << (((dte >> 1U) & SIZE_MASK) + 1));
> > +
> > + ite_valid = get_ite(s, eventid, dte, &icid, &pIntid,
> > &res);
> > +
> > + if (res != MEMTX_OK) {
> > + return res;
> > + }
> > +
> > + if (ite_valid) {
> > + cte_valid = get_cte(s, icid, &cte, &res);
> > + }
> > +
> > + if (res != MEMTX_OK) {
> > + return res;
> > + }
> > + }
> > +
> > + if ((devid > s->dt.max_devids) || !dte_valid || !ite_valid ||
> > + !cte_valid || (eventid > max_eventid)) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: invalid interrupt translation table attributes "
> > + "devid %d or eventid %d\n",
> > + __func__, devid, eventid);
> > + /*
> > + * in this implementation,in case of error
> > + * we ignore this command and move onto the next
> > + * command in the queue
> > + */
>
> ...but we could just return an error from this function and
> get the 'stall' behaviour. It would be more consistent to just
> stall for everything, if we're going to be stalling for various
> kinds of "failed to read memory" anyway. (Same for some instances
> of this in the previous patches.)
>
> The intent here was to chose the spec defined options to 'ignore and
> move on' for all ITS command specific errors but differentiate system
> errors like mem read/write with 'implementation defined' STALL.
>
>
> > + } else {
> > + /*
> > + * Current implementation only supports rdbase == procnum
> > + * Hence rdbase physical address is ignored
> > + */
> > + if (cmd == DISCARD) {
> > + /* remove mapping from interrupt translation table */
> > + res = update_ite(s, eventid, dte, itel, iteh);
> > + }
> > + }
> > +
> > + if (cmd != NONE) {
> > + offset += NUM_BYTES_IN_DW;
> > + offset += NUM_BYTES_IN_DW;
>
> More dead increments.
>
> > + }
> > +
> > + return res;
> > +}
> > +
>
> -- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 6/8] hw/intc: GICv3 redistributor ITS processing
2021-05-20 11:01 ` Peter Maydell
@ 2021-05-25 17:58 ` shashi.mallela
0 siblings, 0 replies; 25+ messages in thread
From: shashi.mallela @ 2021-05-25 17:58 UTC (permalink / raw)
To: Peter Maydell
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
Have taken care of all comments ,except one with my inline response
On Thu, 2021-05-20 at 12:01 +0100, Peter Maydell wrote:
> On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <
> shashi.mallela@linaro.org> wrote:
> > Implemented lpi processing at redistributor to get lpi config info
> > from lpi configuration table,determine priority,set pending state
> > in
> > lpi pending table and forward the lpi to cpuif.Added logic to
> > invoke
> > redistributor lpi processing with translated LPI which set/clear
> > LPI
> > from ITS device as part of ITS INT,CLEAR,DISCARD command and
> > GITS_TRANSLATER processing.
> >
> > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> > ---
> > hw/intc/arm_gicv3.c | 6 ++
> > hw/intc/arm_gicv3_cpuif.c | 20 ++++--
> > hw/intc/arm_gicv3_its.c | 12 ++--
> > hw/intc/arm_gicv3_redist.c | 133
> > +++++++++++++++++++++++++++++++++++++
> > hw/intc/gicv3_internal.h | 9 +++
> > 5 files changed, 171 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> > index 66eaa97198..618fa1af95 100644
> > --- a/hw/intc/arm_gicv3.c
> > +++ b/hw/intc/arm_gicv3.c
> > @@ -166,6 +166,12 @@ static void
> > gicv3_redist_update_noirqset(GICv3CPUState *cs)
> > cs->hppi.grp = gicv3_irq_group(cs->gic, cs, cs->hppi.irq);
> > }
> >
> > + if (cs->gic->lpi_enable) {
> > + if (gicv3_redist_update_lpi(cs)) {
> > + seenbetter = true;
> > + }
> > + }
> > +
>
> I'm not sure if this call is in the right place. This function
> (gicv3_redist_update_noirqset()) is specifically for when state
> in the *redistributor* has changed, and it is trying to be a fast
> path for "we know that only the redistributor state has changed,
> so we might be able to find the new best interrupt by looking only
> at the redistributor state". It has a fallback case at the bottom
> of the function for "the redistributor state changed such that
> we have to actually look at the whole of the GIC state to find
> the new best interrupt".
>
> The placement of this call is along the lines of SGI,PPI interrupt
> handling in the redistributor wherein at the end of each of PPI.SGI
> (and also gicv3_redist_process_lpi())interrupt functions
> gicv3_redist_update() is called which internally calls
> gicv3_redist_update_noirqset() to determine the highest priority
> interrupt between SGI,PPI.So,i added this call here so that LPIs are
> also included in determining the highest priority interrupt between
> pending SGI,PPI,LPI.
> On the other hand if LPI pending interrupt was deferred until whole
> GIC scan,in the current implementation the whole GIC scanning
> fallback case could be skipped when either of SGI or PPI are
> found,thereby not even considering LPI interrupt priorities.
>
>
> > /* If the best interrupt we just found would preempt whatever
> > * was the previous best interrupt before this update, then
> > * we know it's definitely the best one now.
> > diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> > index 43ef1d7a84..11b1df5b6b 100644
> > --- a/hw/intc/arm_gicv3_cpuif.c
> > +++ b/hw/intc/arm_gicv3_cpuif.c
> > @@ -899,9 +899,14 @@ static void icc_activate_irq(GICv3CPUState
> > *cs, int irq)
> > cs->gicr_ipendr0 = deposit32(cs->gicr_ipendr0, irq, 1, 0);
> > gicv3_redist_update(cs);
> > } else {
> > - gicv3_gicd_active_set(cs->gic, irq);
> > - gicv3_gicd_pending_clear(cs->gic, irq);
> > - gicv3_update(cs->gic, irq, 1);
> > + if (irq >= GICV3_LPI_INTID_START) {
> > + gicv3_redist_lpi_pending(cs, irq, 0);
> > + gicv3_redist_update(cs);
> > + } else {
> > + gicv3_gicd_active_set(cs->gic, irq);
> > + gicv3_gicd_pending_clear(cs->gic, irq);
> > + gicv3_update(cs->gic, irq, 1);
> > + }
>
> Don't nest this if(), instead write the whole ladder at the same
> nesting depth:
> if (irq < GIC_INTERNAL) {
> handle internal irq;
> } else if (irq < GICV3_LPI_INTID_START) {
> handle normal irq;
> } else {
> handle LPI;
> }
>
> > }
> > }
> >
> > @@ -1328,7 +1333,8 @@ static void icc_eoir_write(CPUARMState *env,
> > const ARMCPRegInfo *ri,
> > }
> > }
> >
> > - if (irq >= cs->gic->num_irq) {
> > + if ((irq >= cs->gic->num_irq) && (!(cs->gic->lpi_enable &&
> > + (irq >= GICV3_LPI_INTID_START)))) {
> > /* This handles two cases:
> > * 1. If software writes the ID of a spurious interrupt
> > [ie 1020-1023]
> > * to the GICC_EOIR, the GIC ignores that write.
> > @@ -1348,7 +1354,11 @@ static void icc_eoir_write(CPUARMState *env,
> > const ARMCPRegInfo *ri,
> >
> > if (!icc_eoi_split(env, cs)) {
> > /* Priority drop and deactivate not split: deactivate irq
> > now */
> > - icc_deactivate_irq(cs, irq);
> > + if (irq >= GICV3_LPI_INTID_START) {
> > + gicv3_update(cs->gic, irq, 1);
>
> This doesn't look right. You're not actually doing anything here for
> an LPI interrupt, so you shouldn't need to call gicv3_update().
>
> > + } else {
> > + icc_deactivate_irq(cs, irq);
> > + }
> > }
> > }
> >
> > diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> > index 98c984dd22..28da2d1d77 100644
> > --- a/hw/intc/arm_gicv3_its.c
> > +++ b/hw/intc/arm_gicv3_its.c
> > @@ -219,6 +219,7 @@ static MemTxResult process_int(GICv3ITSState
> > *s, uint64_t value,
> > bool ite_valid = false;
> > uint64_t cte = 0;
> > bool cte_valid = false;
> > + uint64_t rdbase;
> > uint64_t itel = 0;
> > uint32_t iteh = 0;
> >
> > @@ -275,10 +276,13 @@ static MemTxResult process_int(GICv3ITSState
> > *s, uint64_t value,
> > * command in the queue
> > */
> > } else {
> > - /*
> > - * Current implementation only supports rdbase == procnum
> > - * Hence rdbase physical address is ignored
> > - */
> > + rdbase = (cte >> 1U) & RDBASE_PROCNUM_MASK;
> > + if ((cmd == CLEAR) || (cmd == DISCARD)) {
> > + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase],
> > pIntid, 0);
> > + } else {
> > + gicv3_redist_process_lpi(&s->gicv3->cpu[rdbase],
> > pIntid, 1);
> > + }
>
> You need to check that rdbase is actually within the range of the
> number of CPUs here, otherwise you might access the cpu[] array
> out of bounds.
>
> > +
> > if (cmd == DISCARD) {
> > /* remove mapping from interrupt translation table */
> > res = update_ite(s, eventid, dte, itel, iteh);
> > diff --git a/hw/intc/arm_gicv3_redist.c
> > b/hw/intc/arm_gicv3_redist.c
> > index 7604ccdc83..82ca9d71e5 100644
> > --- a/hw/intc/arm_gicv3_redist.c
> > +++ b/hw/intc/arm_gicv3_redist.c
> > @@ -256,6 +256,8 @@ static MemTxResult gicr_writel(GICv3CPUState
> > *cs, hwaddr offset,
> > if (cs->gicr_typer & GICR_TYPER_PLPIS) {
> > if (value & GICR_CTLR_ENABLE_LPIS) {
> > cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
> > + /* Check for any pending interr in pending table
> > */
> > + gicv3_redist_update(cs);
> > } else {
> > cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
> > }
> > @@ -546,6 +548,137 @@ MemTxResult gicv3_redist_write(void *opaque,
> > hwaddr offset, uint64_t data,
> > return r;
> > }
> >
> > +bool gicv3_redist_update_lpi(GICv3CPUState *cs)
> > +{
> > + /*
> > + * This function scans the LPI pending table and for each
> > pending
> > + * LPI, reads the corresponding entry from LPI configuration
> > table
> > + * to extract the priority info and determine if the LPI
> > priority
> > + * is lower than the current high priority interrupt.If yes,
> > update
> > + * high priority pending interrupt to that of LPI.
> > + */
>
> I would still like to see some profiling of whether we spend a
> significant
> amount of time in this function...
>
> > + AddressSpace *as = &cs->gic->dma_as;
> > + uint64_t lpict_baddr, lpipt_baddr;
> > + uint32_t pendt_size = 0;
> > + uint8_t lpite;
> > + uint8_t prio, pend;
> > + int i;
> > + bool seenbetter = false;
> > +
> > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs-
> > >gicr_propbaser ||
> > + !cs->gicr_pendbaser || (FIELD_EX64(cs->gicr_propbaser,
> > GICR_PROPBASER,
> > + IDBITS) < GICR_PROPBASER_IDBITS_THRESHOLD)) {
> > + return seenbetter;
> > + }
> > +
> > + lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
> > PHYADDR);
> > + lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
> > +
> > + lpipt_baddr = FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER,
> > PHYADDR);
> > + lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
> > +
> > + /* Determine the highest priority pending interrupt among LPIs
> > */
> > + pendt_size = (1UL << (FIELD_EX64(cs->gicr_propbaser,
> > GICR_PROPBASER,
> > + IDBITS) - 1));
> > +
> > + for (i = 0; i < pendt_size; i++) {
> > + address_space_read(as, lpipt_baddr +
> > + (((GICV3_LPI_INTID_START + i) / 8) *
> > sizeof(pend)),
> > + MEMTXATTRS_UNSPECIFIED, &pend, sizeof(pend));
> > +
> > + if ((1 << ((GICV3_LPI_INTID_START + i) % 8)) & pend) {
> > + address_space_read(as, lpict_baddr + (i *
> > sizeof(lpite)),
> > + MEMTXATTRS_UNSPECIFIED, &lpite,
> > sizeof(lpite));
> > +
> > + prio = ((lpite >> LPI_CTE_PRIORITY_OFFSET) &
> > + LPI_CTE_PRIORITY_MASK);
> > + prio &= LPI_PRIORITY_MASK;
>
> The priority field interpretation depends on whether GICD_CTLR.DS is
> 0 or 1,
> (see section 5.1.1 in the GICv3 spec).
>
> > +
> > + if (prio < cs->hppi.prio) {
> > + cs->hppi.irq = GICV3_LPI_INTID_START + i;
> > + cs->hppi.prio = prio;
> > + /* LPIs are always non-secure Grp1 interrupts */
> > + cs->hppi.grp = GICV3_G1NS;
> > + seenbetter = true;
> > + }
> > + }
> > + }
> > + return seenbetter;
> > +}
> > +
> > +void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int
> > level)
> > +{
> > + AddressSpace *as = &cs->gic->dma_as;
> > + uint64_t lpipt_baddr;
> > + bool ispend = false;
> > + uint8_t pend;
> > +
> > + /*
> > + * get the bit value corresponding to this irq in the
> > + * lpi pending table
> > + */
> > + lpipt_baddr = FIELD_EX64(cs->gicr_pendbaser, GICR_PENDBASER,
> > PHYADDR);
> > + lpipt_baddr <<= R_GICR_PENDBASER_PHYADDR_SHIFT;
>
> You can write
> lpipt_baddr = cs->gicr_pendbaser & R_GICR_PENDBASER_PHYADDR_MASK;
>
> because the register is laid out deliberately so that bits [51:16] in
> the
> register specify bits [51:16] of the address.
>
> > +
> > + address_space_read(as, lpipt_baddr + ((irq / 8) *
> > sizeof(pend)),
> > + MEMTXATTRS_UNSPECIFIED, &pend,
> > sizeof(pend));
> > + ispend = ((pend >> (irq % 8)) & 0x1);
> > +
> > + if (ispend) {
> > + if (!level) {
> > + /*
> > + * clear the pending bit and update the lpi pending
> > table
> > + */
> > + pend &= ~(1 << (irq % 8));
> > +
> > + address_space_write(as, lpipt_baddr + ((irq / 8) *
> > sizeof(pend)),
> > + MEMTXATTRS_UNSPECIFIED, &pend,
> > sizeof(pend));
> > + }
> > + } else {
> > + if (level) {
> > + /*
> > + * if pending bit is not already set for this
> > irq,turn-on the
> > + * pending bit and update the lpi pending table
> > + */
> > + pend |= (1 << (irq % 8));
> > +
> > + address_space_write(as, lpipt_baddr + ((irq / 8) *
> > sizeof(pend)),
> > + MEMTXATTRS_UNSPECIFIED, &pend,
> > sizeof(pend));
> > + }
> > + }
> > +}
> > +
> > +void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int
> > level)
> > +{
> > + AddressSpace *as = &cs->gic->dma_as;
> > + uint64_t lpict_baddr;
> > + uint8_t lpite;
> > +
> > + if ((!cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) || !cs-
> > >gicr_propbaser ||
> > + !cs->gicr_pendbaser || (FIELD_EX64(cs->gicr_propbaser,
> > GICR_PROPBASER,
> > + IDBITS) < GICR_PROPBASER_IDBITS_THRESHOLD)) {
> > + return;
> > + }
>
> Something needs to be checking for "attempt to work with an interrupt
> number that is too large according to either GICR_PROPBASER.IDbits or
> GICD_TYPER.IDbits". You could do it either in this function or in the
> callers; doing it here is probably as good as anywhere.
>
> > + lpict_baddr = FIELD_EX64(cs->gicr_propbaser, GICR_PROPBASER,
> > PHYADDR);
> > + lpict_baddr <<= R_GICR_PROPBASER_PHYADDR_SHIFT;
> > +
> > + /* get the lpi config table entry corresponding to this irq */
> > + address_space_read(as, lpict_baddr + ((irq -
> > GICV3_LPI_INTID_START) *
> > + sizeof(lpite)), MEMTXATTRS_UNSPECIFIED,
> > + &lpite, sizeof(lpite));
> > +
> > + /* check if this irq is enabled before proceeding further */
> > + if (!(lpite & LPI_CTE_ENABLED)) {
> > + return;
> > + }
> > +
> > + /* set/clear the pending bit for this irq */
> > + gicv3_redist_lpi_pending(cs, irq, level);
> > +
> > + gicv3_redist_update(cs);
> > +}
> > +
> > void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level)
> > {
> > /* Update redistributor state for a change in an external PPI
> > input line */
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 0/8] GICv3 LPI and ITS feature implementation
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
` (9 preceding siblings ...)
2021-05-18 14:46 ` Peter Maydell
@ 2021-05-25 18:26 ` Alex Bennée
2021-05-25 19:30 ` Alex Bennée
10 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2021-05-25 18:26 UTC (permalink / raw)
To: Shashi Mallela; +Cc: peter.maydell, leif, qemu-devel, qemu-arm, rad
Shashi Mallela <shashi.mallela@linaro.org> writes:
> This patchset implements qemu device model for enabling physical
> LPI support and ITS functionality in GIC as per GICv3 specification.
> Both flat table and 2 level tables are implemented.The ITS commands
> for adding/deleting ITS table entries,trigerring LPI interrupts are
> implemented.Translated LPI interrupt ids are processed by redistributor
> to determine priority and set pending state appropriately before
> forwarding the same to cpu interface.
> The ITS feature support has been added to sbsa-ref platform as well as
> virt platform,wherein the emulated functionality co-exists with kvm
> kernel functionality.
So I'm definitely seeing a slow down in one of my testcases but it
doesn't seem to be HW access related. Via:
./qemu-system-aarch64 -cpu max,pauth-impdef=on -machine type=virt,virtualization=on,gic-version=3 -display none -serial mon:stdio -kernel ~/lsrc/linux.git/builds/arm64.initramfs/arch/arm64/boot/Image -append "console=ttyAMA0" -m 4096 -smp 1 -plugin contrib/plugins/libhwprofile.so,arg=source -d plugin -D before.log
pl011 @ 0xffffffc017043000
pc:ffffffc0103502cc, 1, 4, 0, 0
pc:ffffffc010350308, 1, 4, 0, 0
pc:ffffffc0103a3c14, 1, 11620, 0, 0
pc:ffffffc0103a3c4c, 0, 0, 1, 11688
gicv3_dist @ 0xffffffc018030000
pc:ffffffc01030c258, 1, 2, 0, 0
pc:ffffffc01030c31c, 0, 0, 1, 14
pc:ffffffc01030c348, 0, 0, 1, 56
pc:ffffffc01030c370, 0, 0, 1, 7
pc:ffffffc01030c37c, 0, 0, 1, 7
pc:ffffffc01030cba8, 1, 2, 0, 0
pc:ffffffc01030cc38, 0, 0, 1, 6
pc:ffffffc01030cf3c, 1, 1, 0, 0
pc:ffffffc01030d2b8, 1, 8, 0, 0
pc:ffffffc01030d6e4, 0, 0, 1, 2
pc:ffffffc01086b01c, 1, 1, 0, 0
pc:ffffffc01086b1e0, 1, 1, 0, 0
pc:ffffffc01086b1ec, 1, 1, 0, 0
pc:ffffffc01086b258, 1, 1, 0, 0
pc:ffffffc01086b39c, 0, 0, 1, 1
pc:ffffffc01086b3d4, 0, 0, 1, 7
pc:ffffffc01086b4d4, 0, 0, 1, 1
pc:ffffffc01086b51c, 0, 0, 1, 224
pcie-mmcfg-mmio @ 0xffffffc030000000
pc:ffffffc01031de00, 1, 65, 0, 0
pc:ffffffc01031de1c, 1, 105, 0, 0
pc:ffffffc01031de3c, 1, 77, 0, 0
pc:ffffffc01031de9c, 0, 0, 1, 2
pc:ffffffc01031deb0, 0, 0, 1, 8
pc:ffffffc01031debc, 0, 0, 1, 31
virtio-pci-common-virtio-net @ 0xffffffc0202d5000
pc:ffffffc01035d348, 1, 9, 0, 0
pc:ffffffc01035d368, 1, 12, 0, 0
pc:ffffffc01035d388, 1, 2, 0, 0
pc:ffffffc01035d3bc, 0, 0, 1, 1
pc:ffffffc01035d3dc, 0, 0, 1, 1
pc:ffffffc01035d54c, 0, 0, 1, 1
pc:ffffffc01035d560, 0, 0, 1, 1
pc:ffffffc01035d574, 0, 0, 1, 1
pc:ffffffc01035d588, 0, 0, 1, 1
pc:ffffffc01035d5bc, 0, 0, 1, 4
pc:ffffffc01035d780, 0, 0, 1, 3
pc:ffffffc01035d790, 0, 0, 1, 3
pc:ffffffc01035d7d0, 0, 0, 1, 1
pc:ffffffc01035db18, 0, 0, 1, 3
pc:ffffffc01035dbc0, 0, 0, 1, 3
pc:ffffffc01035dbd4, 0, 0, 1, 3
pc:ffffffc01035dbe4, 0, 0, 1, 3
pc:ffffffc01035dbf8, 0, 0, 1, 3
pc:ffffffc01035dc08, 0, 0, 1, 3
pc:ffffffc01035dc1c, 0, 0, 1, 3
pc:ffffffc01035dc2c, 0, 0, 1, 3
gicv3_redist_region[0] @ 0xffffffc018f60000
pc:ffffffc01030c258, 1, 10, 0, 0
pc:ffffffc01030c3cc, 0, 0, 1, 1
pc:ffffffc01030c3d8, 0, 0, 1, 1
pc:ffffffc01030c410, 0, 0, 1, 8
pc:ffffffc01030c428, 0, 0, 1, 1
pc:ffffffc01030c8f4, 1, 4, 0, 0
pc:ffffffc01030cc38, 0, 0, 1, 5
pc:ffffffc01030d164, 1, 2, 0, 0
pc:ffffffc01030d2b8, 1, 6, 0, 0
pc:ffffffc01030d360, 1, 1, 0, 0
pc:ffffffc01030d374, 0, 0, 1, 1
pc:ffffffc01030d388, 1, 1, 0, 0
pc:ffffffc01030e0c8, 0, 0, 1, 1
virtio-pci-notify-virtio-net @ 0xffffffc0202e2000
pc:ffffffc01035e554, 0, 0, 1, 29
But with your series applied the serial output is still the biggest hit:
pl011 @ 0xffffffc017043000
pc:ffffffc0103502cc, 1, 4, 0, 0
pc:ffffffc010350308, 1, 4, 0, 0
pc:ffffffc0103a3c14, 1, 11899, 0, 0
pc:ffffffc0103a3c4c, 0, 0, 1, 11938
gicv3_dist @ 0xffffffc018030000
pc:ffffffc01030c258, 1, 1, 0, 0
pc:ffffffc01030c31c, 0, 0, 1, 14
pc:ffffffc01030c348, 0, 0, 1, 56
pc:ffffffc01030c370, 0, 0, 1, 7
pc:ffffffc01030c37c, 0, 0, 1, 7
pc:ffffffc01030cba8, 1, 1, 0, 0
pc:ffffffc01030cc38, 0, 0, 1, 3
pc:ffffffc01030cf3c, 1, 4, 0, 0
pc:ffffffc01030d2b8, 1, 5, 0, 0
pc:ffffffc01030d6e4, 0, 0, 1, 1
pc:ffffffc01086b01c, 1, 1, 0, 0
pc:ffffffc01086b1e0, 1, 1, 0, 0
pc:ffffffc01086b1ec, 1, 1, 0, 0
pc:ffffffc01086b258, 1, 1, 0, 0
pc:ffffffc01086b39c, 0, 0, 1, 1
pc:ffffffc01086b3d4, 0, 0, 1, 7
pc:ffffffc01086b4d4, 0, 0, 1, 1
pc:ffffffc01086b51c, 0, 0, 1, 224
pcie-mmcfg-mmio @ 0xffffffc030000000
pc:ffffffc01031de00, 1, 65, 0, 0
pc:ffffffc01031de1c, 1, 122, 0, 0
pc:ffffffc01031de3c, 1, 79, 0, 0
pc:ffffffc01031de9c, 0, 0, 1, 2
pc:ffffffc01031deb0, 0, 0, 1, 13
pc:ffffffc01031debc, 0, 0, 1, 32
control @ 0xffffffc020050000
pc:ffffffc01030e6c4, 1, 21, 0, 0
pc:ffffffc01030eb00, 0, 0, 1, 5
pc:ffffffc01030ec94, 1, 1, 0, 0
pc:ffffffc01030eec8, 1, 16, 0, 0
pc:ffffffc01030ef6c, 1, 9, 0, 0
pc:ffffffc010310cb8, 1, 9, 0, 0
pc:ffffffc010310cc8, 0, 0, 1, 9
pc:ffffffc01086ba88, 1, 1, 0, 0
pc:ffffffc01086bd10, 1, 1, 0, 0
pc:ffffffc01086c1cc, 0, 0, 1, 1
pc:ffffffc01086c22c, 0, 0, 1, 1
pc:ffffffc01086c230, 1, 1, 0, 0
pc:ffffffc01086c248, 0, 0, 1, 1
virtio-pci-common-virtio-net @ 0xffffffc0202d5000
pc:ffffffc01035d348, 1, 9, 0, 0
pc:ffffffc01035d368, 1, 15, 0, 0
pc:ffffffc01035d388, 1, 2, 0, 0
pc:ffffffc01035d3bc, 0, 0, 1, 1
pc:ffffffc01035d3dc, 0, 0, 1, 1
pc:ffffffc01035d464, 0, 0, 1, 1
pc:ffffffc01035d54c, 0, 0, 1, 1
pc:ffffffc01035d560, 0, 0, 1, 1
pc:ffffffc01035d574, 0, 0, 1, 1
pc:ffffffc01035d588, 0, 0, 1, 1
pc:ffffffc01035d5bc, 0, 0, 1, 4
pc:ffffffc01035d780, 0, 0, 1, 3
pc:ffffffc01035d790, 0, 0, 1, 3
pc:ffffffc01035d7d0, 0, 0, 1, 1
pc:ffffffc01035db18, 0, 0, 1, 3
pc:ffffffc01035dbc0, 0, 0, 1, 3
pc:ffffffc01035dbd4, 0, 0, 1, 3
pc:ffffffc01035dbe4, 0, 0, 1, 3
pc:ffffffc01035dbf8, 0, 0, 1, 3
pc:ffffffc01035dc08, 0, 0, 1, 3
pc:ffffffc01035dc1c, 0, 0, 1, 3
pc:ffffffc01035dc2c, 0, 0, 1, 3
pc:ffffffc01035dcc4, 0, 0, 1, 2
gicv3_redist_region[0] @ 0xffffffc018f60000
pc:ffffffc01030c258, 1, 10, 0, 0
pc:ffffffc01030c3cc, 0, 0, 1, 1
pc:ffffffc01030c3d8, 0, 0, 1, 1
pc:ffffffc01030c410, 0, 0, 1, 8
pc:ffffffc01030c428, 0, 0, 1, 1
pc:ffffffc01030c8f4, 1, 4, 0, 0
pc:ffffffc01030cc38, 0, 0, 1, 5
pc:ffffffc01030d164, 1, 2, 0, 0
pc:ffffffc01030d2b8, 1, 6, 0, 0
pc:ffffffc01030d360, 1, 1, 0, 0
pc:ffffffc01030d374, 0, 0, 1, 1
pc:ffffffc01030d388, 1, 1, 0, 0
pc:ffffffc01030e0c8, 0, 0, 1, 1
pc:ffffffc01030e6c4, 1, 4, 0, 0
pc:ffffffc0103131dc, 1, 1, 0, 0
pc:ffffffc0103132b8, 1, 1, 0, 0
pc:ffffffc01031339c, 0, 0, 1, 1
pc:ffffffc010313410, 0, 0, 1, 1
pc:ffffffc01031342c, 1, 1, 0, 0
pc:ffffffc010313434, 0, 0, 1, 1
pc:ffffffc01086c3c4, 1, 1, 0, 0
So I ran with the hotblocks plugin:
./qemu-system-aarch64 -cpu max,pauth-impdef=on -machine type=virt,virtualization=on,gic-version=3 -display none -serial mon:stdio -kernel ~/lsrc/linux.git/builds/arm64.initramfs/arch/arm64/boot/Image -append "console=ttyAMA0" -m 4096 -smp 1 -plugin contrib/plugins/libhotblocks.so -d plugin -D hotblocks.log
collected 130606 entries in the hash table
pc, tcount, icount, ecount
0xffffffc010627fd0, 4, 10, 3998721 - memcpy
0xffffffc010628288, 2, 6, 3984790 - memset
0xffffffc01062832c, 3, 4, 1812870 - memset
0xffffffc0100a8df8, 4, 4, 1743432 - __my_cpu_offset
0xffffffc01015c394, 2, 4, 1304617 - __my_cpu_offset
0xffffffc010093348, 3, 3, 1228845 - decay_load
0xffffffc010093354, 3, 3, 1228447 - decay_load
0xffffffc01009338c, 3, 2, 1228447 - decay_load
0xffffffc01009336c, 3, 7, 1180051 - decay_load
0xffffffc010631300, 3, 4, 1114347 - __radix_tree_lookup
0xffffffc0106312c8, 3, 12, 1114337 - __radix_tree_lookup
0xffffffc0106312f8, 3, 2, 1114337 -
0xffffffc010132aec, 3, 4, 1080983
0xffffffc010132afc, 3, 12, 1080983
0xffffffc010132b30, 3, 2, 1080983
0x000000004084b58c, 1, 1, 1052116
0x000000004084b590, 1, 7, 1052116
0x000000004084b57c, 1, 4, 1051127
0xffffffc01001a118, 2, 6, 1049119
0xffffffc01001a944, 2, 2, 1048689
So whatever is holding it up is because it's heavily spamming core
functions.
--
Alex Bennée
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 0/8] GICv3 LPI and ITS feature implementation
2021-05-25 18:26 ` Alex Bennée
@ 2021-05-25 19:30 ` Alex Bennée
2021-05-27 0:22 ` shashi.mallela
0 siblings, 1 reply; 25+ messages in thread
From: Alex Bennée @ 2021-05-25 19:30 UTC (permalink / raw)
To: Shashi Mallela; +Cc: peter.maydell, leif, qemu-devel, qemu-arm, rad
Alex Bennée <alex.bennee@linaro.org> writes:
> Shashi Mallela <shashi.mallela@linaro.org> writes:
>
>> This patchset implements qemu device model for enabling physical
>> LPI support and ITS functionality in GIC as per GICv3 specification.
>> Both flat table and 2 level tables are implemented.The ITS commands
>> for adding/deleting ITS table entries,trigerring LPI interrupts are
>> implemented.Translated LPI interrupt ids are processed by redistributor
>> to determine priority and set pending state appropriately before
>> forwarding the same to cpu interface.
>> The ITS feature support has been added to sbsa-ref platform as well as
>> virt platform,wherein the emulated functionality co-exists with kvm
>> kernel functionality.
>
> So I'm definitely seeing a slow down in one of my testcases but it
> doesn't seem to be HW access related. Via:
>
<snip>
>
> So I ran with the hotblocks plugin:
>
> ./qemu-system-aarch64 -cpu max,pauth-impdef=on -machine type=virt,virtualization=on,gic-version=3 -display none -serial mon:stdio -kernel ~/lsrc/linux.git/builds/arm64.initramfs/arch/arm64/boot/Image -append "console=ttyAMA0" -m 4096 -smp 1 -plugin contrib/plugins/libhotblocks.so -d plugin -D hotblocks.log
>
> collected 130606 entries in the hash table
> pc, tcount, icount, ecount
> 0xffffffc010627fd0, 4, 10, 3998721 - memcpy
> 0xffffffc010628288, 2, 6, 3984790 - memset
> 0xffffffc01062832c, 3, 4, 1812870 - memset
> 0xffffffc0100a8df8, 4, 4, 1743432 - __my_cpu_offset
> 0xffffffc01015c394, 2, 4, 1304617 - __my_cpu_offset
> 0xffffffc010093348, 3, 3, 1228845 - decay_load
> 0xffffffc010093354, 3, 3, 1228447 - decay_load
> 0xffffffc01009338c, 3, 2, 1228447 - decay_load
> 0xffffffc01009336c, 3, 7, 1180051 - decay_load
> 0xffffffc010631300, 3, 4, 1114347 - __radix_tree_lookup
> 0xffffffc0106312c8, 3, 12, 1114337 - __radix_tree_lookup
> 0xffffffc0106312f8, 3, 2, 1114337 -
> 0xffffffc010132aec, 3, 4, 1080983
> 0xffffffc010132afc, 3, 12, 1080983
> 0xffffffc010132b30, 3, 2, 1080983
> 0x000000004084b58c, 1, 1, 1052116
> 0x000000004084b590, 1, 7, 1052116
> 0x000000004084b57c, 1, 4, 1051127
> 0xffffffc01001a118, 2, 6, 1049119
> 0xffffffc01001a944, 2, 2, 1048689
>
> So whatever is holding it up is because it's heavily spamming core
> functions.
Well given I've seen it hit gic_handle_irq > 1000 times already while in
the "PCI: CLS 0 bytes, default 64" phase of the kernel boot makes me
think the IRQs are just re-asserting themselves and firing continuously.
Indeed -d trace:gicv3_redist_set_irq shows a lot of:
gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level changed to 0
gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level changed to 1
gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level changed to 0
gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level changed to 1
gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level changed to 0
gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level changed to 1
--
Alex Bennée
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 0/8] GICv3 LPI and ITS feature implementation
2021-05-25 19:30 ` Alex Bennée
@ 2021-05-27 0:22 ` shashi.mallela
0 siblings, 0 replies; 25+ messages in thread
From: shashi.mallela @ 2021-05-27 0:22 UTC (permalink / raw)
To: Alex Bennée; +Cc: peter.maydell, leif, qemu-devel, qemu-arm, rad
Have identified the cause of slow down and redesigned the code to scan
LPI pending table and config table right after lpi pending state
changes(SET/RESET) through gicv3_redist_update_lpi() call to determine
the highest priority lpi among the active lpis and save the details.The
high priority interrupt determination logic in redistributor now uses
the saved high priority lpi details (alongside other interrupt types)
instead of calling gicv3_redist_update_lpi() everytime(as in current
design).This significantly reduces the call overhead associated with
address_space_read of lpi config and pending tables.Testing with this
new design showed no boot delays.
Will share changes in next patch version v4.
Thanks
Shashi
On Tue, 2021-05-25 at 20:30 +0100, Alex Bennée wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
>
> > Shashi Mallela <shashi.mallela@linaro.org> writes:
> >
> > > This patchset implements qemu device model for enabling physical
> > > LPI support and ITS functionality in GIC as per GICv3
> > > specification.
> > > Both flat table and 2 level tables are implemented.The ITS
> > > commands
> > > for adding/deleting ITS table entries,trigerring LPI interrupts
> > > are
> > > implemented.Translated LPI interrupt ids are processed by
> > > redistributor
> > > to determine priority and set pending state appropriately before
> > > forwarding the same to cpu interface.
> > > The ITS feature support has been added to sbsa-ref platform as
> > > well as
> > > virt platform,wherein the emulated functionality co-exists with
> > > kvm
> > > kernel functionality.
> >
> > So I'm definitely seeing a slow down in one of my testcases but it
> > doesn't seem to be HW access related. Via:
> >
> <snip>
> > So I ran with the hotblocks plugin:
> >
> > ./qemu-system-aarch64 -cpu max,pauth-impdef=on -machine
> > type=virt,virtualization=on,gic-version=3 -display none -serial
> > mon:stdio -kernel
> > ~/lsrc/linux.git/builds/arm64.initramfs/arch/arm64/boot/Image
> > -append "console=ttyAMA0" -m 4096 -smp 1 -plugin
> > contrib/plugins/libhotblocks.so -d plugin -D hotblocks.log
> >
> > collected 130606 entries in the hash table
> > pc, tcount, icount, ecount
> > 0xffffffc010627fd0, 4, 10, 3998721 - memcpy
> > 0xffffffc010628288, 2, 6, 3984790 - memset
> > 0xffffffc01062832c, 3, 4, 1812870 - memset
> > 0xffffffc0100a8df8, 4, 4, 1743432 - __my_cpu_offset
> > 0xffffffc01015c394, 2, 4, 1304617 - __my_cpu_offset
> > 0xffffffc010093348, 3, 3, 1228845 - decay_load
> > 0xffffffc010093354, 3, 3, 1228447 - decay_load
> > 0xffffffc01009338c, 3, 2, 1228447 - decay_load
> > 0xffffffc01009336c, 3, 7, 1180051 - decay_load
> > 0xffffffc010631300, 3, 4, 1114347 - __radix_tree_lookup
> > 0xffffffc0106312c8, 3, 12, 1114337 - __radix_tree_lookup
> > 0xffffffc0106312f8, 3, 2, 1114337 -
> > 0xffffffc010132aec, 3, 4, 1080983
> > 0xffffffc010132afc, 3, 12, 1080983
> > 0xffffffc010132b30, 3, 2, 1080983
> > 0x000000004084b58c, 1, 1, 1052116
> > 0x000000004084b590, 1, 7, 1052116
> > 0x000000004084b57c, 1, 4, 1051127
> > 0xffffffc01001a118, 2, 6, 1049119
> > 0xffffffc01001a944, 2, 2, 1048689
> >
> > So whatever is holding it up is because it's heavily spamming core
> > functions.
>
> Well given I've seen it hit gic_handle_irq > 1000 times already while
> in
> the "PCI: CLS 0 bytes, default 64" phase of the kernel boot makes me
> think the IRQs are just re-asserting themselves and firing
> continuously.
>
> Indeed -d trace:gicv3_redist_set_irq shows a lot of:
>
> gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level
> changed to 0
> gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level
> changed to 1
> gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level
> changed to 0
> gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level
> changed to 1
> gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level
> changed to 0
> gicv3_redist_set_irq GICv3 redistributor 0x0 interrupt 26 level
> changed to 1
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 0/8] GICv3 LPI and ITS feature implementation
2021-05-18 14:46 ` Peter Maydell
@ 2021-06-02 17:55 ` Shashi Mallela
0 siblings, 0 replies; 25+ messages in thread
From: Shashi Mallela @ 2021-06-02 17:55 UTC (permalink / raw)
To: Peter Maydell
Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki
[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]
This is due to missing IORT acpi data,applicable with latest ITS changes.
Will be sharing a separate review with new files.
On May 18 2021, at 10:46 am, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
> >
> > This patchset implements qemu device model for enabling physical
> > LPI support and ITS functionality in GIC as per GICv3 specification.
> > Both flat table and 2 level tables are implemented.The ITS commands
> > for adding/deleting ITS table entries,trigerring LPI interrupts are
> > implemented.Translated LPI interrupt ids are processed by redistributor
> > to determine priority and set pending state appropriately before
> > forwarding the same to cpu interface.
> > The ITS feature support has been added to sbsa-ref platform as well as
> > virt platform,wherein the emulated functionality co-exists with kvm
> > kernel functionality.
> >
> > Changes in v3:
> > - review comments addressed
> >
> > Shashi Mallela (8):
> > hw/intc: GICv3 ITS initial framework
> > hw/intc: GICv3 ITS register definitions added
> > hw/intc: GICv3 ITS command queue framework
> > hw/intc: GICv3 ITS Command processing
> > hw/intc: GICv3 ITS Feature enablement
> > hw/intc: GICv3 redistributor ITS processing
> > hw/arm/sbsa-ref: add ITS support in SBSA GIC
> > hw/arm/virt: add ITS support in virt GIC
>
> Something in here breaks "make check":
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-from-laptop/qemu/tests/dbus-vmstate-daemon.sh
> QTEST_QEMU_BINARY=./qemu-system-aarch64 tests/qtest/bios-tables-test
> --tap -k
>
> Looking for expected file 'tests/data/acpi/virt/FACP'
> Using expected file 'tests/data/acpi/virt/FACP'
> Looking for expected file 'tests/data/acpi/virt/APIC'
> Using expected file 'tests/data/acpi/virt/APIC'
> Looking for expected file 'tests/data/acpi/virt/GTDT'
> Using expected file 'tests/data/acpi/virt/GTDT'
> Looking for expected file 'tests/data/acpi/virt/MCFG'
> Using expected file 'tests/data/acpi/virt/MCFG'
> Looking for expected file 'tests/data/acpi/virt/SPCR'
> Using expected file 'tests/data/acpi/virt/SPCR'
> Looking for expected file 'tests/data/acpi/virt/IORT'
> **
> ERROR:../../tests/qtest/bios-tables-test.c:385:load_expected_aml:
> assertion failed: (exp_sdt.aml_file)
> ERROR qtest-aarch64/bios-tables-test - Bail out!
> ERROR:../../tests/qtest/bios-tables-test.c:385:load_expected_aml:
> assertion failed: (exp_sdt.aml_file)
>
> (and then it hangs)
> -- PMM
[-- Attachment #2: Type: text/html, Size: 3191 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-06-02 17:57 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 23:41 [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Shashi Mallela
2021-04-29 23:41 ` [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework Shashi Mallela
2021-05-18 13:52 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 2/8] hw/intc: GICv3 ITS register definitions added Shashi Mallela
2021-05-18 14:27 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 3/8] hw/intc: GICv3 ITS command queue framework Shashi Mallela
2021-05-18 15:43 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 4/8] hw/intc: GICv3 ITS Command processing Shashi Mallela
2021-05-18 15:49 ` Peter Maydell
2021-05-25 17:57 ` shashi.mallela
2021-04-29 23:41 ` [PATCH v3 5/8] hw/intc: GICv3 ITS Feature enablement Shashi Mallela
2021-05-18 15:58 ` Peter Maydell
2021-04-29 23:41 ` [PATCH v3 6/8] hw/intc: GICv3 redistributor ITS processing Shashi Mallela
2021-05-20 11:01 ` Peter Maydell
2021-05-25 17:58 ` shashi.mallela
2021-04-29 23:42 ` [PATCH v3 7/8] hw/arm/sbsa-ref: add ITS support in SBSA GIC Shashi Mallela
2021-05-18 16:03 ` Peter Maydell
2021-04-29 23:42 ` [PATCH v3 8/8] hw/arm/virt: add ITS support in virt GIC Shashi Mallela
2021-05-18 16:06 ` Peter Maydell
2021-05-18 13:41 ` [PATCH v3 0/8] GICv3 LPI and ITS feature implementation Peter Maydell
2021-05-18 14:46 ` Peter Maydell
2021-06-02 17:55 ` Shashi Mallela
2021-05-25 18:26 ` Alex Bennée
2021-05-25 19:30 ` Alex Bennée
2021-05-27 0:22 ` shashi.mallela
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.