Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1684631 > unrolled thread

[PATCH v4 0/3] Expose VMFUNC to the nested hypervisor

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/3] Expose VMFUNC to the nested hypervisor Bandan Das <bsd@redhat.com> - 2017-07-10 23:00 +0200
    [PATCH v4 2/3] KVM: nVMX: Enable VMFUNC for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-10 23:00 +0200
    [PATCH v4 1/3] KVM: vmx: Enable VMFUNCs Bandan Das <bsd@redhat.com> - 2017-07-10 23:00 +0200
    [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-10 23:00 +0200
      Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor David Hildenbrand <david@redhat.com> - 2017-07-11 10:00 +0200
        Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Paolo Bonzini <pbonzini@redhat.com> - 2017-07-11 10:50 +0200
        Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-11 16:00 +0200
          Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 20:10 +0200
            Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-11 21:20 +0200
              Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 21:40 +0200
        Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 20:00 +0200
          Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 20:30 +0200
            Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-11 21:40 +0200
              Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 22:00 +0200
                Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-11 22:30 +0200
                  Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 22:40 +0200
                    Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-11 22:50 +0200
                      Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 23:10 +0200
                        Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-12 15:30 +0200
                          Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-12 20:20 +0200
                            Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-12 21:20 +0200
                  Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-17 20:00 +0200
          Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Jim Mattson <jmattson@google.com> - 2017-07-11 20:30 +0200
            Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 20:40 +0200
              Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-11 21:20 +0200
                Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 21:40 +0200
                  Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-11 22:30 +0200
                    Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-11 22:50 +0200
                      Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor Radim Krčmář <rkrcmar@redhat.com> - 2017-07-12 15:50 +0200
                        Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-12 20:10 +0200
          Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1  hypervisor David Hildenbrand <david@redhat.com> - 2017-07-13 17:50 +0200
            Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor Bandan Das <bsd@redhat.com> - 2017-07-13 19:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1685987 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

2017-07-12 14:11-0400, Bandan Das:
> As much as I would like to disagree with you, I have already spent way more
> time on this then I want. Please let's just leave it here, then ? The mmu unload
> will make sure there's an invalid root hpa and whatever happens next, happens.

Sure;  let's discuss the subtleties of hardware emulation over a beer.

[toc] | [prev] | [next] | [standalone]

#1689313 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

Radim Krčmář <rkrcmar@redhat.com> writes:
...
>> > and no other mentions of a VM exit, so I think that the VM exit happens
>> > only under these conditions:
>> >
>> >   — The EPT memory type (bits 2:0) must be a value supported by the
>> >     processor as indicated in the IA32_VMX_EPT_VPID_CAP MSR (see
>> >     Appendix A.10).
>> >   — Bits 5:3 (1 less than the EPT page-walk length) must be 3, indicating
>> >     an EPT page-walk length of 4; see Section 28.2.2.
>> >   — Bit 6 (enable bit for accessed and dirty flags for EPT) must be 0 if
>> >     bit 21 of the IA32_VMX_EPT_VPID_CAP MSR (see Appendix A.10) is read
>> >     as 0, indicating that the processor does not support accessed and
>> >     dirty flags for EPT.
>> >   — Reserved bits 11:7 and 63:N (where N is the processor’s
>> >     physical-address width) must all be 0.
>> >
>> > And it looks like we need parts of nested_ept_init_mmu_context() to
>> > properly handle VMX_EPT_AD_ENABLE_BIT.
>> 
>> I completely ignored AD and the #VE sections. I will add a TODO item
>> in the comment section.
>
> AFAIK, we don't support #VE, but AD would be nice to handle from the
> beginning.  (I think that caling nested_ept_init_mmu_context() as-is
> isn't that bad.)

I went back to the spec to take a look at the AD handling. It doesn't look
like anything needs to be done since nested_ept_init_mmu_context() is already
being called with the correct eptp in prepare_vmcs02 ? Anything else that
needs to be done for AD handling in vmfunc context ?

Thanks,
Bandan

[toc] | [prev] | [next] | [standalone]

#1685283 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

On Tue, Jul 11, 2017 at 10:58 AM, Bandan Das <bsd@redhat.com> wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> 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
>
> I tend to follow the SDM names because it's easy to look for them.
>
>>> +             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.
>
> It's a good hint to see if L2 misbehaved and WARN_ON_ONCE makes sure it's
> not misused.
>
>>> +            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?
>
> I don't think there is. Since, we are basically emulating eptp switching
> for L2, this is a good check to have.

There is nothing wrong with a hypervisor using physical page 0 for
whatever purpose it likes, including an EPTP list.

>>> +
>>> +    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)
>
> If there's a triple fault, I think it's a good idea to inject it
> back. Basically, there's no need to take care of damage control
> that L1 is intentionally doing.
>
>>> +                    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.
>
> I think the reason is vmcs12->ept_pointer is never used directly. It's
> used to create a shadow table but nevertheless, the check should be there.
>
>>
>>> +            kunmap(page);
>>> +            nested_release_page_clean(page);
>>
>> shouldn't the kunmap + nested_release_page_clean go outside the if clause?
>
> :) Indeed, thanks for the catch.
>
> Bandan
>
>>> +    }
>>> +    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.

