Re: [Qemu-devel] [PATCH target-arm v1 13/15] arm: xilinx-zynq-mp-generic: Add external RAM

[Alistair Francis]

On Tue, Mar 3, 2015 at 8:59 AM, Peter Crosthwaite
<address@hidden> wrote:
> On Mon, Mar 2, 2015 at 2:38 PM, Alistair Francis
> <address@hidden> wrote:
>> On Tue, Mar 3, 2015 at 5:40 AM, Peter Crosthwaite
>> <address@hidden> wrote:
>>> On Mon, Feb 23, 2015 at 6:24 PM, Alistair Francis
>>> <address@hidden> wrote:
>>>> On Tue, Feb 24, 2015 at 9:04 AM, Peter Crosthwaite
>>>> <address@hidden> wrote:
>>>>> Zynq MPSoC supports external DDR RAM. Add a RAM at 0 to the model.
>>>>>
>>>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>>>> ---
>>>>>  hw/arm/xlnx-zynq-mp-generic.c | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/hw/arm/xlnx-zynq-mp-generic.c b/hw/arm/xlnx-zynq-mp-generic.c
>>>>> index ff69b07..7394e82 100644
>>>>> --- a/hw/arm/xlnx-zynq-mp-generic.c
>>>>> +++ b/hw/arm/xlnx-zynq-mp-generic.c
>>>>> @@ -18,9 +18,11 @@
>>>>>  #include "hw/arm/xlnx-zynq-mp.h"
>>>>>  #include "hw/boards.h"
>>>>>  #include "qemu/error-report.h"
>>>>> +#include "exec/address-spaces.h"
>>>>>
>>>>>  typedef struct XlnxZynqMPGeneric {
>>>>>      XlnxZynqMPState soc;
>>>>> +    MemoryRegion ddr_ram;
>>>>>  } XlnxZynqMPGeneric;
>>>>>
>>>>>  static void xlnx_zynq_mp_generic_init(MachineState *machine)
>>>>> @@ -36,6 +38,11 @@ static void xlnx_zynq_mp_generic_init(MachineState 
>>>>> *machine)
>>>>>          error_report("%s", error_get_pretty(err));
>>>>>          exit(1);
>>>>>      }
>>>>> +
>>>>> +    memory_region_init_ram(&s->ddr_ram, NULL, "ddr-ram", 
>>>>> machine->ram_size,
>>>>> +                           &error_abort);
>>>>
>>>> Shouldn't there be a default size if none is specified? This looks
>>>> like it will cause user
>>>> issues if they don't understand that they must specify the memory size.
>>>>
>>>
>>> So vl.c provides a core default of 128MB if -m is unspecified. It
>>> should boot with that successfully. The error case is probably when
>>> the user overrides -m to a low value such that the boot cant work
>>> anymore, but that would be a error caught by the boot loader I think.
>>
>> I still think that this could cause issues. If a user doesn't realise that
>> they have to specify -m and the boot still works, they will just assume
>> that the correct memory size is in the machine model. When in fact all
>> that is happening is they are getting a small amount of memory that is
>> just lucky enough to work. 128MB is very small for ZynqMP.
>>
>
> So there is precedent for upper clamps on this but not lower clamps. I
> think we may actually want the same as zynq with an upper clamp:
>
> 158     /* max 2GB ram */
> 159     if (ram_size > 0x80000000) {
> 160         ram_size = 0x80000000;
> 161     }
>
>> Maybe instead of an error or a default just have a warning if the memory
>> is below a certain size. That way it will still boot, but the user will know
>> that they should be specifying a size.
>>
>
> Adding the warning for <= 128MB. Anything more than that then the user
> must have manually specified -m so not too worried about that.

Great, both sound good

Thanks,

Alistair

>
> Regards,
> Peter
>
>> Thanks,
>>
>> Alistair
>>
>>>
>>> Regards,
>>> Peter
>>>
>>>> At least return an error if none is specified.
>>>>
>>>> Thanks,
>>>>
>>>> Alistair
>>>>
>>>>> +    vmstate_register_ram_global(&s->ddr_ram);
>>>>> +    memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>>>>>  }
>>>>>
>>>>>  static QEMUMachine xlnx_zynq_mp_generic_machine = {
>>>>> --
>>>>> 2.3.0.1.g27a12f1
>>>>>
>>>>>
>>>>
>>>
>>
>