Re: [Qemu-devel] [RFC PATCH v2 3/3] hw/vfio: add pl330 device support

[Eric Auger <eric.auger@linaro.org>]

On 12/22/2014 05:23 PM, Baptiste Reynal wrote:
> Create a meta-device for PL330 DMA.
> Add add_arm_pl330_fdt_node function, with multiple compatible string
> and clocks support.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c          | 84 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/vfio/Makefile.objs        |  1 +
>  hw/vfio/pl330.c              | 41 +++++++++++++++++++++
>  include/hw/vfio/vfio-pl330.h | 26 ++++++++++++++
>  4 files changed, 152 insertions(+)
>  create mode 100644 hw/vfio/pl330.c
>  create mode 100644 include/hw/vfio/vfio-pl330.h
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index f6ff8a7..efdeea7 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -28,6 +28,9 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/vfio/vfio-platform.h"
>  #include "hw/vfio/vfio-calxeda-xgmac.h"
> +#include "hw/vfio/vfio-pl330.h"
> +
> +#include <libfdt.h>
>  
>  /*
>   * internal struct that contains the information to create dynamic
> @@ -182,9 +185,90 @@ fail:
>     return -1;
>  }
>  
> +/**
> + * add_arm_pl330_fdt_node
> + *
> + * Generates a very simple node with following properties:
> + * compatible string, regs, interrupts, clocks, clock-names
> + */
> +static int add_arm_pl330_fdt_node(SysBusDevice *sbdev, void *opaque)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    void *fdt = data->fdt;
> +    const char *parent_node = data->pbus_node_name;
> +    int compat_str_len;
> +    char *nodename;
> +    int ret;
> +    uint64_t mmio_base;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +    Object *obj = OBJECT(sbdev);
> +
> +    mmio_base = object_property_get_int(obj, "mmio[0]", NULL);
> +
> +    nodename = g_strdup_printf("%s/%s@%" PRIx64, parent_node,
> +                               vbasedev->name,
> +                               mmio_base);
> +
> +    qemu_fdt_add_subnode(fdt, nodename);
> +
> +    /*
> +     * Process compatible string to deal with multiple strings
> +     * (; is replaced by \0)
> +     */
> +    char *compat = g_strdup(vdev->compat);
> +    compat_str_len = strlen(compat) + 1;
> +
> +    char *semicolon = compat;
> +    while ((semicolon = strchr(semicolon, ';')) != NULL) {
> +        *semicolon = '\0';
> +    }
> +
> +    qemu_fdt_setprop(fdt, nodename, "compatible",
> +                          compat, compat_str_len);
> +
> +    /* Setup clocks for AMBA device */
> +    /* Check clock existence */
> +    ret = fdt_path_offset(fdt, "/apb-pclk");
> +
> +    if (ret < 0) {
> +        error_report("could not set clocks property of node %s", nodename);
in case apb-clk is not found shouldn't we jump to a fail section?
> +    } else {
> +        qemu_fdt_setprop_cells(fdt, nodename, "clocks",
> +                qemu_fdt_getprop_cell(fdt, "/apb-pclk", "phandle"));
> +        char clock_names[] = "apb_pclk";
> +        qemu_fdt_setprop(fdt, nodename, "clock-names", clock_names,
> +                sizeof(clock_names));
> +    }
> +
> +    ret = set_regions_fdt_node(nodename, sbdev, opaque);
> +
> +    if (ret < 0) {
> +        error_report("could not set reg property of node %s", nodename);
> +        goto fail;
> +    }
> +
> +    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
> +
> +    if (ret < 0) {
> +        error_report("could not set interrupts property of node %s",
> +                     nodename);
> +        goto fail;
> +    }
> +
> +    g_free(nodename);
> +
> +    return 0;
> +
> +fail:
> +
I think we should free nodename too here.

Otherwise, to me, it fits with the original spirit now.

Thanks

