Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:
> 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.
Once upon a time the was an operating system called "RHEL5". Some people
think it's long gone but you know, "no one's ever really gone" :-)
>
>> Fixes: 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it to 'gpte_size'")
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>
> Reviewed-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
Thanks!
>
>> ---
>> - 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.
Yes, probably. Not sure I know any performance critical use-cases with
CR0.PG==0 but who knows, maybe running hundreds of 8086 emulators IS a
thing :-)
> 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
>>
--
Vitaly
