Re: [Qemu-devel] [PATCH v9 04/14] hw/arm/smmuv3: Skeleton

[Peter Maydell]

On 17 February 2018 at 18:46, 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>


I just have a few minor nits on this one...


> +static inline int smmu_enabled(SMMUv3State *s)
> +{
> +    return FIELD_EX32(s->cr[0], CR0, SMMU_ENABLE);
> +}
> +
> +typedef struct Cmd {
> +    uint32_t word[4];
> +} Cmd;
> +
> +typedef struct Evt  {
> +    uint32_t word[8];
> +} Evt;

Some one-liner comments noting what these structs are for
would be helpful.

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

A doc comment here would help in describing what the purpose of
this utility function is.

> +{
> +    if (size == 8 && !offset) {
> +        return r;

If you take my advice a bit further down about just checking
up front that 8-byte accesses are to definitely permitted 64
bit register offsets, you won't need the check on offset.

> +    }
> +
> +    /* 32 bit access */
> +
> +    if (offset && offset != 4)  {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "SMMUv3 MMIO read: bad offset/size %u/%u\n",
> +                      offset, size);

This isn't a guest error, because the function is only ever called
with constant values for the 'offset' parameter. You should just
assert that the offset is 0 or 4.

> +        return 0;
> +    }
> +
> +    return extract64(r, offset << 3, 32);
> +}
> +
> +#endif

> +static void smmuv3_init_regs(SMMUv3State *s)
> +{
> +    /**
> +     * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> +     *       multi-level stream table
> +     */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1); /* stage 1 supported */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, COHACC, 1); /* IO coherent */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTENDIAN, 2); /* little endian */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STALL_MODEL, 1); /* No stall */
> +    /* terminated transaction will always be aborted/error returned */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TERM_MODEL, 1);
> +    /* 2-level stream table supported */
> +    s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, 1);
> +
> +    s->idr[1] = FIELD_DP32(s->idr[1], IDR1, SIDSIZE, SMMU_IDR1_SIDSIZE);
> +    s->idr[1] = FIELD_DP32(s->idr[1], IDR1, EVENTQS, 19);
> +    s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   19);
> +
> +   /* 4K and 64K granule support */
> +    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> +    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> +    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits 
> */
> +
> +    s->cmdq.base = deposit64(s->cmdq.base, 0, 5, 19); /* LOG2SIZE = 19 */
> +    s->cmdq.prod = 0;
> +    s->cmdq.cons = 0;
> +    s->cmdq.entry_size = sizeof(struct Cmd);
> +    s->eventq.base = deposit64(s->eventq.base, 0, 5, 19); /* LOG2SIZE = 19 */

Have some #defines for max cmd queue and event queue size, since
we use them here and also above in filling in the IDR fields ?

> +    s->eventq.prod = 0;
> +    s->eventq.cons = 0;
> +    s->eventq.entry_size = sizeof(struct Evt);
> +
> +    s->features = 0;
> +    s->sid_split = 0;
> +}
> +
> +static void smmu_write_mmio(void *opaque, hwaddr addr,
> +                            uint64_t val, unsigned size)
> +{
> +    /* not yet implemented */
> +}
> +
> +static uint64_t smmu_read_mmio(void *opaque, hwaddr addr, unsigned size)
> +{
> +    SMMUState *sys = opaque;
> +    SMMUv3State *s = ARM_SMMUV3(sys);
> +    uint64_t val;
> +
> +    /* CONSTRAINED UNPREDICTABLE choice to have page0/1 be exact aliases */
> +    addr &= ~0x10000;
> +
> +    if (size != 4 && size != 8) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "SMMUv3 MMIO read: bad size %u\n", 
> size);

Your MemoryRegionOps settings mean this can never happen, so you
don't need to check it at runtime.

> +        return 0;
> +    }

Consider specifically catching 8-byte accesses to non-64-bit registers?
This is CONSTRAINED UNPREDICTABLE (see spec section 6.2), and "one
of the registers is read/written and other half is RAZ/WI" is permitted
behaviour, but it does mean you need to be a little careful about not
letting the top 32 bits of val become non-zero for the 32-bit register
codepaths. Logging bad 64-bit accesses as LOG_GUEST_ERROR and making
them RAZ/WI might be nicer for guest software developers.

