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