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
