lkml.org 
[lkml]   [2017]   [Jul]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor
    From
    Date
    On 10.07.2017 22:49, Bandan Das wrote:
    > When L2 uses vmfunc, L0 utilizes the associated vmexit to
    > emulate a switching of the ept pointer by reloading the
    > guest MMU.
    >
    > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    > Signed-off-by: Bandan Das <bsd@redhat.com>
    > ---
    > arch/x86/include/asm/vmx.h | 6 +++++
    > arch/x86/kvm/vmx.c | 58 +++++++++++++++++++++++++++++++++++++++++++---
    > 2 files changed, 61 insertions(+), 3 deletions(-)
    >
    > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
    > index da5375e..5f63a2e 100644
    > --- a/arch/x86/include/asm/vmx.h
    > +++ b/arch/x86/include/asm/vmx.h
    > @@ -115,6 +115,10 @@
    > #define VMX_MISC_SAVE_EFER_LMA 0x00000020
    > #define VMX_MISC_ACTIVITY_HLT 0x00000040
    >
    > +/* VMFUNC functions */
    > +#define VMX_VMFUNC_EPTP_SWITCHING 0x00000001
    > +#define VMFUNC_EPTP_ENTRIES 512
    > +
    > static inline u32 vmx_basic_vmcs_revision_id(u64 vmx_basic)
    > {
    > return vmx_basic & GENMASK_ULL(30, 0);
    > @@ -200,6 +204,8 @@ enum vmcs_field {
    > EOI_EXIT_BITMAP2_HIGH = 0x00002021,
    > EOI_EXIT_BITMAP3 = 0x00002022,
    > EOI_EXIT_BITMAP3_HIGH = 0x00002023,
    > + EPTP_LIST_ADDRESS = 0x00002024,
    > + EPTP_LIST_ADDRESS_HIGH = 0x00002025,
    > VMREAD_BITMAP = 0x00002026,
    > VMWRITE_BITMAP = 0x00002028,
    > XSS_EXIT_BITMAP = 0x0000202C,
    > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
    > index fe8f5fc..0a969fb 100644
    > --- a/arch/x86/kvm/vmx.c
    > +++ b/arch/x86/kvm/vmx.c
    > @@ -246,6 +246,7 @@ struct __packed vmcs12 {
    > u64 eoi_exit_bitmap1;
    > u64 eoi_exit_bitmap2;
    > u64 eoi_exit_bitmap3;
    > + u64 eptp_list_address;
    > u64 xss_exit_bitmap;
    > u64 guest_physical_address;
    > u64 vmcs_link_pointer;
    > @@ -771,6 +772,7 @@ static const unsigned short vmcs_field_to_offset_table[] = {
    > FIELD64(EOI_EXIT_BITMAP1, eoi_exit_bitmap1),
    > FIELD64(EOI_EXIT_BITMAP2, eoi_exit_bitmap2),
    > FIELD64(EOI_EXIT_BITMAP3, eoi_exit_bitmap3),
    > + FIELD64(EPTP_LIST_ADDRESS, eptp_list_address),
    > FIELD64(XSS_EXIT_BITMAP, xss_exit_bitmap),
    > FIELD64(GUEST_PHYSICAL_ADDRESS, guest_physical_address),
    > FIELD64(VMCS_LINK_POINTER, vmcs_link_pointer),
    > @@ -1402,6 +1404,13 @@ static inline bool nested_cpu_has_vmfunc(struct vmcs12 *vmcs12)
    > return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_VMFUNC);
    > }
    >
    > +static inline bool nested_cpu_has_eptp_switching(struct vmcs12 *vmcs12)
    > +{
    > + return nested_cpu_has_vmfunc(vmcs12) &&
    > + (vmcs12->vm_function_control &

    I wonder if it makes sense to rename vm_function_control to
    - vmfunc_control
    - vmfunc_controls (so it matches nested_vmx_vmfunc_controls)
    - vmfunc_ctrl

    > + VMX_VMFUNC_EPTP_SWITCHING);
    > +}
    > +
    > static inline bool is_nmi(u32 intr_info)
    > {
    > return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
    > @@ -2791,7 +2800,12 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
    > if (cpu_has_vmx_vmfunc()) {
    > vmx->nested.nested_vmx_secondary_ctls_high |=
    > SECONDARY_EXEC_ENABLE_VMFUNC;
    > - vmx->nested.nested_vmx_vmfunc_controls = 0;
    > + /*
    > + * Advertise EPTP switching unconditionally
    > + * since we emulate it
    > + */
    > + vmx->nested.nested_vmx_vmfunc_controls =
    > + VMX_VMFUNC_EPTP_SWITCHING;> }
    >
    > /*
    > @@ -7772,6 +7786,9 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
    > struct vcpu_vmx *vmx = to_vmx(vcpu);
    > struct vmcs12 *vmcs12;
    > u32 function = vcpu->arch.regs[VCPU_REGS_RAX];
    > + u32 index = vcpu->arch.regs[VCPU_REGS_RCX];
    > + struct page *page = NULL;
    > + u64 *l1_eptp_list, address;
    >
    > /*
    > * VMFUNC is only supported for nested guests, but we always enable the
    > @@ -7784,11 +7801,46 @@ static int handle_vmfunc(struct kvm_vcpu *vcpu)
    > }
    >
    > vmcs12 = get_vmcs12(vcpu);
    > - if ((vmcs12->vm_function_control & (1 << function)) == 0)
    > + if (((vmcs12->vm_function_control & (1 << function)) == 0) ||
    > + WARN_ON_ONCE(function))

    "... instruction causes a VM exit if the bit at position EAX is 0 in the
    VM-function controls (the selected VM function is
    not enabled)."

    So g2 can trigger this WARN_ON_ONCE, no? I think we should drop it then
    completely.

    > + goto fail;
    > +
    > + if (!nested_cpu_has_ept(vmcs12) ||
    > + !nested_cpu_has_eptp_switching(vmcs12))
    > + goto fail;
    > +
    > + if (!vmcs12->eptp_list_address || index >= VMFUNC_EPTP_ENTRIES)
    > + goto fail;

    I can find the definition for an vmexit in case of index >=
    VMFUNC_EPTP_ENTRIES, but not for !vmcs12->eptp_list_address in the SDM.

    Can you give me a hint?

    > +
    > + page = nested_get_page(vcpu, vmcs12->eptp_list_address);
    > + if (!page)
    > goto fail;
    > - WARN_ONCE(1, "VMCS12 VM function control should have been zero");
    > +
    > + l1_eptp_list = kmap(page);
    > + address = l1_eptp_list[index];
    > + if (!address)
    > + goto fail;

    Can you move that check to the other address checks below? (or rework if
    this make sense, see below)

    > + /*
    > + * If the (L2) guest does a vmfunc to the currently
    > + * active ept pointer, we don't have to do anything else
    > + */
    > + if (vmcs12->ept_pointer != address) {
    > + if (address >> cpuid_maxphyaddr(vcpu) ||
    > + !IS_ALIGNED(address, 4096))

    Couldn't the pfn still be invalid and make kvm_mmu_reload() fail?
    (triggering a KVM_REQ_TRIPLE_FAULT)

    > + goto fail;
    > + kvm_mmu_unload(vcpu);
    > + vmcs12->ept_pointer = address;
    > + kvm_mmu_reload(vcpu);

    I was thinking about something like this:

    kvm_mmu_unload(vcpu);
    old = vmcs12->ept_pointer;
    vmcs12->ept_pointer = address;
    if (kvm_mmu_reload(vcpu)) {
    /* pointer invalid, restore previous state */
    kvm_clear_request(KVM_REQ_TRIPLE_FAULT, vcpu);
    vmcs12->ept_pointer = old;
    kvm_mmu_reload(vcpu);
    goto fail;
    }

    The you can inherit the checks from mmu_check_root().


    Wonder why I can't spot checks for cpuid_maxphyaddr() /
    IS_ALIGNED(address, 4096) for ordinary use of vmcs12->ept_pointer. The
    checks should be identical.


    > + kunmap(page);
    > + nested_release_page_clean(page);

    shouldn't the kunmap + nested_release_page_clean go outside the if clause?

    > + }
    > + return kvm_skip_emulated_instruction(vcpu);
    >
    > fail:
    > + if (page) {
    > + kunmap(page);
    > + nested_release_page_clean(page);
    > + }
    > nested_vmx_vmexit(vcpu, vmx->exit_reason,
    > vmcs_read32(VM_EXIT_INTR_INFO),
    > vmcs_readl(EXIT_QUALIFICATION));
    >

    David and mmu code are not yet best friends. So sorry if I am missing
    something.

    --

    Thanks,

    David

    \
     
     \ /
      Last update: 2017-07-11 09:52    [W:4.866 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site