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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Oct 09, 2024 at 03:44:25PM +0200, Markus Armbruster wrote:
> 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?

Sounds like a reasonable idea to me, as I don't think it is very likely
that someone is passing a number greater than UINT32_MAX and relying
on truncation.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|