On Mon, Oct 14, 2019 at 12:15 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> On Mon, Oct 14, 2019 at 11:50:37AM -0700, Jim Mattson wrote:
> > On Mon, Oct 14, 2019 at 10:59 AM Sean Christopherson
> > <sean.j.christopherson@xxxxxxxxx> wrote:
> > > > @@ -2947,19 +2947,18 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> > > > vmx->nested.apic_access_page = NULL;
> > > > }
> > > > page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->apic_access_addr);
> > > > - /*
> > > > - * If translation failed, no matter: This feature asks
> > > > - * to exit when accessing the given address, and if it
> > > > - * can never be accessed, this feature won't do
> > > > - * anything anyway.
> > > > - */
> > > > if (!is_error_page(page)) {
> > > > vmx->nested.apic_access_page = page;
> > > > hpa = page_to_phys(vmx->nested.apic_access_page);
> > > > vmcs_write64(APIC_ACCESS_ADDR, hpa);
> > > > } else {
> > > > - secondary_exec_controls_clearbit(vmx,
> > > > - SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES);
> > > > + pr_debug_ratelimited("%s: non-cacheable APIC-access address in vmcs12\n",
> > > > + __func__);
> > >
> > > Hmm, "non-cacheable" is confusing, especially in the context of the APIC,
> > > which needs to be mapped "uncacheable". Maybe just "invalid"?
> >
> > "Invalid" is not correct. L1 MMIO addresses are valid; they're just
> > not cacheable. Perhaps:
> >
> > "vmcs12 APIC-access address references a page not backed by a memslot in L1"?
>
> Hmm, technically is_error_page() isn't limited to a non-existent memslot,
> any GFN that doesn't lead to a 'struct page' will trigger is_error_page().
>
> Maybe just spit out what literally went wrong? E.g something like
>
> pr_debug_ratelimited("%s: no backing 'struct page' for APIC-access address in vmcs12\n"
Perfect!
> > > > + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> > > > + vcpu->run->internal.suberror =
> > > > + KVM_INTERNAL_ERROR_EMULATION;
> > > > + vcpu->run->internal.ndata = 0;
> > > > + return false;
> > > > }
> > > > }
> > > >
> > > > @@ -3004,6 +3003,7 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
> > > > exec_controls_setbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
> > > > else
> > > > exec_controls_clearbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
> > > > + return true;
> > > > }
> > > >
> > > > /*
> > > > @@ -3042,13 +3042,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
> > > > /*
> > > > * If from_vmentry is false, this is being called from state restore (either RSM
> > > > * or KVM_SET_NESTED_STATE). Otherwise it's called from vmlaunch/vmresume.
> > > > -+ *
> > > > -+ * Returns:
> > > > -+ * 0 - success, i.e. proceed with actual VMEnter
> > > > -+ * 1 - consistency check VMExit
> > > > -+ * -1 - consistency check VMFail
> > > > + *
> > > > + * Returns:
> > > > + * ENTER_VMX_SUCCESS: Successfully entered VMX non-root mode
> > >
> > > "Enter VMX" usually refers to VMXON, e.g. the title of VMXON in the SDM is
> > > "Enter VMX Operation".
> > >
> > > Maybe NVMX_ENTER_NON_ROOT_?
> >
> > How about NESTED_VMX_ENTER_NON_ROOT_MODE_STATUS_?
> >
> > > > + * ENTER_VMX_VMFAIL: Consistency check VMFail
> > > > + * ENTER_VMX_VMEXIT: Consistency check VMExit
> > > > + * ENTER_VMX_ERROR: KVM internal error
> > >
> > > Probably need to more explicit than VMX_ERROR, e.g. all of the VM-Fail
> > > defines are prefixed with VMXERR_##.
> > >
> > > May ENTER_VMX_KVM_ERROR? (Or NVMX_ENTER_NON_ROOT_KVM_ERROR).
> >
> > NESTED_VMX_ENTER_NON_ROOT_MODE_STATUS_KVM_INTERNAL_ERROR?
>
> I can't tell if you're making fun of me for being pedantic about "Enter VMX",
> or if you really want to have a 57 character enum. :-)
>
> NESTED_VMENTER_?
It's difficult to balance brevity and clarity. I have no problem with
57 character enums, but I understand that Linux line-wrapping
conventions are designed for the VT100, so long enums present a
challenge. :-)
How about:
NVMX_VMENTRY_SUCCESS
NVMX_VMENTRY_VMFAIL
NVMX_VMENTRY_VMEXIT
NVMX_VMENTRY_INTERNAL_ERROR
