On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
<[email protected]> wrote:
>
> Introduce a model of Xilinx Versal's Configuration Frame Unit's data out
> port (CFU_FDRO).
>
> Signed-off-by: Francisco Iglesias <[email protected]>
> ---
> hw/misc/xlnx-versal-cfu.c | 105 ++++++++++++++++++++++++++++++
> include/hw/misc/xlnx-versal-cfu.h | 11 ++++
> 2 files changed, 116 insertions(+)
>
> diff --git a/hw/misc/xlnx-versal-cfu.c b/hw/misc/xlnx-versal-cfu.c
> index cbd17d2351..528090ef1b 100644
> --- a/hw/misc/xlnx-versal-cfu.c
> +++ b/hw/misc/xlnx-versal-cfu.c
> @@ -257,6 +257,26 @@ static void cfu_stream_write(void *opaque, hwaddr addr,
> uint64_t value,
> }
> }
>
> +static uint64_t cfu_fdro_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(opaque);
> + uint64_t ret = 0;
> +
> + if (s->fdro_data->len) {
> + ret = g_array_index(s->fdro_data, uint32_t, 0);
> + g_array_remove_index(s->fdro_data, 0);
This is pretty expensive because everything in the GArray
after element 0 must be copied downwards. Are you sure you
don't want a different data structure ?
What actually is this, and what are the operations we want
to do on it ?
> + }
> +
> + return ret;
> +}
> +
> +static void cfu_fdro_write(void *opaque, hwaddr addr, uint64_t value,
> + unsigned size)
> +{
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Unsupported write from addr=%"
> + HWADDR_PRIx "\n", __func__, addr);
> +}
> +
> static const MemoryRegionOps cfu_stream_ops = {
> .read = cfu_stream_read,
> .write = cfu_stream_write,
> @@ -267,6 +287,16 @@ static const MemoryRegionOps cfu_stream_ops = {
> },
> };
>
> +static const MemoryRegionOps cfu_fdro_ops = {
> + .read = cfu_fdro_read,
> + .write = cfu_fdro_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid = {
> + .min_access_size = 4,
> + .max_access_size = 4,
> + },
> +};
> +
> static void cfu_apb_init(Object *obj)
> {
> XlnxVersalCFUAPB *s = XLNX_VERSAL_CFU_APB(obj);
> @@ -298,6 +328,24 @@ static void cfu_apb_init(Object *obj)
> sysbus_init_irq(sbd, &s->irq_cfu_imr);
> }
>
> +static void cfu_fdro_init(Object *obj)
> +{
> + XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(obj);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->iomem_fdro, obj, &cfu_fdro_ops, s,
> + TYPE_XLNX_VERSAL_CFU_FDRO, KEYHOLE_STREAM_4K);
> + sysbus_init_mmio(sbd, &s->iomem_fdro);
> + s->fdro_data = g_array_new(FALSE, FALSE, sizeof(uint32_t));
> +}
> +
> +static void cfu_fdro_cfi_transfer_packet(XlnxCfiIf *cfi_if, XlnxCfiPacket
> *pkt)
> +{
> + XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(cfi_if);
> +
> + g_array_append_vals(s->fdro_data, &pkt->data[0], 4);
> +}
> +
> static Property cfu_props[] = {
> DEFINE_PROP_LINK("cframe0", XlnxVersalCFUAPB, cfg.cframe[0],
> TYPE_XLNX_CFI_IF, XlnxCfiIf *),
> @@ -344,6 +392,41 @@ static const VMStateDescription vmstate_cfu_apb = {
> }
> };
>
> +static int cfdro_reg_pre_save(void *opaque)
> +{
> + XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(opaque);
> +
> + if (s->fdro_data->len) {
> + s->ro_data = (uint32_t *) s->fdro_data->data;
> + s->ro_dlen = s->fdro_data->len;
> + }
I think we need to initialise ro_data and ro_dlen in
the else case here as well. Otherwise they might have old
stale stuff in them that then goes into the migration stream.
> +
> + return 0;
> +}
> +
> +static int cfdro_reg_post_load(void *opaque, int version_id)
> +{
> + XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(opaque);
> +
> + if (s->ro_dlen) {
> + g_array_append_vals(s->fdro_data, s->ro_data, s->ro_dlen);
> + }
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_cfu_fdro = {
> + .name = TYPE_XLNX_VERSAL_CFU_FDRO,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .pre_save = cfdro_reg_pre_save,
> + .post_load = cfdro_reg_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_VARRAY_UINT32_ALLOC(ro_data, XlnxVersalCFUFDRO, ro_dlen,
> + 0, vmstate_info_uint32, uint32_t),
This kind of _ALLOC vmstate will cause the migration core
code to g_malloc() you a buffer for the data. We don't
free that anywhere (and if we have a subsequent vmsave
then we will overwrite the ro-data pointer, and leak the
memory).
It might be better to avoid the GArray and just directly
work with a g_malloc()'d buffer of our own, to fit better
with how the _ALLOC vmstate wants to work.
> + VMSTATE_END_OF_LIST(),
> + }
> +};
> +
> static void cfu_apb_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -353,6 +436,15 @@ static void cfu_apb_class_init(ObjectClass *klass, void
> *data)
> device_class_set_props(dc, cfu_props);
> }
>
> +static void cfu_fdro_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + XlnxCfiIfClass *xcic = XLNX_CFI_IF_CLASS(klass);
> +
> + dc->vmsd = &vmstate_cfu_fdro;
> + xcic->cfi_transfer_packet = cfu_fdro_cfi_transfer_packet;
> +}
Missing reset method ?
thanks
-- PMM
