[Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Thomas Huth]

We can have basic support for the "-kernel" parameter quite easily
by using the generic loader device. This should be enough for most
boards which do not need special machine-specific magic for loading
a kernel (and for those that need special magic, the generic "none"
machine is likely not suitable for using it as an instruction set
simulator board anyway).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 PS: If we can't agree on using the generic loader here, I can also
     prepare a patch instead that simply prints out an error message
     if the user tried to use the "-kernel" parameter.

 hw/core/null-machine.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
index 27c8369..866e699 100644
--- a/hw/core/null-machine.c
+++ b/hw/core/null-machine.c
@@ -5,6 +5,7 @@
  *
  * Authors:
  *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Thomas Huth       <thuth@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -16,6 +17,7 @@
 #include "qemu/error-report.h"
 #include "hw/hw.h"
 #include "hw/boards.h"
+#include "hw/core/generic-loader.h"
 #include "sysemu/sysemu.h"
 #include "exec/address-spaces.h"
 #include "cpu.h"
@@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
         memory_region_allocate_system_memory(ram, NULL, "ram", mch->ram_size);
         memory_region_add_subregion(get_system_memory(), 0, ram);
     }
+
+    /* Load kernel */
+    if (mch->kernel_filename) {
+        DeviceState *loader;
+
+        loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
+        qdev_prop_set_string(loader, "file", mch->kernel_filename);
+        if (cpu) {
+            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
+        }
+        qdev_init_nofail(loader);
+    }
 }
 
 static void machine_none_machine_init(MachineClass *mc)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Laurent Vivier]

Le 25/01/2017 à 09:40, Thomas Huth a écrit :
> We can have basic support for the "-kernel" parameter quite easily
> by using the generic loader device. This should be enough for most
> boards which do not need special machine-specific magic for loading
> a kernel (and for those that need special magic, the generic "none"
> machine is likely not suitable for using it as an instruction set
> simulator board anyway).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  PS: If we can't agree on using the generic loader here, I can also
>      prepare a patch instead that simply prints out an error message
>      if the user tried to use the "-kernel" parameter.
> 
>  hw/core/null-machine.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> index 27c8369..866e699 100644
> --- a/hw/core/null-machine.c
> +++ b/hw/core/null-machine.c
> @@ -5,6 +5,7 @@
>   *
>   * Authors:
>   *  Anthony Liguori   <aliguori@us.ibm.com>
> + *  Thomas Huth       <thuth@redhat.com>
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -16,6 +17,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "hw/boards.h"
> +#include "hw/core/generic-loader.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/address-spaces.h"
>  #include "cpu.h"
> @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
>          memory_region_allocate_system_memory(ram, NULL, "ram", mch->ram_size);
>          memory_region_add_subregion(get_system_memory(), 0, ram);
>      }
> +
> +    /* Load kernel */
> +    if (mch->kernel_filename) {
> +        DeviceState *loader;
> +
> +        loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
> +        qdev_prop_set_string(loader, "file", mch->kernel_filename);
> +        if (cpu) {
> +            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
> +        }
> +        qdev_init_nofail(loader);
> +    }
>  }

It seems you need to check "-cpu" is set otherwise we have a segfault in
the loader:

Thread 1 "qemu-system-m68" received signal SIGSEGV, Segmentation fault.
...
#0  0x000055555564e5f8 in generic_loader_realize (dev=<optimized out>,
errp=0x7fffffffd900) at hw/core/generic-loader.c:141

140         if (!s->force_raw) {
141             size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL,
142                                big_endian, 0, 0, 0, s->cpu->as);
143

(gdb) p s->cpu
$2 = (CPUState *) 0x0

Laurent

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Thomas Huth]

