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

[Jim Shu <jim.shu@sifive.com>]

[-- Attachment #1: Type: text/plain, Size: 2796 bytes --]

Tested-by: Jim Shu <jim.shu@sifive.com>

On Fri, Mar 4, 2022 at 12:36 AM Damien Hedde <damien.hedde@greensocs.com>
wrote:

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

[-- Attachment #2: Type: text/html, Size: 3905 bytes --]