Re: [Qemu-arm] [PATCH v11 04/17] hw/arm/smmuv3: Skeleton

[Peter Maydell]

On 12 April 2018 at 08:37, Eric Auger <address@hidden> wrote:
> From: Prem Mallappa <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
> 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.

> +#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 ?


> +static inline uint64_t smmu_read64(uint64_t r, unsigned offset,
> +                                   unsigned size)

Looks like this function is unused now ?


> +    /*
> +     * 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) ?


Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM