Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86 architectural msrs

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 16, 2024, Sean Christopherson wrote:
> On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> 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.

I take that back, I think we're going to need a v4 (see patch 3).