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"
> > > + 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_?
