diff mbox

[v3,2/2] Add configure option --enable-pc-1-0-qemu-kvm

Message ID 1411310339-27733-3-git-send-email-alex@alex.org.uk
State New
Headers show

Commit Message

Alex Bligh Sept. 21, 2014, 2:38 p.m. UTC
Add a configure option --enable-pc-1-0-qemu-kvm and the
corresponding --disable-pc-1-0-qemu-kvm, defaulting
to disabled.

Rename machine type pc-1.0 to pc-1.0-qemu-git.

Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
or pc-1.0-qemu-git depending on the value of the config
option.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 configure         |   12 ++++++++++++
 hw/i386/pc_piix.c |    8 +++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

Comments

Michael S. Tsirkin Sept. 22, 2014, 11:36 a.m. UTC | #1
On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> Add a configure option --enable-pc-1-0-qemu-kvm and the
> corresponding --disable-pc-1-0-qemu-kvm, defaulting
> to disabled.
> 
> Rename machine type pc-1.0 to pc-1.0-qemu-git.
> 
> Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> or pc-1.0-qemu-git depending on the value of the config
> option.
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>

I have to say, this one bothers me.
We end up not being able to predict what does pc-1.0
reference.

Users also don't get qemu from git so I don't see
why does git make sense in the name?

Legacy management applications invoked qemu as qemu-kvm -
how about detecting that name and switching
the machine types?
It might make sense to also set -enable-kvm and
change default CPU to kvm64 in this case.


> ---
>  configure         |   12 ++++++++++++
>  hw/i386/pc_piix.c |    8 +++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index f7685b5..b143302 100755
> --- a/configure
> +++ b/configure
> @@ -335,6 +335,7 @@ libssh2=""
>  vhdx=""
>  quorum=""
>  numa=""
> +pc_1_0_qemu_kvm="no"
>  
>  # parse CC options first
>  for opt do
> @@ -1125,6 +1126,10 @@ for opt do
>    ;;
>    --enable-numa) numa="yes"
>    ;;
> +  --disable-pc-1-0-qemu-kvm) pc_1_0_qemu_kvm="no"
> +  ;;
> +  --enable-pc-1-0-qemu-kvm) pc_1_0_qemu_kvm="yes"
> +  ;;
>    *)
>        echo "ERROR: unknown option $opt"
>        echo "Try '$0 --help' for more information"
> @@ -1394,6 +1399,8 @@ Advanced options (experts only):
>    --enable-quorum          enable quorum block filter support
>    --disable-numa           disable libnuma support
>    --enable-numa            enable libnuma support
> +  --disable-pc-1-0-qemu-kvm disable pc-1.0 machine type reflecting qemu-kvm
> +  --enable-pc-1-0-qemu-kvm enable pc-1.0 machine type reflecting qemu-kvm
>  
>  NOTE: The object files are built at the place where configure is launched
>  EOF
> @@ -4262,6 +4269,7 @@ echo "Quorum            $quorum"
>  echo "lzo support       $lzo"
>  echo "snappy support    $snappy"
>  echo "NUMA host support $numa"
> +echo "pc-1.0 qemu-kvm   $pc_1_0_qemu_kvm"
>  
>  if test "$sdl_too_old" = "yes"; then
>  echo "-> Your SDL version is too old - please upgrade to have SDL support"
> @@ -5241,6 +5249,10 @@ if test "$numa" = "yes"; then
>    echo "CONFIG_NUMA=y" >> $config_host_mak
>  fi
>  
> +if test "$pc_1_0_qemu_kvm" = "yes"; then
> +  echo "CONFIG_PC_1_0_QEMU_KVM=y" >> $config_host_mak
> +fi
> +
>  # build tree in object directory in case the source is not in the current directory
>  DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests"
>  DIRS="$DIRS fsdev"
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 48a4942..b7a4af0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -646,7 +646,10 @@ static QEMUMachine pc_machine_v1_1 = {
>  
>  static QEMUMachine pc_machine_v1_0 = {
>      PC_I440FX_1_2_MACHINE_OPTIONS,
> -    .name = "pc-1.0",
> +    .name = "pc-1.0-qemu-git",
> +#ifndef CONFIG_PC_1_0_QEMU_KVM
> +    .alias = "pc-1.0",
> +#endif
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_0,
>          { /* end of list */ }
> @@ -665,6 +668,9 @@ static QEMUMachine pc_machine_v1_0 = {
>  static QEMUMachine pc_machine_v1_0_qemu_kvm = {
>      PC_I440FX_1_2_MACHINE_OPTIONS,
>      .name = "pc-1.0-qemu-kvm",
> +#ifdef CONFIG_PC_1_0_QEMU_KVM
> +    .alias = "pc-1.0",
> +#endif
>      .init = pc_init_pci_1_2_qemu_kvm,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_0_QEMU_KVM,
> -- 
> 1.7.9.5
Daniel P. Berrangé Sept. 22, 2014, 11:42 a.m. UTC | #2
On Mon, Sep 22, 2014 at 02:36:55PM +0300, Michael S. Tsirkin wrote:
> On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> > Add a configure option --enable-pc-1-0-qemu-kvm and the
> > corresponding --disable-pc-1-0-qemu-kvm, defaulting
> > to disabled.
> > 
> > Rename machine type pc-1.0 to pc-1.0-qemu-git.
> > 
> > Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> > or pc-1.0-qemu-git depending on the value of the config
> > option.
> > 
> > Signed-off-by: Alex Bligh <alex@alex.org.uk>
> 
> I have to say, this one bothers me.
> We end up not being able to predict what does pc-1.0
> reference.

Yeah, this is not good. Any single machine type name should have
fixed semantics - having two different semantics depending on build
options means mgmt apps can no longer simply compare the machine
type name to determine if it is a match with the same name on a
different host.


Regards,
Daniel
Alex Bligh Sept. 22, 2014, 11:50 a.m. UTC | #3
On 22 Sep 2014, at 12:36, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
>> Add a configure option --enable-pc-1-0-qemu-kvm and the
>> corresponding --disable-pc-1-0-qemu-kvm, defaulting
>> to disabled.
>> 
>> Rename machine type pc-1.0 to pc-1.0-qemu-git.
>> 
>> Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
>> or pc-1.0-qemu-git depending on the value of the config
>> option.
>> 
>> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> 
> I have to say, this one bothers me.
> We end up not being able to predict what does pc-1.0
> reference.

The whole point is to make it easy for distributions to make
pc-1.0 do an appropriate thing for them, an appropriate thing
being what their previous release(s) meant by pc-1.0. Sadly,
pc-1.0 means more than one thing - on Ubuntu it really means
"qemu-kvm's pc-1.0" and on other systems (and upstream) it
means "qemu-git's pc-1.0". This is horrible, but it's not
the fault of this patch, is simply current reality. Right
now, you can't tell what pc-1.0 does anyway, as it does a
different thing on Ubuntu Precise to Ubuntu Trusty (for instance).

If we're going to provide compatibility to the machine type
name level (which is the purpose here) then sadly we need
something like this.

As for 'not being able to predict', actually post this patch
the situation is far clearer, as '-M ?' tells you exactly
what pc-1.0 will do, by showing what it's an alias for.

> Users also don't get qemu from git so I don't see
> why does git make sense in the name?

Agree with that, but I couldn't think of anything better.
'qemu-upstream'? 'qemu'?

> Legacy management applications invoked qemu as qemu-kvm -
> how about detecting that name and switching
> the machine types?
> It might make sense to also set -enable-kvm a

Sadly that is not true. For instance on Ubuntu Precise
it's invoked as qemu-system-x86_64 by at least one
management application known to me.

I'm not quite sure why you say "legacy management
applications". This applies to /any/ management application.
Unless we're going to burden every management application
with this problem, we need to fix it.

Just as a reminder, the ./configure value defaults to
off, which means there is no change in current behaviour.
Only if the ./configure value is changed does it result
in the behaviour of pc-1.0 changing, and that is evident
from the help line. And it's possible to specify a specific
variant of 1.0 (irrespective of the default) from the
command line.
Michael S. Tsirkin Sept. 22, 2014, 11:53 a.m. UTC | #4
On Mon, Sep 22, 2014 at 12:42:47PM +0100, Daniel P. Berrange wrote:
> On Mon, Sep 22, 2014 at 02:36:55PM +0300, Michael S. Tsirkin wrote:
> > On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> > > Add a configure option --enable-pc-1-0-qemu-kvm and the
> > > corresponding --disable-pc-1-0-qemu-kvm, defaulting
> > > to disabled.
> > > 
> > > Rename machine type pc-1.0 to pc-1.0-qemu-git.
> > > 
> > > Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> > > or pc-1.0-qemu-git depending on the value of the config
> > > option.
> > > 
> > > Signed-off-by: Alex Bligh <alex@alex.org.uk>
> > 
> > I have to say, this one bothers me.
> > We end up not being able to predict what does pc-1.0
> > reference.
> 
> Yeah, this is not good. Any single machine type name should have
> fixed semantics - having two different semantics depending on build
> options means mgmt apps can no longer simply compare the machine
> type name to determine if it is a match with the same name on a
> different host.
> 
> 
> Regards,
> Daniel

Right. However, the fact remains that distros shipped qemu
and qemu-kvm that had pc-1.0 meaning different things.
What do you think about my idea to detect the meaning
for pc-1.0 based on argv[0]?


> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
Michael S. Tsirkin Sept. 22, 2014, 12:10 p.m. UTC | #5
On Mon, Sep 22, 2014 at 12:50:14PM +0100, Alex Bligh wrote:
> 
> On 22 Sep 2014, at 12:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> >> Add a configure option --enable-pc-1-0-qemu-kvm and the
> >> corresponding --disable-pc-1-0-qemu-kvm, defaulting
> >> to disabled.
> >> 
> >> Rename machine type pc-1.0 to pc-1.0-qemu-git.
> >> 
> >> Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> >> or pc-1.0-qemu-git depending on the value of the config
> >> option.
> >> 
> >> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> > 
> > I have to say, this one bothers me.
> > We end up not being able to predict what does pc-1.0
> > reference.
> 
> The whole point is to make it easy for distributions to make
> pc-1.0 do an appropriate thing for them, an appropriate thing
> being what their previous release(s) meant by pc-1.0. Sadly,
> pc-1.0 means more than one thing - on Ubuntu it really means
> "qemu-kvm's pc-1.0" and on other systems (and upstream) it
> means "qemu-git's pc-1.0". This is horrible, but it's not
> the fault of this patch, is simply current reality. Right
> now, you can't tell what pc-1.0 does anyway, as it does a
> different thing on Ubuntu Precise to Ubuntu Trusty (for instance).
> 
> If we're going to provide compatibility to the machine type
> name level (which is the purpose here) then sadly we need
> something like this.

Ack for "something" but not like this, I suspect.

> As for 'not being able to predict', actually post this patch
> the situation is far clearer, as '-M ?' tells you exactly
> what pc-1.0 will do, by showing what it's an alias for.
> 
> > Users also don't get qemu from git so I don't see
> > why does git make sense in the name?
> 
> Agree with that, but I couldn't think of anything better.
> 'qemu-upstream'? 'qemu'?
> 
> > Legacy management applications invoked qemu as qemu-kvm -
> > how about detecting that name and switching
> > the machine types?
> > It might make sense to also set -enable-kvm a
> 
> Sadly that is not true. For instance on Ubuntu Precise
> it's invoked as qemu-system-x86_64 by at least one
> management application known to me.

Well change it to call qemu-kvm then :)
Also what happens if you install qemu as well?
Does it conflict?

> I'm not quite sure why you say "legacy management
> applications".

Because any non legacy one can be patched.

> This applies to /any/ management application.
> Unless we're going to burden every management application
> with this problem, we need to fix it.
>
> Just as a reminder, the ./configure value defaults to
> off, which means there is no change in current behaviour.

Yes but this still perpetuates the mess.

If you prefer using -M pc-1.0, add a new property
and teach management to set it.

But no silent compile-time behind the scenes changes please.

> Only if the ./configure value is changed does it result
> in the behaviour of pc-1.0 changing, and that is evident
> from the help line. And it's possible to specify a specific
> variant of 1.0 (irrespective of the default) from the
> command line.
> 
> -- 
> Alex Bligh
> 
> 
>
Alex Bligh Sept. 22, 2014, 1:05 p.m. UTC | #6
>> Sadly that is not true. For instance on Ubuntu Precise
>> it's invoked as qemu-system-x86_64 by at least one
>> management application known to me.
> 
> Well change it to call qemu-kvm then :)
> Also what happens if you install qemu as well?
> Does it conflict?

I'm not an Ubuntu maintainer but AFAIK qemu-kvm is deprecated.
It may only be there to support upgrades.

We still need to support migration from qemu running on Precise
to qemu running on Trusty. The trusty instance may not have
qemu-kvm installed. If we were looking at argv[0], we'd
really want to look at it on the /sending/ machine.

Forcing qemu to be involved as qemu-kvm solely on the basis
some users might want to migrate a VM in from a previous
version does not sound practical.

>> I'm not quite sure why you say "legacy management
>> applications".
> 
> Because any non legacy one can be patched.

Well this is where we diverge. I think the distro needs
a way to change the default behaviour, i.e. so the existing
command line will do something different.

>> This applies to /any/ management application.
>> Unless we're going to burden every management application
>> with this problem, we need to fix it.
>> 
>> Just as a reminder, the ./configure value defaults to
>> off, which means there is no change in current behaviour.
> 
> Yes but this still perpetuates the mess.
> 
> If you prefer using -M pc-1.0, add a new property
> and teach management to set it.
> 
> But no silent compile-time behind the scenes changes please.

OK, how about we keep the aliases, and make pc-1.0
default to the pc-1.0-qemu-git. We then add a command
line option to make pc-1.0 mean pc-1.0-qemu-kvm, with
that obviously defaulting to off.

Then distros can then put the option in
/etc/qemu/target-x86_64.conf or whatever.
Markus Armbruster Sept. 22, 2014, 3:24 p.m. UTC | #7
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Mon, Sep 22, 2014 at 02:36:55PM +0300, Michael S. Tsirkin wrote:
>> On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
>> > Add a configure option --enable-pc-1-0-qemu-kvm and the
>> > corresponding --disable-pc-1-0-qemu-kvm, defaulting
>> > to disabled.
>> > 
>> > Rename machine type pc-1.0 to pc-1.0-qemu-git.
>> > 
>> > Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
>> > or pc-1.0-qemu-git depending on the value of the config
>> > option.
>> > 
>> > Signed-off-by: Alex Bligh <alex@alex.org.uk>
>> 
>> I have to say, this one bothers me.
>> We end up not being able to predict what does pc-1.0
>> reference.
>
> Yeah, this is not good. Any single machine type name should have
> fixed semantics - having two different semantics depending on build
> options means mgmt apps can no longer simply compare the machine
> type name to determine if it is a match with the same name on a
> different host.

You're right.  However, this particular horse left the barn a long time
ago: the pc-* machine types differ in qemu-kvm and upstream QEMU.

Sure, when qemu-kvm was merged back into QEMU, its machine type variants
were dropped.  But they live on in various downstreams that just like
QEMU had to pick between compatibility with upstream QEMU and qemu-kvm,
but unlike QEMU picked compatibility with qemu-kvm.

So this patch does *not* break any management apps by letting them "no
longer simply compare the machine type name to determine if it is a
match with the same name on a different host".  They never could for
these messed up machine types, at least not without knowing exactly what
kind of QEMU runs on the hosts in question.

All this patch does is adding another facet to "exactly what kind of
QEMU".
Markus Armbruster Sept. 22, 2014, 3:32 p.m. UTC | #8
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
>> Add a configure option --enable-pc-1-0-qemu-kvm and the
>> corresponding --disable-pc-1-0-qemu-kvm, defaulting
>> to disabled.
>> 
>> Rename machine type pc-1.0 to pc-1.0-qemu-git.
>> 
>> Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
>> or pc-1.0-qemu-git depending on the value of the config
>> option.
>> 
>> Signed-off-by: Alex Bligh <alex@alex.org.uk>
>
> I have to say, this one bothers me.
> We end up not being able to predict what does pc-1.0
> reference.
>
> Users also don't get qemu from git so I don't see
> why does git make sense in the name?
>
> Legacy management applications invoked qemu as qemu-kvm -
> how about detecting that name and switching
> the machine types?

Ugh!  I like that even less than a configure option.

> It might make sense to also set -enable-kvm and
> change default CPU to kvm64 in this case.

Yup.
Michael S. Tsirkin Sept. 22, 2014, 3:36 p.m. UTC | #9
On Mon, Sep 22, 2014 at 05:24:55PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Mon, Sep 22, 2014 at 02:36:55PM +0300, Michael S. Tsirkin wrote:
> >> On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> >> > Add a configure option --enable-pc-1-0-qemu-kvm and the
> >> > corresponding --disable-pc-1-0-qemu-kvm, defaulting
> >> > to disabled.
> >> > 
> >> > Rename machine type pc-1.0 to pc-1.0-qemu-git.
> >> > 
> >> > Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> >> > or pc-1.0-qemu-git depending on the value of the config
> >> > option.
> >> > 
> >> > Signed-off-by: Alex Bligh <alex@alex.org.uk>
> >> 
> >> I have to say, this one bothers me.
> >> We end up not being able to predict what does pc-1.0
> >> reference.
> >
> > Yeah, this is not good. Any single machine type name should have
> > fixed semantics - having two different semantics depending on build
> > options means mgmt apps can no longer simply compare the machine
> > type name to determine if it is a match with the same name on a
> > different host.
> 
> You're right.  However, this particular horse left the barn a long time
> ago: the pc-* machine types differ in qemu-kvm and upstream QEMU.
> 
> Sure, when qemu-kvm was merged back into QEMU, its machine type variants
> were dropped.  But they live on in various downstreams that just like
> QEMU had to pick between compatibility with upstream QEMU and qemu-kvm,
> but unlike QEMU picked compatibility with qemu-kvm.
> 
> So this patch does *not* break any management apps by letting them "no
> longer simply compare the machine type name to determine if it is a
> match with the same name on a different host".  They never could for
> these messed up machine types, at least not without knowing exactly what
> kind of QEMU runs on the hosts in question.
> 
> All this patch does is adding another facet to "exactly what kind of
> QEMU".

Right, but IMHO doing it at compile-time is wrong.
If distros want compatiblity with both sometimes, what then?
Build two binaries with different flags?
Should be a runtime option that management sets after (somehow?)
figuring out what's going on, on source.

How does it do that? Not sure - but I'm sure destination distro has no
way to figure it out.
Paolo Bonzini Sept. 22, 2014, 3:44 p.m. UTC | #10
Il 22/09/2014 17:24, Markus Armbruster ha scritto:
> You're right.  However, this particular horse left the barn a long time
> ago: the pc-* machine types differ in qemu-kvm and upstream QEMU.
> 
> Sure, when qemu-kvm was merged back into QEMU, its machine type variants
> were dropped.  But they live on in various downstreams that just like
> QEMU had to pick between compatibility with upstream QEMU and qemu-kvm,
> but unlike QEMU picked compatibility with qemu-kvm.

And fixing this should have been downstream's job.  Fedora did it at the
time, RHEL7 did it.  Ubuntu didn't, and probably neither did Debian.

This patch singles out pc-1.0 just because it used to be the default in
Ubuntu 12.04.  So basically it's making upstream carry the burden of a
decision of the Ubuntu folks.  It's understandable that Alex disagrees
with the decision, but nevertheless it's not something that upstream
should agree with.

Also, another horse that has left the barn: it's already too late to
apply this patch to upstream Ubuntu.  If you do that, any machine
created with 12.04 and reused with 14.04 will fail to migrate to another
14.04 machine that includes this patch, as I understand it.

So as things stand, I don't see a reason to apply this patch upstream.

Paolo
Andreas Färber Sept. 22, 2014, 3:45 p.m. UTC | #11
Am 22.09.2014 um 15:05 schrieb Alex Bligh:
>>> Sadly that is not true. For instance on Ubuntu Precise
>>> it's invoked as qemu-system-x86_64 by at least one
>>> management application known to me.
>>
>> Well change it to call qemu-kvm then :)
>> Also what happens if you install qemu as well?
>> Does it conflict?
> 
> I'm not an Ubuntu maintainer but AFAIK qemu-kvm is deprecated.
> It may only be there to support upgrades.
> 
> We still need to support migration from qemu running on Precise
> to qemu running on Trusty. The trusty instance may not have
> qemu-kvm installed. If we were looking at argv[0], we'd
> really want to look at it on the /sending/ machine.
> 
> Forcing qemu to be involved as qemu-kvm solely on the basis
> some users might want to migrate a VM in from a previous
> version does not sound practical.
> 
>>> I'm not quite sure why you say "legacy management
>>> applications".
>>
>> Because any non legacy one can be patched.
> 
> Well this is where we diverge. I think the distro needs
> a way to change the default behaviour, i.e. so the existing
> command line will do something different.
> 
>>> This applies to /any/ management application.
>>> Unless we're going to burden every management application
>>> with this problem, we need to fix it.
>>>
>>> Just as a reminder, the ./configure value defaults to
>>> off, which means there is no change in current behaviour.
>>
>> Yes but this still perpetuates the mess.
>>
>> If you prefer using -M pc-1.0, add a new property
>> and teach management to set it.
>>
>> But no silent compile-time behind the scenes changes please.
> 
> OK, how about we keep the aliases, and make pc-1.0
> default to the pc-1.0-qemu-git. We then add a command
> line option to make pc-1.0 mean pc-1.0-qemu-kvm, with
> that obviously defaulting to off.
> 
> Then distros can then put the option in
> /etc/qemu/target-x86_64.conf or whatever.

What about adding a bool property "qemu-kvm-compat" to the MachineClass?
Then a qemu-kvm shell script (like SUSE uses) can pass -global
machine.qemu-kvm-compat=on whereas qemu-system-x86_64 would run in the
default non-qemu-kvm mode (config on disk would affect both). It would
also allow running -machine pc-0.15,qemu-kvm-compat=on, ditching lots of
new machine names and avoiding the name bikeshedding.

Regards,
Andreas
Serge E. Hallyn Sept. 22, 2014, 3:47 p.m. UTC | #12
Quoting Michael S. Tsirkin (mst@redhat.com):
> On Mon, Sep 22, 2014 at 05:24:55PM +0200, Markus Armbruster wrote:
> > "Daniel P. Berrange" <berrange@redhat.com> writes:
> > 
> > > On Mon, Sep 22, 2014 at 02:36:55PM +0300, Michael S. Tsirkin wrote:
> > >> On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> > >> > Add a configure option --enable-pc-1-0-qemu-kvm and the
> > >> > corresponding --disable-pc-1-0-qemu-kvm, defaulting
> > >> > to disabled.
> > >> > 
> > >> > Rename machine type pc-1.0 to pc-1.0-qemu-git.
> > >> > 
> > >> > Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> > >> > or pc-1.0-qemu-git depending on the value of the config
> > >> > option.
> > >> > 
> > >> > Signed-off-by: Alex Bligh <alex@alex.org.uk>
> > >> 
> > >> I have to say, this one bothers me.
> > >> We end up not being able to predict what does pc-1.0
> > >> reference.
> > >
> > > Yeah, this is not good. Any single machine type name should have
> > > fixed semantics - having two different semantics depending on build
> > > options means mgmt apps can no longer simply compare the machine
> > > type name to determine if it is a match with the same name on a
> > > different host.
> > 
> > You're right.  However, this particular horse left the barn a long time
> > ago: the pc-* machine types differ in qemu-kvm and upstream QEMU.
> > 
> > Sure, when qemu-kvm was merged back into QEMU, its machine type variants
> > were dropped.  But they live on in various downstreams that just like
> > QEMU had to pick between compatibility with upstream QEMU and qemu-kvm,
> > but unlike QEMU picked compatibility with qemu-kvm.
> > 
> > So this patch does *not* break any management apps by letting them "no
> > longer simply compare the machine type name to determine if it is a
> > match with the same name on a different host".  They never could for
> > these messed up machine types, at least not without knowing exactly what
> > kind of QEMU runs on the hosts in question.
> > 
> > All this patch does is adding another facet to "exactly what kind of
> > QEMU".
> 
> Right, but IMHO doing it at compile-time is wrong.
> If distros want compatiblity with both sometimes, what then?
> Build two binaries with different flags?
> Should be a runtime option that management sets after (somehow?)
> figuring out what's going on, on source.
> 
> How does it do that? Not sure - but I'm sure destination distro has no
> way to figure it out.

It's not even just between distributions.  Anyone who used qemu-2.0 to
start a pc-1.0 machine type will have a different mt than someone who
starts a vm under Ubuntu's (qemu-)kvm 1.0.  Sadly.

So in the packages at https://launchpad.net/~serge-hallyn/+archive/ubuntu/qemu-p-migration
the default can be configured at build-time, but it can be specified on
the command-line (which is then controlled by a new libvirt flag).

-serge
Alex Bligh Sept. 22, 2014, 4:54 p.m. UTC | #13
On 22 Sep 2014, at 16:45, Andreas Färber <afaerber@suse.de> wrote:

> What about adding a bool property "qemu-kvm-compat" to the MachineClass?
> Then a qemu-kvm shell script (like SUSE uses) can pass -global
> machine.qemu-kvm-compat=on whereas qemu-system-x86_64 would run in the
> default non-qemu-kvm mode (config on disk would affect both). It would
> also allow running -machine pc-0.15,qemu-kvm-compat=on, ditching lots of
> new machine names and avoiding the name bikeshedding.

I'd be happy with that. Presumably downstream can then patch
things so qemu-kvm-compat defaults in the way they want (if
we don't like the configure option).

However, that's not compatible with using PC_COMPAT as far as I
know (unless there is some cunning way you can make a machine
parameter change compat_props things).
Michael S. Tsirkin Sept. 22, 2014, 5:21 p.m. UTC | #14
On Mon, Sep 22, 2014 at 05:32:16PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> >> Add a configure option --enable-pc-1-0-qemu-kvm and the
> >> corresponding --disable-pc-1-0-qemu-kvm, defaulting
> >> to disabled.
> >> 
> >> Rename machine type pc-1.0 to pc-1.0-qemu-git.
> >> 
> >> Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> >> or pc-1.0-qemu-git depending on the value of the config
> >> option.
> >> 
> >> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> >
> > I have to say, this one bothers me.
> > We end up not being able to predict what does pc-1.0
> > reference.
> >
> > Users also don't get qemu from git so I don't see
> > why does git make sense in the name?
> >
> > Legacy management applications invoked qemu as qemu-kvm -
> > how about detecting that name and switching
> > the machine types?
> 
> Ugh!  I like that even less than a configure option.

configure options are tested even less than runtime ones:
each distro has to pick up a set of options and all
users are stuck with it.
So let's assume Ubuntu sets this and something is broken.  How is a
Fedora user to test this? Find out configure flags that Ubuntu uses
and rebuild from source? It's hard to get people to triage bugs as it
is.
That's why changing behaviour in subtle ways depending on
configure options is a very bad idea.

So a flag might be better, but if we want to be compatible
with qemu-kvm, poking at argv[0] still better than configure.


> > It might make sense to also set -enable-kvm and
> > change default CPU to kvm64 in this case.
> 
> Yup.
Markus Armbruster Sept. 22, 2014, 5:26 p.m. UTC | #15
Alex Bligh <alex@alex.org.uk> writes:

> On 22 Sep 2014, at 16:45, Andreas Färber <afaerber@suse.de> wrote:
>
>> What about adding a bool property "qemu-kvm-compat" to the MachineClass?
>> Then a qemu-kvm shell script (like SUSE uses) can pass -global
>> machine.qemu-kvm-compat=on whereas qemu-system-x86_64 would run in the

No need to mess with -global, just use -machine qemu-kvm-compat=off.
You can have multiple -machine, and the last key=value wins.

>> default non-qemu-kvm mode (config on disk would affect both). It would
>> also allow running -machine pc-0.15,qemu-kvm-compat=on, ditching lots of
>> new machine names and avoiding the name bikeshedding.
>
> I'd be happy with that. Presumably downstream can then patch
> things so qemu-kvm-compat defaults in the way they want (if
> we don't like the configure option).
>
> However, that's not compatible with using PC_COMPAT as far as I
> know (unless there is some cunning way you can make a machine
> parameter change compat_props things).

Monkey-patching MACHINE_GET_CLASS(machine)->compat_props?  Gross, but I
don't have better ideas to offer.
Alex Bligh Sept. 22, 2014, 5:30 p.m. UTC | #16
On 22 Sep 2014, at 16:44, Paolo Bonzini <pbonzini@redhat.com> wrote:

> time, RHEL7 did it.  Ubuntu didn't, and probably neither did Debian.
> 
> This patch singles out pc-1.0 just because it used to be the default in
> Ubuntu 12.04.  So basically it's making upstream carry the burden of a
> decision of the Ubuntu folks.  It's understandable that Alex disagrees
> with the decision, but nevertheless it's not something that upstream
> should agree with.
> 
> Also, another horse that has left the barn: it's already too late to
> apply this patch to upstream Ubuntu.  If you do that, any machine
> created with 12.04 and reused with 14.04 will fail to migrate to another
> 14.04 machine that includes this patch, as I understand it.
> 
> So as things stand, I don't see a reason to apply this patch upstream.

Well, Ubuntu (Serge I think) said in the Ubuntu bug report he'd
be quite willing to break migration of pc-1.0 machine types
from 14.04 to 14.04 because that machine type isn't the default
anyway on 14.04.

But that isn't the point, as the patch (as a whole) doesn't
break anything - it merely gives the possibility to import
pc-1.0 machines from qemu-kvm. That's useful even if you
build qemu from source every time.

Or were you just arguing against the ./configure option?
Paolo Bonzini Sept. 22, 2014, 7:10 p.m. UTC | #17
Il 22/09/2014 19:30, Alex Bligh ha scritto:
> Well, Ubuntu (Serge I think) said in the Ubuntu bug report he'd
> be quite willing to break migration of pc-1.0 machine types
> from 14.04 to 14.04 because that machine type isn't the default
> anyway on 14.04.
> 
> But that isn't the point, as the patch (as a whole) doesn't
> break anything - it merely gives the possibility to import
> pc-1.0 machines from qemu-kvm. That's useful even if you
> build qemu from source every time.
> 
> Or were you just arguing against the ./configure option?

I'm arguing against special-casing pc-1.0.  Just apply the patch to
Ubuntu downstream and call it a day.

It's perfectly normal for machine types to be part of the downstream
(not so secret) sauce.

Paolo
Alex Bligh Sept. 22, 2014, 7:36 p.m. UTC | #18
On 22 Sep 2014, at 20:10, Paolo Bonzini <pbonzini@redhat.com> wrote:

> I'm arguing against special-casing pc-1.0.  Just apply the patch to
> Ubuntu downstream and call it a day.
> 
> It's perfectly normal for machine types to be part of the downstream
> (not so secret) sauce.

Well, I've just sent through a version that works as a machine
parameter instead. If upstream doesn't like this, I'd prefer
downstream took v2 of the patch (which makes -M pc-1.0 work)
instead. That's also what Serge tested.
Serge E. Hallyn Sept. 23, 2014, 12:12 a.m. UTC | #19
Quoting Alex Bligh (alex@alex.org.uk):
> 
> On 22 Sep 2014, at 20:10, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > I'm arguing against special-casing pc-1.0.  Just apply the patch to
> > Ubuntu downstream and call it a day.
> > 
> > It's perfectly normal for machine types to be part of the downstream
> > (not so secret) sauce.
> 
> Well, I've just sent through a version that works as a machine
> parameter instead. If upstream doesn't like this, I'd prefer
> downstream took v2 of the patch (which makes -M pc-1.0 work)
> instead. That's also what Serge tested.

Yeah, prefer v2 as well.  Sorry if that means wasted time on your
part.

Though I'm also thinking I prefer to have pc-1.0-precise defined in
both precise's qemu-kvm 1.0, and later qemu 2.x, and requiring some
purely-on-precise action to switch the machine type from pc-1.0 to
pc-1.0-precise before migrating to trusty/later.

But as I mentioned elsewhere there's still the concerns of (a) how
much other still (like virito-net) will still break, and (b) the
practical issues of how to ship the old roms.

-serge
Michael S. Tsirkin Sept. 23, 2014, 3:44 a.m. UTC | #20
On Mon, Sep 22, 2014 at 05:45:28PM +0200, Andreas Färber wrote:
> Am 22.09.2014 um 15:05 schrieb Alex Bligh:
> >>> Sadly that is not true. For instance on Ubuntu Precise
> >>> it's invoked as qemu-system-x86_64 by at least one
> >>> management application known to me.
> >>
> >> Well change it to call qemu-kvm then :)
> >> Also what happens if you install qemu as well?
> >> Does it conflict?
> > 
> > I'm not an Ubuntu maintainer but AFAIK qemu-kvm is deprecated.
> > It may only be there to support upgrades.
> > 
> > We still need to support migration from qemu running on Precise
> > to qemu running on Trusty. The trusty instance may not have
> > qemu-kvm installed. If we were looking at argv[0], we'd
> > really want to look at it on the /sending/ machine.
> > 
> > Forcing qemu to be involved as qemu-kvm solely on the basis
> > some users might want to migrate a VM in from a previous
> > version does not sound practical.
> > 
> >>> I'm not quite sure why you say "legacy management
> >>> applications".
> >>
> >> Because any non legacy one can be patched.
> > 
> > Well this is where we diverge. I think the distro needs
> > a way to change the default behaviour, i.e. so the existing
> > command line will do something different.
> > 
> >>> This applies to /any/ management application.
> >>> Unless we're going to burden every management application
> >>> with this problem, we need to fix it.
> >>>
> >>> Just as a reminder, the ./configure value defaults to
> >>> off, which means there is no change in current behaviour.
> >>
> >> Yes but this still perpetuates the mess.
> >>
> >> If you prefer using -M pc-1.0, add a new property
> >> and teach management to set it.
> >>
> >> But no silent compile-time behind the scenes changes please.
> > 
> > OK, how about we keep the aliases, and make pc-1.0
> > default to the pc-1.0-qemu-git. We then add a command
> > line option to make pc-1.0 mean pc-1.0-qemu-kvm, with
> > that obviously defaulting to off.
> > 
> > Then distros can then put the option in
> > /etc/qemu/target-x86_64.conf or whatever.
> 
> What about adding a bool property "qemu-kvm-compat" to the MachineClass?
> Then a qemu-kvm shell script (like SUSE uses) can pass -global
> machine.qemu-kvm-compat=on whereas qemu-system-x86_64 would run in the
> default non-qemu-kvm mode (config on disk would affect both). It would
> also allow running -machine pc-0.15,qemu-kvm-compat=on, ditching lots of
> new machine names and avoiding the name bikeshedding.
> 
> Regards,
> Andreas

Ack. Exactly.

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Michael S. Tsirkin Sept. 23, 2014, 3:46 a.m. UTC | #21
On Mon, Sep 22, 2014 at 05:54:02PM +0100, Alex Bligh wrote:
> However, that's not compatible with using PC_COMPAT as far as I
> know (unless there is some cunning way you can make a machine
> parameter change compat_props things).

Of course not, PC_COMPAT is the reverse: have machine type
influence properties.
Markus Armbruster Sept. 23, 2014, 7:59 a.m. UTC | #22
"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Sep 22, 2014 at 05:32:16PM +0200, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
>> >> Add a configure option --enable-pc-1-0-qemu-kvm and the
>> >> corresponding --disable-pc-1-0-qemu-kvm, defaulting
>> >> to disabled.
>> >> 
>> >> Rename machine type pc-1.0 to pc-1.0-qemu-git.
>> >> 
>> >> Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
>> >> or pc-1.0-qemu-git depending on the value of the config
>> >> option.
>> >> 
>> >> Signed-off-by: Alex Bligh <alex@alex.org.uk>
>> >
>> > I have to say, this one bothers me.
>> > We end up not being able to predict what does pc-1.0
>> > reference.
>> >
>> > Users also don't get qemu from git so I don't see
>> > why does git make sense in the name?
>> >
>> > Legacy management applications invoked qemu as qemu-kvm -
>> > how about detecting that name and switching
>> > the machine types?
>> 
>> Ugh!  I like that even less than a configure option.
>
> configure options are tested even less than runtime ones:
> each distro has to pick up a set of options and all
> users are stuck with it.
> So let's assume Ubuntu sets this and something is broken.  How is a
> Fedora user to test this? Find out configure flags that Ubuntu uses
> and rebuild from source? It's hard to get people to triage bugs as it
> is.
> That's why changing behaviour in subtle ways depending on
> configure options is a very bad idea.

All it changes is a default machine type.  I wouldn't classify that as
"very bad".  Fortunately, our difference of opinion doesn't matter,
because the conclusion of the thread is "leave it to the distros".

> So a flag might be better, but if we want to be compatible
> with qemu-kvm, poking at argv[0] still better than configure.

That I do consider a genuinely bad idea.

Example of usage messed up by it: I symlink from my ~/bin to executables
in various build trees.  If one of these programs changed behavior
depending on what it finds in argv[0], I'd have to know *exactly* how it
matches argv[0] to choose suitable names for my symlinks.

The name "qemu-kvm" is not a useful indicator of compatibility with the
qemu-kvm fork of QEMU anyway.  There are programs out there calling
themselves qemu-kvm that aren't compatible with the qemu-kvm fork.  And
there are programs out there that are compatible, but call themselves
something else.
Michael S. Tsirkin Sept. 23, 2014, 8:24 a.m. UTC | #23
On Tue, Sep 23, 2014 at 09:59:17AM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Sep 22, 2014 at 05:32:16PM +0200, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Sun, Sep 21, 2014 at 03:38:59PM +0100, Alex Bligh wrote:
> >> >> Add a configure option --enable-pc-1-0-qemu-kvm and the
> >> >> corresponding --disable-pc-1-0-qemu-kvm, defaulting
> >> >> to disabled.
> >> >> 
> >> >> Rename machine type pc-1.0 to pc-1.0-qemu-git.
> >> >> 
> >> >> Make pc-1.0 machine type an alias of either pc-1.0-qemu-kvm
> >> >> or pc-1.0-qemu-git depending on the value of the config
> >> >> option.
> >> >> 
> >> >> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> >> >
> >> > I have to say, this one bothers me.
> >> > We end up not being able to predict what does pc-1.0
> >> > reference.
> >> >
> >> > Users also don't get qemu from git so I don't see
> >> > why does git make sense in the name?
> >> >
> >> > Legacy management applications invoked qemu as qemu-kvm -
> >> > how about detecting that name and switching
> >> > the machine types?
> >> 
> >> Ugh!  I like that even less than a configure option.
> >
> > configure options are tested even less than runtime ones:
> > each distro has to pick up a set of options and all
> > users are stuck with it.
> > So let's assume Ubuntu sets this and something is broken.  How is a
> > Fedora user to test this? Find out configure flags that Ubuntu uses
> > and rebuild from source? It's hard to get people to triage bugs as it
> > is.
> > That's why changing behaviour in subtle ways depending on
> > configure options is a very bad idea.
> 
> All it changes is a default machine type.  I wouldn't classify that as
> "very bad".  Fortunately, our difference of opinion doesn't matter,
> because the conclusion of the thread is "leave it to the distros".
> 
> > So a flag might be better, but if we want to be compatible
> > with qemu-kvm, poking at argv[0] still better than configure.
> 
> That I do consider a genuinely bad idea.
> 
> Example of usage messed up by it: I symlink from my ~/bin to executables
> in various build trees.  If one of these programs changed behavior
> depending on what it finds in argv[0], I'd have to know *exactly* how it
> matches argv[0] to choose suitable names for my symlinks.
> 
> The name "qemu-kvm" is not a useful indicator of compatibility with the
> qemu-kvm fork of QEMU anyway.  There are programs out there calling
> themselves qemu-kvm that aren't compatible with the qemu-kvm fork.  And
> there are programs out there that are compatible, but call themselves
> something else.

I think the approach v4 takes is reasonable, though.
I didn't look at the implementation yet.
diff mbox

Patch

diff --git a/configure b/configure
index f7685b5..b143302 100755
--- a/configure
+++ b/configure
@@ -335,6 +335,7 @@  libssh2=""
 vhdx=""
 quorum=""
 numa=""
+pc_1_0_qemu_kvm="no"
 
 # parse CC options first
 for opt do
@@ -1125,6 +1126,10 @@  for opt do
   ;;
   --enable-numa) numa="yes"
   ;;
+  --disable-pc-1-0-qemu-kvm) pc_1_0_qemu_kvm="no"
+  ;;
+  --enable-pc-1-0-qemu-kvm) pc_1_0_qemu_kvm="yes"
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1394,6 +1399,8 @@  Advanced options (experts only):
   --enable-quorum          enable quorum block filter support
   --disable-numa           disable libnuma support
   --enable-numa            enable libnuma support
