| Message ID | 1408527905-11705-2-git-send-email-wanpeng.li@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 20/08/2014 11:45, Wanpeng Li ha scritto: > Introduce apic_access_and_virtual_page_valid() to check the valid > of nested apic access page and virtual apic page earlier. > > Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com> > --- > arch/x86/kvm/vmx.c | 82 ++++++++++++++++++++++++++++++------------------------ > 1 file changed, 46 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index caf239d..02bc07d 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -7838,6 +7838,50 @@ static void vmx_inject_page_fault_nested(struct kvm_vcpu *vcpu, > kvm_inject_page_fault(vcpu, fault); > } > > +static bool apic_access_and_virtual_page_valid(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + > + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) { > + if (!PAGE_ALIGNED(vmcs12->apic_access_addr)) > + /*TODO: Also verify bits beyond physical address width are 0*/ > + return false; > + > + /* > + * Translate L1 physical address to host physical > + * address for vmcs02. Keep the page pinned, so this > + * physical address remains valid. We keep a reference > + * to it so we can release it later. > + */ > + if (vmx->nested.apic_access_page) /* shouldn't happen */ > + nested_release_page(vmx->nested.apic_access_page); > + vmx->nested.apic_access_page = > + nested_get_page(vcpu, vmcs12->apic_access_addr); > + } > + > + if (nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) { > + if (vmx->nested.virtual_apic_page) /* shouldn't happen */ > + nested_release_page(vmx->nested.virtual_apic_page); > + vmx->nested.virtual_apic_page = > + nested_get_page(vcpu, vmcs12->virtual_apic_page_addr); > + > + /* > + * Failing the vm entry is _not_ what the processor does > + * but it's basically the only possibility we have. > + * We could still enter the guest if CR8 load exits are > + * enabled, CR8 store exits are enabled, and virtualize APIC > + * access is disabled; in this case the processor would never > + * use the TPR shadow and we could simply clear the bit from > + * the execution control. But such a configuration is useless, > + * so let's keep the code simple. > + */ > + if (!vmx->nested.virtual_apic_page) > + return false; > + } > + return true; > +} > + > static void vmx_start_preemption_timer(struct kvm_vcpu *vcpu) > { > u64 preemption_timeout = get_vmcs12(vcpu)->vmx_preemption_timer_value; > @@ -7984,16 +8028,6 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > > if (exec_control & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES) { > /* > - * Translate L1 physical address to host physical > - * address for vmcs02. Keep the page pinned, so this > - * physical address remains valid. We keep a reference > - * to it so we can release it later. > - */ > - if (vmx->nested.apic_access_page) /* shouldn't happen */ > - nested_release_page(vmx->nested.apic_access_page); > - vmx->nested.apic_access_page = > - nested_get_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 > @@ -8040,30 +8074,8 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > exec_control |= vmcs12->cpu_based_vm_exec_control; > > if (exec_control & CPU_BASED_TPR_SHADOW) { > - if (vmx->nested.virtual_apic_page) > - nested_release_page(vmx->nested.virtual_apic_page); > - vmx->nested.virtual_apic_page = > - nested_get_page(vcpu, vmcs12->virtual_apic_page_addr); > - if (!vmx->nested.virtual_apic_page) > - exec_control &= > - ~CPU_BASED_TPR_SHADOW; > - else > - vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, > + vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, > page_to_phys(vmx->nested.virtual_apic_page)); > - > - /* > - * Failing the vm entry is _not_ what the processor does > - * but it's basically the only possibility we have. > - * We could still enter the guest if CR8 load exits are > - * enabled, CR8 store exits are enabled, and virtualize APIC > - * access is disabled; in this case the processor would never > - * use the TPR shadow and we could simply clear the bit from > - * the execution control. But such a configuration is useless, > - * so let's keep the code simple. > - */ > - if (!vmx->nested.virtual_apic_page) > - nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD); > - > vmcs_write32(TPR_THRESHOLD, vmcs12->tpr_threshold); > } else if (vm_need_tpr_shadow(vmx->vcpu.kvm)) > vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, > @@ -8230,9 +8242,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) > return 1; > } > > - if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES) && > - !PAGE_ALIGNED(vmcs12->apic_access_addr)) { > - /*TODO: Also verify bits beyond physical address width are 0*/ > + if (!apic_access_and_virtual_page_valid(vcpu, vmcs12)) { > nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD); > return 1; > } > Thanks Wanpeng. The code now looks good. Just one thing: please swap them, so that the series remains bisectable. Also, I think nested_get_vmcs12_pages would be a better name for the function. apic_access_and_virtual_page_valid doesn't hint at the side effects of the function (for example calling nested_get_page). The way I swap patches is by using "git checkout -p" like this: git branch tpr-shadow-old git reset --hard HEAD^^ git checkout -p tpr-shadow-old ... pick hunks related to the second patch ... git commit -c tpr-shadow-old ... edit commit message if needed ... git checkout -p tpr-shadow-old ... pick hunks related to the first patch ... git commit -C tpr-shadow-old^ ... edit commit message if needed ... git diff tpr-shadow-old HEAD Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paolo, On Wed, Aug 20, 2014 at 12:50:38PM +0200, Paolo Bonzini wrote: >Il 20/08/2014 11:45, Wanpeng Li ha scritto: >> Introduce apic_access_and_virtual_page_valid() to check the valid >> of nested apic access page and virtual apic page earlier. >> >> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com> >> --- >> arch/x86/kvm/vmx.c | 82 ++++++++++++++++++++++++++++++------------------------ >> 1 file changed, 46 insertions(+), 36 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index caf239d..02bc07d 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -7838,6 +7838,50 @@ static void vmx_inject_page_fault_nested(struct kvm_vcpu *vcpu, >> kvm_inject_page_fault(vcpu, fault); >> } >> >> +static bool apic_access_and_virtual_page_valid(struct kvm_vcpu *vcpu, >> + struct vmcs12 *vmcs12) >> +{ >> + struct vcpu_vmx *vmx = to_vmx(vcpu); >> + >> + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) { >> + if (!PAGE_ALIGNED(vmcs12->apic_access_addr)) >> + /*TODO: Also verify bits beyond physical address width are 0*/ >> + return false; >> + >> + /* >> + * Translate L1 physical address to host physical >> + * address for vmcs02. Keep the page pinned, so this >> + * physical address remains valid. We keep a reference >> + * to it so we can release it later. >> + */ >> + if (vmx->nested.apic_access_page) /* shouldn't happen */ >> + nested_release_page(vmx->nested.apic_access_page); >> + vmx->nested.apic_access_page = >> + nested_get_page(vcpu, vmcs12->apic_access_addr); >> + } >> + >> + if (nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) { >> + if (vmx->nested.virtual_apic_page) /* shouldn't happen */ >> + nested_release_page(vmx->nested.virtual_apic_page); >> + vmx->nested.virtual_apic_page = >> + nested_get_page(vcpu, vmcs12->virtual_apic_page_addr); >> + >> + /* >> + * Failing the vm entry is _not_ what the processor does >> + * but it's basically the only possibility we have. >> + * We could still enter the guest if CR8 load exits are >> + * enabled, CR8 store exits are enabled, and virtualize APIC >> + * access is disabled; in this case the processor would never >> + * use the TPR shadow and we could simply clear the bit from >> + * the execution control. But such a configuration is useless, >> + * so let's keep the code simple. >> + */ >> + if (!vmx->nested.virtual_apic_page) >> + return false; >> + } >> + return true; >> +} >> + >> static void vmx_start_preemption_timer(struct kvm_vcpu *vcpu) >> { >> u64 preemption_timeout = get_vmcs12(vcpu)->vmx_preemption_timer_value; >> @@ -7984,16 +8028,6 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) >> >> if (exec_control & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES) { >> /* >> - * Translate L1 physical address to host physical >> - * address for vmcs02. Keep the page pinned, so this >> - * physical address remains valid. We keep a reference >> - * to it so we can release it later. >> - */ >> - if (vmx->nested.apic_access_page) /* shouldn't happen */ >> - nested_release_page(vmx->nested.apic_access_page); >> - vmx->nested.apic_access_page = >> - nested_get_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 >> @@ -8040,30 +8074,8 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) >> exec_control |= vmcs12->cpu_based_vm_exec_control; >> >> if (exec_control & CPU_BASED_TPR_SHADOW) { >> - if (vmx->nested.virtual_apic_page) >> - nested_release_page(vmx->nested.virtual_apic_page); >> - vmx->nested.virtual_apic_page = >> - nested_get_page(vcpu, vmcs12->virtual_apic_page_addr); >> - if (!vmx->nested.virtual_apic_page) >> - exec_control &= >> - ~CPU_BASED_TPR_SHADOW; >> - else >> - vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, >> + vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, >> page_to_phys(vmx->nested.virtual_apic_page)); >> - >> - /* >> - * Failing the vm entry is _not_ what the processor does >> - * but it's basically the only possibility we have. >> - * We could still enter the guest if CR8 load exits are >> - * enabled, CR8 store exits are enabled, and virtualize APIC >> - * access is disabled; in this case the processor would never >> - * use the TPR shadow and we could simply clear the bit from >> - * the execution control. But such a configuration is useless, >> - * so let's keep the code simple. >> - */ >> - if (!vmx->nested.virtual_apic_page) >> - nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD); >> - >> vmcs_write32(TPR_THRESHOLD, vmcs12->tpr_threshold); >> } else if (vm_need_tpr_shadow(vmx->vcpu.kvm)) >> vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, >> @@ -8230,9 +8242,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) >> return 1; >> } >> >> - if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES) && >> - !PAGE_ALIGNED(vmcs12->apic_access_addr)) { >> - /*TODO: Also verify bits beyond physical address width are 0*/ >> + if (!apic_access_and_virtual_page_valid(vcpu, vmcs12)) { >> nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD); >> return 1; >> } >> > >Thanks Wanpeng. The code now looks good. Just one thing: please swap >them, so that the series remains bisectable. > Do you mean the first patch introduce nested_get_vmcs12_pages() and the second patch implement nested TPR shadow/threshold emulation? >Also, I think nested_get_vmcs12_pages would be a better name for the >function. apic_access_and_virtual_page_valid doesn't hint at the side >effects of the function (for example calling nested_get_page). > Will do. Regards, Wanpeng Li >The way I swap patches is by using "git checkout -p" like this: > > git branch tpr-shadow-old > git reset --hard HEAD^^ > git checkout -p tpr-shadow-old > ... pick hunks related to the second patch ... > git commit -c tpr-shadow-old > ... edit commit message if needed ... > git checkout -p tpr-shadow-old > ... pick hunks related to the first patch ... > git commit -C tpr-shadow-old^ > ... edit commit message if needed ... > git diff tpr-shadow-old HEAD > >Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 21/08/2014 10:08, Wanpeng Li ha scritto: >> >Thanks Wanpeng. The code now looks good. Just one thing: please swap >> >them, so that the series remains bisectable. >> > > Do you mean the first patch introduce nested_get_vmcs12_pages() and the > second patch implement nested TPR shadow/threshold emulation? > Yes, exactly. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index caf239d..02bc07d 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -7838,6 +7838,50 @@ static void vmx_inject_page_fault_nested(struct kvm_vcpu *vcpu, kvm_inject_page_fault(vcpu, fault); } +static bool apic_access_and_virtual_page_valid(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + struct vcpu_vmx *vmx = to_vmx(vcpu); + + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) { + if (!PAGE_ALIGNED(vmcs12->apic_access_addr)) + /*TODO: Also verify bits beyond physical address width are 0*/ + return false; + + /* + * Translate L1 physical address to host physical + * address for vmcs02. Keep the page pinned, so this + * physical address remains valid. We keep a reference + * to it so we can release it later. + */ + if (vmx->nested.apic_access_page) /* shouldn't happen */ + nested_release_page(vmx->nested.apic_access_page); + vmx->nested.apic_access_page = + nested_get_page(vcpu, vmcs12->apic_access_addr); + } + + if (nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) { + if (vmx->nested.virtual_apic_page) /* shouldn't happen */ + nested_release_page(vmx->nested.virtual_apic_page); + vmx->nested.virtual_apic_page = + nested_get_page(vcpu, vmcs12->virtual_apic_page_addr); + + /* + * Failing the vm entry is _not_ what the processor does + * but it's basically the only possibility we have. + * We could still enter the guest if CR8 load exits are + * enabled, CR8 store exits are enabled, and virtualize APIC + * access is disabled; in this case the processor would never + * use the TPR shadow and we could simply clear the bit from + * the execution control. But such a configuration is useless, + * so let's keep the code simple. + */ + if (!vmx->nested.virtual_apic_page) + return false; + } + return true; +} + static void vmx_start_preemption_timer(struct kvm_vcpu *vcpu) { u64 preemption_timeout = get_vmcs12(vcpu)->vmx_preemption_timer_value; @@ -7984,16 +8028,6 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) if (exec_control & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES) { /* - * Translate L1 physical address to host physical - * address for vmcs02. Keep the page pinned, so this - * physical address remains valid. We keep a reference - * to it so we can release it later. - */ - if (vmx->nested.apic_access_page) /* shouldn't happen */ - nested_release_page(vmx->nested.apic_access_page); - vmx->nested.apic_access_page = - nested_get_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 @@ -8040,30 +8074,8 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) exec_control |= vmcs12->cpu_based_vm_exec_control; if (exec_control & CPU_BASED_TPR_SHADOW) { - if (vmx->nested.virtual_apic_page) - nested_release_page(vmx->nested.virtual_apic_page); - vmx->nested.virtual_apic_page = - nested_get_page(vcpu, vmcs12->virtual_apic_page_addr); - if (!vmx->nested.virtual_apic_page) - exec_control &= - ~CPU_BASED_TPR_SHADOW; - else - vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, + vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, page_to_phys(vmx->nested.virtual_apic_page)); - - /* - * Failing the vm entry is _not_ what the processor does - * but it's basically the only possibility we have. - * We could still enter the guest if CR8 load exits are - * enabled, CR8 store exits are enabled, and virtualize APIC - * access is disabled; in this case the processor would never - * use the TPR shadow and we could simply clear the bit from - * the execution control. But such a configuration is useless, - * so let's keep the code simple. - */ - if (!vmx->nested.virtual_apic_page) - nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD); - vmcs_write32(TPR_THRESHOLD, vmcs12->tpr_threshold); } else if (vm_need_tpr_shadow(vmx->vcpu.kvm)) vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, @@ -8230,9 +8242,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) return 1; } - if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES) && - !PAGE_ALIGNED(vmcs12->apic_access_addr)) { - /*TODO: Also verify bits beyond physical address width are 0*/ + if (!apic_access_and_virtual_page_valid(vcpu, vmcs12)) { nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD); return 1; }
Introduce apic_access_and_virtual_page_valid() to check the valid of nested apic access page and virtual apic page earlier. Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com> --- arch/x86/kvm/vmx.c | 82 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 36 deletions(-)