Eric
> +   return -1;
> +}
> +
>  /* list of supported dynamic sysbus devices */
>  static const NodeCreationPair add_fdt_node_functions[] = {
>  {TYPE_VFIO_CALXEDA_XGMAC, add_calxeda_midway_xgmac_fdt_node},
> +{TYPE_VFIO_PL330, add_arm_pl330_fdt_node},
>  {"", NULL}, /*last element*/
>  };
>  
> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> index 913ab14..be3023b 100644
> --- a/hw/vfio/Makefile.objs
> +++ b/hw/vfio/Makefile.objs
> @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
>  obj-$(CONFIG_PCI) += pci.o
>  obj-$(CONFIG_SOFTMMU) += platform.o
>  obj-$(CONFIG_SOFTMMU) += calxeda_xgmac.o
> +obj-$(CONFIG_SOFTMMU) += pl330.o
>  endif
> diff --git a/hw/vfio/pl330.c b/hw/vfio/pl330.c
> new file mode 100644
> index 0000000..a409024
> --- /dev/null
> +++ b/hw/vfio/pl330.c
> @@ -0,0 +1,41 @@
> +#include "hw/vfio/vfio-pl330.h"
> +
> +static void pl330_realize(DeviceState *dev, Error **errp)
> +{
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
> +    VFIOPl330DeviceClass *k = VFIO_PL330_DEVICE_GET_CLASS(dev);
> +
> +    vdev->compat = g_strdup("arm,pl330;arm,primecell");
> +
> +    k->parent_realize(dev, errp);
> +}
> +
> +static const VMStateDescription vfio_platform_vmstate = {
> +    .name = TYPE_VFIO_PL330,
> +    .unmigratable = 1,
> +};
> +
> +static void vfio_pl330_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    VFIOPl330DeviceClass *vpc =
> +        VFIO_PL330_DEVICE_CLASS(klass);
> +    vpc->parent_realize = dc->realize;
> +    dc->realize = pl330_realize;
> +    dc->desc = "VFIO PL330";
> +}
> +
> +static const TypeInfo vfio_pl330_dev_info = {
> +    .name = TYPE_VFIO_PL330,
> +    .parent = TYPE_VFIO_PLATFORM,
> +    .instance_size = sizeof(VFIOPl330Device),
> +    .class_init = vfio_pl330_class_init,
> +    .class_size = sizeof(VFIOPl330DeviceClass),
> +};
> +
> +static void register_pl330_dev_type(void)
> +{
> +    type_register_static(&vfio_pl330_dev_info);
> +}
> +
> +type_init(register_pl330_dev_type)
> diff --git a/include/hw/vfio/vfio-pl330.h b/include/hw/vfio/vfio-pl330.h
> new file mode 100644
> index 0000000..1cdf039
> --- /dev/null
> +++ b/include/hw/vfio/vfio-pl330.h
> @@ -0,0 +1,26 @@
> +#ifndef HW_VFIO_VFIO_PL330
> +#define HW_VFIO_VFIO_PL330
> +
> +#include "hw/vfio/vfio-platform.h"
> +
> +#define TYPE_VFIO_PL330 "vfio-pl330"
> +
> +typedef struct VFIOPl330Device {
> +    VFIOPlatformDevice vdev;
> +} VFIOPl330Device;
> +
> +typedef struct VFIOPl330DeviceClass {
> +    VFIOPlatformDeviceClass parent_class;
> +    DeviceRealize parent_realize;
> +} VFIOPl330DeviceClass;
> +
> +#define VFIO_PL330_DEVICE(obj) \
> +     OBJECT_CHECK(VFIOPl330Device, (obj), TYPE_VFIO_PL330)
> +#define VFIO_PL330_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(VFIOPl330DeviceClass, (klass), \
> +                        TYPE_VFIO_PL330)
> +#define VFIO_PL330_DEVICE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(VFIOPl330DeviceClass, (obj), \
> +                        TYPE_VFIO_PL330)
> +
> +#endif
>