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

[Pavel Hrdina]

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 at redhat.com>
> > ---
> >  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.

In order to do that, look at qemuProcessRefreshState() function where we
already gather some information from QEMU after starting domain, the
tseg default value can by updated by calling:

    qemuMonitorJSONGetObjectProperty() with
        path: /machine/q35/mch
        property: extended-tseg-mbytes

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180531/d7858c7f/attachment-0001.sig>