[libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available

Daniel P. Berrangé posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180803125947.1536-1-berrange@redhat.com
Test syntax-check passed
src/qemu/qemu_capabilities.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
[libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Daniel P. Berrangé 5 years, 7 months ago
It is increasingly likely that some distro is going to change the
default "x86" machine type in QEMU from "pc" to "q35". This will
certainly break existing applications which write their XML on the
assumption that its using a "pc" machine by default. For example they'll
lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
radically.

Libvirt promises to isolate applications from hypervisor changes that
may cause incompatibilities, so we must ensure that we always use the
"pc" machine type if it is available. Only use QEMU's own reported
default machine type if "pc" does not exist.

Note this change assumes there will always be a "pc" alias as long as a
versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
machine type but not provide the "pc" alias, it is too hard to decide
which to default so. Versioned machine types are supposed to be
considered opaque strings, so we can't apply any sensible ordering
ourselves and QEMU isn't reporting the list of machines in any sensible
ordering itself.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/qemu/qemu_capabilities.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 0fb800589a..9eb58afef3 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2242,6 +2242,17 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
     int ret = -1;
     size_t i;
     size_t defIdx = 0;
+    ssize_t preferredIdx = -1;
+    const char *preferredAlias = NULL;
+
+    /* Historically QEMU defaulted to 'pc' machine type but in
+     * future might switch 'q35'. Such a change is considered
+     * an ABI break from lbivirt's POV, so we must be sure to
+     * keep 'pc' as default machine no matter what QEMU says.
+     */
+    if (qemuCaps->arch == VIR_ARCH_X86_64 ||
+        qemuCaps->arch == VIR_ARCH_I686)
+        preferredAlias = "pc";
 
     if ((nmachines = qemuMonitorGetMachines(mon, &machines)) < 0)
         return -1;
@@ -2263,12 +2274,16 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
         mach->maxCpus = machines[i]->maxCpus;
         mach->hotplugCpus = machines[i]->hotplugCpus;
 
+        if (STREQ_NULLABLE(mach->alias, preferredAlias))
+            preferredIdx = qemuCaps->nmachineTypes - 1;
+
         if (machines[i]->isDefault)
             defIdx = qemuCaps->nmachineTypes - 1;
     }
 
-    if (defIdx)
-        virQEMUCapsSetDefaultMachine(qemuCaps, defIdx);
+    if (preferredIdx == -1)
+        preferredIdx = defIdx;
+    virQEMUCapsSetDefaultMachine(qemuCaps, preferredIdx);
 
     ret = 0;
 
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Christian Ehrhardt 5 years, 7 months ago
On Sun, Aug 5, 2018 at 10:53 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> It is increasingly likely that some distro is going to change the
> default "x86" machine type in QEMU from "pc" to "q35". This will
> certainly break existing applications which write their XML on the
> assumption that its using a "pc" machine by default. For example they'll
> lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> radically.
>

Hi,
Distributions carry custom manchine types for quite a while now to
encapsulate differences of backports and similar [1][2].
That said, in all those "pc" isn't the default for a long time and it was
actually quite comfortable to be able to switch the default from qemu where
changes take place and not having to touch libvirt in that regard.

I agree that pc->q35 is a "bigger" switch in terms of default behavior than
default enabling a single new feature out of the i440fx scope, therefore I
understand the preference of libvirt to preserve the old default.
But with the change proposed here the "default" machine type of qemu looses
a lot of its (benficial) implications that that so far.

Ideally we'd not switch just back to "pc" here, but to something qemu can
mark like a certain i400fx-abcd type.
I know qemu can only have one default, and it is about to change - which
from a pure qemu's POV makes sense.
We always carried an alias "ubuntu" that changed with each release and
pointed to the preferred default type, just like "pc" pointed to the most
recent pc-i440 type.
So maybe I just modify the patch proposed here for Ubuntu to pick "ubuntu"
instead of "pc" - no sure yet?

Or we can carry a revert of the patch discussed here (but then would get
all the pain of old working XML tools failing, doesn't sound like a good
option),
but I wanted to know if there were some more complex considerations have
been done already how those cases are supposed to be handled?

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=895959
[2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1294823



> Libvirt promises to isolate applications from hypervisor changes that
> may cause incompatibilities, so we must ensure that we always use the
> "pc" machine type if it is available. Only use QEMU's own reported
> default machine type if "pc" does not exist.
>
> Note this change assumes there will always be a "pc" alias as long as a
> versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
> machine type but not provide the "pc" alias, it is too hard to decide
> which to default so. Versioned machine types are supposed to be
> considered opaque strings, so we can't apply any sensible ordering
> ourselves and QEMU isn't reporting the list of machines in any sensible
> ordering itself.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 0fb800589a..9eb58afef3 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2242,6 +2242,17 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr
> qemuCaps,
>      int ret = -1;
>      size_t i;
>      size_t defIdx = 0;
> +    ssize_t preferredIdx = -1;
> +    const char *preferredAlias = NULL;
> +
> +    /* Historically QEMU defaulted to 'pc' machine type but in
> +     * future might switch 'q35'. Such a change is considered
> +     * an ABI break from lbivirt's POV, so we must be sure to
> +     * keep 'pc' as default machine no matter what QEMU says.
> +     */
> +    if (qemuCaps->arch == VIR_ARCH_X86_64 ||
> +        qemuCaps->arch == VIR_ARCH_I686)
> +        preferredAlias = "pc";
>
>      if ((nmachines = qemuMonitorGetMachines(mon, &machines)) < 0)
>          return -1;
> @@ -2263,12 +2274,16 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr
> qemuCaps,
>          mach->maxCpus = machines[i]->maxCpus;
>          mach->hotplugCpus = machines[i]->hotplugCpus;
>
> +        if (STREQ_NULLABLE(mach->alias, preferredAlias))
> +            preferredIdx = qemuCaps->nmachineTypes - 1;
> +
>          if (machines[i]->isDefault)
>              defIdx = qemuCaps->nmachineTypes - 1;
>      }
>
> -    if (defIdx)
> -        virQEMUCapsSetDefaultMachine(qemuCaps, defIdx);
> +    if (preferredIdx == -1)
> +        preferredIdx = defIdx;
> +    virQEMUCapsSetDefaultMachine(qemuCaps, preferredIdx);
>
>      ret = 0;
>
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Mon, Aug 06, 2018 at 07:04:32AM +0200, Christian Ehrhardt wrote:
> On Sun, Aug 5, 2018 at 10:53 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > It is increasingly likely that some distro is going to change the
> > default "x86" machine type in QEMU from "pc" to "q35". This will
> > certainly break existing applications which write their XML on the
> > assumption that its using a "pc" machine by default. For example they'll
> > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> > radically.
> >
> 
> Hi,
> Distributions carry custom manchine types for quite a while now to
> encapsulate differences of backports and similar [1][2].
> That said, in all those "pc" isn't the default for a long time and it was
> actually quite comfortable to be able to switch the default from qemu where
> changes take place and not having to touch libvirt in that regard.
> 
> I agree that pc->q35 is a "bigger" switch in terms of default behavior than
> default enabling a single new feature out of the i440fx scope, therefore I
> understand the preference of libvirt to preserve the old default.
> But with the change proposed here the "default" machine type of qemu looses
> a lot of its (benficial) implications that that so far.
> 
> Ideally we'd not switch just back to "pc" here, but to something qemu can
> mark like a certain i400fx-abcd type.
> I know qemu can only have one default, and it is about to change - which
> from a pure qemu's POV makes sense.
> We always carried an alias "ubuntu" that changed with each release and
> pointed to the preferred default type, just like "pc" pointed to the most
> recent pc-i440 type.
> So maybe I just modify the patch proposed here for Ubuntu to pick "ubuntu"
> instead of "pc" - no sure yet?
> 
> Or we can carry a revert of the patch discussed here (but then would get
> all the pain of old working XML tools failing, doesn't sound like a good
> option),
> but I wanted to know if there were some more complex considerations have
> been done already how those cases are supposed to be handled?
> 
> [1]: https://bugzilla.redhat.com/show_bug.cgi?id=895959
> [2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1294823

There's multiple things at play. First all the canonical machine type
names which are versioned

  pc-i440fx-2.5.0
  pc-i440fx-2.6.0
  pc-i440fx-2.7.0
  ...
  pc-i440fx-2.10.0
  pc-q35-2.5.0
  pc-q35-2.6.0
  pc-q35-2.7.0
  ...
  pc-q35-2.10.0

Then there are the convenient aliases mapping to the most recent versioned
machine type

  pc  -> pc-i440fx-2.10.0
  q35 -> pc-q35-2.10.0

Finally, the versioned machine type that is resolved by the "pc" alias
is listed as the default.

Even when distros ship custom machine types this has little to no impact
on applications. It generally just means the version number part of the
machine type changes. 'pc' still resolves to the most recent versioned
machine type, and is listed as the default.

The key thing is that applicatons (virt-install, virt-manager, oVirt,
OpenStack, etc) need to know whether they're using "pc" or "q35" when
building guest XML, as that difference is used to trigger different
code paths.

In looking at the code for various mgmt apps we see alot of patterns
like

   if machine != NULL & strstr(machine, "q35")
      ...write XML suitable for q35...
   else
      ...write XML suitable for pc...

IOW, the application is assuming that if the user hasn't requested an
explicit type, they'll get "i440fx" based "pc".

If QEMU changes its default so that the "q35" alias is marked as the
default, then this will break every single mgmt application that we
have looked at. This is exactly the kind of thing that libvirt promises
will not happen to mgmt apps, so we must guarantee that if no machine
type is listed in the XML, then app will always get the i440fx based
"pc" machine, and not "q35".

WRT your point about the "ubuntu" machine type. I think using such a
machine type name is not a desirable thing todo. No application that
I've looked at, nor libvirt itself, has logic to know whether "ubuntu"
machine type is based on i440fx or q35 chipsets. I expect that, by
luck, they'll mostly end up treating it as i440fx based, since most
apps do an explicit check for "q35" in the name and assume everything
else is i440fx.

So the general guidance we give is it that distros should honour the
QEMU machine type name prefixes "pc-q35-" and "pc-i440fx-", and only
ever change the version suffix if they want to add custom distro
specific variants. ie don't invent new prefixes, nor new aliases,
as no application will know what todo with those.

On that basis I'd ecourage you to look at whether you can phase out
the "ubuntu" machine type alias.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Mon, Aug 06, 2018 at 10:03:52AM +0100, Daniel P. Berrangé wrote:
> There's multiple things at play. First all the canonical machine type
> names which are versioned
> 
>   pc-i440fx-2.5.0
>   pc-i440fx-2.6.0
>   pc-i440fx-2.7.0
>   ...
>   pc-i440fx-2.10.0
>   pc-q35-2.5.0
>   pc-q35-2.6.0
>   pc-q35-2.7.0
>   ...
>   pc-q35-2.10.0
> 
> Then there are the convenient aliases mapping to the most recent versioned
> machine type
> 
>   pc  -> pc-i440fx-2.10.0
>   q35 -> pc-q35-2.10.0
> 
> Finally, the versioned machine type that is resolved by the "pc" alias
> is listed as the default.
> 
> Even when distros ship custom machine types this has little to no impact
> on applications. It generally just means the version number part of the
> machine type changes. 'pc' still resolves to the most recent versioned
> machine type, and is listed as the default.
> 
> The key thing is that applicatons (virt-install, virt-manager, oVirt,
> OpenStack, etc) need to know whether they're using "pc" or "q35" when
> building guest XML, as that difference is used to trigger different
> code paths.
> 
> In looking at the code for various mgmt apps we see alot of patterns
> like
> 
>    if machine != NULL & strstr(machine, "q35")
>       ...write XML suitable for q35...
>    else
>       ...write XML suitable for pc...
> 
> IOW, the application is assuming that if the user hasn't requested an
> explicit type, they'll get "i440fx" based "pc".
> 
> If QEMU changes its default so that the "q35" alias is marked as the
> default, then this will break every single mgmt application that we
> have looked at. This is exactly the kind of thing that libvirt promises
> will not happen to mgmt apps, so we must guarantee that if no machine
> type is listed in the XML, then app will always get the i440fx based
> "pc" machine, and not "q35".
> 
> WRT your point about the "ubuntu" machine type. I think using such a
> machine type name is not a desirable thing todo. No application that
> I've looked at, nor libvirt itself, has logic to know whether "ubuntu"
> machine type is based on i440fx or q35 chipsets. I expect that, by
> luck, they'll mostly end up treating it as i440fx based, since most
> apps do an explicit check for "q35" in the name and assume everything
> else is i440fx.
> 
> So the general guidance we give is it that distros should honour the
> QEMU machine type name prefixes "pc-q35-" and "pc-i440fx-", and only
> ever change the version suffix if they want to add custom distro
> specific variants. ie don't invent new prefixes, nor new aliases,
> as no application will know what todo with those.
> 
> On that basis I'd ecourage you to look at whether you can phase out
> the "ubuntu" machine type alias.

Oh and on the bigger picture...

In discussions with QEMU maintainers the ultimate end goal is that we would
like all applications to be using the q35 based machine type on x86. Some
applications have support for q35 as an opt-in at user choice, but we would
like to get to a place where it is used without user having to ask for it.
The complication is that not all guest OS are compatible with it, since it
uses PCI-Express and SATA instead of IDE.

Our intention is thus to have libosinfo record metadata against each OS
saying which machine types it is capable of supporting. Mgmt applications
will thus be recommended to use libosinfo to see if q35 is supported by
the guest OS in question and use that if possible, only using the i440fx
machine for legacy OS. Furthermore when using q35 mgmt apps should avoid
adding PCI based devices (eg rtl8139), only use PCI-X capable devices
(eg e1000, virtio-net, etc). This avoids the need for PCIX-to-PCI on q35
so simplifies testing matrix for apps.

There's alot of mgmt apps out there so it will take a while to get them
all using libosinfo for this, but eventually we should get to a place
where q35 is widely used, without libvirt having to risk app breakage
by changint the default to q35 behind their back.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Christian Ehrhardt 5 years, 7 months ago
On Mon, Aug 6, 2018 at 11:03 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Aug 06, 2018 at 07:04:32AM +0200, Christian Ehrhardt wrote:
> > On Sun, Aug 5, 2018 at 10:53 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > It is increasingly likely that some distro is going to change the
> > > default "x86" machine type in QEMU from "pc" to "q35". This will
> > > certainly break existing applications which write their XML on the
> > > assumption that its using a "pc" machine by default. For example
> they'll
> > > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> > > radically.
> > >
> >
> > Hi,
> > Distributions carry custom manchine types for quite a while now to
> > encapsulate differences of backports and similar [1][2].
> > That said, in all those "pc" isn't the default for a long time and it was
> > actually quite comfortable to be able to switch the default from qemu
> where
> > changes take place and not having to touch libvirt in that regard.
> >
> > I agree that pc->q35 is a "bigger" switch in terms of default behavior
> than
> > default enabling a single new feature out of the i440fx scope, therefore
> I
> > understand the preference of libvirt to preserve the old default.
> > But with the change proposed here the "default" machine type of qemu
> looses
> > a lot of its (benficial) implications that that so far.
> >
> > Ideally we'd not switch just back to "pc" here, but to something qemu can
> > mark like a certain i400fx-abcd type.
> > I know qemu can only have one default, and it is about to change - which
> > from a pure qemu's POV makes sense.
> > We always carried an alias "ubuntu" that changed with each release and
> > pointed to the preferred default type, just like "pc" pointed to the most
> > recent pc-i440 type.
> > So maybe I just modify the patch proposed here for Ubuntu to pick
> "ubuntu"
> > instead of "pc" - no sure yet?
> >
> > Or we can carry a revert of the patch discussed here (but then would get
> > all the pain of old working XML tools failing, doesn't sound like a good
> > option),
> > but I wanted to know if there were some more complex considerations have
> > been done already how those cases are supposed to be handled?
> >
> > [1]: https://bugzilla.redhat.com/show_bug.cgi?id=895959
> > [2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1294823
>
> There's multiple things at play. First all the canonical machine type
> names which are versioned
>
>   pc-i440fx-2.5.0
>   pc-i440fx-2.6.0
>   pc-i440fx-2.7.0
>   ...
>   pc-i440fx-2.10.0
>   pc-q35-2.5.0
>   pc-q35-2.6.0
>   pc-q35-2.7.0
>   ...
>   pc-q35-2.10.0
>
> Then there are the convenient aliases mapping to the most recent versioned
> machine type
>
>   pc  -> pc-i440fx-2.10.0
>   q35 -> pc-q35-2.10.0
>
> Finally, the versioned machine type that is resolved by the "pc" alias
> is listed as the default.
>
> Even when distros ship custom machine types this has little to no impact
> on applications. It generally just means the version number part of the
> machine type changes. 'pc' still resolves to the most recent versioned
> machine type, and is listed as the default.
>
> The key thing is that applicatons (virt-install, virt-manager, oVirt,
> OpenStack, etc) need to know whether they're using "pc" or "q35" when
> building guest XML, as that difference is used to trigger different
> code paths.
>
> In looking at the code for various mgmt apps we see alot of patterns
> like
>
>    if machine != NULL & strstr(machine, "q35")
>       ...write XML suitable for q35...
>    else
>       ...write XML suitable for pc...
>
> IOW, the application is assuming that if the user hasn't requested an
> explicit type, they'll get "i440fx" based "pc".
>
> If QEMU changes its default so that the "q35" alias is marked as the
> default, then this will break every single mgmt application that we
> have looked at. This is exactly the kind of thing that libvirt promises
> will not happen to mgmt apps, so we must guarantee that if no machine
> type is listed in the XML, then app will always get the i440fx based
> "pc" machine, and not "q35".
>
> WRT your point about the "ubuntu" machine type. I think using such a
> machine type name is not a desirable thing todo. No application that
> I've looked at, nor libvirt itself, has logic to know whether "ubuntu"
> machine type is based on i440fx or q35 chipsets. I expect that, by
> luck, they'll mostly end up treating it as i440fx based, since most
> apps do an explicit check for "q35" in the name and assume everything
> else is i440fx.
>
> So the general guidance we give is it that distros should honour the
> QEMU machine type name prefixes "pc-q35-" and "pc-i440fx-", and only
> ever change the version suffix if they want to add custom distro
> specific variants. ie don't invent new prefixes, nor new aliases,
> as no application will know what todo with those.
>

IMHO new aliases are actually fine, as they will never stay the alias but
be resolve on first definition.
If a management App/User knows about them it is free to pass them, but then
gets mapped to the more defined names automatically.
If App/User didn't know about them, then hopefully they won't specify it
and use the full definitions.

The real Distribution machine types in the Ubuntu case already follow the
preferred prefix-known like pc-i440fx-... and pc-q35-... pattern.


> On that basis I'd ecourage you to look at whether you can phase out
> the "ubuntu" machine type alias.


The type was invented before my time, it might have been intended to NOT
imply machine types.
We kept "pc" pointing to the upstream types like pc-i440fx-2.11 and
"ubuntu" was pointing to the latest Distro type like pc-i440fx-bionic.
We could instead of the alias "ubuntu" modify the "pc" alias to alias the
particular Distro Type we want, but I always liked that we did not touch
the existing types at all :-/

Eventually we will need the same that your patch implements here, to not
break the default.
But we need that to point to the most recent Distribution type - so I have
to pick one of
 - keep ubuntu alias, use that to fall back to from libvirt
 - phase out ubuntu alias, but then "pc" would have to point to the Distro
type - I can't guarantee that back in time (new libvirt + old qemu remote)

At least it is no real type that has to be kept and "only" an alias.
I'll look at phasing out ubuntu or not - starting with an internal
discussion on the next sprint - thanks!

Thanks also for the Big picture reply - keeping the defaults sane and
establishing an per-guest opt-in via metadata sounds fine.

Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Mon, Aug 06, 2018 at 01:20:23PM +0200, Christian Ehrhardt wrote:
> On Mon, Aug 6, 2018 at 11:03 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Mon, Aug 06, 2018 at 07:04:32AM +0200, Christian Ehrhardt wrote:
> > > On Sun, Aug 5, 2018 at 10:53 AM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > It is increasingly likely that some distro is going to change the
> > > > default "x86" machine type in QEMU from "pc" to "q35". This will
> > > > certainly break existing applications which write their XML on the
> > > > assumption that its using a "pc" machine by default. For example
> > they'll
> > > > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> > > > radically.
> > > >
> > >
> > > Hi,
> > > Distributions carry custom manchine types for quite a while now to
> > > encapsulate differences of backports and similar [1][2].
> > > That said, in all those "pc" isn't the default for a long time and it was
> > > actually quite comfortable to be able to switch the default from qemu
> > where
> > > changes take place and not having to touch libvirt in that regard.
> > >
> > > I agree that pc->q35 is a "bigger" switch in terms of default behavior
> > than
> > > default enabling a single new feature out of the i440fx scope, therefore
> > I
> > > understand the preference of libvirt to preserve the old default.
> > > But with the change proposed here the "default" machine type of qemu
> > looses
> > > a lot of its (benficial) implications that that so far.
> > >
> > > Ideally we'd not switch just back to "pc" here, but to something qemu can
> > > mark like a certain i400fx-abcd type.
> > > I know qemu can only have one default, and it is about to change - which
> > > from a pure qemu's POV makes sense.
> > > We always carried an alias "ubuntu" that changed with each release and
> > > pointed to the preferred default type, just like "pc" pointed to the most
> > > recent pc-i440 type.
> > > So maybe I just modify the patch proposed here for Ubuntu to pick
> > "ubuntu"
> > > instead of "pc" - no sure yet?
> > >
> > > Or we can carry a revert of the patch discussed here (but then would get
> > > all the pain of old working XML tools failing, doesn't sound like a good
> > > option),
> > > but I wanted to know if there were some more complex considerations have
> > > been done already how those cases are supposed to be handled?
> > >
> > > [1]: https://bugzilla.redhat.com/show_bug.cgi?id=895959
> > > [2]: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1294823
> >
> > There's multiple things at play. First all the canonical machine type
> > names which are versioned
> >
> >   pc-i440fx-2.5.0
> >   pc-i440fx-2.6.0
> >   pc-i440fx-2.7.0
> >   ...
> >   pc-i440fx-2.10.0
> >   pc-q35-2.5.0
> >   pc-q35-2.6.0
> >   pc-q35-2.7.0
> >   ...
> >   pc-q35-2.10.0
> >
> > Then there are the convenient aliases mapping to the most recent versioned
> > machine type
> >
> >   pc  -> pc-i440fx-2.10.0
> >   q35 -> pc-q35-2.10.0
> >
> > Finally, the versioned machine type that is resolved by the "pc" alias
> > is listed as the default.
> >
> > Even when distros ship custom machine types this has little to no impact
> > on applications. It generally just means the version number part of the
> > machine type changes. 'pc' still resolves to the most recent versioned
> > machine type, and is listed as the default.
> >
> > The key thing is that applicatons (virt-install, virt-manager, oVirt,
> > OpenStack, etc) need to know whether they're using "pc" or "q35" when
> > building guest XML, as that difference is used to trigger different
> > code paths.
> >
> > In looking at the code for various mgmt apps we see alot of patterns
> > like
> >
> >    if machine != NULL & strstr(machine, "q35")
> >       ...write XML suitable for q35...
> >    else
> >       ...write XML suitable for pc...
> >
> > IOW, the application is assuming that if the user hasn't requested an
> > explicit type, they'll get "i440fx" based "pc".
> >
> > If QEMU changes its default so that the "q35" alias is marked as the
> > default, then this will break every single mgmt application that we
> > have looked at. This is exactly the kind of thing that libvirt promises
> > will not happen to mgmt apps, so we must guarantee that if no machine
> > type is listed in the XML, then app will always get the i440fx based
> > "pc" machine, and not "q35".
> >
> > WRT your point about the "ubuntu" machine type. I think using such a
> > machine type name is not a desirable thing todo. No application that
> > I've looked at, nor libvirt itself, has logic to know whether "ubuntu"
> > machine type is based on i440fx or q35 chipsets. I expect that, by
> > luck, they'll mostly end up treating it as i440fx based, since most
> > apps do an explicit check for "q35" in the name and assume everything
> > else is i440fx.
> >
> > So the general guidance we give is it that distros should honour the
> > QEMU machine type name prefixes "pc-q35-" and "pc-i440fx-", and only
> > ever change the version suffix if they want to add custom distro
> > specific variants. ie don't invent new prefixes, nor new aliases,
> > as no application will know what todo with those.
> >
> 
> IMHO new aliases are actually fine, as they will never stay the alias but
> be resolve on first definition.
> If a management App/User knows about them it is free to pass them, but then
> gets mapped to the more defined names automatically.
> If App/User didn't know about them, then hopefully they won't specify it
> and use the full definitions.
>
> The real Distribution machine types in the Ubuntu case already follow the
> preferred prefix-known like pc-i440fx-... and pc-q35-... pattern.

The caveat around this is that the mgmt application sees these machine
aliases at the time it is building the XML config. eg In OpenStack
/etc/nova/nova.conf might list a preferred machine type, or it can be
set in the Flavour config.  When Nova builds up the full guest XML
it is doing its "if machinetype == blah" logic, *before* the alias
has been resolved into the versioned type. 

So long term it is safe, but during initial provisioning of new guests
an unusual machine alias name still has potential to confuse apps.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Eduardo Habkost 5 years, 7 months ago
On Fri, Aug 03, 2018 at 01:59:47PM +0100, Daniel P. Berrangé wrote:
> It is increasingly likely that some distro is going to change the
> default "x86" machine type in QEMU from "pc" to "q35". This will
> certainly break existing applications which write their XML on the
> assumption that its using a "pc" machine by default. For example they'll
> lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> radically.
> 
> Libvirt promises to isolate applications from hypervisor changes that
> may cause incompatibilities, so we must ensure that we always use the
> "pc" machine type if it is available. Only use QEMU's own reported
> default machine type if "pc" does not exist.
> 
> Note this change assumes there will always be a "pc" alias as long as a
> versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
> machine type but not provide the "pc" alias, it is too hard to decide
> which to default so. Versioned machine types are supposed to be
> considered opaque strings, so we can't apply any sensible ordering
> ourselves and QEMU isn't reporting the list of machines in any sensible
> ordering itself.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Won't this break qemuParseCommandLine() if it sees a QEMU binary
running without "-machine"?  It will assume the QEMU default is
"pc" but this may be not true.


> ---
>  src/qemu/qemu_capabilities.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 0fb800589a..9eb58afef3 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2242,6 +2242,17 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
>      int ret = -1;
>      size_t i;
>      size_t defIdx = 0;
> +    ssize_t preferredIdx = -1;
> +    const char *preferredAlias = NULL;
> +
> +    /* Historically QEMU defaulted to 'pc' machine type but in
> +     * future might switch 'q35'. Such a change is considered
> +     * an ABI break from lbivirt's POV, so we must be sure to
> +     * keep 'pc' as default machine no matter what QEMU says.
> +     */
> +    if (qemuCaps->arch == VIR_ARCH_X86_64 ||
> +        qemuCaps->arch == VIR_ARCH_I686)
> +        preferredAlias = "pc";
>  
>      if ((nmachines = qemuMonitorGetMachines(mon, &machines)) < 0)
>          return -1;
> @@ -2263,12 +2274,16 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
>          mach->maxCpus = machines[i]->maxCpus;
>          mach->hotplugCpus = machines[i]->hotplugCpus;
>  
> +        if (STREQ_NULLABLE(mach->alias, preferredAlias))
> +            preferredIdx = qemuCaps->nmachineTypes - 1;
> +

Isn't STREQ_NULLABLE(NULL, NULL) true?  You don't want to set
preferredIdx here if preferredAlias==NULL.

>          if (machines[i]->isDefault)
>              defIdx = qemuCaps->nmachineTypes - 1;
>      }
>  
> -    if (defIdx)
> -        virQEMUCapsSetDefaultMachine(qemuCaps, defIdx);
> +    if (preferredIdx == -1)
> +        preferredIdx = defIdx;
> +    virQEMUCapsSetDefaultMachine(qemuCaps, preferredIdx);
>  
>      ret = 0;
>  
> -- 
> 2.17.1

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Fri, Aug 03, 2018 at 01:05:49PM -0300, Eduardo Habkost wrote:
> On Fri, Aug 03, 2018 at 01:59:47PM +0100, Daniel P. Berrangé wrote:
> > It is increasingly likely that some distro is going to change the
> > default "x86" machine type in QEMU from "pc" to "q35". This will
> > certainly break existing applications which write their XML on the
> > assumption that its using a "pc" machine by default. For example they'll
> > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> > radically.
> > 
> > Libvirt promises to isolate applications from hypervisor changes that
> > may cause incompatibilities, so we must ensure that we always use the
> > "pc" machine type if it is available. Only use QEMU's own reported
> > default machine type if "pc" does not exist.
> > 
> > Note this change assumes there will always be a "pc" alias as long as a
> > versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
> > machine type but not provide the "pc" alias, it is too hard to decide
> > which to default so. Versioned machine types are supposed to be
> > considered opaque strings, so we can't apply any sensible ordering
> > ourselves and QEMU isn't reporting the list of machines in any sensible
> > ordering itself.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Won't this break qemuParseCommandLine() if it sees a QEMU binary
> running without "-machine"?  It will assume the QEMU default is
> "pc" but this may be not true.

If no -machine arg is present in ARGV, then the code will lookup the
default machine type for the emulator binary in the capabilities
record. So this should just "do the right thing" with my changes
in this patch.

> > @@ -2263,12 +2274,16 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps,
> >          mach->maxCpus = machines[i]->maxCpus;
> >          mach->hotplugCpus = machines[i]->hotplugCpus;
> >  
> > +        if (STREQ_NULLABLE(mach->alias, preferredAlias))
> > +            preferredIdx = qemuCaps->nmachineTypes - 1;
> > +
> 
> Isn't STREQ_NULLABLE(NULL, NULL) true?  You don't want to set
> preferredIdx here if preferredAlias==NULL.

Opps, yes.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Eduardo Habkost 5 years, 7 months ago
On Tue, Aug 07, 2018 at 12:17:33PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 03, 2018 at 01:05:49PM -0300, Eduardo Habkost wrote:
> > On Fri, Aug 03, 2018 at 01:59:47PM +0100, Daniel P. Berrangé wrote:
> > > It is increasingly likely that some distro is going to change the
> > > default "x86" machine type in QEMU from "pc" to "q35". This will
> > > certainly break existing applications which write their XML on the
> > > assumption that its using a "pc" machine by default. For example they'll
> > > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> > > radically.
> > > 
> > > Libvirt promises to isolate applications from hypervisor changes that
> > > may cause incompatibilities, so we must ensure that we always use the
> > > "pc" machine type if it is available. Only use QEMU's own reported
> > > default machine type if "pc" does not exist.
> > > 
> > > Note this change assumes there will always be a "pc" alias as long as a
> > > versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
> > > machine type but not provide the "pc" alias, it is too hard to decide
> > > which to default so. Versioned machine types are supposed to be
> > > considered opaque strings, so we can't apply any sensible ordering
> > > ourselves and QEMU isn't reporting the list of machines in any sensible
> > > ordering itself.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > Won't this break qemuParseCommandLine() if it sees a QEMU binary
> > running without "-machine"?  It will assume the QEMU default is
> > "pc" but this may be not true.
> 
> If no -machine arg is present in ARGV, then the code will lookup the
> default machine type for the emulator binary in the capabilities
> record. So this should just "do the right thing" with my changes
> in this patch.

I don't see how it would do the right thing.  e.g.: if we have a
QEMU binary that defaults to "q35" and it is running without
"-machine", it will be emulating a q35 machine, not i440fx.  But
with this change qemuParseCommandLine() will incorrectly assume
the existing process is emulating i440fx.

qemuParseCommandLine() calls:

  capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, def->os.arch,
                                             def->virtType, NULL, NULL)

and as far as I can see, your patch will make
qemuParseCommandLine() set capsdata->machinetype to a pc-i440fx-*
machine, even if the QEMU binary defaults to something else.

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Tue, Aug 07, 2018 at 03:53:53PM -0300, Eduardo Habkost wrote:
> On Tue, Aug 07, 2018 at 12:17:33PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 03, 2018 at 01:05:49PM -0300, Eduardo Habkost wrote:
> > > On Fri, Aug 03, 2018 at 01:59:47PM +0100, Daniel P. Berrangé wrote:
> > > > It is increasingly likely that some distro is going to change the
> > > > default "x86" machine type in QEMU from "pc" to "q35". This will
> > > > certainly break existing applications which write their XML on the
> > > > assumption that its using a "pc" machine by default. For example they'll
> > > > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> > > > radically.
> > > > 
> > > > Libvirt promises to isolate applications from hypervisor changes that
> > > > may cause incompatibilities, so we must ensure that we always use the
> > > > "pc" machine type if it is available. Only use QEMU's own reported
> > > > default machine type if "pc" does not exist.
> > > > 
> > > > Note this change assumes there will always be a "pc" alias as long as a
> > > > versioned "pc-XXX" machine type exists. If QEMU were to ship a "pc-XXX"
> > > > machine type but not provide the "pc" alias, it is too hard to decide
> > > > which to default so. Versioned machine types are supposed to be
> > > > considered opaque strings, so we can't apply any sensible ordering
> > > > ourselves and QEMU isn't reporting the list of machines in any sensible
> > > > ordering itself.
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > 
> > > Won't this break qemuParseCommandLine() if it sees a QEMU binary
> > > running without "-machine"?  It will assume the QEMU default is
> > > "pc" but this may be not true.
> > 
> > If no -machine arg is present in ARGV, then the code will lookup the
> > default machine type for the emulator binary in the capabilities
> > record. So this should just "do the right thing" with my changes
> > in this patch.
> 
> I don't see how it would do the right thing.  e.g.: if we have a
> QEMU binary that defaults to "q35" and it is running without
> "-machine", it will be emulating a q35 machine, not i440fx.  But
> with this change qemuParseCommandLine() will incorrectly assume
> the existing process is emulating i440fx.
> 
> qemuParseCommandLine() calls:
> 
>   capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, def->os.arch,
>                                              def->virtType, NULL, NULL)
> 
> and as far as I can see, your patch will make
> qemuParseCommandLine() set capsdata->machinetype to a pc-i440fx-*
> machine, even if the QEMU binary defaults to something else.

