Re: [PATCH] x86/kvm/mmu: reset MMU context when 32-bit guest switches PAE

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Apr 30, 2019 at 07:33:26PM +0200, Vitaly Kuznetsov wrote:
> Commit 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it
> to 'gpte_size'") introduced a regression: 32-bit PAE guests stopped

"gpte_is_8_bytes" is really confusing in this case. :-(  Unfortunately I
can't think I can't think of a better name that isn't ridiculously verbose.

> working. The issue appears to be: when guest switches (enables) PAE we need
> to re-initialize MMU context (set context->root_level, do
> reset_rsvds_bits_mask(), ...) but init_kvm_tdp_mmu() doesn't do that
> because we threw away is_pae(vcpu) flag from mmu role. Restore it to
> kvm_mmu_extended_role (as we now don't need it in base role) to fix
> the issue.

The change makes sense, but I'm amazed that there's a kernel that can
actually trigger the bug.  The extended role tracks CR0.PG, so I'm pretty
sure hitting this bug requires toggling CR4.PAE *while paging is enabled*.
Which is legal, but crazy.  E.g. my 32-bit Linux VM runs fine with and
without PAE enabled.

> Fixes: 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it to 'gpte_size'")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>

> ---
> - RFC: it was proven multiple times that mmu code is more complex than it
>   appears (at least to me) :-)

LOL, maybe you're just more optimistic than most people.  Every time I
look at the code I say something along the lines of "holy $&*#".

> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/mmu.c              | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a9d03af34030..c79abe7ca093 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -295,6 +295,7 @@ union kvm_mmu_extended_role {
>  		unsigned int valid:1;
>  		unsigned int execonly:1;
>  		unsigned int cr0_pg:1;
> +		unsigned int cr4_pae:1;
>  		unsigned int cr4_pse:1;
>  		unsigned int cr4_pke:1;
>  		unsigned int cr4_smap:1;
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index e10962dfc203..d9c7b45d231f 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4781,6 +4781,7 @@ static union kvm_mmu_extended_role kvm_calc_mmu_role_ext(struct kvm_vcpu *vcpu)
>  	union kvm_mmu_extended_role ext = {0};
>  
>  	ext.cr0_pg = !!is_paging(vcpu);
> +	ext.cr4_pae = !!is_pae(vcpu);

This got me thinking, I wonder if we can/should leave the CR4 bits clear
if !is_paging().  Technically I think we're unnecessarily purging the MMU
when the guest is toggling CR4 bits with CR0.PG==0.  And I think we can
also git rid of the oddball nx flag in struct kvm_mmu.  I'll play around
with the code and hopefully send a patch or two.

>  	ext.cr4_smep = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMEP);
>  	ext.cr4_smap = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);
>  	ext.cr4_pse = !!is_pse(vcpu);
> -- 
> 2.20.1
>