Re: [libvirt PATCH v2 07/20] cpu_x86: Implement virCPUDataIsIdentical for x86
On Fri, 2021-11-05 at 16:30 +0100, Michal Prívozník wrote:
On 11/4/21 5:27 PM, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
> ---
> src/cpu/cpu_x86.c | 69
> +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index a08ac225ef..5ce193e693 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -3332,6 +3332,74 @@ virCPUx86DataAddFeature(virCPUData *cpuData,
> }
>
>
> +static bool
> +virCPUx86DataItemIsIdentical(const virCPUx86DataItem *a,
> + const virCPUx86DataItem *b)
> +{
> + if (a->type != b->type)
> + return false;
> +
> + switch (a->type) {
> + case VIR_CPU_X86_DATA_NONE:
> + break;
> +
> + case VIR_CPU_X86_DATA_CPUID:
> + return a->data.cpuid.eax_in == b->data.cpuid.eax_in &&
> + a->data.cpuid.ecx_in == b->data.cpuid.ecx_in &&
> + a->data.cpuid.eax == b->data.cpuid.eax &&
> + a->data.cpuid.ebx == b->data.cpuid.ebx &&
> + a->data.cpuid.ecx == b->data.cpuid.ecx &&
> + a->data.cpuid.edx == b->data.cpuid.edx;
So this can be replaced with memcmp(), but the moment we will want to
store a pointer in .cpuid we will have to rewrite the code to this
explicit variant. So I guess keep it as is?
Thanks, I somehow overlooked that. Will replace this ...
> +
> + case VIR_CPU_X86_DATA_MSR:
> + return a->data.msr.index == b->data.msr.index &&
> + a->data.msr.eax == b->data.msr.eax &&
> + a->data.msr.edx == b->data.msr.edx;
> + }
> +
... and this with calls to `memcmp() == 0` before pushing.
> + return true;
> +}
> +
> +static virCPUCompareResult
> +virCPUx86DataIsIdentical(const virCPUData *a,
> + const virCPUData *b)
> +{
> + const virCPUx86Data *adata;
> + const virCPUx86Data *bdata;
> + size_t i;
> + size_t j;
> +
> + if (!a || !b)
> + return VIR_CPU_COMPARE_ERROR;
> +
> + if (a->arch != b->arch)
> + return VIR_CPU_COMPARE_INCOMPATIBLE;
> +
> + if (!((adata = &a->data.x86) && (bdata = &b->data.x86)))
> + return VIR_CPU_COMPARE_ERROR;
> +
> + if (adata->len != bdata->len)
> + return VIR_CPU_COMPARE_INCOMPATIBLE;
> +
> + for (i = 0; i < adata->len; ++i) {
> + bool found = false;
> +
> + for (j = 0; j < bdata->len; ++j) {
> + if (!virCPUx86DataItemIsIdentical(&adata->items[i],
> + &bdata->items[j]))
> + continue;
> +
> + found = true;
> + break;
> + }
> +
> + if (!found)
> + return VIR_CPU_COMPARE_INCOMPATIBLE;
Do we need this? I mean, couldn't we replace 'found = true' with
;return
VIR_CPU_COMPARE_IDENTICAL;' Or even better, drop the negation in if
and
return the identical value instead of continue.
The order of entries is not guaranteed, this is why we cannot compare
the entries of "adata" and "bdata" one-by-one (O(n)), but check for
every entry in "adata" whether there is an identical entry in "bdata"
(O(n²)).
At the "found = true;" line we only know that the i-th element in
"adata" has an identical entry in "bdata"; but we still need to check
elements "i+1" to "adata->len".
Cheers,
Tim
> + }
> +
> + return VIR_CPU_COMPARE_IDENTICAL;
This can then be INCOMPATIBLE.
> +}
> +
> static bool
> virCPUx86FeatureIsMSR(const char *name)
> {
> @@ -3415,4 +3483,5 @@ struct cpuArchDriver cpuDriverX86 = {
> .copyMigratable = virCPUx86CopyMigratable,
> .validateFeatures = virCPUx86ValidateFeatures,
> .dataAddFeature = virCPUx86DataAddFeature,
> + .dataIsIdentical = virCPUx86DataIsIdentical,
> };
>
Michal