| Message ID | flspncygsvj.fsf@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Len Brown |
| Headers | show |
| Series | x86_energy_perf_policy fails with Input/output error in a VM | expand |
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. Is parsing /proc/cpuinfo a universal/reliable way to detect this situation?
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; --
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; > -- >
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;