Re: [Qemu-devel] [PATCH] hw/core/null-machine: Add the possibility to instantiate a CPU, RAM and kernel

[Thomas Huth]

On 16.01.2017 18:25, Eduardo Habkost wrote:
> On Sat, Jan 14, 2017 at 07:51:02AM +0100, Thomas Huth wrote:
>> Sometimes it is useful to have just a machine with CPU and RAM, without
>> any further hardware in it, e.g. if you just want to do some instruction
>> debugging for TCG with a remote GDB attached to QEMU, or run some embedded
>> code with the "-semihosting" QEMU parameter. qemu-system-m68k already
>> features a "dummy" machine, and xtensa a "sim" machine for exactly this
>> purpose.
>> All target architectures have nowadays also a "none" machine, which would
>> be a perfect match for this, too - but it currently does not allow to add
>> CPU, RAM or a kernel yet. Thus let's add these possibilities in a generic
>> way to the "none" machine, too, so that we hopefully do not need additional
>> "dummy" machines in the future anymore (and maybe can also get rid of the
>> already existing "dummy"/"sim" machines one day).
>> Note that the default behaviour of the "none" machine is not changed, i.e.
>> no CPU and no RAM is instantiated by default. You've explicitely got to
>> specify the CPU model with "-cpu" and the amount of RAM with "-m" to get
>> these new features.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
> 
> This sounds nice, but I would like initialization code used by
> "-machine none" to be generic code usable by other machines,
> whenever possible.
> 
> The CPU and RAM creation probably is simple enough to not deserve
> a generic wrapper by now. But the kernel loader probably could be
> made reusable in the first version, so existing machines that
> already have similar logic could reuse it.
> 
> I would love to make all machines automatically use new generic
> code to create CPUs, allocate RAM, and load a kernel. But
> probably this would be hard to do in a single step due to subtle
> initialization ordering requirements in each machine init
> function.
> 
> More specific comments below:
> 
>> ---
>>  hw/core/Makefile.objs  |  2 +-
>>  hw/core/null-machine.c | 81 
>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 81 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index a4c94e5..0b6c0f1 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -12,7 +12,6 @@ common-obj-$(CONFIG_XILINX_AXI) += stream.o
>>  common-obj-$(CONFIG_PTIMER) += ptimer.o
>>  common-obj-$(CONFIG_SOFTMMU) += sysbus.o
>>  common-obj-$(CONFIG_SOFTMMU) += machine.o
>> -common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>>  common-obj-$(CONFIG_SOFTMMU) += register.o
>> @@ -20,3 +19,4 @@ common-obj-$(CONFIG_SOFTMMU) += or-irq.o
>>  common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
>>  
>>  obj-$(CONFIG_SOFTMMU) += generic-loader.o
>> +obj-$(CONFIG_SOFTMMU) += null-machine.o
> 
> I'd like to keep null-machine.o in common-obj-y, if possible. We
> already have an arch_type variable provided by arch_init.c, maybe
> it could also provide arch_endianness?

Yes, that sounds like an idea. Or maybe it's feasible to tweak
load_elf(), so it can also be called with the endianess parameter set to
"-1", meaning the caller does not care whether the endianess is big or
little?

>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>> index 0351ba7..b2468ed 100644
>> --- a/hw/core/null-machine.c
>> +++ b/hw/core/null-machine.c
>> @@ -13,18 +13,97 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>> +#include "qemu/error-report.h"
>>  #include "hw/hw.h"
>>  #include "hw/boards.h"
>> +#include "hw/loader.h"
>> +#include "sysemu/sysemu.h"
>> +#include "exec/address-spaces.h"
>> +#include "cpu.h"
>> +#include "elf.h"
>> +
>> +#ifdef TARGET_WORDS_BIGENDIAN
>> +#define LOAD_ELF_ENDIAN_FLAG 1
>> +#else
>> +#define LOAD_ELF_ENDIAN_FLAG 0
>> +#endif
>> +
>> +static hwaddr cpu_initial_pc;
> 
> Other architectures and machines probably have something similar
> to cpu_initial_pc, so this could be probably be moved to an
> existing generic struct. But I am not sure if this would be a
> MachineState field or CPUState field.

