[PATCH] virhostcpu: fix getting CPU freq for Apple Silicon

Menci posted 1 patch 2 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20220207084806.47411-1-huanghaorui301@gmail.com
src/util/virhostcpu.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
[PATCH] virhostcpu: fix getting CPU freq for Apple Silicon
Posted by Menci 2 years, 2 months ago
The current implementation of virHostCPUGetInfo for macOS (__APPLE__)
reads "hw.cpufrequency" from sysctl, which is available only on x86_64
architecture.

On Apple Silicon ARM Macs, it's not available:

  $ sysctl hw.cpufrequency              # No output
  $ arch -x86_64 sysctl hw.cpufrequency # Run with Rosetta 2
  hw.cpufrequency: 2400000000

When running libvirtd on Apple Silicon, I got the error:

  cannot obtain CPU freq: No such file or directory.

To fix it, we can calculate it with "hw.tbfrequency" and
"kern.clockrate" instead:

  $ sysctl hw.tbfrequency
  hw.tbfrequency: 24000000
  $ sysctl kern.clockrate
  kern.clockrate: { hz = 100, tick = 10000, tickadj = 0, profhz = 100, stathz = 100 }

The result value would be hw.tbfrequency * kern.clockrate.hz.

Signed-off-by: Menci <huanghaorui301@gmail.com>
---
 src/util/virhostcpu.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index a07c00a0e9..72983c91f3 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -927,9 +927,18 @@ virHostCPUGetInfo(virArch hostarch G_GNUC_UNUSED,
 
     *mhz = cpu_freq;
 # else
+    /* This works for Intel Macs */
     if (sysctlbyname("hw.cpufrequency", &cpu_freq, &cpu_freq_len, NULL, 0) < 0) {
-        virReportSystemError(errno, "%s", _("cannot obtain CPU freq"));
-        return -1;
+        /* On Apple Silicon fallback to hw.tbfrequency and kern.clockrate.hz */
+        struct clockinfo clockrate;
+        size_t clockrate_len = sizeof(clockrate);
+        if (sysctlbyname("hw.tbfrequency", &cpu_freq, &cpu_freq_len, NULL, 0) < 0 ||
+            sysctlbyname("kern.clockrate", &clockrate, &clockrate_len, NULL, 0) < 0) {
+            virReportSystemError(errno, "%s", _("cannot obtain CPU freq"));
+            return -1;
+        }
+
+        cpu_freq *= clockrate.hz;
     }
 
     *mhz = cpu_freq / 1000000;
-- 
2.34.1

Re: [PATCH] virhostcpu: fix getting CPU freq for Apple Silicon
Posted by Andrea Bolognani 2 years, 2 months ago
On Mon, Feb 07, 2022 at 04:48:06PM +0800, Menci wrote:
> The current implementation of virHostCPUGetInfo for macOS (__APPLE__)
> reads "hw.cpufrequency" from sysctl, which is available only on x86_64
> architecture.
>
> On Apple Silicon ARM Macs, it's not available:
>
>   $ sysctl hw.cpufrequency              # No output
>   $ arch -x86_64 sysctl hw.cpufrequency # Run with Rosetta 2
>   hw.cpufrequency: 2400000000
>
> When running libvirtd on Apple Silicon, I got the error:
>
>   cannot obtain CPU freq: No such file or directory.
>
> To fix it, we can calculate it with "hw.tbfrequency" and
> "kern.clockrate" instead:
>
>   $ sysctl hw.tbfrequency
>   hw.tbfrequency: 24000000
>   $ sysctl kern.clockrate
>   kern.clockrate: { hz = 100, tick = 10000, tickadj = 0, profhz = 100, stathz = 100 }
>
> The result value would be hw.tbfrequency * kern.clockrate.hz.
>
> Signed-off-by: Menci <huanghaorui301@gmail.com>

The tag should be in the form

  Signed-off-by: Firstname Lastname <email@domain.tld>

Can you please update it, and your git authorship information too?

> +    /* This works for Intel Macs */
>      if (sysctlbyname("hw.cpufrequency", &cpu_freq, &cpu_freq_len, NULL, 0) < 0) {
> -        virReportSystemError(errno, "%s", _("cannot obtain CPU freq"));
> -        return -1;
> +        /* On Apple Silicon fallback to hw.tbfrequency and kern.clockrate.hz */
> +        struct clockinfo clockrate;
> +        size_t clockrate_len = sizeof(clockrate);
> +        if (sysctlbyname("hw.tbfrequency", &cpu_freq, &cpu_freq_len, NULL, 0) < 0 ||
> +            sysctlbyname("kern.clockrate", &clockrate, &clockrate_len, NULL, 0) < 0) {
> +            virReportSystemError(errno, "%s", _("cannot obtain CPU freq"));
> +            return -1;
> +        }
> +
> +        cpu_freq *= clockrate.hz;

Do you have any reference explaining why these specific values,
combined in this specific way, are going to result in the CPU
freqency? In the examples you provided in the commit message the
values seem to match up, but I'd like to see some stronger evidence.

Additionally, is there any reason why we wouldn't just use the new
set of sysctls regardless of architecture? Are those not available on
Intel?

One last thing to keep in mind: even when running on Linux, we don't
report the CPU frequency for Arm CPUs. This is explicitly allowed:

  struct virNodeInfo {
      ...
      unsigned int mhz;     /* expected CPU frequency, 0 if not known or
                               on unusual architectures */
      ...

So if there is no way to accurately detect the CPU frequency on Apple
Silicon, it would be perfectly fine to do something like

  if (sysctlbyname("hw.cpufrequency", &cpu_freq, &cpu_freq_len, NULL, 0) < 0) {
      if (errno == ENOENT) {
          cpu_freq = 0;
      } else {
          virReportSystemError(errno, "%s", _("cannot obtain CPU freq"));
          return -1;
      }
      ...

and simply report the fact that we couldn't figure out the CPU
frequency to the user.

-- 
Andrea Bolognani / Red Hat / Virtualization