diff mbox series

Modify virCPUarmCompare to perform compare actions

Message ID OSYP286MB0181C4EA8A2E1DBCF0F0D457995B0@OSYP286MB0181.JPNP286.PROD.OUTLOOK.COM
State New
Headers show
Series Modify virCPUarmCompare to perform compare actions | expand

Commit Message

Zhenyu Zheng Aug. 21, 2020, 2:20 a.m. UTC
Modify virCPUarmCompare in cpu_arm.c to perform
actual compare actions. Compare host cpu vendor
and model info with guest cpu as initial implementation,
as most ARM clouds uses host-passthrogh mode.

Signed-off-by: Zhenyu Zheng <zheng.zhenyu@outlook.com>

---
 src/cpu/cpu_arm.c | 188 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 184 insertions(+), 4 deletions(-)

-- 
2.20.1

Comments

Daniel Henrique Barboza Aug. 31, 2020, 1:13 p.m. UTC | #1
On 8/20/20 11:20 PM, Zhenyu Zheng wrote:
> Modify virCPUarmCompare in cpu_arm.c to perform

> actual compare actions. Compare host cpu vendor

> and model info with guest cpu as initial implementation,

> as most ARM clouds uses host-passthrogh mode.


Typo: host-passthrogh -> host-passthrough

> 

> Signed-off-by: Zhenyu Zheng <zheng.zhenyu@outlook.com>

> ---

>   src/cpu/cpu_arm.c | 188 +++++++++++++++++++++++++++++++++++++++++++++-

>   1 file changed, 184 insertions(+), 4 deletions(-)

> 

> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c

> index 374a4d6f6c..98c5e551f5 100644

> --- a/src/cpu/cpu_arm.c

> +++ b/src/cpu/cpu_arm.c

> @@ -118,6 +118,36 @@ virCPUarmMapNew(void)

>       return map;

>   }

>   

> +static int

> +virCPUarmDataCopy(virCPUarmData *dst, const virCPUarmData *src)

> +{

> +    if (VIR_ALLOC(dst->features) < 0)

> +        return -1;

> +

> +    dst->pvr = src->pvr;

> +    dst->vendor_id = src->vendor_id;

> +    dst->features = src->features;

> +

> +    return 0;

> +}

> +

> +static virCPUDataPtr

> +virCPUarmMakeCPUData(virArch arch,

> +                     virCPUarmData *data)

> +{

> +    virCPUDataPtr cpuData;

> +

> +    if (VIR_ALLOC(cpuData) < 0)

> +        return NULL;

> +

> +    cpuData->arch = arch;

> +

> +    if (virCPUarmDataCopy(&cpuData->data.arm, data) < 0)

> +        VIR_FREE(cpuData);

> +

> +    return cpuData;

> +}

> +

>   static void

>   virCPUarmDataClear(virCPUarmData *data)

