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

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Thu, Jun 07, 2018 at 10:37:43AM +0200, Martin Kletzander wrote:
> The default is stable per machine type so there should be no need to keep that.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1469338
>
> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c                        |  3 +-
> src/qemu/qemu_command.c                       | 18 ++++++++
> src/qemu/qemu_domain.c                        | 35 ++++++++++++++
> .../tseg-explicit-size.x86_64-latest.args     | 35 ++++++++++++++
> tests/qemuxml2argvdata/tseg-explicit-size.xml | 23 ++++++++++
> tests/qemuxml2argvdata/tseg-i440fx.xml        | 23 ++++++++++
> tests/qemuxml2argvdata/tseg-invalid-size.xml  | 23 ++++++++++
> tests/qemuxml2argvtest.c                      | 25 ++++++++++
> .../qemuxml2xmloutdata/tseg-explicit-size.xml | 46 +++++++++++++++++++
> .../tseg-old-machine-type.xml                 | 44 ++++++++++++++++++
> tests/qemuxml2xmloutdata/tseg.xml             | 44 ++++++++++++++++++
> tests/qemuxml2xmltest.c                       |  9 ++++
> 12 files changed, 327 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/tseg-explicit-size.x86_64-latest.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/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/conf/domain_conf.c b/src/conf/domain_conf.c
> index 62bf6bb803bb..e83487d6b0de 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -22043,7 +22043,8 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>             return false;
>         }
>
> -        if (src->tseg_size != dst->tseg_size) {
> +        if (src->tseg_specified &&

Why this change?

IIUC if they weren't specified on both sides, they should both be 0
here.

If you're sure it's needed, put it in the commit adding this check.

> +            src->tseg_size != dst->tseg_size) {
>             const char *unit_src, *unit_dst;
>             unsigned long long short_size_src = virFormatIntPretty(src->tseg_size,
>                                                                    &unit_src);
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6bc9bf5ffab8..4a87b892b7c5 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7295,6 +7295,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>     return ret;
> }
>
> +
> +static void
> +qemuBuildTSEGCommandLine(virCommandPtr cmd,
> +                         const virDomainDef *def)
> +{
> +    if (!def->tseg_size)
> +        return;

If you don't need the tseg_specified bool at all, I suggest just
dropping it. Does a size of 0 MiB make sense? It's divisible by 1 MiB.

> +
> +    virCommandAddArg(cmd, "-global");
> +
> +    /* PostParse callback guarantees that the size is divisible by 1 MiB */
> +    virCommandAddArgFormat(cmd, "mch.extended-tseg-mbytes=%llu",
> +                           def->tseg_size >> 20);
> +}
> +
> +
> static int
> qemuBuildSmpCommandLine(virCommandPtr cmd,
>                         virDomainDefPtr def)
> @@ -10108,6 +10124,8 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>     if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0)
>         goto error;
>
> +    qemuBuildTSEGCommandLine(cmd, def);
> +
>     if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)
>         goto error;
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index a2c4d3a36090..643fca52c17b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3632,6 +3632,38 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
> }
>
>
> +static int
> +qemuDomainDefTsegPostParse(virDomainDefPtr def,

s/qemuDomainDefTsegPostParse/qemuDomainDefTSEGPostParse/

> +                           virQEMUCapsPtr qemuCaps)
> +{
> +    if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON)
> +        return 0;
> +

With the bool variable dropped or defended and consistently used:

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