| Message ID | 1484376662-32063-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
Le 14/01/2017 à 07:51, Thomas Huth a écrit : > 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. Did you try to use the generic-loader to load the kernel? Something like "-device loader,file=vmlinux" instead of adding this part in the none machine? Perhaps we could also add a cpu this way, as they are already available in the device list for the machine that allows hotplug. With the same idea, we could also have a "-device ram,size=XXX" to add ram (not DIMM). I think is is the idea behind the none machine: commit b4a738bf93c3137b92d532e59d60edccc4e1ea96 Author: Anthony Liguori <aliguori@us.ibm.com> Date: Wed Aug 22 15:22:05 2012 -0500 boards: add a 'none' machine type to all platforms This allows any QEMU binary to be executed with: $QEMU_BINARY -M none -qmp stdio Without errors from missing options that are required by various boards. This also provides a mode that we can use in the future to construct machines entirely through QMP commands. Laurent
On 14.01.2017 12:03, Laurent Vivier wrote: > Le 14/01/2017 à 07:51, Thomas Huth a écrit : >> 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. > > Did you try to use the generic-loader to load the kernel? > > Something like "-device loader,file=vmlinux" instead of adding this part > in the none machine? It does not work by default - because we need a way to set the CPU's program counter to the entry point of the ELF file. But I think the users also expect the "-kernel" parameter to be working, so I think we should add the loader code in null-machine.c anyway. > Perhaps we could also add a cpu this way, as they are already available > in the device list for the machine that allows hotplug. The only machine that allows hot-plugging of CPUs with the device interface is ppc64 spapr, as far as I know. So this does not help with embedded boards like m68k or xtensa. Also I think you need some additional magic like a bus where the CPUs could be attached, and maybe firmware interfaces, so this does not fit the idea of an embedded board very well. > With the same idea, we could also have a "-device ram,size=XXX" to add > ram (not DIMM). That might be useful indeed, but mainly because some embedded boards expect some additinal RAM at a higher, non-zero addresses, too. > I think is is the idea behind the none machine: [...] > This > also provides a mode that we can use in the future to construct machines > entirely through QMP commands. We're still very far away from the possibility that everything could be constructed on the fly. (Embedded) CPUs, RAM, buses ... all that is not really pluggable yet. So I think my patch is a good first step into this direction to get at least an initial playground for a machine that can be defined by the user. Thomas
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 <thuth@redhat.com> 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? Anyway, arch_init.c is not very clean code currently, so maybe this could be done as a follow-up. > 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. > + > +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_NONE, 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? 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. Also, if this is likely to grow in complexity to become a generic loader useful for additional architectures, this would be another reason to move it to reusable common code, instead of adding complexity inside null-machine.c. > + > +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. > + > + /* RAM at address zero */ > + if (ram_size) { > + ram = g_new(MemoryRegion, 1); > + memory_region_allocate_system_memory(ram, NULL, "ram", ram_size); > + memory_region_add_subregion(get_system_memory(), 0, ram); > + } I was going to suggest moving this to common reusable code too, but in this case it is very simple logic (most of the complexity is already inside memory_region_allocate_system_memory()), so it is probably not worth creating a generic wrapper. > + > + if (machine->kernel_filename) { > + machine_none_load_kernel(cpu, machine->kernel_filename, ram_size); > + } > } > > 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. > + mc->default_ram_size = 0; > } > > DEFINE_MACHINE("none", machine_none_machine_init) > -- > 1.8.3.1 >
On Sun, Jan 15, 2017 at 11:59 PM, Thomas Huth <thuth@redhat.com> wrote: > On 14.01.2017 12:03, Laurent Vivier wrote: >> Le 14/01/2017 à 07:51, Thomas Huth a écrit : >>> 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. >> >> Did you try to use the generic-loader to load the kernel? >> >> Something like "-device loader,file=vmlinux" instead of adding this part >> in the none machine? > > It does not work by default - because we need a way to set the CPU's > program counter to the entry point of the ELF file. The -device loader logic can set the PC to the entry point of the ELF. You just need to specify 'cpu-num=x' when loading the file. > But I think the users also expect the "-kernel" parameter to be working, > so I think we should add the loader code in null-machine.c anyway. I agree that uses probably expect the '-kernel' option to work as well. Thanks, Alistair > >> Perhaps we could also add a cpu this way, as they are already available >> in the device list for the machine that allows hotplug. > > The only machine that allows hot-plugging of CPUs with the device > interface is ppc64 spapr, as far as I know. So this does not help with > embedded boards like m68k or xtensa. Also I think you need some > additional magic like a bus where the CPUs could be attached, and maybe > firmware interfaces, so this does not fit the idea of an embedded board > very well. > >> With the same idea, we could also have a "-device ram,size=XXX" to add >> ram (not DIMM). > > That might be useful indeed, but mainly because some embedded boards > expect some additinal RAM at a higher, non-zero addresses, too. > >> I think is is the idea behind the none machine: > [...] >> This >> also provides a mode that we can use in the future to construct machines >> entirely through QMP commands. > > We're still very far away from the possibility that everything could be > constructed on the fly. (Embedded) CPUs, RAM, buses ... all that is not > really pluggable yet. So I think my patch is a good first step into this > direction to get at least an initial playground for a machine that can > be defined by the user. > > Thomas > >
On Mon, Jan 16, 2017 at 10:53:07AM -0800, Alistair Francis wrote: > On Sun, Jan 15, 2017 at 11:59 PM, Thomas Huth <thuth@redhat.com> wrote: > > On 14.01.2017 12:03, Laurent Vivier wrote: > >> Le 14/01/2017 à 07:51, Thomas Huth a écrit : > >>> 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. > >> > >> Did you try to use the generic-loader to load the kernel? > >> > >> Something like "-device loader,file=vmlinux" instead of adding this part > >> in the none machine? > > > > It does not work by default - because we need a way to set the CPU's > > program counter to the entry point of the ELF file. > > The -device loader logic can set the PC to the entry point of the ELF. > You just need to specify 'cpu-num=x' when loading the file. > > > But I think the users also expect the "-kernel" parameter to be working, > > so I think we should add the loader code in null-machine.c anyway. > > I agree that uses probably expect the '-kernel' option to work as well. So, is it possible to write a generic load_kernel() function that simply reuses the generic-loader code?
On 16 January 2017 at 19:25, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jan 16, 2017 at 10:53:07AM -0800, Alistair Francis wrote: >> On Sun, Jan 15, 2017 at 11:59 PM, Thomas Huth <thuth@redhat.com> wrote: >> > But I think the users also expect the "-kernel" parameter to be working, >> > so I think we should add the loader code in null-machine.c anyway. >> >> I agree that uses probably expect the '-kernel' option to work as well. > > So, is it possible to write a generic load_kernel() function that > simply reuses the generic-loader code? No, because users expect -kernel to actually load a Linux kernel (meaning with the calling conventions etc the kernel requires), whereas generic-loader is just "load a binary blob and start there". (-kernel is arch-specific and messy and doesn't behave the same way on all targets either :-() thanks -- PMM
On Mon, Jan 16, 2017 at 07:27:21PM +0000, Peter Maydell wrote: > On 16 January 2017 at 19:25, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Jan 16, 2017 at 10:53:07AM -0800, Alistair Francis wrote: > >> On Sun, Jan 15, 2017 at 11:59 PM, Thomas Huth <thuth@redhat.com> wrote: > >> > But I think the users also expect the "-kernel" parameter to be working, > >> > so I think we should add the loader code in null-machine.c anyway. > >> > >> I agree that uses probably expect the '-kernel' option to work as well. > > > > So, is it possible to write a generic load_kernel() function that > > simply reuses the generic-loader code? > > No, because users expect -kernel to actually load a Linux kernel > (meaning with the calling conventions etc the kernel requires), > whereas generic-loader is just "load a binary blob and start there". I don't mean a generic function that works for all machines and architectures, but a generic function that is good enough for "-machine none". Isn't "load a binary blob and start there" exactly what machine_none_load_kernel() in this patch does? > (-kernel is arch-specific and messy and doesn't behave the same > way on all targets either :-() I assume this means the kernel loading code used by "-machine none" will eventually need to call a arch-specific hook, then. But while we don't have arch-specific hooks implemented, isn't an implementation based on generic-loader good enough for some cases?
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 <thuth@redhat.com> > > 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
On Mon, Jan 16, 2017 at 08:52:28PM +0100, Thomas Huth wrote: > 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 <thuth@redhat.com> > > > > 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. Yeah. I would love to avoid adding another static variable to QEMU, but maybe this is good enough to avoid making the changes too intrusive. Anyway, I believe it will be possible to replace machine_none_load_kernel() with a simple instantiation of TYPE_GENERIC_LOADER, which already takes care of CPU reset. > > >> +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... I assume we will want to make "-machine none -kernel ..." work on those other architectures too, eventually. In this case, we need to keep in mind that this implementation is just a generic default implementation, but that may be overridden by arch-specific implementations in the future. (Probably through a CPUClass::load_kernel hook?) > > > 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. Agreed. > > >> 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 >
On 16 January 2017 at 19:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jan 16, 2017 at 07:27:21PM +0000, Peter Maydell wrote: >> On 16 January 2017 at 19:25, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Mon, Jan 16, 2017 at 10:53:07AM -0800, Alistair Francis wrote: >> >> On Sun, Jan 15, 2017 at 11:59 PM, Thomas Huth <thuth@redhat.com> wrote: >> >> > But I think the users also expect the "-kernel" parameter to be working, >> >> > so I think we should add the loader code in null-machine.c anyway. >> >> >> >> I agree that uses probably expect the '-kernel' option to work as well. >> > >> > So, is it possible to write a generic load_kernel() function that >> > simply reuses the generic-loader code? >> >> No, because users expect -kernel to actually load a Linux kernel >> (meaning with the calling conventions etc the kernel requires), >> whereas generic-loader is just "load a binary blob and start there". > > I don't mean a generic function that works for all machines and > architectures, but a generic function that is good enough for > "-machine none". Isn't "load a binary blob and start there" > exactly what machine_none_load_kernel() in this patch does? If you just want "load a blob and start it" then we already have -device loader. Making -kernel have yet another set of semantics that this time depends on the machine being selected seems like a bad idea. If -kernel doesn't do what it does for the other machines of the same architecture then we should just not accept it. thanks -- PMM
On Tue, Jan 17, 2017 at 09:29:19AM +0000, Peter Maydell wrote: > On 16 January 2017 at 19:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Jan 16, 2017 at 07:27:21PM +0000, Peter Maydell wrote: > >> On 16 January 2017 at 19:25, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > On Mon, Jan 16, 2017 at 10:53:07AM -0800, Alistair Francis wrote: > >> >> On Sun, Jan 15, 2017 at 11:59 PM, Thomas Huth <thuth@redhat.com> wrote: > >> >> > But I think the users also expect the "-kernel" parameter to be working, > >> >> > so I think we should add the loader code in null-machine.c anyway. > >> >> > >> >> I agree that uses probably expect the '-kernel' option to work as well. > >> > > >> > So, is it possible to write a generic load_kernel() function that > >> > simply reuses the generic-loader code? > >> > >> No, because users expect -kernel to actually load a Linux kernel > >> (meaning with the calling conventions etc the kernel requires), > >> whereas generic-loader is just "load a binary blob and start there". > > > > I don't mean a generic function that works for all machines and > > architectures, but a generic function that is good enough for > > "-machine none". Isn't "load a binary blob and start there" > > exactly what machine_none_load_kernel() in this patch does? > > If you just want "load a blob and start it" then we already > have -device loader. Making -kernel have yet another set of > semantics that this time depends on the machine being selected > seems like a bad idea. If -kernel doesn't do what it does > for the other machines of the same architecture then we should > just not accept it. Good point. In this case, implementing -kernel on "-machine none" will require calling an arch-specific hook from the beginning. I'm not sure if it's worth the effort, but I won't object if somebody really wants to implement that. While this is not implemented, I suggest we make "-machine none" reject -kernel instead of silently ignoring it.
On 17.01.2017 13:36, Eduardo Habkost wrote: > On Tue, Jan 17, 2017 at 09:29:19AM +0000, Peter Maydell wrote: >> On 16 January 2017 at 19:44, Eduardo Habkost <ehabkost@redhat.com> wrote: >>> On Mon, Jan 16, 2017 at 07:27:21PM +0000, Peter Maydell wrote: >>>> On 16 January 2017 at 19:25, Eduardo Habkost <ehabkost@redhat.com> wrote: >>>>> On Mon, Jan 16, 2017 at 10:53:07AM -0800, Alistair Francis wrote: >>>>>> On Sun, Jan 15, 2017 at 11:59 PM, Thomas Huth <thuth@redhat.com> wrote: >>>>>>> But I think the users also expect the "-kernel" parameter to be working, >>>>>>> so I think we should add the loader code in null-machine.c anyway. >>>>>> >>>>>> I agree that uses probably expect the '-kernel' option to work as well. >>>>> >>>>> So, is it possible to write a generic load_kernel() function that >>>>> simply reuses the generic-loader code? >>>> >>>> No, because users expect -kernel to actually load a Linux kernel >>>> (meaning with the calling conventions etc the kernel requires), >>>> whereas generic-loader is just "load a binary blob and start there". >>> >>> I don't mean a generic function that works for all machines and >>> architectures, but a generic function that is good enough for >>> "-machine none". Isn't "load a binary blob and start there" >>> exactly what machine_none_load_kernel() in this patch does? >> >> If you just want "load a blob and start it" then we already >> have -device loader. Making -kernel have yet another set of >> semantics that this time depends on the machine being selected >> seems like a bad idea. If -kernel doesn't do what it does >> for the other machines of the same architecture then we should >> just not accept it. > > Good point. In this case, implementing -kernel on "-machine none" > will require calling an arch-specific hook from the beginning. > I'm not sure if it's worth the effort, but I won't object if > somebody really wants to implement that. The -kernel parameter is not just only dependent on the target architecture, it is even dependent on the machine type, e.g. on ppc it is quite different between embedded (e500.c) and server (spapr.c) variants. So an arch-specific hook might not make too much sense and using the generic loader is likely the best we can do - if that does not work, you likely can't use the "none" machine anyway. > While this is not implemented, I suggest we make "-machine none" > reject -kernel instead of silently ignoring it. I'd prefer to use the generic loader for "-kernel" as a shortcut to "-device loader,file=..." here, but if the common sense is rather to refuse the -kernel option on the "none" machine, that's OK for me, too. Anybody else got an opinion on this? Thomas
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 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; + +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_NONE, 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; + } +} + +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); + } + + /* RAM at address zero */ + if (ram_size) { + ram = g_new(MemoryRegion, 1); + memory_region_allocate_system_memory(ram, NULL, "ram", ram_size); + memory_region_add_subregion(get_system_memory(), 0, ram); + } + + if (machine->kernel_filename) { + machine_none_load_kernel(cpu, machine->kernel_filename, ram_size); + } } static void machine_none_machine_init(MachineClass *mc) { mc->desc = "empty machine"; mc->init = machine_none_init; - mc->max_cpus = 0; + mc->default_ram_size = 0; } DEFINE_MACHINE("none", machine_none_machine_init)
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 <thuth@redhat.com> --- hw/core/Makefile.objs | 2 +- hw/core/null-machine.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 2 deletions(-)