Re: [PATCH v1 5/8] hw/misc: Introduce a model of Xilinx Versal's CFRAME_REG

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
<francisco.iglesias@amd.com> wrote:
>
> Introduce a model of Xilinx Versal's Configuration Frame controller
> (CFRAME_REG).
>
> Signed-off-by: Francisco Iglesias <francisco.iglesias@amd.com>
> ---
>  MAINTAINERS                              |   2 +
>  hw/misc/meson.build                      |   1 +
>  hw/misc/xlnx-versal-cframe-reg.c         | 714 +++++++++++++++++++++++
>  include/hw/misc/xlnx-versal-cframe-reg.h | 288 +++++++++
>  4 files changed, 1005 insertions(+)
>  create mode 100644 hw/misc/xlnx-versal-cframe-reg.c
>  create mode 100644 include/hw/misc/xlnx-versal-cframe-reg.h
>



> +#define KEYHOLE_STREAM_4K 0x1000

You could define this as (4 * KiB) which would then be more
clearly 4K.

> +
> +#define MAX_BLOCKTYPE 6
> +#define MAX_BLOCKTYPE_FRAMES 0xFFFFF
> +
> +enum {
> +    CFRAME_CMD_WCFG = 1,
> +    CFRAME_CMD_ROWON = 2,
> +    CFRAME_CMD_ROWOFF = 3,
> +    CFRAME_CMD_RCFG = 4,
> +    CFRAME_CMD_DLPARK = 5

minor style nit -- leaving the trailing ',' on the last
item in an enum or similar means that the next person
to add another entry doesn't have to edit the previous
line to put the comma in.

> +};

> +static void cfrm_fdri_post_write(RegisterInfo *reg, uint64_t val)
> +{
> +    XlnxVersalCFrameReg *s = XLNX_VERSAL_CFRAME_REG(reg->opaque);
> +
> +    if (s->row_configured && s->rowon && s->wcfg) {
> +        XlnxCFrame *new_f = &s->new_f;
> +
> +        new_f->data[new_f->idx++] = s->regs[R_FDRI0];
> +        new_f->data[new_f->idx++] = s->regs[R_FDRI1];
> +        new_f->data[new_f->idx++] = s->regs[R_FDRI2];
> +        new_f->data[new_f->idx++] = s->regs[R_FDRI3];
> +
> +        assert(new_f->idx <= FRAME_NUM_WORDS);

We should assert that we're not overrunning the array
*before* we write the data, not afterwards.

More generally, this kind of "we have a data array and
an index into it and we store stuff in at the index"
is prime territory for bugs resulting in array overruns.
If there's a way to write it that makes it clear that
that can't happen (esp. abstracting out what the operations
on the data structure are, documenting the invariants, etc)
then we should take it.

> +
> +        if (new_f->idx == FRAME_NUM_WORDS) {
> +            XlnxCFrame *cur_f;
> +
> +            /* Include block type and frame address */
> +            new_f->addr = extract32(s->regs[R_FAR0], 0, 23);
> +
> +            cur_f = cframes_get_frame(s, new_f->addr);
> +
> +            if (cur_f) {
> +                /* Overwrite current frame */
> +                cur_f[0] = new_f[0];
> +            } else {
> +                g_array_append_val(s->cframes, new_f[0]);
> +            }
> +
> +            cframe_incr_far(s);
> +
> +            /* Clear out new_f */
> +            memset(new_f, 0, sizeof(*new_f));
> +        }
> +    }
> +}

> +static int cframes_reg_pre_save(void *opaque)
> +{
> +    XlnxVersalCFrameReg *s = XLNX_VERSAL_CFRAME_REG(opaque);
> +
> +    if (s->cframes->len) {
> +        s->cf_data = (uint32_t *) s->cframes->data;
> +        s->cf_dlen = s->cframes->len * g_array_get_element_size(s->cframes) 
> / 4;
> +    }
> +    return 0;
> +}
> +
> +static int cframes_reg_post_load(void *opaque, int version_id)
> +{
> +    XlnxVersalCFrameReg *s = XLNX_VERSAL_CFRAME_REG(opaque);
> +
> +    if (s->cf_dlen) {
> +        uint32_t num_frames = s->cf_dlen /
> +                              (g_array_get_element_size(s->cframes) / 4);
> +        g_array_append_vals(s->cframes, s->cf_data, num_frames);
> +    }
> +    return 0;
> +}

Same remarks here about handling of GArray vs VMSTATE ALLOC.

(Or we could implement a VMSTATE macro/support for GArrays,
I guess. That would arguably be neater.)

> +
> +static const VMStateDescription vmstate_cframe_reg = {
> +    .name = TYPE_XLNX_VERSAL_CFRAME_REG,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .pre_save = cframes_reg_pre_save,
> +    .post_load = cframes_reg_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(wfifo, XlnxVersalCFrameReg, 4),
> +        VMSTATE_UINT32_ARRAY(regs, XlnxVersalCFrameReg, CFRAME_REG_R_MAX),
> +        VMSTATE_BOOL(rowon, XlnxVersalCFrameReg),
> +        VMSTATE_BOOL(wcfg, XlnxVersalCFrameReg),
> +        VMSTATE_BOOL(rcfg, XlnxVersalCFrameReg),
> +        VMSTATE_VARRAY_UINT32_ALLOC(cf_data, XlnxVersalCFrameReg, cf_dlen,
> +                                    0, vmstate_info_uint32, uint32_t),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
> +static Property cframe_regs_props[] = {
> +    /* Kept for backwards compatibility */

Backwards compatibility with who? This is the first implementation
of this as far as upstream is concerned...

> +    DEFINE_PROP_LINK("cfu", XlnxVersalCFrameReg, cfg.cfu_fdro,
> +                     TYPE_XLNX_CFI_IF, XlnxCfiIf *),
> +    DEFINE_PROP_LINK("cfu-fdro", XlnxVersalCFrameReg, cfg.cfu_fdro,
> +                     TYPE_XLNX_CFI_IF, XlnxCfiIf *),
> +    DEFINE_PROP_UINT32("blktype0-frames", XlnxVersalCFrameReg,
> +                       cfg.blktype_num_frames[0], 0),
> +    DEFINE_PROP_UINT32("blktype1-frames", XlnxVersalCFrameReg,
> +                       cfg.blktype_num_frames[1], 0),
> +    DEFINE_PROP_UINT32("blktype2-frames", XlnxVersalCFrameReg,
> +                       cfg.blktype_num_frames[2], 0),
> +    DEFINE_PROP_UINT32("blktype3-frames", XlnxVersalCFrameReg,
> +                       cfg.blktype_num_frames[3], 0),
> +    DEFINE_PROP_UINT32("blktype4-frames", XlnxVersalCFrameReg,
> +                       cfg.blktype_num_frames[4], 0),
> +    DEFINE_PROP_UINT32("blktype5-frames", XlnxVersalCFrameReg,
> +                       cfg.blktype_num_frames[5], 0),
> +    DEFINE_PROP_UINT32("blktype6-frames", XlnxVersalCFrameReg,
> +                       cfg.blktype_num_frames[6], 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +

thanks
-- PMM