<div dir="ltr">Hi,<div><br></div><div>Thanks alot for all the reviews, I'm updating the codes, as for those headers,</div><div>hwcap.h is here: <a href="https://github.com/torvalds/linux/blob/v5.6/arch/arm/include/asm/hwcap.h">https://github.com/torvalds/linux/blob/v5.6/arch/arm/include/asm/hwcap.h</a> and should be available for 5.6,</div><div>auxv.h is from glibc: <a href="https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/sys/auxv.h;h=1a563e1337e64e4218c1613b68dc2551a762ba00;hb=HEAD">https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/sys/auxv.h;h=1a563e1337e64e4218c1613b68dc2551a762ba00;hb=HEAD</a></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">virCPUarmGetHost and the helpers it calls are architecture specific and<br>should not be even compiled in on non-ARM. </blockquote><div> yeah, understandable, I just don't see other archs like ppc and s390 doing this so I didn't do it. </div><div><br></div><div>BR,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 9, 2020 at 7:03 PM Jiri Denemark <<a href="mailto:jdenemar@redhat.com">jdenemar@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Apr 02, 2020 at 17:03:59 +0800, Zhenyu Zheng wrote:<br>
> Introduce getHost support for ARM CPU driver, read<br>
> CPU vendor_id, part_id and flags from registers<br>
> directly.<br>
> <br>
> Signed-off-by: Zhenyu Zheng <<a href="mailto:zhengzhenyulixi@gmail.com" target="_blank">zhengzhenyulixi@gmail.com</a>><br>
> ---<br>
> src/cpu/cpu_arm.c | 194 +++++++++++++++++++++++++++++++++++++++++++++-<br>
> 1 file changed, 193 insertions(+), 1 deletion(-)<br>
<br>
In addition to all the coding style issues which Daniel pointed out...<br>
<br>
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c<br>
> index 88b4d91946..c8f5ce7e8a 100644<br>
> --- a/src/cpu/cpu_arm.c<br>
> +++ b/src/cpu/cpu_arm.c<br>
> @@ -21,6 +21,8 @@<br>
> */<br>
> <br>
> #include <config.h><br>
> +#include <asm/hwcap.h><br>
> +#include <sys/auxv.h><br>
<br>
None of these header files exist on my system with linux 5.6 headers.<br>
<br>
> #include "viralloc.h"<br>
> #include "cpu.h"<br>
> @@ -31,6 +33,10 @@<br>
> #include "virxml.h"<br>
> <br>
> #define VIR_FROM_THIS VIR_FROM_CPU<br>
> +/* Shift bit mask for parsing cpu flags */<br>
> +#define BIT_SHIFTS(n) (1UL << (n))<br>
> +/* The current max number of cpu flags on ARM is 32 */<br>
> +#define MAX_CPU_FLAGS 32<br>
> <br>
> VIR_LOG_INIT("cpu.cpu_arm");<br>
> <br>
> @@ -498,14 +504,200 @@ virCPUarmValidateFeatures(virCPUDefPtr cpu)<br>
> return 0;<br>
> }<br>
> <br>
> +/**<br>
> + * armCpuDataFromRegs:<br>
> + *<br>
> + * @data: 64-bit arm CPU specific data<br>
> + *<br>
> + * Fetches CPU vendor_id and part_id from MIDR_EL1 register, parse CPU<br>
> + * flags from AT_HWCAP. There are currently 32 valid flags on ARM arch<br>
> + * represented by each bit.<br>
> + */<br>
> +static int<br>
> +armCpuDataFromRegs(virCPUarmData *data)<br>
> +{<br>
> + /* Generate human readable flag list according to the order of */<br>
> + /* AT_HWCAP bit map */<br>
> + const char *flag_list[MAX_CPU_FLAGS] = {<br>
> + "fp", "asimd", "evtstrm", "aes", "pmull", "sha1", "sha2",<br>
> + "crc32", "atomics", "fphp", "asimdhp", "cpuid", "asimdrdm",<br>
> + "jscvt", "fcma", "lrcpc", "dcpop", "sha3", "sm3", "sm4",<br>
> + "asimddp", "sha512", "sve", "asimdfhm", "dit", "uscat",<br>
> + "ilrcpc", "flagm", "ssbs", "sb", "paca", "pacg"};<br>
<br>
I would expect these to be defined in src/cpu_map/arm_features.xml,<br>
although I'm not sure how well the new features would play with the<br>
existing ones... What do you think, Andrea?<br>
<br>
> + unsigned long cpuid, hwcaps;<br>
> + char **features = NULL;<br>
> + char *cpu_feature_str = NULL;<br>
> + int cpu_feature_index = 0;<br>
> + size_t i;<br>
> +<br>
> + if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {<br>
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",<br>
> + _("CPUID registers unavailable"));<br>
> + return -1;<br>
> + }<br>
> +<br>
> + /* read the cpuid data from MIDR_EL1 register */<br>
> + asm("mrs %0, MIDR_EL1" : "=r" (cpuid));<br>
> + VIR_DEBUG("CPUID read from register: 0x%016lx", cpuid);<br>
> +<br>
> + /* parse the coresponding part_id bits */<br>
> + data->pvr = cpuid>>4&0xFFF;<br>
> + /* parse the coresponding vendor_id bits */<br>
> + data->vendor_id = cpuid>>24&0xFF;<br>
> +<br>
> + hwcaps = getauxval(AT_HWCAP);<br>
> + VIR_DEBUG("CPU flags read from register: 0x%016lx", hwcaps);<br>
> +<br>
> + if (VIR_ALLOC_N(features, MAX_CPU_FLAGS) < 0)<br>
> + return -1;<br>
> +<br>
> + /* shift bit map mask to parse for CPU flags */<br>
> + for (i = 0; i< MAX_CPU_FLAGS; i++) {<br>
> + if (hwcaps & BIT_SHIFTS(i)) {<br>
> + features[cpu_feature_index] = g_strdup(flag_list[i]);<br>
> + cpu_feature_index++;<br>
> + }<br>
> + }<br>
> +<br>
> + if (cpu_feature_index > 1) {<br>
> + cpu_feature_str = virStringListJoin((const char **)features, " ");<br>
> + if (!cpu_feature_str)<br>
> + goto cleanup;<br>
> + }<br>
> + data->features = g_strdup(cpu_feature_str);<br>
> +<br>
> + return 0;<br>
> +<br>
> + cleanup:<br>
> + virStringListFree(features);<br>
> + VIR_FREE(cpu_feature_str);<br>
> + return -1;<br>
> +}<br>
> +<br>
> +static int<br>
> +armCpuDataParseFeatures(virCPUDefPtr cpu,<br>
> + const virCPUarmData *cpuData)<br>
> +{<br>
> + int ret = -1;<br>
> + size_t i;<br>
> + char **features;<br>
> +<br>
> + if (!cpu || !cpuData)<br>
> + return ret;<br>
> +<br>
> + if (!(features = virStringSplitCount(cpuData->features, " ",<br>
> + 0, &cpu->nfeatures)))<br>
> + return ret;<br>
> + if (cpu->nfeatures) {<br>
> + if (VIR_ALLOC_N(cpu->features, cpu->nfeatures) < 0)<br>
> + goto error;<br>
> +<br>
> + for (i = 0; i < cpu->nfeatures; i++) {<br>
> + cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;<br>
> + cpu->features[i].name = g_strdup(features[i]);<br>
> + }<br>
> + }<br>
> +<br>
> + ret = 0;<br>
> +<br>
> + cleanup:<br>
> + virStringListFree(features);<br>
> + return ret;<br>
> +<br>
> + error:<br>
> + for (i = 0; i < cpu->nfeatures; i++)<br>
> + VIR_FREE(cpu->features[i].name);<br>
> + VIR_FREE(cpu->features);<br>
> + cpu->nfeatures = 0;<br>
> + goto cleanup;<br>
> +}<br>
> +<br>
> +static int<br>
> +armDecode(virCPUDefPtr cpu,<br>
> + const virCPUarmData *cpuData,<br>
> + virDomainCapsCPUModelsPtr models)<br>
> +{<br>
> + virCPUarmMapPtr map;<br>
> + virCPUarmModelPtr model;<br>
> + virCPUarmVendorPtr vendor = NULL;<br>
> +<br>
> + if (!cpuData || !(map = virCPUarmGetMap()))<br>
> + return -1;<br>
> +<br>
> + if (!(model = armModelFindByPVR(map, cpuData->pvr))) {<br>
> + virReportError(VIR_ERR_OPERATION_FAILED,<br>
> + _("Cannot find CPU model with PVR 0x%03lx"),<br>
> + cpuData->pvr);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + if (!virCPUModelIsAllowed(model->name, models)) {<br>
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,<br>
> + _("CPU model %s is not supported by hypervisor"),<br>
> + model->name);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + cpu->model = g_strdup(model->name);<br>
> +<br>
> + if (cpuData->vendor_id &&<br>
> + !(vendor = armVendorFindByID(map, cpuData->vendor_id))) {<br>
> + virReportError(VIR_ERR_OPERATION_FAILED,<br>
> + _("Cannot find CPU vendor with vendor id 0x%02lx"),<br>
> + cpuData->vendor_id);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + if (vendor)<br>
> + cpu->vendor = g_strdup(vendor->name);<br>
> +<br>
> + if (cpuData->features &&<br>
> + armCpuDataParseFeatures(cpu, cpuData) < 0)<br>
> + return -1;<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int<br>
> +armDecodeCPUData(virCPUDefPtr cpu,<br>
> + const virCPUData *data,<br>
> + virDomainCapsCPUModelsPtr models)<br>
> +{<br>
> + return armDecode(cpu, &data->data.arm, models);<br>
> +}<br>
> +<br>
> +static int<br>
> +virCPUarmGetHost(virCPUDefPtr cpu,<br>
> + virDomainCapsCPUModelsPtr models)<br>
> +{<br>
> + virCPUDataPtr cpuData = NULL;<br>
> + int ret = -1;<br>
> +<br>
> + if (virCPUarmDriverInitialize() < 0)<br>
> + goto cleanup;<br>
> +<br>
> + if (!(cpuData = virCPUDataNew(archs[0])))<br>
> + goto cleanup;<br>
> +<br>
> + if (armCpuDataFromRegs(&cpuData->data.arm) < 0)<br>
> + goto cleanup;<br>
> +<br>
> + ret = armDecodeCPUData(cpu, cpuData, models);<br>
> +<br>
> + cleanup:<br>
> + virCPUarmDataFree(cpuData);<br>
> + return ret;<br>
> +}<br>
<br>
virCPUarmGetHost and the helpers it calls are architecture specific and<br>
should not be even compiled in on non-ARM.<br>
<br>
Jirka<br>
<br>
</blockquote></div>