Re: [PATCH V2 5/5] cpu: Introduce getHost support for ARM CPU driver

Hi, Thanks alot for all the reviews, I'm updating the codes, as for those headers, hwcap.h is here: https://github.com/torvalds/linux/blob/v5.6/arch/arm/include/asm/hwcap.h and should be available for 5.6, auxv.h is from glibc: https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/sys/auxv.h;h=1a563e... yeah, understandable, I just don't see other archs like ppc and s390 doing this so I didn't do it. BR, On Thu, Apr 9, 2020 at 7:03 PM Jiri Denemark <jdenemar(a)redhat.com&gt; wrote: > On Thu, Apr 02, 2020 at 17:03:59 +0800, Zhenyu Zheng wrote: > > Introduce getHost support for ARM CPU driver, read > > CPU vendor_id, part_id and flags from registers > > directly. > > > > Signed-off-by: Zhenyu Zheng <zhengzhenyulixi(a)gmail.com&gt; > > --- > > src/cpu/cpu_arm.c | 194 +++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 193 insertions(+), 1 deletion(-) > > In addition to all the coding style issues which Daniel pointed out... > > > diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c > > index 88b4d91946..c8f5ce7e8a 100644 > > --- a/src/cpu/cpu_arm.c > > +++ b/src/cpu/cpu_arm.c > > @@ -21,6 +21,8 @@ > > */ > > > > #include <config.h> > > +#include <asm/hwcap.h> > > +#include <sys/auxv.h> > > None of these header files exist on my system with linux 5.6 headers. > > > #include "viralloc.h" > > #include "cpu.h" > > @@ -31,6 +33,10 @@ > > #include "virxml.h" > > > > #define VIR_FROM_THIS VIR_FROM_CPU > > +/* 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 > > > > VIR_LOG_INIT("cpu.cpu_arm"); > > > > @@ -498,14 +504,200 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu) > > return 0; > > } > > > > +/** > > + * armCpuDataFromRegs: > > + * > > + * @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 > > +armCpuDataFromRegs(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 would expect these to be defined in src/cpu_map/arm_features.xml, > although I'm not sure how well the new features would play with the > existing ones... What do you think, Andrea? > > > + unsigned long cpuid, hwcaps; > > + char **features = NULL; > > + 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; > > + } > > + > > + /* 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); > > + > > + if (VIR_ALLOC_N(features, MAX_CPU_FLAGS) < 0) > > + return -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(flag_list[i]); > > + cpu_feature_index++; > > + } > > + } > > + > > + if (cpu_feature_index > 1) { > > + cpu_feature_str = virStringListJoin((const char **)features, " > "); > > + if (!cpu_feature_str) > > + goto cleanup; > > + } > > + data->features = g_strdup(cpu_feature_str); > > + > > + return 0; > > + > > + cleanup: > > + virStringListFree(features); > > + VIR_FREE(cpu_feature_str); > > + return -1; > > +} > > + > > +static int > > +armCpuDataParseFeatures(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; > > + 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]); > > + } > > + } > > + > > + 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 > > +armDecode(virCPUDefPtr cpu, > > + const virCPUarmData *cpuData, > > + virDomainCapsCPUModelsPtr models) > > +{ > > + virCPUarmMapPtr map; > > + virCPUarmModelPtr model; > > + virCPUarmVendorPtr vendor = NULL; > > + > > + if (!cpuData || !(map = virCPUarmGetMap())) > > + return -1; > > + > > + if (!(model = armModelFindByPVR(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 = armVendorFindByID(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 && > > + armCpuDataParseFeatures(cpu, cpuData) < 0) > > + return -1; > > + > > + return 0; > > +} > > + > > +static int > > +armDecodeCPUData(virCPUDefPtr cpu, > > + const virCPUData *data, > > + virDomainCapsCPUModelsPtr models) > > +{ > > + return armDecode(cpu, &data->data.arm, models); > > +} > > + > > +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 (armCpuDataFromRegs(&cpuData->data.arm) < 0) > > + goto cleanup; > > + > > + ret = armDecodeCPUData(cpu, cpuData, models); > > + > > + cleanup: > > + virCPUarmDataFree(cpuData); > > + return ret; > > +} > > > Jirka > >