Re: [PATCH] KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch incorrectly returns L2's TSC frame instead of L1's TSC frame for IA32_TSC in the VM-exit MSR-store list, directly violating the Intel SDM.
- [Medium] The patch introduces an inconsistency between the MSR_IA32_TSC autostore path and the fallback read path.
--

--- Patch [1]: [PATCH] KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list ---
commit 9a886b64a3ad54e2783daef1b8de78fa7019e1c2
Author: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>

KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list

This commit updates KVM to store L2's TSC frame in the VM-exit IA32_TSC
MSR-store list. It recomposes L2's frame using L1 state and vmcs12 TSC
controls.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 6957bb6f5cf7e..610b58933d6b4 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1111,8 +1111,26 @@ static bool nested_vmx_get_vmexit_msr_value(struct kvm_vcpu *vcpu,
>  	if (msr_index == MSR_IA32_TSC && vmx->nested.tsc_autostore_slot >= 0) {
>  		int slot = vmx->nested.tsc_autostore_slot;
>  		u64 host_tsc = vmx->msr_autostore.val[slot].value;
> +		u64 l2_multiplier = vmx_get_l2_tsc_multiplier(vcpu);
> +		u64 l2_offset, l2_ratio;
>  
> -		*data = kvm_read_l1_tsc(vcpu, host_tsc);
> +		/*
> +		 * The guest that just exited is L2, so the value auto-stored on
> +		 * its behalf must be L2's frame: RDMSR(IA32_TSC) in L2 reads
> +		 * (host_tsc scaled by the composed ratio) + the composed offset
> +		 * (SDM Vol 3C 27.4).  nested_vmx_vmexit() has already restored
> +		 * the composed TSC state to L1's by the time the MSR-store list
> +		 * is processed, so recompose L2's frame from L1 state and the
> +		 * vmcs12 TSC controls.  kvm_read_l1_tsc() would instead record
> +		 * L1's frame, dropping L2's offset (and scaling) entirely.
> +		 */
> +		l2_offset = kvm_calc_nested_tsc_offset(vcpu->arch.l1_tsc_offset,
> +						       vmx_get_l2_tsc_offset(vcpu),
> +						       l2_multiplier);
> +		l2_ratio = kvm_calc_nested_tsc_multiplier(vcpu->arch.l1_tsc_scaling_ratio,
> +							  l2_multiplier);
> +
> +		*data = kvm_scale_tsc(host_tsc, l2_ratio) + l2_offset;

[Severity: High]
Does this calculation violate the architectural requirements for the VM-exit
MSR-store list?

According to the Intel SDM (Vol 3C, 27.4), reads of MSR_IA32_TSC for the
VM-exit MSR-store list are not subject to TSC offsetting or TSC scaling,
which requires the hardware to save the bare host TSC.

From L1's perspective, the bare host TSC is L1's TSC frame. By applying L2's
offset and multiplier here, this returns L2's TSC frame instead. A
conforming L1 hypervisor that correctly applies its guest's offset and
multiplier could inadvertently apply them a second time, which could
corrupt the L2 guest's clock.

>  		return true;
>  	}
>  

[Severity: Medium]
Is there an inconsistency introduced between the autostore path and the
fallback MSR read path?

If the autostore slot was not successfully allocated, the code falls back to
calling kvm_emulate_msr_read() right below this block. Because the vcpu
state is already reverted to L1's configuration before processing the
MSR-store list, the fallback path continues to evaluate and return L1's TSC
frame.

This divergence causes the returned value to unexpectedly toggle between L2's
frame and L1's frame depending on whether the autostore array was full.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713144605.2773845-1-amirmohammad.eftekhar@cispa.de?part=1