Re: [PATCH v2 3/4] conf: Introduce TCG domain features
On Mon, Nov 22, 2021 at 13:24:02 +0100, Michal Prívozník wrote:
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(a)redhat.com>
>> Tested-by: Kashyap Chamarthy <kchamart(a)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
[...]
>> +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.
That's one of the problems actually. You've designed it such that it
looks very extensible. Separate functions for formatting, parsing etc,
separate structs.
But once another thing is added, everything will need to be refactored,
because the parser will no longer skip setting VIR_DOMAIN_FEATURE_TCG
since the new field will be populated, but not the tb-cache.
This means that everything would need to be refactored anyways.
Do you want me to put if (val > 0) check here or something
different?
Yes, exactly. Same way when you'd be the one adding another field into
this struct.
[...]
>> 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.
Well, it's borderline and I don't care that much either.