I'm also not sure whether this makes sense right from the start ... the
intial PC handling seems to be slightly different for each board, so
trying to generalize this variable sounds quite complicated ... so
that's maybe rather something for a later clean-up patch instead.

>> +static uint64_t translate_phys_addr(void *opaque, uint64_t addr)
>> +{
>> +    return cpu_get_phys_page_debug(CPU(opaque), addr);
>> +}
>> +
>> +static void machine_none_load_kernel(CPUState *cpu, const char 
>> *kernel_fname,
>> +                                     ram_addr_t ram_size)
>> +{
>> +    uint64_t elf_entry;
>> +    int kernel_size;
>> +
>> +    if (!ram_size) {
>> +        error_report("You need RAM for loading a kernel");
>> +        return;
>> +    }
>> +
>> +    kernel_size = load_elf(kernel_fname, translate_phys_addr, cpu, 
>> &elf_entry,
>> +                           NULL, NULL, LOAD_ELF_ENDIAN_FLAG, EM_NONqE, 0, 
>> 0);
>> +    cpu_initial_pc = elf_entry;
>> +    if (kernel_size < 0) {
>> +        kernel_size = load_uimage(kernel_fname, &cpu_initial_pc, NULL, NULL,
>> +                                  NULL, NULL);
>> +    }
>> +    if (kernel_size < 0) {
>> +        kernel_size = load_image_targphys(kernel_fname, 0, ram_size);
>> +    }
>> +    if (kernel_size < 0) {
>> +        error_report("Could not load kernel '%s'", kernel_fname);
>> +        return;
>> +    }
>> +}
> 
> Do you know how many different architectures will be able to use
> this generic ELF loading logic as-is?

I've only tried m68k and xtensa so far, and it worked there. OpenRISC
"or32-sim" machine looks very similar, too. Most other board look
slightly different, either lacking the "load_uimage" or
"load_image_targphys" (but the generic code might still work there), or
having some other magic code inbetween...

> Probably other machines already have very similar logic, so it
> would be nice if we could make this reusable so other machines
> don't need to duplicate it.

I can have a try...

>> +static void machine_none_cpu_reset(void *opaque)
>> +{
>> +    CPUState *cpu = CPU(opaque);
>> +
>> +    cpu_reset(cpu);
>> +    cpu_set_pc(cpu, cpu_initial_pc);
>> +}
>>  
>>  static void machine_none_init(MachineState *machine)
>>  {
>> +    ram_addr_t ram_size = machine->ram_size;
>> +    MemoryRegion *ram;
>> +    CPUState *cpu = NULL;
>> +
>> +    /* Initialize CPU (if a model has been specified) */
>> +    if (machine->cpu_model) {
>> +        cpu = cpu_init(machine->cpu_model);
>> +        if (!cpu) {
>> +            error_report("Unable to initialize CPU");
>> +            exit(1);
>> +        }
>> +        qemu_register_reset(machine_none_cpu_reset, cpu);
>> +        cpu_reset(cpu);
>> +    }
> 
> This looks like a sign that we need a generic wrapper to create
> CPUs. Probably lots of other machines do something very similar.
> But I think this is something for later.

I don't think that this sequence can be generalized in a wrapper
function so easily: Most other machines apparently do some additional
magic between cpu_init() and qemu_register_reset() ... so if we really
want to generalize this, this is certainly a task for a separate clean
up patch later.

>>  static void machine_none_machine_init(MachineClass *mc)
>>  {
>>      mc->desc = "empty machine";
>>      mc->init = machine_none_init;
>> -    mc->max_cpus = 0;
> 
> Does this line removal has any visible effect? vl.c already
> interprets max_cpus==0 as the same as max_cpus==1.
> 
> Maybe we should set max_cpus=1 explicitly to make it clear that
> this new code doesn't work if smp_cpus > 1.

You're right, max_cpus = 1 would make more sense here ... I'll change my
patch accordingly.

 Thomas