On 25.01.2017 15:42, Laurent Vivier wrote:
> Le 25/01/2017 à 09:40, Thomas Huth a écrit :
>> We can have basic support for the "-kernel" parameter quite easily
>> by using the generic loader device. This should be enough for most
>> boards which do not need special machine-specific magic for loading
>> a kernel (and for those that need special magic, the generic "none"
>> machine is likely not suitable for using it as an instruction set
>> simulator board anyway).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  PS: If we can't agree on using the generic loader here, I can also
>>      prepare a patch instead that simply prints out an error message
>>      if the user tried to use the "-kernel" parameter.
>>
>>  hw/core/null-machine.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>> index 27c8369..866e699 100644
>> --- a/hw/core/null-machine.c
>> +++ b/hw/core/null-machine.c
>> @@ -5,6 +5,7 @@
>>   *
>>   * Authors:
>>   *  Anthony Liguori   <aliguori@us.ibm.com>
>> + *  Thomas Huth       <thuth@redhat.com>
>>   *
>>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>   * See the COPYING file in the top-level directory.
>> @@ -16,6 +17,7 @@
>>  #include "qemu/error-report.h"
>>  #include "hw/hw.h"
>>  #include "hw/boards.h"
>> +#include "hw/core/generic-loader.h"
>>  #include "sysemu/sysemu.h"
>>  #include "exec/address-spaces.h"
>>  #include "cpu.h"
>> @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
>>          memory_region_allocate_system_memory(ram, NULL, "ram", mch->ram_size);
>>          memory_region_add_subregion(get_system_memory(), 0, ram);
>>      }
>> +
>> +    /* Load kernel */
>> +    if (mch->kernel_filename) {
>> +        DeviceState *loader;
>> +
>> +        loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
>> +        qdev_prop_set_string(loader, "file", mch->kernel_filename);
>> +        if (cpu) {
>> +            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
>> +        }
>> +        qdev_init_nofail(loader);
>> +    }
>>  }
> 
> It seems you need to check "-cpu" is set otherwise we have a segfault in
> the loader:
> 
> Thread 1 "qemu-system-m68" received signal SIGSEGV, Segmentation fault.
> ...
> #0  0x000055555564e5f8 in generic_loader_realize (dev=<optimized out>,
> errp=0x7fffffffd900) at hw/core/generic-loader.c:141
> 
> 140         if (!s->force_raw) {
> 141             size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL,
> 142                                big_endian, 0, 0, 0, s->cpu->as);
> 143
> 
> (gdb) p s->cpu
> $2 = (CPUState *) 0x0

Oh, nice catch! ... but I think this should rather be fixed in the
generic-loader instead, e.g. by using get_system_memory() instead of
s->cpu->as if s->cpu is NULL. Otherwise you can still trigger the crash
if using the loader device directly, e.g. with "-M none -device
loader,file=something". I'll send a separate patch for this...

 Thomas

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Thomas Huth]

On 25.01.2017 09:40, Thomas Huth wrote:
> We can have basic support for the "-kernel" parameter quite easily
> by using the generic loader device. This should be enough for most
> boards which do not need special machine-specific magic for loading
> a kernel (and for those that need special magic, the generic "none"
> machine is likely not suitable for using it as an instruction set
> simulator board anyway).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  PS: If we can't agree on using the generic loader here, I can also
>      prepare a patch instead that simply prints out an error message
>      if the user tried to use the "-kernel" parameter.
> 
>  hw/core/null-machine.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> index 27c8369..866e699 100644
> --- a/hw/core/null-machine.c
> +++ b/hw/core/null-machine.c
> @@ -5,6 +5,7 @@
>   *
>   * Authors:
>   *  Anthony Liguori   <aliguori@us.ibm.com>
> + *  Thomas Huth       <thuth@redhat.com>
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -16,6 +17,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "hw/boards.h"
> +#include "hw/core/generic-loader.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/address-spaces.h"
>  #include "cpu.h"
> @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
>          memory_region_allocate_system_memory(ram, NULL, "ram", mch->ram_size);
>          memory_region_add_subregion(get_system_memory(), 0, ram);
>      }
> +
> +    /* Load kernel */
> +    if (mch->kernel_filename) {
> +        DeviceState *loader;
> +
> +        loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
> +        qdev_prop_set_string(loader, "file", mch->kernel_filename);
> +        if (cpu) {
> +            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
> +        }
> +        qdev_init_nofail(loader);
> +    }
>  }
>  
>  static void machine_none_machine_init(MachineClass *mc)

