On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ce7c00894f32..2e83f7d74591 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> sizeof(kvm_vcpu_stats_desc),
> };
>
> +
> +/*
> + * Most x86 arch MSR values which contain linear addresses like
Is it most, or all? I'm guessing all?
> + * segment bases, addresses that are used in instructions (e.g SYSENTER),
> + * have static canonicality checks,
Weird and early line breaks.
How about this?
/*
* The canonicality checks for MSRs that hold linear addresses, e.g. segment
* bases, SYSENTER targets, etc., are static, in the sense that they are based
* on CPU _support_ for 5-level paging, not the state of CR4.LA57.
> + * size of whose depends only on CPU's support for 5-level
> + * paging, rather than state of CR4.LA57.
> + *
> + * In addition to that, some of these MSRS are directly passed
> + * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
> + * doen't have LA57 enabled in its CPUID, for consistency with
> + * CPUs' ucode, it is better to pivot the check around host
> + * support for 5 level paging.
I think we should elaborate on why it's better. It only takes another line or
two, and that way we don't forget the edge cases that make properly emulating
guest CPUID a bad idea.
* This creates a virtualization hole where a guest writes to passthrough MSRs
* may incorrectly succeed if the CPU supports LA57, but the vCPU does not
* (because hardware has no awareness of guest CPUID). Do not try to plug this
* hole, i.e. emulate the behavior for intercepted accesses, as injecting #GP
* depending on whether or not KVM happens to emulate a WRMSR would result in
* non-deterministic behavior, and could even allow L2 to crash L1, e.g. if L1
* passes through an MSR to L2, and then tries to save+restore L2's value.
*/
> +
> +static u8 max_host_supported_virt_addr_bits(void)
Any objection to dropping the "supported", i.e. going with max_host_virt_addr_bits()?
Mostly to shorten the name, but also because "supported" suggests there's software
involvement, e.g. the max supported by the kernel/KVM, which isn't the case.
If you're ok with the above, I'll fixup when applying.