> +
> +    /* Primecell/Corelink ID registers */
> +    switch (addr) {
> +    case A_CIDR0:
> +        val = 0x0D;
> +        break;
> +    case A_CIDR1:
> +        val = 0xF0;
> +        break;
> +    case A_CIDR2:
> +        val = 0x05;
> +        break;
> +    case A_CIDR3:
> +        val = 0xB1;
> +        break;
> +    case A_PIDR0:
> +        val = 0x84; /* Part Number */
> +        break;
> +    case A_PIDR1:
> +        val = 0xB4; /* JEP106 ID code[3:0] for Arm and Part numver[11:8] */
> +        break;
> +    case A_PIDR3:
> +        val = 0x10; /* MMU600 p1 */
> +        break;
> +    case A_PIDR4:
> +        val = 0x4; /* 4KB region count, JEP106 continuation code for Arm */
> +        break;
> +    case 0xFD4 ... 0xFDC: /* SMMU_PDIR 5-7 */
> +        val = 0;
> +        break;

I usually put all the const CIDR/PIDR values in a const array, since
there are always 12 of them next to each other, but since you already
have this code it's fine.

> +    case A_IDR0 ... A_IDR5:
> +        val = s->idr[(addr - A_IDR0) / 4];
> +        break;
> +    case A_IIDR:
> +        val = s->iidr;
> +        break;
> +    case A_CR0:
> +        val = s->cr[0];
> +        break;
> +    case A_CR0ACK:
> +        val = s->cr0ack;
> +        break;
> +    case A_CR1:
> +        val = s->cr[1];
> +        break;
> +    case A_CR2:
> +        val = s->cr[2];
> +        break;
> +    case A_STATUSR:
> +        val = s->statusr;
> +        break;
> +    case A_IRQ_CTRL:
> +        val = s->irq_ctrl;
> +        break;
> +    case A_IRQ_CTRL_ACK:
> +        val = s->irq_ctrl_ack;
> +        break;
> +    case A_GERROR:
> +        val = s->gerror;
> +        break;
> +    case A_GERRORN:
> +        val = s->gerrorn;
> +        break;
> +    case A_GERROR_IRQ_CFG0: /* 64b */
> +        val = smmu_read64(s->gerror_irq_cfg0, 0, size);
> +        break;
> +    case A_GERROR_IRQ_CFG0 + 4:
> +        val = smmu_read64(s->gerror_irq_cfg0, 4, size);
> +        break;
> +    case A_GERROR_IRQ_CFG1:
> +        val = s->gerror_irq_cfg1;
> +        break;
> +    case A_GERROR_IRQ_CFG2:
> +        val = s->gerror_irq_cfg2;
> +        break;
> +    case A_STRTAB_BASE: /* 64b */
> +        val = smmu_read64(s->strtab_base, 0, size);
> +        break;
> +    case A_STRTAB_BASE + 4: /* 64b */
> +        val = smmu_read64(s->strtab_base, 4, size);
> +        break;
> +    case A_STRTAB_BASE_CFG:
> +        val = s->strtab_base_cfg;
> +        break;
> +    case A_CMDQ_BASE: /* 64b */
> +        val = smmu_read64(s->cmdq.base, 0, size);
> +        break;
> +    case A_CMDQ_BASE + 4:
> +        val = smmu_read64(s->cmdq.base, 4, size);
> +        break;
> +    case A_CMDQ_PROD:
> +        val = s->cmdq.prod;
> +        break;
> +    case A_CMDQ_CONS:
> +        val = s->cmdq.cons;
> +        break;
> +    case A_EVENTQ_BASE: /* 64b */
> +        val = smmu_read64(s->eventq.base, 0, size);
> +        break;
> +    case A_EVENTQ_BASE + 4: /* 64b */
> +        val = smmu_read64(s->eventq.base, 4, size);
> +        break;
> +    case A_EVENTQ_PROD:
> +        val = s->eventq.prod;
> +        break;
> +    case A_EVENTQ_CONS:
> +        val = s->eventq.cons;
> +        break;
> +    default:
> +        error_report("%s unhandled access at 0x%"PRIx64, __func__, addr);

This should be a LOG_GUEST_ERROR (if there are registers we don't
implement, you can define the A_* constants for them and have those
do a LOG_UNIMP.)

> +        break;
> +    }
> +
> +    trace_smmuv3_read_mmio(addr, val, size);
> +    return val;
> +}
> +

