Re: [PATCH v4 08/14] none-machine: add 'ram-addr' property

[Damien Hedde <damien.hedde@greensocs.com>]

On 3/3/22 15:41, Philippe Mathieu-Daudé wrote:
> On 23/2/22 10:07, Damien Hedde wrote:
>> Add the property to configure a the base address of the ram.
>> The default value remains zero.
>>
>> This commit is needed to use the 'none' machine as a base, and
>> subsequently to dynamically populate it using qapi commands. Having
>> a non null 'ram' is really hard to workaround because of the actual
>> constraints on the generic loader: it prevents loading binaries
>> bigger than ram_size (with a null ram, we cannot load anything).
>> For now we need to be able to use the existing ram creation
>> feature of the none machine with a configurable base address.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>   hw/core/null-machine.c | 34 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>> index 7eb258af07..5fd1cc0218 100644
>> --- a/hw/core/null-machine.c
>> +++ b/hw/core/null-machine.c
>> @@ -16,9 +16,11 @@
>>   #include "hw/boards.h"
>>   #include "exec/address-spaces.h"
>>   #include "hw/core/cpu.h"
>> +#include "qapi/visitor.h"
>>   struct NoneMachineState {
>>       MachineState parent;
>> +    uint64_t ram_addr;
>>   };
>>   #define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
>> @@ -26,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, 
>> NONE_MACHINE)
>>   static void machine_none_init(MachineState *mch)
>>   {
>> +    NoneMachineState *nms = NONE_MACHINE(mch);
>>       CPUState *cpu = NULL;
>>       /* Initialize CPU (if user asked for it) */
>> @@ -37,9 +40,13 @@ static void machine_none_init(MachineState *mch)
>>           }
>>       }
>> -    /* RAM at address zero */
>> +    /* RAM at configured address (default: 0) */
>>       if (mch->ram) {
>> -        memory_region_add_subregion(get_system_memory(), 0, mch->ram);
>> +        memory_region_add_subregion(get_system_memory(), nms->ram_addr,
>> +                                    mch->ram);
>> +    } else if (nms->ram_addr) {
>> +        error_report("'ram-addr' has been specified but the size is 
>> zero");
> 
> I'm not sure about this error message, IIUC we can get here if no ram
> backend is provided, not if we have one zero-sized. Otherwise LGTM.

You're most probably right. Keeping the ram_size to 0 is just one way of 
getting here. I can replace the message by a more generic formulation
"'ram-addr' has been specified but the machine has no ram"