>   {

> @@ -376,6 +406,42 @@ virCPUarmModelParse(xmlXPathContextPtr ctxt,

>       return 0;

>   }

>   

> +static virCPUarmModelPtr

> +virCPUarmModelCopy(virCPUarmModelPtr model)

> +{

> +    virCPUarmModelPtr copy;

> +

> +    copy = g_new0(virCPUarmModel, 1);

> +    copy->name = g_strdup(model->name);

> +    virCPUarmDataCopy(&copy->data, &model->data);

> +    copy->vendor = model->vendor;

> +

> +    return g_steal_pointer(&copy);



You don't need g_steal_pointer() in this situation - you're not
attempting to VIR_FREE ou using g_autoptr() the 'copy' pointer.
'return copy' works fine in this situation.


> +}

> +

> +static virCPUarmModelPtr

> +virCPUarmModelFromCPU(const virCPUDef *cpu,

> +                      virCPUarmMapPtr map)

> +{

> +    g_autoptr(virCPUarmModel) model = NULL;

> +

> +    if (!cpu->model) {

> +        virReportError(VIR_ERR_INVALID_ARG, "%s",

> +                       _("no CPU model specified"));

> +        return NULL;

> +    }

> +

> +    if (!(model = virCPUarmModelFind(map, cpu->model))) {

> +        virReportError(VIR_ERR_INTERNAL_ERROR,

> +                        _("Unknown CPU model %s"), cpu->model);

> +        return NULL;

> +    }

> +

> +    model = virCPUarmModelCopy(model);

> +

> +    return g_steal_pointer(&model);



The reason g_steal_pointer() is needed here is because you're using
g_autoptr() in the initialization, but you don't need g_autoptr() in
this case. virCPUarmModelFind() will return a pointer to an existing
virCPUarmModel in 'map', then you're copying it to be able to return it
to the caller.

You can get rid of both g_autoptr() and g_steal_pointer().

> +}

> +

>   static virCPUarmMapPtr

>   virCPUarmLoadMap(void)

>   {

> @@ -463,11 +529,125 @@ virCPUarmBaseline(virCPUDefPtr *cpus,

>   }

>   

>   static virCPUCompareResult

> -virCPUarmCompare(virCPUDefPtr host G_GNUC_UNUSED,

> -                 virCPUDefPtr cpu G_GNUC_UNUSED,

> -                 bool failMessages G_GNUC_UNUSED)

> +armCompute(virCPUDefPtr host,

> +           virCPUDefPtr cpu,

> +           virCPUDataPtr *guestData,

> +           char **message)

>   {

> -    return VIR_CPU_COMPARE_IDENTICAL;

> +    virCPUarmMapPtr map = NULL;

> +    virCPUarmModelPtr host_model = NULL;

> +    virCPUarmModelPtr guest_model = NULL;

> +    virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR;

> +    virArch arch;

> +    size_t i;

> +

> +    if (cpu->arch != VIR_ARCH_NONE) {

> +        bool found = false;

> +

> +        for (i = 0; i < G_N_ELEMENTS(archs); i++) {

> +            if (archs[i] == cpu->arch) {

> +                found = true;

> +                break;

> +            }

> +        }

> +

> +        if (!found) {

> +            VIR_DEBUG("CPU arch %s does not match host arch",

> +                      virArchToString(cpu->arch));

> +            if (message)

> +                *message = g_strdup_printf(_("CPU arch %s does not match host arch"),

> +                                           virArchToString(cpu->arch));

> +

> +            ret = VIR_CPU_COMPARE_INCOMPATIBLE;

> +            goto cleanup;

> +        }

> +        arch = cpu->arch;

> +    } else {

> +        arch = host->arch;

> +    }

> +

> +    if (cpu->vendor &&

> +        (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) {

> +        VIR_DEBUG("host CPU vendor does not match required CPU vendor %s",

> +                  cpu->vendor);

> +        if (message) {

> +            *message = g_strdup_printf(_("host CPU vendor does not match required "

> +                                         "CPU vendor %s"),

> +                                       cpu->vendor);

> +        }

> +

> +        ret = VIR_CPU_COMPARE_INCOMPATIBLE;

> +        goto cleanup;

> +    }

> +

> +    if (!(map = virCPUarmLoadMap()))

> +        goto cleanup;

> +

> +    /* Host CPU information */

> +    if (!(host_model = virCPUarmModelFromCPU(host, map)))

> +        goto cleanup;

> +

> +    /* Guest CPU information */

> +    if (!(guest_model = virCPUarmModelFromCPU(cpu, map)))

> +        goto cleanup;

> +

> +    if (STRNEQ(guest_model->name, host_model->name)) {

> +        VIR_DEBUG("host CPU model does not match required CPU model %s",

> +                  guest_model->name);

> +        if (message) {

> +            *message = g_strdup_printf(_("host CPU model does not match required "

> +                                         "CPU model %s"),

> +                                       guest_model->name);

> +        }

> +

> +        ret = VIR_CPU_COMPARE_INCOMPATIBLE;

> +        goto cleanup;

> +    }

> +

> +    if (guestData)

> +        if (!(*guestData = virCPUarmMakeCPUData(arch, &guest_model->data)))

> +            goto cleanup;

> +

> +    ret = VIR_CPU_COMPARE_IDENTICAL;

> +

> + cleanup:

> +    virCPUarmModelFree(host_model);

> +    virCPUarmModelFree(guest_model);

> +    return ret;

> +}

> +

> +static virCPUCompareResult

> +virCPUarmCompare(virCPUDefPtr host,

> +                 virCPUDefPtr cpu,

> +                 bool failIncompatible)

> +{

> +        virCPUCompareResult ret;


Extra spaces on indentation


> +    char *message = NULL;


You can use g_autofree char *message and avoid the last VIR_FREE call.



Thanks,


DHB



> +

> +    if (!host || !host->model) {

> +        if (failIncompatible) {

> +            virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s",

> +                           _("unknown host CPU"));

> +        } else {

> +            VIR_WARN("unknown host CPU");

> +            ret = VIR_CPU_COMPARE_INCOMPATIBLE;

> +        }

> +        return -1;

> +    }

> +

> +    ret = armCompute(host, cpu, NULL, &message);

> +

> +    if (failIncompatible && ret == VIR_CPU_COMPARE_INCOMPATIBLE) {

> +        ret = VIR_CPU_COMPARE_ERROR;

> +        if (message) {

> +            virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s", message);

> +        } else {

> +            virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL);

> +        }

> +    }

> +    VIR_FREE(message);

> +

> +    return ret;

>   }

>   

>   static int

>
Jiri Denemark Sept. 1, 2020, 2:44 p.m. UTC | #2
On Fri, Aug 21, 2020 at 10:20:15 +0800, Zhenyu Zheng wrote:
> Modify virCPUarmCompare in cpu_arm.c to perform

> actual compare actions. Compare host cpu vendor

> and model info with guest cpu as initial implementation,

> as most ARM clouds uses host-passthrogh mode.


In addition to the low-level coding issues found by Daniel Henrique
Barboza, I'd like to ask some high level questions...

What is the point in making this patch (except for copying the logic
from x86 CPU driver, which mostly does not fit ARM world very well)?

As you say, most ARM clouds use host-passthrough, so why would CPU
comparison be needed at all? Host-passthrough CPU is by definition
compatible with any host CPU as it asks for the host CPU itself.

Also IIRC the CPU model names as advertised by libvirt in host
capabilities are only useful for identifying the host CPU, but they
cannot be directly pass to QEMU. As such, you can't use these models
when defining a CPU in a domain XML.

That said, I doubt this patch is useful at all.

Jirka
Zhenyu Zheng Sept. 3, 2020, 11:50 a.m. UTC | #3
Hi,

Thanks alot for the review and feedback. As for host-passthrough cases, I
have some other understandings,
if I understand correctly, what you mean is that if a vm uses
host-passthrough, it can migrate to any other
host, since it asks for host-passthrough. In my point of view, I think in
real cases, there are few different kinds
of ARM datacenter CPUs on the market, and there CPU capabilities are
different, so one might create a vm on
hostA with feature1 and feature2 because it uses host-passthrough and hostA
has these features. Now in your
definition(if I understand correctly) of host-passthrough and the current
code(returns identical directly), this vm
can be migrated to hostB with only feature1, since there are no
limitations. If one has some important application
that depends on feature2, the app will break as feature2 is not available
on hostB. Considering this, I proposed
to add basic checks to compare CPU to limit the migration to only the same
CPU models. And once the capability
of ARM driver is enhanced in QEMU or other related projects, we can make
the compare API better.

And yes, the code referenced X86 and S390 driver, I have modified them to
be workable with ARM and tested
the functions, I was also thinking that in the future there might be
possibility that we can compare cpu features
so I kept the data compare case.

Thanks again for the feedback.

Zhenyu



On Tue, Sep 1, 2020 at 10:50 PM Jiri Denemark <jdenemar@redhat.com> wrote:

> On Fri, Aug 21, 2020 at 10:20:15 +0800, Zhenyu Zheng wrote:

> > Modify virCPUarmCompare in cpu_arm.c to perform

> > actual compare actions. Compare host cpu vendor

> > and model info with guest cpu as initial implementation,

> > as most ARM clouds uses host-passthrogh mode.

>

> In addition to the low-level coding issues found by Daniel Henrique

> Barboza, I'd like to ask some high level questions...

>

> What is the point in making this patch (except for copying the logic

> from x86 CPU driver, which mostly does not fit ARM world very well)?

>

> As you say, most ARM clouds use host-passthrough, so why would CPU

> comparison be needed at all? Host-passthrough CPU is by definition

> compatible with any host CPU as it asks for the host CPU itself.

>

> Also IIRC the CPU model names as advertised by libvirt in host

> capabilities are only useful for identifying the host CPU, but they

> cannot be directly pass to QEMU. As such, you can't use these models

> when defining a CPU in a domain XML.

>

> That said, I doubt this patch is useful at all.

>

> Jirka

>

>
<div dir="ltr"><div>Hi,</div><div><br></div>Thanks alot for the review and feedback. As for host-passthrough cases, I have some other understandings,<div>if I understand correctly, what you mean is that if a vm uses host-passthrough, it can migrate to any other</div><div>host, since it asks for host-passthrough. In my point of view, I think in real cases, there are few different kinds</div><div>of ARM datacenter CPUs on the market, and there CPU capabilities are different, so one might create a vm on</div><div>hostA with feature1 and feature2 because it uses host-passthrough and hostA has these features. Now in your</div><div>definition(if I understand correctly) of host-passthrough and the current code(returns identical directly), this vm</div><div>can be migrated to hostB with only feature1, since there are no limitations. If one has some important application</div><div>that depends on feature2, the app will break as feature2 is not available on hostB. Considering this, I proposed</div><div>to add basic checks to compare CPU to limit the migration to only the same CPU models. And once the capability</div><div>of ARM driver is enhanced in QEMU or other related projects, we can make the compare API better.</div><div><br></div><div>And yes, the code referenced X86 and S390 driver, I have modified them to be workable with ARM and tested</div><div>the functions, I was also thinking that in the future there might be possibility that we can compare cpu features</div><div>so I kept the data compare case.</div><div><br></div><div>Thanks again for the feedback.</div><div><br></div><div>Zhenyu</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 1, 2020 at 10:50 PM Jiri Denemark &lt;<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Aug 21, 2020 at 10:20:15 +0800, Zhenyu Zheng wrote:<br>
&gt; Modify virCPUarmCompare in cpu_arm.c to perform<br>
&gt; actual compare actions. Compare host cpu vendor<br>
&gt; and model info with guest cpu as initial implementation,<br>
&gt; as most ARM clouds uses host-passthrogh mode.<br>
<br>
In addition to the low-level coding issues found by Daniel Henrique<br>
Barboza, I&#39;d like to ask some high level questions...<br>
<br>
What is the point in making this patch (except for copying the logic<br>
from x86 CPU driver, which mostly does not fit ARM world very well)?<br>
<br>
As you say, most ARM clouds use host-passthrough, so why would CPU<br>
comparison be needed at all? Host-passthrough CPU is by definition<br>
compatible with any host CPU as it asks for the host CPU itself.<br>
<br>
Also IIRC the CPU model names as advertised by libvirt in host<br>
capabilities are only useful for identifying the host CPU, but they<br>
cannot be directly pass to QEMU. As such, you can&#39;t use these models<br>
when defining a CPU in a domain XML.<br>
<br>
That said, I doubt this patch is useful at all.<br>
<br>
Jirka<br>
<br>
</blockquote></div>
Jiri Denemark Sept. 4, 2020, 11:59 a.m. UTC | #4
On Thu, Sep 03, 2020 at 19:50:13 +0800, Zhenyu Zheng wrote:
> Hi,

> 

> Thanks alot for the review and feedback. As for host-passthrough cases, I

> have some other understandings,

> if I understand correctly, what you mean is that if a vm uses

> host-passthrough, it can migrate to any other

> host, since it asks for host-passthrough. In my point of view, I think in

> real cases, there are few different kinds

> of ARM datacenter CPUs on the market, and there CPU capabilities are

> different, so one might create a vm on

> hostA with feature1 and feature2 because it uses host-passthrough and hostA

> has these features. Now in your

> definition(if I understand correctly) of host-passthrough and the current

> code(returns identical directly), this vm

> can be migrated to hostB with only feature1, since there are no

> limitations. If one has some important application

> that depends on feature2, the app will break as feature2 is not available

> on hostB.


And that's why migrating a domain with host-passthrough CPU is generally
not supported. It should work if you have two identical hosts in terms
of CPU, microcode (if applicable to ARM), system settings (not sure
about ARM, but x86 let's you hide some CPU features), kernel and its
command line arguments, kvm modules and their options, and QEMU. If any
of this is different the guest CPU may change in an unexpected way
during migration and there's no way libvirt could know about it or
prevent it in general.

Even if we made the change and compared CPU features, the CPUs may have
other features which libvirt does not know about or the CPUs could even
differ in other aspects which libvirt does not cover in the CPU modeling
code.

But thinking about this and your patch, I guess I know what you are
trying to do. You're not trying to compare the CPU definition from a
domain XML (which was the primary use case for CPU comparison APIs), but
you want to compare the host CPU definition from one host to the host
CPU on the other host to see whether the two hosts might be compatible.

This sounds like a valid use case (not sure it is that useful), but we
need to be careful. But we should make sure implementing this does not
break anything. This means, we need to do different things depending on
the type of the CPU definition we are asked to compare. Guest CPU
definitions should keep the old behaviour (return IDENTICAL) and host
CPU definitions can be compared to the host CPU. But when doing so,
don't get too influenced by x86 code, which I believe is way too
complicated for ARM. Specifically you don't need to create armCompute
and mess with guestData and other stuff there as all you want to do is
compare the two CPU definitions. In x86 the same function is used for
several things, but that's not the case for ARM.

> Considering this, I proposed to add basic checks to compare CPU to

> limit the migration to only the same CPU models. And once the

> capability of ARM driver is enhanced in QEMU or other related

> projects, we can make the compare API better.


Sure, but that would be comparing guest CPU definition with the CPU we
get from QEMU rather than the one we detect ourselves.

Jirka
Zhenyu Zheng Sept. 7, 2020, 1:21 a.m. UTC | #5
Thanks alot for the reply,

This sounds like a valid use case (not sure it is that useful), but we
> need to be careful. But we should make sure implementing this does not

> break anything. This means, we need to do different things depending on

> the type of the CPU definition we are asked to compare. Guest CPU

> definitions should keep the old behaviour (return IDENTICAL) and host

> CPU definitions can be compared to the host CPU. But when doing so,

> don't get too influenced by x86 code, which I believe is way too

> complicated for ARM. Specifically you don't need to create armCompute

> and mess with guestData and other stuff there as all you want to do is

> compare the two CPU definitions. In x86 the same function is used for

> several things, but that's not the case for ARM.



for this, what I have in mind now is that we can check
`VIR_CPU_MODE_HOST_PASSTHROUGH`
and if that is the case, we compare CPU vendors and models to allow only
identical definitions to pass,
like the implementation of
https://gitlab.com/libvirt/libvirt/-/blob/master/src/cpu/cpu_ppc64.c#L505 ,
(this is because when a VM is in host-passthrough mode, its' CPU xml
reflects the original host CPU
definition, and we actually compare the source host and destination host
CPU definitions,
I think this suits your ideas too) and for other cases, we just return
IDENTICAL as is.

What do you think?

Thanks alot.

Zhenyu Zheng


On Fri, Sep 4, 2020 at 8:00 PM Jiri Denemark <jdenemar@redhat.com> wrote:

> On Thu, Sep 03, 2020 at 19:50:13 +0800, Zhenyu Zheng wrote:

> > Hi,

> >

> > Thanks alot for the review and feedback. As for host-passthrough cases, I

> > have some other understandings,

> > if I understand correctly, what you mean is that if a vm uses

> > host-passthrough, it can migrate to any other

> > host, since it asks for host-passthrough. In my point of view, I think in

> > real cases, there are few different kinds

> > of ARM datacenter CPUs on the market, and there CPU capabilities are

> > different, so one might create a vm on

> > hostA with feature1 and feature2 because it uses host-passthrough and

> hostA

> > has these features. Now in your

> > definition(if I understand correctly) of host-passthrough and the current

> > code(returns identical directly), this vm

> > can be migrated to hostB with only feature1, since there are no

> > limitations. If one has some important application

> > that depends on feature2, the app will break as feature2 is not available

> > on hostB.

>

> And that's why migrating a domain with host-passthrough CPU is generally

> not supported. It should work if you have two identical hosts in terms

> of CPU, microcode (if applicable to ARM), system settings (not sure

> about ARM, but x86 let's you hide some CPU features), kernel and its

> command line arguments, kvm modules and their options, and QEMU. If any

> of this is different the guest CPU may change in an unexpected way

> during migration and there's no way libvirt could know about it or

> prevent it in general.

>

> Even if we made the change and compared CPU features, the CPUs may have

> other features which libvirt does not know about or the CPUs could even

> differ in other aspects which libvirt does not cover in the CPU modeling

> code.

>

> But thinking about this and your patch, I guess I know what you are

> trying to do. You're not trying to compare the CPU definition from a

> domain XML (which was the primary use case for CPU comparison APIs), but

> you want to compare the host CPU definition from one host to the host

> CPU on the other host to see whether the two hosts might be compatible.

>

> This sounds like a valid use case (not sure it is that useful), but we

> need to be careful. But we should make sure implementing this does not

> break anything. This means, we need to do different things depending on

> the type of the CPU definition we are asked to compare. Guest CPU

> definitions should keep the old behaviour (return IDENTICAL) and host

> CPU definitions can be compared to the host CPU. But when doing so,

> don't get too influenced by x86 code, which I believe is way too

> complicated for ARM. Specifically you don't need to create armCompute

> and mess with guestData and other stuff there as all you want to do is

> compare the two CPU definitions. In x86 the same function is used for

> several things, but that's not the case for ARM.

>

> > Considering this, I proposed to add basic checks to compare CPU to

> > limit the migration to only the same CPU models. And once the

> > capability of ARM driver is enhanced in QEMU or other related

> > projects, we can make the compare API better.

>

> Sure, but that would be comparing guest CPU definition with the CPU we

> get from QEMU rather than the one we detect ourselves.

>

> Jirka

>

>
<div dir="ltr">Thanks alot for the reply, <div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This sounds like a valid use case (not sure it is that useful), but we<br>need to be careful. But we should make sure implementing this does not<br>break anything. This means, we need to do different things depending on<br>the type of the CPU definition we are asked to compare. Guest CPU<br>definitions should keep the old behaviour (return IDENTICAL) and host<br>CPU definitions can be compared to the host CPU. But when doing so,<br>don&#39;t get too influenced by x86 code, which I believe is way too<br>complicated for ARM. Specifically you don&#39;t need to create armCompute<br>and mess with guestData and other stuff there as all you want to do is<br>compare the two CPU definitions. In x86 the same function is used for<br>several things, but that&#39;s not the case for ARM.</blockquote><div><br></div><div>for this, what I have in mind now is that we can check `VIR_CPU_MODE_HOST_PASSTHROUGH`</div><div>and if that is the case, we compare CPU vendors and models to allow only identical definitions to pass,</div><div>like the implementation of <a href="https://gitlab.com/libvirt/libvirt/-/blob/master/src/cpu/cpu_ppc64.c#L505">https://gitlab.com/libvirt/libvirt/-/blob/master/src/cpu/cpu_ppc64.c#L505</a> ,</div><div>(this is because when a VM is in host-passthrough mode, its&#39; CPU xml reflects the original host CPU</div><div>definition, and we actually compare the source host and destination host CPU definitions,</div><div>I think this suits your ideas too) and for other cases, we just return IDENTICAL as is.</div><div><br></div><div>What do you think?</div><div><br></div><div>Thanks alot.</div><div><br></div><div>Zhenyu Zheng</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 4, 2020 at 8:00 PM Jiri Denemark &lt;<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Sep 03, 2020 at 19:50:13 +0800, Zhenyu Zheng wrote:<br>
&gt; Hi,<br>
&gt; <br>
&gt; Thanks alot for the review and feedback. As for host-passthrough cases, I<br>
&gt; have some other understandings,<br>
&gt; if I understand correctly, what you mean is that if a vm uses<br>
&gt; host-passthrough, it can migrate to any other<br>
&gt; host, since it asks for host-passthrough. In my point of view, I think in<br>
&gt; real cases, there are few different kinds<br>
&gt; of ARM datacenter CPUs on the market, and there CPU capabilities are<br>
&gt; different, so one might create a vm on<br>
&gt; hostA with feature1 and feature2 because it uses host-passthrough and hostA<br>
&gt; has these features. Now in your<br>
&gt; definition(if I understand correctly) of host-passthrough and the current<br>
&gt; code(returns identical directly), this vm<br>
&gt; can be migrated to hostB with only feature1, since there are no<br>
&gt; limitations. If one has some important application<br>
&gt; that depends on feature2, the app will break as feature2 is not available<br>
&gt; on hostB.<br>
<br>
And that&#39;s why migrating a domain with host-passthrough CPU is generally<br>
not supported. It should work if you have two identical hosts in terms<br>
of CPU, microcode (if applicable to ARM), system settings (not sure<br>
about ARM, but x86 let&#39;s you hide some CPU features), kernel and its<br>
command line arguments, kvm modules and their options, and QEMU. If any<br>
of this is different the guest CPU may change in an unexpected way<br>
during migration and there&#39;s no way libvirt could know about it or<br>
prevent it in general.<br>
<br>
Even if we made the change and compared CPU features, the CPUs may have<br>
other features which libvirt does not know about or the CPUs could even<br>
differ in other aspects which libvirt does not cover in the CPU modeling<br>
code.<br>
<br>
But thinking about this and your patch, I guess I know what you are<br>
trying to do. You&#39;re not trying to compare the CPU definition from a<br>
domain XML (which was the primary use case for CPU comparison APIs), but<br>
you want to compare the host CPU definition from one host to the host<br>
CPU on the other host to see whether the two hosts might be compatible.<br>
<br>
This sounds like a valid use case (not sure it is that useful), but we<br>
need to be careful. But we should make sure implementing this does not<br>
break anything. This means, we need to do different things depending on<br>
the type of the CPU definition we are asked to compare. Guest CPU<br>
definitions should keep the old behaviour (return IDENTICAL) and host<br>
CPU definitions can be compared to the host CPU. But when doing so,<br>
don&#39;t get too influenced by x86 code, which I believe is way too<br>
complicated for ARM. Specifically you don&#39;t need to create armCompute<br>
and mess with guestData and other stuff there as all you want to do is<br>
compare the two CPU definitions. In x86 the same function is used for<br>
several things, but that&#39;s not the case for ARM.<br>
<br>
&gt; Considering this, I proposed to add basic checks to compare CPU to<br>
&gt; limit the migration to only the same CPU models. And once the<br>
&gt; capability of ARM driver is enhanced in QEMU or other related<br>
&gt; projects, we can make the compare API better.<br>
<br>
Sure, but that would be comparing guest CPU definition with the CPU we<br>
get from QEMU rather than the one we detect ourselves.<br>
<br>
Jirka<br>
<br>
</blockquote></div>
Jiri Denemark Sept. 7, 2020, 11:15 a.m. UTC | #6
On Mon, Sep 07, 2020 at 09:21:02 +0800, Zhenyu Zheng wrote:
> Thanks alot for the reply,

> 

> This sounds like a valid use case (not sure it is that useful), but we

> > need to be careful. But we should make sure implementing this does not

> > break anything. This means, we need to do different things depending on

> > the type of the CPU definition we are asked to compare. Guest CPU

> > definitions should keep the old behaviour (return IDENTICAL) and host

> > CPU definitions can be compared to the host CPU. But when doing so,

> > don't get too influenced by x86 code, which I believe is way too

> > complicated for ARM. Specifically you don't need to create armCompute

> > and mess with guestData and other stuff there as all you want to do is

> > compare the two CPU definitions. In x86 the same function is used for

> > several things, but that's not the case for ARM.

> 

> 

> for this, what I have in mind now is that we can check

> `VIR_CPU_MODE_HOST_PASSTHROUGH`

> and if that is the case, we compare CPU vendors and models to allow only

> identical definitions to pass, like the implementation of

> https://gitlab.com/libvirt/libvirt/-/blob/master/src/cpu/cpu_ppc64.c#L505 ,


This doesn't sound like a good idea. PPC is quite special in the way it
uses host-model and host-passthrough. But anyway, it's actually easier
to just return IDENTICAL for host-passthrough CPUs (and all other guest
CPUs) than copying the host CPU from capabilities and comparing it to
itself.

> (this is because when a VM is in host-passthrough mode, its' CPU xml

> reflects the original host CPU

> definition, and we actually compare the source host and destination host

> CPU definitions,


The CPU definition of a domain with host-passthrough remains the same.
That is, it doesn't reflect the original host CPU definition, it's still
just host-passthrough. The only way to compare source and destinations
host CPUs is by taking the host CPU def from one host and passing it to
cpu-compare called on the other host.

Jirka
Zhenyu Zheng Sept. 7, 2020, 12:15 p.m. UTC | #7
So the suitable way of doing this will be checking for `VIR_CPU_TYPE_HOST`
and perform a comparison in this case and still return IDENTICAL for all
other
cases right? Then the upper layer caller(like OpenStack Nova) will have to
pass source host cpu info as a parameter.

BR,

Zhenyu


On Mon, Sep 7, 2020 at 7:16 PM Jiri Denemark <jdenemar@redhat.com> wrote:

> On Mon, Sep 07, 2020 at 09:21:02 +0800, Zhenyu Zheng wrote:

> > Thanks alot for the reply,

> >

> > This sounds like a valid use case (not sure it is that useful), but we

> > > need to be careful. But we should make sure implementing this does not

> > > break anything. This means, we need to do different things depending on

> > > the type of the CPU definition we are asked to compare. Guest CPU

> > > definitions should keep the old behaviour (return IDENTICAL) and host

> > > CPU definitions can be compared to the host CPU. But when doing so,

> > > don't get too influenced by x86 code, which I believe is way too

> > > complicated for ARM. Specifically you don't need to create armCompute

> > > and mess with guestData and other stuff there as all you want to do is

> > > compare the two CPU definitions. In x86 the same function is used for

> > > several things, but that's not the case for ARM.

> >

> >

> > for this, what I have in mind now is that we can check

> > `VIR_CPU_MODE_HOST_PASSTHROUGH`

> > and if that is the case, we compare CPU vendors and models to allow only

> > identical definitions to pass, like the implementation of

> >

> https://gitlab.com/libvirt/libvirt/-/blob/master/src/cpu/cpu_ppc64.c#L505

> ,

>

> This doesn't sound like a good idea. PPC is quite special in the way it

> uses host-model and host-passthrough. But anyway, it's actually easier

> to just return IDENTICAL for host-passthrough CPUs (and all other guest

> CPUs) than copying the host CPU from capabilities and comparing it to

> itself.

>

> > (this is because when a VM is in host-passthrough mode, its' CPU xml

> > reflects the original host CPU

> > definition, and we actually compare the source host and destination host

> > CPU definitions,

>

> The CPU definition of a domain with host-passthrough remains the same.

> That is, it doesn't reflect the original host CPU definition, it's still

> just host-passthrough. The only way to compare source and destinations

> host CPUs is by taking the host CPU def from one host and passing it to

> cpu-compare called on the other host.

>

> Jirka

>

>
<div dir="ltr">So the suitable way of doing this will be checking for `VIR_CPU_TYPE_HOST`<div>and perform a comparison in this case and still return IDENTICAL for all other</div><div>cases right? Then the upper layer caller(like OpenStack Nova) will have to</div><div>pass source host cpu info as a parameter. </div><div><br></div><div>BR,</div><div><br></div><div>Zhenyu<br><div><div><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 7, 2020 at 7:16 PM Jiri Denemark &lt;<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Sep 07, 2020 at 09:21:02 +0800, Zhenyu Zheng wrote:<br>
&gt; Thanks alot for the reply,<br>
&gt; <br>
&gt; This sounds like a valid use case (not sure it is that useful), but we<br>
&gt; &gt; need to be careful. But we should make sure implementing this does not<br>
&gt; &gt; break anything. This means, we need to do different things depending on<br>
&gt; &gt; the type of the CPU definition we are asked to compare. Guest CPU<br>
&gt; &gt; definitions should keep the old behaviour (return IDENTICAL) and host<br>
&gt; &gt; CPU definitions can be compared to the host CPU. But when doing so,<br>
&gt; &gt; don&#39;t get too influenced by x86 code, which I believe is way too<br>
&gt; &gt; complicated for ARM. Specifically you don&#39;t need to create armCompute<br>
&gt; &gt; and mess with guestData and other stuff there as all you want to do is<br>
&gt; &gt; compare the two CPU definitions. In x86 the same function is used for<br>
&gt; &gt; several things, but that&#39;s not the case for ARM.<br>
&gt; <br>
&gt; <br>
&gt; for this, what I have in mind now is that we can check<br>
&gt; `VIR_CPU_MODE_HOST_PASSTHROUGH`<br>
&gt; and if that is the case, we compare CPU vendors and models to allow only<br>
&gt; identical definitions to pass, like the implementation of<br>
&gt; <a href="https://gitlab.com/libvirt/libvirt/-/blob/master/src/cpu/cpu_ppc64.c#L505" rel="noreferrer" target="_blank">https://gitlab.com/libvirt/libvirt/-/blob/master/src/cpu/cpu_ppc64.c#L505</a> ,<br>
<br>
This doesn&#39;t sound like a good idea. PPC is quite special in the way it<br>
uses host-model and host-passthrough. But anyway, it&#39;s actually easier<br>
to just return IDENTICAL for host-passthrough CPUs (and all other guest<br>
CPUs) than copying the host CPU from capabilities and comparing it to<br>
itself.<br>
<br>
&gt; (this is because when a VM is in host-passthrough mode, its&#39; CPU xml<br>
&gt; reflects the original host CPU<br>
&gt; definition, and we actually compare the source host and destination host<br>
&gt; CPU definitions,<br>
<br>
The CPU definition of a domain with host-passthrough remains the same.<br>
That is, it doesn&#39;t reflect the original host CPU definition, it&#39;s still<br>
just host-passthrough. The only way to compare source and destinations<br>
host CPUs is by taking the host CPU def from one host and passing it to<br>
cpu-compare called on the other host.<br>
<br>
Jirka<br>
<br>
</blockquote></div>
Jiri Denemark Sept. 18, 2020, 12:01 p.m. UTC | #8
On Mon, Sep 07, 2020 at 20:15:59 +0800, Zhenyu Zheng wrote:
> So the suitable way of doing this will be checking for `VIR_CPU_TYPE_HOST`

> and perform a comparison in this case and still return IDENTICAL for all

> other cases right?


Yes.

> Then the upper layer caller(like OpenStack Nova) will have to pass

> source host cpu info as a parameter.


Right. That's what they would have to do even for x86.

Jirka
Zhenyu Zheng Sept. 19, 2020, 1:13 a.m. UTC | #9
Thanks, I've updated the codes, please have a look :), the topic of the
mail is [PATCH V3] Modify virCPUarmCompare to perform compare actions

BR,

Zhenyu

On Fri, Sep 18, 2020 at 8:01 PM Jiri Denemark <jdenemar@redhat.com> wrote:

> On Mon, Sep 07, 2020 at 20:15:59 +0800, Zhenyu Zheng wrote:

> > So the suitable way of doing this will be checking for

> `VIR_CPU_TYPE_HOST`

> > and perform a comparison in this case and still return IDENTICAL for all

> > other cases right?

>

> Yes.

>

> > Then the upper layer caller(like OpenStack Nova) will have to pass

> > source host cpu info as a parameter.

>

> Right. That's what they would have to do even for x86.

>

> Jirka

>
<div dir="ltr">Thanks, I&#39;ve updated the codes, please have a look :), the topic of the mail is [PATCH V3] Modify virCPUarmCompare to perform compare actions<br><div><br></div><div>BR,</div><div><br></div><div>Zhenyu</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 18, 2020 at 8:01 PM Jiri Denemark &lt;<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Sep 07, 2020 at 20:15:59 +0800, Zhenyu Zheng wrote:<br>
&gt; So the suitable way of doing this will be checking for `VIR_CPU_TYPE_HOST`<br>
&gt; and perform a comparison in this case and still return IDENTICAL for all<br>
&gt; other cases right?<br>
<br>
Yes.<br>
<br>
&gt; Then the upper layer caller(like OpenStack Nova) will have to pass<br>
&gt; source host cpu info as a parameter.<br>
<br>
Right. That&#39;s what they would have to do even for x86.<br>
<br>
Jirka<br>
</blockquote></div>
diff mbox series

Patch

diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
index 374a4d6f6c..98c5e551f5 100644
--- a/src/cpu/cpu_arm.c
+++ b/src/cpu/cpu_arm.c
@@ -118,6 +118,36 @@  virCPUarmMapNew(void)
     return map;
 }
 
+static int
+virCPUarmDataCopy(virCPUarmData *dst, const virCPUarmData *src)
+{
+    if (VIR_ALLOC(dst->features) < 0)
+        return -1;
+
+    dst->pvr = src->pvr;
+    dst->vendor_id = src->vendor_id;
+    dst->features = src->features;
+
+    return 0;
+}
+
+static virCPUDataPtr
+virCPUarmMakeCPUData(virArch arch,
+                     virCPUarmData *data)
+{
+    virCPUDataPtr cpuData;
+
+    if (VIR_ALLOC(cpuData) < 0)
+        return NULL;
+
+    cpuData->arch = arch;
+
+    if (virCPUarmDataCopy(&cpuData->data.arm, data) < 0)
+        VIR_FREE(cpuData);
+
+    return cpuData;
+}
+
 static void
 virCPUarmDataClear(virCPUarmData *data)
 {
@@ -376,6 +406,42 @@  virCPUarmModelParse(xmlXPathContextPtr ctxt,
     return 0;
 }
 
+static virCPUarmModelPtr
+virCPUarmModelCopy(virCPUarmModelPtr model)
+{
+    virCPUarmModelPtr copy;
+
+    copy = g_new0(virCPUarmModel, 1);
+    copy->name = g_strdup(model->name);
+    virCPUarmDataCopy(&copy->data, &model->data);
+    copy->vendor = model->vendor;
+
+    return g_steal_pointer(&copy);
+}
+
+static virCPUarmModelPtr
+virCPUarmModelFromCPU(const virCPUDef *cpu,
+                      virCPUarmMapPtr map)
+{
+    g_autoptr(virCPUarmModel) model = NULL;
+
+    if (!cpu->model) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("no CPU model specified"));
+        return NULL;
+    }
+
+    if (!(model = virCPUarmModelFind(map, cpu->model))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("Unknown CPU model %s"), cpu->model);
+        return NULL;
+    }
+
+    model = virCPUarmModelCopy(model);
+
+    return g_steal_pointer(&model);
+}
+
 static virCPUarmMapPtr
 virCPUarmLoadMap(void)
 {
@@ -463,11 +529,125 @@  virCPUarmBaseline(virCPUDefPtr *cpus,
 }
 
 static virCPUCompareResult
-virCPUarmCompare(virCPUDefPtr host G_GNUC_UNUSED,
-                 virCPUDefPtr cpu G_GNUC_UNUSED,
-                 bool failMessages G_GNUC_UNUSED)
+armCompute(virCPUDefPtr host,
+           virCPUDefPtr cpu,
+           virCPUDataPtr *guestData,
+           char **message)
 {
-    return VIR_CPU_COMPARE_IDENTICAL;
+    virCPUarmMapPtr map = NULL;
+    virCPUarmModelPtr host_model = NULL;
+    virCPUarmModelPtr guest_model = NULL;
+    virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR;
+    virArch arch;
+    size_t i;
+
+    if (cpu->arch != VIR_ARCH_NONE) {
+        bool found = false;
+
+        for (i = 0; i < G_N_ELEMENTS(archs); i++) {
+            if (archs[i] == cpu->arch) {
+                found = true;
+                break;
+            }
+        }
+
+        if (!found) {
+            VIR_DEBUG("CPU arch %s does not match host arch",
+                      virArchToString(cpu->arch));
+            if (message)
+                *message = g_strdup_printf(_("CPU arch %s does not match host arch"),
+                                           virArchToString(cpu->arch));
+
+            ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+            goto cleanup;
+        }
+        arch = cpu->arch;
+    } else {
+        arch = host->arch;
+    }
+
+    if (cpu->vendor &&
+        (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) {
+        VIR_DEBUG("host CPU vendor does not match required CPU vendor %s",
+                  cpu->vendor);
+        if (message) {
+            *message = g_strdup_printf(_("host CPU vendor does not match required "
+                                         "CPU vendor %s"),
+                                       cpu->vendor);
+        }
+
+        ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+        goto cleanup;
+    }
+
+    if (!(map = virCPUarmLoadMap()))
+        goto cleanup;
+
+    /* Host CPU information */
+    if (!(host_model = virCPUarmModelFromCPU(host, map)))
+        goto cleanup;
+
+    /* Guest CPU information */
+    if (!(guest_model = virCPUarmModelFromCPU(cpu, map)))
+        goto cleanup;
+
+    if (STRNEQ(guest_model->name, host_model->name)) {
+        VIR_DEBUG("host CPU model does not match required CPU model %s",
+                  guest_model->name);
+        if (message) {
+            *message = g_strdup_printf(_("host CPU model does not match required "
+                                         "CPU model %s"),
+                                       guest_model->name);
+        }
+
+        ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+        goto cleanup;
+    }
+
+    if (guestData)
+        if (!(*guestData = virCPUarmMakeCPUData(arch, &guest_model->data)))
+            goto cleanup;
+
+    ret = VIR_CPU_COMPARE_IDENTICAL;
+
+ cleanup:
+    virCPUarmModelFree(host_model);
+    virCPUarmModelFree(guest_model);
+    return ret;
+}
+
+static virCPUCompareResult
+virCPUarmCompare(virCPUDefPtr host,
+                 virCPUDefPtr cpu,
+                 bool failIncompatible)
+{
+        virCPUCompareResult ret;
+    char *message = NULL;
+
+    if (!host || !host->model) {
+        if (failIncompatible) {
+            virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s",
+                           _("unknown host CPU"));
+        } else {
+            VIR_WARN("unknown host CPU");
+            ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+        }
+        return -1;
+    }
+
+    ret = armCompute(host, cpu, NULL, &message);
+
+    if (failIncompatible && ret == VIR_CPU_COMPARE_INCOMPATIBLE) {
+        ret = VIR_CPU_COMPARE_ERROR;
+        if (message) {
+            virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s", message);
+        } else {
+            virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL);
+        }
+    }
+    VIR_FREE(message);
+
+    return ret;
 }
 
 static int