Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model

[Peter Crosthwaite]

Hi Anthony,

On Fri, May 31, 2013 at 5:41 AM, Anthony Liguori <address@hidden> wrote:
> Peter Crosthwaite <address@hidden> writes:
[snip]
>>> }
>>>
>>
>> That's still possible using just the register API (Patch 2 content
>> only) and throwing away the memory API glue. I think its actually
>> quite similar to V1 of the patch series where I did not have the
>> callbacks, and use the switch-cases for register side-effects only.
>> This looks like the intention of your patch. Gerd made a push for the
>> callbacks in an earlier review and there was push to integrate to
>> Memory API . Is it reasonable to leave it up to the developer whether
>> they want to do full conversion (Patches 2 & 3) or half conversion
>> (Patch 2 only + your decode refactoring). V1 of this patch at:
>>
>> http://patchwork.ozlabs.org/patch/224534/
>>
>> heres the relevant snippet (comments from me inline):
>>
>> +static void xilinx_devcfg_write(void *opaque, hwaddr addr, uint64_t value,
>> +                                unsigned size)
>> +{
>> +    XilinxDevcfg *s = opaque;
>> +    int i;
>> +    uint32_t aes_en;
>> +    const char *prefix = "";
>> +
>> +    /* FIXME: use tracing to avoid these gymnastics */
>> +    if (XILINX_DEVCFG_ERR_DEBUG || qemu_get_log_flags() & LOG_GUEST_ERROR) {
>> +        prefix = g_strdup_printf("%s:Addr %#08x",
>> +                                 object_get_canonical_path(OBJECT(s)),
>> +                                 (unsigned)addr);
>> +    }
>> +    addr >>= 2;
>>
>> This is the open coded decode logic but is trivial in this case.
>
> But this is invisible to the common layer.
>
> I think being able to have a common layer insight into "this value is
> being written to this device register" would be extremely useful.
>
> But my fear is that the current proposal will not work for a large set
> of devices.
>
> Let me provide another example (attached below) but highlighting the
> description:
>
>> static RegisterDecodeEntry decoder[] = {
>>     /* addresses 0-1 must be open decoded due to latching */
>>     { .addr = 2, .regno = UART_FCR, .flags = REG_WRITE },
>>     { .addr = 2, .regno = UART_IIR, .flags = REG_READ },

The TRM im reading (for the Xilinx 16550 serial) has the read decoding
of FCR vs IIR dependent on LCR, so I think these two are open decoded
as well.

>>     { .addr = 3, .regno = UART_LCR, .flags = REG_RW },
>>     { .addr = 4, .regno = UART_MCR, .flags = REG_RW },
>>     { .addr = 5, .regno = UART_LSR, .flags = REG_READ },
>>     { .addr = 6, .regno = UART_MSR, .flags = REG_READ },
>>     { .addr = 7, .regno = UART_SCR, .flags = REG_RW },
>> };
>>
>> static RegisterMapEntry regmap[] = {
>>     DEFINE_REG_U8(SerialState, ier, UART_IER),
>>     DEFINE_REG_U8(SerialState, iir, UART_IIR),
>>     DEFINE_REG_U8(SerialState, lcr, UART_LCR),
>>     DEFINE_REG_U8(SerialState, mcr, UART_MCR),
>>     DEFINE_REG_U8(SerialState, lsr, UART_LSR),
>>     DEFINE_REG_U8(SerialState, scr, UART_SCR),
>>     DEFINE_REG_U8(SerialState, thr, UART_THR),
>> };

I like it, but can we merge the two to avoid the replicated lists?
Append the decode information to the existing RegisterMapEntry (or
other way round if you want to think of it like that). I know there is
not a 1:1 correlation between decode lines and storage, but register
API already supports no-storage entries for this very purpose - you
can describe a "register" with no storage and define only side effects
(currently implemented via post_foo callbacks). Then we dont need the
enum to perform mapping from decode to register (except for the open
decode case).

Perhaps its better to think of the RegisterAccessInfo array in the
patch as corresponding to decode lines, rather than storage, with
optional storage information appended

Will send rework shortly.

Regards,
Peter

>
> This is a clear separation between decode logic and load/store logic.
> They are very different things from a hardware point of view.
>
>
>> +    assert(addr < R_MAX);
>> +
>> +    if (s->lock && addr != R_UNLOCK) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s:write to non-lock register while"
>> +                      " locked\n", prefix);
>> +        return;
>> +    }
>> +
>>
>> some pre write logic (I dropped it later as it was actually unimplemented).
>>
>> +    uint32_write(&s->regs[addr], &xilinx_devcfg_regs_info[addr], value, 
>> prefix,
>> +                 XILINX_DEVCFG_ERR_DEBUG);
>> +
>>
>> this is the data-driven register_write() call under its old name.
>>
>> +    /*side effects */
>> +    switch (addr) {
>> +    case R_CTRL:
>> +        for (i = 0; i < ARRAY_SIZE(lock_ctrl_map); ++i) {
>> +            if (s->regs[R_LOCK] & 1 << i) {
>> +                value &= ~lock_ctrl_map[i];
>> +                value |= lock_ctrl_map[i] & s->regs[R_CTRL];
>> +            }
>> +        }
>> +        aes_en = extract32(value, PCFG_AES_EN_SHIFT, PCFG_AES_EN_LEN);
>> +        if (aes_en != 0 && aes_en != 7) {
>> +            qemu_log_mask(LOG_UNIMP, "%s: warning, aes-en bits 
>> inconsistent,"
>> +                          "unimplemeneted security reset should happen!\n",
>> +                          prefix);
>> +        }
>> +        break;
>> +
>> +    case R_DMA_DST_LEN:
>> +        /* TODO: implement command queue overflow check and interrupt */
>> +        s->dma_command_fifo[s->dma_command_fifo_num].src_addr =
>> +                s->regs[R_DMA_SRC_ADDR] & ~0x3UL;
>> +        s->dma_command_fifo[s->dma_command_fifo_num].dest_addr =
>> +                s->regs[R_DMA_DST_ADDR] & ~0x3UL;
>> +        s->dma_command_fifo[s->dma_command_fifo_num].src_len =
>> +                s->regs[R_DMA_SRC_LEN] << 2;
>> +        s->dma_command_fifo[s->dma_command_fifo_num].dest_len =
>> +                s->regs[R_DMA_DST_LEN] << 2;
>> +        s->dma_command_fifo_num++;
>> +        DB_PRINT("dma transfer started; %d total transfers pending\n",
>> +                 s->dma_command_fifo_num);
>> +        xilinx_devcfg_dma_go(s);
>> +        break;
>> +
>> +    case R_UNLOCK:
>> +        if (value == R_UNLOCK_MAGIC) {
>> +            s->lock = 0;
>> +            DB_PRINT("successful unlock\n");
>> +        } else if (s->lock) {/* bad unlock attempt */
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:failed unlock\n", prefix);
>> +            s->regs[R_CTRL] &= ~PCAP_PR;
>> +            s->regs[R_CTRL] &= ~PCFG_AES_EN_MASK;
>> +        }
>> +        break;
>> +    }
>>
>> And the post-write logic.
>>
>> +    xilinx_devcfg_update_ixr(s);
>> +
>> +    if (*prefix) {
>> +        g_free((void *)prefix);
>> +    }
>> +}
>> +
>>
>>> It makes it much easier to convert existing code.  We can then leave
>>> most of the switch statements intact and just introduce register
>>> descriptions.
>>>
>>> I think splitting decode from data processing is useful too because it
>>> makes it easier to support latching.
>>>
>>> I think the current spin is probably over generalizing too.  I don't
>>> think supporting ge0/ge1 makes a lot of sense from the start.  Decisions
>>> aren't always that simple and it's weird to have sanity checking split
>>> across two places.
>>>
>>
>> My goal is to pretty much copy paste out the TRM for the clear GE
>> write. Some of my register fields in the TRM for this device say
>> things like "Reserved, always write as 1" which im trying to capture
>> in a data driven way without having to pollute my switch-cases with
>> this junk. It would be nice to autogenerate this as well.
>
> I think you're trying to solve too many problems at once.
>
> I don't think putting error messages in a register layout description is
> a good idea.
>
>
>>> BTW: I think it's also a good idea to model this as a QOM object so that
>>> device state can be access through the QOM tree.
>
> FWIW, I now think this is a bad idea :-)
>
> Here's the full example:
>
>
>
> Regards,
>
> Anthony Liguori
>
>>>
>>
>> Hmm ill have to think on this one.
>>
>> Regards,
>> Peter
>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>> +    },
>>>> +    [R_LOCK] = { .name = "LOCK",
>>>> +        .ro = ~ONES(5),
>>>> +        .pre_write = r_lock_pre_write,
>>>> +    },
>>>> +    [R_CFG] = { .name = "CFG",
>>>> +        .reset = 1 << RFIFO_TH_SHIFT | 1 << WFIFO_TH_SHIFT | 0x8,
>>>> +        .ge1 = (RegisterAccessError[]) {
>>>> +            { .mask = 0x7, .reason = "Reserved - do not modify" },
>>>> +            {},
>>>> +        },
>>>> +        .ge0 = (RegisterAccessError[]) {
>>>> +            { .mask = 0x8, .reason = "Reserved - do not modify" },
>>>> +            {},
>>>> +        },
>>>> +        .ro = 0x00f | ~ONES(12),
>>>> +    },
>>>> +    [R_INT_STS] = { .name = "INT_STS",
>>>> +        .w1c = ~R_INT_STS_RSVD,
>>>> +        .reset = PSS_GTS_USR_B_INT | PSS_CFG_RESET_B_INT | 
>>>> WR_FIFO_LVL_INT,
>>>> +        .ro = R_INT_STS_RSVD,
>>>> +        .post_write = r_ixr_post_write,
>>>> +    },
>>>> +    [R_INT_MASK] = { .name = "INT_MASK",
>>>> +        .reset = ~0,
>>>> +        .ro = R_INT_STS_RSVD,
>>>> +        .post_write = r_ixr_post_write,
>>>> +    },
>>>> +    [R_STATUS] = { .name = "STATUS",
>>>> +        .reset = DMA_CMD_Q_E | PSS_GTS_USR_B | PSS_CFG_RESET_B,
>>>> +        .ro = ~0,
>>>> +        .ge1 = (RegisterAccessError[])  {
>>>> +            {.mask = 0x1, .reason = "Reserved - do not modify" },
>>>> +            {},
>>>> +        },
>>>> +    },
>>>> +    [R_DMA_SRC_ADDR] = { .name = "DMA_SRC_ADDR" },
>>>> +    [R_DMA_DST_ADDR] = { .name = "DMA_DST_ADDR" },
>>>> +    [R_DMA_SRC_LEN] = { .name = "DMA_SRC_LEN", .ro = ~ONES(27) },
>>>> +    [R_DMA_DST_LEN] = { .name = "DMA_DST_LEN",
>>>> +        .ro = ~ONES(27),
>>>> +        .post_write = r_dma_dst_len_post_write,
>>>> +    },
>>>> +    [R_ROM_SHADOW] = { .name = "ROM_SHADOW",
>>>> +        .ge1 = (RegisterAccessError[])  {
>>>> +            {.mask = ~0, .reason = "Reserved - do not modify" },
>>>> +            {},
>>>> +        },
>>>> +    },
>>>> +    [R_SW_ID] = { .name = "SW_ID" },
>>>> +    [R_UNLOCK] = { .name = "UNLOCK",
>>>> +        .post_write = r_unlock_post_write,
>>>> +    },
>>>> +    [R_MCTRL] = { .name = "MCTRL",
>>>> +        /* Silicon 3.0 for version field, and the mysterious reserved bit 
>>>> 23 */
>>>> +        .reset = 0x2 << PS_VERSION_SHIFT | 1 << 23,
>>>> +        /* some reserved bits are rw while others are ro */
>>>> +        .ro = ~INT_PCAP_LPBK,
>>>> +        .ge1 = (RegisterAccessError[]) {
>>>> +            { .mask = 0x00F00300, .reason = "Reserved - do not modify" },
>>>> +            { .mask = 0x00000003, .reason = "Reserved - always write with 
>>>> 0" },
>>>> +            {}
>>>> +        },
>>>> +        .ge0 = (RegisterAccessError[]) {
>>>> +            { .mask = 1 << 23, .reason = "Reserved - always write with 1" 
>>>> },
>>>> +            {}
>>>> +        },
>>>> +    },
>>>> +    [R_MAX] = {}
>>>> +};
>>>> +
>>>> +static const MemoryRegionOps devcfg_reg_ops = {
>>>> +    .read = register_read_memory_le,
>>>> +    .write = register_write_memory_le,
>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>> +    .valid = {
>>>> +        .min_access_size = 4,
>>>> +        .max_access_size = 4,
>>>> +    }
>>>> +};
>>>> +
>>>> +static void xilinx_devcfg_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    XilinxDevcfg *s = XILINX_DEVCFG(dev);
>>>> +    const char *prefix = object_get_canonical_path(OBJECT(dev));
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < R_MAX; ++i) {
>>>> +        RegisterInfo *r = &s->regs_info[i];
>>>> +
>>>> +        *r = (RegisterInfo) {
>>>> +            .data = &s->regs[i],
>>>> +            .data_size = sizeof(uint32_t),
>>>> +            .access = &xilinx_devcfg_regs_info[i],
>>>> +            .debug = XILINX_DEVCFG_ERR_DEBUG,
>>>> +            .prefix = prefix,
>>>> +            .opaque = s,
>>>> +        };
>>>> +        memory_region_init_io(&r->mem, &devcfg_reg_ops, r, "devcfg-regs", 
>>>> 4);
>>>> +        memory_region_add_subregion(&s->iomem, i * 4, &r->mem);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void xilinx_devcfg_init(Object *obj)
>>>> +{
>>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>>> +    XilinxDevcfg *s = XILINX_DEVCFG(obj);
>>>> +
>>>> +    s->timer_bh = qemu_bh_new(xilinx_devcfg_dma_go, s);
>>>> +    s->timer = ptimer_init(s->timer_bh);
>>>> +
>>>> +    sysbus_init_irq(sbd, &s->irq);
>>>> +
>>>> +    memory_region_init(&s->iomem, "devcfg", R_MAX*4);
>>>> +    sysbus_init_mmio(sbd, &s->iomem);
>>>> +}
>>>> +
>>>> +static void xilinx_devcfg_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +
>>>> +    dc->reset = xilinx_devcfg_reset;
>>>> +    dc->vmsd = &vmstate_xilinx_devcfg;
>>>> +    dc->realize = xilinx_devcfg_realize;
>>>> +}
>>>> +
>>>> +static const TypeInfo xilinx_devcfg_info = {
>>>> +    .name           = TYPE_XILINX_DEVCFG,
>>>> +    .parent         = TYPE_SYS_BUS_DEVICE,
>>>> +    .instance_size  = sizeof(XilinxDevcfg),
>>>> +    .instance_init  = xilinx_devcfg_init,
>>>> +    .class_init     = xilinx_devcfg_class_init,
>>>> +};
>>>> +
>>>> +static void xilinx_devcfg_register_types(void)
>>>> +{
>>>> +    type_register_static(&xilinx_devcfg_info);
>>>> +}
>>>> +
>>>> +type_init(xilinx_devcfg_register_types)
>>>> --
>>>> 1.8.3.rc1.44.gb387c77.dirty
>>>
>