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

[Alistair Francis <alistair.francis@xilinx.com>]

On Thu, Jun 23, 2016 at 6:08 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 June 2016 at 21:24, Alistair Francis <alistair.francis@xilinx.com> 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 <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>
>> +        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.

Fixed.

>
>> +        }
>> +        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.

Agreed. I have added the ability to unlock.

>
>> +    } 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...)

Fixed, it's now using the macro.

>
>
>> +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.

Removed.

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

Yes it should, I have fixed this as well.

Thanks,

Alistair

>
> thanks
> -- PMM
>