| Message ID | 20230710140249.56324-4-francisco.iglesias@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Xilinx Versal CFI support | expand |
On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias <francisco.iglesias@amd.com> wrote: > > Introduce a model of Xilinx Versal's Configuration Frame Unit's data out > port (CFU_FDRO). > > Signed-off-by: Francisco Iglesias <francisco.iglesias@amd.com> > --- > 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
Hi Peter, On 2023-08-03 15:48, Peter Maydell wrote: > On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias > <francisco.iglesias@amd.com> wrote: >> >> Introduce a model of Xilinx Versal's Configuration Frame Unit's data out >> port (CFU_FDRO). >> >> Signed-off-by: Francisco Iglesias <francisco.iglesias@amd.com> >> --- >> 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 ? Thank you very much for reviewing! Regarding above, it is a fifo so changed to use a Fifo32 in v2 and I also tried to update according to all other comments! Best regards, Francisco Iglesias > >> + } >> + >> + 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
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); + } + + 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; + } + + 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), + 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; +} + static const TypeInfo cfu_apb_info = { .name = TYPE_XLNX_VERSAL_CFU_APB, .parent = TYPE_SYS_BUS_DEVICE, @@ -365,9 +457,22 @@ static const TypeInfo cfu_apb_info = { } }; +static const TypeInfo cfu_fdro_info = { + .name = TYPE_XLNX_VERSAL_CFU_FDRO, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(XlnxVersalCFUFDRO), + .class_init = cfu_fdro_class_init, + .instance_init = cfu_fdro_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_XLNX_CFI_IF }, + { } + } +}; + static void cfu_apb_register_types(void) { type_register_static(&cfu_apb_info); + type_register_static(&cfu_fdro_info); } type_init(cfu_apb_register_types) diff --git a/include/hw/misc/xlnx-versal-cfu.h b/include/hw/misc/xlnx-versal-cfu.h index 4936d6e5f0..3603bb2862 100644 --- a/include/hw/misc/xlnx-versal-cfu.h +++ b/include/hw/misc/xlnx-versal-cfu.h @@ -24,6 +24,9 @@ #define TYPE_XLNX_VERSAL_CFU_APB "xlnx,versal-cfu-apb" OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUAPB, XLNX_VERSAL_CFU_APB) +#define TYPE_XLNX_VERSAL_CFU_FDRO "xlnx,versal-cfu-fdro" +OBJECT_DECLARE_SIMPLE_TYPE(XlnxVersalCFUFDRO, XLNX_VERSAL_CFU_FDRO) + REG32(CFU_ISR, 0x0) FIELD(CFU_ISR, USR_GTS_EVENT, 9, 1) FIELD(CFU_ISR, USR_GSR_EVENT, 8, 1) @@ -209,4 +212,12 @@ struct XlnxVersalCFUAPB { } cfg; }; +struct XlnxVersalCFUFDRO { + SysBusDevice parent_obj; + MemoryRegion iomem_fdro; + + GArray *fdro_data; + uint32_t *ro_data; + uint32_t ro_dlen; +}; #endif
Introduce a model of Xilinx Versal's Configuration Frame Unit's data out port (CFU_FDRO). Signed-off-by: Francisco Iglesias <francisco.iglesias@amd.com> --- hw/misc/xlnx-versal-cfu.c | 105 ++++++++++++++++++++++++++++++ include/hw/misc/xlnx-versal-cfu.h | 11 ++++ 2 files changed, 116 insertions(+)