*ping*

Apparently the discussion has ceased ... can we get a consensus whether
we want to support the "-kernel" parameter for the "none" machine or not?

 Thomas

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Marcel Apfelbaum]

On 02/27/2017 01:43 PM, Thomas Huth wrote:
> On 25.01.2017 09:40, Thomas Huth wrote:
>> We can have basic support for the "-kernel" parameter quite easily
>> by using the generic loader device. This should be enough for most
>> boards which do not need special machine-specific magic for loading
>> a kernel (and for those that need special magic, the generic "none"
>> machine is likely not suitable for using it as an instruction set
>> simulator board anyway).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  PS: If we can't agree on using the generic loader here, I can also
>>      prepare a patch instead that simply prints out an error message
>>      if the user tried to use the "-kernel" parameter.
>>
>>  hw/core/null-machine.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>> index 27c8369..866e699 100644
>> --- a/hw/core/null-machine.c
>> +++ b/hw/core/null-machine.c
>> @@ -5,6 +5,7 @@
>>   *
>>   * Authors:
>>   *  Anthony Liguori   <aliguori@us.ibm.com>
>> + *  Thomas Huth       <thuth@redhat.com>
>>   *
>>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>   * See the COPYING file in the top-level directory.
>> @@ -16,6 +17,7 @@
>>  #include "qemu/error-report.h"
>>  #include "hw/hw.h"
>>  #include "hw/boards.h"
>> +#include "hw/core/generic-loader.h"
>>  #include "sysemu/sysemu.h"
>>  #include "exec/address-spaces.h"
>>  #include "cpu.h"
>> @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
>>          memory_region_allocate_system_memory(ram, NULL, "ram", mch->ram_size);
>>          memory_region_add_subregion(get_system_memory(), 0, ram);
>>      }
>> +
>> +    /* Load kernel */
>> +    if (mch->kernel_filename) {
>> +        DeviceState *loader;
>> +
>> +        loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
>> +        qdev_prop_set_string(loader, "file", mch->kernel_filename);
>> +        if (cpu) {
>> +            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
>> +        }
>> +        qdev_init_nofail(loader);
>> +    }
>>  }
>>
>>  static void machine_none_machine_init(MachineClass *mc)
>
> *ping*
>
> Apparently the discussion has ceased ... can we get a consensus whether
> we want to support the "-kernel" parameter for the "none" machine or not?
>

Why do we need the "-kernel" parameter for the null-machine? I mean - what are the use cases?

Thanks,
Marcel

>  Thomas
>

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Eduardo Habkost]

