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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
