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

[Alistair Francis]

On Mon, Jan 11, 2016 at 8:04 AM, Peter Maydell <address@hidden> wrote:
> 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?

I think I have fixed them. Unfortunately I don't have access to a
32-bit machine to test.

>
> 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

Ok, I now create a uint64_t variable to store the value.

>
> (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.

If I'm using a 64-bit variable to store the value won't this no longer
be a problem?

>
> (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.)

Again, isn't this fixed by changing to a variable?

Thanks,

Alistair

>
> thanks
> -- PMM
>