[libvirt PATCH v2 07/20] cpu_x86: Implement virCPUDataIsIdentical for x86

[Tim Wiederhake]

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