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

[Damien Hedde]

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"