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

[Michal Prívozník]

On 11/22/21 10:30, Peter Krempa wrote:
> 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.

Fair enough. I don't have any evidence. Let me remove the whole function.

> 
>> +        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.

I'm not sure what you mean. If value 0 is passed then the parser won't
set def->features[VIR_DOMAIN_FEATURE_TCG] so this function exits early.
Do you want me to put if (val > 0) check here or something different?

> 
>> +
>> +    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. 

Fair enough. I thought that in this case it borderline okay, but I don't
care that much really.

Michal