> +static void smmu_realize(DeviceState *d, Error **errp)
> +{
> +    SMMUState *sys = ARM_SMMU(d);
> +    SMMUv3State *s = ARM_SMMUV3(sys);
> +    SMMUv3Class *c = ARM_SMMUV3_GET_CLASS(s);
> +    SysBusDevice *dev = SYS_BUS_DEVICE(d);
> +    Error *local_err = NULL;
> +
> +    c->parent_realize(d, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    memory_region_init_io(&sys->iomem, OBJECT(s),
> +                          &smmu_mem_ops, sys, TYPE_ARM_SMMUV3, 0x20000);
> +
> +    sys->mrtypename = g_strdup(TYPE_SMMUV3_IOMMU_MEMORY_REGION);

Nothing ever modifies this later, so I don't think you nede to
do the g_strdup() ?  (The declaration of the struct field should
probably have a 'const'.)

> +
> +    sysbus_init_mmio(dev, &sys->iomem);
> +
> +    smmu_init_irq(s, dev);
> +}
> +
> +static const VMStateDescription vmstate_smmuv3 = {
> +    .name = "smmuv3",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(features, SMMUv3State),
> +        VMSTATE_UINT8(sid_size, SMMUv3State),
> +        VMSTATE_UINT8(sid_split, SMMUv3State),
> +
> +        VMSTATE_UINT32_ARRAY(idr, SMMUv3State, 6),

These are constant ID registers, right? You don't need to
migrate anything that's a fixed value set when the device
is created and never modified.

> +        VMSTATE_UINT32(iidr, SMMUv3State),
> +        VMSTATE_UINT32_ARRAY(cr, SMMUv3State, 3),
> +        VMSTATE_UINT32(cr0ack, SMMUv3State),
> +        VMSTATE_UINT32(statusr, SMMUv3State),
> +        VMSTATE_UINT32(irq_ctrl, SMMUv3State),
> +        VMSTATE_UINT32(irq_ctrl_ack, SMMUv3State),
> +        VMSTATE_UINT32(gerror, SMMUv3State),
> +        VMSTATE_UINT32(gerrorn, SMMUv3State),
> +        VMSTATE_UINT64(gerror_irq_cfg0, SMMUv3State),
> +        VMSTATE_UINT32(gerror_irq_cfg1, SMMUv3State),
> +        VMSTATE_UINT32(gerror_irq_cfg2, SMMUv3State),
> +        VMSTATE_UINT64(strtab_base, SMMUv3State),
> +        VMSTATE_UINT32(strtab_base_cfg, SMMUv3State),
> +        VMSTATE_UINT64(eventq_irq_cfg0, SMMUv3State),
> +        VMSTATE_UINT32(eventq_irq_cfg1, SMMUv3State),
> +        VMSTATE_UINT32(eventq_irq_cfg2, SMMUv3State),
> +
> +        VMSTATE_UINT64(cmdq.base, SMMUv3State),
> +        VMSTATE_UINT32(cmdq.prod, SMMUv3State),
> +        VMSTATE_UINT32(cmdq.cons, SMMUv3State),
> +        VMSTATE_UINT8(cmdq.entry_size, SMMUv3State),
> +        VMSTATE_UINT64(eventq.base, SMMUv3State),
> +        VMSTATE_UINT32(eventq.prod, SMMUv3State),
> +        VMSTATE_UINT32(eventq.cons, SMMUv3State),
> +        VMSTATE_UINT8(eventq.entry_size, SMMUv3State),

It's a little neater to define a separate VMStateDescription
for the SMMUQueue struct and then just use it twice here with
VMSTATE_STRUCT. (Example in hw/dma/pl330.c for vmstate_pl330_chan.)

Also, isn't the entry_size constant and fixed at device creation?
If so, you don't need to migrate it.

> +
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +static void smmuv3_instance_init(Object *obj)
> +{
> +    /* Nothing much to do here as of now */
> +}
> +
> +static void smmuv3_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SMMUv3Class *c = ARM_SMMUV3_CLASS(klass);
> +
> +    dc->vmsd    = &vmstate_smmuv3;

It would be nice to go through the patchset and remove these
unnecessary extra spaces in various assignments and declarations.

> +    device_class_set_parent_reset(dc, smmu_reset, &c->parent_reset);
> +    c->parent_realize = dc->realize;
> +    dc->realize = smmu_realize;
> +}


> +type_init(smmuv3_register_types)
> +

Stray blank line at end of file.

> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 3584974..64d2b9b 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -12,3 +12,6 @@ smmu_ptw_invalid_pte(int stage, int level, uint64_t 
> baseaddr, uint64_t pteaddr,
>  smmu_ptw_page_pte(int stage, int level,  uint64_t iova, uint64_t baseaddr, 
> uint64_t pteaddr, uint64_t pte, uint64_t address) "stage=%d level=%d 
> iova=0x%"PRIx64" address@hidden"PRIx64" address@hidden"PRIx64" 
> pte=0x%"PRIx64" page address = 0x%"PRIx64
>  smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t 
> pteaddr, uint64_t pte, uint64_t iova, uint64_t gpa, int bsize_mb) "stage=%d 
> level=%d address@hidden"PRIx64" address@hidden"PRIx64" pte=0x%"PRIx64" 
> iova=0x%"PRIx64" block address = 0x%"PRIx64" block size = %d MiB"
>  smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) 
> "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
> +
> +#hw/arm/smmuv3.c
> +smmuv3_read_mmio(hwaddr addr, uint64_t val, unsigned size) "addr: 
> 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x"