+  --disable-pc-1-0-qemu-kvm disable pc-1.0 machine type reflecting qemu-kvm
+  --enable-pc-1-0-qemu-kvm enable pc-1.0 machine type reflecting qemu-kvm
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -4262,6 +4269,7 @@  echo "Quorum            $quorum"
 echo "lzo support       $lzo"
 echo "snappy support    $snappy"
 echo "NUMA host support $numa"
+echo "pc-1.0 qemu-kvm   $pc_1_0_qemu_kvm"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -5241,6 +5249,10 @@  if test "$numa" = "yes"; then
   echo "CONFIG_NUMA=y" >> $config_host_mak
 fi
 
+if test "$pc_1_0_qemu_kvm" = "yes"; then
+  echo "CONFIG_PC_1_0_QEMU_KVM=y" >> $config_host_mak
+fi
+
 # build tree in object directory in case the source is not in the current directory
 DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests"
 DIRS="$DIRS fsdev"
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 48a4942..b7a4af0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -646,7 +646,10 @@  static QEMUMachine pc_machine_v1_1 = {
 
 static QEMUMachine pc_machine_v1_0 = {
     PC_I440FX_1_2_MACHINE_OPTIONS,
-    .name = "pc-1.0",
+    .name = "pc-1.0-qemu-git",
+#ifndef CONFIG_PC_1_0_QEMU_KVM
+    .alias = "pc-1.0",
+#endif
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_0,
         { /* end of list */ }
@@ -665,6 +668,9 @@  static QEMUMachine pc_machine_v1_0 = {
 static QEMUMachine pc_machine_v1_0_qemu_kvm = {
     PC_I440FX_1_2_MACHINE_OPTIONS,
     .name = "pc-1.0-qemu-kvm",
+#ifdef CONFIG_PC_1_0_QEMU_KVM
+    .alias = "pc-1.0",
+#endif
     .init = pc_init_pci_1_2_qemu_kvm,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_0_QEMU_KVM,