Re: [PATCH V5 3/4] cpu: Introduce getHost support for ARM CPU driver

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

 



On Wed, May 13, 2020 at 18:48:34 +0800, Zhenyu Zheng wrote:
> Introduce getHost support for ARM CPU driver,
> read CPU vendor_id, part_id and flags from
> registers directly. These codes will only be
> compiled on aarch64 hardware.
> 
> Signed-off-by: Zhenyu Zheng <zheng.zhenyu@xxxxxxxxxxx>
> ---
>  src/cpu/cpu_arm.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 162 insertions(+)
> 
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index c7a6abfb22..6aef568503 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -21,6 +21,10 @@
>   */
>  
>  #include <config.h>
> +#if defined(__aarch64__)
> +# include <asm/hwcap.h>
> +# include <sys/auxv.h>
> +#endif
>  
>  #include "viralloc.h"
>  #include "cpu.h"
> @@ -31,6 +35,13 @@
>  #include "virxml.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_CPU
> +#if defined(__aarch64__)
> +/* Shift bit mask for parsing cpu flags */
> +# define BIT_SHIFTS(n) (1UL << (n))
> +/* The current max number of cpu flags on ARM is 32 */
> +# define MAX_CPU_FLAGS 32
> +#endif
> +
>  
>  VIR_LOG_INIT("cpu.cpu_arm");
>  
> @@ -486,11 +497,162 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu)
>      return 0;
>  }
>  
> +#if defined(__aarch64__)
> +/* Generate human readable flag list according to the order of */
> +/* AT_HWCAP bit map */
> +const char *aarch64_cpu_flags[MAX_CPU_FLAGS] = {
> +    "fp", "asimd", "evtstrm", "aes", "pmull", "sha1", "sha2",
> +    "crc32", "atomics", "fphp", "asimdhp", "cpuid", "asimdrdm",
> +    "jscvt", "fcma", "lrcpc", "dcpop", "sha3", "sm3", "sm4",
> +    "asimddp", "sha512", "sve", "asimdfhm", "dit", "uscat",
> +    "ilrcpc", "flagm", "ssbs", "sb", "paca", "pacg"};
> +/**
> + * virCPUarmCpuDataFromRegs:
> + *
> + * @data: 64-bit arm CPU specific data
> + *
> + * Fetches CPU vendor_id and part_id from MIDR_EL1 register, parse CPU
> + * flags from AT_HWCAP. There are currently 32 valid flags  on ARM arch
> + * represented by each bit.
> + */
> +static int
> +virCPUarmCpuDataFromRegs(virCPUarmData *data)
> +{
> +    unsigned long cpuid;
> +    unsigned long hwcaps;
> +    VIR_AUTOSTRINGLIST features = NULL;
> +    int cpu_feature_index = 0;
> +    size_t i;
> +
> +    if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("CPUID registers unavailable"));
> +            return -1;
> +    }
> +
> +    /* read the cpuid data from MIDR_EL1 register */
> +    asm("mrs %0, MIDR_EL1" : "=r" (cpuid));
> +    VIR_DEBUG("CPUID read from register:  0x%016lx", cpuid);
> +
> +    /* parse the coresponding part_id bits */
> +    data->pvr = (cpuid >> 4) & 0xfff;
> +    /* parse the coresponding vendor_id bits */
> +    data->vendor_id = (cpuid >> 24) & 0xff;
> +
> +    hwcaps = getauxval(AT_HWCAP);
> +    VIR_DEBUG("CPU flags read from register:  0x%016lx", hwcaps);
> +
> +    features = g_new0(char *, MAX_CPU_FLAGS + 1);
> +
> +    /* shift bit map mask to parse for CPU flags */
> +    for (i = 0; i < MAX_CPU_FLAGS; i++) {
> +        if (hwcaps & BIT_SHIFTS(i)) {
> +            features[cpu_feature_index] = g_strdup(aarch64_cpu_flags[i]);
> +            cpu_feature_index++;
> +        }
> +    }
> +
> +    if (cpu_feature_index > 0) {
> +        data->features = g_steal_pointer(&features);
> +        data->nfeatures = cpu_feature_index;

There's no need for data->nfeatures as data->features is
NULL-terminated.

> +    }
> +
> +    return 0;
> +}
> +
> +static int
> +virCPUarmDecode(virCPUDefPtr cpu,
> +                const virCPUarmData *cpuData,
> +                virDomainCapsCPUModelsPtr models)
> +{
> +    size_t i;
> +    virCPUarmMapPtr map;
> +    virCPUarmModelPtr model;
> +    virCPUarmVendorPtr vendor = NULL;
> +
> +    if (!cpuData || !(map = virCPUarmGetMap()))
> +        return -1;
> +
> +    if (!(model = virCPUarmModelFindByPVR(map, cpuData->pvr))) {
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("Cannot find CPU model with PVR 0x%03lx"),
> +                       cpuData->pvr);
> +        return -1;
> +    }
> +
> +    if (!virCPUModelIsAllowed(model->name, models)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("CPU model %s is not supported by hypervisor"),
> +                       model->name);
> +        return -1;
> +    }
> +
> +    cpu->model = g_strdup(model->name);
> +
> +    if (cpuData->vendor_id &&
> +        !(vendor = virCPUarmVendorFindByID(map, cpuData->vendor_id))) {
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("Cannot find CPU vendor with vendor id 0x%02lx"),
> +                       cpuData->vendor_id);
> +        return -1;
> +    }
> +
> +    if (vendor)
> +        cpu->vendor = g_strdup(vendor->name);
> +
> +    if (cpuData->nfeatures) {
> +        cpu->nfeatures = cpuData->nfeatures;
> +        if (VIR_ALLOC_N(cpu->features, cpu->nfeatures) < 0)
> +            goto error;

Even better (and without cpuData->nfeatures):

    if (cpuData->features) {
        cpu->nfeatures = virStringListLength((const char **)cpuData->features);
        cpu->features = g_new0(virCPUFeatureDef, cpu->nfeatures);

> +        for (i = 0; i < cpu->nfeatures; i++) {
> +            cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
> +            cpu->features[i].name = g_strdup(cpuData->features[i]);
> +        }
> +    }
> +
> +    return 0;
> +
> + error:
> +    for (i = 0; i < cpu->nfeatures; i++)
> +        VIR_FREE(cpu->features[i].name);
> +    VIR_FREE(cpu->features);
> +    cpu->nfeatures = 0;
> +    return -1;

The only way we could get here is when allocating cpu->features fails,
which means there's nothing to cleanup. Anyway, we can avoid the only
goto statement by using g_new0 and drop this section entirely.

> +}
> +
> +static int
> +virCPUarmGetHost(virCPUDefPtr cpu,
> +                 virDomainCapsCPUModelsPtr models)
> +{
> +    virCPUDataPtr cpuData = NULL;

I forgot to mention this in my previous review, but this function would
benefit from using g_autoptr as we could just drop the cleanup section
and return directly in call cases:

    g_autoptr(virCPUData) cpuData = NULL;

> +    int ret = -1;

Not needed anymore.

> +
> +    if (virCPUarmDriverInitialize() < 0)
> +        goto cleanup;

return -1;

> +
> +    if (!(cpuData = virCPUDataNew(archs[0])))
> +        goto cleanup;

return -1;

> +
> +    if (virCPUarmCpuDataFromRegs(&cpuData->data.arm) < 0)
> +        goto cleanup;

return -1;

> +
> +    ret = virCPUarmDecode(cpu, &cpuData->data.arm, models);

return virCPUarmDecode(cpu, &cpuData->data.arm, models);

> +
> + cleanup:
> +    virCPUarmDataFree(cpuData);
> +    return ret;
> +}

Not needed anymore.

> +#endif
> +
> +
>  struct cpuArchDriver cpuDriverArm = {
>      .name = "arm",
>      .arch = archs,
>      .narch = G_N_ELEMENTS(archs),
>      .compare = virCPUarmCompare,
> +#if defined(__aarch64__)
> +    .getHost = virCPUarmGetHost,
> +#endif
>      .decode = NULL,
>      .encode = NULL,
>      .dataFree = virCPUarmDataFree,

Jirka




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux