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@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
>