Oh right, I mis-interpreted things :-(  This patch is pushed, so I'll think
about a followup fix.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Andrea Bolognani 5 years, 7 months ago
On Fri, 2018-08-03 at 13:59 +0100, Daniel P. Berrangé wrote:
> It is increasingly likely that some distro is going to change the
> default "x86" machine type in QEMU from "pc" to "q35". This will
> certainly break existing applications which write their XML on the
> assumption that its using a "pc" machine by default. For example they'll

s/its/it's/

> lack a IDE CDROM and get PCI-X instad of PCI which changes the topology

s/PCI-X instad/PCIe instead/

[...]
> +    /* Historically QEMU defaulted to 'pc' machine type but in
> +     * future might switch 'q35'. Such a change is considered
> +     * an ABI break from lbivirt's POV, so we must be sure to

s/lbivirt/libvirt/

> +     * keep 'pc' as default machine no matter what QEMU says.
> +     */
> +    if (qemuCaps->arch == VIR_ARCH_X86_64 ||
> +        qemuCaps->arch == VIR_ARCH_I686)
> +        preferredAlias = "pc";

You can use ARCH_IS_X86() here.

Since we're shielding users from changes in the default x86
machine type, I think it would make sense to do the same for other
architectures at the same time: for example, ppc64 should prefer
pseries, s390 should prefer s390-ccw-virtio and so on.

I wonder how to handle architectures where QEMU never declared a
default machine type, such as aarch64 and riscv64, though: I think
it would make sense to prefer the virt machine type there, but I'm
not entirely sure that wouldn't cause any breakages either...

[...]
> +        if (STREQ_NULLABLE(mach->alias, preferredAlias))
> +            preferredIdx = qemuCaps->nmachineTypes - 1;

Eduardo has a good point here - we should make sure preferredAlias
is not NULL before attempting to set preferredIdx.

[...]
> -    if (defIdx)
> -        virQEMUCapsSetDefaultMachine(qemuCaps, defIdx);
> +    if (preferredIdx == -1)
> +        preferredIdx = defIdx;
> +    virQEMUCapsSetDefaultMachine(qemuCaps, preferredIdx);

With this change, you're calling virQEMUCapsSetDefaultMachine()
even when the default machine type is already at the beginning
of the list, which didn't happen before. Shouldn't make any
difference in practice; however, I find preferredIdx and defIdx
having different semantics a bit confusing, so I'd really rather
you made sure that doesn't happen...

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Mon, Aug 06, 2018 at 11:38:06AM +0200, Andrea Bolognani wrote:
> On Fri, 2018-08-03 at 13:59 +0100, Daniel P. Berrangé wrote:
> > It is increasingly likely that some distro is going to change the
> > default "x86" machine type in QEMU from "pc" to "q35". This will
> > certainly break existing applications which write their XML on the
> > assumption that its using a "pc" machine by default. For example they'll
> 
> s/its/it's/
> 
> > lack a IDE CDROM and get PCI-X instad of PCI which changes the topology
> 
> s/PCI-X instad/PCIe instead/
> 
> [...]
> > +    /* Historically QEMU defaulted to 'pc' machine type but in
> > +     * future might switch 'q35'. Such a change is considered
> > +     * an ABI break from lbivirt's POV, so we must be sure to
> 
> s/lbivirt/libvirt/
> 
> > +     * keep 'pc' as default machine no matter what QEMU says.
> > +     */
> > +    if (qemuCaps->arch == VIR_ARCH_X86_64 ||
> > +        qemuCaps->arch == VIR_ARCH_I686)
> > +        preferredAlias = "pc";
> 
> You can use ARCH_IS_X86() here.
> 
> Since we're shielding users from changes in the default x86
> machine type, I think it would make sense to do the same for other
> architectures at the same time: for example, ppc64 should prefer
> pseries, s390 should prefer s390-ccw-virtio and so on.
> 
> I wonder how to handle architectures where QEMU never declared a
> default machine type, such as aarch64 and riscv64, though: I think
> it would make sense to prefer the virt machine type there, but I'm
> not entirely sure that wouldn't cause any breakages either...

Existing libvirt behaviour is that we'll pick the first reported machine
type, so we have to preserve that.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: ensure "pc" machine is always used as default if available
Posted by Andrea Bolognani 5 years, 7 months ago
On Tue, 2018-08-07 at 13:19 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 06, 2018 at 11:38:06AM +0200, Andrea Bolognani wrote:
> > I wonder how to handle architectures where QEMU never declared a
> > default machine type, such as aarch64 and riscv64, though: I think
> > it would make sense to prefer the virt machine type there, but I'm
> > not entirely sure that wouldn't cause any breakages either...
> 
> Existing libvirt behaviour is that we'll pick the first reported machine
> type, so we have to preserve that.

Right, makes sense.

I guess with aarch64/virt first and x86_64/q35 now it won't take
too long for applications to figure out they should specify the
machine type explicitly rather than leaving that up to libvirt :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list