[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] CPU: Implement guestData for PPC CPU driver



On 2013年09月02日 12:08, Doug Goldstein wrote:
On Thu, Aug 29, 2013 at 3:46 AM, Li Zhang <zhlcindy gmail com <mailto:zhlcindy gmail com>> wrote:

    From: Li Zhang <zhlcindy linux vnet ibm com
    <mailto:zhlcindy 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 linux vnet ibm com
    <mailto:zhlcindy 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]