[toc] | [prev] | [next] | [standalone]

#1685286 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

Jim Mattson <jmattson@google.com> writes:
...
>>> 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?
>>
>> I don't think there is. Since, we are basically emulating eptp switching
>> for L2, this is a good check to have.
>
> There is nothing wrong with a hypervisor using physical page 0 for
> whatever purpose it likes, including an EPTP list.

Right, but of all the things, a l1 hypervisor wanting page 0 for a eptp list
address most likely means it forgot to initialize it. Whatever damage it does will
still end up with vmfunc vmexit anyway.

Bandan

[toc] | [prev] | [next] | [standalone]

#1685304 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

2017-07-11 14:35-0400, Bandan Das:
> Jim Mattson <jmattson@google.com> writes:
> ...
> >>> 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?
> >>
> >> I don't think there is. Since, we are basically emulating eptp switching
> >> for L2, this is a good check to have.
> >
> > There is nothing wrong with a hypervisor using physical page 0 for
> > whatever purpose it likes, including an EPTP list.
> 
> Right, but of all the things, a l1 hypervisor wanting page 0 for a eptp list
> address most likely means it forgot to initialize it. Whatever damage it does will
> still end up with vmfunc vmexit anyway.

Most likely, but not certainly.  I also don't see a to diverge from the
spec here.

[toc] | [prev] | [next] | [standalone]

#1685315 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

Radim Krčmář <rkrcmar@redhat.com> writes:

> 2017-07-11 14:35-0400, Bandan Das:
>> Jim Mattson <jmattson@google.com> writes:
>> ...
>> >>> 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?
>> >>
>> >> I don't think there is. Since, we are basically emulating eptp switching
>> >> for L2, this is a good check to have.
>> >
>> > There is nothing wrong with a hypervisor using physical page 0 for
>> > whatever purpose it likes, including an EPTP list.
>> 
>> Right, but of all the things, a l1 hypervisor wanting page 0 for a eptp list
>> address most likely means it forgot to initialize it. Whatever damage it does will
>> still end up with vmfunc vmexit anyway.
>
> Most likely, but not certainly.  I also don't see a to diverge from the
> spec here.

Actually, this is a specific case where I would like to diverge from the spec.
But then again, it's L1 shooting itself in the foot and this would be a rarely
used code path, so, I am fine removing it.

[toc] | [prev] | [next] | [standalone]

#1685328 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

2017-07-11 15:38-0400, Bandan Das:
> Radim Krčmář <rkrcmar@redhat.com> writes:
> 
> > 2017-07-11 14:35-0400, Bandan Das:
> >> Jim Mattson <jmattson@google.com> writes:
> >> ...
> >> >>> 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?
> >> >>
> >> >> I don't think there is. Since, we are basically emulating eptp switching
> >> >> for L2, this is a good check to have.
> >> >
> >> > There is nothing wrong with a hypervisor using physical page 0 for
> >> > whatever purpose it likes, including an EPTP list.
> >> 
> >> Right, but of all the things, a l1 hypervisor wanting page 0 for a eptp list
> >> address most likely means it forgot to initialize it. Whatever damage it does will
> >> still end up with vmfunc vmexit anyway.
> >
> > Most likely, but not certainly.  I also don't see a to diverge from the
> > spec here.
> 
> Actually, this is a specific case where I would like to diverge from the spec.
> But then again, it's L1 shooting itself in the foot and this would be a rarely
> used code path, so, I am fine removing it.

Thanks, we're not here to judge the guest, but to provide a bare-metal
experience. :)

[toc] | [prev] | [next] | [standalone]

#1685333 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

Radim Krčmář <rkrcmar@redhat.com> writes:

> 2017-07-11 15:38-0400, Bandan Das:
>> Radim Krčmář <rkrcmar@redhat.com> writes:
>> 
>> > 2017-07-11 14:35-0400, Bandan Das:
>> >> Jim Mattson <jmattson@google.com> writes:
>> >> ...
>> >> >>> 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?
>> >> >>
>> >> >> I don't think there is. Since, we are basically emulating eptp switching
>> >> >> for L2, this is a good check to have.
>> >> >
>> >> > There is nothing wrong with a hypervisor using physical page 0 for
>> >> > whatever purpose it likes, including an EPTP list.
>> >> 
>> >> Right, but of all the things, a l1 hypervisor wanting page 0 for a eptp list
>> >> address most likely means it forgot to initialize it. Whatever damage it does will
>> >> still end up with vmfunc vmexit anyway.
>> >
>> > Most likely, but not certainly.  I also don't see a to diverge from the
>> > spec here.
>> 
>> Actually, this is a specific case where I would like to diverge from the spec.
>> But then again, it's L1 shooting itself in the foot and this would be a rarely
>> used code path, so, I am fine removing it.
>
> Thanks, we're not here to judge the guest, but to provide a bare-metal
> experience. :)

