We currently query the host MSRs to determine if TSC scaling is
supported. This works OK when running privileged and can open
the /dev/cpu/0/msr. When unprivileged we fallback to querying
MSRs from /dev/kvm. This is incorrect because /dev/kvm only
reports accurate info for MSRs that are valid to use from inside
a guest.  The TSC scaling support MSR is not, thus we always end
up reporting lack of TSC scaling when unprivileged.

The solution to this is easy, because KVM can directly report
whether TSC scaling is available, which matches what QEMU will
do at startup.

https://gitlab.com/libvirt/libvirt/-/issues/188
Reported-by: Roman Mohr <rmohr@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/util/virhostcpu.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 7aa92ad11d..f140610b47 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -1349,11 +1349,10 @@ virHostCPUGetMSR(unsigned long index,
 virHostCPUTscInfo *
 virHostCPUGetTscInfo(void)
 {
-    virHostCPUTscInfo *info;
+    g_autofree virHostCPUTscInfo *info = g_new0(virHostCPUTscInfo, 1);
     VIR_AUTOCLOSE kvmFd = -1;
     VIR_AUTOCLOSE vmFd = -1;
     VIR_AUTOCLOSE vcpuFd = -1;
-    uint64_t msr = 0;
     int rc;
 
     if ((kvmFd = open(KVM_DEVICE, O_RDONLY)) < 0) {
@@ -1378,23 +1377,19 @@ virHostCPUGetTscInfo(void)
                              _("Unable to probe TSC frequency"));
         return NULL;
     }
-
-    info = g_new0(virHostCPUTscInfo, 1);
-
     info->frequency = rc * 1000ULL;
 
-    if (virHostCPUGetMSR(VMX_PROCBASED_CTLS2_MSR, &msr) == 0) {
-        /* High 32 bits of the MSR value indicate whether specific control
-         * can be set to 1. */
-        msr >>= 32;
-
-        info->scaling = virTristateBoolFromBool(!!(msr & VMX_USE_TSC_SCALING));
+    if ((rc = ioctl(kvmFd, KVM_CHECK_EXTENSION, KVM_CAP_TSC_CONTROL)) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("Unable to query TSC scaling support"));
+        return NULL;
     }
+    info->scaling = rc ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO;
 
     VIR_DEBUG("Detected TSC frequency %llu Hz, scaling %s",
               info->frequency, virTristateBoolTypeToString(info->scaling));
 
-    return info;
+    return g_steal_pointer(&info);
 }
 
 #else
-- 
2.31.1

On 8/4/21 7:09 PM, Daniel P. Berrangé wrote:
> We currently query the host MSRs to determine if TSC scaling is
> supported. This works OK when running privileged and can open
> the /dev/cpu/0/msr. When unprivileged we fallback to querying
> MSRs from /dev/kvm. This is incorrect because /dev/kvm only
> reports accurate info for MSRs that are valid to use from inside
> a guest.  The TSC scaling support MSR is not, thus we always end
> up reporting lack of TSC scaling when unprivileged.
> 
> The solution to this is easy, because KVM can directly report
> whether TSC scaling is available, which matches what QEMU will
> do at startup.
> 
> https://gitlab.com/libvirt/libvirt/-/issues/188

Perhaps, Closes: https://gitlab.com/... ?

> Reported-by: Roman Mohr <rmohr@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/util/virhostcpu.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 7aa92ad11d..f140610b47 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -1349,11 +1349,10 @@ virHostCPUGetMSR(unsigned long index,
>  virHostCPUTscInfo *
>  virHostCPUGetTscInfo(void)
>  {
> -    virHostCPUTscInfo *info;
> +    g_autofree virHostCPUTscInfo *info = g_new0(virHostCPUTscInfo, 1);
>      VIR_AUTOCLOSE kvmFd = -1;
>      VIR_AUTOCLOSE vmFd = -1;
>      VIR_AUTOCLOSE vcpuFd = -1;
> -    uint64_t msr = 0;
>      int rc;
>  
>      if ((kvmFd = open(KVM_DEVICE, O_RDONLY)) < 0) {
> @@ -1378,23 +1377,19 @@ virHostCPUGetTscInfo(void)
>                               _("Unable to probe TSC frequency"));
>          return NULL;
>      }
> -
> -    info = g_new0(virHostCPUTscInfo, 1);
> -
>      info->frequency = rc * 1000ULL;
>  
> -    if (virHostCPUGetMSR(VMX_PROCBASED_CTLS2_MSR, &msr) == 0) {
> -        /* High 32 bits of the MSR value indicate whether specific control
> -         * can be set to 1. */
> -        msr >>= 32;
> -
> -        info->scaling = virTristateBoolFromBool(!!(msr & VMX_USE_TSC_SCALING));
> +    if ((rc = ioctl(kvmFd, KVM_CHECK_EXTENSION, KVM_CAP_TSC_CONTROL)) < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to query TSC scaling support"));
> +        return NULL;
>      }
> +    info->scaling = rc ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO;
>  
>      VIR_DEBUG("Detected TSC frequency %llu Hz, scaling %s",
>                info->frequency, virTristateBoolTypeToString(info->scaling));
>  
> -    return info;
> +    return g_steal_pointer(&info);
>  }
>  
>  #else
> 

I think you can also remove defines of those VMX_ macros that are being
removed:

# define VMX_PROCBASED_CTLS2_MSR 0x48b
# define VMX_USE_TSC_SCALING (1 << 25)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal