Re: [Qemu-devel] [PATCH v2 1/1] xlnx-zynqmp: Add support for high DDR memory regions

[Peter Crosthwaite]

On Wed, Dec 30, 2015 at 6:19 PM, Peter Crosthwaite
<address@hidden> wrote:
> This concept might also be relevant to rPI work, where the SoC aliases
> RAM. CC Andrew.
>
> On Wed, Dec 16, 2015 at 11:27 AM, Alistair Francis
> <address@hidden> wrote:
>> The Xilinx ZynqMP SoC and EP108 board supports three memory regions:
>>  - A 2GB region starting at 0
>>  - A 32GB region starting at 32GB
>>  - A 256GB region starting at 768GB
>>
>> This patch adds support for the first two memory regions, which is
>> automatically created based on the size specified by the QEMU memory
>> command line argument.
>>
>> On hardware the physical memory region is one continuous region, it is then
>> mapped into the three different regions by the DDRC. As we don't model the
>> DDRC this is done at startup by QEMU. The board creates the memory region and
>> then passes that memory region to the SoC. The SoC then maps the memory
>> regions.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> V2:
>>  - Create one continuous memory region and pass it to the SoC
>>
>> Also, the Xilinx ZynqMP TRM is avaliable at:
>> http://www.xilinx.com/products/silicon-devices/soc/zynq-ultrascale-mpsoc.html?resultsTablePreSelect=documenttype:User%20Guides#documentation
>>
>>  hw/arm/xlnx-ep108.c          | 42 +++++++++++++++++++++++-------------------
>>  hw/arm/xlnx-zynqmp.c         | 31 +++++++++++++++++++++++++++++++
>>  include/hw/arm/xlnx-zynqmp.h | 12 ++++++++++++
>>  3 files changed, 66 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
>> index 85b978f..a0174d5 100644
>> --- a/hw/arm/xlnx-ep108.c
>> +++ b/hw/arm/xlnx-ep108.c
>> @@ -22,12 +22,9 @@
>>
>>  typedef struct XlnxEP108 {
>>      XlnxZynqMPState soc;
>> -    MemoryRegion ddr_ram;
>> +    MemoryRegion ddr_board_ram;
>
> Rename not needed. The Machine is the board and has no other DDR RAM
> to refer to other than than the off-SoC DDR chips.
>
>>  } XlnxEP108;
>>
>> -/* Max 2GB RAM */
>> -#define EP108_MAX_RAM_SIZE 0x80000000ull
>> -
>>  static struct arm_boot_info xlnx_ep108_binfo;
>>
>>  static void xlnx_ep108_init(MachineState *machine)
>> @@ -35,20 +32,12 @@ static void xlnx_ep108_init(MachineState *machine)
>>      XlnxEP108 *s = g_new0(XlnxEP108, 1);
>>      Error *err = NULL;
>>
>> -    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
>> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>> -                              &error_abort);
>> -
>> -    object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
>> -    if (err) {
>> -        error_report("%s", error_get_pretty(err));
>> -        exit(1);
>> -    }
>> -
>> -    if (machine->ram_size > EP108_MAX_RAM_SIZE) {
>> +    /* Create the memory region to pass to the SoC */
>> +    if (machine->ram_size > XLNX_ZYNQMP_MAX_RAM_SIZE) {
>>          error_report("WARNING: RAM size " RAM_ADDR_FMT " above max 
>> supported, "
>> -                     "reduced to %llx", machine->ram_size, 
>> EP108_MAX_RAM_SIZE);
>> -        machine->ram_size = EP108_MAX_RAM_SIZE;
>> +                     "reduced to %llx", machine->ram_size,
>> +                     XLNX_ZYNQMP_MAX_RAM_SIZE);
>> +        machine->ram_size = XLNX_ZYNQMP_MAX_RAM_SIZE;
>>      }
>>
>>      if (machine->ram_size < 0x08000000) {
>> @@ -56,9 +45,24 @@ static void xlnx_ep108_init(MachineState *machine)
>>                   machine->ram_size);
>>      }
>>
>> -    memory_region_allocate_system_memory(&s->ddr_ram, NULL, "ddr-ram",
>> +    memory_region_allocate_system_memory(&s->ddr_board_ram, NULL,
>> +                                         "xlnx-zynqmp-board-ram",
>>                                           machine->ram_size);
>> -    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>> +
>> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
>> +    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>> +                              &error_abort);
>> +
>> +    object_property_set_int(OBJECT(&s->soc), machine->ram_size,
>> +                            "ram-size", &error_abort);
>> +    object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_board_ram),
>> +                         "xlnx-zynqmp-board-ram", &error_abort);
>> +
>> +    object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
>> +    if (err) {
>> +        error_report("%s", error_get_pretty(err));
>> +        exit(1);
>> +    }
>>
>>      xlnx_ep108_binfo.ram_size = machine->ram_size;
>>      xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index 87553bb..848d1ff 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -90,6 +90,11 @@ static void xlnx_zynqmp_init(Object *obj)
>>                                    &error_abort);
>>      }
>>
>> +    object_property_add_link(obj, "xlnx-zynqmp-board-ram", 
>> TYPE_MEMORY_REGION,
>
> It is already implicit that this is a zynqmp, so you don't need the
> "xlnx-zynqmp" prefix. I don't think "board" is needed either, IIRC the
> docs largely refer to it as "ddr-ram".
>
>> +                             (Object **)&s->ddr_board_ram,
>
> Drop board_
>
>> +                             qdev_prop_allow_set_link_before_realize,
>> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort);
>> +
>>      object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
>>      qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
>>
>> @@ -120,9 +125,33 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
>> **errp)
>>      MemoryRegion *system_memory = get_system_memory();
>>      uint8_t i;
>>      const char *boot_cpu = s->boot_cpu ? s->boot_cpu : "apu-cpu[0]";
>> +    ram_addr_t ddr_low_size, ddr_high_size;
>>      qemu_irq gic_spi[GIC_NUM_SPI_INTR];
>>      Error *err = NULL;
>>
>> +    /* Create the DDR Memory Regions */
>> +    if (s->ram_size > XLNX_ZYNQMP_MAX_LOW_RAM_SIZE) {
>> +        /* The RAM size is above the maximum avaliable for the low DDR.
>
> "available".
>
>> +         * Create the high DDR memory region as well
>> +         */
>> +        ddr_low_size = XLNX_ZYNQMP_MAX_LOW_RAM_SIZE;
>> +        ddr_high_size = s->ram_size - XLNX_ZYNQMP_MAX_LOW_RAM_SIZE;
>
> Should error check against > 34GB total (which should errp return).
>
>> +
>> +        memory_region_init_alias(&s->ddr_ram_high, NULL,
>> +                                 "ddr-ram-high", s->ddr_board_ram,
>> +                                  ddr_low_size, ddr_high_size);
>> +        memory_region_add_subregion(get_system_memory(),
>> +                                    XLNX_ZYNQMP_HIGH_RAM_START,
>> +                                    &s->ddr_ram_high);
>> +    } else {
>> +        ddr_low_size = s->ram_size;
>> +    }
>> +
>> +    memory_region_init_alias(&s->ddr_ram_low, NULL,
>> +                             "ddr-ram-low", s->ddr_board_ram,
>> +                              0, ddr_low_size);
>> +    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram_low);
>> +
>>      /* Create the four OCM banks */
>>      for (i = 0; i < XLNX_ZYNQMP_NUM_OCM_BANKS; i++) {
>>          char *ocm_name = g_strdup_printf("zynqmp.ocm_ram_bank_%d", i);
>> @@ -290,6 +319,8 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error 
>> **errp)
>>
>>  static Property xlnx_zynqmp_props[] = {
>>      DEFINE_PROP_STRING("boot-cpu", XlnxZynqMPState, boot_cpu),
>> +    /* Needs to be set by the board, so default to an invalid number */
>
> Check for this missing in realize.
>
>> +    DEFINE_PROP_UINT64("ram-size", XlnxZynqMPState, ram_size, -1),
>
> Although, I think you can drop this property. You can query MR sizes
> directly from the MRs themselves via QOM. From memory.c:
>
>     object_property_add(OBJECT(mr), "size", "uint64",
>                         memory_region_get_size,
>                         NULL, /* memory_region_set_size, */
>                         NULL, NULL, &error_abort);
>
> You should be able to use object_property_get_int(... "size" ...) to
> grab it from the linked MR.
>

There is the memory_region_size() API that will make shorter work of this.

Regards,
Peter

>>      DEFINE_PROP_END_OF_LIST()
>>  };
>>
>> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
>> index d116092..01eeadf 100644
>> --- a/include/hw/arm/xlnx-zynqmp.h
>> +++ b/include/hw/arm/xlnx-zynqmp.h
>> @@ -51,6 +51,14 @@
>>  #define XLNX_ZYNQMP_GIC_REGION_SIZE 0x1000
>>  #define XLNX_ZYNQMP_GIC_ALIASES     (0x10000 / XLNX_ZYNQMP_GIC_REGION_SIZE 
>> - 1)
>>
>> +#define XLNX_ZYNQMP_MAX_LOW_RAM_SIZE    0x80000000ull
>> +
>> +#define XLNX_ZYNQMP_MAX_HIGH_RAM_SIZE   0x800000000ull
>> +#define XLNX_ZYNQMP_HIGH_RAM_START      0x800000000ull
>> +
>> +#define XLNX_ZYNQMP_MAX_RAM_SIZE (XLNX_ZYNQMP_MAX_LOW_RAM_SIZE + \
>> +                                  XLNX_ZYNQMP_MAX_HIGH_RAM_SIZE)
>> +
>>  typedef struct XlnxZynqMPState {
>>      /*< private >*/
>>      DeviceState parent_obj;
>> @@ -60,7 +68,11 @@ typedef struct XlnxZynqMPState {
>>      ARMCPU rpu_cpu[XLNX_ZYNQMP_NUM_RPU_CPUS];
>>      GICState gic;
>>      MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
>> +
>>      MemoryRegion ocm_ram[XLNX_ZYNQMP_NUM_OCM_BANKS];
>
> A nit, but i'd probably blank line here too.
>
> Regards,
> Peter
>
>> +    uint64 ram_size;
>> +    MemoryRegion *ddr_board_ram;
>> +    MemoryRegion ddr_ram_low, ddr_ram_high;
>>
>>      CadenceGEMState gem[XLNX_ZYNQMP_NUM_GEMS];
>>      CadenceUARTState uart[XLNX_ZYNQMP_NUM_UARTS];
>> --
>> 2.5.0
>>