diff mbox series

[v1,3/8] hw/misc/xlnx-versal-cfu: Introduce a model of Xilinx Versal CFU_FDRO

Message ID 20230710140249.56324-4-francisco.iglesias@amd.com (mailing list archive)
State New, archived
Headers show
Series Xilinx Versal CFI support | expand

Commit Message

Francisco Iglesias July 10, 2023, 2:02 p.m. UTC 
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(+)

Comments

Peter Maydell Aug. 3, 2023, 1:48 p.m. UTC | #1 
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
Francisco Iglesias Aug. 10, 2023, 7:15 p.m. UTC | #2 
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 mbox series

Patch

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