| Message ID | flspncygsvj.fsf@redhat.com |
|---|---|
| State | Accepted |
| Commit | 0936cdfbb527a4fa2559292069ebff2e8cf2c843 |
| Headers | show |
| Series | x86_energy_perf_policy fails with Input/output error in a VM | expand |
Applied. thanks! -Len On Wed, Apr 1, 2020 at 12:51 PM Ondřej Lysoněk <olysonek@redhat.com> wrote: > > Hi, > > "Brown, Len" <len.brown@intel.com> writes: > > > Thanks for the note, > > > > I agree that is unfriendly how the tool tells the user that it is not possible for it to run in a VM guest. > > If people are running into that, and we can make it more graceful, we should. > > My use case is being able to differentiate why x86_energy_perf_policy > failed in programs that use it, namely Tuned [1]. > > > > > Is parsing /proc/cpuinfo a universal/reliable way to detect this situation? > > From what I've read it seems that it's possible to create a VM that does > not have 'hypervisor' in CPU flags. However I don't think this is a > problem for us - false negatives in the detection will just preserve the > current behaviour. > > Regarding the reverse case, to get the 'hypervisor' CPU flag on bare > metal, you'd have to change the CPU microcode so that the CPUID > instruction returns different values, as far as I understand it. > > Also, the kernel uses the 'hypervisor' flag (X86_FEATURE_HYPERVISOR) > internally in a number of places to detect a virtual machine. E.g. > arch/x86/events/core.c:270 or drivers/acpi/processor_idle.c:509 as of > commit 9420e8ade4353a6710. > > However, perhaps it would be good to change the patch so that it prints > the original msr reading error in cases where /proc/cpuinfo is unavailable. > > I've fixed up the patch a bit and changed it so that it searches only > for whole words '\<hypervisor\>' on lines beginning with > 'flags\t\t'. This should make it more reliable. > > Consider the patch > Signed-off-by: Ondřej Lysoněk <olysonek@redhat.com> > > [1] https://github.com/redhat-performance/tuned/blob/master/tuned/plugins/plugin_cpu.py#L96 > > Ondrej Lysonek > > > diff --git a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c > index 3fe1eed900d4..29e0afbb7b4f 100644 > --- a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c > +++ b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c > @@ -622,6 +622,77 @@ void cmdline(int argc, char **argv) > } > } > > +/* > + * Open a file, and exit on failure > + */ > +FILE *fopen_or_die(const char *path, const char *mode) > +{ > + FILE *filep = fopen(path, "r"); > + > + if (!filep) > + err(1, "%s: open failed", path); > + return filep; > +} > + > +void err_on_hypervisor(void) > +{ > + FILE *cpuinfo; > + char *buffer; > + char *flags; > + char *start_pos, *stop_pos; > + const char *err_msg = NULL; > + const char *hypervisor = " hypervisor"; > + > + /* On VMs, /proc/cpuinfo contains a "hypervisor" flags entry */ > + cpuinfo = fopen_or_die("/proc/cpuinfo", "ro"); > + > + buffer = malloc(4096); > + if (!buffer) { > + err_msg = "buffer malloc failed"; > + goto close_file; > + } > + > + if (!fread(buffer, 1024, 1, cpuinfo)) { > + err_msg = "Reading /proc/cpuinfo failed"; > + goto free_mem; > + } > + > + flags = strstr(buffer, "flags\t\t"); > + if (!flags || (flags > buffer && *(flags - 1) != '\n')) > + goto free_mem; > + > + if (fseek(cpuinfo, flags - buffer, SEEK_SET)) { > + err_msg = "fseek on /proc/cpuinfo failed"; > + goto free_mem; > + } > + if (!fgets(buffer, 4096, cpuinfo)) { > + err_msg = "Reading /proc/cpuinfo failed"; > + goto free_mem; > + } > + > + start_pos = buffer; > + while (1) { > + start_pos = strstr(start_pos, hypervisor); > + stop_pos = start_pos + strlen(hypervisor); > + if (!start_pos || (*stop_pos == ' ' || > + *stop_pos == '\n' || > + *stop_pos == '\0')) > + break; > + start_pos = stop_pos; > + } > + > + if (start_pos) { > + err_msg = "not supported on this virtual machine"; > + } > + > +free_mem: > + free(buffer); > +close_file: > + fclose(cpuinfo); > + > + if (err_msg) > + err(1, err_msg); > +} > > int get_msr(int cpu, int offset, unsigned long long *msr) > { > @@ -635,8 +706,10 @@ int get_msr(int cpu, int offset, unsigned long long *msr) > err(-1, "%s open failed, try chown or chmod +r /dev/cpu/*/msr, or run as root", pathname); > > retval = pread(fd, msr, sizeof(*msr), offset); > - if (retval != sizeof(*msr)) > + if (retval != sizeof(*msr)) { > + err_on_hypervisor(); > err(-1, "%s offset 0x%llx read failed", pathname, (unsigned long long)offset); > + } > > if (debug > 1) > fprintf(stderr, "get_msr(cpu%d, 0x%X, 0x%llX)\n", cpu, offset, *msr); > @@ -1086,18 +1159,6 @@ int update_cpu_msrs(int cpu) > return 0; > } > > -/* > - * Open a file, and exit on failure > - */ > -FILE *fopen_or_die(const char *path, const char *mode) > -{ > - FILE *filep = fopen(path, "r"); > - > - if (!filep) > - err(1, "%s: open failed", path); > - return filep; > -} > - > unsigned int get_pkg_num(int cpu) > { > FILE *fp; > -- > -- Len Brown, Intel Open Source Technology Center
diff --git a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c index 3fe1eed900d4..ff6c6661f075 100644 --- a/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c +++ b/tools/power/x86/x86_energy_perf_policy/x86_energy_perf_policy.c @@ -622,6 +622,57 @@ void cmdline(int argc, char **argv) } } +/* + * Open a file, and exit on failure + */ +FILE *fopen_or_die(const char *path, const char *mode) +{ + FILE *filep = fopen(path, "r"); + + if (!filep) + err(1, "%s: open failed", path); + return filep; +} + +void err_on_hypervisor(void) +{ + FILE *cpuinfo; + char *flags, *hypervisor; + char *buffer; + + /* On VMs /proc/cpuinfo contains a "flags" entry for hypervisor */ + cpuinfo = fopen_or_die("/proc/cpuinfo", "ro"); + + buffer = malloc(4096); + if (!buffer) { + fclose(cpuinfo); + err(-ENOMEM, "buffer malloc fail"); + } + + if (!fread(buffer, 1024, 1, cpuinfo)) { + fclose(cpuinfo); + free(buffer); + err(1, "Reading /proc/cpuinfo failed"); + } + + flags = strstr(buffer, "flags"); + rewind(cpuinfo); + fseek(cpuinfo, flags - buffer, SEEK_SET); + if (!fgets(buffer, 4096, cpuinfo)) { + fclose(cpuinfo); + free(buffer); + err(1, "Reading /proc/cpuinfo failed"); + } + fclose(cpuinfo); + + hypervisor = strstr(buffer, "hypervisor"); + + free(buffer); + + if (hypervisor) + err(-1, + "not supported on this virtual machine"); +} int get_msr(int cpu, int offset, unsigned long long *msr) { @@ -635,8 +686,10 @@ int get_msr(int cpu, int offset, unsigned long long *msr) err(-1, "%s open failed, try chown or chmod +r /dev/cpu/*/msr, or run as root", pathname); retval = pread(fd, msr, sizeof(*msr), offset); - if (retval != sizeof(*msr)) + if (retval != sizeof(*msr)) { + err_on_hypervisor(); err(-1, "%s offset 0x%llx read failed", pathname, (unsigned long long)offset); + } if (debug > 1) fprintf(stderr, "get_msr(cpu%d, 0x%X, 0x%llX)\n", cpu, offset, *msr); @@ -1086,18 +1139,6 @@ int update_cpu_msrs(int cpu) return 0; } -/* - * Open a file, and exit on failure - */ -FILE *fopen_or_die(const char *path, const char *mode) -{ - FILE *filep = fopen(path, "r"); - - if (!filep) - err(1, "%s: open failed", path); - return filep; -} - unsigned int get_pkg_num(int cpu) { FILE *fp;