[PATCH v2 3/4] conf: Introduce TCG domain features

[Peter Krempa]

On Fri, Nov 05, 2021 at 10:35:19 +0100, Michal Privoznik wrote:
> It may come handy to be able to tweak TCG options, in this
> specific case the size of translation block cache size (tb-size).
> Since we can expect more knobs to tweak let's put them under
> common element, like this:
> 
>   <domain>
>     <features>
>       <tcg>
>         <tb-cache unit='MiB'>128</tb-cache>
>       </tcg>
>     </features>
>   </domain>
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> Tested-by: Kashyap Chamarthy <kchamart at redhat.com>
> ---
>  docs/formatdomain.rst                         | 11 +++
>  docs/schemas/domaincommon.rng                 | 15 +++-
>  src/conf/domain_conf.c                        | 90 +++++++++++++++++++
>  src/conf/domain_conf.h                        |  7 ++
>  src/qemu/qemu_validate.c                      | 11 +++
>  .../x86_64-default-cpu-tcg-features.xml       | 67 ++++++++++++++
>  ...default-cpu-tcg-features.x86_64-latest.xml |  1 +
>  tests/qemuxml2xmltest.c                       |  1 +
>  8 files changed, 202 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml
>  create mode 120000 tests/qemuxml2xmloutdata/x86_64-default-cpu-tcg-features.x86_64-latest.xml

[...]


> @@ -21555,6 +21585,39 @@ virDomainRedirFilterDefCheckABIStability(virDomainRedirFilterDef *src,
>  }
>  
>  
> +static bool
> +virDomainFeatureTCGCheckABIStability(const virDomainDef *src,
> +                                     const virDomainDef *dst)
> +{
> +    const int srcF = src->features[VIR_DOMAIN_FEATURE_TCG];
> +    const int dstF = dst->features[VIR_DOMAIN_FEATURE_TCG];
> +
> +    if (srcF != dstF ||
> +        !!src->tcg_features != !!dst->tcg_features) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("State of feature '%s' differs: "
> +                         "source: '%s', destination: '%s'"),
> +                       virDomainFeatureTypeToString(VIR_DOMAIN_FEATURE_TCG),
> +                       virTristateSwitchTypeToString(srcF),
> +                       virTristateSwitchTypeToString(dstF));
> +        return false;
> +    }
> +
> +    if (!src->tcg_features && !dst->tcg_features)
> +        return true;

This check is either questionable (e.g. if just one of them is non-NULL,
this doesn't trigger and the subsequent condition dereferences NULL in
the other one), or unnecessary.

> +    if (src->tcg_features->tb_cache != dst->tcg_features->tb_cache) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("TCG tb-cache mismatch: source %llu, destination: %llu"),
> +                       src->tcg_features->tb_cache,
> +                       dst->tcg_features->tb_cache);

I don't think this is ABI, do you have any supporting evidence? If yes,
put it into the commnet for the next person questioning this.

> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +
>  static bool
>  virDomainDefFeaturesCheckABIStability(virDomainDef *src,
>                                        virDomainDef *dst)

[...]

> @@ -27691,6 +27759,24 @@ virDomainDefFormatBlkiotune(virBuffer *buf,
>  }
>  
>  
> +static void
> +virDomainFeatureTCGFormat(virBuffer *buf,
> +                          const virDomainDef *def)
> +{
> +    g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
> +
> +    if (!def->tcg_features ||
> +        def->features[VIR_DOMAIN_FEATURE_TCG] != VIR_TRISTATE_SWITCH_ON)
> +        return;
> +
> +    virBufferAsprintf(&childBuf,
> +                      "<tb-cache unit='KiB'>%lld</tb-cache>\n",
> +                      def->tcg_features->tb_cache);

This is not very extensible and similarly the parser as setting the
cache to 0 is considered as not being present.

> +
> +    virXMLFormatElement(buf, "tcg", NULL, &childBuf);
> +}
> +
> +
>  static int
>  virDomainDefFormatFeatures(virBuffer *buf,
>                             virDomainDef *def)

[...]

> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 397eea5ede..bd33c9a800 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -294,6 +294,17 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
>              }
>              break;
>  
> +        case VIR_DOMAIN_FEATURE_TCG:
> +            if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> +                if (def->virtType != VIR_DOMAIN_VIRT_QEMU) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                                   _("TCG features are incompatible with domain type '%s'"),
> +                                   virDomainVirtTypeToString(def->virtType));
> +                    return -1;
> +                }
> +            }
> +            break;
> +

Preferably, implement the qemu logic in the patch adding the qemu bits. 

>          case VIR_DOMAIN_FEATURE_SMM:
>          case VIR_DOMAIN_FEATURE_KVM:
>          case VIR_DOMAIN_FEATURE_XEN:
> diff --git a/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml
> new file mode 100644
> index 0000000000..e2058487b2
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/x86_64-default-cpu-tcg-features.xml
> @@ -0,0 +1,67 @@
> +<domain type='qemu'>
> +  <name>guest</name>
> +  <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
> +  <memory unit='KiB'>4194304</memory>
> +  <currentMemory unit='KiB'>4194304</currentMemory>
> +  <vcpu placement='static'>4</vcpu>
> +  <os>
> +    <type arch='x86_64' machine='pc-q35-6.2'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <features>
> +    <acpi/>
> +    <apic/>
> +    <tcg>
> +      <tb-cache unit='KiB'>102400</tb-cache>
> +    </tcg>
> +  </features>
> +  <cpu mode='custom' match='exact' check='none'>
> +    <model fallback='forbid'>qemu64</model>
> +  </cpu>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-x86_64</emulator>
> +    <disk type='file' device='disk'>
> +      <driver name='qemu' type='qcow2'/>
> +      <source file='/var/lib/libvirt/images/guest.qcow2'/>
> +      <target dev='vda' bus='virtio'/>
> +      <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
> +    </disk>

Storage is not relevant to the test. Please remove the disk from this
definition.