There are certain cases where do. For example, when L2 instruction emulation
fails we decide to kill L2 instead of injecting the error to L1 and let it handle
that. Anyway, that's a different topic, I was just trying to point out there
are cases kvm does a somewhat policy decision...

[toc] | [prev] | [next] | [standalone]

#1685783 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

2017-07-11 16:45-0400, Bandan Das:
> Radim Krčmář <rkrcmar@redhat.com> writes:
> 
> > 2017-07-11 15:38-0400, Bandan Das:
> >> Radim Krčmář <rkrcmar@redhat.com> writes:
> >> 
> >> > 2017-07-11 14:35-0400, Bandan Das:
> >> >> Jim Mattson <jmattson@google.com> writes:
> >> >> ...
> >> >> >>> 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?
> >> >> >>
> >> >> >> I don't think there is. Since, we are basically emulating eptp switching
> >> >> >> for L2, this is a good check to have.
> >> >> >
> >> >> > There is nothing wrong with a hypervisor using physical page 0 for
> >> >> > whatever purpose it likes, including an EPTP list.
> >> >> 
> >> >> Right, but of all the things, a l1 hypervisor wanting page 0 for a eptp list
> >> >> address most likely means it forgot to initialize it. Whatever damage it does will
> >> >> still end up with vmfunc vmexit anyway.
> >> >
> >> > Most likely, but not certainly.  I also don't see a to diverge from the
> >> > spec here.
> >> 
> >> Actually, this is a specific case where I would like to diverge from the spec.
> >> But then again, it's L1 shooting itself in the foot and this would be a rarely
> >> used code path, so, I am fine removing it.
> >
> > Thanks, we're not here to judge the guest, but to provide a bare-metal
> > experience. :)
> 
> There are certain cases where do. For example, when L2 instruction emulation
> fails we decide to kill L2 instead of injecting the error to L1 and let it handle
> that. Anyway, that's a different topic, I was just trying to point out there
> are cases kvm does a somewhat policy decision...

Emulation failure is a KVM bug and we are too lazy to implement the
bare-metal behavior correctly, but avoiding the EPTP list bug is
actually easier than introducing it.  You can make KVM simpler and
improve bare-metal emulation at the same time.

[toc] | [prev] | [next] | [standalone]

#1685944 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

Radim Krčmář <rkrcmar@redhat.com> writes:
...
>> > Thanks, we're not here to judge the guest, but to provide a bare-metal
>> > experience. :)
>> 
>> There are certain cases where do. For example, when L2 instruction emulation
>> fails we decide to kill L2 instead of injecting the error to L1 and let it handle
>> that. Anyway, that's a different topic, I was just trying to point out there
>> are cases kvm does a somewhat policy decision...
>
> Emulation failure is a KVM bug and we are too lazy to implement the
> bare-metal behavior correctly, but avoiding the EPTP list bug is
> actually easier than introducing it.  You can make KVM simpler and
> improve bare-metal emulation at the same time.

We are just talking past each other here trying to impose point of views.
Checking for 0 makes KVM simpler. As I said before, a 0 list_address means
that the hypervisor forgot to initialize it. Feel free to show me examples
where the hypervisor does indeed use a 0 address for eptp list address or
anything vm specific. You disagreed and I am fine with it.

[toc] | [prev] | [next] | [standalone]

#1686691 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

>>> +	/*
>>> +	 * 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)
> 
> If there's a triple fault, I think it's a good idea to inject it
> back. Basically, there's no need to take care of damage control
> that L1 is intentionally doing.

I quickly rushed over the massive amount of comments. Sounds like you'll
be preparing a v5. Would be great if you could add some comments that
were the result of this discussion (for parts that are not that obvious
- triple faults) - thanks!

-- 

Thanks,

David

[toc] | [prev] | [next] | [standalone]

#1686782 — Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor

David Hildenbrand <david@redhat.com> writes:

>>>> +	/*
>>>> +	 * 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)
>> 
>> If there's a triple fault, I think it's a good idea to inject it
>> back. Basically, there's no need to take care of damage control
>> that L1 is intentionally doing.
>
> I quickly rushed over the massive amount of comments. Sounds like you'll
> be preparing a v5. Would be great if you could add some comments that
> were the result of this discussion (for parts that are not that obvious
> - triple faults) - thanks!

Will do. Basically, we agreed that we don't need to do anything with mmu_reload() faillures
because the invalid eptp that mmu_unload will write to root_hpa will result in an ept
violation.

Bandan

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web