Re: [Patch 2/2] hw/arm/xlnx-zynqmp: Add unimplemented APU mmio

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Wed, Aug 18, 2021 at 08:15:25PM -0700, Tong Ho wrote:
> Add unimplemented APU mmio region to xlnx-zynqmp for booting
> bare-metal guests built with standalone bsp published at:
>   https://github.com/Xilinx/embeddedsw/tree/master/lib/bsp/standalone/src/arm/ARMv8/64bit
> 
> Signed-off-by: Tong Ho <tong.ho@xilinx.com>
> ---
>  hw/arm/xlnx-zynqmp.c         | 32 ++++++++++++++++++++++++++++++++
>  include/hw/arm/xlnx-zynqmp.h |  7 +++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 3597e8db4d..790df2b6f1 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -20,6 +20,7 @@
>  #include "qemu/module.h"
>  #include "hw/arm/xlnx-zynqmp.h"
>  #include "hw/intc/arm_gic_common.h"
> +#include "hw/misc/unimp.h"
>  #include "hw/boards.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/sysemu.h"
> @@ -56,6 +57,9 @@
>  #define DPDMA_ADDR          0xfd4c0000
>  #define DPDMA_IRQ           116
>  
> +#define APU_ADDR            0xfd5c0000
> +#define APU_SIZE            0x100
> +
>  #define IPI_ADDR            0xFF300000
>  #define IPI_IRQ             64
>  
> @@ -222,6 +226,32 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
>      qdev_realize(DEVICE(&s->rpu_cluster), NULL, &error_fatal);
>  }
>  
> +static void xlnx_zynqmp_create_unimp_mmio(XlnxZynqMPState *s)
> +{
> +    static const struct UnimpInfo {
> +        const char *name;
> +        hwaddr base;
> +        hwaddr size;
> +    } unimp_areas[ARRAY_SIZE(s->mr_unimp)] = {
> +        { .name = "apu", APU_ADDR, APU_SIZE },
> +    };
> +

Nitpick, I'd probably drop this blank line and spell out the unsigned int below.

Also, we might want consider asserting that every array member has been
initialized. Eg assert(info->name) in the loop below. Unless perhaps something
already bails out in those cases (e.g sysbus_realize_and_unref()).

In either case:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> +    unsigned nr;
> +
> +    for (nr = 0; nr < ARRAY_SIZE(unimp_areas); nr++) {
> +        const struct UnimpInfo *info = &unimp_areas[nr];
> +        DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +        qdev_prop_set_string(dev, "name", info->name);
> +        qdev_prop_set_uint64(dev, "size", info->size);
> +        object_property_add_child(OBJECT(s), info->name, OBJECT(dev));
> +
> +        sysbus_realize_and_unref(sbd, &error_fatal);
> +        sysbus_mmio_map(sbd, 0, info->base);
> +    }
> +}
> +
>  static void xlnx_zynqmp_init(Object *obj)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
> @@ -616,6 +646,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->rtc), 0, RTC_ADDR);
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->rtc), 0, gic_spi[RTC_IRQ]);
>  
> +    xlnx_zynqmp_create_unimp_mmio(s);
> +
>      for (i = 0; i < XLNX_ZYNQMP_NUM_GDMA_CH; i++) {
>          if (!object_property_set_uint(OBJECT(&s->gdma[i]), "bus-width", 128,
>                                        errp)) {
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index d3e2ef97f6..c84fe15996 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -79,6 +79,11 @@ OBJECT_DECLARE_SIMPLE_TYPE(XlnxZynqMPState, XLNX_ZYNQMP)
>  #define XLNX_ZYNQMP_MAX_RAM_SIZE (XLNX_ZYNQMP_MAX_LOW_RAM_SIZE + \
>                                    XLNX_ZYNQMP_MAX_HIGH_RAM_SIZE)
>  
> +/*
> + * Unimplemented mmio regions needed to boot some images.
> + */
> +#define XLNX_ZYNQMP_NUM_UNIMP_AREAS 1
> +
>  struct XlnxZynqMPState {
>      /*< private >*/
>      DeviceState parent_obj;
> @@ -96,6 +101,8 @@ struct XlnxZynqMPState {
>      MemoryRegion *ddr_ram;
>      MemoryRegion ddr_ram_low, ddr_ram_high;
>  
> +    MemoryRegion mr_unimp[XLNX_ZYNQMP_NUM_UNIMP_AREAS];
> +
>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>      CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
>      XlnxZynqMPCANState can[XLNX_ZYNQMP_NUM_CAN];
> -- 
> 2.25.1
>