On Mon, Feb 27, 2017 at 12:43:23PM +0100, Thomas Huth wrote:
> On 25.01.2017 09:40, Thomas Huth wrote:
> > We can have basic support for the "-kernel" parameter quite easily
> > by using the generic loader device. This should be enough for most
> > boards which do not need special machine-specific magic for loading
> > a kernel (and for those that need special magic, the generic "none"
> > machine is likely not suitable for using it as an instruction set
> > simulator board anyway).
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  PS: If we can't agree on using the generic loader here, I can also
> >      prepare a patch instead that simply prints out an error message
> >      if the user tried to use the "-kernel" parameter.
> > 
> >  hw/core/null-machine.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> > index 27c8369..866e699 100644
> > --- a/hw/core/null-machine.c
> > +++ b/hw/core/null-machine.c
> > @@ -5,6 +5,7 @@
> >   *
> >   * Authors:
> >   *  Anthony Liguori   <aliguori@us.ibm.com>
> > + *  Thomas Huth       <thuth@redhat.com>
> >   *
> >   * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >   * See the COPYING file in the top-level directory.
> > @@ -16,6 +17,7 @@
> >  #include "qemu/error-report.h"
> >  #include "hw/hw.h"
> >  #include "hw/boards.h"
> > +#include "hw/core/generic-loader.h"
> >  #include "sysemu/sysemu.h"
> >  #include "exec/address-spaces.h"
> >  #include "cpu.h"
> > @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
> >          memory_region_allocate_system_memory(ram, NULL, "ram", mch->ram_size);
> >          memory_region_add_subregion(get_system_memory(), 0, ram);
> >      }
> > +
> > +    /* Load kernel */
> > +    if (mch->kernel_filename) {
> > +        DeviceState *loader;
> > +
> > +        loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
> > +        qdev_prop_set_string(loader, "file", mch->kernel_filename);
> > +        if (cpu) {
> > +            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
> > +        }
> > +        qdev_init_nofail(loader);
> > +    }
> >  }
> >  
> >  static void machine_none_machine_init(MachineClass *mc)
> 
> *ping*
> 
> Apparently the discussion has ceased ... can we get a consensus whether
> we want to support the "-kernel" parameter for the "none" machine or not?

I think Peter's point is still valid:

] 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.

If you add a mechanism to ensure "-machine none -kernel" has the
same behavior as the other machines in the same QEMU binary, then
I believe it will be OK.

I believe we don't need to make this work on all architectures at
the same time, though. We can do this using an optional
per-architecture kernel-loading hook.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Thomas Huth]

On 27.02.2017 14:16, Marcel Apfelbaum wrote:
> On 02/27/2017 01:43 PM, Thomas Huth wrote:
>> On 25.01.2017 09:40, Thomas Huth wrote:
>>> We can have basic support for the "-kernel" parameter quite easily
>>> by using the generic loader device. This should be enough for most
>>> boards which do not need special machine-specific magic for loading
>>> a kernel (and for those that need special magic, the generic "none"
>>> machine is likely not suitable for using it as an instruction set
>>> simulator board anyway).
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  PS: If we can't agree on using the generic loader here, I can also
>>>      prepare a patch instead that simply prints out an error message
>>>      if the user tried to use the "-kernel" parameter.
>>>
>>>  hw/core/null-machine.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>>> index 27c8369..866e699 100644
>>> --- a/hw/core/null-machine.c
>>> +++ b/hw/core/null-machine.c
>>> @@ -5,6 +5,7 @@
>>>   *
>>>   * Authors:
>>>   *  Anthony Liguori   <aliguori@us.ibm.com>
>>> + *  Thomas Huth       <thuth@redhat.com>
>>>   *
>>>   * This work is licensed under the terms of the GNU GPL, version 2
>>> or later.
>>>   * See the COPYING file in the top-level directory.
>>> @@ -16,6 +17,7 @@
>>>  #include "qemu/error-report.h"
>>>  #include "hw/hw.h"
>>>  #include "hw/boards.h"
>>> +#include "hw/core/generic-loader.h"
>>>  #include "sysemu/sysemu.h"
>>>  #include "exec/address-spaces.h"
>>>  #include "cpu.h"
>>> @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
>>>          memory_region_allocate_system_memory(ram, NULL, "ram",
>>> mch->ram_size);
>>>          memory_region_add_subregion(get_system_memory(), 0, ram);
>>>      }
>>> +
>>> +    /* Load kernel */
>>> +    if (mch->kernel_filename) {
>>> +        DeviceState *loader;
>>> +
>>> +        loader = qdev_create(sysbus_get_default(),
>>> TYPE_GENERIC_LOADER);
>>> +        qdev_prop_set_string(loader, "file", mch->kernel_filename);
>>> +        if (cpu) {
>>> +            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
>>> +        }
>>> +        qdev_init_nofail(loader);
>>> +    }
>>>  }
>>>
>>>  static void machine_none_machine_init(MachineClass *mc)
>>
>> *ping*
>>
>> Apparently the discussion has ceased ... can we get a consensus whether
>> we want to support the "-kernel" parameter for the "none" machine or not?
>>
> 
> Why do we need the "-kernel" parameter for the null-machine? I mean -
> what are the use cases?

