| Message ID | OSYP286MB0181C4EA8A2E1DBCF0F0D457995B0@OSYP286MB0181.JPNP286.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | Modify virCPUarmCompare to perform compare actions | expand |
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(©->data, &model->data); > + copy->vendor = model->vendor; > + > + return g_steal_pointer(©); 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 >
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
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 <<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>> 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> > Modify virCPUarmCompare in cpu_arm.c to perform<br> > actual compare actions. Compare host cpu vendor<br> > and model info with guest cpu as initial implementation,<br> > 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'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'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>
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
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't get too influenced by x86 code, which I believe is way too<br>complicated for ARM. Specifically you don'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'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' 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 <<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>> 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> > Hi,<br> > <br> > Thanks alot for the review and feedback. As for host-passthrough cases, I<br> > have some other understandings,<br> > if I understand correctly, what you mean is that if a vm uses<br> > host-passthrough, it can migrate to any other<br> > host, since it asks for host-passthrough. In my point of view, I think in<br> > real cases, there are few different kinds<br> > of ARM datacenter CPUs on the market, and there CPU capabilities are<br> > different, so one might create a vm on<br> > hostA with feature1 and feature2 because it uses host-passthrough and hostA<br> > has these features. Now in your<br> > definition(if I understand correctly) of host-passthrough and the current<br> > code(returns identical directly), this vm<br> > can be migrated to hostB with only feature1, since there are no<br> > limitations. If one has some important application<br> > that depends on feature2, the app will break as feature2 is not available<br> > on hostB.<br> <br> And that'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'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'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'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't get too influenced by x86 code, which I believe is way too<br> complicated for ARM. Specifically you don'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's not the case for ARM.<br> <br> > Considering this, I proposed to add basic checks to compare CPU to<br> > limit the migration to only the same CPU models. And once the<br> > capability of ARM driver is enhanced in QEMU or other related<br> > 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>
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
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 <<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>> 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> > Thanks alot for the reply,<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't get too influenced by x86 code, which I believe is way too<br> > > complicated for ARM. Specifically you don'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's not the case for ARM.<br> > <br> > <br> > for this, what I have in mind now is that we can check<br> > `VIR_CPU_MODE_HOST_PASSTHROUGH`<br> > and if that is the case, we compare CPU vendors and models to allow only<br> > identical definitions to pass, like the implementation of<br> > <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't sound like a good idea. PPC is quite special in the way it<br> uses host-model and host-passthrough. But anyway, it'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> > (this is because when a VM is in host-passthrough mode, its' CPU xml<br> > reflects the original host CPU<br> > definition, and we actually compare the source host and destination host<br> > CPU definitions,<br> <br> The CPU definition of a domain with host-passthrough remains the same.<br> That is, it doesn't reflect the original host CPU definition, it'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>
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
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'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 <<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>> 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> > So the suitable way of doing this will be checking for `VIR_CPU_TYPE_HOST`<br> > and perform a comparison in this case and still return IDENTICAL for all<br> > other cases right?<br> <br> Yes.<br> <br> > Then the upper layer caller(like OpenStack Nova) will have to pass<br> > source host cpu info as a parameter.<br> <br> Right. That's what they would have to do even for x86.<br> <br> Jirka<br> </blockquote></div>
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(©->data, &model->data); + copy->vendor = model->vendor; + + return g_steal_pointer(©); +} + +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
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