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

[Peter Maydell]

On 5 January 2016 at 22:05, 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>
> Reviewed-by: Peter Crosthwaite <address@hidden>
> ---
> V4:
>  - Small fixes
>  - Localisation of ram_size
> V3:
>  - Assert on the RAM sizes
>  - Remove ram_size property
>  - General fixes
> V2:
>  - Create one continuous memory region and pass it to the SoC

> @@ -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) {

Unfortunately this doesn't build on 32-bit hosts:

/home/petmay01/qemu/hw/arm/xlnx-ep108.c: In function 'xlnx_ep108_init':
/home/petmay01/qemu/hw/arm/xlnx-ep108.c:36:16: error: comparison is
always false due to limited range of data type [-Werror=type-limits]
     if (machine->ram_size > XLNX_ZYNQMP_MAX_RAM_SIZE) {
                ^
/home/petmay01/qemu/hw/arm/xlnx-ep108.c:40:9: error: large integer
implicitly truncated to unsigned type [-Werror=overflow]
         machine->ram_size = XLNX_ZYNQMP_MAX_RAM_SIZE;
         ^
cc1: all warnings being treated as errors

so I'm going to drop it from the target-arm pullreq. Please could
you fix the compile issue and resend?

There are a couple of problems you're running into:

(1) machine->ram_size is a ram_addr_t so might be 32 bit; you
can do what virt.c does to avoid the warning and use a local
uin64_t variable for the comparison

(2) complaint about reassigning back to ram_size. this is spurious
but you can avoid it by making this board behave the same way as
virt.c, vexpress.c etc do if presented with an unsupported
ram_size -- you should fail, rather than truncating and continuing.

(3) %llx is not the correct format string for a ram_addr_t:
use RAM_ADDR_FMT. (This isn't making the compiler complain,
but I noticed it looking at the code.)

thanks
-- PMM