Hi Peter, On 04/16/2018 03:08 PM, Peter Maydell wrote: > On 12 April 2018 at 08:37, Eric Auger <eric.au...@redhat.com> wrote: >> From: Prem Mallappa <prem.malla...@broadcom.com> >> >> This patch implements a skeleton for the smmuv3 device. >> Datatypes and register definitions are introduced. The MMIO >> region, the interrupts and the queue are initialized. >> >> Only the MMIO read operation is implemented here. >> >> Signed-off-by: Prem Mallappa <prem.malla...@broadcom.com> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> >> --- >> v10 -> v11: >> - remove irq_ctrl_ack and return irq_ctrl on A_IRQ_CTRL_ACK >> read >> >> v9 -> v10: >> - s/hwaddr/uint64_t in trace-events >> - add comments >> - s->mrtypename = TYPE_SMMUV3_IOMMU_MEMORY_REGION >> - removed iidr and idr from VMState >> - use VMSTATE_STRUCT for the queues >> - use qemu_log_mask(LOG_UNIMP,*) for unimplemented regs >> - added SMMU_CMDQS, SMMU_EVENTQS >> - use ops with attributes >> - split readl/readll >> - put id_regs in an array >> - removed smmu_read64 >> - removed SMMU_FEATURE_2LVL_STE and NB_REGS >> - RAZ when read access at unexpected address >> >> v8 -> v9: >> - add #include "qemu/log.h" >> - add parent_reset >> >> v7 -> v8: >> - remove __smmu_data structs >> - revisit struct SMMUQueue >> - do not advertise stage 2 support anymore >> - use the register definition API and get rid of REG array >> - get read of queue structs >> >> v6 -> v7: >> - split into several patches >> >> v5 -> v6: >> - Use IOMMUMemoryregion >> - regs become uint32_t and fix 64b MMIO access (.impl) >> - trace_smmuv3_write/read_mmio take the size param >> >> v4 -> v5: >> - change smmuv3_translate proto (IOMMUAccessFlags flag) >> - has_stagex replaced by is_ste_stagex >> - smmu_cfg_populate removed >> - added smmuv3_decode_config and reworked error management >> - remwork the naming of IOMMU mrs >> - fix SMMU_CMDQ_CONS offset >> >> v3 -> v4 >> - smmu_irq_update >> - fix hash key allocation >> - set smmu_iommu_ops >> - set SMMU_REG_CR0, >> - smmuv3_translate: ret.perm not set in bypass mode >> - use trace events >> - renamed STM2U64 into L1STD_L2PTR and STMSPAN into L1STD_SPAN >> - rework smmu_find_ste >> - fix tg2granule in TT0/0b10 corresponds to 16kB >> >> v2 -> v3: >> - move creation of include/hw/arm/smmuv3.h to this patch to fix compil issue >> - compilation allowed >> - fix sbus allocation in smmu_init_pci_iommu >> - restructure code into headers >> - misc cleanups >> >> Conflicts: >> hw/arm/Makefile.objs >> --- >> hw/arm/Makefile.objs | 2 +- >> hw/arm/smmuv3-internal.h | 167 ++++++++++++++++++++++ >> hw/arm/smmuv3.c | 365 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> hw/arm/trace-events | 3 + >> include/hw/arm/smmuv3.h | 87 +++++++++++ >> 5 files changed, 623 insertions(+), 1 deletion(-) >> create mode 100644 hw/arm/smmuv3-internal.h >> create mode 100644 hw/arm/smmuv3.c >> create mode 100644 include/hw/arm/smmuv3.h >> >> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs >> index 558436f..d51fcec 100644 >> --- a/hw/arm/Makefile.objs >> +++ b/hw/arm/Makefile.objs >> @@ -35,4 +35,4 @@ obj-$(CONFIG_MPS2) += mps2-tz.o >> obj-$(CONFIG_MSF2) += msf2-soc.o msf2-som.o >> obj-$(CONFIG_IOTKIT) += iotkit.o >> obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o >> -obj-$(CONFIG_ARM_SMMUV3) += smmu-common.o >> +obj-$(CONFIG_ARM_SMMUV3) += smmu-common.o smmuv3.o >> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h >> new file mode 100644 >> index 0000000..a6461fe >> --- /dev/null >> +++ b/hw/arm/smmuv3-internal.h >> @@ -0,0 +1,167 @@ >> +/* >> + * ARM SMMUv3 support - Internal API >> + * >> + * Copyright (C) 2014-2016 Broadcom Corporation >> + * Copyright (c) 2017 Red Hat, Inc. >> + * Written by Prem Mallappa, Eric Auger >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License along >> + * with this program; if not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#ifndef HW_ARM_SMMU_V3_INTERNAL_H >> +#define HW_ARM_SMMU_V3_INTERNAL_H >> + >> +#include "qemu/log.h" >> +#include "trace.h" >> +#include "qemu/error-report.h" > > I don't think you use these headers in this .h file -- they should > probably be included from the .c file(s) instead. indeed > >> +#include "hw/arm/smmu-common.h" >> + >> +/* MMIO Registers */ >> + >> +REG32(IDR0, 0x0) >> + FIELD(IDR0, S1P, 1 , 1) >> + FIELD(IDR0, TTF, 2 , 2) >> + FIELD(IDR0, COHACC, 4 , 1) >> + FIELD(IDR0, ASID16, 12, 1) >> + FIELD(IDR0, TTENDIAN, 21, 2) >> + FIELD(IDR0, STALL_MODEL, 24, 2) >> + FIELD(IDR0, TERM_MODEL, 26, 1) >> + FIELD(IDR0, STLEVEL, 27, 2) >> + >> +REG32(IDR1, 0x4) >> + FIELD(IDR1, SIDSIZE, 0 , 6) >> + FIELD(IDR1, EVENTQS, 16, 5) >> + FIELD(IDR1, CMDQS, 21, 5) >> + >> +#define SMMU_IDR1_SIDSIZE 16 >> +#define SMMU_CMDQS 19 >> +#define SMMU_EVENTQS 19 >> + >> +REG32(IDR2, 0x8) >> +REG32(IDR3, 0xc) >> +REG32(IDR4, 0x10) >> +REG32(IDR5, 0x14) >> + FIELD(IDR5, OAS, 0, 3); >> + FIELD(IDR5, GRAN4K, 4, 1); >> + FIELD(IDR5, GRAN16K, 5, 1); >> + FIELD(IDR5, GRAN64K, 6, 1); >> + >> +#define SMMU_IDR5_OAS 4 >> + >> +REG32(IIDR, 0x1c) >> +REG32(CR0, 0x20) >> + FIELD(CR0, SMMU_ENABLE, 0, 1) >> + FIELD(CR0, EVENTQEN, 2, 1) >> + FIELD(CR0, CMDQEN, 3, 1) >> + >> +REG32(CR0ACK, 0x24) >> +REG32(CR1, 0x28) >> +REG32(CR2, 0x2c) >> +REG32(STATUSR, 0x40) >> +REG32(IRQ_CTRL, 0x50) >> + FIELD(IRQ_CTRL, GERROR_IRQEN, 0, 1) >> + FIELD(IRQ_CTRL, PRI_IRQEN, 1, 1) >> + FIELD(IRQ_CTRL, EVENTQ_IRQEN, 2, 1) >> + >> +REG32(IRQ_CTRL_ACK, 0x54) >> +REG32(GERROR, 0x60) >> + FIELD(GERROR, CMDQ_ERR, 0, 1) >> + FIELD(GERROR, EVENTQ_ABT_ERR, 2, 1) >> + FIELD(GERROR, PRIQ_ABT_ERR, 3, 1) >> + FIELD(GERROR, MSI_CMDQ_ABT_ERR, 4, 1) >> + FIELD(GERROR, MSI_EVENTQ_ABT_ERR, 5, 1) >> + FIELD(GERROR, MSI_PRIQ_ABT_ERR, 6, 1) >> + FIELD(GERROR, MSI_GERROR_ABT_ERR, 7, 1) >> + FIELD(GERROR, MSI_SFM_ERR, 8, 1) >> + >> +REG32(GERRORN, 0x64) >> + >> +#define A_GERROR_IRQ_CFG0 0x68 /* 64b */ >> +REG32(GERROR_IRQ_CFG1, 0x70) >> +REG32(GERROR_IRQ_CFG2, 0x74) >> + >> +#define A_STRTAB_BASE 0x80 /* 64b */ > > Why do this constant and A_GERROR_IRQ_CFG0 have different values > but the same cryptic comment ? Those are 64-bit registers as opposed to other registers which are 32-bit, hence the comment. I did not see any register API primitives for those regs. > > >> +static inline uint64_t smmu_read64(uint64_t r, unsigned offset, >> + unsigned size) > > Looks like this function is unused now ? indeed. > > >> + /* >> + * Return the value of the Primecell/Corelink ID registers at the >> + * specified offset from the first ID register. >> + * These value indicate an ARM implementation of MMU600 p1 >> + */ >> + static const uint8_t smmuv3_ids[] = { >> + 0x4, 0, 0, 0, 0x84, 0xB4, 0xF0, 0x10, 0x0D, 0xF0, 0x05, 0xB1 > > Can you be consistent about whether you put the leading 0 in > for hex values less than 0x10 (0x4 vs 0x0D) ? sure > > > Otherwise > Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Thanks Eric > > thanks > -- PMM >
