On Wed, Apr 22, 2020 at 15:11:24 +0800, ZhengZhenyu 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 hardwares.
>
> Signed-off-by: Zhenyu Zheng <zhengzhenyulixi@xxxxxxxxx>
> ---
> src/cpu/cpu_arm.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 204 insertions(+)
>
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 6e9ff9bf11..ec50a5615d 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"
> @@ -32,6 +36,15 @@
>
> #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");
> +
> static const virArch archs[] = {
> VIR_ARCH_ARMV6L,
> VIR_ARCH_ARMV7B,
> @@ -491,12 +504,203 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu)
> return 0;
> }
>
> +#if defined(__aarch64__)
> +/**
> + * 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)
> +{
> + /* Generate human readable flag list according to the order of */
> + /* AT_HWCAP bit map */
> + const char *flag_list[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"};
I'd move this array out of the function.
> + unsigned long cpuid, hwcaps;
One variable per line, please.
> + char **features = NULL;
VIR_AUTOSTRINGLIST features = NULL;
> + g_autofree char *cpu_feature_str = 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;
Wrong indentation.
> + }
> +
> + /* 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;
Please, add spaces around the operators and consider using () for
clarity. So, I guess:
data->pvr = (cpuid >>Â 4) & 0xfff;
> + /* parse the coresponding vendor_id bits */
> + data->vendor_id = cpuid>>24&0xFF;
data->vendor_id = (cpuid >>Â 24) & 0xff;
> +
> + hwcaps = getauxval(AT_HWCAP);
> + VIR_DEBUG("CPU flags read from register: 0x%016lx", hwcaps);
> +
> + if (VIR_ALLOC_N(features, MAX_CPU_FLAGS) < 0)
> + return -1;
The string list is supposed to be NULL-terminated so you need to
allocate space for MAX_CPU_FLAGS + 1:
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++) {
i < MAX_CPU_FLAGS
> + if (hwcaps & BIT_SHIFTS(i)) {
> + features[cpu_feature_index] = g_strdup(flag_list[i]);
> + cpu_feature_index++;
> + }
This could also be written as
if (hwcaps & BIT_SHIFTS(i))
features[cpu_feature_index++] = g_strdup(flag_list[i]);
it doesn't matter that much, though.
> + }
> +
> + if (cpu_feature_index > 1) {
This would not work for CPUs with exactly one feature.
> + cpu_feature_str = virStringListJoin((const char **)features, " ");
> + if (!cpu_feature_str)
> + goto error;
> + }
> + data->features = g_strdup(cpu_feature_str);
Just store the features string list in the data directly:
if (cpu_feature_index > 0)
data->features = g_steal_pointer(&features);
> +
> + return 0;
> +
> + error:
> + virStringListFree(features);
> + return -1;
This can be dropped thanks to VIR_AUTOSTRINGLIST.
> +}
> +
> +static int
> +virCPUarmDataParseFeatures(virCPUDefPtr cpu,
> + const virCPUarmData *cpuData)
> +{
> + int ret = -1;
> + size_t i;
> + char **features;
> +
> + if (!cpu || !cpuData)
> + return ret;
> +
> + if (!(features = virStringSplitCount(cpuData->features, " ",
> + 0, &cpu->nfeatures)))
> + return ret;
Please don't do this. The virCPUarmData can contain the feature list
directly.
> + if (cpu->nfeatures) {
> + if (VIR_ALLOC_N(cpu->features, cpu->nfeatures) < 0)
> + goto error;
> +
> + for (i = 0; i < cpu->nfeatures; i++) {
> + cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
> + cpu->features[i].name = g_strdup(features[i]);
> + }
> + }
I'd move this loop to virCPUarmDecode as this function name is pretty
confusing and there won't be much more than this loop in it when the
feature list is stored directly in virCPUarmData.
> +
> + ret = 0;
> +
> + cleanup:
> + virStringListFree(features);
> + return ret;
> +
> + error:
> + for (i = 0; i < cpu->nfeatures; i++)
> + VIR_FREE(cpu->features[i].name);
> + VIR_FREE(cpu->features);
> + cpu->nfeatures = 0;
> + goto cleanup;
> +}
> +
> +static int
> +virCPUarmDecode(virCPUDefPtr cpu,
> + const virCPUarmData *cpuData,
> + virDomainCapsCPUModelsPtr models)
> +{
> + 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->features &&
> + virCPUarmDataParseFeatures(cpu, cpuData) < 0)
> + return -1;
Hmm, the code seems to be inconsistent with the cpu_map definitions:
- Either you define CPU models without any features and add all the
detected features to the host CPU definition (this is what your
current code does).
- Or you define CPU models with features and only add extra features
(those not included in the model definition) to the host CPU
definition (this is what the cpu_map changes suggest). In this case
you also need to implement virCPUExpandFeatures API for the arm driver
to make sure one can get both the base and extra features.
Personally, I think the first option is both simpler and better for
reporting host CPU model and features.
> +
> + return 0;
> +}
> +
> +static int
> +virCPUarmDecodeCPUData(virCPUDefPtr cpu,
> + const virCPUData *data,
> + virDomainCapsCPUModelsPtr models)
> +{
> + return virCPUarmDecode(cpu, &data->data.arm, models);
> +}
This function is not needed.
> +
> +static int
> +virCPUarmGetHost(virCPUDefPtr cpu,
> + virDomainCapsCPUModelsPtr models)
> +{
> + virCPUDataPtr cpuData = NULL;
> + int ret = -1;
> +
> + if (virCPUarmDriverInitialize() < 0)
> + goto cleanup;
> +
> + if (!(cpuData = virCPUDataNew(archs[0])))
> + goto cleanup;
> +
> + if (virCPUarmCpuDataFromRegs(&cpuData->data.arm) < 0)
> + goto cleanup;
> +
> + ret = virCPUarmDecodeCPUData(cpu, cpuData, models);
You can call virCPUarmDecode directly here.
> +
> + cleanup:
> + virCPUarmDataFree(cpuData);
> + return ret;
> +}
> +#endif
> +
> struct cpuArchDriver cpuDriverArm = {
> .name = "arm",
> .arch = archs,
> .narch = G_N_ELEMENTS(archs),
> .compare = virCPUarmCompare,
> +#if defined(__aarch64__)
> + .getHost = virCPUarmGetHost,
> + .decode = virCPUarmDecodeCPUData,
> +#else
> .decode = NULL,
> +#endif
Just
#if defined(__aarch64__)
.getHost = virCPUarmGetHost,
#endif
is enough, there should be no need to initialize .decode to implement
host CPU detection.
> .encode = NULL,
> .dataFree = virCPUarmDataFree,
> .baseline = virCPUarmBaseline,
Jirka
