Re: [Qemu-devel] [PATCH v7 07/12] dma: Add Xilinx Zynq devcfg device model

[Peter Maydell]

On 22 June 2016 at 21:24, Alistair Francis <address@hidden> wrote:
> Add a minimal model for the devcfg device which is part of Zynq.
> This model supports DMA capabilities and interrupt generation.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> Signed-off-by: Alistair Francis <address@hidden>

> +        if (!dmah->src_len && !dmah->dest_len) {
> +            DB_PRINT("dma operation finished\n");
> +            s->regs[R_INT_STS] |= R_INT_STS_DMA_DONE_MASK |
> +                                  R_INT_STS_DMA_P_DONE_MASK;
> +            s->dma_cmd_fifo_num--;
> +            memmove(s->dma_cmd_fifo, &s->dma_cmd_fifo[1],
> +                    sizeof(s->dma_cmd_fifo));

This looks like an off-by-one error in the size argument.

> +        }
> +        xlnx_zynq_devcfg_update_ixr(s);
> +    } while (s->dma_cmd_fifo_num);
> +}

> +static void r_unlock_post_write(RegisterInfo *reg, uint64_t val)
> +{
> +    XlnxZynqDevcfg *s = XLNX_ZYNQ_DEVCFG(reg->opaque);
> +    const char *device_prefix = object_get_typename(OBJECT(s));
> +
> +    if (val == R_UNLOCK_MAGIC) {
> +        DB_PRINT("successful unlock\n");
> +        /* BootROM will have already done the actual unlock so no need to do
> +         * anything in successful subsequent unlock
> +         */

I don't understand this comment. Shouldn't we be marking the
memory region as enabled here, to handle the case of
"guest locks the device via a bad-unlock-attempt; guest
unlocks via a good unlock attempt" ?  What the guest bootrom
does or doesn't do isn't usually relevant to the device
implementation.

> +    } else { /* bad unlock attempt */
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed unlock\n", device_prefix);
> +        s->regs[R_CTRL] &= ~R_CTRL_PCAP_PR_MASK;
> +        s->regs[R_CTRL] &= ~R_CTRL_PCFG_AES_EN_MASK;
> +        /* core becomes inaccessible */
> +        memory_region_set_enabled(&s->iomem, false);
> +    }
> +}

> +
> +static const RegisterAccessInfo xlnx_zynq_devcfg_regs_info[] = {
> +    {   .name = "CTRL",                 .addr = A_CTRL,
> +        .reset = R_CTRL_PCAP_PR_MASK | R_CTRL_PCAP_MODE_MASK | 0x3 << 13,
> +        .rsvd = 0x1 << 28 | 0x3ff << 13 | 0x3 << 13,
> +        .pre_write = r_ctrl_pre_write,
> +        .post_write = r_ctrl_post_write,
> +    },
> +    {   .name = "LOCK",                 .addr = A_LOCK,
> +        .rsvd = MAKE_64BIT_MASK(5, 64 - 5),
> +        .pre_write = r_lock_pre_write,
> +    },
> +    {   .name = "CFG",                  .addr = A_CFG,
> +        .reset = 1 << R_CFG_RFIFO_TH_SHIFT | 1 << R_CFG_WFIFO_TH_SHIFT | 0x8,

I was expecting this to use the R_CFG_RESET value defined earlier
(or alternatively don't bother defining a #define for it...)


> +static const VMStateDescription vmstate_xlnx_zynq_devcfg = {
> +    .name = "xlnx_zynq_devcfg",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,

You don't need an _old field.

> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_ARRAY(dma_cmd_fifo, XlnxZynqDevcfg,
> +                             XLNX_ZYNQ_DEVCFG_DMA_CMD_FIFO_LEN, 0,
> +                             vmstate_xlnx_zynq_devcfg_dma_cmd,
> +                             XlnxZynqDevcfgDMACmd),
> +        VMSTATE_UINT8(dma_cmd_fifo_num, XlnxZynqDevcfg),
> +        VMSTATE_UINT32_ARRAY(regs, XlnxZynqDevcfg, XLNX_ZYNQ_DEVCFG_R_MAX),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};


> diff --git a/include/hw/register.h b/include/hw/register.h
> index 104d381..3e4d1ae 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -208,7 +208,7 @@ MemoryRegion *register_init_block32(DeviceState *owner,
>      enum { R_ ## reg ## _ ## field ## _SHIFT = (shift)};                  \
>      enum { R_ ## reg ## _ ## field ## _LENGTH = (length)};                \
>      enum { R_ ## reg ## _ ## field ## _MASK =                             \
> -                                        MAKE_64BIT_MASK(shift, length);
> +                                        MAKE_64BIT_MASK(shift, length)};
>
>  /* Extract a field from a register */
>  #define FIELD_EX32(storage, reg, field)                                   \

Should this hunk be in some earlier patch ?

thanks
-- PMM