Since it is now possible to instantiate a CPU and memory with the
null-machine, you can use it as a simple instruction set simulator board
now. Start QEMU with something like "-M none -cpu xxx -m 1G -S -s" and
then connect a remote GDB to interact with the emulated CPU. This is
e.g. useful for CPU models that do not have a matching emulated machine
yet. Adding the possibility to use the "-kernel" parameter for this,
too, would make things a little bit more comfortable on certain targets,
since you then don't have to fiddle with the generic-loader device.

 Thomas

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Peter Maydell]

On 27 February 2017 at 17:49, Thomas Huth <thuth@redhat.com> wrote:
> On 27.02.2017 14:16, Marcel Apfelbaum wrote:
>> Why do we need the "-kernel" parameter for the null-machine? I mean -
>> what are the use cases?
>
> Since it is now possible to instantiate a CPU and memory with the
> null-machine, you can use it as a simple instruction set simulator board
> now. Start QEMU with something like "-M none -cpu xxx -m 1G -S -s" and
> then connect a remote GDB to interact with the emulated CPU. This is
> e.g. useful for CPU models that do not have a matching emulated machine
> yet. Adding the possibility to use the "-kernel" parameter for this,
> too, would make things a little bit more comfortable on certain targets,
> since you then don't have to fiddle with the generic-loader device.

But it still won't do the expected magic -kernel does for you on x86
or ARM or..., so it's just confusing to use the same option. We have
a command line option for "just load the binary please", and that's
the generic-loader. If you think generic-loader's syntax is a bit
tedious you could argue for a convenience option that was syntactic
sugar for it (in the same way that -hda is sugar for -drive and
-device options), but that shouldn't be limited to the null-machine.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Thomas Huth]

On 27.02.2017 15:00, Eduardo Habkost wrote:
> On Mon, Feb 27, 2017 at 12:43:23PM +0100, Thomas Huth wrote:
>> On 25.01.2017 09:40, Thomas Huth wrote:
>>> We can have basic support for the "-kernel" parameter quite easily
>>> by using the generic loader device. This should be enough for most
>>> boards which do not need special machine-specific magic for loading
>>> a kernel (and for those that need special magic, the generic "none"
>>> machine is likely not suitable for using it as an instruction set
>>> simulator board anyway).
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  PS: If we can't agree on using the generic loader here, I can also
>>>      prepare a patch instead that simply prints out an error message
>>>      if the user tried to use the "-kernel" parameter.
>>>
>>>  hw/core/null-machine.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>>> index 27c8369..866e699 100644
>>> --- a/hw/core/null-machine.c
>>> +++ b/hw/core/null-machine.c
>>> @@ -5,6 +5,7 @@
>>>   *
>>>   * Authors:
>>>   *  Anthony Liguori   <aliguori@us.ibm.com>
>>> + *  Thomas Huth       <thuth@redhat.com>
>>>   *
>>>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>   * See the COPYING file in the top-level directory.
>>> @@ -16,6 +17,7 @@
>>>  #include "qemu/error-report.h"
>>>  #include "hw/hw.h"
>>>  #include "hw/boards.h"
>>> +#include "hw/core/generic-loader.h"
>>>  #include "sysemu/sysemu.h"
>>>  #include "exec/address-spaces.h"
>>>  #include "cpu.h"
>>> @@ -40,6 +42,18 @@ static void machine_none_init(MachineState *mch)
>>>          memory_region_allocate_system_memory(ram, NULL, "ram", mch->ram_size);
>>>          memory_region_add_subregion(get_system_memory(), 0, ram);
>>>      }
>>> +
>>> +    /* Load kernel */
>>> +    if (mch->kernel_filename) {
>>> +        DeviceState *loader;
>>> +
>>> +        loader = qdev_create(sysbus_get_default(), TYPE_GENERIC_LOADER);
>>> +        qdev_prop_set_string(loader, "file", mch->kernel_filename);
>>> +        if (cpu) {
>>> +            qdev_prop_set_uint32(loader, "cpu-num", cpu->cpu_index);
>>> +        }
>>> +        qdev_init_nofail(loader);
>>> +    }
>>>  }
>>>  
>>>  static void machine_none_machine_init(MachineClass *mc)
>>
>> *ping*
>>
>> Apparently the discussion has ceased ... can we get a consensus whether
>> we want to support the "-kernel" parameter for the "none" machine or not?
> 
> I think Peter's point is still valid:
> 
> ] 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.
> 
> If you add a mechanism to ensure "-machine none -kernel" has the
> same behavior as the other machines in the same QEMU binary, then
> I believe it will be OK.
> 
> I believe we don't need to make this work on all architectures at
> the same time, though. We can do this using an optional
> per-architecture kernel-loading hook.