"hwaddr" isn't valid as a type for trace event parameters. This should
be uint64_t; otherwise trace backends like 'ust' won't build. (There's a
patch on list to tracetool that will make this mistake a compile error
for all backends, so it's easier to catch.)


> +#ifndef HW_ARM_SMMUV3_H
> +#define HW_ARM_SMMUV3_H
> +
> +#include "hw/arm/smmu-common.h"
> +#include "hw/registerfields.h"
> +
> +#define TYPE_SMMUV3_IOMMU_MEMORY_REGION "smmuv3-iommu-memory-region"
> +
> +#define SMMU_NREGS            0x200
> +
> +typedef struct SMMUQueue {
> +     hwaddr base;
> +     uint32_t prod;
> +     uint32_t cons;
> +     uint8_t entry_size;
> +} SMMUQueue;
> +
> +typedef struct SMMUv3State {
> +    SMMUState     smmu_state;
> +
> +    /* Local cache of most-frequently used registers */
> +#define SMMU_FEATURE_2LVL_STE (1 << 0)

Minor thing, I think it would be better for this define to
not be in the middle of the struct definition.

> +    uint32_t features;
> +    uint8_t sid_size;
> +    uint8_t sid_split;
> +
> +    uint32_t idr[6];
> +    uint32_t iidr;
> +    uint32_t cr[3];
> +    uint32_t cr0ack;
> +    uint32_t statusr;
> +    uint32_t irq_ctrl;
> +    uint32_t irq_ctrl_ack;
> +    uint32_t gerror;
> +    uint32_t gerrorn;
> +    uint64_t gerror_irq_cfg0;
> +    uint32_t gerror_irq_cfg1;
> +    uint32_t gerror_irq_cfg2;
> +    uint64_t strtab_base;
> +    uint32_t strtab_base_cfg;
> +    uint64_t eventq_irq_cfg0;
> +    uint32_t eventq_irq_cfg1;
> +    uint32_t eventq_irq_cfg2;
> +
> +    SMMUQueue eventq, cmdq;
> +
> +    qemu_irq     irq[4];
> +} SMMUv3State;
> +
> +typedef enum {
> +    SMMU_IRQ_EVTQ,
> +    SMMU_IRQ_PRIQ,
> +    SMMU_IRQ_CMD_SYNC,
> +    SMMU_IRQ_GERROR,
> +} SMMUIrq;
> +
> +typedef struct {
> +    /*< private >*/
> +    SMMUBaseClass smmu_base_class;
> +    /*< public >*/
> +
> +    DeviceRealize parent_realize;
> +    DeviceReset   parent_reset;
> +} SMMUv3Class;
> +
> +#define TYPE_ARM_SMMUV3   "arm-smmuv3"
> +#define ARM_SMMUV3(obj) OBJECT_CHECK(SMMUv3State, (obj), TYPE_ARM_SMMUV3)
> +#define ARM_SMMUV3_CLASS(klass)                              \
> +    OBJECT_CLASS_CHECK(SMMUv3Class, (klass), TYPE_ARM_SMMUV3)
> +#define ARM_SMMUV3_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(SMMUv3Class, (obj), TYPE_ARM_SMMUV3)
> +
> +#endif

thanks
-- PMM