Re: [PATCH 5/5] qemu: Add support for setting the TSEG size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 31, 2018 at 03:52:26PM +0200, Pavel Hrdina wrote:
On Thu, May 31, 2018 at 02:22:05PM +0200, Martin Kletzander wrote:
On Thu, May 31, 2018 at 10:52:08AM +0200, Pavel Hrdina wrote:
> On Thu, May 31, 2018 at 10:09:46AM +0200, Martin Kletzander wrote:
> > On Thu, May 31, 2018 at 08:45:39AM +0200, Pavel Hrdina wrote:
> > > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote:
> > > >
> > > > This is way too sparse.
> > > >
> > > > On 05/21/2018 11:00 AM, Martin Kletzander wrote:
> > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
> > > > >
> > > > > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > > > > ---
> > > > >  src/qemu/qemu_command.c                       | 18 ++++
> > > > >  src/qemu/qemu_domain.c                        | 84 +++++++++++++++++++
> > > > >  .../qemuxml2argvdata/tseg-explicit-size.args  | 28 +++++++
> > > > >  tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 +++++
> > > > >  tests/qemuxml2argvdata/tseg-i440fx.xml        | 23 +++++
> > > > >  tests/qemuxml2argvdata/tseg-invalid-size.xml  | 23 +++++
> > > > >  .../tseg-old-machine-type.args                | 27 ++++++
> > > > >  .../tseg-old-machine-type.xml                 | 21 +++++
> > > > >  tests/qemuxml2argvdata/tseg.args              | 28 +++++++
> > > > >  tests/qemuxml2argvdata/tseg.xml               | 21 +++++
> > > > >  tests/qemuxml2argvtest.c                      | 48 +++++++++++
> > > > >  .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 ++++++++++
> > > > >  .../tseg-old-machine-type.xml                 | 44 ++++++++++
> > > > >  tests/qemuxml2xmloutdata/tseg.xml             | 46 ++++++++++
> > > > >  tests/qemuxml2xmltest.c                       | 25 ++++++
> > > > >  15 files changed, 505 insertions(+)
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.args
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.xml
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg-i440fx.xml
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg-invalid-size.xml
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.args
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg-old-machine-type.xml
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg.args
> > > > >  create mode 100644 tests/qemuxml2argvdata/tseg.xml
> > > > >  create mode 100644 tests/qemuxml2xmloutdata/tseg-explicit-size.xml
> > > > >  create mode 100644 tests/qemuxml2xmloutdata/tseg-old-machine-type.xml
> > > > >  create mode 100644 tests/qemuxml2xmloutdata/tseg.xml
> > >
> > > [...]
> > >
> > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > > index 881d0ea46a75..3ea9e3d47344 100644
> > > > > --- a/src/qemu/qemu_domain.c
> > > > > +++ b/src/qemu/qemu_domain.c
> > > > > @@ -3319,6 +3319,87 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> > > > >  }
> > > > >
> > > > >
> > > > > +static int
> > > > > +qemuDomainSetDefaultTsegSize(virDomainDef *def,
> > > > > +                             virQEMUCapsPtr qemuCaps)
> > > > > +{
> > > > > +    const char *machine = NULL;
> > > > > +    char *end_ptr = NULL;
> > > > > +    unsigned int major = 0;
> > > > > +    unsigned int minor = 0;
> > > > > +
> > > > > +    def->tseg_size = 0;
> > > >
> > > > Pointless since the only way in here is "if (tseg_size == 0)"
> > > >
> > > > > +
> > > > > +    if (!qemuDomainIsQ35(def))
> > > > > +        return 0;
> > > > > +
> > > > > +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
> > > >
> > > > Reading this now makes me realized _MBYTES is probably unnecessary, IDC
> > > > though since it does follow the QEMU name.
> > > >
> > > > > +        return 0;
> > > > > +
> > > > > +    machine = STRSKIP(def->os.machine, "pc-q35-");
> > > > > +
> > > > > +    if (!machine)
> > > > > +        goto error;
> > > > > +
> > > > > +    if (virStrToLong_uip(machine, &end_ptr, 10, &major) < 0)
> > > > > +        goto error;
> > > > > +
> > > > > +    if (*end_ptr != '.')
> > > > > +        goto error;
> > > > > +
> > > > > +    machine = end_ptr + 1;
> > > > > +
> > > > > +    if (virStrToLong_uip(machine, &end_ptr, 10, &minor) < 0)
> > > > > +        goto error;
> > > > > +    if (*end_ptr != '\0')
> > > > > +        goto error;
> > > > > +
> > > > > +    /* QEMU started defaulting to 16MiB after 2.9 */
> > > > > +    if (major > 2 || (major == 2 && minor > 9))
> > > > > +        def->tseg_size = 16 * 1024 * 1024;
> > > >
> > > > So, if QEMU defaults to 16MiB, then why do we need so set this at all?
> > > >
> > > > This seems to me we are setting policy which based on history of many
> > > > patches before is a no go.  I'd say NAK to this, unless there is some
> > > > convincing argument made in the commit message and followup responses to
> > > > the series (or you can take Jan's R-By and ignore me - your call.
> > >
> > > I agree with John, this whole function should be removed, we don't have
> > > to set the default in config XML.  However we should record that default
> > > value into live/status XML to make sure that it will not be changed
> > > during migration and to let the user know what is the default value.
> > >
> >
> > That doesn't make sense.  This is part of guest ABI and if you want to be on the
> > safe side when migrating, then you should be way more cautious with the
> > stop/start scenario.  Migration will probably fail (or silently corrupt data,
> > but I believe that wouldn't happen), but stop/start without keeping the default
> > will change that in case QEMU changes default and then the guest ABI will
> > change.  See my other reply to this particular concern.
>
> I'm not sure that's true.  You can consider CPU as guest ABI but using
> host-model the actual CPU is described only in live/status XML but the
> config XML always has mode set to host-model.
>

Well, but you used the wrong example.  The documentation for host-model properly
says:

 "...but shutting down and restarting the guest may present different hardware
  to the guest according to the capabilities of the new host."

host-model is one of the special cases where we cannot provide stable guest ABI
and we explicitly say that.

> Another thing is that if user doesn't care about this value they would
> lose the benefit of using the default hypervisor value and once it get's
> updated in the hypervisor they would be stuck with the previous default.
>

Yes.  But that's actually precisely the reason we are doing this.

> We might need to somehow specify what guest ABI means and what it should
> cover and what can be left to hypervisor.
>

Well, we don't, guest ABI is not a term we would thought of.  Whatever the guest
OS sees as part of the hardware, whatever it can gather about the virtual
machine is part of the guest ABI.

Agreed, I should have express myself precisely, it's definitely guest
ABI.  What I meant is that some changes are not that critical to the
guest OS or they don't have any real effect.

In this case it's just a size of memory, it's not like it will
disable/enable that feature.  FWIU the size can affect whether some
guest with lot of vcpus and/or with lot of memory will not boot.

So yes, ideally we should record the default value also into the config
XML to make sure that once the guest is installed/configured the value
is not changed.  The only thing that I don't personally like is that
the value is hard-coded into libvirt and in the future we will most
likely change it.

One possible compromise could be that we would not set any value at
define time, instead if no value is configured we would get the default
from QEMU and propagate that default into config as well to make sure
that every other time the guest is started again it will have the same
ABI.  I hope that this idea is not too crazy :).


No, that's one of the possibilities.  We might also believe QEMU that
they will not change the default without machine type change and be done
with it.  It will have the same effect as if the VM was launched before
with older libvirt and then with an updated one, which got updated with
QEMU as well.  If QEMU doesn't change the default per machine type, then
we're fine. It seems like an okay way to go.  At least I can yell "told
you so" later in case QEMU changes that default and someone asks us to
start recording the 16MiB value and it's going to be too late =) But
that's not going to happen, right?

I'll send a simplified v2 and we'll see how that goes.

Attachment: signature.asc
Description: Digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux