On Thu, Jan 23, 2025, David Woodhouse wrote:
> On Thu, 2025-01-23 at 19:02 +0000, Fred Griffoul wrote:
>
> > +static inline void kvm_xen_may_update_tsc_info(struct kvm_vcpu *vcpu,
> > + u32 function, u32 index,
> > + u32 *eax, u32 *ecx, u32 *edx)
>
> Should this be called kvm_xen_maybe_update_tsc_info() ?
>
> Is it worth adding if (static_branch_unlikely(&kvm_xen_enabled.key))?
Or add a helper? Especially if we end up processing KVM_REQ_CLOCK_UPDATE.
static inline bool kvm_xen_is_tsc_leaf(struct kvm_vcpu *vcpu, u32 function)
{
return static_branch_unlikely(&kvm_xen_enabled.key) &&
vcpu->arch.xen.cpuid.base &&
function < vcpu->arch.xen.cpuid.limit;
function == (vcpu->arch.xen.cpuid.base | XEN_CPUID_LEAF(3));
}
>
> > +{
> > + u32 base = vcpu->arch.xen.cpuid.base;
> > +
> > + if (base && (function == (base | XEN_CPUID_LEAF(3)))) {
Pretty sure cpuid.limit needs to be checked, e.g. to avoid a false positive in
the unlikely scenario that userspace advertised a lower limit but still filled
the CPUID entry.
> > + if (index == 1) {
> > + *ecx = vcpu->arch.hv_clock.tsc_to_system_mul;
> > + *edx = vcpu->arch.hv_clock.tsc_shift;
>
> Are these fields in vcpu->arch.hv_clock definitely going to be set?
Set, yes. Up-to-date, no. If there is a pending KVM_REQ_CLOCK_UPDATE, e.g. due
to frequency change, KVM could emulate CPUID before processing the request if
the CPUID VM-Exit occurred before the request was made.
> If so, can we have a comment to that effect? And perhaps a warning to
> assert the truth of that claim?
>
> Before this patch, if the hv_clock isn't yet set then the guest would
> see the original content of the leaves as set by userspace?
In theory, yes, but in practice that can't happen because KVM always pends a
KVM_REQ_CLOCK_UPDATE before entering the guest (it's stupidly hard to see).
On the first kvm_arch_vcpu_load(), vcpu->cpu will be -1, which results in
KVM_REQ_GLOBAL_CLOCK_UPDATE being pending.
if (unlikely(vcpu->cpu != cpu) || kvm_check_tsc_unstable()) {
...
if (!vcpu->kvm->arch.use_master_clock || vcpu->cpu == -1)
kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
}
That in turn triggers a KVM_REQ_CLOCK_UPDATE.
if (kvm_check_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu))
kvm_gen_kvmclock_update(vcpu);
static void kvm_gen_kvmclock_update(struct kvm_vcpu *v)
{
struct kvm *kvm = v->kvm;
kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
schedule_delayed_work(&kvm->arch.kvmclock_update_work,
KVMCLOCK_UPDATE_DELAY);
}
And in the extremely unlikely failure path, which I assume handles the case where
TSC calibration hasn't completed, KVM requests another KVM_REQ_CLOCK_UPDATE and
aborts VM-Enter. So AFAICT, it's impossible to trigger CPUID emulation without
first stuffing Xen CPUID.
/* Keep irq disabled to prevent changes to the clock */
local_irq_save(flags);
tgt_tsc_khz = get_cpu_tsc_khz();
if (unlikely(tgt_tsc_khz == 0)) {
local_irq_restore(flags);
kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
return 1;
}
> Now it gets zeroes if that happens?
Somewhat of a tangent, if userspace is providing non-zero values, commit f422f853af03
("KVM: x86/xen: update Xen CPUID Leaf 4 (tsc info) sub-leaves, if present") would
have broken userspace. QEMU doesn't appear to stuff non-zero values and no one
has complained, so I think we escaped this time.
Jumping back to the code, if we add kvm_xen_is_tsc_leaf(), I would be a-ok with
handling the CPUID manipulations in kvm_cpuid(). I'd probably even prefer it,
because overall I think bleeding a few Xen details into common code is worth
making the flow easier to follow.
Putting it all together, something like this? Compile tested only.
---
arch/x86/kvm/cpuid.c | 16 ++++++++++++++++
arch/x86/kvm/x86.c | 3 +--
arch/x86/kvm/x86.h | 1 +
arch/x86/kvm/xen.c | 23 -----------------------
arch/x86/kvm/xen.h | 13 +++++++++++--
5 files changed, 29 insertions(+), 27 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index edef30359c19..689882326618 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -2005,6 +2005,22 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
} else if (function == 0x80000007) {
if (kvm_hv_invtsc_suppressed(vcpu))
*edx &= ~feature_bit(CONSTANT_TSC);
+ } else if (IS_ENABLED(CONFIG_KVM_XEN) &&
+ kvm_xen_is_tsc_leaf(vcpu, function)) {
+ /*
+ * Update guest TSC frequency information is necessary.
+ * Ignore failures, there is no sane value that can be
+ * provided if KVM can't get the TSC frequency.
+ */
+ if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, vcpu))
+ kvm_guest_time_update(vcpu);
+
+ if (index == 1) {
+ *ecx = vcpu->arch.hv_clock.tsc_to_system_mul;
+ *edx = vcpu->arch.hv_clock.tsc_shift;
+ } else if (index == 2) {
+ *eax = vcpu->arch.hw_tsc_khz;
+ }
}
} else {
*eax = *ebx = *ecx = *edx = 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f61d71783d07..817a7e522935 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3173,7 +3173,7 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
trace_kvm_pvclock_update(v->vcpu_id, &vcpu->hv_clock);
}
-static int kvm_guest_time_update(struct kvm_vcpu *v)
+int kvm_guest_time_update(struct kvm_vcpu *v)
{
unsigned long flags, tgt_tsc_khz;
unsigned seq;
@@ -3256,7 +3256,6 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
&vcpu->hv_clock.tsc_shift,
&vcpu->hv_clock.tsc_to_system_mul);
vcpu->hw_tsc_khz = tgt_tsc_khz;
- kvm_xen_update_tsc_info(v);
}
vcpu->hv_clock.tsc_timestamp = tsc_timestamp;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 7a87c5fc57f1..5fdf32ba9406 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -362,6 +362,7 @@ void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
u64 get_kvmclock_ns(struct kvm *kvm);
uint64_t kvm_get_wall_clock_epoch(struct kvm *kvm);
bool kvm_get_monotonic_and_clockread(s64 *kernel_ns, u64 *tsc_timestamp);
+int kvm_guest_time_update(struct kvm_vcpu *v);
int kvm_read_guest_virt(struct kvm_vcpu *vcpu,
gva_t addr, void *val, unsigned int bytes,
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index a909b817b9c0..ed5c2f088361 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -2247,29 +2247,6 @@ void kvm_xen_destroy_vcpu(struct kvm_vcpu *vcpu)
del_timer_sync(&vcpu->arch.xen.poll_timer);
}
-void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu)
-{
- struct kvm_cpuid_entry2 *entry;
- u32 function;
-
- if (!vcpu->arch.xen.cpuid.base)
- return;
-
- function = vcpu->arch.xen.cpuid.base | XEN_CPUID_LEAF(3);
- if (function > vcpu->arch.xen.cpuid.limit)
- return;
-
- entry = kvm_find_cpuid_entry_index(vcpu, function, 1);
- if (entry) {
- entry->ecx = vcpu->arch.hv_clock.tsc_to_system_mul;
- entry->edx = vcpu->arch.hv_clock.tsc_shift;
- }
-
- entry = kvm_find_cpuid_entry_index(vcpu, function, 2);
- if (entry)
- entry->eax = vcpu->arch.hw_tsc_khz;
-}
-
void kvm_xen_init_vm(struct kvm *kvm)
{
mutex_init(&kvm->arch.xen.xen_lock);
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index f5841d9000ae..5ee7f3f1b45f 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -9,6 +9,7 @@
#ifndef __ARCH_X86_KVM_XEN_H__
#define __ARCH_X86_KVM_XEN_H__
+#include <asm/xen/cpuid.h>
#include <asm/xen/hypervisor.h>
#ifdef CONFIG_KVM_XEN
@@ -35,7 +36,6 @@ int kvm_xen_set_evtchn_fast(struct kvm_xen_evtchn *xe,
int kvm_xen_setup_evtchn(struct kvm *kvm,
struct kvm_kernel_irq_routing_entry *e,
const struct kvm_irq_routing_entry *ue);
-void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu);
static inline void kvm_xen_sw_enable_lapic(struct kvm_vcpu *vcpu)
{
@@ -50,6 +50,14 @@ static inline void kvm_xen_sw_enable_lapic(struct kvm_vcpu *vcpu)
kvm_xen_inject_vcpu_vector(vcpu);
}
+static inline bool kvm_xen_is_tsc_leaf(struct kvm_vcpu *vcpu, u32 function)
+{
+ return static_branch_unlikely(&kvm_xen_enabled.key) &&
+ vcpu->arch.xen.cpuid.base &&
+ function < vcpu->arch.xen.cpuid.limit &&
+ function == (vcpu->arch.xen.cpuid.base | XEN_CPUID_LEAF(3));
+}
+
static inline bool kvm_xen_msr_enabled(struct kvm *kvm)
{
return static_branch_unlikely(&kvm_xen_enabled.key) &&
@@ -157,8 +165,9 @@ static inline bool kvm_xen_timer_enabled(struct kvm_vcpu *vcpu)
return false;
}
-static inline void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu)
+static inline bool kvm_xen_is_tsc_leaf(struct kvm_vcpu *vcpu, u32 function)
{
+ return false;
}
#endif
base-commit: 84be94b5b6490e29a6f386cec90f8d5c6d14f0df
--
