[libvirt] [PATCH 1/3] CPU: Implement guestData for PPC CPU driver
Li Zhang
zhlcindy at gmail.com
Mon Sep 2 06:02:44 UTC 2013
On 2013年09月02日 12:08, Doug Goldstein wrote:
> On Thu, Aug 29, 2013 at 3:46 AM, Li Zhang <zhlcindy at gmail.com
> <mailto:zhlcindy at gmail.com>> wrote:
>
> From: Li Zhang <zhlcindy at linux.vnet.ibm.com
> <mailto:zhlcindy at linux.vnet.ibm.com>>
>
> On Power platform, Power7+ can support Power7 guest.
> It needs to define XML configuration to specify guest's CPU model.
>
> For exmaple:
> <cpu match='exact'>
> <model>POWER7+_v2.1</model>
> <vendor>IBM</vendor>
> </cpu>
>
> Signed-off-by: Li Zhang <zhlcindy at linux.vnet.ibm.com
> <mailto:zhlcindy at linux.vnet.ibm.com>>
> ---
> src/cpu/cpu_powerpc.c | 166
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 164 insertions(+), 2 deletions(-)
>
> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
> index 647a8a1..84fa3f7 100644
> --- a/src/cpu/cpu_powerpc.c
> +++ b/src/cpu/cpu_powerpc.c
> @@ -99,6 +99,23 @@ ppcModelFindPVR(const struct ppc_map *map,
> return NULL;
> }
>
> +static struct ppc_model *
> +ppcModelCopy(const struct ppc_model *model)
> +{
> + struct ppc_model *copy;
> +
> + if (VIR_ALLOC(copy) < 0 ||
> + VIR_STRDUP(copy->name, model->name) < 0) {
> + ppcModelFree(copy);
> + return NULL;
> + }
> +
> + copy->data.pvr = model->data.pvr;
> + copy->vendor = model->vendor;
> +
> + return copy;
> +}
> +
> static struct ppc_vendor *
> ppcVendorFind(const struct ppc_map *map,
> const char *name)
> @@ -126,6 +143,29 @@ ppcVendorFree(struct ppc_vendor *vendor)
> VIR_FREE(vendor);
> }
>
> +static struct ppc_model *
> +ppcModelFromCPU(const virCPUDefPtr cpu,
> + const struct ppc_map *map)
> +{
> + struct ppc_model *model = NULL;
> +
> + if ((model = ppcModelFind(map, cpu->model)) == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown CPU model %s"), cpu->model);
> + goto error;
> + }
> +
> + if ((model = ppcModelCopy(model)) == NULL)
> + goto error;
> +
> + return model;
> +
> +error:
> + ppcModelFree(model);
> + return NULL;
> +}
> +
> +
> static int
> ppcVendorLoad(xmlXPathContextPtr ctxt,
> struct ppc_map *map)
> @@ -288,6 +328,112 @@ error:
> return NULL;
> }
>
> +static virCPUDataPtr
> +ppcMakeCPUData(virArch arch, struct cpuPPCData *data)
> +{
> + virCPUDataPtr cpuData;
> +
> + if (VIR_ALLOC(cpuData) < 0)
> + return NULL;
> +
> + cpuData->arch = arch;
> + cpuData->data.ppc = *data;
> + data = NULL;
> +
> + return cpuData;
> +}
> +
> +static virCPUCompareResult
> +ppcCompute(virCPUDefPtr host,
> + const virCPUDefPtr cpu,
> + virCPUDataPtr *guestData,
> + char **message)
> +
> +{
> + struct ppc_map *map = NULL;
> + struct ppc_model *host_model = NULL;
> + struct ppc_model *guest_model = NULL;
> +
> + int ret = 0;
> + virArch arch;
> + size_t i;
> +
> + if (cpu->arch != VIR_ARCH_NONE) {
> + bool found = false;
> +
> + for (i = 0; i < ARRAY_CARDINALITY(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 &&
> + virAsprintf(message,
> + _("CPU arch %s does not match host
> arch"),
> + virArchToString(cpu->arch)) < 0)
> + goto error;
> + return VIR_CPU_COMPARE_INCOMPATIBLE;
> + }
> + 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 &&
> + virAsprintf(message,
> + _("host CPU vendor does not match required "
> + "CPU vendor %s"),
> + cpu->vendor) < 0)
> + goto error;
> + return VIR_CPU_COMPARE_INCOMPATIBLE;
> + }
>
>
> Could this beginning part of ppcCompute() not go into some common
> function in cpu_generic.c? It just looks entirely copied and pasted
> from cpu_x86.c from x86Compute()
CPU driver's functions can be called separately for different
architectures.
Doesn't it break the structure to let PPC driver go into some functions
in cpu_generic.c?
PPC's CPU logic is the same as X86.
The difference is that CPUID is not supported on PPC and it is removed.
> +
> + if (!(map = ppcLoadMap()) ||
> + !(host_model = ppcModelFromCPU(host, map)) ||
> + !(guest_model = ppcModelFromCPU(cpu, map)))
> + goto error;
> +
> + if (guestData != NULL) {
> + if (cpu->type == VIR_CPU_TYPE_GUEST &&
> + cpu->match == VIR_CPU_MATCH_STRICT &&
> + STRNEQ(guest_model->name, host_model->name)) {
> + VIR_DEBUG("host CPU model does not match required CPU
> model %s",
> + guest_model->name);
> + if (message &&
> + virAsprintf(message,
> + _("host CPU model does not match
> required "
> + "CPU model %s"),
> + guest_model->name) < 0)
> + goto error;
> + return VIR_CPU_COMPARE_INCOMPATIBLE;
> + }
> +
> + if (!(*guestData = ppcMakeCPUData(arch, &guest_model->data)))
> + goto error;
> + }
> +
> + ret = VIR_CPU_COMPARE_IDENTICAL;
> +
> +out:
>
>
> I don't see this label used anywhere.
It is in the following code:
> + ppcMapFree(map);
> + ppcModelFree(host_model);
> + ppcModelFree(guest_model);
> + return ret;
> +
> +error:
> + ret = VIR_CPU_COMPARE_ERROR;
> + goto out;
>
^^^
>
> +
> +}
> +
> static virCPUCompareResult
> ppcCompare(virCPUDefPtr host,
> virCPUDefPtr cpu)
> @@ -369,6 +515,15 @@ ppcNodeData(void)
> }
> #endif
>
> +static virCPUCompareResult
> +ppcGuestData(virCPUDefPtr host,
> + virCPUDefPtr guest,
> + virCPUDataPtr *data,
> + char **message)
> +{
> + return ppcCompute(host, guest, data, message);
> +}
> +
> static int
> ppcUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED,
> const virCPUDefPtr host ATTRIBUTE_UNUSED)
> @@ -466,6 +621,13 @@ error:
> goto cleanup;
> }
>
> +static int
> +ppcHasFeature(const virCPUDataPtr data ATTRIBUTE_UNUSED,
> + const char *name ATTRIBUTE_UNUSED)
> +{
> + return 0;
> +}
> +
>
>
> I'm really curious about this addition. I'm assuming you are hitting a
> check from qemu_command.c and I'm wondering which one because it
> likely seems that we need to fix that rather than just providing this
> dummy stub. That or this was left in and isn't really necessary.
Yes, you are right.
In the qemuBuildCpuArgStr, it calls this function.
hasSVM = cpuHasFeature(data, "svm");
That is X86 specific. I will remove it for PPC.
> struct cpuArchDriver cpuDriverPowerPC = {
> .name = "ppc64",
> .arch = archs,
> @@ -479,8 +641,8 @@ struct cpuArchDriver cpuDriverPowerPC = {
> #else
> .nodeData = NULL,
> #endif
> - .guestData = NULL,
> + .guestData = ppcGuestData,
> .baseline = ppcBaseline,
> .update = ppcUpdate,
> - .hasFeature = NULL,
> + .hasFeature = ppcHasFeature,
> };
> --
> 1.8.1.4
>
>
> I don't have PPC however so I can't actually test the code. But I did
> add a few comments above.
Thanks very much for your comments.
>
> --
> Doug Goldstein
More information about the libvir-list
mailing list