And I still think it does not really make sense to introduce a
per-architecture hook here - let me cite my reply to Peter's mail a
couple of mails later in that e-mail thread:

'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.'

So as far as I can see, we should either go with the generic loader
here, or print out an error message if the user tries to run QEMU with
the "-kernel" parameter.

 Thomas

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Peter Maydell]

On 27 February 2017 at 17:57, Thomas Huth <thuth@redhat.com> wrote:
> And I still think it does not really make sense to introduce a
> per-architecture hook here - let me cite my reply to Peter's mail a
> couple of mails later in that e-mail thread:
>
> '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.'
>
> So as far as I can see, we should either go with the generic loader
> here, or print out an error message if the user tries to run QEMU with
> the "-kernel" parameter.

I would go for "don't support -kernel". The fundamental problem
with -kernel is that it tries to be a magic "do what I mean"
command line argument. Adding extra magic is going to make things
worse, not better.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Thomas Huth]

On 27.02.2017 19:00, Peter Maydell wrote:
> On 27 February 2017 at 17:57, Thomas Huth <thuth@redhat.com> wrote:
>> And I still think it does not really make sense to introduce a
>> per-architecture hook here - let me cite my reply to Peter's mail a
>> couple of mails later in that e-mail thread:
>>
>> '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.'
>>
>> So as far as I can see, we should either go with the generic loader
>> here, or print out an error message if the user tries to run QEMU with
>> the "-kernel" parameter.
> 
> I would go for "don't support -kernel". The fundamental problem
> with -kernel is that it tries to be a magic "do what I mean"
> command line argument. Adding extra magic is going to make things
> worse, not better.

OK, fair, then I'll try to come up with a patch that prints out an error
message instead if the user tries to use the "-kernel" parameter there...

 Thomas

Re: [Qemu-devel] [PATCH] null-machine: Add support for the "-kernel" parameter

[Eduardo Habkost]

On Mon, Feb 27, 2017 at 07:10:23PM +0100, Thomas Huth wrote:
> On 27.02.2017 19:00, Peter Maydell wrote:
> > On 27 February 2017 at 17:57, Thomas Huth <thuth@redhat.com> wrote:
> >> And I still think it does not really make sense to introduce a
> >> per-architecture hook here - let me cite my reply to Peter's mail a
> >> couple of mails later in that e-mail thread:
> >>
> >> '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.'
> >>
> >> So as far as I can see, we should either go with the generic loader
> >> here, or print out an error message if the user tries to run QEMU with
> >> the "-kernel" parameter.
> > 
> > I would go for "don't support -kernel". The fundamental problem
> > with -kernel is that it tries to be a magic "do what I mean"
> > command line argument. Adding extra magic is going to make things
> > worse, not better.
> 
> OK, fair, then I'll try to come up with a patch that prints out an error
> message instead if the user tries to use the "-kernel" parameter there...

Agreed.

-- 
Eduardo