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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Wed, May 30, 2018 at 08:01:10PM +0200, Ján Tomko wrote:
> On Mon, May 21, 2018 at 05:00:53PM +0200, 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;
> > +
> > +    if (!qemuDomainIsQ35(def))
> > +        return 0;
> > +
> > +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES))
> > +        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))
>
> Please use virParseVersionString

We have that?  Cool.  There was no patch related to that function since I
started working on libvirt =D  And only few of that that just used it.

> > +        def->tseg_size = 16 * 1024 * 1024;
> > +
> > +    return 0;
> > +
> > + error:
> > +    virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                   _("Cannot parse QEMU machine type version '%s'"),
> > +                   def->os.machine);
> > +    return -1;
> > +}
> > +
> > +
> > +static int
> > +qemuDomainDefTsegPostParse(virDomainDefPtr def,
> > +                           virQEMUCapsPtr qemuCaps)
> > +{
> > +    if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
> > +        return 0;
> > +
> > +    if (!def->tseg_size)
> > +        return qemuDomainSetDefaultTsegSize(def, qemuCaps);
> > +
> > +    if (!qemuDomainIsQ35(def)) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                       _("SMM TSEG is only supported with q35 machine type"));
> > +        return -1;
> > +    }
> > +
> > +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                       _("Setting TSEG size is not supported with this QEMU binary"));
> > +        return -1;
> > +    }
> > +
> > +    if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) {
>
> Interesting way of writing the % operator.
>
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                       _("SMM TSEG size must be divisible by 1 MiB"));
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +
> > static int
> > qemuDomainDefPostParseBasic(virDomainDefPtr def,
> >                             virCapsPtr caps,
>
> I'm not sure whether this trial-and-error attribute angers some purists,
> but they'll have plenty of time to object until the freeze is over.

Dou you have any other better idea?  Laszlo properly compared it to picking for
example the right amount of memory.  How can you know how much do you need?  It
can never be automatically guessed.  You can have extra, but each use case will
be hindered a different amount by that.  Feel free to suggest a better idea or
Cc some purists ;)

> Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>
>
> Jano

Attachment: signature.asc
Description: Digital signature

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