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

[mlevitsk@redhat.com]

У пт, 2024-08-16 у 14:49 -0700, Sean Christopherson пише:
> > > > 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?

I can't be sure that all of them are like that - there could be some outliers that behave differently.

One of the things my work at Intel taught me is that there is nothing consistent
in x86 spec, anything is possible and nothing can be assumed.

I dealt only with those msrs, that KVM checks for canonicality, therefore I use the word 
'most'. There could be other msrs that are not known to me and/or to KVM.

I can write 'some' if you prefer.

> > > > 
> > > > > > > > + * 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.

OK, will do.

> > > > 
> > > >  * 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.

Doesn't matter to me.

> > > > 
> > > > If you're ok with the above, I'll fixup when applying.
> > > > 

Best regards,
   Maxim Levitsky