Re: [PATCH v4 8/8] x86,kvm: Teach KVM's VMX code that CR3 isn't a constant

[Andy Lutomirski <luto@kernel.org>]

On Fri, Jul 14, 2017 at 1:06 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Sun, May 28, 2017 at 10:00:17AM -0700, Andy Lutomirski wrote:
>> When PCID is enabled, CR3's PCID bits can change during context
>> switches, so KVM won't be able to treat CR3 as a per-mm constant any
>> more.
>>
>> I structured this like the existing CR4 handling.  Under ordinary
>> circumstances (PCID disabled or if the current PCID and the value
>> that's already in the VMCS match), then we won't do an extra VMCS
>> write, and we'll never do an extra direct CR3 read.  The overhead
>> should be minimal.
>>
>> I disallowed using the new helper in non-atomic context because
>> PCID support will cause CR3 to stop being constant in non-atomic
>> process context.
>>
>> (Frankly, it also scares me a bit that KVM ever treated CR3 as
>> constant, but it looks like it was okay before.)
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Cc: kvm@vger.kernel.org
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: Dave Hansen <dave.hansen@intel.com>
>> Cc: Nadav Amit <namit@vmware.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/include/asm/mmu_context.h | 19 +++++++++++++++++++
>>  arch/x86/kvm/vmx.c                 | 21 ++++++++++++++++++---
>>  2 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>> index 187c39470a0b..f20d7ea47095 100644
>> --- a/arch/x86/include/asm/mmu_context.h
>> +++ b/arch/x86/include/asm/mmu_context.h
>> @@ -266,4 +266,23 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
>>       return __pkru_allows_pkey(vma_pkey(vma), write);
>>  }
>>
>> +
>> +/*
>> + * This can be used from process context to figure out what the value of
>> + * CR3 is without needing to do a (slow) read_cr3().
>> + *
>> + * It's intended to be used for code like KVM that sneakily changes CR3
>> + * and needs to restore it.  It needs to be used very carefully.
>> + */
>> +static inline unsigned long __get_current_cr3_fast(void)
>> +{
>> +     unsigned long cr3 = __pa(this_cpu_read(cpu_tlbstate.loaded_mm)->pgd);
>> +
>> +     /* For now, be very restrictive about when this can be called. */
>> +     VM_WARN_ON(in_nmi() || !in_atomic());
>
> With the following config (from Fedora26 + olddefconfig)
>
>   $ grep PREEMPT .config
>   CONFIG_PREEMPT_NOTIFIERS=y
>   # CONFIG_PREEMPT_NONE is not set
>   CONFIG_PREEMPT_VOLUNTARY=y
>   # CONFIG_PREEMPT is not set
>
> I hit this warning on !in_atomic() on every vm entry.  Shouldn't this be
> preemptible() instead?

Ugh, I hate in_atomic() and its willingness to return the sort-of-wrong answer.

Want to send a patch?

--Andy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>