Re: x86 CPU properties "family", "model", "stepping", "tsc-freq"

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Oct 09, 2024 at 01:01:40PM +0200, Markus Armbruster wrote:
>> QOM properties serve several purposes: initial configuration (external
>> interface), run time control and monitoring (external interface), and
>> internal purposes like versioning.  Which purpose(s) a property serves
>> is often unclear.

[...]

>> Property "tsc-freq" seems not to be used internally.  I figure changing
>> it at run time makes no sense.  Looks like it is be meant to be set by
>> the user: x86_cpu_parse_featurestr() seems to parse it from the argument
>> of -cpu.  Correct?  Note that -cpu help doesn't mention it.
>
> "tsc-freq" is a back compat alias for 'tsc-frequency' AFAICT, and
> we need the latter if you want to guarantee fixed TSC freq across
> migration. eg commit 561dbb41b1d752098249128d8462aaadc56fd15d

Looking more closely...

"tsc-freq" wraps around "tsc-frequency" except the value is converted
with qemu_strtosz_metric().

"tsc-frequency" accepts values between 0 and INT64_MAX.  The unit is Hz.
However, the frequency is internally stored in kHz, in CPUArchState
member tsc_khz, and the conversion from the value "tsc-frequency"
silently truncates.  Ugh!  See x86_cpuid_set_tsc_freq().

kvm_arch_set_tsc_khz() passes the kHz value to the kernel like this:

    kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz)

I believe the kernel code consuming the value is in arch/x86/kvm/x86.c's
kvm_arch_vcpu_ioctl().  It appears to silently truncate the value to 32
bit unsigned:

	case KVM_SET_TSC_KHZ: {
		u32 user_tsc_khz;

		r = -EINVAL;
--->		user_tsc_khz = (u32)arg;

		if (kvm_caps.has_tsc_control &&
		    user_tsc_khz >= kvm_caps.max_guest_tsc_khz)
			goto out;

		if (user_tsc_khz == 0)
			user_tsc_khz = tsc_khz;

		if (!kvm_set_tsc_khz(vcpu, user_tsc_khz))
			r = 0;

		goto out;
	}

Ugh again!

Should we reject values exceeding UINT32_MAX kHz in QEMU?