Patchwork boards: add a 'none' machine type to all platforms

login
register
mail settings
Submitter Anthony Liguori
Date Aug. 22, 2012, 8:24 p.m.
Message ID <1345667067-24298-1-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/179403/
State New
Headers show

Comments

Anthony Liguori - Aug. 22, 2012, 8:24 p.m.
This allows any QEMU binary to be executed with:

  $QEMU_BINARY -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.

Cc: Daniel Berrange <berrange@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/Makefile.objs  |    2 ++
 hw/null-machine.c |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100644 hw/null-machine.c
Eric Blake - Aug. 22, 2012, 8:52 p.m.
On 08/22/2012 02:24 PM, Anthony Liguori wrote:
> This allows any QEMU binary to be executed with:
> 
>   $QEMU_BINARY -qmp stdio

Don't you mean:

$QEMU_BINARY -M none -qmp stdio

I agree with including this in 1.2, as otherwise your new query-target
and other commands are incomplete (that is, this is a 'bug fix' of
rounding out a feature already promised at hard freeze, and not a new
feature on its own).

> +static QEMUMachine machine_none = {
> +    .name = "none",
> +    .desc = "empty machine",
> +    .init = machine_none_init,
> +    .max_cpus = 0,
> +};

I guess libvirt just blindly tries '-S -M none'; if it works, we must be
talking to new enough qemu (and all the other QMP commands that we want
to probe are then immediately available); if it doesn't work, then we
must be talking to older qemu and can fall back to -help scraping (since
older versions won't be further modifying their -help output now that
they are released).  I like the idea, although I'm not familiar enough
with this part of the code to know if my review counts for anything:

Reviewed-by: Eric Blake <eblake@redhat.com>
Peter Maydell - Aug. 22, 2012, 8:53 p.m.
On 22 August 2012 21:24, Anthony Liguori <aliguori@us.ibm.com> wrote:
> This allows any QEMU binary to be executed with:
>
>   $QEMU_BINARY -qmp stdio

...presumably you mean -qmp stdio -M none ?

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

How about documenting this machine (and its purpose) somewhere?

> Cc: Daniel Berrange <berrange@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/Makefile.objs  |    2 ++
>  hw/null-machine.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 0 deletions(-)
>  create mode 100644 hw/null-machine.c
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 7f57ed5..6dfebd2 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -134,6 +134,8 @@ hw-obj-$(CONFIG_DP8393X) += dp8393x.o
>  hw-obj-$(CONFIG_DS1225Y) += ds1225y.o
>  hw-obj-$(CONFIG_MIPSNET) += mipsnet.o
>
> +hw-obj-y += null-machine.o
> +
>  # Sound
>  sound-obj-y =
>  sound-obj-$(CONFIG_SB16) += sb16.o
> diff --git a/hw/null-machine.c b/hw/null-machine.c
> new file mode 100644
> index 0000000..69910d3
> --- /dev/null
> +++ b/hw/null-machine.c
> @@ -0,0 +1,40 @@
> +/*
> + * Empty machine
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.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.
> + *
> + */
> +
> +#include "qemu-common.h"
> +#include "hw/hw.h"
> +#include "hw/boards.h"
> +
> +static void machine_none_init(ram_addr_t ram_size,
> +                              const char *boot_device,
> +                              const char *kernel_filename,
> +                              const char *kernel_cmdline,
> +                              const char *initrd_filename,
> +                              const char *cpu_model)
> +{
> +}
> +
> +static QEMUMachine machine_none = {
> +    .name = "none",
> +    .desc = "empty machine",
> +    .init = machine_none_init,
> +    .max_cpus = 0,
> +};
> +
> +static void register_machines(void)
> +{
> +    qemu_register_machine(&machine_none);
> +}
> +
> +machine_init(register_machines);

We seem to be about evenly split about whether machine_init()
should have a trailing semicolon (it doesn't need one
but it doesn't hurt either...)

-- PMM
Anthony Liguori - Aug. 22, 2012, 9:29 p.m.
Eric Blake <eblake@redhat.com> writes:

> On 08/22/2012 02:24 PM, Anthony Liguori wrote:
>> This allows any QEMU binary to be executed with:
>> 
>>   $QEMU_BINARY -qmp stdio
>
> Don't you mean:
>
> $QEMU_BINARY -M none -qmp stdio

I did, thanks.

>
> I agree with including this in 1.2, as otherwise your new query-target
> and other commands are incomplete (that is, this is a 'bug fix' of
> rounding out a feature already promised at hard freeze, and not a new
> feature on its own).
>
>> +static QEMUMachine machine_none = {
>> +    .name = "none",
>> +    .desc = "empty machine",
>> +    .init = machine_none_init,
>> +    .max_cpus = 0,
>> +};
>
> I guess libvirt just blindly tries '-S -M none'; if it works, we must be
> talking to new enough qemu (and all the other QMP commands that we want
> to probe are then immediately available);

Correct.  '-M none' will fail with a non-zero exit status in all old
versions of QEMU.

-S isn't really needed FWIW but it certainly doesn't hurt.  There aren't
any VCPUs created with -M none so strictly speaking, -S doesn't do
anything.

No ram is allocated with -M none either which is another nice touch
(less resource usage).

> if it doesn't work, then we
> must be talking to older qemu and can fall back to -help scraping (since
> older versions won't be further modifying their -help output now that
> they are released). 

Correct.

Regards,

Anthony Liguori

> I like the idea, although I'm not familiar enough
> with this part of the code to know if my review counts for anything:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Anthony Liguori - Aug. 22, 2012, 10:42 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 22 August 2012 21:24, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> This allows any QEMU binary to be executed with:
>>
>>   $QEMU_BINARY -qmp stdio
>
> ...presumably you mean -qmp stdio -M none ?
>
>>
>> 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.
>
> How about documenting this machine (and its purpose) somewhere?

Okay, but where?  I don't know an obvious place.

>> Cc: Daniel Berrange <berrange@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/Makefile.objs  |    2 ++
>>  hw/null-machine.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 42 insertions(+), 0 deletions(-)
>>  create mode 100644 hw/null-machine.c
>>
>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> index 7f57ed5..6dfebd2 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -134,6 +134,8 @@ hw-obj-$(CONFIG_DP8393X) += dp8393x.o
>>  hw-obj-$(CONFIG_DS1225Y) += ds1225y.o
>>  hw-obj-$(CONFIG_MIPSNET) += mipsnet.o
>>
>> +hw-obj-y += null-machine.o
>> +
>>  # Sound
>>  sound-obj-y =
>>  sound-obj-$(CONFIG_SB16) += sb16.o
>> diff --git a/hw/null-machine.c b/hw/null-machine.c
>> new file mode 100644
>> index 0000000..69910d3
>> --- /dev/null
>> +++ b/hw/null-machine.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Empty machine
>> + *
>> + * Copyright IBM, Corp. 2012
>> + *
>> + * Authors:
>> + *  Anthony Liguori   <aliguori@us.ibm.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.
>> + *
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "hw/hw.h"
>> +#include "hw/boards.h"
>> +
>> +static void machine_none_init(ram_addr_t ram_size,
>> +                              const char *boot_device,
>> +                              const char *kernel_filename,
>> +                              const char *kernel_cmdline,
>> +                              const char *initrd_filename,
>> +                              const char *cpu_model)
>> +{
>> +}
>> +
>> +static QEMUMachine machine_none = {
>> +    .name = "none",
>> +    .desc = "empty machine",
>> +    .init = machine_none_init,
>> +    .max_cpus = 0,
>> +};
>> +
>> +static void register_machines(void)
>> +{
>> +    qemu_register_machine(&machine_none);
>> +}
>> +
>> +machine_init(register_machines);
>
> We seem to be about evenly split about whether machine_init()
> should have a trailing semicolon (it doesn't need one
> but it doesn't hurt either...)

It's obviously superior to use a semicolon...  C is completely
consistent synactically about the usage of semicolons afterall :-)

Regards,

Anthony Liguori

>
> -- PMM
Peter Maydell - Aug. 22, 2012, 10:53 p.m.
On 22 August 2012 23:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> How about documenting this machine (and its purpose) somewhere?
>
> Okay, but where?  I don't know an obvious place.

If we hadn't got ourselves into this mess where we can't change
-help output, the obvious place would be in the qemu-options.hx
documentation of the -machine option.

Is it possible to mark up qemu-options.hx for "put this text
in the HTML docs but not the -help output" ?

-- PMM
Anthony Liguori - Aug. 23, 2012, 2:03 a.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 22 August 2012 23:42, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> How about documenting this machine (and its purpose) somewhere?
>>
>> Okay, but where?  I don't know an obvious place.
>
> If we hadn't got ourselves into this mess where we can't change
> -help output, the obvious place would be in the qemu-options.hx
> documentation of the -machine option.
>
> Is it possible to mark up qemu-options.hx for "put this text
> in the HTML docs but not the -help output" ?

Since 1.3 is just a couple weeks away from being open, let's just wait
until then and change the help output.

Regards,

Anthony Liguori

>
> -- PMM
Andreas Färber - Aug. 23, 2012, 12:03 p.m.
Am 23.08.2012 00:42, schrieb Anthony Liguori:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On 22 August 2012 21:24, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>> This allows any QEMU binary to be executed with:
>>>
>>>   $QEMU_BINARY -qmp stdio
>>
>> ...presumably you mean -qmp stdio -M none ?
>>
>>>
>>> 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.
>>
>> How about documenting this machine (and its purpose) somewhere?
> 
> Okay, but where?  I don't know an obvious place.
> 
>>> Cc: Daniel Berrange <berrange@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>>  hw/Makefile.objs  |    2 ++
>>>  hw/null-machine.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 42 insertions(+), 0 deletions(-)
>>>  create mode 100644 hw/null-machine.c
>>>
>>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>>> index 7f57ed5..6dfebd2 100644
>>> --- a/hw/Makefile.objs
>>> +++ b/hw/Makefile.objs
>>> @@ -134,6 +134,8 @@ hw-obj-$(CONFIG_DP8393X) += dp8393x.o
>>>  hw-obj-$(CONFIG_DS1225Y) += ds1225y.o
>>>  hw-obj-$(CONFIG_MIPSNET) += mipsnet.o
>>>
>>> +hw-obj-y += null-machine.o
>>> +
>>>  # Sound
>>>  sound-obj-y =
>>>  sound-obj-$(CONFIG_SB16) += sb16.o
>>> diff --git a/hw/null-machine.c b/hw/null-machine.c
>>> new file mode 100644
>>> index 0000000..69910d3
>>> --- /dev/null
>>> +++ b/hw/null-machine.c
>>> @@ -0,0 +1,40 @@
>>> +/*
>>> + * Empty machine
>>> + *
>>> + * Copyright IBM, Corp. 2012
>>> + *
>>> + * Authors:
>>> + *  Anthony Liguori   <aliguori@us.ibm.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.
>>> + *
>>> + */
>>> +
>>> +#include "qemu-common.h"
>>> +#include "hw/hw.h"
>>> +#include "hw/boards.h"
>>> +
>>> +static void machine_none_init(ram_addr_t ram_size,
>>> +                              const char *boot_device,
>>> +                              const char *kernel_filename,
>>> +                              const char *kernel_cmdline,
>>> +                              const char *initrd_filename,
>>> +                              const char *cpu_model)
>>> +{
>>> +}
>>> +
>>> +static QEMUMachine machine_none = {
>>> +    .name = "none",
>>> +    .desc = "empty machine",
>>> +    .init = machine_none_init,
>>> +    .max_cpus = 0,
>>> +};
>>> +
>>> +static void register_machines(void)
>>> +{
>>> +    qemu_register_machine(&machine_none);
>>> +}
>>> +
>>> +machine_init(register_machines);
>>
>> We seem to be about evenly split about whether machine_init()
>> should have a trailing semicolon (it doesn't need one
>> but it doesn't hurt either...)
> 
> It's obviously superior to use a semicolon...  C is completely
> consistent synactically about the usage of semicolons afterall :-)

I disagree. This macro does not turn into a statement but a full
function definition. Neither our Coding Style nor any other that I know
does

void foo(bar baz)
{
};

with a trailing semicolon. For type_init() I cleaned this up, and I
attribute the existing semicolons to people's ignorance of what these
macros actually do.

You might remember that Eduardo and I wanted to QOM'ify machines so that
machine_init() would get replaced with type_init() but you stopped us at
the time, leaving it untouched. So there's no strict need to go through
and change existing users, but I believe in setting good examples.

Regards,
Andreas
Anthony Liguori - Aug. 23, 2012, 1:09 p.m.
Andreas Färber <afaerber@suse.de> writes:

> Am 23.08.2012 00:42, schrieb Anthony Liguori:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>>> On 22 August 2012 21:24, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>
>>> We seem to be about evenly split about whether machine_init()
>>> should have a trailing semicolon (it doesn't need one
>>> but it doesn't hurt either...)
>> 
>> It's obviously superior to use a semicolon...  C is completely
>> consistent synactically about the usage of semicolons afterall :-)
>
> I disagree. This macro does not turn into a statement but a full
> function definition. Neither our Coding Style nor any other that I know
> does

I was only responding to be funny.  This is not something worth anyone
arguing about.  We've got *much* more important things to sort through.

Regards,

Anthony Liguori

>
> void foo(bar baz)
> {
> };
>
> with a trailing semicolon. For type_init() I cleaned this up, and I
> attribute the existing semicolons to people's ignorance of what these
> macros actually do.
>
> You might remember that Eduardo and I wanted to QOM'ify machines so that
> machine_init() would get replaced with type_init() but you stopped us at
> the time, leaving it untouched. So there's no strict need to go through
> and change existing users, but I believe in setting good examples.
>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 7f57ed5..6dfebd2 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -134,6 +134,8 @@  hw-obj-$(CONFIG_DP8393X) += dp8393x.o
 hw-obj-$(CONFIG_DS1225Y) += ds1225y.o
 hw-obj-$(CONFIG_MIPSNET) += mipsnet.o
 
+hw-obj-y += null-machine.o
+
 # Sound
 sound-obj-y =
 sound-obj-$(CONFIG_SB16) += sb16.o
diff --git a/hw/null-machine.c b/hw/null-machine.c
new file mode 100644
index 0000000..69910d3
--- /dev/null
+++ b/hw/null-machine.c
@@ -0,0 +1,40 @@ 
+/*
+ * Empty machine
+ *
+ * Copyright IBM, Corp. 2012
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.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.
+ *
+ */
+
+#include "qemu-common.h"
+#include "hw/hw.h"
+#include "hw/boards.h"
+
+static void machine_none_init(ram_addr_t ram_size,
+                              const char *boot_device,
+                              const char *kernel_filename,
+                              const char *kernel_cmdline,
+                              const char *initrd_filename,
+                              const char *cpu_model)
+{
+}
+
+static QEMUMachine machine_none = {
+    .name = "none",
+    .desc = "empty machine",
+    .init = machine_none_init,
+    .max_cpus = 0,
+};
+
+static void register_machines(void)
+{
+    qemu_register_machine(&machine_none);
+}
+
+machine_init(register_machines);
+